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

[evan@…]

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

[Anssi Kääriäinen]

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.

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

Thanks!

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

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

[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: ​https://github.com/tunixman/django/commit/386ef64784a90aafbd8eace51d512dee886030a4

[Tim Graham]

​https://github.com/django/django/pull/1197

[Tim Graham]

Left comments for improvement on the PR.

[Tim Graham]

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

[albertyw@…]

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.

[albertyw@…]

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

[Shai Berger]

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

[albertyw@…]

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

[albertyw@…]

ping?

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

[albertyw@…]

Made patch pep8.

[Tim Graham <timograham@…>]

In 8bbdcc76e4a84cde92b8dbfd01581d098bd2187d:

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

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