/django

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 1 commit into
from

Conversation

Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
5 participants
@albertyw @timgraham @bmispelon @charettes @shaib
@albertyw
Contributor

albertyw commented Sep 29, 2013

This pull request fixes https://code.djangoproject.com/ticket/19299 (and https://code.djangoproject.com/ticket/21194)

@charettes charettes and 1 other commented on an outdated diff Sep 30, 2013

django/db/models/fields/related.py
@@ -1257,7 +1258,9 @@ def get_default(self):
return field_default
def get_db_prep_save(self, value, connection):
- if value == '' or value is None:
+ if value is None:
+ return None
+ elif value == '' and not isinstance(self.related_field, CharField):
@charettes

charettes Sep 30, 2013

Member

I think you should refer to the empty_strings_allowed property of the related field instead of special casing CharField here.

if value is None or (value == '' and not related_field.empty_strings_allowed)

Not sure an extra connection.features.interprets_empty_strings_as_nulls check is also required here since both values will be interpreted as NULL anyway.

@albertyw

albertyw Oct 1, 2013

Contributor

I changed it to your suggestion (using self.related_field instead of just related_field) in 61f5df9

@bmispelon bmispelon commented on an outdated diff Oct 2, 2013

tests/model_fields/models.py
class FksToBooleans(models.Model):
"""Model wih FKs to models with {Null,}BooleanField's, #15040"""
bf = models.ForeignKey(BooleanModel)
nbf = models.ForeignKey(NullBooleanModel)
+class FkToChar(models.Model):
+ """Model with FK to models with {""} #21194"""
@bmispelon

bmispelon Oct 2, 2013

Member

Nice touch to follow the previous model's docstring, but I think this one should simply be:
Model with FK to a model with a CharField primarey key, #21194

The {Null,}BooleanField of the previous one just means NullBooleanField and BooleanField.

@bmispelon bmispelon commented on an outdated diff Oct 2, 2013

tests/model_fields/models.py
@@ -62,11 +62,18 @@ class BooleanModel(models.Model):
bfield = models.BooleanField(default=None)
string = models.CharField(max_length=10, default='abc')
+class CharModel(models.Model):
@bmispelon

bmispelon Oct 2, 2013

Member

Nitpicky but I'd call it PrimaryKeyCharmodel, just to be more explicit about what it is.

@bmispelon bmispelon commented on an outdated diff Oct 2, 2013

tests/model_fields/models.py
@@ -62,11 +62,18 @@ class BooleanModel(models.Model):
bfield = models.BooleanField(default=None)
string = models.CharField(max_length=10, default='abc')
+class CharModel(models.Model):
+ string = models.CharField(max_length = 10, primary_key=True)
@bmispelon

bmispelon Oct 2, 2013

Member

Per pep8, this should be max_length=10 (note the lack of whitespace around the equal sign).

@bmispelon bmispelon commented on an outdated diff Oct 2, 2013

tests/model_fields/tests.py
@@ -140,6 +140,18 @@ def test_callable_default(self):
b = Bar.objects.create(b="bcd")
self.assertEqual(b.a, a)
+ def test_get_db_prep_save(self):
@bmispelon

bmispelon Oct 2, 2013

Member

Not sure if that's the best name for this test. How about test_empty_string_fk?

@bmispelon bmispelon commented on an outdated diff Oct 2, 2013

