Fixed #31055 -- Handled database errors on check constraint support check.#12201
Fixed #31055 -- Handled database errors on check constraint support check.#12201charettes wants to merge 5 commits intodjango:masterfrom
Conversation
django/db/models/base.py
Outdated
There was a problem hiding this comment.
I'm afraid that we can hit this in other cases, e.g. in _check_json_fields() in #11452. What do you think about checking connection usability in mysql_server_info(), e.g.
@cached_property
def mysql_server_info(self):
if not self.is_usable():
return ''
with self.temporary_connection() as cursor:
cursor.execute('SELECT VERSION()')
return cursor.fetchone()[0]There was a problem hiding this comment.
@felixxm that's what I initially tried but it was crashing with AttributeError
File "/tests/django/django/db/backends/mysql/base.py", line 351, in mysql_server_info
if not self.is_usable():
File "/tests/django/django/db/backends/mysql/base.py", line 329, in is_usable
self.connection.ping()
AttributeError: 'NoneType' object has no attribute 'ping'
There was a problem hiding this comment.
So maybe we should check connection in is_usable() 🤔 like in related fix for PostgreSQL
a96b901#diff-aef216ccdc37941bfed98ea3c80f7642R251-R252
|
@felixxm it looks like we don't run django/django/core/checks/registry.py Lines 67 to 69 in 8d087f9 And we only run these tests on django/django/core/management/commands/migrate.py Lines 62 to 65 in 8d087f9 The solution is likely to write these two tests the other way around by iterating over models using check constraints when the feature is not available. Thoughts? |
fca498d to
1250550
Compare
django/db/models/constraints.py
Outdated
There was a problem hiding this comment.
Totally opened to a better name here.
There was a problem hiding this comment.
I have a suggestion. We previously added PostgresIndex.check_supported() to handle checking support for various index types/flags.
While that currently takes in schema_editor, we could change this line to pass schema_editor.connection and then we would have some consistency if you also adopted def check_supported(self, connection): here.
There was a problem hiding this comment.
Having said that, PostgresIndex.check_supported() doesn't return a boolean, it expects NotSupportedError to be raised as it allows checking for multiple conditions. We also don't handle those exceptions intentionally.
I'm not sure that is a deal breaker to making this look similar, however.
1250550 to
a9e9e24
Compare
This check function is tagged with "database" which ensures these checks are only performed when running the migrate command since they may require access to a database connection run appropriately. Refs #26351, #31055.
cbffa24 to
339ebc0
Compare
Also introduced the undocumented Constraint.supported(connection) API to delegate support determination to the Constraint instance.
|
Something we could do here instead is of special casing That allows the test runner to only enable checks on databases that were created and the migrate command to only run checks on the specified |
339ebc0 to
1b71f90
Compare
1b71f90 to
0dc1387
Compare
|
I still think it's worth exploring plugging |
Since #28478 test databases are only created if necessary for the subset of
tests targeted.
Under such circumstances its possible for some databases defined in
settings.DATABASES not to exist which prevents the retrieval of the
supports_table_check_constraints feature flag on MySQL since it requires a
usable connection.
Skipping the check when this happens should not be harmful given it will run
under normal circumstances when the full set of tests are targeted or when
checks are run from a different management command.
https://code.djangoproject.com/ticket/31055