#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 , 11 years ago
| Triage Stage: | Unreviewed → Accepted |
|---|
comment:2 by , 11 years ago
| Type: | Uncategorized → Cleanup/optimization |
|---|
comment:3 by , 11 years ago
comment:6 by , 11 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 , 11 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 , 11 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 , 11 years ago
Shai, I'll try to outperform your expectations :-)
https://github.com/django/django/pull/3278
comment:11 by , 11 years ago
| Triage Stage: | Accepted → Ready for checkin |
|---|
comment:12 by , 11 years ago
| Resolution: | → fixed |
|---|---|
| Status: | new → closed |
comment:13 by , 11 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 , 11 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 , 11 years ago
| Resolution: | → fixed |
|---|---|
| Status: | new → closed |
comment:17 by , 11 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.