Skip to content

Warn when queries cannot be resolved unexpectedly #508

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
merged 5 commits into from
Feb 19, 2023
Merged

Warn when queries cannot be resolved unexpectedly #508

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ad62a5c
Warn when queries cannot be resolved unexpectedly
hemberger Feb 13, 2023
b2a7620
Update src/UnresolvableQueryStringTypeException.php
hemberger Feb 16, 2023
940112b
fixup: refactor early return into QueryReflection::isResolvable
hemberger Feb 16, 2023
9be05c9
Merge branch 'main' into issue-504
staabm Feb 19, 2023
a706a5f
cs
staabm Feb 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions src/QueryReflection/QueryReflection.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,11 @@
use PhpParser\Node\Scalar\EncapsedStringPart;
use PHPStan\Analyser\Scope;
use PHPStan\ShouldNotHappenException;
use PHPStan\TrinaryLogic;
use PHPStan\Type\Constant\ConstantArrayType;
use PHPStan\Type\Constant\ConstantIntegerType;
use PHPStan\Type\Constant\ConstantStringType;
use PHPStan\Type\StringType;
use PHPStan\Type\Type;
use PHPStan\Type\TypeUtils;
use PHPStan\Type\UnionType;
Expand Down Expand Up @@ -99,6 +101,27 @@ public function getResultType(string $queryString, int $fetchType): ?Type
return self::reflector()->getResultType($queryString, $fetchType);
}

