#36774 closed Cleanup/optimization (wontfix)
catch DatabaseError instead of OperationalError in makemigrations check_consistent_history()
| Reported by: | Piotr Kubiak | Owned by: | |
|---|---|---|---|
| Component: | Migrations | Version: | 6.0 |
| Severity: | Normal | Keywords: | |
| Cc: | Piotr Kubiak | Triage Stage: | Unreviewed |
| Has patch: | yes | Needs documentation: | no |
| Needs tests: | no | Patch needs improvement: | no |
| Easy pickings: | yes | UI/UX: | no |
Description (last modified by )
This is a follow up to #31504.
I am proposing a change to the way that the check_consistent_history() behaves when an error occurs while accessing the database during the makemigrations command. Currently, only the OperationalError is caught and silenced.
In my setup, the database is accessible (I am able to connect), but database administrator implemented custom triggers that limit access for specific users. This makes my connection attempts fail with DatabaseError rather than OperationalError. But since the former is not silenced, makemigrations effectively requires a fully working database setup, which according to #31504 should not be necessary.
The original issue states that Django should "fail gracefully if the connection to the database isn't available." Therefore, I suggest catching the more general DatabaseError instead of OperationalError, as it covers more cases of database unavailability.
Proposed fix, I am happy to prepare a PR.
Change History (4)
follow-up: 3 comment:1 by , 3 weeks ago
| Easy pickings: | unset |
|---|---|
| Resolution: | → wontfix |
| Status: | new → closed |
comment:2 by , 3 weeks ago
| Description: | modified (diff) |
|---|---|
| Easy pickings: | set |
| Has patch: | set |
comment:3 by , 3 weeks ago
Replying to Jacob Walls:
Thanks for the ticket. I'm concerned the wide catch would hide legitimate programming errors from other sources (e.g. introspection syntax). This appears to be a niche use case; you could work around it at the project level by overriding the makemigrations command.
Are you open for discussion?
I understand your point, but is the purpose of check_consistent_history() really to raise on introspection syntax errors?
If it is, that is not clear. Is it even possible that check_consistent_history() will raise ProgrammingError or any other specific error? Maybe it's just a side effect.
For me, check_consistent_history() is an optional check. #31504 gives a rationale that if database is not available, skip this check. I think my use case may be niche, but it is valid.
comment:4 by , 3 weeks ago
I agree with the wontfix status. The goal of the existing OperationalError catch is narrow: avoid blocking makemigrations when the database is simply not reachable. It was never meant to ignore all database-related failures. This is what #31504 solved.
Catching DatabaseError is too broad, and worse backwards incompatible. It wraps real programming errors, backend bugs, bad introspection queries, and misconfigured schemas. Swallowing those would hide exactly the signals developers need when something is actually wrong. That is a regression risk in a core command.
OperationalError hits the intended scope without masking unrelated issues. Broadening it would change the contract of makemigrations from "skip the consistency check when the DB is down" to "ignore any error that happens to come from the DB layer".
The reported setup is valid but, IMHO, niche. Projects with custom triggers can override the command locally because they understand their own failure modes. Django cannot safely broaden the catch in a way that preserves predictable error reporting for the common case.
Keeping this as wontfix keeps the boundary clear and avoids silent failures in migrations.
Thanks for the ticket. I'm concerned the wide catch would hide legitimate programming errors from other sources (e.g. introspection syntax). This appears to be a niche use case; you could work around it at the project level by overriding the makemigrations command.