[Django] #36774: catch DatabaseError instead of OperationalError in makemigrations check_consistent_history()

[Django]

#36774: catch DatabaseError instead of OperationalError in makemigrations
check_consistent_history()
-------------------------------------+-------------------------------------
Reporter: Piotr Kubiak | Type:
| Cleanup/optimization
Status: new | Component:
| Migrations
Version: 6.0 | Severity: Normal
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
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.

I am happy to prepare a PR.
--
Ticket URL: <https://code.djangoproject.com/ticket/36774>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#36774: catch DatabaseError instead of OperationalError in makemigrations
check_consistent_history()
-------------------------------------+-------------------------------------

Reporter: Piotr Kubiak | Owner: (none)
Type: | Status: closed
Cleanup/optimization |
Component: Migrations | Version: 6.0
Severity: Normal | Resolution: wontfix

Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Jacob Walls):

* easy: 1 => 0
* resolution: => wontfix
* status: new => closed

Comment:

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.
--
Ticket URL: <https://code.djangoproject.com/ticket/36774#comment:1>

[Django]

#36774: catch DatabaseError instead of OperationalError in makemigrations
check_consistent_history()
-------------------------------------+-------------------------------------
Reporter: Piotr Kubiak | Owner: (none)
Type: | Status: closed
Cleanup/optimization |
Component: Migrations | Version: 6.0
Severity: Normal | Resolution: wontfix
Keywords: | Triage Stage:
| Unreviewed

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Piotr Kubiak):

* easy: 0 => 1
* has_patch: 0 => 1


Old description:

> 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.
>
> I am happy to prepare a PR.

New description:

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.

[https://github.com/piotr-
kubiak/django/commit/c42edfdbed441afdeba213b502741ef3e8190d85 Proposed
fix], I am happy to prepare a PR.

--
--
Ticket URL: <https://code.djangoproject.com/ticket/36774#comment:2>

[Django]

#36774: catch DatabaseError instead of OperationalError in makemigrations
check_consistent_history()
-------------------------------------+-------------------------------------
Reporter: Piotr Kubiak | Owner: (none)
Type: | Status: closed
Cleanup/optimization |
Component: Migrations | Version: 6.0
Severity: Normal | Resolution: wontfix
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Piotr Kubiak):

Replying to [comment:1 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/36774#comment:3>

[Django]

#36774: catch DatabaseError instead of OperationalError in makemigrations
check_consistent_history()
-------------------------------------+-------------------------------------
Reporter: Piotr Kubiak | Owner: (none)
Type: | Status: closed
Cleanup/optimization |
Component: Migrations | Version: 6.0
Severity: Normal | Resolution: wontfix
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Natalia Bidart):

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.
--
Ticket URL: <https://code.djangoproject.com/ticket/36774#comment:4>