Throw QueryException on improperly initialized from clause #4219

nreynis · 2020-08-21T20:50:57Z

Q	A
Type	improvement
BC Break	no
Fixed issues	#3601

Summary

Throw a QueryException when using the query builder improperly.

lib/Doctrine/DBAL/Query/QueryBuilder.php

lib/Doctrine/DBAL/Query/QueryException.php

tests/Doctrine/Tests/DBAL/Query/QueryBuilderTest.php

lib/Doctrine/DBAL/Query/QueryBuilder.php

greg0ire · 2020-08-22T09:48:13Z

I have asked you some changes, and had a quick look at the CI, these issues look like they could be transient. When you address the changes I asked, that will trigger a new build and we will see if they are indeed transient or not.

greg0ire · 2020-08-22T12:16:07Z

Looks like the build issues were indeed transient 👍

morozov

2.10.x looks like a wrong target branch. It's not a bug in a way that something doesn't work as documented, and it may be a breaking change since it throws new exceptions. Please reconsider the target branch.

morozov · 2020-08-22T15:17:50Z

tests/Doctrine/Tests/DBAL/Query/QueryBuilderTest.php

+            ->from('users');
+        $this->expectException(QueryException::class);
+        $this->expectExceptionMessage(
+            'There is no currently registered FROM clause.'


I don't get this exception message. Why should there be a FROM clause in an UPDATE statement?

Changed the error message to be more focused on the functional part, and less on the internal structure.

morozov · 2020-08-22T15:18:41Z

tests/Doctrine/Tests/DBAL/Query/QueryBuilderTest.php

+            ->from('users');
+        $this->expectException(QueryException::class);
+        $this->expectExceptionMessage(
+            'There is no currently registered FROM clause.'


Why should there be a FROM clause in an INSERT statement?

Changed the error message to be more focused on the functional part, and less on the internal structure.

lib/Doctrine/DBAL/Query/QueryException.php

nreynis · 2020-08-22T19:37:53Z

2.10.x looks like a wrong target branch. It's not a bug in a way that something doesn't work as documented, and it may be a breaking change since it throws new exceptions. Please reconsider the target branch.

It can't really be a breaking change: the existing code already throw. The goal is more to handle the misuse and return a less confusing exception. Also execute and getSQL methods already throw a DBALException.

The only way it could break something is if it were deliberately abusing the QueryBuilder to make it throw a specific exception.

But I have no problem targeting any branch you see fit.

morozov · 2020-08-22T23:02:57Z

It can't really be a breaking change: the existing code already throw.

Thanks for the confirmation. Then it's safe to have these changes in 2.11.0.

.idea/.gitignore

morozov · 2020-09-13T23:25:30Z

Please retarget (#4219 (comment)).

greg0ire

@morozov should OP do :%s/TableName/Table/?

greg0ire · 2020-09-14T06:19:52Z

lib/Doctrine/DBAL/Query/QueryBuilder.php

+    /**
+     * @throws QueryException
+     */
+    private function assertHasSingleFromClause(): void


Suggested change

private function assertHasSingleFromClause(): void

private function assertHasSingleTableName(): void

greg0ire · 2020-09-14T06:21:00Z

lib/Doctrine/DBAL/Query/QueryBuilder.php

+     */
+    private function assertHasSingleFromClause(): void
+    {
+        if (! array_key_exists('table', $this->sqlParts['from'])) {


Suggested change

if (! array_key_exists('table', $this->sqlParts['from'])) {

/* INSERT and UPDATE statements do not use FROM clause

but the table name is still stored in $this->sqlParts['from']

in these cases */

if (! array_key_exists('table', $this->sqlParts['from'])) {

greg0ire · 2020-09-14T06:21:27Z

lib/Doctrine/DBAL/Query/QueryBuilder.php

     */
    private function getSQLForInsert()
    {
+        $this->assertHasSingleFromClause();


Suggested change

$this->assertHasSingleFromClause();

$this->assertHasSingleTableName();

greg0ire · 2020-09-14T06:21:38Z

lib/Doctrine/DBAL/Query/QueryBuilder.php

     */
    private function getSQLForUpdate()
    {
+        $this->assertHasSingleFromClause();


Suggested change

$this->assertHasSingleFromClause();

$this->assertHasSingleTableName();

greg0ire · 2020-09-14T06:21:49Z

lib/Doctrine/DBAL/Query/QueryBuilder.php

     */
    private function getSQLForDelete()
    {
+        $this->assertHasSingleFromClause();


Suggested change

$this->assertHasSingleFromClause();

$this->assertHasSingleTableName();

greg0ire · 2020-09-14T06:22:54Z

lib/Doctrine/DBAL/Query/QueryException.php

+
+    public static function uninitializedTableName(): self
+    {
+        return new self('There is no currently registered table name.');


That message is misleading, it will be used when there are 2 registered table names. The name of the method is misleading too.

Have you a suggestion?

morozov · 2020-09-14T16:19:29Z

@morozov should OP do :%s/TableName/Table/?

Makes sense. I'm not going to spend much time on this personally (#4220 (comment)).

nreynis force-pushed the 3601-query-exception-improper-from branch 3 times, most recently from 4ffb029 to 4e3b8b8 Compare August 21, 2020 20:59

greg0ire reviewed Aug 22, 2020

View reviewed changes

nreynis force-pushed the 3601-query-exception-improper-from branch 2 times, most recently from 1135c14 to 211366d Compare August 22, 2020 12:06

greg0ire previously approved these changes Aug 22, 2020

View reviewed changes

greg0ire requested a review from morozov August 22, 2020 12:17

morozov requested changes Aug 22, 2020

View reviewed changes

greg0ire mentioned this pull request Aug 23, 2020

Use better typing in QueryBuilder #4220

Closed

nreynis dismissed greg0ire’s stale review via ebfc170 September 13, 2020 20:36

nreynis force-pushed the 3601-query-exception-improper-from branch from 211366d to ebfc170 Compare September 13, 2020 20:36

greg0ire reviewed Sep 13, 2020

View reviewed changes

.idea/.gitignore Outdated Show resolved Hide resolved

nreynis force-pushed the 3601-query-exception-improper-from branch from ebfc170 to 9bb0e7c Compare September 13, 2020 20:41

morozov added Error Handling QueryBuilder labels Sep 13, 2020

Throw QueryException on improperly initialized from clause

6cd4e05

nreynis force-pushed the 3601-query-exception-improper-from branch from 9bb0e7c to 6cd4e05 Compare September 14, 2020 05:12

nreynis changed the base branch from 2.10.x to 2.11.x September 14, 2020 05:12

greg0ire reviewed Sep 14, 2020

View reviewed changes

morozov changed the base branch from 2.11.x to 2.12.x November 29, 2020 21:38

morozov closed this Apr 21, 2021

morozov deleted the branch doctrine:2.12.x April 21, 2021 16:26

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

	private function assertHasSingleFromClause(): void
	private function assertHasSingleTableName(): void

-        if (! array_key_exists('table', $this->sqlParts['from'])) {
+        /* INSERT and UPDATE statements do not use FROM clause
+           but the table name is still stored in $this->sqlParts['from']
+           in these cases */
+        if (! array_key_exists('table', $this->sqlParts['from'])) {

	$this->assertHasSingleFromClause();
	$this->assertHasSingleTableName();

Uh oh!

Throw QueryException on improperly initialized from clause #4219

Throw QueryException on improperly initialized from clause #4219

Uh oh!

Conversation

nreynis commented Aug 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

greg0ire commented Aug 22, 2020

Uh oh!

greg0ire commented Aug 22, 2020

Uh oh!

morozov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nreynis commented Aug 22, 2020

Uh oh!

morozov commented Aug 22, 2020

Uh oh!

Uh oh!

morozov commented Sep 13, 2020

Uh oh!

greg0ire left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

morozov commented Sep 14, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

nreynis commented Aug 21, 2020 •

edited

Loading