Code

Opened 2 years ago

Closed 2 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 ikelly 2 years ago.

Download all attachments as: .zip

Change History (14)

comment:1 follow-up: Changed 2 years ago by ramiro

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Resolution set to needsinfo
  • Status changed from new to closed

We'd need some clarifications to understand and the 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=otheror can the example be simplified even more by removing that noisy moving part?

Reopen the ticket when you can provide us that information.

Version 0, edited 2 years ago by ramiro (next)

comment:2 in reply to: ↑ 1 ; follow-up: Changed 2 years ago by bhuztez

  • Resolution needsinfo deleted
  • Status changed from closed to reopened

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 ; follow-up: Changed 2 years ago by 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?

comment:4 Changed 2 years ago by ramiro

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.

See:

Last edited 2 years ago by ramiro (previous) (diff)

comment:5 in reply to: ↑ 3 Changed 2 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 2 years ago by ramiro

  • Summary changed from ORM do not respect null=False if default db engine interprets_empty_strings_as_nulls to 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
  • Triage Stage changed from Unreviewed to Accepted

Ah I get it now. Thanks for the patience.

comment:7 Changed 2 years ago by ramiro

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

comment:8 Changed 2 years ago by akaariai

  • 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 2 years ago by ikelly

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 2 years ago by ikelly

  • Has patch set

Changed 2 years ago by ikelly

comment:11 Changed 2 years ago by ikelly

  • Keywords oracle added

comment:12 Changed 2 years ago by akaariai

  • Triage Stage changed from Accepted to Ready 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 2 years ago by akaariai

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

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.

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.