Spin off from #3609986: Improve database exception messages.

Problem/Motivation

Database exceptions' hierarchy is a bit messy, and couple of them are unused. Found out in #3609986: Improve database exception messages.

Proposed resolution

  • Cleanup the hierarchy structure and deprecate those that are unused.
  • Introduce dedicated exceptions for table existence checks: SchemaTableDoesNotExistException and SchemaTableAlreadyExistsException
  • Introduce a SqlExecutionInfo value object that carries the details of the SQL operation that failed.
  • Change database wrapper exceptions to include the SqlExecutionInfo optionally.

New hierarchy structure of the database exceptions after MR merge (AI assisted in drawing the tree):

DatabaseException                                      [interface]
│
├── DatabaseExceptionWrapper
│   │
│   ├── DatabaseAccessDeniedException
│   ├── DatabaseConnectionRefusedException
│   ├── DatabaseNotFoundException
│   │
│   ├── IntegrityConstraintViolationException
│   │
│   ├── SchemaException
│   │   │
│   │   ├── SchemaObjectDoesNotExistException
│   │   │   └── SchemaTableDoesNotExistException
│   │   │
│   │   ├── SchemaObjectExistsException
│   │   │   └── SchemaTableAlreadyExistsException
│   │   │
│   │   ├── SchemaPrimaryKeyMustBeDroppedException
│   │   ├── SchemaTableColumnSizeTooLargeException
│   │   └── SchemaTableKeyTooLargeException
│   │
│   └── TransactionException
│       │
│       ├── TransactionCommitFailedException
│       ├── TransactionExplicitCommitNotAllowedException   [deprecated]
│       ├── TransactionNameNonUniqueException
│       ├── TransactionNoActiveException                  [deprecated]
│       └── TransactionOutOfOrderException
│
├── ConnectionNotDefinedException
├── DriverNotSpecifiedException
├── InvalidQueryException
├── RowCountException
│
├── SchemaDefinitionException
│
├── Query\FieldsOverlapException
├── Query\InvalidMergeQueryException
├── Query\NoFieldsException
└── Query\NoUniqueFieldException

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3622582

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

mondrake created an issue. See original summary.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Issue summary: View changes
mondrake’s picture

mondrake
Location 🇮🇹
commented
Issue summary: View changes

mondrake opened merge request !17095

mondrake’s picture

mondrake
Location 🇮🇹
commented
Issue summary: View changes
Status: Active » Needs review
dcam’s picture

dcam commented
Status: Needs review » Needs work

I think this is mostly OK, but I did find one issue. There's a docblock reference to the deprecated TransactionNoActiveException at TransationManagerInterface::rollback(), https://git.drupalcode.org/project/drupal/-/blob/main/core/lib/Drupal/Co.... As far as I can tell only TransationManagerBase::rollback() implements this function and it does not throw TransactionNoActiveException. Maybe it did at one time. I attempted a brief search through its Git history to see if I could find out. Eventually I decided it wasn't that important and stopped. Anyway, I'm not familiar enough with the database subsystem to know for certain, but it seems like we can just delete this @throws annotation.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs work » Needs review

Done #6 (thanks), added draft CR, linked in deprecation messages, bumped deprecation version to 12.1.

dcam’s picture

dcam commented
Status: Needs review » Reviewed & tested by the community

Thank you for taking care of the deprecation messages. I knew there was something that I was forgetting. I did this review over several days and it slipped my mind.

Anyway, that was all the feedback I had. I'll go ahead and RTBC it so we can see what a committer says.

daffie’s picture

daffie commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs issue summary update

The code changes in the MR look good.
Just a single old deprecation that needs to be updated.
What I am missing in the IS and maybe the CR is information about the new class SqlExecutionInfo. Also why it is added. I am not saying that it wrong or anything. Just explain what is added and why it should be added.
The same for the other changes in the MR. I am getting the feeling that they should be in different issues.
A lot of exception are getting an extra parameter. Should that be documented. At least in the IS.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs issue summary update

Introduce dedicated exceptions for table existence checks: SchemaTableDoesNotExistException and SchemaTableAlreadyExistsException, to help simplifying #2371709: Move the on-demand-table creation into the database API.

longwave-bot made their first commit to this issue’s fork.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Normalized all exceptions across all drivers as some SQLite and Postgres cases were missing before. Also improved the formatter a bit.

Parts of these commits were assisted by GPT 6.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs review » Needs work

Couple of comments inline. Man AI-built tests are hard to follow...

mondrake’s picture

mondrake
Location 🇮🇹
commented

All drivers' ExceptionHandlers now implement the pattern handleStatementException() -> rethrowNormalizedException(), but the base class does not and is now dead code for core anyway.

Can we deprecate calling the base class' handleStatementException() on the basis that it will become abstract in 13? and implement a stub rethrowNormalizedException() in the base class too that just throws?

daffie’s picture

daffie commented
Priority: Normal » Critical
Related issues: +#2371709: Move the on-demand-table creation into the database API

This issue has become a blocker for #2371709: Move the on-demand-table creation into the database API and that issue has the priority critical. Therefore this issue also gets the priority critical.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Related issues: +#3629290: Naming and location of database exceptions

Filed a follow up.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs work » Needs review

All pipelines are green.

Addressed the feedback from #13 and #14:

  • Replaced the per-driver unit tests with a kernel test in SchemaTableExceptionTest that triggers real table errors through query() on every driver.
  • Removed the duplicate pgsql unit test.
  • Removed the SQLite trigger case.
  • DatabaseExceptionWrapperTest now asserts SchemaTableDoesNotExistException.
  • Converted simple data providers to #[TestWith].
  • Simplified SqlExecutionInfoTest.
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Did another self-review pass, removed a bunch of tests that were testing nothing useful or duplicates. Also removed fromThrowable() and stripFromMessage() as nothing calls them - we can reintroduce them when we need them? Hopefully this addresses @daffie's concerns in #9.

mondrake’s picture

mondrake
Location 🇮🇹
commented

Lovely and neat. Thanks!

+1 for RTBC but I cannot mark.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Added two CRs, one for database driver maintainers who need to add the new exceptions, and another for the message format changes and SqlExceptionInfo.

daffie’s picture

daffie commented
Status: Needs review » Needs work

I was a bit worried about a possible BC break with all the changes to the exceptions. Only I could not find an exception change that would be a BC break.
All the code changes look good to me.
@longwave what to do a minor documentation change in the MR, which is fine by me.
After that is it RTBC for me.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs work » Needs review

Renamed the variable, updated some comments and the CR to match.

daffie’s picture

daffie commented
Status: Needs review » Reviewed & tested by the community

The last changes from @longwave look good to me.
There is enough testing added.
All code changes look good to me.
For me it is RTBC.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Assigned: Unassigned » mondrake
Status: Reviewed & tested by the community » Needs work

found a small inconsistency, will update soon

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs work » Needs review

With this MR, DatabaseExceptionWrapper becomes the base exception class for any error that is thrown by the lower-level database driver client (PDO, mysqli, etc).

Based on this, IMHO

  • DatabaseAccessDeniedException, DatabaseConnectionRefusedException and DatabaseNotFoundException should be descendants of DatabaseExceptionWrapper, since they are triggered after the client returns an error.
  • Conversely, SchemaDefinitionException should no longer be a descendent of SchemaException, since that inherits from DatabaseExceptionWrapper and is triggered when execution of DML fails. But errors from the schema definition have nothing to do with the client connection. So it could be a direct implementation of the interface instead.

Made changes accordingly in the last commit.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Issue summary: View changes

Updated the inheritance tree in the IS (AI assisted generation)