fix: prisma query batching #1027
Conversation
📝 WalkthroughWalkthroughThis change updates the project version from 0.5.1-rc.4 to 0.5.1-rc.5 across multiple configuration files, dependencies, and version constants. It also refactors the Prisma runtime's batching and dependency resolution logic, adds a new test for batching Prisma mutations, and adjusts test assertions to reflect the new batching approach. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant PrismaRuntime
participant Database
Client->>PrismaRuntime: Submit batched mutation (multiple createOneRecord)
PrismaRuntime->>PrismaRuntime: Separate stages (with/without dependencies)
PrismaRuntime->>PrismaRuntime: Build batch query for all stages
PrismaRuntime->>Database: Execute combined batch query
Database-->>PrismaRuntime: Return batch results
PrismaRuntime->>PrismaRuntime: Map results to individual stages using stageOrder
PrismaRuntime-->>Client: Return individual mutation results (IDs)
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
tests/runtimes/prisma/prisma_test.ts (1)
200-216: Consider enhancing the test coverage.While the test correctly validates the batching functionality, consider these improvements:
- Verify that the two returned IDs are different to ensure distinct records were created
- Add a follow-up query to confirm both records exist in the database
- Test error scenarios (e.g., one mutation succeeds while another fails)
Here's an enhanced version that verifies the IDs are different:
.expectBody(({ data }) => { assert(v4.validate(data.aa.id)); assert(v4.validate(data.one.id)); + assert(data.aa.id !== data.one.id, "IDs should be different"); })src/typegate/src/runtimes/prisma/prisma.ts (1)
294-294: Consider using deterministic IDs for debugging.Using
crypto.randomUUID()for the batch stage path makes debugging and tracing difficult.- path: [crypto.randomUUID()], + path: [`batch_${Date.now()}_${this.name}`],
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.locktests/metagen/__snapshots__/metagen_test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (24)
Cargo.toml(1 hunks)examples/templates/deno/api/example.ts(1 hunks)examples/templates/deno/compose.yml(1 hunks)examples/templates/node/compose.yml(1 hunks)examples/templates/node/package.json(1 hunks)examples/templates/python/compose.yml(1 hunks)examples/templates/python/pyproject.toml(1 hunks)pyproject.toml(1 hunks)src/pyrt_wit_wire/pyproject.toml(1 hunks)src/typegate/src/runtimes/prisma/prisma.ts(3 hunks)src/typegate/src/runtimes/wit_wire/mod.ts(1 hunks)src/typegate/src/services/auth/protocols/oauth2.ts(0 hunks)src/typegraph/core/Cargo.toml(1 hunks)src/typegraph/core/src/global_store.rs(1 hunks)src/typegraph/deno/deno.json(1 hunks)src/typegraph/python/pyproject.toml(1 hunks)src/typegraph/python/typegraph/__init__.py(1 hunks)src/xtask/Cargo.toml(1 hunks)tests/metagen/typegraphs/identities/rs/Cargo.toml(1 hunks)tests/metagen/typegraphs/sample/rs/Cargo.toml(1 hunks)tests/metagen/typegraphs/sample/rs_upload/Cargo.toml(1 hunks)tests/runtimes/prisma/prisma_test.ts(1 hunks)tests/runtimes/prisma/query_builder_test.ts(1 hunks)tools/consts.ts(1 hunks)
💤 Files with no reviewable changes (1)
- src/typegate/src/services/auth/protocols/oauth2.ts
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: build-docker (linux/amd64, custom-ubuntu-large)
- GitHub Check: build-docker (linux/arm64, ubuntu-22.04-arm)
- GitHub Check: test-full
- GitHub Check: lint-compat (macos-14, aarch64-apple-darwin, false)
- GitHub Check: lint-compat (macos-13, x86_64-apple-darwin, false)
- GitHub Check: pre-commit
🔇 Additional comments (22)
src/typegraph/deno/deno.json (2)
3-3: Approve Deno SDK version bumpVersion updated from "0.5.1-rc.4" to "0.5.1-rc.5". Matches the coordinated release.
3-3:✅ Verification successful
Check for leftover references
Ensure no occurrences of the previous version remain. Run the following to catch any stale
0.5.1-rc.4references:
🏁 Script executed:
#!/bin/bash rg "0.5.1-rc.4"Length of output: 15
No stale version references found
The search for0.5.1-rc.4returned no results, confirming that all previous version strings have been updated.src/typegraph/core/Cargo.toml (1)
3-3: Approve core package version bumpUpdated version to "0.5.1-rc.5" as part of the release.
pyproject.toml (1)
9-9: Approve Metatype Python package version bumpVersion updated to "0.5.1-rc.5" in root
pyproject.toml.src/typegraph/python/pyproject.toml (1)
3-3: Approve Typegraph Python package version bumpVersion updated to "0.5.1-rc.5".
tests/metagen/typegraphs/sample/rs_upload/Cargo.toml (1)
4-4: Approve sample_client_upload version bumpUpdated version to "0.5.1-rc.5".
tests/metagen/typegraphs/identities/rs/Cargo.toml (1)
4-4: Approve version bump to "0.5.1-rc.5"
Theversionfield has been correctly updated to align with the new release candidate.src/typegate/src/runtimes/wit_wire/mod.ts (1)
10-10: ApproveMETATYPE_VERSIONupdate
The constant has been bumped from"0.5.1-rc.4"to"0.5.1-rc.5", matching the overall version alignment.src/xtask/Cargo.toml (1)
3-3: Approve xtask package version bump
The[package]version is now"0.5.1-rc.5", consistent with the release candidate across the workspace.Cargo.toml (1)
33-33: Approve workspace package version bump
The workspace-wideversionhas been updated to"0.5.1-rc.5"as intended.src/pyrt_wit_wire/pyproject.toml (1)
3-3: Approve Python package version bump
Thepyrt_wit_wirepackage version is updated to"0.5.1-rc.5", keeping it in sync with other components.examples/templates/node/package.json (1)
9-9: Version bump for @typegraph/sdk dependency
The update to^0.5.1-rc.5aligns with the coordinated version bump across the repo.tests/metagen/typegraphs/sample/rs/Cargo.toml (1)
4-4: Update sample_client crate version
Bumping to0.5.1-rc.5is consistent with the overall release candidate update.tools/consts.ts (1)
4-4: Confirm LATEST_PRE_RELEASE_VERSION accuracy
CURRENT_VERSIONis now"0.5.1-rc.5", butLATEST_PRE_RELEASE_VERSIONremains at"0.5.1-rc.2". Please verify whether this is intentional or if it should be updated to reflect the latest prerelease.src/typegraph/python/typegraph/__init__.py (1)
8-8: Python package version bump
Updatingversionto"0.5.1-rc.5"matches the release candidate update in other manifests.src/typegraph/core/src/global_store.rs (1)
108-108: SDK_VERSION constant updated
ChangingSDK_VERSIONto"0.5.1-rc.5"keeps the runtime constant in sync with the new release.examples/templates/deno/api/example.ts (1)
1-3: Consistent SDK version bump
Imports have been updated from0.5.1-rc.4to0.5.1-rc.5, aligning with the overall project version.examples/templates/python/pyproject.toml (1)
3-3: Bump example template version & dependency
The[tool.poetry]version andtypegraphdependency have been updated to0.5.1-rc.5, ensuring consistency with the release.Also applies to: 9-9
examples/templates/python/compose.yml (1)
3-3: Update Docker image version
Thetypegateservice image has been bumped toghcr.io/metatypedev/typegate:v0.5.1-rc.5to match the new release.examples/templates/node/compose.yml (1)
3-3: Update Docker image version
Thetypegateservice image has been bumped toghcr.io/metatypedev/typegate:v0.5.1-rc.5to reflect the version bump.examples/templates/deno/compose.yml (1)
3-3: Update Docker image version
Thetypegateservice image has been updated toghcr.io/metatypedev/typegate:v0.5.1-rc.5, aligning with project versioning.src/typegate/src/runtimes/prisma/prisma.ts (1)
178-180: LGTM! The enhanced return type supports proper batch result mapping.The addition of the
orderarray is essential for correctly mapping batch query results back to their corresponding stages.
| // FIXME: this test uses the stages from the initial plan | ||
| // which wilil include the polymorphic batch stage (which is then included | ||
| // in a concrete batch stage when we genereate again) | ||
| { | ||
| action: "batch", | ||
| modelName: undefined, | ||
| query: { | ||
| arguments: { | ||
| context: {}, | ||
| parent: {}, | ||
| variables: {}, | ||
| }, | ||
| selection: {}, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Address the FIXME comment and test design issue.
The FIXME comment indicates that the test is exposing internal implementation details (the polymorphic batch stage). This suggests the test might be testing at the wrong abstraction level.
Consider refactoring the test to:
- Test the public API behavior rather than internal query structure
- Filter out internal stages before assertions
- Move this to an integration test if it needs to test internal structures
🏁 Script executed:
#!/bin/bash
# Check if there are other tests that might be affected by this internal structure exposure
rg -A 5 "polymorphic batch stage" --type tsLength of output: 536
Refactor the test to avoid asserting internal “polymorphic batch” stages
The added FIXME shows this test is leaking implementation details instead of validating the public API. You should either filter out or mock internal stages, or move this into an integration‐level test that can safely inspect the full plan.
• Location to update:
- tests/runtimes/prisma/query_builder_test.ts: lines ~47–61
• Example approach:
- // FIXME: this test uses the stages from the initial plan
- // which will include the polymorphic batch stage…
- {
- action: "batch",
- modelName: undefined,
- query: { … },
- },
+ // only assert on the final, concrete batch stage
+ {
+ action: "batch",
+ modelName: undefined,
+ query: { … },
+ },Tips:
- Wrap the result in a helper that strips out any
{ action: "polymorphicBatch", … }entries before your assertion. - Or elevate this to an integration test where you can inspect the full pipeline without mocking.
- Remove the FIXME once internal stages are no longer part of your unit‐level assertions.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In tests/runtimes/prisma/query_builder_test.ts around lines 47 to 61, the test
currently exposes internal implementation details by including polymorphic batch
stages in its assertions. To fix this, refactor the test to avoid asserting on
these internal stages by either filtering out any stages with action
"polymorphicBatch" before assertions or mocking them out. Alternatively, move
this test to an integration test where inspecting the full internal plan is
appropriate. Remove the FIXME comment once the test no longer leaks internal
structure details.
| if (new Set(stageOrder).size != stageOrder.length) { | ||
| throw new Error("duplicate top stage ids"); | ||
| } |
There was a problem hiding this comment.
Improve error message for duplicate stage IDs.
The error message "duplicate top stage ids" is not descriptive enough for debugging.
- if (new Set(stageOrder).size != stageOrder.length) {
- throw new Error("duplicate top stage ids");
- }
+ if (new Set(stageOrder).size != stageOrder.length) {
+ const duplicates = stageOrder.filter((id, index) => stageOrder.indexOf(id) !== index);
+ throw new Error(`Duplicate stage IDs found in batch: ${duplicates.join(", ")}`);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (new Set(stageOrder).size != stageOrder.length) { | |
| throw new Error("duplicate top stage ids"); | |
| } | |
| if (new Set(stageOrder).size != stageOrder.length) { | |
| const duplicates = stageOrder.filter((id, index) => stageOrder.indexOf(id) !== index); | |
| throw new Error(`Duplicate stage IDs found in batch: ${duplicates.join(", ")}`); | |
| } |
🤖 Prompt for AI Agents
In src/typegate/src/runtimes/prisma/prisma.ts around lines 277 to 279, the error
message for detecting duplicate stage IDs is too vague. Update the error message
to explicitly state that duplicate stage IDs were found in the top stages and
include details such as which IDs are duplicated if possible, to make debugging
easier.
| renamedPath[0] = renames[path[0]] ?? path[0]; | ||
| const myRes = res[0][stageOrder.indexOf(stage.id())]; | ||
|
|
There was a problem hiding this comment.
Add bounds checking for array access.
The code accesses res[0][stageOrder.indexOf(stage.id())] without verifying the index exists.
+ const stageIndex = stageOrder.indexOf(stage.id());
+ if (stageIndex === -1) {
+ throw new Error(`Stage ${stage.id()} not found in batch order`);
+ }
+ if (!res[0] || !res[0][stageIndex]) {
+ throw new Error(`No result found for stage ${stage.id()} at index ${stageIndex}`);
+ }
- const myRes = res[0][stageOrder.indexOf(stage.id())];
+ const myRes = res[0][stageIndex];📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| renamedPath[0] = renames[path[0]] ?? path[0]; | |
| const myRes = res[0][stageOrder.indexOf(stage.id())]; | |
| renamedPath[0] = renames[path[0]] ?? path[0]; | |
| const stageIndex = stageOrder.indexOf(stage.id()); | |
| if (stageIndex === -1) { | |
| throw new Error(`Stage ${stage.id()} not found in batch order`); | |
| } | |
| if (!res[0] || !res[0][stageIndex]) { | |
| throw new Error(`No result found for stage ${stage.id()} at index ${stageIndex}`); | |
| } | |
| const myRes = res[0][stageIndex]; |
🤖 Prompt for AI Agents
In src/typegate/src/runtimes/prisma/prisma.ts around lines 321 to 323, the code
accesses res[0][stageOrder.indexOf(stage.id())] without checking if the index
returned by stageOrder.indexOf(stage.id()) is valid or if res[0] exists. Add
bounds checking to ensure res[0] is defined and the index is within the valid
range before accessing res[0][index]. Handle cases where the index is -1 or out
of bounds to avoid runtime errors.
| ...field.props.dependencies, | ||
| ...(field.props.parent?.id() ?? "") | ||
| .split(".") | ||
| .filter((str) => str.length) | ||
| .map(str => parentToResolver.get(str)!) | ||
| .filter(Boolean), | ||
| ], |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor fragile string-based dependency resolution.
The dependency resolution using string splitting and filtering is fragile and hard to understand.
Consider extracting this logic into a well-tested helper method:
+ private resolveDependencyChain(parentId: string, parentToResolver: Map<string, string>): string[] {
+ return parentId
+ .split(".")
+ .filter((str) => str.length > 0)
+ .map(str => parentToResolver.get(str))
+ .filter((id): id is string => id !== undefined);
+ }
dependencies: [
...field.props.dependencies,
- ...(field.props.parent?.id() ?? "")
- .split(".")
- .filter((str) => str.length)
- .map(str => parentToResolver.get(str)!)
- .filter(Boolean),
+ ...this.resolveDependencyChain(field.props.parent?.id() ?? "", parentToResolver),
],🤖 Prompt for AI Agents
In src/typegate/src/runtimes/prisma/prisma.ts around lines 365 to 371, the
current dependency resolution uses fragile string splitting and filtering that
is hard to understand. Refactor this by extracting the logic that processes the
parent id string into a separate helper function. This helper should take the
parent id string, split it, filter empty parts, map them using parentToResolver,
and filter out falsy values. Then replace the inline code with a call to this
new helper function to improve readability and maintainability.
| const batchStage = new ComputeStage({ | ||
| ...stage.props, | ||
| materializer: { | ||
| data: { operation: "batch" }, |
There was a problem hiding this comment.
nitpick: Fake stage? If it's only for temporary compute then I suggest we prefix with fake, tmp, or similar.
There was a problem hiding this comment.
That's a good idea. Do you have any recommendations on how I could improve this? I think the original code was also trying to avoid adding a new stage.
There was a problem hiding this comment.
I don't see why not use it although it does break the assumption that it comes from the query. Instead we can maybe create a new kind of stage specifically for this sort of purpose? We can all at least be clear that there are two kind of stages.
There was a problem hiding this comment.
A new kind of shadow stage makes sense but I'd hold that off until we have more usecases. Abstraction count is harder to manage than code count. In this specific case though, I think I'll just use a bespoke function inline that will be cached or something. The introduction of this stage is breaking some of the tests.
| ...field.props.dependencies, | ||
| ...(field.props.parent?.id() ?? "") | ||
| .split(".") | ||
| .filter((str) => str.length) | ||
| .map(str => parentToResolver.get(str)!) |
There was a problem hiding this comment.
Is there a guarantee that any subpath embedded within id() would not overlap? Or did you meant using the entire id() as a key.. unless I completely off.
Migration notes
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores
Refactor
Style