/**
* Determine if a query will be resolvable.
*
* - If yes, the query is a literal string.
* - If no, the query is a non-literal string or mixed type.
* - If maybe, the query is neither of the two.
*
* We will typically skip processing of queries that return no, which are
* likely part of a software abstraction layer that we know nothing about.
*/
public function isResolvable(Expr $queryExpr, Scope $scope): TrinaryLogic
{
$type = $scope->getType($queryExpr);
if ($type->isLiteralString()->yes()) {
return TrinaryLogic::createYes();
}
$isStringOrMixed = $type->isSuperTypeOf(new StringType());

return $isStringOrMixed->negate();
Comment on lines +206 to +208
Copy link
Owner

@staabm staabm Feb 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I would prefer something like

Suggested change
$isStringOrMixed = $type->isSuperTypeOf(new StringType());
return $isStringOrMixed->negate();
if ($type instanceof MixedType) {
return TrinaryLogic::createNo();
}
return $type->isString();

Copy link
Owner

Choose a reason for hiding this comment

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

I am pretty sure the logic in my suggestion is wrong, but I think you get the point.

otherwise the PR looks good, thank you

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I'm actually having a bit of trouble with this logic. Do you think you could give me some pointers?

Basically, the logic that I'm trying to encapsulate is that if any part of the string is a literal string, then we don't want to skip the query, because there is literal SQL that we should be checking. Then either it will resolve and be checked, or we'll warn that it doesn't resolve (with this PR). For example, I'd want the warning for the following:

/** @var string $string */
$query = 'SELECT * FROM table WHERE ' . $string;

Pre-existing tests explicitly state that entirely non-literal string and mixed types should not be checked, e.g.:

/** @var string $string */
$query = $string;

with the reasoning that this is likely part of a s/w abstraction layer that phpstan-dba won't know about, which is reasonable.

But how does one distinguish between $string and $literal . $string? I was thinking that the former is a StringType and the latter is an IntersectionType, but many non-literal string variables are an IntersectionType with an Accessory, e.g. if $string is a non-empty-string. Is there any way to robustly do what I'm trying to do with the current phpstan type system, or is my idea fundamentally flawed?

To be clear, the edge cases that my attempts fail at look like, e.g.

/** @var non-empty-string $string */
$query = $string;

I still think this is an important feature, especially for people modernizing old codebases that didn't use prepared queries. Enabling this warning has uncovered many unresolvable queries that I didn't find by hand (including ones that were already migrated to prepared statements, but unexpectedly used a non-literal string in the query construction).

If there is no such solution, I would honestly be totally happy with an option to just warn for all unresolvable queries (with the idea that it's safer to explicitly ignore an unwanted warning than silently miss a bunch of real errors). But I look to you for guidance here, before I go too far off the deep end. :)

Thanks again for your time!

Copy link
Owner

@staabm staabm Feb 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I agree with the goal and the results of this PR.

my suggestion was more about how to make the logic in isResolvable more readable without changing what it is doing.

but since you added extensive test coverage, I will just merge it, after merge conflicts are resolved.

sorry for the noise

}

/**
* @return iterable<string>
*
Expand Down
11 changes: 8 additions & 3 deletions src/QueryReflection/QuerySimulation.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,8 @@
use PHPStan\Type\UnionType;
use PHPStan\Type\VerbosityLevel;
use staabm\PHPStanDba\DbaException;
use staabm\PHPStanDba\UnresolvableQueryException;
use staabm\PHPStanDba\UnresolvableQueryMixedTypeException;
use staabm\PHPStanDba\UnresolvableQueryStringTypeException;

/**
* @internal
Expand All @@ -30,7 +31,7 @@ final class QuerySimulation
private const DATE_FORMAT = 'Y-m-d';

/**
* @throws UnresolvableQueryException
* @throws \staabm\PHPStanDba\UnresolvableQueryException
*/
public static function simulateParamValueType(Type $paramType, bool $preparedParam): ?string
{
Expand Down Expand Up @@ -94,6 +95,10 @@ public static function simulateParamValueType(Type $paramType, bool $preparedPar
}

// plain string types can contain anything.. we cannot reason about it
if (QueryReflection::getRuntimeConfiguration()->isDebugEnabled()) {
throw new UnresolvableQueryStringTypeException('Cannot resolve query with variable type: '.$paramType->describe(VerbosityLevel::precise()));
}

return null;
}

Expand All @@ -113,7 +118,7 @@ public static function simulateParamValueType(Type $paramType, bool $preparedPar
// all types which we can't simulate and render a query unresolvable at analysis time
if ($paramType instanceof MixedType || $paramType instanceof IntersectionType) {
if (QueryReflection::getRuntimeConfiguration()->isDebugEnabled()) {
throw new UnresolvableQueryException('Cannot simulate parameter value for type: '.$paramType->describe(VerbosityLevel::precise()));
throw new UnresolvableQueryMixedTypeException('Cannot simulate parameter value for type: '.$paramType->describe(VerbosityLevel::precise()));
}

return null;
Expand Down
7 changes: 3 additions & 4 deletions src/Rules/PdoStatementExecuteMethodRule.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
use PHPStan\Type\Constant\ConstantArrayType;
use PHPStan\Type\Constant\ConstantIntegerType;
use PHPStan\Type\Constant\ConstantStringType;
use PHPStan\Type\MixedType;
use staabm\PHPStanDba\PdoReflection\PdoStatementReflection;
use staabm\PHPStanDba\QueryReflection\PlaceholderValidation;
use staabm\PHPStanDba\QueryReflection\QueryReflection;
Expand Down Expand Up @@ -67,7 +66,7 @@ private function checkErrors(MethodReflection $methodReflection, MethodCall $met
if (null === $queryExpr) {
return [];
}
if ($scope->getType($queryExpr) instanceof MixedType) {
if ($queryReflection->isResolvable($queryExpr, $scope)->no()) {
return [];
}

Expand Down Expand Up @@ -98,7 +97,7 @@ private function checkErrors(MethodReflection $methodReflection, MethodCall $met
$parameters = $queryReflection->resolveParameters($parameterTypes) ?? [];
} catch (UnresolvableQueryException $exception) {
return [
RuleErrorBuilder::message($exception->asRuleMessage())->tip(UnresolvableQueryException::RULE_TIP)->line($methodCall->getLine())->build(),
RuleErrorBuilder::message($exception->asRuleMessage())->tip($exception::getTip())->line($methodCall->getLine())->build(),
];
}

Expand All @@ -111,7 +110,7 @@ private function checkErrors(MethodReflection $methodReflection, MethodCall $met
}
} catch (UnresolvableQueryException $exception) {
return [
RuleErrorBuilder::message($exception->asRuleMessage())->tip(UnresolvableQueryException::RULE_TIP)->line($methodCall->getLine())->build(),
RuleErrorBuilder::message($exception->asRuleMessage())->tip($exception::getTip())->line($methodCall->getLine())->build(),
];
}

Expand Down
16 changes: 3 additions & 13 deletions src/Rules/QueryPlanAnalyzerRule.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
use PHPStan\Rules\RuleError;
use PHPStan\Rules\RuleErrorBuilder;
use PHPStan\ShouldNotHappenException;
use PHPStan\Type\MixedType;
use PHPStan\Type\ObjectType;
use staabm\PHPStanDba\QueryReflection\QueryReflection;
use staabm\PHPStanDba\Tests\QueryPlanAnalyzerRuleTest;
Expand Down Expand Up @@ -89,20 +88,11 @@ public function processNode(Node $callLike, Scope $scope): array
return [];
}

$args = $callLike->getArgs();
if (!\array_key_exists($queryArgPosition, $args)) {
return [];
}

if ($scope->getType($args[$queryArgPosition]->value) instanceof MixedType) {
return [];
}

try {
return $this->analyze($callLike, $scope);
} catch (UnresolvableQueryException $exception) {
return [
RuleErrorBuilder::message($exception->asRuleMessage())->tip(UnresolvableQueryException::RULE_TIP)->line($callLike->getLine())->build(),
RuleErrorBuilder::message($exception->asRuleMessage())->tip($exception::getTip())->line($callLike->getLine())->build(),
];
}
}
Expand All @@ -125,8 +115,9 @@ private function analyze(CallLike $callLike, Scope $scope): array
}

$queryExpr = $args[0]->value;
$queryReflection = new QueryReflection();

if ($scope->getType($queryExpr) instanceof MixedType) {
if ($queryReflection->isResolvable($queryExpr, $scope)->no()) {
return [];
}

Expand All @@ -136,7 +127,6 @@ private function analyze(CallLike $callLike, Scope $scope): array
}

$ruleErrors = [];
$queryReflection = new QueryReflection();
$proposal = "\n\nConsider optimizing the query.\nIn some cases this is not a problem and this error should be ignored.";

foreach ($queryReflection->analyzeQueryPlan($scope, $queryExpr, $parameterTypes) as $queryPlanResult) {
Expand Down
10 changes: 4 additions & 6 deletions src/Rules/SyntaxErrorInPreparedStatementMethodRule.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
use PHPStan\Rules\RuleError;
use PHPStan\Rules\RuleErrorBuilder;
use PHPStan\ShouldNotHappenException;
use PHPStan\Type\MixedType;
use PHPStan\Type\ObjectType;
use staabm\PHPStanDba\QueryReflection\PlaceholderValidation;
use staabm\PHPStanDba\QueryReflection\QueryReflection;
Expand Down Expand Up @@ -101,21 +100,20 @@ private function checkErrors(CallLike $callLike, Scope $scope): array
}

$queryExpr = $args[0]->value;
$queryReflection = new QueryReflection();

if ($scope->getType($queryExpr) instanceof MixedType) {
if ($queryReflection->isResolvable($queryExpr, $scope)->no()) {
return [];
}

$queryReflection = new QueryReflection();

$parameters = null;
if (\count($args) > 1) {
$parameterTypes = $scope->getType($args[1]->value);
try {
$parameters = $queryReflection->resolveParameters($parameterTypes) ?? [];
} catch (UnresolvableQueryException $exception) {
return [
RuleErrorBuilder::message($exception->asRuleMessage())->tip(UnresolvableQueryException::RULE_TIP)->line($callLike->getLine())->build(),
RuleErrorBuilder::message($exception->asRuleMessage())->tip($exception::getTip())->line($callLike->getLine())->build(),
];
}
}
Expand Down Expand Up @@ -152,7 +150,7 @@ private function checkErrors(CallLike $callLike, Scope $scope): array
return $ruleErrors;
} catch (UnresolvableQueryException $exception) {
return [
RuleErrorBuilder::message($exception->asRuleMessage())->tip(UnresolvableQueryException::RULE_TIP)->line($callLike->getLine())->build(),
RuleErrorBuilder::message($exception->asRuleMessage())->tip($exception::getTip())->line($callLike->getLine())->build(),
];
}
}
Expand Down
11 changes: 6 additions & 5 deletions src/Rules/SyntaxErrorInQueryFunctionRule.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@
use PHPStan\Rules\Rule;
use PHPStan\Rules\RuleErrorBuilder;
use PHPStan\ShouldNotHappenException;
use PHPStan\Type\MixedType;
use staabm\PHPStanDba\QueryReflection\QueryReflection;
use staabm\PHPStanDba\Tests\SyntaxErrorInQueryFunctionRuleTest;
use staabm\PHPStanDba\UnresolvableQueryException;
Expand Down Expand Up @@ -85,13 +84,15 @@ public function processNode(Node $node, Scope $scope): array
return [];
}

