fix(qc): switch JSON_TYPE gate to ARRAY for wildcard MySQL paths#5813
Open
truffle-dev wants to merge 1 commit into
Open
fix(qc): switch JSON_TYPE gate to ARRAY for wildcard MySQL paths#5813truffle-dev wants to merge 1 commit into
truffle-dev wants to merge 1 commit into
Conversation
`JSON_EXTRACT(col, '$.items[*].name')` returns a JSON array of matched values, so the existing `JSON_TYPE = 'STRING'` gate around `string_contains`, `string_starts_with`, and `string_ends_with` always evaluated to false and the filter returned 0 rows. Detect the three MySQL path wildcards (`[*]`, `.*`, `**`) and expect `JSON_TYPE = 'ARRAY'` in that case. Postgres' array-form path (`JsonFilterPath::Array`) carries literal segments only and cannot encode a wildcard, so it is unaffected. Closes prisma/prisma#29571
|
|
Summary by CodeRabbit
WalkthroughThis PR fixes JSON path wildcard filtering in MySQL by detecting when a path like ChangesJSON wildcard path type checking for string filters
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@query-engine/connector-test-kit-rs/query-engine-tests/tests/queries/filters/json_filters.rs`:
- Around line 557-597: Add additional tests mirroring
string_contains_wildcard_path to cover the other wildcard patterns and string
operations: create new async test functions (e.g.,
string_contains_member_wildcard, string_contains_recursive_descent,
string_starts_with_wildcard_path, string_ends_with_wildcard_path) that use
create_row to seed rows and run_query!/jsonq with path values "$.items.*.name"
and "$.items**.name" (and with string_starts_with / string_ends_with payloads)
and assert the expected snapshots similarly to string_contains_wildcard_path;
ensure tests are annotated with the same connector_test capabilities
(JsonFilteringJsonPath) and MySql versions so they exercise the same fix.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a623da30-9365-4776-8efd-47e7388dd138
📒 Files selected for processing (2)
query-compiler/query-builders/sql-query-builder/src/filter/visitor.rsquery-engine/connector-test-kit-rs/query-engine-tests/tests/queries/filters/json_filters.rs
Comment on lines
+557
to
+597
| // Regression for https://github.com/prisma/prisma/issues/29571. | ||
| // | ||
| // MySQL JSON path wildcards (`[*]`, `.*`, `**`) make JSON_EXTRACT return an | ||
| // array even when each matched value is a scalar string, so the | ||
| // `JSON_TYPE = 'STRING'` gate around `string_contains` always evaluated to | ||
| // false and the filter returned 0 rows. With the fix, wildcard-path filters | ||
| // expect `JSON_TYPE = 'ARRAY'` and `LIKE` matches against the JSON-serialized | ||
| // array text. | ||
| #[connector_test(capabilities(JsonFilteringJsonPath), only(MySql(5.7), MySql(8)))] | ||
| async fn string_contains_wildcard_path(runner: Runner) -> TestResult<()> { | ||
| create_row( | ||
| &runner, | ||
| 1, | ||
| r#"{ \"items\": [{ \"name\": \"Widget A\" }, { \"name\": \"Gadget B\" }] }"#, | ||
| false, | ||
| ) | ||
| .await?; | ||
| create_row( | ||
| &runner, | ||
| 2, | ||
| r#"{ \"items\": [{ \"name\": \"Gadget X\" }, { \"name\": \"Gadget Y\" }] }"#, | ||
| false, | ||
| ) | ||
| .await?; | ||
| create_row(&runner, 3, r#"{ \"items\": [] }"#, false).await?; | ||
|
|
||
| let res = run_query!( | ||
| runner, | ||
| jsonq( | ||
| &runner, | ||
| r#"path: "$.items[*].name", string_contains: "Widget" "#, | ||
| Some("") | ||
| ) | ||
| ); | ||
| insta::assert_snapshot!( | ||
| res, | ||
| @r###"{"data":{"findManyTestModel":[{"id":1}]}}"### | ||
| ); | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Consider adding tests for other wildcard patterns and string operations.
The test validates the [*] wildcard with string_contains, which covers the reported issue. However, the implementation also handles .* (object member wildcard) and ** (recursive descent), and applies the fix to string_starts_with and string_ends_with as well. Adding tests for these variations would strengthen confidence in the fix.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@query-engine/connector-test-kit-rs/query-engine-tests/tests/queries/filters/json_filters.rs`
around lines 557 - 597, Add additional tests mirroring
string_contains_wildcard_path to cover the other wildcard patterns and string
operations: create new async test functions (e.g.,
string_contains_member_wildcard, string_contains_recursive_descent,
string_starts_with_wildcard_path, string_ends_with_wildcard_path) that use
create_row to seed rows and run_query!/jsonq with path values "$.items.*.name"
and "$.items**.name" (and with string_starts_with / string_ends_with payloads)
and assert the expected snapshots similarly to string_contains_wildcard_path;
ensure tests are annotated with the same connector_test capabilities
(JsonFilteringJsonPath) and MySql versions so they exercise the same fix.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
On MySQL,
JSON_EXTRACT(col, '$.items[*].name')returns a JSON array of matched values rather than a scalar. The existingJSON_TYPE = 'STRING'gate added aroundstring_contains,string_starts_with, andstring_ends_withalways evaluated to false in that case, so the filter quietly returned 0 rows even when matching strings existed in the array elements.This PR detects the three MySQL path wildcards (
[*],.*,**) at the orchestration site inconvert_json_filter, threads apath_returns_array: boolthroughJsonFilterExt, and switches the gate toJSON_TYPE = 'ARRAY'when a wildcard is present. The produced SQL matches the expected query in the issue:Postgres' array-form path (
JsonFilterPath::Array) carries literal segments only and cannot encode a wildcard, so it remains on theSTRINGgate.Regression test in
query-engine-tests/tests/queries/filters/json_filters.rs::string_contains_wildcard_pathcovers the issue case (MySQL 5.7 and 8). The connector test kit needs a live MySQL instance, which isn't available in my dev environment, so the test compiles cleanly but I haven't run it against a database — happy to retarget the regression in a sql-query-builder unit test (visitor SQL snapshot) if you'd prefer.Closes prisma/prisma#29571