Opened 7 years ago

Closed 6 years ago

Last modified 4 years ago

#9308 closed (fixed)

Django fails to set nullable foreign key field to NULL before deleting the referenced record

Reported by: egenix_viktor Owned by: nobody
Component: Database layer (models, ORM) Version: 1.0
Severity: Keywords: delete record foreign key constraint check NULL nullable field
Cc: bthomas@…, liangent@…, jdunck@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

Unit test: modeltests/delete

Test case:

# Since E.f is nullable, we should delete F first (after nulling out
# the E.f field), then E.

>>> o = CollectedObjects()
>>> e1._collect_sub_objects(o)
>>> o.keys()
[<class 'modeltests.delete.models.F'>, <class 'modeltests.delete.models.E'>]

>>> e1.delete()

It should work as described in the associated comment, since f_id is a nullable column of the delete_e table:

CREATE TABLE [delete_e] (
    [id] int IDENTITY (1, 1) NOT NULL PRIMARY KEY,
    [f_id] int NULL
)

Django should set the f_id field in the E record to NULL before trying to delete the F record, but it does not do so. Instead it tries to delete the F record first, which fails the foreign key constraint check associated to the f_id field of the delete_e table (E record).

It does not work on databases where deferred constrain checking (checking constraints only COMMIT time) is not an option. For example I found this behavior while running the unit tests through pyodbc (third party, I know) and MSSQL. (Relying on such deferred constraint checking is not a good practice at all.)

Attachments (3)

fix_delete.diff (718 bytes) - added by bthomas 7 years ago.
Fix regression from 7778, set FK to null
9308.diff (1.8 KB) - added by bthomas 6 years ago.
Fix tests
9308-b.diff (1.8 KB) - added by markshep 6 years ago.
use subclassing instead of monkey patching

Download all attachments as: .zip

Change History (26)

comment:1 Changed 7 years ago by bthomas

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

This is essentially the problem that caused me to re-open ticket #7778. The patch I attached should resolve this.

comment:2 Changed 7 years ago by anonymous

  • Cc bthomas@… added

Changed 7 years ago by bthomas

Fix regression from 7778, set FK to null

comment:3 Changed 7 years ago by bthomas

  • Has patch set

comment:4 follow-up: Changed 7 years ago by bthomas

Django used to do this properly, but the change from #7778 broke this unintentionally. I re-opened #7778 with this fix, but your ticket describes the new problem better. Attached my original patch (with the line numbers slightly offset to match 1.0.x branch).

comment:5 in reply to: ↑ 4 Changed 7 years ago by psagers

In case it's not clear, modeltests/delete currently fails under MySQL 5.0.67, not just pyodbc and MSSQL. This is pretty close to a deal-breaker for MySQL support.

comment:6 Changed 6 years ago by jacob

  • milestone set to 1.1
  • Triage Stage changed from Unreviewed to Accepted

comment:7 Changed 6 years ago by hey560@…

I tried the patch on trunk with sqlite and it doesn't seem to work.

comment:8 Changed 6 years ago by bthomas

I'm sorry to hear that, but some more information would be useful. What doesn't work?

comment:9 follow-up: Changed 6 years ago by liangent

in the patch, lambda f: f.column == field.rel.get_related_field().name, why can we expect that a field's name can equals to a field's column name?

comment:10 Changed 6 years ago by liangent

  • Cc liangent@… added

also, i cannot understand why do we need this filter

comment:11 Changed 6 years ago by Alex

  • Needs tests set

comment:12 in reply to: ↑ 9 Changed 6 years ago by bthomas

Replying to liangent:

in the patch, lambda f: f.column == field.rel.get_related_field().name, why can we expect that a field's name can equals to a field's column name?

You're right, it should be lambda f: f.column == field.rel.get_related_field().column, of course

Changed 6 years ago by bthomas

Fix tests

comment:13 Changed 6 years ago by bthomas

  • Needs tests unset

I updated the test case mentioned in this bug's description to actually verify that E.f is set to null. It fails before the patch and passes afterwords, regardless of backend or deferred constraint checking.

comment:14 follow-up: Changed 6 years ago by liangent

i still have the question that why do we need this filter?

comment:15 in reply to: ↑ 14 Changed 6 years ago by bthomas

Replying to liangent:

i still have the question that why do we need this filter?

It was added to fix #7778

comment:16 Changed 6 years ago by anonymous

  • Cc jdunck@… added

comment:17 Changed 6 years ago by guettli

  • Cc hv@… added

comment:18 Changed 6 years ago by jacob

The test case is *nasty* -- maybe subclass UpdateQuery?

Changed 6 years ago by markshep

use subclassing instead of monkey patching

comment:19 Changed 6 years ago by russellm

  • Resolution set to fixed
  • Status changed from new to closed

(In [10822]) Fixed #9308 -- Corrected the updated of nullable foreign key fields when deleting objects. Thanks to Bob Thomas for the fix, and markshep for the improvements on the test case.

comment:20 Changed 6 years ago by russellm

(In [10823]) [1.0.X] Fixed #9308 -- Corrected the updated of nullable foreign key fields when deleting objects. Thanks to Bob Thomas for the fix, and markshep for the improvements on the test case.

Merge of r10822 from trunk.

comment:22 Changed 5 years ago by anonymous

  • Cc hv@… removed

comment:23 Changed 4 years ago by jacob

  • milestone 1.1 deleted

Milestone 1.1 deleted

Note: See TracTickets for help on using tickets.
Back to Top