[Django] #25172: System check is failing on multi-database setup with different backends
Django
-------------------------------------+-------------------------------------
Reporter: delinhabit | Owner: nobody
Type: Uncategorized | Status: new
Component: Core (System | Version: 1.7
checks) | Keywords: system-checks multi-
Severity: Normal | database
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
We have the following database set-up:
* the default database is MySQL
* two additional databases are PostGIS
The system check framework (either `manage.py check` or `manage.py
runserver`) is using the default database (mysql) to validate
`django.contrib.gis.db.models.Model`-based models, and the commands are
blowing up with the following error:
AttributeError: 'DatabaseOperations' object has no attribute
'geo_db_type'.
We set-up database routers for the PostGIS models to use the proper
connection, but the check framework seems to ignore them.
You can find the detailed stack trace here:
https://gist.github.com/delinhabit/587d6f26afb4bbc559cb
The discussion that led to this ticket:
https://groups.google.com/forum/#!msg/django-
developers/_QUT4hV43YE/b-1eTcv_5yUJ
I can try to create a PR, though I'm not sure where to start. Specifically
my questions are:
* What tests case file should I extend with a regression tests? The
`tests/multiple_database` doesn't test any system checks at all.
`tests/system_check` could be a candidate but I need a multiple database
set-up and it doesn't feel right to change that test app. Should I better
create a new test app for this use case?
* I think `Field._check_backend_specific_checks` should be the place, only
that at that point it already uses connection and I'm not sure how can I
detect what connection should I use from the field instance (or the
associated model). Should I run the Database routing code to get the
connection alias? (seems a little gross to me)
Any feedback appreciated.
Thanks,
Ion
--
Ticket URL: <https://code.djangoproject.com/ticket/25172>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Reporter: delinhabit | Owner: nobody
Component: Core (System | Version: 1.7
checks) |
Keywords: system-checks | Triage Stage: Accepted
multi-database |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
* type: Uncategorized => Bug
* stage: Unreviewed => Accepted
Comment:
I think the code for `_check_backend_specific_checks()` would be roughly
like this:
{{{
from django.db import connections
for db in connections:
if allow_migrate(...):
db.validation.check_field(self, **kwargs)
}}}
Let's get some working code and then we can talk about how to test this. I
guess it might involve mocking `connections` to have two different
databases as we don't run the tests against two different databases on CI
and getting that to work might not be feasible.
--
Ticket URL: <https://code.djangoproject.com/ticket/25172#comment:1>
Django
Reporter: delinhabit | Owner: nobody
Component: Core (System | Version: 1.7
checks) |
Keywords: system-checks | Triage Stage: Accepted
multi-database |
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* Attachment "multi_db_checks.diff" added.
An attempt to fix the issues (patch not complete, requires tests)
Django
Reporter: delinhabit | Owner: nobody
Component: Core (System | Version: 1.7
checks) |
Keywords: system-checks | Triage Stage: Accepted
multi-database |
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by timgraham):
If it works, great! For a test, I would create a new file
`test/check_framework/test_multi_db.py` and write a custom database router
based on the default/other databases aliases so that the checks are only
run on one of the databases and then use mock to verify which connection's
validation methods are called. Let me know if you run into trouble. Not
positive that will work, but that would be the direction I'd try.
--
Ticket URL: <https://code.djangoproject.com/ticket/25172#comment:2>
Django
Reporter: delinhabit | Owner: nobody
Component: Core (System | Version: 1.7
checks) |
Keywords: system-checks | Triage Stage: Accepted
multi-database |
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* Attachment "multi_db_checks.diff" added.
Fix the issue and add regression tests
Django
Reporter: delinhabit | Owner: nobody
Component: Core (System | Version: 1.7
checks) |
Keywords: system-checks | Triage Stage: Accepted
multi-database |
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by delinhabit):
The patch was created against master.
If we decide to also include a fix in the next 1.7.x bugfix release, I
think it won't work as it is, because of the signature change of the
`allow_migrate` method.
--
Ticket URL: <https://code.djangoproject.com/ticket/25172#comment:3>
Django
Reporter: delinhabit | Owner: nobody
Component: Core (System | Version: 1.7
checks) |
Keywords: system-checks | Triage Stage: Accepted
multi-database |
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by timgraham):
Are you able to open a pull request? That makes review easier.
--
Ticket URL: <https://code.djangoproject.com/ticket/25172#comment:4>
Django
Reporter: delinhabit | Owner: nobody
Component: Core (System | Version: 1.7
checks) |
Keywords: system-checks | Triage Stage: Accepted
multi-database |
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by delinhabit):
Sure. Here it is: https://github.com/django/django/pull/5084
--
Ticket URL: <https://code.djangoproject.com/ticket/25172#comment:5>
Django
Reporter: delinhabit | Owner: nobody
Component: Core (System | Version: 1.7
checks) |
Keywords: system-checks | Triage Stage: Accepted
multi-database |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* has_patch: 0 => 1
Comment:
Great. By the way, don't forget to check "Has patch" so the ticket appears
in the review queue.
--
Ticket URL: <https://code.djangoproject.com/ticket/25172#comment:6>
Django
Reporter: delinhabit | Owner: nobody
Component: Core (System | Version: 1.7
checks) |
Keywords: system-checks | Triage Stage: Accepted
multi-database |
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
Comment:
Some test failures need to be addressed.
--
Ticket URL: <https://code.djangoproject.com/ticket/25172#comment:7>
Django
Reporter: delinhabit | Owner: nobody
Component: Core (System | Version: 1.7
checks) |
Keywords: system-checks | Triage Stage: Accepted
multi-database |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
-------------------------------------+-------------------------------------
* Attachment "multi_db_checks.diff" added.
Fix the issue and add regression tests
--
Django
Reporter: delinhabit | Owner: nobody
Component: Core (System | Version: 1.7
checks) |
Keywords: system-checks | Triage Stage: Accepted
multi-database |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
-------------------------------------+-------------------------------------
* Attachment "multi_db_checks.diff" added.
Fix the issue and add regression tests
--
Django
Reporter: delinhabit | Owner: nobody
Component: Core (System | Version: 1.7
checks) |
Keywords: system-checks | Triage Stage: Accepted
multi-database |
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 1 => 0
Comment:
FYI, there's no need to submit a pull request *and* attach the patch to
the ticket.
However, please do uncheck "Patch needs improvement" after updating the
pull request so the ticket appears in the review queue.
--
Ticket URL: <https://code.djangoproject.com/ticket/25172#comment:8>
Django
Reporter: delinhabit | Owner: nobody
Component: Core (System | Version: 1.7
checks) |
Keywords: system-checks | Triage Stage: Accepted
multi-database |
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by delinhabit):
Oh, didn't know that. It's definitely easier to work with pull requests
instead of patches.
Thanks!
--
Ticket URL: <https://code.djangoproject.com/ticket/25172#comment:9>
Django
Reporter: delinhabit | Owner: nobody
Component: Core (System | Version: 1.7
checks) |
Keywords: system-checks | Triage Stage: Accepted
multi-database |
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/25172#comment:10>
Django
Reporter: delinhabit | Owner: nobody
Component: Core (System | Version: 1.7
checks) |
Keywords: system-checks | Triage Stage: Ready for
multi-database | checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 1 => 0
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/25172#comment:11>
Django
Reporter: delinhabit | Owner: nobody
Component: Core (System | Version: 1.7
checks) |
Keywords: system-checks | Triage Stage: Ready for
multi-database | checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"0cc059cd104cdb70340bd08e597d403d80dc42a6" 0cc059c]:
{{{
#!CommitTicketReference repository=""
revision="0cc059cd104cdb70340bd08e597d403d80dc42a6"
Fixed #25172 -- Fixed check framework to work with multiple databases.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/25172#comment:12>