support for cross joins. don't error on joins of subqueries #569

jakubvojacek · 2023-03-15T09:43:44Z

support for multiple joins, up to now it was OK just for single join, this adds support for multiple
skips AST for
- cross joins
- queries with subqueries
- when condition is null (previous it was throwing runtime exception, now it just screams in debug)

The changes works fine for my codebase

staabm · 2023-03-15T09:50:11Z

changes look good to me, but require test-cases which fail without the actual code changes

jakubvojacek · 2023-03-15T09:52:48Z

changes look good to me, but require test-cases which fail without the actual code changes

yea, I will try to add some

staabm · 2023-03-15T11:19:11Z

tests/default/data/sql-ast-narrowing.php

@@ -365,4 +365,18 @@ public function joinSelectOutsideCondition(PDO $pdo): void
        assertType('PDOStatement<array{adaid: int<-32768, 32767>, 0: int<-32768, 32767>, eadavk: numeric-string|null, 1: numeric-string|null}>', $stmt);
    }

+    public function multipleJoins(PDO $pdo): void
+    {
+        $stmt = $pdo->query('SELECT adaid from ada join ak on adaid = eladaid inner join typemix on adaid = c_int');


Please also select columns from within the join condition, make sure these are narrowed properly and make sure columns from joined tables not part of the condition are also narrowed properly.

I tried, please check, is that you you had in mind?

staabm · 2023-03-16T07:44:28Z

src/SqlAst/ParserInference.php

+                        if ($from->getRight() instanceof TableReferenceSubquery || $from->getLeft() instanceof TableReferenceSubquery) {
+                            if (QueryReflection::getRuntimeConfiguration()->isDebugEnabled()) {
+                                throw new UnresolvableQueryMixedTypeException('Cannot narrow down types for SQLs with subqueries: ' . $queryString);
+                            }
+
+                            return $resultType;
+                        }
+
+                        if ($from instanceof InnerJoin && $from->isCrossJoin()) {
+                            if (QueryReflection::getRuntimeConfiguration()->isDebugEnabled()) {
+                                throw new UnresolvableQueryMixedTypeException('Cannot narrow down types for cross joins: ' . $queryString);
+                            }
+
+                            return $resultType;
+                        }


why do we need these early returns?

it cannot collect proper data about joins and the columns hence it gives up on narrowing down the type

but would it make a difference for the added test-cases if we just drop it here. wouldn't it give up after the loop then?

without the early return, it wouldnt work, the tests would fail

1) Error The data provider specified for staabm\PHPStanDba\Tests\DbaInferenceTest::testFileAsserts is invalid. Error: Call to undefined method SqlFtw\Sql\Dml\TableReference\TableReferenceSubquery::getTable() /Users/jakubvojacek/Sites/phpstan-dba/src/SqlAst/ParserInference.php:96 /Users/jakubvojacek/Sites/phpstan-dba/src/QueryReflection/QueryReflection.php:135 /Users/jakubvojacek/Sites/phpstan-dba/src/PdoReflection/PdoStatementReflection.php:88 /Users/jakubvojacek/Sites/phpstan-dba/src/Extensions/PdoQueryDynamicReturnTypeExtension.php:106

ahh I see.

lets keep it then, but add a new exception type to use instead of UnresolvableQueryMixedTypeException here.
the tip for the exception should make the user aware that he triggered a case not yet supported and that it would be awesome if a repro can be reported to our issue tracker

but that will trigger phpstan internal error and the analysis will fail and phpstan cache wont be saved,right?I originally had the exception there but it was so annoying when running on my codebase that I decided to hide it behind debug flag.

I think it should work when we create a new subclass of UnresolvableQueryException

similar to how we did with https://github.com/staabm/phpstan-dba/blob/main/src/UnresolvableQueryMixedTypeException.php which seems to work for you atm

e.g. UnresolvableAstInQueryException

so you want me to rename UnresolvableQueryMixedTypeException to UnresolvableAstInQueryException on those three places that it thrown in ParserInference right?

