Code

Opened 17 months ago

Closed 2 months 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

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

Attachments (0)

Change History (18)

comment:1 Changed 17 months ago by akaariai

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

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 17 months ago by akaariai (previous) (diff)

comment:2 Changed 17 months ago by tunixman

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 Changed 17 months ago by tunixman

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 17 months ago by tunixman

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 17 months ago by tunixman

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 Changed 7 months ago by timo

  • Patch needs improvement set

Left comments for improvement on the PR.

comment:8 Changed 7 months ago by timo

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

comment:9 Changed 6 months ago by 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.

comment:10 Changed 6 months ago by shai

  • Keywords oracle added

comment:11 Changed 6 months ago by albertyw@…

  • Cc albertyw@… added

comment:12 Changed 5 months ago by albertyw@…

  • Version changed from 1.4 to master

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

comment:13 Changed 5 months ago by shai

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

comment:14 Changed 5 months ago by albertyw@…

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

comment:15 Changed 4 months ago by albertyw@…

ping?

comment:16 Changed 2 months ago by timo

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 2 months ago by albertyw@…

  • Patch needs improvement unset

Made patch pep8.

comment:18 Changed 2 months ago by Tim Graham <timograham@…>

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

In 8bbdcc76e4a84cde92b8dbfd01581d098bd2187d:

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

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

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.