Opened 7 years ago

Closed 5 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: master
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


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 Changed 7 years ago by Anssi Kääriäinen

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.

Last edited 7 years ago by Anssi Kääriäinen (previous) (diff)

comment:2 Changed 7 years ago by Evan Cofsky

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.


comment:3 Changed 7 years ago by Evan Cofsky

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 Changed 7 years ago by Evan Cofsky

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 Changed 7 years ago by Evan Cofsky

And here's the new repo and branch I've cloned for this ticket, including all of the changes I've made so far:

comment:7 Changed 6 years ago by Tim Graham

Patch needs improvement: set

Left comments for improvement on the PR.

comment:8 Changed 6 years ago by Tim Graham

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

comment:9 Changed 6 years ago by albertyw@…

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

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 Changed 6 years ago by Shai Berger

Keywords: oracle added

comment:11 Changed 6 years ago by albertyw@…

Cc: albertyw@… added

comment:12 Changed 5 years ago by albertyw@…

Version: 1.4master

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

comment:13 Changed 5 years ago by Shai Berger

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

comment:14 Changed 5 years ago by albertyw@…

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

comment:15 Changed 5 years ago by albertyw@…


comment:16 Changed 5 years ago by Tim Graham

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 Changed 5 years ago by albertyw@…

Patch needs improvement: unset

Made patch pep8.

comment:18 Changed 5 years ago by Tim Graham <timograham@…>

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