if ($scope->getType($args[$queryArgPosition]->value) instanceof MixedType) {
$queryExpr = $args[$queryArgPosition]->value;
$queryReflection = new QueryReflection();

if ($queryReflection->isResolvable($queryExpr, $scope)->no()) {
return [];
}

$queryReflection = new QueryReflection();
try {
foreach ($queryReflection->resolveQueryStrings($args[$queryArgPosition]->value, $scope) as $queryString) {
foreach ($queryReflection->resolveQueryStrings($queryExpr, $scope) as $queryString) {
$queryError = $queryReflection->validateQueryString($queryString);
if (null !== $queryError) {
return [
Expand All @@ -101,7 +102,7 @@ public function processNode(Node $node, Scope $scope): array
}
} catch (UnresolvableQueryException $exception) {
return [
RuleErrorBuilder::message($exception->asRuleMessage())->tip(UnresolvableQueryException::RULE_TIP)->line($node->getLine())->build(),
RuleErrorBuilder::message($exception->asRuleMessage())->tip($exception::getTip())->line($node->getLine())->build(),
];
}

Expand Down
11 changes: 6 additions & 5 deletions src/Rules/SyntaxErrorInQueryMethodRule.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@
use PHPStan\Rules\Rule;
use PHPStan\Rules\RuleErrorBuilder;
use PHPStan\ShouldNotHappenException;
use PHPStan\Type\MixedType;
use staabm\PHPStanDba\QueryReflection\QueryReflection;
use staabm\PHPStanDba\UnresolvableQueryException;

Expand Down Expand Up @@ -79,13 +78,15 @@ public function processNode(Node $node, Scope $scope): array
return [];
}

if ($scope->getType($args[$queryArgPosition]->value) instanceof MixedType) {
$queryExpr = $args[$queryArgPosition]->value;
$queryReflection = new QueryReflection();

if ($queryReflection->isResolvable($queryExpr, $scope)->no()) {
return [];
}

try {
$queryReflection = new QueryReflection();
$queryStrings = $queryReflection->resolveQueryStrings($args[$queryArgPosition]->value, $scope);
$queryStrings = $queryReflection->resolveQueryStrings($queryExpr, $scope);
foreach ($queryStrings as $queryString) {
$queryError = $queryReflection->validateQueryString($queryString);
if (null !== $queryError) {
Expand All @@ -96,7 +97,7 @@ public function processNode(Node $node, Scope $scope): array
}
} catch (UnresolvableQueryException $exception) {
return [
RuleErrorBuilder::message($exception->asRuleMessage())->tip(UnresolvableQueryException::RULE_TIP)->line($node->getLine())->build(),
RuleErrorBuilder::message($exception->asRuleMessage())->tip($exception::getTip())->line($node->getLine())->build(),
];
}

Expand Down
7 changes: 5 additions & 2 deletions src/UnresolvableQueryException.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,12 @@

namespace staabm\PHPStanDba;

final class UnresolvableQueryException extends DbaException
/**
* @api
*/
abstract class UnresolvableQueryException extends DbaException
{
public const RULE_TIP = 'Make sure all variables involved have a non-mixed type and array-types are specified.';
abstract public static function getTip(): string;

public function asRuleMessage(): string
{
Expand Down
11 changes: 11 additions & 0 deletions src/UnresolvableQueryMixedTypeException.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
<?php

namespace staabm\PHPStanDba;

final class UnresolvableQueryMixedTypeException extends UnresolvableQueryException
{
public static function getTip(): string
{
return 'Make sure all variables involved have a non-mixed type and array-types are specified.';
}
}
11 changes: 11 additions & 0 deletions src/UnresolvableQueryStringTypeException.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
<?php

namespace staabm\PHPStanDba;

final class UnresolvableQueryStringTypeException extends UnresolvableQueryException
{
public static function getTip(): string
{
return 'Consider replacing concatenated string-variables with prepared statements or @phpstandba-inference-placeholder.';
}
}
19 changes: 18 additions & 1 deletion tests/rules/QueryPlanAnalyzerRuleTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@
use PHPStan\Testing\RuleTestCase;
use staabm\PHPStanDba\QueryReflection\QueryReflection;
use staabm\PHPStanDba\Rules\QueryPlanAnalyzerRule;
use staabm\PHPStanDba\UnresolvableQueryMixedTypeException;
use staabm\PHPStanDba\UnresolvableQueryStringTypeException;

/**
* @extends RuleTestCase<QueryPlanAnalyzerRule>
Expand Down Expand Up @@ -137,7 +139,22 @@ public function testNotUsingIndexInDebugMode(): void
[
'Unresolvable Query: Cannot simulate parameter value for type: mixed.',
61,
'Make sure all variables involved have a non-mixed type and array-types are specified.',
UnresolvableQueryMixedTypeException::getTip(),
],
[
'Unresolvable Query: Cannot resolve query with variable type: string.',
67,
UnresolvableQueryStringTypeException::getTip(),
],
[
'Unresolvable Query: Cannot resolve query with variable type: string.',
70,
UnresolvableQueryStringTypeException::getTip(),
],
[
'Unresolvable Query: Cannot resolve query with variable type: string.',
73,
UnresolvableQueryStringTypeException::getTip(),
],
]);
}
Expand Down
12 changes: 9 additions & 3 deletions tests/rules/UnresolvablePdoStatementRuleTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@
use PHPStan\Testing\RuleTestCase;
use staabm\PHPStanDba\QueryReflection\QueryReflection;
use staabm\PHPStanDba\Rules\PdoStatementExecuteMethodRule;
use staabm\PHPStanDba\UnresolvableQueryException;
use staabm\PHPStanDba\UnresolvableQueryMixedTypeException;
use staabm\PHPStanDba\UnresolvableQueryStringTypeException;

/**
* @extends RuleTestCase<PdoStatementExecuteMethodRule>
Expand Down Expand Up @@ -43,12 +44,17 @@ public function testSyntaxErrorInQueryRule(): void
[
'Unresolvable Query: Cannot simulate parameter value for type: mixed.',
13,
UnresolvableQueryException::RULE_TIP,
UnresolvableQueryMixedTypeException::getTip(),
],
[
'Unresolvable Query: Cannot simulate parameter value for type: mixed.',
17,
UnresolvableQueryException::RULE_TIP,
UnresolvableQueryMixedTypeException::getTip(),
],
[
'Unresolvable Query: Cannot resolve query with variable type: string.',
54,
UnresolvableQueryStringTypeException::getTip(),
],
]);
}
Expand Down
Loading