Opened 4 years ago

Closed 4 years ago

#25172 closed Bug (fixed)

System checks don't respect database routers

Reported by: Ion Scerbatiuc Owned by: nobody
Component: Core (System checks) Version: 1.7
Severity: Normal Keywords: system-checks multi-database
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

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

Attachments (1)

multi_db_checks.diff (6.0 KB) - added by Ion Scerbatiuc 4 years ago.
Fix the issue and add regression tests

Download all attachments as: .zip

Change History (13)

comment:1 Changed 4 years ago by Tim Graham

Summary: System check is failing on multi-database setup with different backendsSystem checks don't respect database routers
Triage Stage: UnreviewedAccepted
Type: UncategorizedBug

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.

comment:2 Changed 4 years ago by Tim Graham

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.

comment:3 Changed 4 years ago by Ion Scerbatiuc

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.

comment:4 Changed 4 years ago by Tim Graham

Are you able to open a pull request? That makes review easier.

comment:5 Changed 4 years ago by Ion Scerbatiuc

comment:6 Changed 4 years ago by Tim Graham

Has patch: set

Great. By the way, don't forget to check "Has patch" so the ticket appears in the review queue.

comment:7 Changed 4 years ago by Tim Graham

Patch needs improvement: set

Some test failures need to be addressed.

Changed 4 years ago by Ion Scerbatiuc

Attachment: multi_db_checks.diff added

Fix the issue and add regression tests

comment:8 Changed 4 years ago by Tim Graham

Patch needs improvement: unset

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.

comment:9 Changed 4 years ago by Ion Scerbatiuc

Oh, didn't know that. It's definitely easier to work with pull requests instead of patches.

Thanks!

comment:10 Changed 4 years ago by Tim Graham

Patch needs improvement: set

comment:11 Changed 4 years ago by Ion Scerbatiuc

Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

comment:12 Changed 4 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: newclosed

In 0cc059c:

Fixed #25172 -- Fixed check framework to work with multiple databases.

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