#22738 closed Cleanup/optimization (fixed)
Schema migration: test_add_field_temp_default_boolean
Reported by: | Maximiliano Robaina | Owned by: | nobody |
---|---|---|---|
Component: | Migrations | Version: | dev |
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)
Change History (19)
comment:1 by , 10 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:2 by , 10 years ago
Type: | Uncategorized → Cleanup/optimization |
---|
comment:3 by , 10 years ago
comment:6 by , 10 years ago
Has patch: | set |
---|---|
Version: | 1.7-beta-2 → master |
I've attached my proposal. Shai, can you see what Oracle backend thinks about it?
comment:7 by , 10 years ago
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.
comment:9 by , 10 years ago
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 by , 10 years ago
Shai, I'll try to outperform your expectations :-)
https://github.com/django/django/pull/3278
comment:11 by , 10 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
comment:12 by , 10 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
comment:13 by , 10 years ago
Resolution: | fixed |
---|---|
Status: | closed → new |
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 by , 10 years ago
...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 by , 10 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
comment:17 by , 10 years ago
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 :-)
See also #23073 for a problem on some versions of Oracle.