Skip to content

Conversation

@marmichalski
Copy link
Contributor

@marmichalski marmichalski commented May 19, 2022

Expect this to fail (in more places than intended).

The significant failing job is: PHPUnit (8.0, mysql:8.0, pdo, recording), the rest is failing because of cache mismatch (I believe?).

Closes: #357

* @phpstan-ignore-next-line
*/
if (!isset($columnMeta['native_type'])) {
$columnMeta['native_type'] = \PDO::PARAM_INT === $columnMeta['pdo_type'] ? 'INT' : 'STRING';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

According to pdo_mysql src, native_type can be missing from the return value:
https://github.com/php/php-src/blob/14319c203cb13a7cc0644fb4276d6e62338fda5e/ext/pdo_mysql/mysql_statement.c#L806-L808

and in that case, we may fallback to the pdo_type, problem is, according to my understanding of the sources, this can be either INT or STR... https://github.com/php/php-src/blob/14319c203cb13a7cc0644fb4276d6e62338fda5e/ext/pdo_mysql/mysql_statement.c#L811-L827.


/*
* Native type may not bo set, for example in case of JSON column.
* @phpstan-ignore-next-line
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have not updated the ColumnMeta to represent the actual state of PDO.getColumnMeta return value as just ignoring this line was so much easier... 🤡

@marmichalski marmichalski marked this pull request as ready for review May 19, 2022 23:32
@staabm
Copy link
Owner

staabm commented May 20, 2022

This looks great, thank you.

Can the same bug be reproduced with PGSQL?

Additionally it would be cool if you could add a new testcase for the DbaInferenceTest which contains sql queries using the JSON column, to make sure it also works in non trivial SELECT * FROM x cases

@marmichalski
Copy link
Contributor Author

marmichalski commented May 20, 2022

Can the same bug be reproduced with PGSQL?

Kind of. PdoPgQueryReflector does not crash in case of json, because the extension fallbacks to selecting TYPNAM from PG_TYPE and the way phpstan-dba works will make it just return MixedType. This has been updated to map json & jsonb types into StringType & bool (boolean) to BooleanType.

This still possibly may not return native_type, but I am unaware of when this could happen, therefore leaving this until reproducible crash scenario is found, which will make it easier to fix 😎

Additionally it would be cool if you could add a new testcase for the DbaInferenceTest which contains sql queries using the JSON column, to make sure it also works in non trivial SELECT * FROM x cases

Yes, I could, but do you have something specific in mind? As SELECT c_json FROM ... is probably just the same thing as the SELECT * ...? 🤔

@staabm
Copy link
Owner

staabm commented May 20, 2022

Kind of. PdoPgQueryReflector does not crash in case of json, because the extension fallbacks to selecting TYPNAM from PG_TYPE and the way phpstan-dba works will make it just return MixedType. This has been updated to map json & jsonb types into StringType & bool (boolean) to BooleanType.

cool

This still possibly may not return native_type, but I am unaware of when this could happen, therefore leaving this until reproducible crash scenario is found, which will make it easier to fix 😎

makes sense, perfect.

Yes, I could, but do you have something specific in mind? As SELECT c_json FROM ... is probably just the same thing as the SELECT * ...? 🤔

I am thinking about very simple json sql select queries which involve the json columns with e.g. WHERE conditions working on the json column with e.g. json functions.

a few real world statements so we can be sure phpstan-dba does not detect syntax errors etc

please put them into a new file under https://github.com/staabm/phpstan-dba/tree/main/tests/default/data

@staabm staabm changed the title Handle json column in pdo reflector Added support for JSON datatype May 20, 2022
@staabm staabm enabled auto-merge (squash) May 20, 2022 14:53
@staabm
Copy link
Owner

staabm commented May 20, 2022

as this is really useful as is, I will merge for now.

would be great you could add the JSON tests we talked about in a separate PR.

Thank you.

@staabm staabm merged commit 5c112b0 into staabm:main May 20, 2022
@marmichalski marmichalski deleted the json branch May 20, 2022 14:55
@marmichalski
Copy link
Contributor Author

would be great you could add the JSON tests we talked about in a separate PR

Sure, I should be able to deliver over the weekend 🙃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

PDO Query Reflector, MySQL & json field

3 participants