Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

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

Closed
wants to merge 2 commits into from

1 participant

@ghost

Ticket #19299 which I forgot to send this for.

Many databases treat these differently, including MySQL, and there are
legitimate cases where '' is a valid value in key fields. This change
updates both the ForeignKey class and the row cache to check the
connection feature interprets_empty_strings_as_nulls and will collapse
these two values if that's True, otherwise it will treat them
differently.

Evan Cofsky added some commits
Evan Cofsky Don't automatically collapse `''` and `None` for keys.
Many databases treat these differently, including MySQL, and there are
legitimate cases where `''` is a valid value in key fields. This change
updates both the ForeignKey class and the row cache to check the
connection feature `interprets_empty_strings_as_nulls` and will collapse
these two values if that's `True`, otherwise it will treat them
differently.
386ef64
Evan Cofsky Merged with current Django.
832e1d8
@timgraham timgraham commented on the diff
django/db/models/fields/related.py
@@ -1194,7 +1194,9 @@ def get_default(self):
return field_default
def get_db_prep_save(self, value, connection):
- if value == '' or value == None:
+ if (value == None or
@timgraham Owner

I'd drop the outer parens and do:

if value is None or (value == '' and connection.features.interprets_empty_strings_as_nulls):

This modification also needs a test as the test you've added passes without it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@timgraham timgraham commented on the diff
django/db/models/query.py
((7 lines not shown))
# object must be non-existent - set the relation to None.
- if fields[pk_idx] == None or fields[pk_idx] == '':
+ if (fields[pk_idx] == None or
@timgraham Owner

same style as above -- you'll need to rebase and resolve some conflicts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@timgraham timgraham commented on the diff
tests/model_forms/models.py
@@ -241,6 +241,14 @@ class FlexibleDatePost(models.Model):
subtitle = models.CharField(max_length=50, unique_for_month='posted', blank=True)
posted = models.DateField(blank=True, null=True)
+
+class EmptyKeySource(models.Model):
@timgraham Owner

model_forms isn't the correct test package to put this in. maybe 'queries'? Try searching for "to_field" in the the tests to get an idea of where else that's tested. If it's possible to reuse an existing model, that would be ideal. Otherwise, try to slim down the model fields to the minimal amount needed for testing -- for example, I don't think "db_index=True" is needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@timgraham timgraham commented on the diff
tests/model_forms/tests.py
@@ -1666,6 +1666,15 @@ def test_model_field_that_returns_none_to_exclude_itself_with_explicit_fields(se
self.assertHTMLEqual(six.text_type(CustomFieldForExclusionForm()),
'''<tr><th><label for="id_name">Name:</label></th><td><input id="id_name" type="text" name="name" maxlength="10" /></td></tr>''')
+ def test_foreignkeys_which_use_to_field_and_blank_string_key(self):
+ source_n = EmptyKeySource.objects.create(name="")
+ source_a = EmptyKeySource.objects.create(name="a")
@timgraham Owner

souce_a / source_b don't appear necessary for the test.

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

I'll resurrect this and take a look at it over the weekend. I know there was a lot of merging since the change originally was for an old release, so I'll find out why the tests pass now even without the changes.

Thanks :)

@timgraham
Owner

Duplicate of #1690

@timgraham timgraham closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Nov 16, 2012
  1. Don't automatically collapse `''` and `None` for keys.

    Evan Cofsky authored
    Many databases treat these differently, including MySQL, and there are
    legitimate cases where `''` is a valid value in key fields. This change
    updates both the ForeignKey class and the row cache to check the
    connection feature `interprets_empty_strings_as_nulls` and will collapse
    these two values if that's `True`, otherwise it will treat them
    differently.
Commits on May 22, 2013
  1. Merged with current Django.

    Evan Cofsky authored
This page is out of date. Refresh to see the latest.
View
4 django/db/models/fields/related.py
@@ -1194,7 +1194,9 @@ def get_default(self):
return field_default
def get_db_prep_save(self, value, connection):
- if value == '' or value == None:
+ if (value == None or
@timgraham Owner

I'd drop the outer parens and do:

if value is None or (value == '' and connection.features.interprets_empty_strings_as_nulls):

This modification also needs a test as the test you've added passes without it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ (connection.features.interprets_empty_strings_as_nulls
+ and value == '')):
return None
else:
return self.related_field.get_db_prep_save(value,
View
7 django/db/models/query.py
@@ -1288,9 +1288,12 @@ def get_cached_row(row, index_start, using, klass_info, offset=0,
fields = row[index_start : index_start + field_count]
- # If the pk column is None (or the Oracle equivalent ''), then the related
+ # If the pk column is None (or the equivalent '' in the case the
+ # connection interprets empty strings as nulls), then the related
# object must be non-existent - set the relation to None.
- if fields[pk_idx] == None or fields[pk_idx] == '':
+ if (fields[pk_idx] == None or
@timgraham Owner

same style as above -- you'll need to rebase and resolve some conflicts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ (connections[using].features.interprets_empty_strings_as_nulls
+ and fields[pk_idx] == '')):
obj = None
elif field_names:
fields = list(fields)
View
8 tests/model_forms/models.py
@@ -241,6 +241,14 @@ class FlexibleDatePost(models.Model):
subtitle = models.CharField(max_length=50, unique_for_month='posted', blank=True)
posted = models.DateField(blank=True, null=True)
+
+class EmptyKeySource(models.Model):
@timgraham Owner

model_forms isn't the correct test package to put this in. maybe 'queries'? Try searching for "to_field" in the the tests to get an idea of where else that's tested. If it's possible to reuse an existing model, that would be ideal. Otherwise, try to slim down the model fields to the minimal amount needed for testing -- for example, I don't think "db_index=True" is needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ name = models.CharField(max_length=50, unique=True, null=False, blank=True, db_index=True)
+
+class EmptyKeySink(models.Model):
+ title = models.CharField(max_length=50, unique=True, null=False, blank=True, db_index=True)
+ source = models.ForeignKey(EmptyKeySource, to_field='name', blank=True, null=False)
+
@python_2_unicode_compatible
class Colour(models.Model):
name = models.CharField(max_length=50)
View
11 tests/model_forms/tests.py
@@ -24,7 +24,7 @@
DerivedPost, ExplicitPK, FlexibleDatePost, ImprovedArticle,
ImprovedArticleWithParentLink, Inventory, Post, Price,
Product, TextFile, AuthorProfile, Colour, ColourfulItem,
- test_images)
+ EmptyKeySource, EmptyKeySink, test_images)
if test_images:
from .models import ImageFile, OptionalImageFile
@@ -1666,6 +1666,15 @@ def test_model_field_that_returns_none_to_exclude_itself_with_explicit_fields(se
self.assertHTMLEqual(six.text_type(CustomFieldForExclusionForm()),
'''<tr><th><label for="id_name">Name:</label></th><td><input id="id_name" type="text" name="name" maxlength="10" /></td></tr>''')
+ def test_foreignkeys_which_use_to_field_and_blank_string_key(self):
+ source_n = EmptyKeySource.objects.create(name="")
+ source_a = EmptyKeySource.objects.create(name="a")
@timgraham Owner

souce_a / source_b don't appear necessary for the test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ source_b = EmptyKeySource.objects.create(name="b")
+
+ sink_n = EmptyKeySink.objects.create(title="Hello", source=source_n)
+
+ self.assertEqual(sink_n.source, source_n)
+
def test_iterable_model_m2m(self) :
colour = Colour.objects.create(name='Blue')
form = ColourfulItemForm()
Something went wrong with that request. Please try again.