From 0d45eef0bd32f46f469ab335c780be5f953282c7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Anssi=20K=C3=A4=C3=A4ri=C3=A4inen?= Date: Sun, 29 Apr 2012 05:44:18 +0300 Subject: [PATCH] Prevent Oracle from changing field.null to True Fixed #17957 -- when using Oracle and character fields, the fields were set null = True to ease the handling of empty strings. This caused problems when using multiple databases from different vendors, or when the character field happened to be also a primary key. The handling was changed so that NOT NULL is not emitted on Oracle even if field.null = False, and field.null is not touched otherwise. Thanks to bhuztez for the report, ramiro for triaging & comments, ikelly for the patch and alex for reviewing. --- django/db/backends/creation.py | 8 +++++++- django/db/models/fields/__init__.py | 5 ----- django/db/models/sql/query.py | 23 +++++++++++++++++++++-- docs/ref/databases.txt | 10 +++++----- docs/ref/models/fields.txt | 5 ++--- tests/regressiontests/queries/tests.py | 22 ++++++++++++++++++++++ 6 files changed, 57 insertions(+), 16 deletions(-) diff --git a/django/db/backends/creation.py b/django/db/backends/creation.py index 0cd055866a3fe..75afe926fabb2 100644 --- a/django/db/backends/creation.py +++ b/django/db/backends/creation.py @@ -50,7 +50,13 @@ def sql_create_model(self, model, style, known_models=set()): # Make the definition (e.g. 'foo VARCHAR(30)') for this field. field_output = [style.SQL_FIELD(qn(f.column)), style.SQL_COLTYPE(col_type)] - if not f.null: + # Oracle treats the empty string ('') as null, so coerce the null + # option whenever '' is a possible value. + null = f.null + if (f.empty_strings_allowed and not f.primary_key and + self.connection.features.interprets_empty_strings_as_nulls): + null = True + if not null: field_output.append(style.SQL_KEYWORD('NOT NULL')) if f.primary_key: field_output.append(style.SQL_KEYWORD('PRIMARY KEY')) diff --git a/django/db/models/fields/__init__.py b/django/db/models/fields/__init__.py index dc4495390a639..5a5e6cc8c3d82 100644 --- a/django/db/models/fields/__init__.py +++ b/django/db/models/fields/__init__.py @@ -85,11 +85,6 @@ def __init__(self, verbose_name=None, name=None, primary_key=False, self.primary_key = primary_key self.max_length, self._unique = max_length, unique self.blank, self.null = blank, null - # Oracle treats the empty string ('') as null, so coerce the null - # option whenever '' is a possible value. - if (self.empty_strings_allowed and - connection.features.interprets_empty_strings_as_nulls): - self.null = True self.rel = rel self.default = default self.editable = editable diff --git a/django/db/models/sql/query.py b/django/db/models/sql/query.py index cf527b18b0822..6111a881186ce 100644 --- a/django/db/models/sql/query.py +++ b/django/db/models/sql/query.py @@ -1193,7 +1193,7 @@ def add_filter(self, filter_expr, connector=AND, negate=False, trim=False, entry.negate() self.where.add(entry, AND) break - if field.null: + if self.is_nullable(field): # In SQL NULL = anyvalue returns unknown, and NOT unknown # is still unknown. However, in Python None = anyvalue is False # (and not False is True...), and we want to return this Python's @@ -1396,7 +1396,8 @@ def setup_joins(self, names, opts, alias, dupe_multis, allow_many=True, opts, target) alias = self.join((alias, table, from_col, to_col), - exclusions=exclusions, nullable=field.null) + exclusions=exclusions, + nullable=self.is_nullable(field)) joins.append(alias) else: # Non-relation fields. @@ -1946,6 +1947,24 @@ def set_start(self, start): self.select = [(select_alias, select_col)] self.remove_inherited_models() + def is_nullable(self, field): + """ + A helper to check if the given field should be treated as nullable. + + Some backends treat '' as null and Django treats such fields as + nullable for those backends. In such situations field.null can be + False even if we should treat the field as nullable. + """ + # We need to use DEFAULT_DB_ALIAS here, as QuerySet does not have + # (nor should it have) knowledge of which connection is going to be + # used. The proper fix would be to defer all decisions where + # is_nullable() is needed to the compiler stage, but that is not easy + # to do currently. + if ((connections[DEFAULT_DB_ALIAS].features.interprets_empty_strings_as_nulls) + and field.empty_strings_allowed): + return True + else: + return field.null def get_order_dir(field, default='ASC'): """ diff --git a/docs/ref/databases.txt b/docs/ref/databases.txt index 8fbe4125c93bf..2dea337d2bc74 100644 --- a/docs/ref/databases.txt +++ b/docs/ref/databases.txt @@ -685,11 +685,11 @@ NULL and empty strings Django generally prefers to use the empty string ('') rather than NULL, but Oracle treats both identically. To get around this, the -Oracle backend coerces the ``null=True`` option on fields that have -the empty string as a possible value. When fetching from the database, -it is assumed that a NULL value in one of these fields really means -the empty string, and the data is silently converted to reflect this -assumption. +Oracle backend ignores an explicit ``null`` option on fields that +have the empty string as a possible value and generates DDL as if +``null=True``. When fetching from the database, it is assumed that +a ``NULL`` value in one of these fields really means the empty +string, and the data is silently converted to reflect this assumption. ``TextField`` limitations ------------------------- diff --git a/docs/ref/models/fields.txt b/docs/ref/models/fields.txt index d1f12ffc95183..eab7ad91e1448 100644 --- a/docs/ref/models/fields.txt +++ b/docs/ref/models/fields.txt @@ -55,9 +55,8 @@ string, not ``NULL``. .. note:: - When using the Oracle database backend, the ``null=True`` option will be - coerced for string-based fields that have the empty string as a possible - value, and the value ``NULL`` will be stored to denote the empty string. + When using the Oracle database backend, the value ``NULL`` will be stored to + denote the empty string regardless of this attribute. If you want to accept :attr:`~Field.null` values with :class:`BooleanField`, use :class:`NullBooleanField` instead. diff --git a/tests/regressiontests/queries/tests.py b/tests/regressiontests/queries/tests.py index fb78219977422..416970d4936a6 100644 --- a/tests/regressiontests/queries/tests.py +++ b/tests/regressiontests/queries/tests.py @@ -1930,3 +1930,25 @@ def test_col_not_in_list_containing_null(self): self.assertQuerysetEqual( NullableName.objects.exclude(name__in=[None]), ['i1'], attrgetter('name')) + +class EmptyStringsAsNullTest(TestCase): + """ + Test that filtering on non-null character fields works as expected. + The reason for these tests is that Oracle treats '' as NULL, and this + can cause problems in query construction. Refs #17957. + """ + + def setUp(self): + self.nc = NamedCategory.objects.create(name='') + + def test_direct_exclude(self): + self.assertQuerysetEqual( + NamedCategory.objects.exclude(name__in=['nonexisting']), + [self.nc.pk], attrgetter('pk') + ) + + def test_joined_exclude(self): + self.assertQuerysetEqual( + DumbCategory.objects.exclude(namedcategory__name__in=['nonexisting']), + [self.nc.pk], attrgetter('pk') + )