Unresolvable query not reported in debug mode #504

hemberger · 2023-02-12T23:11:59Z

I was hoping to use debug mode to find all queries that phpstan-dba fails to resolve, since these will not report any errors in non-debug mode. However, there appears to be a class of queries that silently fail to resolve, which makes it challenging to find and fix the queries.

Here are some examples that I used to explore this behavior:

If the query is joined with a mixed type, we are alerted to the failure to resolve the query:
```
$query = 'SELECT * FROM table_does_not_exist WHERE col1 = ' . return_mixed();
```
Unresolvable Query: Cannot simulate parameter value for type: mixed.
If the query is joined with an int type, the query is properly analyzed:
```
$query = 'SELECT * FROM table_does_not_exist WHERE col1 = ' . return_int();
```
Query error: SQLSTATE[42S02]: Base table or view not found: 1146 Table 'dbname.table_does_not_exist' doesn't exist (42S02).

If the query is joined with a string type, the error is silently ignored:

$query = 'SELECT * FROM table_does_not_exist WHERE col1 = ' . return_string();

No errors

Obviously, in this particular example, a prepared query would solve the problem, but there are legitimate use cases where we might join a non-literal string to a query string, and in such a case we'd want to get an error when the query cannot be resolved (in debug mode, at least). Is this possible?

Please let me know if further information is needed. I'm also happy to look into this myself if you can point me in the right direction.

Thanks again for this awesome library! I'm really looking forward to using it to help refactor an old codebase that used manual escaping instead of prepared queries.

hemberger · 2023-02-12T23:43:37Z

Would #243 solve this issue?

staabm · 2023-02-13T09:04:55Z

Obviously, in this particular example, a prepared query would solve the problem, but there are legitimate use cases where we might join a non-literal string to a query string, and in such a case we'd want to get an error when the query cannot be resolved (in debug mode, at least). Is this possible?

I was hesitant adding error for all unresolvable cases when I have implemented debug mode, because I thought it might be a problem for new-comers to see value in the tool, when e.g. in legacy code-bases most queries would error.

since the tool evolved and we are understanding queries better (and I also think not a lot of people are using debug mode), I think we could change debug mode to also report errors for the cases a string is contained.

if I understand you correctly, you would prefer having this error in the phpstan baseline over not having them reported, right?

staabm · 2023-02-13T09:07:42Z

I guess you would need to report the error here in debug mode:

phpstan-dba/src/QueryReflection/QuerySimulation.php

Lines 96 to 97 in e0f23fd

    
           // plain string types can contain anything.. we cannot reason about it 
        
           return null;

similar to how we do it here

phpstan-dba/src/QueryReflection/QuerySimulation.php

Lines 115 to 117 in e0f23fd

    
           if (QueryReflection::getRuntimeConfiguration()->isDebugEnabled()) { 
        
               throw new UnresolvableQueryException('Cannot simulate parameter value for type: '.$paramType->describe(VerbosityLevel::precise())); 
        
           }

hemberger · 2023-02-13T20:19:50Z

if I understand you correctly, you would prefer having this error in the phpstan baseline over not having them reported, right?

Yes, that is definitely my preference. I tried throwing an UnresolvableQueryException as you described above, and it was very helpful for identifying queries that were not being checked.

I was hesitant adding error for all unresolvable cases when I have implemented debug mode, because I thought it might be a problem for new-comers to see value in the tool, when e.g. in legacy code-bases most queries would error.

That's a reasonable concern, but I think that most (if not all) people would try out the tool with debug mode turned off to start - I know I did. In fact, I thought debug mode was just going to be more verbose, but now that I know what it does I can't imagine using phpstan-dba without it!

If you'd like me to submit a PR, I have a couple questions about your preferences for implementation:

Would you want this to be part of debug mode, or a separate configuration option? (e.g. $config->reportUnresolvableQueries(true), perhaps along with the mixed-type error)
Would you want the UnresolvableQueryException::RULE_TIP to be expanded to include info about non-literal strings, or would you rather have separate exceptions for each type of failure (deriving from UnresolvableQueryException) with their own customized tips?

Otherwise, I can leave it to you. Thanks again!

staabm · 2023-02-13T20:53:40Z

In fact, I thought debug mode was just going to be more verbose, but now that I know what it does I can't imagine using phpstan-dba without it!

thanks for the feedback. looking forward to more feedback as you use the tool even more.

Would you want this to be part of debug mode, or a separate configuration option? (e.g. $config->reportUnresolvableQueries(true), perhaps along with the mixed-type error)

atm the debug mode is meant todo that for now. it might make sense to move this to a separate flag as suggested in the future though.

Would you want the UnresolvableQueryException::RULE_TIP to be expanded to include info about non-literal strings, or would you rather have separate exceptions for each type of failure (deriving from UnresolvableQueryException) with their own customized tips?

