Opened 5 years ago

Closed 5 years ago

#17957 closed Bug (fixed)

If engine for default DB alias has interprets_empty_strings_as_nulls (i.e. Oracle) it affects DDL SQL for model fields null=False in other DBs

Reported by: bhuztez Owned by: nobody
Component: Database layer (models, ORM) Version: master
Severity: Normal Keywords: oracle
Cc: anssi.kaariainen@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

My settings.py looks like this

DATABASES = {
    'default': {
        'ENGINE': 'django.db.backends.sqlite3',
    },
    'other': {
        'ENGINE': 'django.db.backends.sqlite3', 
    }
}

INSTALLED_APPS = (
    'django.contrib.sites',
)

Run python manage.py sql sites --database=other

BEGIN;
CREATE TABLE "django_site" (
    "id" integer NOT NULL PRIMARY KEY,
    "domain" varchar(100) NOT NULL,
    "name" varchar(50) NOT NULL
)
;
COMMIT;

Change default database engine to django.db.backends.oracle. Run again python manage.py sql sites --database=other

BEGIN;
CREATE TABLE "django_site" (
    "id" integer NOT NULL PRIMARY KEY,
    "domain" varchar(100),
    "name" varchar(50)
)
;
COMMIT;

Disclaimer: I do not have an oracle db. I actually change dummy backend's feature.interprets_empty_strings_as_nulls to True, and set default db engine to dummy.

Attachments (1)

17957.patch (3.4 KB) - added by Ian Kelly 5 years ago.

Download all attachments as: .zip

Change History (14)

comment:1 Changed 5 years ago by Ramiro Morales

Resolution: needsinfo
Status: newclosed

We'd need some clarifications to understand and then decide what to do with this issue. Particularly:

  • What do you mean when you say "default db engine".
  • What did you really do? Change the DB backend used to the Oracle backend or to the dummy backend?
  • Did you change the backend used in both default and other aliases?. If not, in which of them?
  • Why are you messing with the dummy backend and with the interprets_empty_strings_as_nulls feature?
  • Does the fact that you are running sql sites with the --database=other switch affect the outcome of the tests you are performing or can the example be simplified even more by removing that noisy moving part?

Reopen the ticket when you can provide us that information.

Last edited 5 years ago by Ramiro Morales (previous) (diff)

comment:2 in reply to:  1 ; Changed 5 years ago by bhuztez

Resolution: needsinfo
Status: closedreopened

What do you mean when you say "default db engine".

DATABASES['default']['ENGINE']

What did you really do? Change the DB backend used to the Oracle backend or to the dummy backend?

Why are you messing with the dummy backend and with the interprets_empty_strings_as_nulls feature?

I was reading django/db/models/fields/__init__.py, found the following lines very suspicious.

        # 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

But I do not have oracle installed on my desktop. So I decide to change dummy backend to test.

Did you change the backend used in both default and other aliases?. If not, in which of them?

No, just switch the default one.

Does the fact that you are running sql sites with the --database=other switch affect the outcome of the tests you are performing or can the example be simplified even more by removing that noisy moving part?

I run the same command python manage.py sql sites --database=other before and after the switch.

comment:3 in reply to:  2 ; Changed 5 years ago by Ramiro Morales

Replying to bhuztez:

Does the fact that you are running sql sites with the --database=other switch affect the outcome of the tests you are performing or can the example be simplified even more by removing that noisy moving part?

I run the same command python manage.py sql sites --database=other before and after the switch.

That's clear. What I want to know is if there is any particular reason you are modifying the backend associated with the 'default' DB connection but then you are running the 'sql' management command specifying the 'other' DB connection. Has the latter any influence of the outcome of the issue you are seeing?

comment:4 Changed 5 years ago by Ramiro Morales

Also, this is part of by design, documented behavior of Django with CHAR-based model fields that include '' among the possible valid values when using Oracle to accommodate particular characteristics of such RDBMS.

See:

Version 1, edited 5 years ago by Ramiro Morales (previous) (next) (diff)

comment:5 in reply to:  3 Changed 5 years ago by bhuztez

Replying to ramiro:

