Store query params in driver exceptions by BenMorel · Pull Request #4387 · doctrine/dbal

BenMorel · 2020-10-24T22:24:15Z

Q	A
Type	improvement
BC Break	no
Fixed issues	none

Summary

This is an attempt at #4325 (comment).

This PR removes the text representation of the bound parameters in the query exception message, and stores these params together with their types in the DriverException instead. This shifts the responsibility of (not) storing/displaying bound parameters explicitly to the consumer of the exception.

Just like #4325, this removes the dependency on ext-json (plus a couple lines of code!).

tests/Connection/ExceptionHandlingTest.php

BenMorel · 2020-10-24T22:52:46Z

Oops, after running Psalm, I realized that DriverException is marked as immutable. I cannot easily change this as it cascades to other errors, which makes me believe that you do want to keep exceptions immutable.

Thus I only see 2 solutions:

pass the params & types down to the ExceptionConverter and back to the DriverException; this would be really cumbersome but would work;
change setQueryParams() and setQueryParamTypes() to withQueryParams(), that returns a copy of the exception; I've tried this path in 4ee8fe1 but the exception being uncloneable, this makes the implementation a bit of a mess: I had to suppress phpstan & psalm warnings, so I can't say I'm happy with this.

src/Exception/DriverException.php

src/Connection.php

tests/Connection/ExceptionHandlingTest.php

src/Query.php

BenMorel · 2020-10-26T21:02:06Z

@morozov Please check this new version!

morozov

Looks like we're moving in the right direction.

src/Query.php

src/Connection.php

morozov · 2020-10-26T21:21:40Z

@beberlei do you think we could include this into 3.0 instead of master? While it changes the behavior many developers may be used to, it reduces the possibility of leaking sensitive data by the DBAL which cannot be fixed w/o a BC break.

morozov

Logic-wise looks good to me, only the upgrade notes are missing. Could you try to retarget against 3.0.x?

morozov · 2020-10-27T15:13:08Z

src/Exception/DriverException.php

