Opened 12 years ago

Closed 11 years ago

#19299 closed Bug (fixed)

Foreign keys using to_field and having legitimate empty string values fail with NULL errors

Reported by: evan@… Owned by: nobody
Component: Database layer (models, ORM) Version: dev
Severity: Normal Keywords: oracle
Cc: albertyw@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

If one table references another table with a foreign
key using a to_field that's a string, and the target column has an empty
string as a possible value, the ORM would remap it to None, and thus
NULL, which is not the same, and which breaks the relation.

Minimal test case and fix

Change History (18)

comment:1 by Anssi Kääriäinen, 12 years ago

Triage Stage: UnreviewedAccepted

The problem is that Oracle does treat and None equivalently. And, this shows in some places of Django.

The fix is likely incorrect as it doesn't check for connection.features.interprets_empty_strings_as_nulls.

I am marking this as accepted. Haven't tested this but the description seems correct.

Version 0, edited 12 years ago by Anssi Kääriäinen (next)

comment:2 by Evan Cofsky, 12 years ago

I think if Oracle treats them the same way, it will regardless of which we return, so maybe we can just return the correct value regardless. The databases that make the distinction will, and the ones that don't won't, but we'll have the right value coming out of this function regardless. It may also be something we don't actually want to check in other places either for the same reasons: Oracle collapses both values to NULL, other databases make a distinction.

I'm happy to look for other places this check might happen though, and if adding a check to connection.features.interprets_empty_strings_as_nulls is actually necessary add that as well.

Thanks!

comment:3 by Evan Cofsky, 12 years ago

I'm reworking the patch now to include another case of collapsing '' to None regardless of the interprets_empty_strings_as_nulls feature now, and I'll post an update momentarily.

comment:4 by Evan Cofsky, 12 years ago

I also do believe in the get_db_prep_save case we will need to check since we're not just returning the value in the case of NULL, but also chaining on some additional processing. I've updated the code locally and I'm rerunning the tests for it now.

comment:5 by Evan Cofsky, 12 years ago

And here's the new repo and branch I've cloned for this ticket, including all of the changes I've made so far: https://github.com/tunixman/django/commit/386ef64784a90aafbd8eace51d512dee886030a4

comment:7 by Tim Graham, 11 years ago

Patch needs improvement: set

Left comments for improvement on the PR.

comment:8 by Tim Graham, 11 years ago

Closed #21194 as a duplicate of this - the patch there is slightly different and may be useful.

comment:9 by albertyw@…, 11 years ago

From ticket #21194, I have another pull request there that is based on a more up to date version of django. https://github.com/django/django/pull/1690

The big difference that I've noticed is that in your patch, you're checking connection.features.interprets_empty_strings_as_nulls whereas I'm checking for not self.related_field.empty_strings_allowed. From my interpretation of the rest of the code, the former checks whether the database lumps empty string and None together and changes the query to None if True, whereas the latter checks whether the foreign keyed field accepts empty string and keeps the query at empty string if True.

They're kind of different problems, but I've added in Tunixman's code into my pull request so that it's more complete.

comment:10 by Shai Berger, 11 years ago

Keywords: oracle added

comment:11 by albertyw@…, 11 years ago

Cc: albertyw@… added

comment:12 by albertyw@…, 11 years ago

Version: 1.4master

ping on this? sorry for nagging. I'd like to close this issue.

comment:13 by Shai Berger, 11 years ago

Took a look at the patch, left comments. Appears to me to be almost ready.

comment:14 by albertyw@…, 11 years ago

I fixed the two things that you commented about (interprets_empty_strings_as_nulls and ticket id)

comment:15 by albertyw@…, 11 years ago

ping?

comment:16 by Tim Graham, 11 years ago

Be sure to uncheck "Patch needs improvement" so the patch appears in the review queue. I've left some minor comments and will take another look after your update, thanks.

comment:17 by albertyw@…, 11 years ago

Patch needs improvement: unset

Made patch pep8.

comment:18 by Tim Graham <timograham@…>, 11 years ago

Resolution: fixed
Status: newclosed

In 8bbdcc76e4a84cde92b8dbfd01581d098bd2187d:

Fixed #19299 -- Fixed Nullification of Foreign Keys To CharFields

Thanks tunixman for the report and Baptiste Mispelon and
Shai Berger for reviews.

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