[Django] #22738: Schema migration: test_add_field_temp_default_boolean
Django
-------------------------------+------------------------
Reporter: maxi | Owner: nobody
Type: Uncategorized | Status: new
Component: Migrations | Version: 1.7-beta-2
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+------------------------
Currently, in the schema test app, the test_add_field_temp_default_boolean
method does an exception for mysql engine, which return IntegerField for
boolean fields, else it returns the BooleanField type. In my case,
firebird, a SmallIntegerField is returned, so that the test fails.
It is necessary to support a better approach.
--
Ticket URL: <https://code.djangoproject.com/ticket/22738>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------+--------------------------------------
Reporter: maxi | Owner: nobody
Type: Uncategorized | Status: new
Component: Migrations | Version: 1.7-beta-2
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by charettes):
* needs_better_patch: => 0
* needs_docs: => 0
* needs_tests: => 0
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/22738#comment:1>
Django
-------------------------------------+-------------------------------------
Reporter: maxi | Owner: nobody
Type: | Status: new
Cleanup/optimization | Version:
Component: Migrations | 1.7-beta-2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timo):
* type: Uncategorized => Cleanup/optimization
--
Ticket URL: <https://code.djangoproject.com/ticket/22738#comment:2>
Django
-------------------------------------+-------------------------------------
Type: | Status: new
Cleanup/optimization | Version:
Component: Migrations | 1.7-beta-2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timo):
See also #23073 for a problem on some versions of Oracle.
--
Ticket URL: <https://code.djangoproject.com/ticket/22738#comment:3>
Django
-------------------------------------+-------------------------------------
Type: | Status: new
Cleanup/optimization | Version:
Component: Migrations | 1.7-beta-2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Shai Berger <shai@…>):
In [changeset:"56252e7f46afce36fd112971c28188a3fd509a43"]:
{{{
#!CommitTicketReference repository=""
revision="56252e7f46afce36fd112971c28188a3fd509a43"
Fixed schema test for Oracle 11.2.0.1 which is used in Django Project's
CI.
Refs #23073 Workaround.
Refs #22738 Repeats the mysql "offense". When the issue is solved, the
Oracle special case should be made to play with the solution (that is,
Oracle should be fixed the same way that mysql and the 3rd-party backneds
are).
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/22738#comment:4>
Django
-------------------------------------+-------------------------------------
Type: | Status: new
Cleanup/optimization | Version:
Component: Migrations | 1.7-beta-2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Shai Berger <shai@…>):
In [changeset:"588f66d18235762f8e0f96200c88bc4ba6c1c579"]:
{{{
#!CommitTicketReference repository=""
revision="588f66d18235762f8e0f96200c88bc4ba6c1c579"
[1.7.x] Fixed schema test for Oracle 11.2.0.1 which is used in Django
Project's CI.
Refs #23073 Workaround.
Refs #22738 Repeats the mysql "offense". When the issue is solved, the
Oracle special case should be made to play with the solution (that is,
Oracle should be fixed the same way that mysql and the 3rd-party backneds
are).
Backport of 56252e7 from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/22738#comment:5>
Django
--------------------------------------+------------------------------------
Reporter: maxi | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by claudep):
* has_patch: 0 => 1
* version: 1.7-beta-2 => master
Comment:
I've attached my proposal. Shai, can you see what Oracle backend thinks
about it?
--
Ticket URL: <https://code.djangoproject.com/ticket/22738#comment:6>
Django
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: new
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by shaib):
Oracle won't like it, because it `can_introspect_boolean_field` even in
the buggy version; the problem is specifically with fields that have
defaults. This is explained in #23073.
--
Ticket URL: <https://code.djangoproject.com/ticket/22738#comment:7>
Django
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: new
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by claudep):
I see. Is the new patch better at this regard?
--
Ticket URL: <https://code.djangoproject.com/ticket/22738#comment:8>
Django
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: new
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by shaib):
Yes, and it passes as well.
I had hoped for a more "surgical" solution, where each backend gets to
specify exactly what is expected here; the current suggestion is a little
blunt in this regard, and Oracle stays special-cased in the test, which
I'd rather avoid; but the suggestion does solve the problem in an
acceptable way, IMO.
--
Ticket URL: <https://code.djangoproject.com/ticket/22738#comment:9>
Django
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: new
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by claudep):
Shai, I'll try to outperform your expectations :-)
https://github.com/django/django/pull/3278
--
Ticket URL: <https://code.djangoproject.com/ticket/22738#comment:10>
Django
-------------------------------------+-------------------------------------
Type: | Status: new
Cleanup/optimization | Version: master
Component: Migrations | Resolution:
Severity: Normal | Triage Stage: Ready for
Keywords: | checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/22738#comment:11>
Django
-------------------------------------+-------------------------------------
Type: | Status: closed
Cleanup/optimization | Version: master
Component: Migrations | Resolution: fixed
Severity: Normal | Triage Stage: Ready for
Keywords: | checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Claude Paroz <claude@…>):
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"dbdae3a75512bddbf0b75ea6354fb3fc4bdb53cf"]:
{{{
#!CommitTicketReference repository=""
revision="dbdae3a75512bddbf0b75ea6354fb3fc4bdb53cf"
Fixed #22738 -- Abstracted boolean field type introspection
Thanks maxi for the report, Shai Berger for his help with the patch
and Tim Graham for the review.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/22738#comment:12>
Django
-------------------------------------+-------------------------------------
Type: | Status: new
Cleanup/optimization | Version: master
Component: Migrations | Resolution:
Keywords: | checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by shaib):
* status: closed => new
* resolution: fixed =>
Comment:
The new patch is, indeed, a significant improvement compared to the
previous approach, except for one small thing -- it fails on Oracle :).
I'll fix it.
--
Ticket URL: <https://code.djangoproject.com/ticket/22738#comment:13>
Django
-------------------------------------+-------------------------------------
Type: | Status: new
Cleanup/optimization | Version: master
Component: Migrations | Resolution:
Severity: Normal | Triage Stage: Ready for
Keywords: | checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by shaib):
...so, let's see what Jenkins thinks of
https://github.com/django/django/pull/3283
If I read the earlier failures right -- this will fail as well, because,
apparently, the buggy behavior only kicks in if the column is added by
`ALTER TABLE`, not if it is already present in the `CREATE TABLE`. If that
turns out to be the case, I'll add another argument to the
`DatabaseFeatures` method.
--
Ticket URL: <https://code.djangoproject.com/ticket/22738#comment:14>
Django
-------------------------------------+-------------------------------------
Type: | Status: closed
Cleanup/optimization | Version: master
Component: Migrations | Resolution: fixed
Severity: Normal | Triage Stage: Ready for
Keywords: | checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Shai Berger <shai@…>):
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"c1ae0621bab6b013025ec9024691ce7ad556409e"]:
{{{
#!CommitTicketReference repository=""
revision="c1ae0621bab6b013025ec9024691ce7ad556409e"
Fixed #22738 -- made finer distinctions for when Boolean is not detected
on Oracle
Thanks Claude Paroz for partial fix and Simon Charrette for review
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/22738#comment:15>
Django
-------------------------------------+-------------------------------------
Type: | Status: closed
Cleanup/optimization | Version: master
Component: Migrations | Resolution: fixed
Severity: Normal | Triage Stage: Ready for
Keywords: | checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Shai Berger <shai@…>):
In [changeset:"a54adcecff891374341adaad6a8187d416595b70"]:
{{{
#!CommitTicketReference repository=""
revision="a54adcecff891374341adaad6a8187d416595b70"
Fixed git blunder, refs #22738
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/22738#comment:16>
Django
-------------------------------------+-------------------------------------
Type: | Status: closed
Cleanup/optimization | Version: master
Component: Migrations | Resolution: fixed
Severity: Normal | Triage Stage: Ready for
Keywords: | checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by claudep):
Thanks for fixing this. But frankly, I think this is too much code to just
workaround a bug in a proprietary product. I don't think Django deserves
that. But now it's in, it's in :-)
--
Ticket URL: <https://code.djangoproject.com/ticket/22738#comment:17>