Skip to content

Allow placeholder validation for write queries #506

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
staabm merged 2 commits into from
Feb 15, 2023
Merged

Allow placeholder validation for write queries #506

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4791bbc
Allow placeholder validation for write queries
hemberger Feb 13, 2023
d9693d9
Merge branch 'main' into pr/506
staabm Feb 15, 2023
File filter

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 0 additions & 4 deletions src/QueryReflection/PlaceholderValidation.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,10 +32,6 @@ public function checkQuery(Expr $queryExpr, Scope $scope, array $parameters): it
*/
private function checkErrors(string $queryString, array $parameters): iterable
{
if ('SELECT' !== QueryReflection::getQueryType($queryString)) {
return;
}

$queryReflection = new QueryReflection();
if ($queryReflection->containsNamedPlaceholders($queryString, $parameters)) {
yield from $this->validateNamedPlaceholders($queryString, $parameters);
Expand Down
12 changes: 12 additions & 0 deletions tests/rules/SyntaxErrorInPreparedStatementMethodRuleTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,10 @@ public function testSyntaxErrorInQueryRule(): void
"Query error: You have an error in your SQL syntax; check the manual that corresponds to your MySQL/MariaDB server version for the right syntax to use near 'gesperrt freigabe1u1 FROM ada LIMIT 0' at line 1 (1064).",
319,
],
[
'Query expects placeholder :freigabe1u1, but it is missing from values given.',
325,
],
];
} elseif (PdoPgSqlQueryReflector::NAME === getenv('DBA_REFLECTOR')) {
$expectedErrors = [
Expand Down Expand Up @@ -157,6 +161,10 @@ public function testSyntaxErrorInQueryRule(): void
^ (42601).',
319,
],
[
'Query expects placeholder :freigabe1u1, but it is missing from values given.',
Copy link
Owner

@staabm staabm Feb 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please check the example in this file (which is the one tested here).

$connection->preparedQuery('INSERT into ada(adaid, gesperrt, email,freigabe1u1) values(:adaid, :gesperrt, :email, :freigabe1u1)', ['adaid' => 1, 'gesperrt' => 0, 'email' => 'test@github.com', 0]);

I guess this error is not correct, since the last value of the array will be handled by PDO as the last value of the column list.

please also add new test-cases (at the very end of the file) which show that the rule works for writable queries as you expect it (and also cover cases which it should not error about)
Copy link
Contributor Author

@hemberger hemberger Feb 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this query is invalid (because it names a parameter :freigabe1u1 that it does not define), and the new warning is correct. In my testing, I get the following error if I actually try to execute this query:

PDOException: SQLSTATE[HY093]: Invalid parameter number: parameter was not defined

The write queries that are already part of the test suite seem to cover all the cases I can think of. I'm happy to add more if you feel that some category has been omitted. Please let me know!
Copy link
Owner

@staabm staabm Feb 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, thx for testing. we can merge after conflicts are resolved then.
325,
],
];
} elseif (PdoMysqlQueryReflector::NAME === getenv('DBA_REFLECTOR')) {
$expectedErrors = [
Expand Down Expand Up @@ -208,6 +216,10 @@ public function testSyntaxErrorInQueryRule(): void
"Query error: SQLSTATE[42000]: Syntax error or access violation: 1064 You have an error in your SQL syntax; check the manual that corresponds to your MySQL/MariaDB server version for the right syntax to use near 'gesperrt freigabe1u1 FROM ada LIMIT 0' at line 1 (42000).",
319,
],
[
'Query expects placeholder :freigabe1u1, but it is missing from values given.',
325,
],
];
} else {
throw new \RuntimeException('Unsupported DBA_REFLECTOR '.getenv('DBA_REFLECTOR'));
Expand Down