I think we should make UnresolvableQueryException abstract and create one subclass per case.

thank you

@var

Fixes staabm#504. This adds new functionality to debug mode to warn about queries that cannot be resolved due to the presence of non-literal string variables in the query. This helps to identify places where you might expect a query to be resolvable, but an inline parameter or a variable query fragment prevent it. Some scenarios where this might occur include string query fragments where using `@phpstandba-inference-placeholder` may be needed, e.g. /** @var string $condition */ $query = 'SELECT * FROM table WHERE ' . $condition; Or when not using prepared statements, e.g. /** @var string $param */ $query = 'SELECT * FROM table WHERE foo=' . $conn->quote($param); Since these queries cannot be analyzed, they will now emit the following warning: > Unresolvable Query: Cannot resolve query with variable type: string. > Consider replacing variable strings with prepared statements or @phpstandba-inference-placeholder. Without this warning, errors in these queries will go undetected. Implementation notes: 1. We make `UnresolvableQueryException` abstract, and add child classes for each type of unresolvable query error so that we can provide customized tips for each one. 2. Special care had to be taken to retain the ability to ignore queries that are _entirely_ StringType, because it was explicitly desired for these queries to not return an error (even in debug mode). This is because such queries are likely part of a s/w layer that we know nothing about. Therefore, we early return before checking parameters for any query that is is a superset of `StringType` (which includes the previous `MixedType` condition, and any mix of the two).

@var

Fixes staabm#504. This adds new functionality to debug mode to warn about queries that cannot be resolved due to the presence of non-literal string variables in the query. This helps to identify places where you might expect a query to be resolvable, but an inline parameter or a variable query fragment prevent it. Some scenarios where this might occur include string query fragments where using `@phpstandba-inference-placeholder` may be needed, e.g. /** @var string $condition */ $query = 'SELECT * FROM table WHERE ' . $condition; Or when not using prepared statements, e.g. /** @var string $param */ $query = 'SELECT * FROM table WHERE foo=' . $conn->quote($param); Since these queries cannot be analyzed, they will now emit the following warning: > Unresolvable Query: Cannot resolve query with variable type: string. > Consider replacing variable strings with prepared statements or @phpstandba-inference-placeholder. Without this warning, errors in these queries will go undetected. Implementation notes: 1. We make `UnresolvableQueryException` abstract, and add child classes for each type of unresolvable query error so that we can provide customized tips for each one. 2. Special care had to be taken to retain the ability to ignore queries that are _entirely_ StringType, because it was explicitly desired for these queries to not return an error (even in debug mode). This is because such queries are likely part of a s/w layer that we know nothing about. Therefore, we early return before checking parameters for any query that is is a superset of `StringType` (which includes the previous `MixedType` condition, and any mix of the two).

@var

Fixes staabm#504. This adds new functionality to debug mode to warn about queries that cannot be resolved due to the presence of non-literal string variables in the query. This helps to identify places where you might expect a query to be resolvable, but an inline parameter or a variable query fragment prevent it. Some scenarios where this might occur include string query fragments where using `@phpstandba-inference-placeholder` may be needed, e.g. /** @var string $condition */ $query = 'SELECT * FROM table WHERE ' . $condition; Or when not using prepared statements, e.g. /** @var string $param */ $query = 'SELECT * FROM table WHERE foo=' . $conn->quote($param); Since these queries cannot be analyzed, they will now emit the following warning: > Unresolvable Query: Cannot resolve query with variable type: string. > Consider replacing variable strings with prepared statements or @phpstandba-inference-placeholder. Without this warning, errors in these queries will go undetected. Implementation notes: 1. We make `UnresolvableQueryException` abstract, and add child classes for each type of unresolvable query error so that we can provide customized tips for each one. 2. Special care had to be taken to retain the ability to ignore queries that are _entirely_ StringType, because it was explicitly desired for these queries to not return an error (even in debug mode). This is because such queries are likely part of a s/w layer that we know nothing about. Therefore, we early return before checking parameters for any query that is is a superset of `StringType` (which includes the previous `MixedType` condition, and any mix of the two).

hemberger mentioned this issue Feb 15, 2023

Warn when queries cannot be resolved unexpectedly #508

Merged

staabm closed this as completed in #508 Feb 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unresolvable query not reported in debug mode #504

Unresolvable query not reported in debug mode #504

hemberger commented Feb 12, 2023

hemberger commented Feb 12, 2023

staabm commented Feb 13, 2023

staabm commented Feb 13, 2023

hemberger commented Feb 13, 2023

staabm commented Feb 13, 2023

Unresolvable query not reported in debug mode #504

Unresolvable query not reported in debug mode #504

Comments

hemberger commented Feb 12, 2023

hemberger commented Feb 12, 2023

staabm commented Feb 13, 2023

staabm commented Feb 13, 2023

hemberger commented Feb 13, 2023

staabm commented Feb 13, 2023