Opened 15 years ago

Closed 15 years ago

Last modified 12 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: no UI/UX: no

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 Bob Thomas 15 years ago.
Fix regression from 7778, set FK to null
9308.diff (1.8 KB ) - added by Bob Thomas 15 years ago.
Fix tests
9308-b.diff (1.8 KB ) - added by markshep 15 years ago.
use subclassing instead of monkey patching

Download all attachments as: .zip

Change History (26)

comment:1 by Bob Thomas, 15 years ago

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

comment:2 by anonymous, 15 years ago

Cc: bthomas@… added

by Bob Thomas, 15 years ago

Attachment: fix_delete.diff added

Fix regression from 7778, set FK to null

comment:3 by Bob Thomas, 15 years ago

Has patch: set

comment:4 by Bob Thomas, 15 years ago

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).

in reply to:  4 comment:5 by Peter Sagerson, 15 years ago

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 by Jacob, 15 years ago

milestone: 1.1
Triage Stage: UnreviewedAccepted

comment:7 by hey560@…, 15 years ago

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

comment:8 by Bob Thomas, 15 years ago

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

comment:9 by liangent, 15 years ago

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 by liangent, 15 years ago

Cc: liangent@… added

also, i cannot understand why do we need this filter

comment:11 by Alex Gaynor, 15 years ago

Needs tests: set

in reply to:  9 comment:12 by Bob Thomas, 15 years ago

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

by Bob Thomas, 15 years ago

Attachment: 9308.diff added

Fix tests

comment:13 by Bob Thomas, 15 years ago

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 by liangent, 15 years ago

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

in reply to:  14 comment:15 by Bob Thomas, 15 years ago

Replying to liangent:

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

It was added to fix #7778

comment:16 by anonymous, 15 years ago

Cc: jdunck@… added

comment:17 by Thomas Güttler, 15 years ago

Cc: hv@… added

comment:18 by Jacob, 15 years ago

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

by markshep, 15 years ago

Attachment: 9308-b.diff added

use subclassing instead of monkey patching

comment:19 by Russell Keith-Magee, 15 years ago

Resolution: fixed
Status: newclosed

(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 by Russell Keith-Magee, 15 years ago

(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 by anonymous, 13 years ago

Cc: hv@… removed

comment:23 by Jacob, 12 years ago

milestone: 1.1

Milestone 1.1 deleted

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