Opened 3 years ago

Closed 2 years ago

Last modified 2 years ago

#22738 closed Cleanup/optimization (fixed)

Schema migration: test_add_field_temp_default_boolean

Reported by: Maximiliano Robaina Owned by: nobody
Component: Migrations Version: master
Severity: Normal Keywords:
Cc: 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

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.

Attachments (2)

22738.diff (1.1 KB) - added by Claude Paroz 2 years ago.
Use can_introspect_boolean_field flag
22738-1.diff (1.2 KB) - added by Claude Paroz 2 years ago.
Take Oracle bug into account

Download all attachments as: .zip

Change History (19)

comment:1 Changed 3 years ago by Simon Charette

Triage Stage: UnreviewedAccepted

comment:2 Changed 2 years ago by Tim Graham

Type: UncategorizedCleanup/optimization

comment:3 Changed 2 years ago by Tim Graham

See also #23073 for a problem on some versions of Oracle.

comment:4 Changed 2 years ago by Shai Berger <shai@…>

In 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).

comment:5 Changed 2 years ago by Shai Berger <shai@…>

In 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

Changed 2 years ago by Claude Paroz

Attachment: 22738.diff added

Use can_introspect_boolean_field flag

comment:6 Changed 2 years ago by Claude Paroz

Has patch: set
Version: 1.7-beta-2master

I've attached my proposal. Shai, can you see what Oracle backend thinks about it?

comment:7 Changed 2 years ago by Shai Berger

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.

Changed 2 years ago by Claude Paroz

Attachment: 22738-1.diff added

Take Oracle bug into account

comment:8 Changed 2 years ago by Claude Paroz

I see. Is the new patch better at this regard?

comment:9 Changed 2 years ago by Shai Berger

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.

comment:10 Changed 2 years ago by Claude Paroz

Shai, I'll try to outperform your expectations :-)
https://github.com/django/django/pull/3278

comment:11 Changed 2 years ago by Tim Graham

Triage Stage: AcceptedReady for checkin

comment:12 Changed 2 years ago by Claude Paroz <claude@…>

Resolution: fixed
Status: newclosed

In 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.

comment:13 Changed 2 years ago by Shai Berger

Resolution: fixed
Status: closednew

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.

comment:14 Changed 2 years ago by Shai Berger

...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.

comment:15 Changed 2 years ago by Shai Berger <shai@…>

Resolution: fixed
Status: newclosed

In 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

comment:16 Changed 2 years ago by Shai Berger <shai@…>

In a54adcecff891374341adaad6a8187d416595b70:

Fixed git blunder, refs #22738

comment:17 Changed 2 years ago by Claude Paroz

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 :-)

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