Skip to content

Prevent Oracle from changing field.null to True #19

Closed
wants to merge 1 commit into from

2 participants

@akaariai
Django member

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 and
ikelly for the patch.

@akaariai
Django member

I am still unsure how I should commit code to github - so here is another pull request fixing Oracle specific stuff.

@alex
Django member
alex commented Apr 29, 2012

Do all tests still pass on Oracle?

@akaariai
Django member

All tests haven't passed in a while on Oracle. I'm working on fixes...

At least one more test passes with the patch applied (test_runner tests, related to character field primary keys having null=True). I haven't done a comparison between full test suite with and without this patch as it takes a lot of time to do that. I can leave the full test suite running for the night if needed.

@alex
Django member
alex commented Apr 29, 2012

Ok, FWIW my concern was whether this was needed so the correct INNER vs. OUTER JOINs were generated.

@akaariai
Django member

Worth a test at least (if not already tested somewhere). It seems this could cause some weird behavior there. Should I close this pull request while I check this? I am not sure how long it will take to check and possibly correct errors caused by this...

@alex
Django member
alex commented Apr 29, 2012

I think it's fine to leave the pull request open.

@akaariai
Django member

I have investigated the queryset usage of field.null issue a little, and it seems clear that the change could result in some queries breaking when using Oracle. It is a bit scary to use Oracle's view of if the field is nullable when constructing the query because a later .using() clause could then lead to problems.

Luckily it seems that both of the field.null uses in sql/query.py will work correctly if field.null is "guessed" to true even if it is not true for the backend. So, I am going to update the patch so that sql/query.py will treat fields where (empty strings are allowed and connection is Oracle) as nullable. To me it seems in the worst case this generates slightly less performant queries when a query is generated using Oracle, but executed using some other backend - in other words, we really don't need to care about this issue too much...

I ran the full test suite during the night, before there were 3 failures, with patch only two.

@akaariai akaariai 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.
0d45eef
@akaariai
Django member

I added some tests to check that character fields are handled correctly in query.py. The field.null handling is done using is_nullable(field). It has to use the DEFAULT_DB_ALIAS when deciding if the field should be treated as nullable as the sql.QuerySet doesn't know which connection is used. The field.null handling is thus identical to pre-patch situation. That is, slightly broken in weird corner cases.

@alex
Django member
alex commented Apr 29, 2012

Ok now I remember this, unfortunately your analysis exactly matches what mine was. That is: this decision should be made later, but that's a ton of work. Patch LGTM feel free to commit!

@akaariai
Django member

I pushed this manually -- closing.

@akaariai akaariai closed this Apr 29, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.