+    public function __construct(Driver\Exception $driverException, ?Query $query)
    {
+        if ($query !== null) {
+            $message = 'An exception occurred while executing the query: ' . $driverException->getMessage();


Not sure which "the query" is being referred to since there's no query in the context. @greg0ire what would be the proper wording?

src/Exception/DriverException.php

morozov · 2020-10-27T15:35:42Z

@doctrine/team-dbal, please be informed. With this change, the query and its parameters will be no longer part of the exception message but will be accessible via DriverException::getQuery(). It will help prevent leaking potentially sensitive data to logs, etc. but will require changes in the tooling to display these bits during development.

BenMorel · 2020-10-27T15:58:08Z

@morozov Rebased on 3.0.x, waiting for feedback on wording!

morozov · 2020-10-28T16:53:35Z

@BenMorel I believe the wording is not that critical but the upgrade notes are still missing.

BenMorel · 2020-10-29T21:49:30Z

@morozov Done!

By the way, shouldn't ExceptionConverter and DriverException::__construct() be internal?

morozov · 2020-10-29T22:54:35Z

By the way, shouldn't ExceptionConverter and DriverException::__construct() be internal?

That would make sense, I think. I believe all exception constructors should be internal (not in this patch) since only the DBAL is supposed to instantiate them.

BenMorel · 2020-10-29T22:59:13Z

That would make sense, I think. I believe all exception constructors should be internal (not in this patch) since only the DBAL is supposed to instantiate them.

Should I make at least these 2 @internal? Or we'll have to revert the changelog later.

morozov · 2020-10-29T23:07:20Z

Should I make at least these 2 @internal? Or we'll have to revert the changelog later.

Yeah, let's do, and update the upgrade notes accordingly. The wording changes look good to me as well, so it may be also the time to squash everything.

BenMorel · 2020-10-29T23:54:49Z

@morozov Done!

morozov

I'm sorry. I misread your question in #4387 (comment) and thought you were asking about constructors in both cases.

src/Driver/API/ExceptionConverter.php

src/Exception/DriverException.php

BenMorel · 2020-10-30T10:50:07Z

@morozov I fixed this in 06732c5, please let me know if everything's ok now and I'll squash.

morozov · 2020-10-30T14:36:33Z

Looks good.

BenMorel · 2020-10-30T16:50:32Z

Squashed!

morozov · 2020-11-01T16:07:06Z

Thanks, @BenMorel!

BenMorel commented Oct 24, 2020

View reviewed changes

tests/Connection/ExceptionHandlingTest.php Outdated Show resolved Hide resolved

BenMorel mentioned this pull request Oct 24, 2020

Remove implicit ext-json dependency #4325

Closed

morozov reviewed Oct 25, 2020

View reviewed changes

src/Exception/DriverException.php Outdated Show resolved Hide resolved

src/Connection.php Outdated Show resolved Hide resolved

tests/Connection/ExceptionHandlingTest.php Outdated Show resolved Hide resolved

BenMorel commented Oct 26, 2020

View reviewed changes

src/Query.php Show resolved Hide resolved

BenMorel commented Oct 26, 2020

View reviewed changes

src/Query.php Outdated Show resolved Hide resolved

morozov reviewed Oct 26, 2020

View reviewed changes

src/Query.php Outdated Show resolved Hide resolved

src/Query.php Outdated Show resolved Hide resolved

src/Query.php Outdated Show resolved Hide resolved

src/Query.php Show resolved Hide resolved

src/Connection.php Outdated Show resolved Hide resolved

morozov added the Error Handling label Oct 26, 2020

morozov reviewed Oct 27, 2020

View reviewed changes

BenMorel force-pushed the exception-params branch from 1b08200 to 0bb0233 Compare October 27, 2020 15:55

BenMorel changed the base branch from master to 3.0.x October 27, 2020 15:55

BenMorel force-pushed the exception-params branch from c1c6281 to 73abd2e Compare October 29, 2020 23:22

morozov reviewed Oct 30, 2020

View reviewed changes

src/Driver/API/ExceptionConverter.php Outdated Show resolved Hide resolved

src/Exception/DriverException.php Outdated Show resolved Hide resolved

BenMorel force-pushed the exception-params branch from 06732c5 to a3490cd Compare October 30, 2020 16:50

morozov previously approved these changes Oct 30, 2020

View reviewed changes

morozov added this to the 3.0.0 milestone Oct 30, 2020

Store query in driver exceptions

26fe027

morozov dismissed their stale review via 26fe027 November 1, 2020 15:40

morozov force-pushed the exception-params branch from a3490cd to 26fe027 Compare November 1, 2020 15:40

morozov approved these changes Nov 1, 2020

View reviewed changes

morozov merged commit 83113ce into doctrine:3.0.x Nov 1, 2020

morozov mentioned this pull request Jul 9, 2021

Truncate params in DBALException::formatParameters #2524

Closed

morozov mentioned this pull request Jul 26, 2021

Error handling fails with memory meltdown when manipulating large data structures (like blob/binary) #3625

Closed

morozov mentioned this pull request Dec 12, 2021

Truncate Doctrine\DBAL\Exception\DriverException message if too long #2523

Closed

github-actions bot locked as resolved and limited conversation to collaborators Jul 27, 2022

Uh oh!

Conversation

BenMorel commented Oct 24, 2020

Summary

Uh oh!

Uh oh!

BenMorel commented Oct 24, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

BenMorel commented Oct 26, 2020

Uh oh!

morozov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

morozov commented Oct 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

morozov left a comment

Choose a reason for hiding this comment

Uh oh!

morozov Oct 27, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

morozov commented Oct 27, 2020

Uh oh!

BenMorel commented Oct 27, 2020

Uh oh!

morozov commented Oct 28, 2020

Uh oh!

BenMorel commented Oct 29, 2020

Uh oh!

morozov commented Oct 29, 2020

Uh oh!

BenMorel commented Oct 29, 2020

Uh oh!

morozov commented Oct 29, 2020

Uh oh!

BenMorel commented Oct 29, 2020

Uh oh!

morozov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

BenMorel commented Oct 30, 2020

Uh oh!

morozov commented Oct 30, 2020

Uh oh!

BenMorel commented Oct 30, 2020

Uh oh!

morozov commented Nov 1, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

morozov commented Oct 26, 2020 •

edited

Loading