#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 (3)
follow-up: 3 comment:1 by , 54 minutes ago
| Easy pickings: | unset |
|---|---|
| Resolution: | → wontfix |
| Status: | new → closed |
comment:2 by , 54 minutes ago
| Description: | modified (diff) |
|---|---|
| Easy pickings: | set |
| Has patch: | set |
comment:3 by , 41 minutes 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.
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 migrate command.