Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughWidened aggregation record values to accept either a single expression or an immutable array of expressions; updated freezing, rewriting, lowering, builder helpers, and tests to accept, propagate, and validate the widened shape. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
@prisma-next/mongo-runtime
@prisma-next/family-mongo
@prisma-next/sql-runtime
@prisma-next/family-sql
@prisma-next/extension-paradedb
@prisma-next/extension-pgvector
@prisma-next/postgres
@prisma-next/sql-orm-client
@prisma-next/target-mongo
@prisma-next/adapter-mongo
@prisma-next/driver-mongo
@prisma-next/contract
@prisma-next/utils
@prisma-next/config
@prisma-next/errors
@prisma-next/framework-components
@prisma-next/operations
@prisma-next/contract-authoring
@prisma-next/ids
@prisma-next/psl-parser
@prisma-next/psl-printer
@prisma-next/cli
@prisma-next/emitter
@prisma-next/migration-tools
@prisma-next/vite-plugin-contract-emit
@prisma-next/runtime-executor
@prisma-next/mongo-codec
@prisma-next/mongo-contract
@prisma-next/mongo-value
@prisma-next/mongo-contract-psl
@prisma-next/mongo-emitter
@prisma-next/mongo-query-ast
@prisma-next/mongo-orm
@prisma-next/mongo-pipeline-builder
@prisma-next/mongo-lowering
@prisma-next/mongo-wire
@prisma-next/sql-contract
@prisma-next/sql-errors
@prisma-next/sql-operations
@prisma-next/sql-schema-ir
@prisma-next/sql-contract-psl
@prisma-next/sql-contract-ts
@prisma-next/sql-contract-emitter
@prisma-next/sql-lane-query-builder
@prisma-next/sql-relational-core
@prisma-next/sql-builder
@prisma-next/target-postgres
@prisma-next/adapter-postgres
@prisma-next/driver-postgres
commit: |
ce26b36 to
26d7d93
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/2-mongo-family/5-query-builders/pipeline-builder/src/expression-helpers.ts (1)
412-422: Consider tightening thedefaultscontract onfn.zip.MongoDB only allows
defaultswhenuseLongestLength: true; otherwise$ziperrors. The newinputsarray shape is right, butdefaultsis still independently optional here, so callers can build an invalid operator tree. At minimum, makedefaultsdepend onuseLongestLengthin the args type or reject the missing-useLongestLengthcase before constructing the node. (mongodb.com)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-mongo-family/5-query-builders/pipeline-builder/src/expression-helpers.ts` around lines 412 - 422, The zip helper allows args.defaults even when args.useLongestLength is absent which can produce an invalid $zip; update the zip signature or add a runtime guard so defaults are only allowed when useLongestLength is provided. Either change the args type to a discriminated union so defaults is only present in the branch that requires useLongestLength (e.g. { inputs; useLongestLength?: never; defaults?: never } | { inputs; useLongestLength: TypedAggExpr<BooleanField>; defaults?: TypedAggExpr<ArrayField> }) or add a runtime check inside zip that throws (or sets useLongestLength) if args.defaults is provided but args.useLongestLength is missing before calling MongoAggOperator.of('$zip', ...). Ensure you reference the zip function, args.defaults and args.useLongestLength and prevent constructing the MongoAggOperator with an invalid combination.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/2-mongo-family/4-query/query-ast/src/aggregation-expressions.ts`:
- Line 4: The AggRecordArgs type allows array-valued entries that remain mutable
because MongoAggOperator and MongoAggAccumulator only call Object.freeze on the
top-level object; add a helper function named freezeRecordArgs that iterates
entries and, for any Array.isArray(val), replaces it with
Object.freeze([...val]) then returns Object.freeze(frozen), and call
freezeRecordArgs(...) instead of Object.freeze({ ...args }) in both the
MongoAggOperator constructor and the MongoAggAccumulator constructor branches
that accept record args so nested arrays become deeply frozen.
In
`@packages/2-mongo-family/5-query-builders/pipeline-builder/test/accumulator-helpers.test.ts`:
- Around line 57-69: The test is missing an assertion that the `n` field is
preserved for `topN`/`bottomN` helpers; update the parameterized case that
exercises `acc[helperName]` (the helpers in `acc`) to include `n` in `args` for
topN/bottomN and assert that `recordArg` has property 'n' and that
`recordArg['n']` equals the expected numeric value (in addition to the existing
`output` and `sortBy` checks); keep checks for instance types
(`MongoAggAccumulator`, `MongoAggLiteral`) and reuse `result.node`/`recordArg`
names as currently used.
---
Nitpick comments:
In
`@packages/2-mongo-family/5-query-builders/pipeline-builder/src/expression-helpers.ts`:
- Around line 412-422: The zip helper allows args.defaults even when
args.useLongestLength is absent which can produce an invalid $zip; update the
zip signature or add a runtime guard so defaults are only allowed when
useLongestLength is provided. Either change the args type to a discriminated
union so defaults is only present in the branch that requires useLongestLength
(e.g. { inputs; useLongestLength?: never; defaults?: never } | { inputs;
useLongestLength: TypedAggExpr<BooleanField>; defaults?:
TypedAggExpr<ArrayField> }) or add a runtime check inside zip that throws (or
sets useLongestLength) if args.defaults is provided but args.useLongestLength is
missing before calling MongoAggOperator.of('$zip', ...). Ensure you reference
the zip function, args.defaults and args.useLongestLength and prevent
constructing the MongoAggOperator with an invalid combination.
🪄 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: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 185deed4-0c94-4c02-8bf1-dd3ddbedfc89
📒 Files selected for processing (9)
packages/2-mongo-family/4-query/query-ast/src/aggregation-expressions.tspackages/2-mongo-family/4-query/query-ast/test/aggregation-expressions.test-d.tspackages/2-mongo-family/5-query-builders/pipeline-builder/src/accumulator-helpers.tspackages/2-mongo-family/5-query-builders/pipeline-builder/src/expression-helpers.tspackages/2-mongo-family/5-query-builders/pipeline-builder/test/accumulator-helpers.test-d.tspackages/2-mongo-family/5-query-builders/pipeline-builder/test/accumulator-helpers.test.tspackages/2-mongo-family/5-query-builders/pipeline-builder/test/expression-helpers.test-d.tspackages/2-mongo-family/5-query-builders/pipeline-builder/test/expression-helpers.test.tspackages/3-mongo-target/2-mongo-adapter/src/lowering.ts
packages/2-mongo-family/4-query/query-ast/src/aggregation-expressions.ts
Show resolved
Hide resolved
packages/2-mongo-family/5-query-builders/pipeline-builder/test/accumulator-helpers.test.ts
Show resolved
Hide resolved
26d7d93 to
74fdd61
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/3-mongo-target/2-mongo-adapter/test/lowering.test.ts (1)
482-490: Please split this test into a focused file as you add coverageThis file is already very large, and adding new lowering scenarios here keeps increasing maintenance cost. Prefer moving this case into a split file (for example,
lowering.agg-expr.test.ts) grouped bylowerAggExprbehavior.As per coding guidelines, "Keep test files under 500 lines to maintain readability and navigability. If a test file exceeds this limit, it should be split into multiple files."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/3-mongo-target/2-mongo-adapter/test/lowering.test.ts` around lines 482 - 490, The test for lowering the record-arg operator should be moved out of the large lowering.test.ts into a focused test file (e.g., lowering.agg-expr.test.ts) to keep files under 500 lines; create a new test file that imports MongoAggOperator, MongoAggFieldRef, MongoAggLiteral and lowerAggExpr and re-create this single-spec test ("lowers record-arg operator with array values") there, deleting the original spec from the big file so behavior is unchanged but coverage is split for maintainability.packages/2-mongo-family/5-query-builders/pipeline-builder/src/accumulator-helpers.ts (1)
2-2: Use a direct module import instead of the package barrelLine 2 imports runtime symbols from
@prisma-next/mongo-query-ast; this should use the specific module entrypoint to match repo import-layering/tree-shaking rules.As per coding guidelines, "
packages/**/*.{ts,tsx}: Import directly from the specific module ... instead of barrel imports ... to make dependencies explicit and improve tree-shaking".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-mongo-family/5-query-builders/pipeline-builder/src/accumulator-helpers.ts` at line 2, Replace the barrel import with the package's specific module entrypoint: change the import of MongoAggAccumulator and MongoAggLiteral to import them from the runtime/module entrypoint (e.g. import { MongoAggAccumulator, MongoAggLiteral } from '@prisma-next/mongo-query-ast/runtime') so the file uses a direct module import rather than the package barrel, ensuring explicit dependency and proper tree-shaking.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@packages/2-mongo-family/5-query-builders/pipeline-builder/src/accumulator-helpers.ts`:
- Line 2: Replace the barrel import with the package's specific module
entrypoint: change the import of MongoAggAccumulator and MongoAggLiteral to
import them from the runtime/module entrypoint (e.g. import {
MongoAggAccumulator, MongoAggLiteral } from
'@prisma-next/mongo-query-ast/runtime') so the file uses a direct module import
rather than the package barrel, ensuring explicit dependency and proper
tree-shaking.
In `@packages/3-mongo-target/2-mongo-adapter/test/lowering.test.ts`:
- Around line 482-490: The test for lowering the record-arg operator should be
moved out of the large lowering.test.ts into a focused test file (e.g.,
lowering.agg-expr.test.ts) to keep files under 500 lines; create a new test file
that imports MongoAggOperator, MongoAggFieldRef, MongoAggLiteral and
lowerAggExpr and re-create this single-spec test ("lowers record-arg operator
with array values") there, deleting the original spec from the big file so
behavior is unchanged but coverage is split for maintainability.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 28aebee5-e79d-40f0-a77a-1218e9f08039
📒 Files selected for processing (10)
packages/2-mongo-family/4-query/query-ast/src/aggregation-expressions.tspackages/2-mongo-family/4-query/query-ast/test/aggregation-expressions.test-d.tspackages/2-mongo-family/5-query-builders/pipeline-builder/src/accumulator-helpers.tspackages/2-mongo-family/5-query-builders/pipeline-builder/src/expression-helpers.tspackages/2-mongo-family/5-query-builders/pipeline-builder/test/accumulator-helpers.test-d.tspackages/2-mongo-family/5-query-builders/pipeline-builder/test/accumulator-helpers.test.tspackages/2-mongo-family/5-query-builders/pipeline-builder/test/expression-helpers.test-d.tspackages/2-mongo-family/5-query-builders/pipeline-builder/test/expression-helpers.test.tspackages/3-mongo-target/2-mongo-adapter/src/lowering.tspackages/3-mongo-target/2-mongo-adapter/test/lowering.test.ts
✅ Files skipped from review due to trivial changes (3)
- packages/2-mongo-family/5-query-builders/pipeline-builder/test/expression-helpers.test-d.ts
- packages/2-mongo-family/5-query-builders/pipeline-builder/test/expression-helpers.test.ts
- packages/2-mongo-family/5-query-builders/pipeline-builder/test/accumulator-helpers.test.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/3-mongo-target/2-mongo-adapter/src/lowering.ts
- packages/2-mongo-family/5-query-builders/pipeline-builder/test/accumulator-helpers.test-d.ts
- packages/2-mongo-family/4-query/query-ast/test/aggregation-expressions.test-d.ts
- packages/2-mongo-family/4-query/query-ast/src/aggregation-expressions.ts
- packages/2-mongo-family/5-query-builders/pipeline-builder/src/expression-helpers.ts
…tion - fn.zip: inputs is now TypedAggExpr<ArrayField>[] (array of array expressions), matching MongoDB $zip contract. Extended AggRecordArgs to support array values in record args, updated rewrite and lowering. - acc.top/bottom/topN/bottomN: sortBy is now Record<string, 1 | -1> (sort spec document), wrapped with MongoAggLiteral.of() to match MongoDB accumulator contract. - Shape-changing operators (arrayElemAt, firstElem, lastElem, arrayToObject): no longer copy input codecId to output. These operators change the shape, so output uses generic DocField codecId instead of propagating the input array codec.
…ottomN freezeRecordArgs helper ensures array-valued record entries (e.g. zip inputs) are frozen during MongoAggOperator/MongoAggAccumulator construction, maintaining AST immutability. Also adds explicit n property assertion for topN/bottomN accumulator tests to catch regressions.
93584e9 to
5da1e90
Compare
$(cat <<'EOF'
closes TML-2217
Intent
Follow-up to PR #318 which was merged before this review-fix commit landed. Addresses 3 CodeRabbit findings that improve MongoDB contract fidelity and type safety in expression/accumulator helpers.
Narrative
Fix
fn.zip()inputs to accept an array of array expressions — MongoDB's$ziprequiresinputsto be an array of arrays ([<arr1>, <arr2>]), not a single expression. ExtendedAggRecordArgsto support array values in record args, updatedrewriteRecordArgsandlowerExprRecordto handle the new shape.Model
sortByas a sort-spec document for top/bottom accumulators —acc.top,acc.bottom,acc.topN,acc.bottomNpreviously acceptedTypedAggExpr<DocField>forsortBy, allowing arbitrary computed expressions. MongoDB requires a literal sort specification ({ field: 1 | -1 }). Changed the type toReadonly<Record<string, 1 | -1>>and wrapped withMongoAggLiteral.of().Stop propagating input codec through shape-changing operators —
arrayElemAt,firstElem,lastElem, andarrayToObjectwere copying the input'scodecIdto the output. Since these operators transform the shape (e.g., extracting an element from an array), the output now uses the genericDocFieldcodec. Removed the now-unusednullableDocUnaryExprhelper.Behavior changes & evidence
fn.zip({ inputs })type:TypedAggExpr<ArrayField>→TypedAggExpr<ArrayField>[]acc.top/bottom/topN/bottomNsortBy type:TypedAggExpr<DocField>→Readonly<Record<string, 1 | -1>>AggRecordArgstype:Record<string, MongoAggExpr>→Record<string, MongoAggExpr | ReadonlyArray<MongoAggExpr>>Compatibility / migration / risk
Breaking change for any callers of
fn.zip()(must wrap inputs in an array) andacc.top/bottom/topN/bottomN(must pass sort spec object instead ofTypedAggExpr). These are new APIs from #318 with no external consumers yet.Follow-ups / open questions
None — this completes the review cycle from #318.
EOF
)
Summary by CodeRabbit