Fix QueryPlanAnalyser with dynamic offset and limit params #390
Conversation
The QueryPlanAnalyser does not strip trailing limits and offsets. If limits and offsets are set via parameters it would escape the limits and offsets as strings. This resulted in incorrect SQL being analysed.
|
I'm not sure about the changes to the cache files here 🤔 i did a |
| @@ -306,7 +306,7 @@ private function replaceParameters(string $queryString, array $parameters): stri | |||
| foreach ($parameters as $placeholderKey => $parameter) { | |||
| $value = $parameter->simulatedValue; | |||
|
|
|||
| if (\is_string($value)) { | |||
| if (\is_string($value) && !is_numeric($value)) { | |||
There was a problem hiding this comment.
not sure if this is the right approach here, as it would not escape numeric values at all now 🤔
And at least one test expects that numeric values in the WHERE clause are escaped
Another approach would be to strip the limit and offset of queries here too
| @@ -19,7 +19,12 @@ public function noindexDbal(Connection $conn): void | |||
|
|
|||
| public function noindexPreparedDbal(Connection $conn, string $email): void | |||
| { | |||
| $conn->executeQuery('SELECT * FROM ada WHERE email = ?', [$email]); | |||
| $conn->executeQuery('SELECT * FROM ada WHERE email = ? LIMIT 5 OFFSET 2', [$email]); | |||
There was a problem hiding this comment.
please don't change existing tests.. just add new ones
|
I guess the underlying problem is the same as that regular prepared statements do not work properly yet, see #385 which stems from that we don't pass the resolved query string thru |
|
I guess we are still missing some cases, but its already better then before. Thank you |
|
fix released in https://github.com/staabm/phpstan-dba/releases/tag/0.2.34 - thx @keulinho |
|
Thx for your help! ✌️ |
|
@keulinho btw, looks like shopware is using phpstan-dba? do you have any experience to share? feel free to open issues as you find some |
|
Yep we started using this extension last week for shopware 6, because we like the idea of static analysing your DB queries. But as shopware 6 comes with it own We mostly use plain SQL queries in tests, migrations of performance critical operations, and it is super cool to have this kind of analysis for the latter two cases. Currently most issues identified were passing unnecessary params when executing an query, but i'm quite curious where this thing is heading too, looks really promising. |
if you find examples of this thing, please open a issue. when it e.g. is similar to doctrine dba, it should be easy to re-use existing phpstan-dba functionality for your custom api, see e.g. https://github.com/staabm/phpstan-dba/blob/main/docs/rules.md
I really like this kind of finds, as it often times means you can further simplify methods after you realized parameters are unnecessary |
I was not aware that this is possible, looks cool for frameworks that work on SQL basis, but with shopware 6 we tried to build an abstraction around SQL, e.g. you define the data you want to fetch with a |
I create phpstan extensions for kind of things in the past, which in turn allows to analyze such dynamic built queries even on phpstan-dba, see e.g. https://github.com/staabm/zf-select-strip/blob/a62603941cb2a2aeba6ba121318b93fb93c57430/tests/phpstan/data/zf-select.php#L129-L140 |
The QueryPlanAnalyser does not strip trailing limits and offsets.
If limits and offsets are set via parameters it would escape the limits and offsets as strings.
This resulted in incorrect SQL being analysed.