Replying to bhuztez:

Does the fact that you are running sql sites with the --database=other switch affect the outcome of the tests you are performing or can the example be simplified even more by removing that noisy moving part?

I run the same command python manage.py sql sites --database=other before and after the switch.

That's clear. What I want to know is if there is any particular reason you are modifying the backend associated with the 'default' DB connection but then you are running the 'sql' management command specifying the 'other' DB connection. Has the latter any influence of the outcome of the issue you are seeing?

The outcome of sql command for 'other' DB, is affected by what you set for DATABASES['default']['ENGINE'].

Also, this is part of by design, documented behavior of Django CHAR-based model fields that include among the possible valid values when using Oracle to accommodate particular characteristics of such RDBMS.

This corner case is not documented, AFAIK.

comment:6 Changed 5 years ago by Ramiro Morales

Summary: ORM do not respect null=False if default db engine interprets_empty_strings_as_nullsIf engine for default DB alias has interprets_empty_strings_as_nulls (i.e. Oracle) it affects DDL SQL for model fields null=False in other DBs
Triage Stage: UnreviewedAccepted

Ah I get it now. Thanks for the patience.

comment:7 Changed 5 years ago by Ramiro Morales

See also #13528, #13711 that are about other parts of model characteristics that use data from the 'default' DATABASES alias.

comment:8 Changed 5 years ago by Anssi Kääriäinen

Cc: anssi.kaariainen@… added

There is a related test failure under Oracle:

 ./runtests.py --settings=test_oracle test_runner
Creating test database for alias 'default'...
Creating test user...
Creating test database for alias 'other'...
Creating test user...
...........sError: One or more models did not validate:
sessions.session: "session_key": Primary key fields cannot have null=True.
 
E.
======================================================================
ERROR: test_ticket_16885 (regressiontests.test_runner.tests.Ticket16885RegressionTests)
Features are also confirmed on mirrored databases.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/akaariai/Programming/django/tests/regressiontests/test_runner/tests.py", line 203, in test_ticket_16885
    DjangoTestSuiteRunner(verbosity=0).setup_databases()
  File "/home/akaariai/Programming/django/tests/django/test/simple.py", line 306, in setup_databases
    self.verbosity, autoclobber=not self.interactive)
  File "/home/akaariai/Programming/django/tests/django/db/backends/creation.py", line 271, in create_test_db
    load_initial_data=False)
  File "/home/akaariai/Programming/django/tests/django/core/management/__init__.py", line 150, in call_command
    return klass.execute(*args, **defaults)
  File "/home/akaariai/Programming/django/tests/django/core/management/base.py", line 248, in execute
    sys.exit(1)
SystemExit: 1

I believe the cause of this error is having a CharField with primary_key=True, and this leads to validation failure as pk + null=True isn't a valid combination.

If I am not mistaken it would be enough to create the character fields with the "NOT NULL" part removed, and leaving field.null alone.

comment:9 Changed 5 years ago by Ian Kelly

I've uploaded a patch to prevent emitting "NOT NULL" without actually changing the field.null. This also takes care of the nullable primary key issue that Anssi identified.

comment:10 Changed 5 years ago by Ian Kelly

Has patch: set

Changed 5 years ago by Ian Kelly

Attachment: 17957.patch added

comment:11 Changed 5 years ago by Ian Kelly

Keywords: oracle added

comment:12 Changed 5 years ago by Anssi Kääriäinen

Triage Stage: AcceptedReady for checkin

The patch looks good to me. It doesn't look like any additional tests are needed, the test suite fails pretty spectacularly if you accidentally have char fields as NOT NULL on Oracle.

comment:13 Changed 5 years ago by Anssi Kääriäinen

Resolution: fixed
Status: reopenedclosed

Fixed in 584e2c03376895aeb0404cc1fcc1ad24dfdbc58e

For the record: Alex spotted that the patch altered the way non-nullable character fields are handled in sql/query.py. There were a couple of field.null checks there, and those are now handled by is_nullable(field) so that the old behavior is preserved. See https://github.com/django/django/pull/19/ for more details.

Note: See TracTickets for help on using tickets.
Back to Top