I checked where UnresolvableQueryMixedTypeException was used and its only on one place and its after the QueryReflection::getRuntimeConfiguration()->isDebugEnabled() condition as well

nope, I want you to create a new exception class and use it here in this few places of this PR, but leave UnresolvableQueryMixedTypeException as is across the codebase

UnresolvableQueryMixedTypeException signals that a query cannot be resolved - meaning we are not able to figure out the query string at all.
in our scenario here, we already know the query string but are not (yet?) able to analyse it with AST properly (since we did not yet implement everything you can express with sql)

tests/default/data/sql-ast-narrowing.php

staabm · 2023-03-16T07:46:35Z

tests/default/data/sql-ast-narrowing.php

+
+    public function ignoredAstQueries(PDO $pdo): void
+    {
+        $pdo->query('SELECT adaid from ada inner join (select akid from ak) on akid = adaid');


we are missing a assertion for this line

because it has a different output for pdo-mysql and mysqli reflectors (but that is unrelated to AST analysis since we give up on that due to the subquery)

without an assertion this line does not have value. what is the difference between the reflectors regarding this statement?

my bad, must have been doing something wrong back then when i was testing. I just pushed a new assertion, hopefully CI will be green

staabm

thank you

staabm reviewed Mar 15, 2023

View reviewed changes

staabm reviewed Mar 16, 2023

View reviewed changes

tests/default/data/sql-ast-narrowing.php Show resolved Hide resolved

staabm reviewed Mar 16, 2023

View reviewed changes

jakubvojacek and others added 8 commits March 16, 2023 09:26

wip

efc37ee

Fix code style

f77437d

Added basic tests

a3a45c6

Added tests for multiple joints (not)-nullable columns

4ce5337

Forgot to test other column

15aeaf4

Missing assertion

02ffe01

Added UnresolvableAstInQueryException exception

c03b9e3

Update UnresolvableAstInQueryException.php

161c3b5

staabm approved these changes Mar 16, 2023

View reviewed changes

staabm changed the title ~~AST improvements~~ support for cross joins. don't error on joins of subqueries Mar 16, 2023

staabm enabled auto-merge (squash) March 16, 2023 08:30

staabm added this to the SQL AST milestone Mar 16, 2023

staabm merged commit 0d7b290 into staabm:main Mar 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support for cross joins. don't error on joins of subqueries #569

support for cross joins. don't error on joins of subqueries #569

jakubvojacek commented Mar 15, 2023 •

edited

Loading

staabm commented Mar 15, 2023

jakubvojacek commented Mar 15, 2023

staabm Mar 15, 2023 •

edited

Loading

jakubvojacek Mar 15, 2023

staabm Mar 16, 2023

jakubvojacek Mar 16, 2023

staabm Mar 16, 2023 •

edited

Loading

jakubvojacek Mar 16, 2023

staabm Mar 16, 2023

jakubvojacek Mar 16, 2023

staabm Mar 16, 2023

staabm Mar 16, 2023

jakubvojacek Mar 16, 2023

staabm Mar 16, 2023 •

edited

Loading

staabm Mar 16, 2023 •

edited

Loading

jakubvojacek Mar 16, 2023

staabm Mar 16, 2023

jakubvojacek Mar 16, 2023

staabm left a comment

support for cross joins. don't error on joins of subqueries #569

support for cross joins. don't error on joins of subqueries #569

Conversation

jakubvojacek commented Mar 15, 2023 • edited Loading

staabm commented Mar 15, 2023

jakubvojacek commented Mar 15, 2023

staabm Mar 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

staabm Mar 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

staabm Mar 16, 2023 • edited Loading

Choose a reason for hiding this comment

staabm Mar 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

staabm left a comment

Choose a reason for hiding this comment

jakubvojacek commented Mar 15, 2023 •

edited

Loading

staabm Mar 15, 2023 •

edited

Loading

staabm Mar 16, 2023 •

edited

Loading

staabm Mar 16, 2023 •

edited

Loading

staabm Mar 16, 2023 •

edited

Loading