tests/model_fields/tests.py
@@ -140,6 +140,18 @@ def test_callable_default(self):
b = Bar.objects.create(b="bcd")
self.assertEqual(b.a, a)
+ def test_get_db_prep_save(self):
+ """
+ Test that foreign key values to empty strings don't get converted
+ to None
@bmispelon

bmispelon Oct 2, 2013

Member

Make sure you reference your ticket number in the docstring (see the other tests in the file).

@bmispelon bmispelon and 1 other commented on an outdated diff Oct 2, 2013

tests/model_fields/tests.py
@@ -140,6 +140,18 @@ def test_callable_default(self):
b = Bar.objects.create(b="bcd")
self.assertEqual(b.a, a)
+ def test_get_db_prep_save(self):
+ """
+ Test that foreign key values to empty strings don't get converted
+ to None
+ """
+ char_model_normal = CharModel.objects.create(string='asdf')
+ fk_model_normal = FkToChar.objects.create(out=char_model_normal)
@bmispelon

bmispelon Oct 2, 2013

Member

I don't think the two lines (*_normal) are needed here since this functionality is most likely tested elsewhere (and you don't make use of those variables in your assertions later on).

@albertyw

albertyw Oct 2, 2013

Contributor

I did look for tests that used the get_db_prep_save method, but I didn't find any. Agreed, I should actually write an assertion for the normal case, assuming it isn't covered somewhere else.

@albertyw

albertyw Oct 2, 2013

Contributor

I just removed the normal case lines because I'm guessing it is covered somewhere else and I just didn't see it. If it isn't covered, it would be outside the scope of this pull request/ticket 21194

@bmispelon bmispelon and 1 other commented on an outdated diff Oct 2, 2013

tests/model_fields/tests.py
@@ -140,6 +140,18 @@ def test_callable_default(self):
b = Bar.objects.create(b="bcd")
self.assertEqual(b.a, a)
+ def test_get_db_prep_save(self):
+ """
+ Test that foreign key values to empty strings don't get converted
+ to None
+ """
+ char_model_normal = CharModel.objects.create(string='asdf')
+ fk_model_normal = FkToChar.objects.create(out=char_model_normal)
+ char_model_empty = CharModel.objects.create(string='')
+ fk_model_empty = FkToChar.objects.create(out=char_model_empty)
@bmispelon

bmispelon Oct 2, 2013

Member

You don't need to assign the result to a variable here.

@albertyw

albertyw Oct 2, 2013

Contributor

I do need the pk of the newly created FkToChar model to lookup (and reload) the same object again on the next line.

@shaib shaib commented on an outdated diff Nov 25, 2013

tests/model_fields/tests.py
@@ -141,6 +141,15 @@ def test_callable_default(self):
b = Bar.objects.create(b="bcd")
self.assertEqual(b.a, a)
+ def test_empty_string_fk(self):
@shaib

shaib Nov 25, 2013

Member

This test needs to be made to pass or skip when the connection interprets_empty_strings_as_null -- skipIfDBFeature is your friend
(on Oracle, line 149 will raise an IntegrityError).

@shaib shaib commented on an outdated diff Nov 25, 2013

tests/model_fields/models.py
class FksToBooleans(models.Model):
"""Model wih FKs to models with {Null,}BooleanField's, #15040"""
bf = models.ForeignKey(BooleanModel)
nbf = models.ForeignKey(NullBooleanModel)
+class FkToChar(models.Model):
+ """Model with FK to a model with a CharField primary key, #21194"""
@shaib

shaib Nov 25, 2013

Member

Since 21194 was closed as a dupe, you may want to update the references to 19299.

@timgraham timgraham referenced this pull request Feb 8, 2014

Closed

Don't automatically collapse `''` and `None` for keys. #1197

@timgraham timgraham commented on an outdated diff Feb 8, 2014

django/db/models/query.py
# object must be non-existent - set the relation to None.
- if fields[pk_idx] is None or fields[pk_idx] == '':
+ if fields[pk_idx] is None or \
@timgraham

timgraham Feb 8, 2014

Owner

Prefer wrapping the expression in parentheses rather than using a backslash

@timgraham timgraham commented on an outdated diff Feb 8, 2014

django/db/models/fields/related.py
@@ -1320,7 +1321,7 @@ def get_default(self):
return field_default
def get_db_prep_save(self, value, connection):
- if value == '' or value is None:
+ if value is None or (value == '' and (not self.related_field.empty_strings_allowed or connection.features.interprets_empty_strings_as_nulls)):
@timgraham

timgraham Feb 8, 2014

Owner

break this into multiple lines?
@timgraham
Owner

timgraham commented Feb 8, 2014

Could you please rebase the patch to apply cleanly?

@timgraham timgraham commented on an outdated diff Feb 13, 2014

django/db/models/fields/related.py
@@ -7,7 +7,8 @@
from django.db.models import signals, Q
from django.db.models.deletion import SET_NULL, SET_DEFAULT, CASCADE
from django.db.models.fields import (AutoField, Field, IntegerField,
- PositiveIntegerField, PositiveSmallIntegerField, FieldDoesNotExist)
+ PositiveIntegerField, PositiveSmallIntegerField, FieldDoesNotExist,
+ CharField)
@timgraham

timgraham Feb 13, 2014

Owner

unused import (please check code with flake8 as there are some "expected 2 blank lines" warnings as well).

@timgraham timgraham commented on the diff Feb 13, 2014

View full changes
django/db/models/query.py
# object must be non-existent - set the relation to None.
- if fields[pk_idx] is None or fields[pk_idx] == '':
+ if (fields[pk_idx] is None or
@timgraham

timgraham Feb 13, 2014

Owner

Could you add a test for this change? If I revert it I don't get any test failures.

@albertyw

albertyw Feb 14, 2014

Contributor

There is a test for it. See https://github.com/django/django/pull/1690/files#diff-e4aa8dc49d7522ae55a932ffc7d09c1cR149

Are you running your tests only on databases that interpret empty strings as null values?

@timgraham

timgraham Feb 14, 2014

Owner

I'm trying on SQLite and the new test isn't skipped. If I revert the changes in django/db/models/fields/related.py it fails, but if I revert the change in this file, it still passes.

@albertyw

albertyw Feb 15, 2014

Contributor

I see what you mean. I changed tests/model_fields/tests.py:157 so that it would raise an error.
@timgraham
Owner

timgraham commented Feb 15, 2014

Great, merged in 8bbdcc7, thank-you.

@timgraham timgraham closed this Feb 15, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment