fix(query-compiler): return None instead of panicking when a batched QuerySingle has zero keys (prisma/prisma#29642)#5818
Conversation
…QuerySingle has zero keys (prisma/prisma#29642) `QuerySingle::new` validated against `has_many_keys()` but never against `has_no_keys()`, so when the TS client batched concurrent `findUnique` calls and at least one had `where: { id: undefined }`, the stripped filter arrived here as an empty `QueryFilters`. Falling through to let (key, value) = first.get_single_key().unwrap(); panicked (`called Option::unwrap() on a None value at selection.rs:218`). The single-call path correctly rejects this with `PrismaClientValidationError`, but the dataloader/batched path bypasses that validation; in production we saw 28 PrismaClientRustPanicError crashes in a 42-second window, and innocent queries sharing the batch window were taken out as collateral when the panic poisoned the batch. This commit narrows the precondition check on `QuerySingle::new` to reject empty filters alongside many-key ones, and adds a `has_no_keys()` helper to make the intent obvious at the call site. `SelectionSet::new` already handles the None-result case — it maps to `SelectionSet::Empty` when every filter is empty, or `SelectionSet::Many(filters)` otherwise — so the upstream behaviour is preserved: the panic becomes a graceful fallthrough that downstream layers can already deal with. This is the lower half of the fix. The higher-level concern from the issue — that the batched path should reject `undefined` unique values with `PrismaClientValidationError` exactly like the single-call path — lives in the TS client and is left for a follow-up; once that lands, the path that reaches `QuerySingle::new` with an empty filter goes away entirely. The guard here is the safety net for any other code path that might construct an empty filter in the future. - query-compiler/core/src/query_document/selection.rs: new `QueryFilters::has_no_keys()` helper; extend the precondition in `QuerySingle::new` to short-circuit on empty filters; rewrite the docstring to call out the bug shape and link the issue - Added a `#[cfg(test)] mod tests` covering: (1) empty filter returns None (the direct regression), (2) mixed-batch with one empty filter returns None (the production shape), (3) the new helper distinguishes empty from populated, (4) well-formed single-key batches still coalesce (behaviour preservation) Verified locally: `cargo test -p query-core --lib selection` → 4 passed. `cargo build -p query-core` clean.
|
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
Summary by CodeRabbit
Walkthrough
🚥 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-compiler/core/src/query_document/selection.rs`:
- Around line 443-444: Update the regression test comment at lines 443-444 to
correctly describe the behavior of SelectionSet::new. The comment currently
states that the all-empty case maps to SelectionSet::Empty, but this is only
true when the input vector itself is empty (Line 277). When you have a non-empty
Vec<QueryFilters> containing empty filters, SelectionSet::new returns
SelectionSet::Many(filters) (Line 278). Revise the comment to accurately reflect
that the all-empty case with non-empty filter vectors maps to
SelectionSet::Many(filters), not SelectionSet::Empty.
🪄 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: 494aa4a5-8f49-4fb2-88e6-610c629a56f3
📒 Files selected for processing (1)
query-compiler/core/src/query_document/selection.rs
The comment claimed the all-empty case maps to `SelectionSet::Empty`, but that branch is only taken when the input `Vec<QueryFilters>` itself is empty (selection.rs:277). A non-empty Vec full of empty filters falls into `SelectionSet::Many(filters)` instead (selection.rs:278). No behavior change — just a more accurate comment.
|
Thanks for the catch @coderabbitai — pushed 73f1408 reworking the regression-test comment so it reflects the actual mapping: a non-empty CLA sign-off coming next. |
|
CodeRabbit chat interactions are restricted to organization members for this repository. Ask an organization member to interact with CodeRabbit, or set |
2 participants
Why
Closes the engine-side half of prisma/prisma#29642.
`QuerySingle::new` validates against `has_many_keys()` but never against "zero keys." When the TS client batches concurrent `findUnique` calls and at least one has `where: { id: undefined }`, the stripped filter arrives here as an empty `QueryFilters`. Falling through to:
```rust
let (key, value) = first.get_single_key().unwrap();
```
panics with `called Option::unwrap() on a None value at selection.rs:218`.
The reporter (prisma/prisma#29642, with Invictnox's root-cause comment) saw 28 `PrismaClientRustPanicError` crashes in a 42-second window in production. The single-call path correctly rejects `undefined` unique values with `PrismaClientValidationError`, but the dataloader/batched path bypasses that validation, hits this unwrap, and panics. Innocent queries sharing the batch window are taken out as collateral when the panic poisons the batch — `Promise.allSettled([bad(), good(), bad(), good()])` rejects every entry.
Fix
Narrow the precondition check on `QuerySingle::new` to reject empty filters alongside many-key ones:
```diff
return None;
}
```
Plus a small `QueryFilters::has_no_keys()` helper to make the intent obvious at the call site.
`SelectionSet::new` already handles the `None` result:
```rust
match single {
Some(single) => SelectionSet::Single(single),
None if filters.is_empty() => SelectionSet::Empty,
None => SelectionSet::Many(filters),
}
```
So upstream behaviour is preserved — the panic becomes a graceful fallthrough that downstream layers can already deal with.
What this is NOT
The higher-level concern from prisma/prisma#29642 — that the batched path in the TS client should reject `undefined` unique values with `PrismaClientValidationError` exactly like the single-call path — lives outside this repo. Once that lands, the path that reaches `QuerySingle::new` with an empty filter goes away entirely. The guard here is the safety net for any other code path that might construct an empty filter in the future, and it makes the immediate production panic stop now.
Tests
```
$ cargo test -p query-core --lib selection
running 4 tests
test query_document::selection::tests::query_single_new_returns_none_for_empty_filter_instead_of_panicking ... ok
test query_document::selection::tests::query_filters_has_no_keys_distinguishes_empty_from_populated ... ok
test query_document::selection::tests::query_single_new_returns_none_when_any_filter_in_batch_is_empty ... ok
test query_document::selection::tests::query_single_new_still_works_for_well_formed_single_key_batch ... ok
test result: ok. 4 passed; 0 failed
```
The four cases cover:
`cargo build -p query-core` clean. I did not run `make pedantic` (clippy + fmt) — happy to do that on request, but the touched lines are minimal and the existing style is preserved.