feat(contract): add value objects and embedded documents#309
Merged
wmadden merged 28 commits intoApr 7, 2026
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughIntroduces composite types (value objects) as a first-class concept in the contract system. Restructures field descriptors from Changes
Sequence Diagram(s)sequenceDiagram
participant PSL as PSL Document
participant Parser as Parser
participant Interpreter as Interpreter
participant Validator as Validator
participant Emitter as Emitter
participant DTS as Generated .d.ts
PSL->>Parser: Parse schema.psl
Parser->>Parser: Extract composite type blocks<br/>(type Address {...})
Parser->>Interpreter: AST with compositeTypes
Interpreter->>Interpreter: Map composites to valueObjects<br/>Map fields to { kind, name/codecId }
Interpreter->>Validator: Contract with valueObjects
Validator->>Validator: Validate value-object<br/>references exist
Validator->>Validator: Validate field modifiers<br/>(reject many + dict)
Validator->>Emitter: Valid Contract
Emitter->>Emitter: Generate type aliases<br/>for value-objects
Emitter->>Emitter: Emit field descriptors<br/>with nested type structure
Emitter->>DTS: ContractBase with<br/>valueObjects & field types
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/2-mongo-family/3-tooling/emitter/src/index.ts (1)
145-158:⚠️ Potential issue | 🟠 MajorValidate scalar members inside union fields too.
Line 147 now exits early for every non-
scalarfield, which meanskind: 'union'fields never validate thecodecIds of their scalar members. A malformed union member will make it through emitter validation and only break later when the generated types are consumed.🛡️ Suggested validation shape
for (const [fieldName, field] of Object.entries(model.fields)) { - const fieldType = field.type; - if (fieldType.kind !== 'scalar') { - continue; - } - const { codecId } = fieldType; - if (!codecId) { - throw new Error(`Field "${fieldName}" on model "${modelName}" is missing codecId`); - } - const match = codecId.match(typeIdRegex); - if (!match || !match[1]) { - throw new Error( - `Field "${fieldName}" on model "${modelName}" has invalid codec ID format "${codecId}". Expected format: ns/name@version`, - ); + const scalarTypes = + field.type.kind === 'scalar' + ? [field.type] + : field.type.kind === 'union' + ? field.type.members.filter( + (member): member is { readonly kind: 'scalar'; readonly codecId: string } => + member.kind === 'scalar', + ) + : []; + + for (const { codecId } of scalarTypes) { + if (!codecId) { + throw new Error(`Field "${fieldName}" on model "${modelName}" is missing codecId`); + } + const match = codecId.match(typeIdRegex); + if (!match || !match[1]) { + throw new Error( + `Field "${fieldName}" on model "${modelName}" has invalid codec ID format "${codecId}". Expected format: ns/name@version`, + ); + } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-mongo-family/3-tooling/emitter/src/index.ts` around lines 145 - 158, The loop currently skips non-scalar fields so scalar members of union fields aren't validated; update the validation inside the iteration over Object.entries(model.fields) to also handle when fieldType.kind === 'union' by iterating the union's member list (e.g., fieldType.types or fieldType.members) and for each member with member.type === 'scalar' validate member.codecId against typeIdRegex exactly like the existing scalar branch (throwing the same errors referencing modelName and fieldName but include the member identifier/index in the message); keep using typeIdRegex and the same error text/format for consistency.packages/2-sql/2-authoring/contract-ts/src/build-contract.ts (1)
373-394:⚠️ Potential issue | 🟠 MajorDon’t bake Postgres JSONB into the shared SQL authoring path.
This builder now emits
pg/jsonb@1/jsonbfor every value-object field regardless ofdefinition.target.targetId. That makesbuildSqlContractFromSemanticDefinition()generate Postgres-specific storage for non-Postgres SQL targets instead of deferring to the target/codec layer.As per coding guidelines,
packages/2-sql/**: “do not place concrete dialect code here” andpackages/{2-sql,3-targets}/**: “Use codec-owned control hooks for storage type behavior in SQL adapters and targets”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/2-authoring/contract-ts/src/build-contract.ts` around lines 373 - 394, The builder is hardcoding Postgres JSONB via JSONB_CODEC_ID/JSONB_NATIVE_TYPE inside buildSqlContractFromSemanticDefinition when handling isValueObjectField; change this to delegate storage codec/nativeType resolution to the codec/target layer instead of baking Postgres specifics: remove use of JSONB_CODEC_ID/JSONB_NATIVE_TYPE in the isValueObjectField branch and instead query codecLookup (or use definition.target.targetId to pick a codec resolver) to obtain the codec id and nativeType for that field (e.g., call codecLookup.getCodec(...) or a target-aware helper) and set columns[field.columnName].type/nativeType from that lookup, falling back to a neutral/generic placeholder or undefined so targets can decide. Ensure references to JSONB_CODEC_ID/JSONB_NATIVE_TYPE are removed and only buildSqlContractFromSemanticDefinition, isValueObjectField, and codecLookup (or definition.target.targetId) are involved.
🧹 Nitpick comments (8)
packages/2-sql/3-tooling/emitter/test/emitter-hook.parameterized-types.test.ts (1)
1-916: File exceeds 500-line limit (916 lines).Consider splitting this test file by logical boundaries. Potential split candidates:
emitter-hook.parameterized-types.basic.test.ts- basic typeParams emission (lines 23-225)emitter-hook.parameterized-types.enums.test.ts- enum unions (lines 227-284)emitter-hook.parameterized-types.typeref.test.ts- typeRef resolution (lines 286-343)emitter-hook.parameterized-types.edge-cases.test.ts- edge cases and backwards compatibility (lines 424-606)emitter-hook.parameterized-types.storage-types.test.ts- storage.types emission (lines 608-851)emitter-hook.parameterized-types.imports.test.ts- parameterizedTypeImports (lines 853-915)As per coding guidelines: "Keep test files under 500 lines to maintain readability and navigability."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/3-tooling/emitter/test/emitter-hook.parameterized-types.test.ts` around lines 1 - 916, The test file is too large—split the single file into multiple smaller test files by logical sections (use the existing describe blocks as boundaries) such as basic parameterized type tests, enum unions, typeRef resolution, edge cases/backwards compatibility, storage.types emission, and imports; for each new file, keep the same imports (e.g., createContract helper and sqlTargetFamilyHook.generateContractTypes) and move the corresponding it() blocks under their matching describe() (e.g., "columns with typeParams", "enum unions", "columns with typeRef", "edge cases", "storage.types emission", "parameterizedTypeImports"), ensuring tests still import TypeRenderEntry and testHashes where needed and that any shared helpers (createContract, testHashes) are either duplicated or exported to a small shared test-utils file to avoid repetition.packages/3-extensions/sql-orm-client/test/integration/include.test.ts (1)
1-710: File exceeds 500-line limit (710 lines).Consider splitting this integration test file by include strategy or feature area:
include.stitching.test.ts- basic one-to-many/one-to-one stitching (lines 68-117)include.aggregates.test.ts- count/sum/avg/min/max aggregations (lines 119-269)include.combine.test.ts- combine() branch tests (lines 271-329)include.lateral.test.ts- lateral strategy tests (lines 331-502)include.correlated.test.ts- correlated strategy tests (lines 504-630)include.nested.test.ts- nested multi-level includes (lines 632-709)As per coding guidelines: "Keep test files under 500 lines to maintain readability and navigability."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/3-extensions/sql-orm-client/test/integration/include.test.ts` around lines 1 - 710, The file is over the 500-line limit; split the large integration test by feature into smaller test files as suggested. Create new test files named include.stitching.test.ts, include.aggregates.test.ts, include.combine.test.ts, include.lateral.test.ts, include.correlated.test.ts, and include.nested.test.ts and move the corresponding it(...) blocks (identified by their test description strings, e.g. "include() stitches one-to-many and one-to-one relations from real rows", "include() supports scalar count() on to-many relations", "include() combine() evaluates branches independently", the lateral-related tests with "lateral" in the description, the correlated/jsonAgg tests mentioning "correlated" or "jsonAgg", and the nested 2-level include test) into those files; keep shared helper functions (createUsersCollection, createPostsCollection, seedUsers, seedPosts, seedComments, seedProfiles, withCollectionRuntime, getTestContext/getTestContract, createUsersCollectionWithCapabilities) imported at the top of each new file so tests compile, remove the moved tests from the original file leaving only any remaining tests and the small type/assert helpers (expectSelectAst, expectDerivedTableSource, etc.), and run the test suite to ensure imports/exports and test timeouts (timeouts.spinUpPpgDev) are preserved and all tests pass.packages/1-framework/0-foundation/contract/src/testing-factories.ts (1)
56-56: UseifDefined()instead of inline conditional spread.Line 56 introduces a new inline conditional spread pattern that the repo explicitly avoids.
As per coding guidelines: "Use `ifDefined()` from `@prisma-next/utils/defined` for conditional object spreads instead of inline conditional spread patterns".♻️ Proposed fix
import type { Contract } from './contract-types'; import type { ContractModel, ContractValueObject, ModelStorageBase } from './domain-types'; import { computeExecutionHash, computeProfileHash, computeStorageHash } from './hashing'; import type { ExecutionSection, ProfileHashBase, StorageBase } from './types'; import { coreHash } from './types'; +import { ifDefined } from '@prisma-next/utils/defined'; @@ - ...(overrides.valueObjects ? { valueObjects: overrides.valueObjects } : {}), + ...ifDefined(overrides.valueObjects, (valueObjects) => ({ valueObjects })),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/1-framework/0-foundation/contract/src/testing-factories.ts` at line 56, The inline conditional spread that adds valueObjects (...(overrides.valueObjects ? { valueObjects: overrides.valueObjects } : {})) should be replaced with the repo-standard ifDefined helper: import ifDefined from '@prisma-next/utils/defined' and use ifDefined to conditionally spread the valueObjects property (referencing overrides.valueObjects) into the object; update the spread expression to use ifDefined and remove the inline ternary, keeping the rest of the factory code unchanged.packages/2-sql/2-authoring/contract-psl/test/interpreter.test.ts (1)
358-594: Move the value-object interpreter coverage into its own test file.Lines 358-594 add a separate value-object/composite-type suite to a file that is now almost 600 lines long. Splitting these cases into
interpreter.value-objects.test.ts(or similar) would keep the interpreter tests readable and aligned with the repo’s file-size rule. 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/2-sql/2-authoring/contract-psl/test/interpreter.test.ts` around lines 358 - 594, The test file has a large block of value-object/composite-type specs that should be split out; create a new test file (e.g., interpreter.value-objects.test.ts) and move the entire suite of tests that reference parsePslDocument and interpretPslDocumentToSqlContract — specifically the tests with titles like "emits composite types as valueObjects", "emits value object field references with valueObject domain type and JSONB storage", "emits scalar list fields with many: true", "emits value object list fields with many: true and valueObject domain type", "emits nested value object references within composite types", and "omits valueObjects from contract when no composite types exist" — into that new file, preserving imports (parsePslDocument, interpretPslDocumentToSqlContract, builtinControlMutationDefaults, and expect utilities) and test bodies so behavior and assertions remain unchanged; remove the moved tests from the original file to keep it under 500 lines and run the test suite to confirm no import or reference regressions.packages/1-framework/0-foundation/contract/test/canonicalization.test.ts (1)
524-559: Split the new value-object canonicalization coverage into its own test file.Lines 524-559 push
canonicalization.test.tspast the repo’s 500-line limit and add a separate concern on top of the existing canonicalization/default/sorting suites. Moving this block to something likecanonicalization.value-objects.test.tswould keep the suite easier to navigate. 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/1-framework/0-foundation/contract/test/canonicalization.test.ts` around lines 524 - 559, The added value-object tests (using orderTopLevel, canonicalizeContractToObject, and minimal) should be split out of canonicalization.test.ts into a new test file (e.g., canonicalization.value-objects.test.ts); move the entire describe('canonicalize with valueObjects', ...) block (including the preceding it('places valueObjects between models and storage' test that references orderTopLevel) into the new file, update imports so minimal, orderTopLevel, and canonicalizeContractToObject are imported/available, and leave canonicalization.test.ts under 500 lines.packages/1-framework/0-foundation/contract/src/validate-domain.ts (1)
176-222: Validation logic is correct but has duplicated iteration.Both
validateValueObjectReferencesandvalidateFieldModifiersiterate over the same field sets (model fields + VO fields). Consider extracting a shared helper to reduce duplication:♻️ Optional: Extract shared field iteration
+function forEachContractField( + contract: DomainContractShape, + callback: (field: unknown, location: string) => void, +): void { + for (const [modelName, model] of Object.entries(contract.models)) { + for (const [fieldName, field] of Object.entries(model.fields)) { + callback(field, `Model "${modelName}" field "${fieldName}"`); + } + } + for (const [voName, vo] of Object.entries(contract.valueObjects ?? {})) { + for (const [fieldName, field] of Object.entries(vo.fields)) { + callback(field, `Value object "${voName}" field "${fieldName}"`); + } + } +} + function validateValueObjectReferences(contract: DomainContractShape, errors: string[]): void { const voNames = new Set(Object.keys(contract.valueObjects ?? {})); - - function checkField(field: unknown, location: string): void { + forEachContractField(contract, (field, location) => { const f = field as FieldLike | undefined; if (f?.type?.kind === 'valueObject' && f.type.name) { if (!voNames.has(f.type.name)) { errors.push( `${location} references value object "${f.type.name}" which does not exist in valueObjects`, ); } } - } - - for (const [modelName, model] of Object.entries(contract.models)) { - for (const [fieldName, field] of Object.entries(model.fields)) { - checkField(field, `Model "${modelName}" field "${fieldName}"`); - } - } - - for (const [voName, vo] of Object.entries(contract.valueObjects ?? {})) { - for (const [fieldName, field] of Object.entries(vo.fields)) { - checkField(field, `Value object "${voName}" field "${fieldName}"`); - } - } + }); } function validateFieldModifiers(contract: DomainContractShape, errors: string[]): void { - function checkField(field: unknown, location: string): void { + forEachContractField(contract, (field, location) => { const f = field as FieldLike | undefined; if (f?.many && f?.dict) { errors.push(`${location} cannot have both "many" and "dict" modifiers`); } - } - - for (const [modelName, model] of Object.entries(contract.models)) { - for (const [fieldName, field] of Object.entries(model.fields)) { - checkField(field, `Model "${modelName}" field "${fieldName}"`); - } - } - - for (const [voName, vo] of Object.entries(contract.valueObjects ?? {})) { - for (const [fieldName, field] of Object.entries(vo.fields)) { - checkField(field, `Value object "${voName}" field "${fieldName}"`); - } - } + }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/1-framework/0-foundation/contract/src/validate-domain.ts` around lines 176 - 222, Extract a shared iterator to avoid duplicated iteration: create a helper function (e.g., iterFields or forEachField) that accepts the DomainContractShape and a callback (location: string, field: unknown) and internally walks contract.models and contract.valueObjects, calling the callback for each field with the same location strings currently used; then replace the per-function for...of loops in validateValueObjectReferences and validateFieldModifiers with calls to this helper and move their inner checkField logic into the callbacks (keeping the existing checkField names or inlined), so only the validation logic remains in those functions while traversal is centralized.packages/2-sql/2-authoring/contract-psl/src/psl-field-resolution.ts (1)
246-258: UseifDefined()for the new optional spreads.The new
many/valueObjectTypeName/scalarCodecIdproperties reintroduce the inline conditional-spread pattern this module already avoids. Keeping them onifDefined()makes the object assembly consistent with the rest of the function.♻️ Suggested cleanup
- ...(isListField ? { many: true as const } : {}), - ...(isValueObjectField ? { valueObjectTypeName: field.typeName } : {}), - ...(scalarCodecId ? { scalarCodecId } : {}), + ...ifDefined('many', isListField ? (true as const) : undefined), + ...ifDefined( + 'valueObjectTypeName', + isValueObjectField ? field.typeName : undefined, + ), + ...ifDefined('scalarCodecId', scalarCodecId),As per coding guidelines, "Use
ifDefined()from@prisma-next/utils/definedfor conditional object spreads instead of inline conditional spread patterns"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/2-authoring/contract-psl/src/psl-field-resolution.ts` around lines 246 - 258, The object pushed into resolvedFields uses inline conditional-spread patterns for many, valueObjectTypeName, and scalarCodecId; replace those with ifDefined() calls (from `@prisma-next/utils/defined`) to match the rest of the function: when isListField is true pass ifDefined('many', true as const), when isValueObjectField pass ifDefined('valueObjectTypeName', field.typeName), and when scalarCodecId is truthy pass ifDefined('scalarCodecId', scalarCodecId) so all optional properties are added via ifDefined in the resolvedFields.push call.packages/2-sql/3-tooling/emitter/test/emitter-hook.generation.basic.test.ts (1)
1079-1275: Split the value-object suite into its own test file.This new
describeadds a separate concern to a file that is already far past the repo's size cap for tests. Moving it to something likeemitter-hook.generation.value-objects.test.tswill keep failures easier to navigate and isolate.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/2-sql/3-tooling/emitter/test/emitter-hook.generation.basic.test.ts` around lines 1079 - 1275, The new "value object type generation" describe block should be moved into its own test file to keep test files under 500 lines: create a new file (e.g., emitter-hook.generation.value-objects.test.ts) and cut the entire describe('value object type generation', ...) block from the existing file and paste it into the new file, retaining the same imports and test helpers used in the block (notably createContract, sqlTargetFamilyHook.generateContractTypes, and testHashes); remove the block from the original file so the tests aren't duplicated and ensure the new file imports any shared setup (fixtures/mocks) required so all specs run unchanged under the test runner.
🤖 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/1-framework/0-foundation/contract/test/validate-domain.test.ts`:
- Around line 451-469: Add the missing type import for DomainContractShape to
the test file's import from ../src/validate-domain so the type used in the test
(DomainContractShape) is available; update the import statement that currently
brings in validateContractDomain to also include the type name
DomainContractShape (use a type import) so references to DomainContractShape in
the test (e.g., the contract const and assertions) compile.
In `@packages/2-mongo-family/1-foundation/mongo-contract/src/contract-schema.ts`:
- Around line 16-24: FieldTypeSchema currently excludes union types so fields
with type.kind === 'union' fail validation; update FieldTypeSchema to include
the UnionFieldTypeSchema (or the schema that models union types) in addition to
ScalarFieldTypeSchema and ValueObjectFieldTypeSchema, and adjust RawFieldSchema
to accept union-specific keys (discriminator, variants, base) when the type is a
union so discriminator/variants/base are not rejected; look for FieldTypeSchema,
RawFieldSchema, ScalarFieldTypeSchema, ValueObjectFieldTypeSchema and the
existing UnionFieldTypeSchema (or equivalent) and add/or-chain it and permit the
union keys in RawFieldSchema.
- Line 97: Replace the loose 'valueObjects?' Record<string, unknown> entry with
a proper Arktype schema: define a ValueObject schema (e.g., valueObjectSchema)
that requires a 'fields' map with validated field definitions (mirroring the
structure used by the existing 'models' schema) and then use a keyed record type
(Record<string, valueObjectSchema>) for the top-level 'valueObjects' property in
the contract schema; update the main contract schema (where 'models' is defined)
to reference this new valueObjects schema so Arktype enforces shapes at
validation time.
In `@packages/2-sql/1-core/contract/src/validators.ts`:
- Around line 166-171: ModelFieldSchema is too permissive because 'nullable' and
'type' are optional; update the schema to require both properties so structural
validation enforces the new contract shape. Change the ModelFieldSchema entries
from 'nullable?' to 'nullable' and from 'type?' to 'type' (keep using
ContractFieldTypeSchema for the type value), and then run tests to ensure the
Arktype validator now rejects field descriptors missing those keys. Ensure you
reference ModelFieldSchema and ContractFieldTypeSchema when making the change so
no manual post-validation is needed.
In `@packages/2-sql/2-authoring/contract-psl/src/interpreter.ts`:
- Around line 923-944: buildValueObjects is invoked after the function's
early-return diagnostics check so any PSL_UNSUPPORTED_FIELD_TYPE diagnostics it
generates won't trigger a failure; move the call to buildValueObjects earlier
(before the existing early-return check that inspects diagnostics and returns
err) so its diagnostics are collected prior to that check, then proceed to call
patchModelDomainFields/patchModels and construct patchedContract as before;
ensure you reference buildValueObjects, patchModelDomainFields (and the
patchedContract construction) so the reordered call is easy to locate.
In `@packages/2-sql/2-authoring/contract-psl/src/psl-field-resolution.ts`:
- Around line 106-178: The code lets fields that are converted to JSON
(isValueObjectField or isListField) silently bypass `@pgvector.column` validation;
to fix, check for the pgvector attribute (getAttribute(..., 'pgvector.column'))
before the JSON branches and, if present and either isValueObjectField or
isListField, push a PSL_INVALID_ATTRIBUTE_ARGUMENT (or appropriate) diagnostic
rejecting `@pgvector.column` on JSON-backed fields (use model.name, field.name and
pgvectorColumnAttribute.span for the diagnostic), then continue; keep the
existing pgvector handling for non-JSON cases (the later pgvectorColumnAttribute
usage, resolveColumnDescriptor, and pgvectorVectorConstructor code) intact.
In `@packages/2-sql/2-authoring/contract-ts/src/build-contract.ts`:
- Around line 50-62: RuntimeModelState currently only records
value-object-backed fields via ValueObjectFieldRef and
buildSqlContractFromSemanticDefinition only populates that map for value-object
fields, causing scalar list metadata (field.many) to be lost before
buildContract reconstructs contract.models[*].fields. Fix by extending
RuntimeModelState to include a unified domainFieldRefs map that records every
field (both scalar and value-object) with its name and many flag; in
buildSqlContractFromSemanticDefinition populate domainFieldRefs for scalar
fields as well (copy field.many into the map) and in buildContract read from
domainFieldRefs when constructing domainFields so scalar list modifiers are
preserved. Ensure you update the types and usages of RuntimeModelState, the map
name (domainFieldRefs), and the logic in buildSqlContractFromSemanticDefinition
and buildContract accordingly.
In `@packages/2-sql/2-authoring/contract-ts/src/semantic-contract.ts`:
- Around line 75-78: SqlSemanticValueObjectNode currently restricts fields to
SqlSemanticFieldNode[], preventing nested value objects and self-references;
update the type of SqlSemanticValueObjectNode.fields to allow recursive
value-object nodes (e.g., union with SqlSemanticValueObjectNode or a new
SqlSemanticSemanticField-like wrapper) and adjust any consumers such as
buildSqlContractFromSemanticDefinition() to handle the nested structure during
contract building and validation so nested value objects are represented and
traversed correctly.
In `@packages/2-sql/3-tooling/emitter/src/index.ts`:
- Around line 293-295: The emitted contract currently forces
contract.valueObjects to appear present by casting it to Record<...,
ContractValueObject> | undefined; instead keep it optional: stop coercing
contract.valueObjects to a non-optional type and update the calls to
generateValueObjectTypeAliases(valueObjects) and
generateValueObjectsDescriptorType(valueObjects) to accept valueObjects possibly
being undefined so the generated .d.ts emits valueObjects as an optional
property (valueObjects?) and the descriptor/alias generators handle undefined
input. Also apply the same change to the other occurrence (the block that calls
the same generators around generateValueObjectsDescriptorType and
generateValueObjectTypeAliases) so both emitted type aliases and descriptor
types reflect optional valueObjects rather than a guaranteed object.
- Around line 28-34: isNonScalarField currently returns false for any field
whose type.kind === 'scalar', which causes scalar fields with collection
modifiers (many/dict) to bypass generateContractFieldDescriptor and lose their
modifiers; update isNonScalarField to treat scalar types that include collection
modifiers as non-scalar by checking for the modifier flags on the field/type
(e.g., inspect f['many'], f['dict'], t['many'], t['dict'] or a modifier
object/property used in your schema) and return true when such modifier flags
are present so generateContractFieldDescriptor (and its modifier emission logic)
is used for those fields.
---
Outside diff comments:
In `@packages/2-mongo-family/3-tooling/emitter/src/index.ts`:
- Around line 145-158: The loop currently skips non-scalar fields so scalar
members of union fields aren't validated; update the validation inside the
iteration over Object.entries(model.fields) to also handle when fieldType.kind
=== 'union' by iterating the union's member list (e.g., fieldType.types or
fieldType.members) and for each member with member.type === 'scalar' validate
member.codecId against typeIdRegex exactly like the existing scalar branch
(throwing the same errors referencing modelName and fieldName but include the
member identifier/index in the message); keep using typeIdRegex and the same
error text/format for consistency.
In `@packages/2-sql/2-authoring/contract-ts/src/build-contract.ts`:
- Around line 373-394: The builder is hardcoding Postgres JSONB via
JSONB_CODEC_ID/JSONB_NATIVE_TYPE inside buildSqlContractFromSemanticDefinition
when handling isValueObjectField; change this to delegate storage
codec/nativeType resolution to the codec/target layer instead of baking Postgres
specifics: remove use of JSONB_CODEC_ID/JSONB_NATIVE_TYPE in the
isValueObjectField branch and instead query codecLookup (or use
definition.target.targetId to pick a codec resolver) to obtain the codec id and
nativeType for that field (e.g., call codecLookup.getCodec(...) or a
target-aware helper) and set columns[field.columnName].type/nativeType from that
lookup, falling back to a neutral/generic placeholder or undefined so targets
can decide. Ensure references to JSONB_CODEC_ID/JSONB_NATIVE_TYPE are removed
and only buildSqlContractFromSemanticDefinition, isValueObjectField, and
codecLookup (or definition.target.targetId) are involved.
---
Nitpick comments:
In `@packages/1-framework/0-foundation/contract/src/testing-factories.ts`:
- Line 56: The inline conditional spread that adds valueObjects
(...(overrides.valueObjects ? { valueObjects: overrides.valueObjects } : {}))
should be replaced with the repo-standard ifDefined helper: import ifDefined
from '@prisma-next/utils/defined' and use ifDefined to conditionally spread the
valueObjects property (referencing overrides.valueObjects) into the object;
update the spread expression to use ifDefined and remove the inline ternary,
keeping the rest of the factory code unchanged.
In `@packages/1-framework/0-foundation/contract/src/validate-domain.ts`:
- Around line 176-222: Extract a shared iterator to avoid duplicated iteration:
create a helper function (e.g., iterFields or forEachField) that accepts the
DomainContractShape and a callback (location: string, field: unknown) and
internally walks contract.models and contract.valueObjects, calling the callback
for each field with the same location strings currently used; then replace the
per-function for...of loops in validateValueObjectReferences and
validateFieldModifiers with calls to this helper and move their inner checkField
logic into the callbacks (keeping the existing checkField names or inlined), so
only the validation logic remains in those functions while traversal is
centralized.
In `@packages/1-framework/0-foundation/contract/test/canonicalization.test.ts`:
- Around line 524-559: The added value-object tests (using orderTopLevel,
canonicalizeContractToObject, and minimal) should be split out of
canonicalization.test.ts into a new test file (e.g.,
canonicalization.value-objects.test.ts); move the entire describe('canonicalize
with valueObjects', ...) block (including the preceding it('places valueObjects
between models and storage' test that references orderTopLevel) into the new
file, update imports so minimal, orderTopLevel, and canonicalizeContractToObject
are imported/available, and leave canonicalization.test.ts under 500 lines.
In `@packages/2-sql/2-authoring/contract-psl/src/psl-field-resolution.ts`:
- Around line 246-258: The object pushed into resolvedFields uses inline
conditional-spread patterns for many, valueObjectTypeName, and scalarCodecId;
replace those with ifDefined() calls (from `@prisma-next/utils/defined`) to match
the rest of the function: when isListField is true pass ifDefined('many', true
as const), when isValueObjectField pass ifDefined('valueObjectTypeName',
field.typeName), and when scalarCodecId is truthy pass
ifDefined('scalarCodecId', scalarCodecId) so all optional properties are added
via ifDefined in the resolvedFields.push call.
In `@packages/2-sql/2-authoring/contract-psl/test/interpreter.test.ts`:
- Around line 358-594: The test file has a large block of
value-object/composite-type specs that should be split out; create a new test
file (e.g., interpreter.value-objects.test.ts) and move the entire suite of
tests that reference parsePslDocument and interpretPslDocumentToSqlContract —
specifically the tests with titles like "emits composite types as valueObjects",
"emits value object field references with valueObject domain type and JSONB
storage", "emits scalar list fields with many: true", "emits value object list
fields with many: true and valueObject domain type", "emits nested value object
references within composite types", and "omits valueObjects from contract when
no composite types exist" — into that new file, preserving imports
(parsePslDocument, interpretPslDocumentToSqlContract,
builtinControlMutationDefaults, and expect utilities) and test bodies so
behavior and assertions remain unchanged; remove the moved tests from the
original file to keep it under 500 lines and run the test suite to confirm no
import or reference regressions.
In `@packages/2-sql/3-tooling/emitter/test/emitter-hook.generation.basic.test.ts`:
- Around line 1079-1275: The new "value object type generation" describe block
should be moved into its own test file to keep test files under 500 lines:
create a new file (e.g., emitter-hook.generation.value-objects.test.ts) and cut
the entire describe('value object type generation', ...) block from the existing
file and paste it into the new file, retaining the same imports and test helpers
used in the block (notably createContract,
sqlTargetFamilyHook.generateContractTypes, and testHashes); remove the block
from the original file so the tests aren't duplicated and ensure the new file
imports any shared setup (fixtures/mocks) required so all specs run unchanged
under the test runner.
In
`@packages/2-sql/3-tooling/emitter/test/emitter-hook.parameterized-types.test.ts`:
- Around line 1-916: The test file is too large—split the single file into
multiple smaller test files by logical sections (use the existing describe
blocks as boundaries) such as basic parameterized type tests, enum unions,
typeRef resolution, edge cases/backwards compatibility, storage.types emission,
and imports; for each new file, keep the same imports (e.g., createContract
helper and sqlTargetFamilyHook.generateContractTypes) and move the corresponding
it() blocks under their matching describe() (e.g., "columns with typeParams",
"enum unions", "columns with typeRef", "edge cases", "storage.types emission",
"parameterizedTypeImports"), ensuring tests still import TypeRenderEntry and
testHashes where needed and that any shared helpers (createContract, testHashes)
are either duplicated or exported to a small shared test-utils file to avoid
repetition.
In `@packages/3-extensions/sql-orm-client/test/integration/include.test.ts`:
- Around line 1-710: The file is over the 500-line limit; split the large
integration test by feature into smaller test files as suggested. Create new
test files named include.stitching.test.ts, include.aggregates.test.ts,
include.combine.test.ts, include.lateral.test.ts, include.correlated.test.ts,
and include.nested.test.ts and move the corresponding it(...) blocks (identified
by their test description strings, e.g. "include() stitches one-to-many and
one-to-one relations from real rows", "include() supports scalar count() on
to-many relations", "include() combine() evaluates branches independently", the
lateral-related tests with "lateral" in the description, the correlated/jsonAgg
tests mentioning "correlated" or "jsonAgg", and the nested 2-level include test)
into those files; keep shared helper functions (createUsersCollection,
createPostsCollection, seedUsers, seedPosts, seedComments, seedProfiles,
withCollectionRuntime, getTestContext/getTestContract,
createUsersCollectionWithCapabilities) imported at the top of each new file so
tests compile, remove the moved tests from the original file leaving only any
remaining tests and the small type/assert helpers (expectSelectAst,
expectDerivedTableSource, etc.), and run the test suite to ensure
imports/exports and test timeouts (timeouts.spinUpPpgDev) are preserved and all
tests pass.
🪄 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: 6f68a651-5323-4ea1-a690-5279288cc87e
⛔ Files ignored due to path filters (14)
packages/2-sql/4-lanes/sql-builder/test/fixtures/generated/contract.d.tsis excluded by!**/generated/**packages/2-sql/4-lanes/sql-builder/test/fixtures/generated/contract.jsonis excluded by!**/generated/**packages/3-extensions/sql-orm-client/test/fixtures/generated/contract.d.tsis excluded by!**/generated/**packages/3-extensions/sql-orm-client/test/fixtures/generated/contract.jsonis excluded by!**/generated/**pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlprojects/orm-consolidation/plan.mdis excluded by!projects/**projects/orm-consolidation/plans/phase-1.75c-value-objects-plan.mdis excluded by!projects/**projects/orm-consolidation/plans/value-objects-design.mdis excluded by!projects/**test/e2e/framework/test/fixtures/generated/contract.d.tsis excluded by!**/generated/**test/integration/test/mongo/fixtures/generated/contract.d.tsis excluded by!**/generated/**test/integration/test/value-objects/fixtures/generated/mongo-contract.d.tsis excluded by!**/generated/**test/integration/test/value-objects/fixtures/generated/mongo-contract.jsonis excluded by!**/generated/**test/integration/test/value-objects/fixtures/generated/sql-contract.d.tsis excluded by!**/generated/**test/integration/test/value-objects/fixtures/generated/sql-contract.jsonis excluded by!**/generated/**
📒 Files selected for processing (96)
packages/1-framework/0-foundation/contract/src/canonicalization.tspackages/1-framework/0-foundation/contract/src/contract-types.tspackages/1-framework/0-foundation/contract/src/domain-types.tspackages/1-framework/0-foundation/contract/src/exports/types.tspackages/1-framework/0-foundation/contract/src/testing-factories.tspackages/1-framework/0-foundation/contract/src/validate-contract.tspackages/1-framework/0-foundation/contract/src/validate-domain.tspackages/1-framework/0-foundation/contract/test/canonicalization.test.tspackages/1-framework/0-foundation/contract/test/contract-types.test-d.tspackages/1-framework/0-foundation/contract/test/contract-types.test.tspackages/1-framework/0-foundation/contract/test/domain-types.test.tspackages/1-framework/0-foundation/contract/test/validate-contract.test.tspackages/1-framework/0-foundation/contract/test/validate-domain.test.tspackages/1-framework/2-authoring/psl-parser/src/exports/types.tspackages/1-framework/2-authoring/psl-parser/src/parser.tspackages/1-framework/2-authoring/psl-parser/src/types.tspackages/1-framework/2-authoring/psl-parser/test/parser.test.tspackages/1-framework/3-tooling/emitter/src/domain-type-generation.tspackages/1-framework/3-tooling/emitter/test/domain-type-generation.test.tspackages/1-framework/3-tooling/emitter/test/emitter.integration.test.tspackages/1-framework/3-tooling/emitter/test/emitter.roundtrip.test.tspackages/1-framework/3-tooling/emitter/test/emitter.test.tspackages/2-mongo-family/1-foundation/mongo-contract/src/contract-schema.tspackages/2-mongo-family/1-foundation/mongo-contract/src/contract-types.tspackages/2-mongo-family/1-foundation/mongo-contract/test/contract-types.test-d.tspackages/2-mongo-family/1-foundation/mongo-contract/test/fixtures/orm-contract.d.tspackages/2-mongo-family/1-foundation/mongo-contract/test/fixtures/orm-contract.jsonpackages/2-mongo-family/1-foundation/mongo-contract/test/validate-domain.test.tspackages/2-mongo-family/1-foundation/mongo-contract/test/validate-storage.test.tspackages/2-mongo-family/1-foundation/mongo-contract/test/validate.test.tspackages/2-mongo-family/2-authoring/contract-psl/src/interpreter.tspackages/2-mongo-family/2-authoring/contract-psl/test/interpreter.test.tspackages/2-mongo-family/3-tooling/emitter/src/index.tspackages/2-mongo-family/3-tooling/emitter/test/emitter-hook.e2e.test.tspackages/2-mongo-family/3-tooling/emitter/test/emitter-hook.generation.test.tspackages/2-mongo-family/3-tooling/emitter/test/emitter-hook.structure.test.tspackages/2-mongo-family/3-tooling/emitter/test/emitter-hook.types.test.tspackages/2-mongo-family/3-tooling/emitter/test/fixtures/blog-contract.tspackages/2-mongo-family/5-query-builders/test/value-object-inputs.test-d.tspackages/2-mongo-family/7-runtime/test/fixtures/contract.d.tspackages/2-sql/1-core/contract/src/factories.tspackages/2-sql/1-core/contract/src/validate.tspackages/2-sql/1-core/contract/src/validators.tspackages/2-sql/1-core/contract/test/domain-types.test.tspackages/2-sql/1-core/contract/test/factories.test.tspackages/2-sql/1-core/contract/test/validate.test.tspackages/2-sql/2-authoring/contract-psl/src/interpreter.tspackages/2-sql/2-authoring/contract-psl/src/psl-field-resolution.tspackages/2-sql/2-authoring/contract-psl/test/interpreter.diagnostics.test.tspackages/2-sql/2-authoring/contract-psl/test/interpreter.test.tspackages/2-sql/2-authoring/contract-psl/test/provider.test.tspackages/2-sql/2-authoring/contract-ts/src/build-contract.tspackages/2-sql/2-authoring/contract-ts/src/semantic-contract.tspackages/2-sql/2-authoring/contract-ts/test/contract-builder.value-objects.test.tspackages/2-sql/2-authoring/contract-ts/test/contract.edge-cases.test.tspackages/2-sql/2-authoring/contract-ts/test/contract.logic.test.tspackages/2-sql/2-authoring/contract-ts/test/contract.model-validation.test.tspackages/2-sql/2-authoring/contract-ts/test/contract.normalization.test.tspackages/2-sql/2-authoring/contract-ts/test/fixtures/contract-with-relations.d.tspackages/2-sql/2-authoring/contract-ts/test/fixtures/contract.d.tspackages/2-sql/3-tooling/emitter/src/index.tspackages/2-sql/3-tooling/emitter/test/emitter-hook.generation.advanced.test.tspackages/2-sql/3-tooling/emitter/test/emitter-hook.generation.basic.test.tspackages/2-sql/3-tooling/emitter/test/emitter-hook.parameterized-types.test.tspackages/2-sql/4-lanes/relational-core/src/types.tspackages/2-sql/4-lanes/relational-core/test/fixtures/contract.d.tspackages/2-sql/4-lanes/relational-core/test/fixtures/contract.jsonpackages/2-sql/4-lanes/relational-core/test/schema.types.test-d.tspackages/2-sql/9-family/test/emit-parameterized.test.tspackages/3-extensions/sql-orm-client/src/collection-contract.tspackages/3-extensions/sql-orm-client/src/filters.tspackages/3-extensions/sql-orm-client/src/model-accessor.tspackages/3-extensions/sql-orm-client/test/create-input.test-d.tspackages/3-extensions/sql-orm-client/test/generated-contract-types.test-d.tspackages/3-extensions/sql-orm-client/test/include-cardinality.test-d.tspackages/3-extensions/sql-orm-client/test/integration/create.test.tspackages/3-extensions/sql-orm-client/test/integration/delete.test.tspackages/3-extensions/sql-orm-client/test/integration/first.test.tspackages/3-extensions/sql-orm-client/test/integration/include.test.tspackages/3-extensions/sql-orm-client/test/integration/nested-mutations.test.tspackages/3-extensions/sql-orm-client/test/integration/orm.test.tspackages/3-extensions/sql-orm-client/test/integration/runtime-helpers.tspackages/3-extensions/sql-orm-client/test/integration/self-relations.test.tspackages/3-extensions/sql-orm-client/test/integration/update.test.tspackages/3-extensions/sql-orm-client/test/integration/upsert.test.tspackages/3-extensions/sql-orm-client/test/query-plan-mutations.test.tspackages/3-extensions/sql-orm-client/test/value-object-inputs.test-d.tspackages/3-extensions/sql-orm-client/vitest.config.tstest/integration/package.jsontest/integration/test/cli.emit-cli-process.e2e.test.tstest/integration/test/cli.emit-command.test.tstest/integration/test/contract-imports.test.tstest/integration/test/fixtures/contract.d.tstest/integration/test/mongo/fixtures/contract.tstest/integration/test/value-objects/value-objects.e2e.test.tstest/integration/test/value-objects/value-objects.integration.test.ts
wmadden
force-pushed
the
tml-2206-value-objects-embedded-documents-in-both-families-adr-178
branch
from
e9c7c73 to
ab40adf
Compare
@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: |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
packages/1-framework/3-tooling/emitter/test/domain-type-generation.test.ts (1)
1-676: File exceeds 500-line guideline - consider splitting by functionality.This test file is 676 lines. Per coding guidelines, test files should be kept under 500 lines. The file has clear logical groupings that could be split:
domain-type-generation.serialization.test.ts(serializeValue, serializeObjectKey, serializeExecutionType)domain-type-generation.models.test.ts(generateModelFieldsType, generateModelsType, generateRootsType, generateModelRelationsType)domain-type-generation.imports.test.ts(deduplicateImports, generateImportLines, generateCodecTypeIntersection)domain-type-generation.value-objects.test.ts(generateFieldResolvedType, generateValueObjectType, generateContractFieldDescriptor, generateValueObjectsDescriptorType, generateValueObjectTypeAliases)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/1-framework/3-tooling/emitter/test/domain-type-generation.test.ts` around lines 1 - 676, The test file is too large (>500 lines); split it into smaller focused test files grouping related specs: create domain-type-generation.serialization.test.ts for serializeValue, serializeObjectKey, serializeExecutionType; domain-type-generation.models.test.ts for generateModelFieldsType, generateModelsType, generateRootsType, generateModelRelationsType; domain-type-generation.imports.test.ts for deduplicateImports, generateImportLines, generateCodecTypeIntersection; and domain-type-generation.value-objects.test.ts for generateFieldResolvedType, generateValueObjectType, generateContractFieldDescriptor, generateValueObjectsDescriptorType, generateValueObjectTypeAliases; move the relevant describe/it blocks into those files, keep imports (vitest, types) at top of each new file, preserve helper factories (e.g., makeModel) where used, and run tests to ensure no breakage from renamed files or missing shared helpers.packages/2-sql/3-tooling/emitter/test/emitter-hook.generation.basic.test.ts (1)
1-1251: File significantly exceeds 500-line guideline - split recommended.At 1251 lines, this file is over 2x the recommended limit. Consider splitting into:
emitter-hook.generation.contract-shape.test.ts(Contract/TypeMaps shape tests)emitter-hook.generation.storage.test.ts(uniques, indexes, foreignKeys, primaryKey tests)emitter-hook.generation.fields.test.ts(nullable columns, missing references, parameterized types)emitter-hook.generation.value-objects.test.ts(value object type generation)emitter-hook.generation.execution.test.ts(execution section tests)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/3-tooling/emitter/test/emitter-hook.generation.basic.test.ts` around lines 1 - 1251, The test file emitter-hook.generation.basic.test.ts is too large (1251 lines); split the tests under the top-level describe 'sql-target-family-hook' into smaller files so each focuses a single area: move Contract/TypeMaps shape tests (references: "Contract and TypeMaps shape", generateContractDts, sqlEmission) into emitter-hook.generation.contract-shape.test.ts; move storage-related tests (uniques/indexes/foreignKeys/primaryKey, references to storage tables and createContract) into emitter-hook.generation.storage.test.ts; move field-related tests (nullable columns, missing references, parameterized type renderer, codec fallbacks — see tests mentioning "nullable", "typeParams", parameterizedRenderers) into emitter-hook.generation.fields.test.ts; move value object tests (valueObjects, Exported type aliases, self-referencing NavItem — see "value object type generation" section, generateContractDts) into emitter-hook.generation.value-objects.test.ts; and move execution tests (tests mentioning execution, ExecutionHashBase, execution defaults) into emitter-hook.generation.execution.test.ts; ensure each new file imports the shared helpers (createContract, testHashes, generateContractDts, sqlEmission) and preserves test names and assertions exactly.packages/2-sql/2-authoring/contract-psl/test/interpreter.test.ts (1)
358-594: Split the new value-object coverage into its own test file.These additions push
interpreter.test.tspast 500 lines, which makes the suite harder to navigate and maintain. Moving the composite/value-object cases into something likeinterpreter.value-objects.test.tswould keep the core interpreter cases focused.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/2-sql/2-authoring/contract-psl/test/interpreter.test.ts` around lines 358 - 594, The interpreter.test.ts file has grown past the 500-line guideline due to several new value-object/composite tests; split these into a new test file (e.g., interpreter.value-objects.test.ts) by moving the it blocks whose descriptions start with "emits composite types as valueObjects", "emits value object field references with valueObject domain type and JSONB storage", "emits scalar list fields with many: true", "emits value object list fields with many: true and valueObject domain type", "emits nested value object references within composite types", and "omits valueObjects from contract when no composite types exist" into the new file; keep imports for parsePslDocument, interpretPslDocumentToSqlContract, and builtinControlMutationDefaults and preserve test bodies and assertions unchanged so the tests still call interpretPslDocumentToSqlContract and assert on result.value.*; update any test runner exports if needed and remove the moved tests from the original interpreter.test.ts to bring it under 500 lines.
🤖 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/1-framework/0-foundation/contract/src/validate-domain.ts`:
- Around line 170-201: The validation currently only checks direct valueObject
types in checkField and misses valueObjects nested inside union types; update
validateValueObjectReferences by enhancing checkField (and FieldLike handling)
to detect when f.type.kind === 'union' and iterate its members, and for each
member with kind === 'valueObject' validate member.name exists in voNames
(pushing the same error message format); keep the existing direct check for
f.type.kind === 'valueObject' and reuse the same error text and location
formatting so unresolved value-object members inside unions on models or value
objects are caught.
In `@packages/2-mongo-family/1-foundation/mongo-contract/src/contract-types.ts`:
- Around line 58-74: InferFieldBaseType currently only handles 'scalar' and
'valueObject' kinds, so fields with kind: 'union' fall through to unknown;
update InferFieldBaseType to handle the union variant (or explicitly narrow it
away). Add a branch like TFieldType extends { kind: 'union'; options: infer
Options } ? (Options extends readonly any[] ? Infer the per-option types via
InferFieldType and produce a union of those outputs) : unknown, using the
existing InferFieldType, TValueObjects and TCodecTypes to compute each option's
type; ensure the new branch is placed before the final fallback so union field
types are resolved into the appropriate union of inferred types.
- Around line 52-56: The ExtractValueObjects<TContract> conditional currently
falls back to Record<string, never>, which yields keyof as string and causes
value-object branches to resolve incorrectly when valueObjects is optional;
change the fallback to an empty-keyed type (e.g., {} or Record<never, never>) so
missing valueObjects is treated as no keys. Update the
ExtractValueObjects<TContract> definition to use that empty-keyed fallback while
keeping the infer VO extends Record<string, ContractValueObject> branch intact
(referencing ExtractValueObjects, valueObjects, and ContractValueObject).
In `@test/integration/test/value-objects/value-objects.integration.test.ts`:
- Around line 191-200: The test currently accepts multiple naming variants
(User/user and homeAddress/home_address) which hides regressions; change the
assertion to check the single canonical path used elsewhere: cast
contract.storage to the same shape, then access
storage.tables['user'].columns['homeAddress'] (no fallbacks) into a variable
(e.g., userTable and homeAddressColumn) and assert it's defined and that
homeAddressColumn.nativeType === 'jsonb', removing the alternate checks for
'User' and 'home_address'.
---
Nitpick comments:
In `@packages/1-framework/3-tooling/emitter/test/domain-type-generation.test.ts`:
- Around line 1-676: The test file is too large (>500 lines); split it into
smaller focused test files grouping related specs: create
domain-type-generation.serialization.test.ts for serializeValue,
serializeObjectKey, serializeExecutionType;
domain-type-generation.models.test.ts for generateModelFieldsType,
generateModelsType, generateRootsType, generateModelRelationsType;
domain-type-generation.imports.test.ts for deduplicateImports,
generateImportLines, generateCodecTypeIntersection; and
domain-type-generation.value-objects.test.ts for generateFieldResolvedType,
generateValueObjectType, generateContractFieldDescriptor,
generateValueObjectsDescriptorType, generateValueObjectTypeAliases; move the
relevant describe/it blocks into those files, keep imports (vitest, types) at
top of each new file, preserve helper factories (e.g., makeModel) where used,
and run tests to ensure no breakage from renamed files or missing shared
helpers.
In `@packages/2-sql/2-authoring/contract-psl/test/interpreter.test.ts`:
- Around line 358-594: The interpreter.test.ts file has grown past the 500-line
guideline due to several new value-object/composite tests; split these into a
new test file (e.g., interpreter.value-objects.test.ts) by moving the it blocks
whose descriptions start with "emits composite types as valueObjects", "emits
value object field references with valueObject domain type and JSONB storage",
"emits scalar list fields with many: true", "emits value object list fields with
many: true and valueObject domain type", "emits nested value object references
within composite types", and "omits valueObjects from contract when no composite
types exist" into the new file; keep imports for parsePslDocument,
interpretPslDocumentToSqlContract, and builtinControlMutationDefaults and
preserve test bodies and assertions unchanged so the tests still call
interpretPslDocumentToSqlContract and assert on result.value.*; update any test
runner exports if needed and remove the moved tests from the original
interpreter.test.ts to bring it under 500 lines.
In `@packages/2-sql/3-tooling/emitter/test/emitter-hook.generation.basic.test.ts`:
- Around line 1-1251: The test file emitter-hook.generation.basic.test.ts is too
large (1251 lines); split the tests under the top-level describe
'sql-target-family-hook' into smaller files so each focuses a single area: move
Contract/TypeMaps shape tests (references: "Contract and TypeMaps shape",
generateContractDts, sqlEmission) into
emitter-hook.generation.contract-shape.test.ts; move storage-related tests
(uniques/indexes/foreignKeys/primaryKey, references to storage tables and
createContract) into emitter-hook.generation.storage.test.ts; move field-related
tests (nullable columns, missing references, parameterized type renderer, codec
fallbacks — see tests mentioning "nullable", "typeParams",
parameterizedRenderers) into emitter-hook.generation.fields.test.ts; move value
object tests (valueObjects, Exported type aliases, self-referencing NavItem —
see "value object type generation" section, generateContractDts) into
emitter-hook.generation.value-objects.test.ts; and move execution tests (tests
mentioning execution, ExecutionHashBase, execution defaults) into
emitter-hook.generation.execution.test.ts; ensure each new file imports the
shared helpers (createContract, testHashes, generateContractDts, sqlEmission)
and preserves test names and assertions exactly.
🪄 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: 58e806c7-72f2-4152-9943-e9fc620113ed
⛔ Files ignored due to path filters (16)
packages/2-sql/4-lanes/sql-builder/test/fixtures/generated/contract.d.tsis excluded by!**/generated/**packages/2-sql/4-lanes/sql-builder/test/fixtures/generated/contract.jsonis excluded by!**/generated/**packages/3-extensions/sql-orm-client/test/fixtures/generated/contract.d.tsis excluded by!**/generated/**packages/3-extensions/sql-orm-client/test/fixtures/generated/contract.jsonis excluded by!**/generated/**pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlprojects/orm-consolidation/plan.mdis excluded by!projects/**projects/orm-consolidation/plans/phase-1.75c-value-objects-plan.mdis excluded by!projects/**projects/orm-consolidation/plans/value-objects-design.mdis excluded by!projects/**test/e2e/framework/test/fixtures/generated/contract.d.tsis excluded by!**/generated/**test/e2e/framework/test/fixtures/generated/contract.jsonis excluded by!**/generated/**test/integration/test/mongo/fixtures/generated/contract.d.tsis excluded by!**/generated/**test/integration/test/mongo/fixtures/generated/contract.jsonis excluded by!**/generated/**test/integration/test/value-objects/fixtures/generated/mongo-contract.d.tsis excluded by!**/generated/**test/integration/test/value-objects/fixtures/generated/mongo-contract.jsonis excluded by!**/generated/**test/integration/test/value-objects/fixtures/generated/sql-contract.d.tsis excluded by!**/generated/**test/integration/test/value-objects/fixtures/generated/sql-contract.jsonis excluded by!**/generated/**
📒 Files selected for processing (117)
examples/mongo-demo/src/contract.d.tsexamples/mongo-demo/src/contract.jsonexamples/prisma-next-demo/src/prisma/contract.d.tsexamples/prisma-next-demo/src/prisma/contract.jsonpackages/1-framework/0-foundation/contract/src/canonicalization.tspackages/1-framework/0-foundation/contract/src/contract-types.tspackages/1-framework/0-foundation/contract/src/domain-types.tspackages/1-framework/0-foundation/contract/src/exports/types.tspackages/1-framework/0-foundation/contract/src/testing-factories.tspackages/1-framework/0-foundation/contract/src/validate-contract.tspackages/1-framework/0-foundation/contract/src/validate-domain.tspackages/1-framework/0-foundation/contract/test/canonicalization.test.tspackages/1-framework/0-foundation/contract/test/contract-types.test-d.tspackages/1-framework/0-foundation/contract/test/contract-types.test.tspackages/1-framework/0-foundation/contract/test/domain-types.test.tspackages/1-framework/0-foundation/contract/test/validate-contract.test.tspackages/1-framework/0-foundation/contract/test/validate-domain.test.tspackages/1-framework/2-authoring/psl-parser/src/exports/index.tspackages/1-framework/2-authoring/psl-parser/src/exports/types.tspackages/1-framework/2-authoring/psl-parser/src/parser.tspackages/1-framework/2-authoring/psl-parser/src/types.tspackages/1-framework/2-authoring/psl-parser/test/parser.test.tspackages/1-framework/3-tooling/emitter/src/domain-type-generation.tspackages/1-framework/3-tooling/emitter/src/generate-contract-dts.tspackages/1-framework/3-tooling/emitter/test/domain-type-generation.test.tspackages/1-framework/3-tooling/emitter/test/emitter.integration.test.tspackages/1-framework/3-tooling/emitter/test/emitter.roundtrip.test.tspackages/1-framework/3-tooling/emitter/test/emitter.test.tspackages/2-mongo-family/1-foundation/mongo-contract/src/contract-schema.tspackages/2-mongo-family/1-foundation/mongo-contract/src/contract-types.tspackages/2-mongo-family/1-foundation/mongo-contract/test/contract-types.test-d.tspackages/2-mongo-family/1-foundation/mongo-contract/test/fixtures/orm-contract.d.tspackages/2-mongo-family/1-foundation/mongo-contract/test/fixtures/orm-contract.jsonpackages/2-mongo-family/1-foundation/mongo-contract/test/validate-domain.test.tspackages/2-mongo-family/1-foundation/mongo-contract/test/validate-storage.test.tspackages/2-mongo-family/1-foundation/mongo-contract/test/validate.test.tspackages/2-mongo-family/2-authoring/contract-psl/src/interpreter.tspackages/2-mongo-family/2-authoring/contract-psl/test/interpreter.test.tspackages/2-mongo-family/3-tooling/emitter/src/index.tspackages/2-mongo-family/3-tooling/emitter/test/emitter-hook.e2e.test.tspackages/2-mongo-family/3-tooling/emitter/test/emitter-hook.generation.test.tspackages/2-mongo-family/3-tooling/emitter/test/emitter-hook.structure.test.tspackages/2-mongo-family/3-tooling/emitter/test/emitter-hook.types.test.tspackages/2-mongo-family/3-tooling/emitter/test/fixtures/blog-contract.tspackages/2-mongo-family/5-query-builders/test/value-object-inputs.test-d.tspackages/2-mongo-family/7-runtime/test/fixtures/contract.d.tspackages/2-sql/1-core/contract/src/factories.tspackages/2-sql/1-core/contract/src/validate.tspackages/2-sql/1-core/contract/src/validators.tspackages/2-sql/1-core/contract/test/domain-types.test.tspackages/2-sql/1-core/contract/test/factories.test.tspackages/2-sql/1-core/contract/test/validate.test.tspackages/2-sql/2-authoring/contract-psl/src/interpreter.tspackages/2-sql/2-authoring/contract-psl/src/psl-field-resolution.tspackages/2-sql/2-authoring/contract-psl/test/interpreter.diagnostics.test.tspackages/2-sql/2-authoring/contract-psl/test/interpreter.test.tspackages/2-sql/2-authoring/contract-psl/test/provider.test.tspackages/2-sql/2-authoring/contract-ts/src/build-contract.tspackages/2-sql/2-authoring/contract-ts/src/semantic-contract.tspackages/2-sql/2-authoring/contract-ts/test/contract-builder.value-objects.test.tspackages/2-sql/2-authoring/contract-ts/test/contract.edge-cases.test.tspackages/2-sql/2-authoring/contract-ts/test/contract.logic.test.tspackages/2-sql/2-authoring/contract-ts/test/contract.model-validation.test.tspackages/2-sql/2-authoring/contract-ts/test/contract.normalization.test.tspackages/2-sql/2-authoring/contract-ts/test/fixtures/contract-with-relations.d.tspackages/2-sql/2-authoring/contract-ts/test/fixtures/contract.d.tspackages/2-sql/3-tooling/emitter/src/index.tspackages/2-sql/3-tooling/emitter/test/emitter-hook.generation.advanced.test.tspackages/2-sql/3-tooling/emitter/test/emitter-hook.generation.basic.test.tspackages/2-sql/3-tooling/emitter/test/emitter-hook.parameterized-types.test.tspackages/2-sql/4-lanes/relational-core/src/types.tspackages/2-sql/4-lanes/relational-core/test/fixtures/contract.d.tspackages/2-sql/4-lanes/relational-core/test/fixtures/contract.jsonpackages/2-sql/4-lanes/relational-core/test/schema.types.test-d.tspackages/2-sql/9-family/test/emit-parameterized.test.tspackages/3-extensions/sql-orm-client/src/collection-contract.tspackages/3-extensions/sql-orm-client/src/filters.tspackages/3-extensions/sql-orm-client/src/model-accessor.tspackages/3-extensions/sql-orm-client/test/create-input.test-d.tspackages/3-extensions/sql-orm-client/test/generated-contract-types.test-d.tspackages/3-extensions/sql-orm-client/test/include-cardinality.test-d.tspackages/3-extensions/sql-orm-client/test/integration/create.test.tspackages/3-extensions/sql-orm-client/test/integration/delete.test.tspackages/3-extensions/sql-orm-client/test/integration/first.test.tspackages/3-extensions/sql-orm-client/test/integration/include.test.tspackages/3-extensions/sql-orm-client/test/integration/nested-mutations.test.tspackages/3-extensions/sql-orm-client/test/integration/orm.test.tspackages/3-extensions/sql-orm-client/test/integration/runtime-helpers.tspackages/3-extensions/sql-orm-client/test/integration/self-relations.test.tspackages/3-extensions/sql-orm-client/test/integration/update.test.tspackages/3-extensions/sql-orm-client/test/integration/upsert.test.tspackages/3-extensions/sql-orm-client/test/query-plan-mutations.test.tspackages/3-extensions/sql-orm-client/test/value-object-inputs.test-d.tspackages/3-extensions/sql-orm-client/vitest.config.tstest/integration/package.jsontest/integration/test/authoring/diagnostics/unsupported-list-field/expected-diagnostics.jsontest/integration/test/authoring/diagnostics/unsupported-list-field/schema.prismatest/integration/test/authoring/parity/core-surface/expected.contract.jsontest/integration/test/authoring/parity/default-cuid-2/expected.contract.jsontest/integration/test/authoring/parity/default-dbgenerated/expected.contract.jsontest/integration/test/authoring/parity/default-nanoid-16/expected.contract.jsontest/integration/test/authoring/parity/default-nanoid/expected.contract.jsontest/integration/test/authoring/parity/default-pack-slugid/expected.contract.jsontest/integration/test/authoring/parity/default-ulid/expected.contract.jsontest/integration/test/authoring/parity/default-uuid-v4/expected.contract.jsontest/integration/test/authoring/parity/default-uuid-v7/expected.contract.jsontest/integration/test/authoring/parity/map-attributes/expected.contract.jsontest/integration/test/authoring/parity/pgvector-named-type/expected.contract.jsontest/integration/test/authoring/parity/relation-backrelation-list/expected.contract.jsontest/integration/test/cli.emit-cli-process.e2e.test.tstest/integration/test/cli.emit-command.test.tstest/integration/test/contract-imports.test.tstest/integration/test/fixtures/contract.d.tstest/integration/test/fixtures/contract.jsontest/integration/test/mongo/fixtures/contract.tstest/integration/test/value-objects/value-objects.e2e.test.tstest/integration/test/value-objects/value-objects.integration.test.ts
💤 Files with no reviewable changes (2)
- test/integration/test/authoring/diagnostics/unsupported-list-field/schema.prisma
- test/integration/test/authoring/diagnostics/unsupported-list-field/expected-diagnostics.json
✅ Files skipped from review due to trivial changes (44)
- test/integration/test/authoring/parity/core-surface/expected.contract.json
- test/integration/test/authoring/parity/relation-backrelation-list/expected.contract.json
- packages/3-extensions/sql-orm-client/test/integration/update.test.ts
- packages/1-framework/2-authoring/psl-parser/src/exports/types.ts
- packages/3-extensions/sql-orm-client/test/integration/runtime-helpers.ts
- packages/1-framework/2-authoring/psl-parser/src/exports/index.ts
- packages/2-sql/1-core/contract/test/factories.test.ts
- packages/2-sql/2-authoring/contract-psl/test/provider.test.ts
- test/integration/package.json
- packages/3-extensions/sql-orm-client/test/integration/delete.test.ts
- packages/3-extensions/sql-orm-client/test/integration/self-relations.test.ts
- packages/1-framework/0-foundation/contract/test/contract-types.test-d.ts
- packages/3-extensions/sql-orm-client/test/create-input.test-d.ts
- test/integration/test/authoring/parity/default-dbgenerated/expected.contract.json
- packages/3-extensions/sql-orm-client/test/integration/orm.test.ts
- test/integration/test/fixtures/contract.json
- packages/2-sql/3-tooling/emitter/test/emitter-hook.generation.advanced.test.ts
- packages/2-sql/4-lanes/relational-core/test/fixtures/contract.json
- packages/1-framework/0-foundation/contract/src/exports/types.ts
- test/integration/test/authoring/parity/default-uuid-v4/expected.contract.json
- examples/prisma-next-demo/src/prisma/contract.json
- packages/2-sql/1-core/contract/src/validate.ts
- test/integration/test/authoring/parity/map-attributes/expected.contract.json
- test/integration/test/authoring/parity/default-ulid/expected.contract.json
- packages/3-extensions/sql-orm-client/test/integration/nested-mutations.test.ts
- packages/2-mongo-family/1-foundation/mongo-contract/test/fixtures/orm-contract.d.ts
- packages/2-mongo-family/3-tooling/emitter/test/emitter-hook.types.test.ts
- test/integration/test/authoring/parity/default-uuid-v7/expected.contract.json
- test/integration/test/authoring/parity/default-nanoid-16/expected.contract.json
- packages/3-extensions/sql-orm-client/test/integration/upsert.test.ts
- packages/2-mongo-family/1-foundation/mongo-contract/test/fixtures/orm-contract.json
- packages/2-mongo-family/1-foundation/mongo-contract/test/validate-domain.test.ts
- packages/1-framework/3-tooling/emitter/test/emitter.test.ts
- packages/3-extensions/sql-orm-client/test/value-object-inputs.test-d.ts
- packages/1-framework/3-tooling/emitter/test/emitter.integration.test.ts
- packages/2-mongo-family/7-runtime/test/fixtures/contract.d.ts
- packages/2-mongo-family/1-foundation/mongo-contract/test/contract-types.test-d.ts
- packages/2-mongo-family/3-tooling/emitter/test/fixtures/blog-contract.ts
- packages/2-sql/2-authoring/contract-ts/test/contract-builder.value-objects.test.ts
- test/integration/test/authoring/parity/default-nanoid/expected.contract.json
- test/integration/test/fixtures/contract.d.ts
- packages/2-sql/1-core/contract/test/validate.test.ts
- packages/2-sql/2-authoring/contract-psl/src/interpreter.ts
- packages/3-extensions/sql-orm-client/test/integration/first.test.ts
🚧 Files skipped from review as they are similar to previous changes (46)
- packages/3-extensions/sql-orm-client/src/model-accessor.ts
- packages/1-framework/0-foundation/contract/src/contract-types.ts
- packages/3-extensions/sql-orm-client/test/query-plan-mutations.test.ts
- packages/2-mongo-family/3-tooling/emitter/src/index.ts
- packages/2-sql/2-authoring/contract-ts/test/contract.normalization.test.ts
- packages/2-mongo-family/3-tooling/emitter/test/emitter-hook.structure.test.ts
- packages/1-framework/0-foundation/contract/src/canonicalization.ts
- packages/1-framework/0-foundation/contract/test/validate-contract.test.ts
- packages/2-sql/2-authoring/contract-ts/test/contract.edge-cases.test.ts
- packages/2-sql/4-lanes/relational-core/test/schema.types.test-d.ts
- packages/2-mongo-family/2-authoring/contract-psl/src/interpreter.ts
- packages/2-sql/2-authoring/contract-ts/test/contract.logic.test.ts
- packages/2-mongo-family/3-tooling/emitter/test/emitter-hook.e2e.test.ts
- packages/1-framework/0-foundation/contract/src/validate-contract.ts
- packages/2-mongo-family/1-foundation/mongo-contract/test/validate.test.ts
- packages/2-sql/2-authoring/contract-psl/test/interpreter.diagnostics.test.ts
- packages/1-framework/2-authoring/psl-parser/src/types.ts
- packages/2-mongo-family/3-tooling/emitter/test/emitter-hook.generation.test.ts
- test/integration/test/contract-imports.test.ts
- test/integration/test/cli.emit-command.test.ts
- packages/2-sql/4-lanes/relational-core/src/types.ts
- packages/1-framework/0-foundation/contract/src/testing-factories.ts
- packages/3-extensions/sql-orm-client/test/integration/include.test.ts
- packages/1-framework/0-foundation/contract/test/canonicalization.test.ts
- packages/2-sql/4-lanes/relational-core/test/fixtures/contract.d.ts
- test/integration/test/mongo/fixtures/contract.ts
- packages/2-sql/2-authoring/contract-ts/test/fixtures/contract-with-relations.d.ts
- packages/2-mongo-family/1-foundation/mongo-contract/src/contract-schema.ts
- packages/1-framework/2-authoring/psl-parser/test/parser.test.ts
- packages/1-framework/3-tooling/emitter/test/emitter.roundtrip.test.ts
- packages/3-extensions/sql-orm-client/vitest.config.ts
- packages/2-sql/1-core/contract/src/factories.ts
- packages/2-sql/2-authoring/contract-ts/test/fixtures/contract.d.ts
- packages/2-mongo-family/2-authoring/contract-psl/test/interpreter.test.ts
- packages/2-mongo-family/5-query-builders/test/value-object-inputs.test-d.ts
- packages/1-framework/0-foundation/contract/src/domain-types.ts
- packages/2-sql/1-core/contract/test/domain-types.test.ts
- packages/2-sql/2-authoring/contract-psl/src/psl-field-resolution.ts
- packages/3-extensions/sql-orm-client/test/generated-contract-types.test-d.ts
- packages/2-sql/2-authoring/contract-ts/src/semantic-contract.ts
- packages/3-extensions/sql-orm-client/src/filters.ts
- packages/2-sql/2-authoring/contract-ts/src/build-contract.ts
- packages/1-framework/0-foundation/contract/test/validate-domain.test.ts
- packages/3-extensions/sql-orm-client/src/collection-contract.ts
- packages/2-sql/2-authoring/contract-ts/test/contract.model-validation.test.ts
- packages/1-framework/0-foundation/contract/test/domain-types.test.ts
Contributor
Code Review — PR #309: Value Objects & Embedded DocumentsReviewed by: 8 parallel analysis agents (architecture, security, performance, patterns, data integrity, simplicity, spec validation, git history) Overall AssessmentThe PR is architecturally excellent. The tagged discriminated union for Two P1 items should be fixed before merge. The P2 items are a mix of defensive gaps and cleanup — none are architecturally concerning. Findings Summary
🔴 P1 — CriticalP1-1:
|
| # | Finding |
|---|---|
| P3-1 | UnionFieldType is defined/exported/rendered but never constructed — speculative (ADR 179) |
| P3-2 | Duplicated field-iteration in validate-domain.ts — could share a forEachField helper |
| P3-3 | Conditional spread Object.keys(vo).length > 0 ? ... could use existing ifDefined pattern |
| P3-4 | SQL ModelFieldSchema marks type? as optional — fields with no type pass validation |
| P3-5 | dict modifier has no PSL authoring surface and no end-to-end test |
| P3-6 | 14 of 34 commits (~41%) are out of scope (pipeline stages, doc housekeeping, style fix) |
Spec/Design Doc Validation
wmadden's docs are accurate on:
- Field descriptor shape (discriminated union) ✅
valueObjectson Contract (optional, not a generic param) ✅- Storage mapping (SQL→JSONB, Mongo→native subdocuments) ✅
ContractFieldTypenaming avoids collision with existingFieldType✅- TML-2204 independence confirmed in code ✅
- Canonicalization ordering ✅
Implementation gaps vs plan:
| Plan Item | Status |
|---|---|
| M1–M5 (contract types, validation, authoring, emission) | ✅ Complete |
| M6.1/6.3 (ORM row inference, both families) | ✅ Complete |
| M6.2 (Mongo dot-path filtering) | ❌ Not implemented |
| M6.4 (SQL dot-path JSONB filtering) | ❌ Not implemented |
M6.5 (.select() support) |
❓ Not verified |
| M7 (ORM writes) | ✅ Types work naturally |
| M8 (Integration + e2e tests) | ✅ Complete |
Stale references:
Most line numbers in the execution plan are off by 5–40 lines due to the type system expansion. Expected post-implementation drift — consider updating if the plan remains a living document.
Recommended Merge Path
- Fix P1-1 and P1-2 — required before merge
- Fix P2-1 through P2-6 — strongly recommended before merge
- P2-7 through P2-10 and all P3 — acceptable as fast-follows
- Acknowledge M6.2/M6.4/M6.5 gaps — confirm these are deferred to follow-up work
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
packages/2-sql/3-tooling/emitter/test/emitter-hook.generation.basic.test.ts (1)
1-1251: Consider splitting this test file.The file has grown to ~1250 lines, exceeding the 500-line guideline. Consider splitting into:
emitter-hook.generation.basic.test.ts- core contract type generationemitter-hook.generation.storage.test.ts- storage-related tests (uniques, indexes, foreignKeys)emitter-hook.generation.value-objects.test.ts- value object type generation testsThis would improve maintainability and make test files easier to navigate. As per coding guidelines: "Keep test files under 500 lines to maintain readability and navigability."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/3-tooling/emitter/test/emitter-hook.generation.basic.test.ts` around lines 1 - 1251, The test file emitter-hook.generation.basic.test.ts is too large (~1250 lines); split tests into three focused files: emitter-hook.generation.basic.test.ts (keep core contract type generation tests such as the top-level describe 'sql-target-family-hook' and related it blocks that assert Contract/TypeMaps/imports and execution behavior), emitter-hook.generation.storage.test.ts (move all storage-related tests that reference uniques/indexes/foreignKeys/index config and extension index config), and emitter-hook.generation.value-objects.test.ts (move all value object tests that assert export type aliases, valueObjects on ContractBase, self-references, nullable/many handling). Ensure shared helpers (createContract, testHashes) and imports (generateContractDts, sqlEmission, extractCodecTypeImports, extractOperationTypeImports, TypeRenderEntry) are either duplicated or factored into a new test helper module and imported by the three files so each file remains <500 lines and all tests keep the same test names and assertions.packages/2-mongo-family/3-tooling/emitter/src/index.ts (1)
14-30: Consider importing the actual field type definitions for better type safety.The inline type assertion works correctly for the new discriminated field type structure, but importing the actual types from
@prisma-next/contract/typeswould improve maintainability and catch future type drift at compile time.♻️ Suggested improvement using imported types
+import type { Contract, ContractModel, ContractFieldType, ScalarFieldType } from '@prisma-next/contract/types'; -import type { Contract, ContractModel } from '@prisma-next/contract/types';Then use the imported types:
- const fieldType = ( - field as { - type?: { - kind: string; - codecId?: string; - members?: ReadonlyArray<{ kind: string; codecId?: string }>; - }; - } - ).type; + const fieldType = (field as { type?: ContractFieldType }).type; if (!fieldType) continue; - const scalarTypes: Array<{ codecId?: string }> = + const scalarTypes: ScalarFieldType[] = fieldType.kind === 'scalar' ? [fieldType] : fieldType.kind === 'union' && fieldType.members - ? fieldType.members.filter((m) => m.kind === 'scalar') + ? (fieldType.members.filter((m): m is ScalarFieldType => m.kind === 'scalar')) : [];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-mongo-family/3-tooling/emitter/src/index.ts` around lines 14 - 30, The code currently uses an inline type assertion for the field variable to access field.type and discriminate on kind (see field, fieldType and scalarTypes in the block); replace the ad-hoc inline type with the actual discriminated type imports from `@prisma-next/contract/types` (import the Field and FieldType/Scalar/Union/member types) and update the annotation on field (or the local fieldType variable) to use those imported types so TypeScript enforces the discriminated union when computing scalarTypes; ensure the imported types cover the kind/codecId/members shape and then remove the inline assertion and keep the same runtime logic that checks fieldType and builds scalarTypes.packages/2-sql/1-core/contract/test/validate.test.ts (1)
1-7: Consider splitting this test file to improve maintainability.This test file is 854 lines, exceeding the 500-line guideline. Natural split boundaries exist:
validate.test.ts→ core validation testsvalidate.storage-semantics.test.ts→ storage semantic validationvalidate.codec-defaults.test.ts→ codec default decodingvalidate.cross-validation.test.ts→ model-to-storage cross-validation (including the new value object tests)This is optional and can be deferred. As per coding guidelines: "Keep test files under 500 lines to maintain readability and navigability."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/1-core/contract/test/validate.test.ts` around lines 1 - 7, The test file validate.test.ts is over the 500-line guideline; split it into multiple smaller test files (suggested: validate.core.test.ts for core validation tests, validate.storage-semantics.test.ts for storage semantic validation, validate.codec-defaults.test.ts for codec default decoding, and validate.cross-validation.test.ts for model-to-storage cross-validation including value-object tests), move the related describe/it blocks into the appropriate new files, keep and reuse shared imports (Contract, ContractValidationError, emptyCodecLookup, SqlStorage, validateContract) at the top of each new file, and update any relative import paths so each new test file imports validateContract and the types it needs; ensure test runner config picks up the new filenames and run tests to verify no import breakages.test/integration/test/value-objects/value-objects.integration.test.ts (1)
132-135: Consider static import forMongoFieldFilter.The dynamic import is unconditionally executed within this test. A static import at the top of the file would be cleaner and avoid the async overhead.
♻️ Suggested change
Add to imports at top of file:
import { MongoFieldFilter } from '@prisma-next/mongo-query-ast';Then simplify the test:
- const { MongoFieldFilter } = await import('@prisma-next/mongo-query-ast'); const updated = await userCollection .where(MongoFieldFilter.eq('name', 'Bob')) .update({ homeAddress: { street: '789 Pine Rd', city: 'Capital City', zip: '99999' } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/integration/test/value-objects/value-objects.integration.test.ts` around lines 132 - 135, Replace the dynamic import of MongoFieldFilter with a static top-level import: add "import { MongoFieldFilter } from '@prisma-next/mongo-query-ast';" to the file header and remove the runtime await import expression in the test; update the test's usage to call MongoFieldFilter.eq(...) directly (the call sites using userCollection.where(...).update(...) remain the same).
🤖 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/1-foundation/mongo-contract/src/contract-types.ts`:
- Around line 83-93: InferFieldType currently only accounts for many and
nullable modifiers and misses dict, so dictionary fields resolve to a single
value instead of Record<string, BaseType>. Update the conditional branching in
the InferFieldType type to handle TField extends { dict: true } (placed
alongside the many check) and return Record<string, InferFieldBaseType<...>> or
Record<string, InferFieldBaseType<...>> | null when nullable is true; keep
existing behavior for many and non-modified fields and continue to use
InferFieldBaseType<TField['type'], TValueObjects, TCodecTypes> as the base
element type.
In `@packages/2-sql/2-authoring/contract-ts/src/semantic-contract.ts`:
- Around line 68-74: The SqlSemanticValueObjectFieldNode interface is missing
default and executionDefault properties so defaults on JSONB-backed value-object
fields are lost; add readonly default?: ExpressionType and readonly
executionDefault?: ExpressionType (matching types used on SqlSemanticFieldNode)
to SqlSemanticValueObjectFieldNode, then update build-contract.ts in the
isValueObjectField() branch to thread those two properties into the JSONB column
state construction (where the JSONB column for a value-object field is built) so
build-contract sees and preserves defaults for value-object fields.
---
Nitpick comments:
In `@packages/2-mongo-family/3-tooling/emitter/src/index.ts`:
- Around line 14-30: The code currently uses an inline type assertion for the
field variable to access field.type and discriminate on kind (see field,
fieldType and scalarTypes in the block); replace the ad-hoc inline type with the
actual discriminated type imports from `@prisma-next/contract/types` (import the
Field and FieldType/Scalar/Union/member types) and update the annotation on
field (or the local fieldType variable) to use those imported types so
TypeScript enforces the discriminated union when computing scalarTypes; ensure
the imported types cover the kind/codecId/members shape and then remove the
inline assertion and keep the same runtime logic that checks fieldType and
builds scalarTypes.
In `@packages/2-sql/1-core/contract/test/validate.test.ts`:
- Around line 1-7: The test file validate.test.ts is over the 500-line
guideline; split it into multiple smaller test files (suggested:
validate.core.test.ts for core validation tests,
validate.storage-semantics.test.ts for storage semantic validation,
validate.codec-defaults.test.ts for codec default decoding, and
validate.cross-validation.test.ts for model-to-storage cross-validation
including value-object tests), move the related describe/it blocks into the
appropriate new files, keep and reuse shared imports (Contract,
ContractValidationError, emptyCodecLookup, SqlStorage, validateContract) at the
top of each new file, and update any relative import paths so each new test file
imports validateContract and the types it needs; ensure test runner config picks
up the new filenames and run tests to verify no import breakages.
In `@packages/2-sql/3-tooling/emitter/test/emitter-hook.generation.basic.test.ts`:
- Around line 1-1251: The test file emitter-hook.generation.basic.test.ts is too
large (~1250 lines); split tests into three focused files:
emitter-hook.generation.basic.test.ts (keep core contract type generation tests
such as the top-level describe 'sql-target-family-hook' and related it blocks
that assert Contract/TypeMaps/imports and execution behavior),
emitter-hook.generation.storage.test.ts (move all storage-related tests that
reference uniques/indexes/foreignKeys/index config and extension index config),
and emitter-hook.generation.value-objects.test.ts (move all value object tests
that assert export type aliases, valueObjects on ContractBase, self-references,
nullable/many handling). Ensure shared helpers (createContract, testHashes) and
imports (generateContractDts, sqlEmission, extractCodecTypeImports,
extractOperationTypeImports, TypeRenderEntry) are either duplicated or factored
into a new test helper module and imported by the three files so each file
remains <500 lines and all tests keep the same test names and assertions.
In `@test/integration/test/value-objects/value-objects.integration.test.ts`:
- Around line 132-135: Replace the dynamic import of MongoFieldFilter with a
static top-level import: add "import { MongoFieldFilter } from
'@prisma-next/mongo-query-ast';" to the file header and remove the runtime await
import expression in the test; update the test's usage to call
MongoFieldFilter.eq(...) directly (the call sites using
userCollection.where(...).update(...) remain the same).
🪄 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: 75633748-45f7-4871-8698-3d0c58c2ae50
⛔ Files ignored due to path filters (1)
test/e2e/framework/test/fixtures/generated/contract.d.tsis excluded by!**/generated/**
📒 Files selected for processing (19)
packages/1-framework/0-foundation/contract/src/testing-factories.tspackages/1-framework/0-foundation/contract/src/validate-domain.tspackages/1-framework/0-foundation/contract/test/validate-domain.test.tspackages/1-framework/3-tooling/emitter/src/generate-contract-dts.tspackages/2-mongo-family/1-foundation/mongo-contract/src/contract-schema.tspackages/2-mongo-family/1-foundation/mongo-contract/src/contract-types.tspackages/2-mongo-family/3-tooling/emitter/src/index.tspackages/2-mongo-family/3-tooling/emitter/test/emitter-hook.generation.test.tspackages/2-sql/1-core/contract/src/factories.tspackages/2-sql/1-core/contract/src/validators.tspackages/2-sql/1-core/contract/test/contract-construction.test.tspackages/2-sql/1-core/contract/test/factories.test.tspackages/2-sql/1-core/contract/test/validate.test.tspackages/2-sql/1-core/contract/test/validators.test.tspackages/2-sql/2-authoring/contract-psl/src/psl-field-resolution.tspackages/2-sql/2-authoring/contract-ts/src/build-contract.tspackages/2-sql/2-authoring/contract-ts/src/semantic-contract.tspackages/2-sql/3-tooling/emitter/test/emitter-hook.generation.basic.test.tstest/integration/test/value-objects/value-objects.integration.test.ts
✅ Files skipped from review due to trivial changes (3)
- packages/2-sql/1-core/contract/test/validators.test.ts
- packages/2-sql/1-core/contract/test/contract-construction.test.ts
- packages/2-sql/1-core/contract/test/factories.test.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/2-sql/1-core/contract/src/factories.ts
- packages/2-mongo-family/1-foundation/mongo-contract/src/contract-schema.ts
- packages/1-framework/3-tooling/emitter/src/generate-contract-dts.ts
- packages/1-framework/0-foundation/contract/src/validate-domain.ts
- packages/2-sql/2-authoring/contract-ts/src/build-contract.ts
- packages/2-mongo-family/3-tooling/emitter/test/emitter-hook.generation.test.ts
wmadden
force-pushed
the
tml-2206-value-objects-embedded-documents-in-both-families-adr-178
branch
from
fc8c83f to
cb6f02a
Compare
….75c dependencies Phase 1.75c (value objects) is independent of Phase 1.75a (typed JSON simplification) — value object types are derived from contract field descriptors, not codec-dispatched renderType. Phase 1.5 (TML-2194) has landed. Design doc captures decisions on field descriptor shape (discriminated union), valueObjects contract placement, PSL syntax, and sequencing.
…ing example Restructure to lead with a concrete end-to-end example (PSL schema, contract JSON, TypeScript types, ORM usage) so design decisions reference something tangible. Incorporate typeParams on scalar fields (from TML-2215).
Add Phase 1.75c execution plan with 8 milestones covering contract field type system, validation, authoring, emission, ORM reads/writes, and integration tests. Refine field descriptor to use a structurally exclusive tagged type specifier (kind: scalar/valueObject/union) inside a single type property, preventing conflicting keys at the data level.
…ted union
ContractField changes from { nullable, codecId, typeParams? } to
{ nullable, type: { kind, codecId, typeParams? }, many?, dict? }.
This is the foundation for value objects (TML-2206). The new shape
uses a tagged ContractFieldType discriminant (scalar | valueObject | union)
so the field descriptor itself prevents conflicting type specifiers.
All existing consumers updated mechanically: field.codecId becomes
field.type.codecId after narrowing on field.type.kind === "scalar".
Test fixtures, generated contract.d.ts files, and arktype schemas
updated to match.
Introduce ContractValueObject type (fields without identity/lifecycle) and optional valueObjects record on Contract. Update canonicalization to place valueObjects between models and storage. Update structural schema, domain validation shape, testing factories, and type exports.
Domain validation now checks: - value object field references resolve to valueObjects keys - self-references within value objects are allowed - many + dict modifiers cannot coexist on the same field SQL storage validation checks that value object fields map to json/jsonb native type columns.
Parse "type X { ... }" blocks as PslCompositeType nodes with fields
and attributes. Composite types reuse parseField logic from model
blocks. Add compositeTypes array to PslDocumentAst.
This is the parser foundation for value objects (TML-2206).
Both Mongo and SQL interpreters now:
- Process compositeTypes from the PSL AST into contract valueObjects
- Resolve value object field references to { kind: "valueObject", name }
- Support scalar array fields (String[]) with many: true
- SQL interpreter maps value object model fields to JSONB storage columns
Support defining value objects and referencing them from model fields
in the TypeScript contract builder. Value object field references
produce { kind: "valueObject", name } domain fields and automatic
JSONB storage columns.
Add shared helpers for generating TypeScript types from value objects. Both SQL and Mongo emitters now emit named value object type aliases and include valueObjects descriptors in ContractBase. Model fields referencing value objects use the named type references.
ResolveModelFieldToJsType now handles kind: "valueObject" fields by looking up the value object definition from contract.valueObjects and recursively expanding its fields to TypeScript types. Also extracts ApplyFieldModifiers to handle nullable/many/dict uniformly for both scalar and value object fields, and updates the ModelEntry type in collection-contract.ts to match the current ContractField shape (type.kind discriminant).
Mongo InferModelRow and SQL DefaultModelRow now recursively expand value object field references into nested TypeScript object types. Nullable and many modifiers are handled (T | null, T[]). SQL value object fields stored as JSONB are transparently decoded.
…amilies Add type-level and fixture tests verifying that both Mongo and SQL ORM collections accept inline value object structures in create and update inputs. Update the SQL test fixture contract to include an Address value object on the User model, matching the already-updated contract.json.
…jection test Ensures the test contract has a proper valueObjects section that matches the Address value object referenced in the model field.
… VO refs in SQL interpreter The Mongo contract arktype schema rejected valueObject field types and the top-level valueObjects property. Add ValueObjectFieldTypeSchema to the union and allow valueObjects in MongoContractSchema. The SQL PSL interpreter did not resolve composite-type references within value object definitions (e.g. ShippingInfo.address referencing Address). Add compositeTypeNames lookup in buildValueObjects so nested VO fields emit kind: valueObject instead of failing with PSL_UNSUPPORTED_FIELD_TYPE. Also fix sql-contract validation test that was missing valueObjects, causing domain validation to reject before storage validation could run.
…-family Add integration tests covering the full value objects pipeline: - Mongo: PSL → interpret → validate → ORM create/read/update against mongodb-memory-server, including nullable value objects - SQL: PSL → interpret → validate, verifying valueObjects definitions, JSONB storage columns, and field descriptors - Cross-family: same PSL schema through both interpreters produces matching valueObjects definitions and field descriptors, including nested value object references
Proves the full pipeline works end-to-end with correct types:
- Mongo: pre-emitted contract → validate → ORM create/read/update → expectTypeOf verifies value object fields resolve to { street, city, zip }
- SQL: pre-emitted contract → validate → Postgres JSONB table → ORM create/all → expectTypeOf verifies value object fields expand correctly through the ORM type system
Hand-crafted contract.d.ts and contract.json fixtures for the Mongo and SQL value objects e2e tests. These need force-adding because the gitignore pattern excludes test/**/fixtures/generated/.
…fter rebase Resolve post-rebase issues from EmissionSpi refactoring on main: - Update all contract fixtures (JSON + d.ts) to discriminated union field format - Fix emitter test references (mongoTargetFamilyHook -> mongoEmission) - Add PslCompositeType to psl-parser main export - Fix exactOptionalPropertyTypes issue in validate-contract - Remove obsolete unsupported-list-field diagnostic fixture - Add typeParams to default-id parity fixtures
- Add union field type to Mongo FieldTypeSchema (was only scalar/valueObject) - Validate valueObjects with proper Arktype schema instead of Record<string, unknown> - Make nullable and type required in SQL ModelFieldSchema - Reject @pgvector.column on JSON-backed (list/value-object) fields - Preserve scalar list many metadata in contract-ts build-contract - Validate scalar members inside union fields in Mongo emitter - Make valueObjects optional in emitted contract.d.ts when absent - Use ifDefined() in testing-factories.ts and psl-field-resolution.ts - Extract shared field iteration in validate-domain.ts
- Validate value object references inside union field members - Change ExtractValueObjects fallback to Record<never, never> - Add union branch to InferFieldBaseType for correct type inference - Assert canonical SQL storage naming in integration test
- Add Address value object and address field to sql-orm-client test fixture contract so value-object type tests resolve correctly - Fix column order in insert returning assertion to match alphabetical - Fix biome formatting in domain-type-generation.ts import
…ntractField The new pipeline-builder package from upstream still used the old flat field format (top-level codecId). Update ModelToDocShape to extract codecId from the nested type discriminant, update test fixture contracts, and fix biome noBannedTypes warnings.
wmadden
force-pushed
the
tml-2206-value-objects-embedded-documents-in-both-families-adr-178
branch
from
cb6f02a to
17ed252
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/1-framework/2-authoring/psl-parser/src/parser.ts (1)
143-148:⚠️ Potential issue | 🟡 MinorMissing conflict detection for composite type names.
The parser validates that named types (in
types { }blocks) don't conflict with model or enum names (lines 158-172), but composite type names (type X { }blocks) aren't checked for conflicts with models, enums, or named types. This could allow a schema with bothmodel User { }andtype User { }, leading to ambiguous references.🛡️ Suggested validation addition
const compositeTypeNames = new Set(compositeTypes.map((ct) => ct.name)); + for (const ct of compositeTypes) { + if (modelNames.has(ct.name)) { + pushDiagnostic(context, { + code: 'PSL_INVALID_COMPOSITE_TYPE', + message: `Composite type "${ct.name}" conflicts with model name "${ct.name}"`, + span: ct.span, + }); + } + if (enumNames.has(ct.name)) { + pushDiagnostic(context, { + code: 'PSL_INVALID_COMPOSITE_TYPE', + message: `Composite type "${ct.name}" conflicts with enum name "${ct.name}"`, + span: ct.span, + }); + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/1-framework/2-authoring/psl-parser/src/parser.ts` around lines 143 - 148, The parser currently builds namedTypeNames, modelNames, enumNames, and compositeTypeNames but only checks conflicts between named types, models, and enums; update the validation logic to also check compositeTypeNames for name collisions against modelNames, enumNames, and namedTypeNames (and vice versa) by computing set intersections (e.g., compositeTypeNames ∩ modelNames, compositeTypeNames ∩ enumNames, compositeTypeNames ∩ namedTypeNames) and report/throw a descriptive error for each conflict; modify the existing conflict-checking block that references namedTypeNames/modelNames/enumNames to include compositeTypeNames and ensure all duplicate names are collected and surfaced consistently (use the same error format/collector used by the parser).
♻️ Duplicate comments (2)
packages/2-mongo-family/1-foundation/mongo-contract/src/contract-types.ts (1)
83-93:⚠️ Potential issue | 🟠 MajorApply
dictwhen inferring Mongo field types.
InferFieldTypehandlesmany, butdict: truestill collapses to a single value. Mongo rows for dictionary fields should inferRecord<string, T>(and| nullwhen nullable).🛠️ Minimal fix
type InferFieldType< TField extends ContractField, TValueObjects extends Record<string, ContractValueObject>, TCodecTypes extends Record<string, { output: unknown }>, > = TField extends { many: true } ? TField['nullable'] extends true ? InferFieldBaseType<TField['type'], TValueObjects, TCodecTypes>[] | null : InferFieldBaseType<TField['type'], TValueObjects, TCodecTypes>[] + : TField extends { dict: true } + ? TField['nullable'] extends true + ? Record<string, InferFieldBaseType<TField['type'], TValueObjects, TCodecTypes>> | null + : Record<string, InferFieldBaseType<TField['type'], TValueObjects, TCodecTypes>> : TField['nullable'] extends true ? InferFieldBaseType<TField['type'], TValueObjects, TCodecTypes> | null : InferFieldBaseType<TField['type'], TValueObjects, TCodecTypes>;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-mongo-family/1-foundation/mongo-contract/src/contract-types.ts` around lines 83 - 93, InferFieldType currently ignores ContractField's dict flag and collapses dictionary fields to a single value; update InferFieldType to branch on TField['dict'] === true and return Record<string, InferFieldBaseType<...>> (or Record<string, InferFieldBaseType<...>>[] if many) and include | null when nullable. Specifically, modify the conditional arms in InferFieldType to check TField['dict'] before falling back to the scalar handling, using InferFieldBaseType<TField['type'], TValueObjects, TCodecTypes> as the value type so dict fields become Record<string, T> (or arrays/nullable variants) rather than a single T.packages/2-sql/3-tooling/emitter/src/index.ts (1)
335-370:⚠️ Potential issue | 🟠 MajorScalar
many/dictmodifiers are still dropped in emitted model fields.When
domainField.type.kind === 'scalar', this branch falls back to the column path and never reappliesmany/dict. Valid scalar list/map fields therefore still get emitted as singular scalars incontract.d.ts.🛠️ Minimal fix
- if (isNonScalarField(domainField)) { + if ( + domainField && + (domainField.type.kind !== 'scalar' || + domainField.many === true || + domainField.dict === true) + ) { fields.push(generateContractFieldDescriptor(fieldName, domainField)); continue; } ... - if (isNonScalarField(domainField)) { + if ( + domainField && + (domainField.type.kind !== 'scalar' || + domainField.many === true || + domainField.dict === true) + ) { fields.push(generateContractFieldDescriptor(fieldName, domainField)); continue; }Also applies to: 379-387
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/3-tooling/emitter/src/index.ts` around lines 335 - 370, The emitted contract field logic drops scalar modifiers (many/dict) because when domainField.type.kind === 'scalar' the code falls through to column-based emission and never reapplies domainField.type.modifier; update the branch that handles scalar domain fields (around isNonScalarField check and the else path using column, domainField, renderer, serializeTypeParamsLiteral) to detect domainField.type.modifier (e.g., 'many' or 'dict') and wrap the resolved renderedType or the constructed scalar descriptor accordingly (preserving nullable semantics), so that generateContractFieldDescriptor semantics for lists/maps are reapplied when emitting readonly ${fieldName} types; ensure both the renderer path and the fallback scalar descriptor path apply the same modifier transformation.
🧹 Nitpick comments (4)
packages/3-extensions/sql-orm-client/test/query-plan-mutations.test.ts (1)
69-81: Returning columns updated to include address.The
addresscolumn is correctly added to the expectedreturningprojection list. The columns appear to be sorted alphabetically, which is consistent with the contract's column ordering.Minor: Line 69 changed from
toEqualtotoMatchObject, which is less strict. Consider keepingtoEqualfor complete structure verification, especially in plan-level tests where the exact AST shape matters.♻️ Consider keeping strict equality
- expect(plan.ast.rows[1]).toMatchObject({ + expect(plan.ast.rows[1]).toEqual({ id: usersColParam(contract, 'id', 11), name: usersColParam(contract, 'name', 'Bob'), email: usersColParam(contract, 'email', 'bob@example.com'), invited_by_id: usersColParam(contract, 'invited_by_id', 10), + address: expect.objectContaining({ kind: 'default-value' }), });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/3-extensions/sql-orm-client/test/query-plan-mutations.test.ts` around lines 69 - 81, The assertion for plan.ast.rows[1] was weakened to toMatchObject; change it back to a strict equality check using toEqual so the test validates the full AST shape (replace expect(plan.ast.rows[1]).toMatchObject(...) with expect(plan.ast.rows[1]).toEqual(...)), keeping the usersColParam(contract, ...) calls as the expected values and leave the updated returning assertion (expect(plan.ast.returning).toEqual([... ColumnRef.of(...) ...])) intact; this ensures the test for the query plan mutations remains exact.packages/2-sql/1-core/contract/src/validators.ts (1)
210-210: Consider adding structural validation forvalueObjects.The SQL contract schema accepts
valueObjectsasRecord<string, unknown>, which allows malformed entries to pass structural validation. The Mongo schema (lines 105-107 incontract-schema.ts) validates value object structure with{ fields: Record<string, FieldSchema> }.This is noted as P2-4 in the PR objectives. If intentionally deferred, this is acceptable; otherwise consider aligning with the Mongo schema's stricter validation.
♻️ Suggested schema alignment
+ const ValueObjectSchema = type({ + '+': 'reject', + fields: type({ '[string]': ModelFieldSchema }), + }); + const SqlContractSchema = type({ // ... - 'valueObjects?': 'Record<string, unknown>', + 'valueObjects?': type({ '[string]': ValueObjectSchema }), });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/1-core/contract/src/validators.ts` at line 210, The schema entry 'valueObjects?' currently permits any Record<string, unknown>; change it to validate the expected shape (each value object must be an object with a 'fields' property mapping to Record<string, FieldSchema>) to match the Mongo contract-schema's structure (see the `{ fields: Record<string, FieldSchema> }` pattern) so malformed valueObjects are rejected; update the validator for 'valueObjects?' in validators.ts to require that each entry is an object with a 'fields' key whose value conforms to FieldSchema.packages/1-framework/3-tooling/emitter/test/domain-type-generation.test.ts (1)
437-676: Split the new value-object helper coverage into a companion test file.This file is now ~676 lines, and the added suites are a distinct concern from the existing serialization/import tests. Moving them into something like
domain-type-generation.value-objects.test.tswould keep failures easier to navigate.As per coding guidelines, "Keep test files under 500 lines to maintain readability and navigability. Split test files when they exceed 500 lines, contain multiple distinct concerns that can be logically separated, or have multiple top-level
describeblocks that can be split by functionality."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/1-framework/3-tooling/emitter/test/domain-type-generation.test.ts` around lines 437 - 676, The file is too large and the new value-object helper tests should be split into a companion test file: create a new test file (e.g. domain-type-generation.value-objects.test.ts) and move the describe suites that target value-object helpers—generateFieldResolvedType, generateValueObjectType, generateValueObjectsDescriptorType, and generateValueObjectTypeAliases—into that new file, leaving other suites (like generateContractFieldDescriptor) in the original file; ensure any shared imports or helper fixtures used by those suites are copied or imported into the new file and run the test runner to confirm names/exports still resolve.packages/2-sql/3-tooling/emitter/test/emitter-hook.generation.basic.test.ts (1)
1054-1250: Split the new value-object generation suite into its own test file.This file is already well past the 500-line cap, and the added value-object block is separable from the existing "basic" emitter cases. Moving it into
emitter-hook.generation.value-objects.test.ts(or similar) would make the suite much easier to navigate.As per coding guidelines, "Keep test files under 500 lines to maintain readability and navigability. Split test files when they exceed 500 lines, contain multiple distinct concerns that can be logically separated, or have multiple top-level
describeblocks that can be split by functionality."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/3-tooling/emitter/test/emitter-hook.generation.basic.test.ts` around lines 1054 - 1250, The added describe('value object type generation') suite should be extracted into its own test file: create a new test file named emitter-hook.generation.value-objects.test.ts containing that entire describe block and its it cases, copy over any required helpers/imports used there (createContract, generateContractDts, sqlEmission, testHashes, etc.), and remove the block from the original emitter-hook.generation.basic.test.ts so the original file drops below the 500-line cap; ensure the new file's imports/exports match the test runner conventions and run tests to confirm no missing references.
🤖 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/1-framework/3-tooling/emitter/src/domain-type-generation.ts`:
- Around line 62-76: The two functions generateModelFieldEntry and
generateContractFieldDescriptor duplicate logic for rendering field descriptors;
extract the shared rendering logic into a single helper (e.g.,
renderFieldDescriptor or buildFieldDescriptorString) that takes the fieldName,
ContractField (or its constituents), and contractFieldModifierSuffix result, and
returns the descriptor string handling scalar (including
typeParams/type.codecId), valueObject (including name), and the generic fallback
with nullable and mods; then have both generateModelFieldEntry and
generateContractFieldDescriptor call that helper so scalar/valueObject/union
rendering is implemented once and reused.
- Around line 224-225: The code is directly interpolating raw VO/model names
into type identifiers (e.g., in the case 'valueObject' where baseType =
type.name), which can produce invalid or malicious DTS (e.g., "Address-1" or
"Foo = never"); fix by validating/sanitizing identifiers before emission: add a
helper (e.g., sanitizeIdentifier or createSafeIdentifier) that verifies the name
against a legal TS identifier pattern (e.g., /^[A-Za-z_$][A-Za-z0-9_$]*$/) and
rejects or maps invalid names to a safe variant (or throws); replace direct uses
of type.name (notably the baseType assignment in the case 'valueObject' and the
other occurrences around the 315-317 area) with the sanitized identifier and
ensure reserved words are handled (e.g., prefix with _ or reject) so only safe
identifiers are emitted.
In `@packages/2-sql/2-authoring/contract-ts/src/build-contract.ts`:
- Around line 240-259: The scalar "many" branch currently emits only { kind:
'scalar', many: true } and drops the element type/codec info, causing array
fields to lose their element descriptor; update the branches that set
domainFields[...] when domainRef?.many (the block using domainRef, columnState,
and setting type.kind === 'scalar') to include the element codecId and
typeParams from columnState (i.e., preserve columnState.type and
columnState.typeParams inside the type object) while keeping many: true, and
apply the same change to the other identical patterns in this file where
domainRef?.many is used (the blocks that reference domainRef, columnState,
type.codecId, and typeParams).
- Around line 548-570: The valueObjects mapping in build-contract.ts incorrectly
flattens every field into a scalar (always emitting kind: 'scalar' and dropping
'many'), so update the mapper for valueObjects (the valueObjects const and its
inner field mapping) to branch on the field descriptor kind and emit either a
scalar descriptor (preserving codecId, typeParams and nullable) or a
value-object reference descriptor (preserving reference name and many/array
info) instead of forcing kind: 'scalar'; also ensure the many flag is propagated
for arrays. In addition, widen SqlSemanticValueObjectNode.fields in
semantic-contract.ts to use SqlSemanticValueObjectFieldNode so the semantic
model accepts the richer field descriptors emitted by build-contract.ts.
In
`@packages/2-sql/2-authoring/contract-ts/test/contract-builder.value-objects.test.ts`:
- Around line 33-51: Add a nested value-object test case to ensure nested
serialization is correct: update the test fixtures used by
contract-builder.value-objects.test.ts to include a parent entity (e.g.,
Company) with a field like address that references a value object (e.g.,
Address) instead of only scalar fields, so the valueObjects array contains both
the Address definition and a Company with a field mapping company.address ->
Address; run the test against buildSqlContractFromSemanticDefinition() to verify
the nested value-object name is serialized correctly and fails if nested
references are mishandled.
In `@packages/2-sql/4-lanes/relational-core/src/types.ts`:
- Around line 360-380: ResolveModelFieldToJsType currently falls through for FT
kinds it doesn't handle, causing union-typed fields to resolve to the raw
contract descriptor; add an explicit branch for FT extends { readonly kind:
'union'; readonly members: infer Members } inside ResolveModelFieldToJsType that
maps union members to their corresponding JS types (and then feeds the result
through ApplyFieldModifiers with Nullable), or if you don't want to implement
member resolution yet, return unknown for the union branch to avoid leaking the
descriptor; reference ResolveModelFieldToJsType, the FT kind 'union',
ApplyFieldModifiers and Nullable when making the change.
---
Outside diff comments:
In `@packages/1-framework/2-authoring/psl-parser/src/parser.ts`:
- Around line 143-148: The parser currently builds namedTypeNames, modelNames,
enumNames, and compositeTypeNames but only checks conflicts between named types,
models, and enums; update the validation logic to also check compositeTypeNames
for name collisions against modelNames, enumNames, and namedTypeNames (and vice
versa) by computing set intersections (e.g., compositeTypeNames ∩ modelNames,
compositeTypeNames ∩ enumNames, compositeTypeNames ∩ namedTypeNames) and
report/throw a descriptive error for each conflict; modify the existing
conflict-checking block that references namedTypeNames/modelNames/enumNames to
include compositeTypeNames and ensure all duplicate names are collected and
surfaced consistently (use the same error format/collector used by the parser).
---
Duplicate comments:
In `@packages/2-mongo-family/1-foundation/mongo-contract/src/contract-types.ts`:
- Around line 83-93: InferFieldType currently ignores ContractField's dict flag
and collapses dictionary fields to a single value; update InferFieldType to
branch on TField['dict'] === true and return Record<string,
InferFieldBaseType<...>> (or Record<string, InferFieldBaseType<...>>[] if many)
and include | null when nullable. Specifically, modify the conditional arms in
InferFieldType to check TField['dict'] before falling back to the scalar
handling, using InferFieldBaseType<TField['type'], TValueObjects, TCodecTypes>
as the value type so dict fields become Record<string, T> (or arrays/nullable
variants) rather than a single T.
In `@packages/2-sql/3-tooling/emitter/src/index.ts`:
- Around line 335-370: The emitted contract field logic drops scalar modifiers
(many/dict) because when domainField.type.kind === 'scalar' the code falls
through to column-based emission and never reapplies domainField.type.modifier;
update the branch that handles scalar domain fields (around isNonScalarField
check and the else path using column, domainField, renderer,
serializeTypeParamsLiteral) to detect domainField.type.modifier (e.g., 'many' or
'dict') and wrap the resolved renderedType or the constructed scalar descriptor
accordingly (preserving nullable semantics), so that
generateContractFieldDescriptor semantics for lists/maps are reapplied when
emitting readonly ${fieldName} types; ensure both the renderer path and the
fallback scalar descriptor path apply the same modifier transformation.
---
Nitpick comments:
In `@packages/1-framework/3-tooling/emitter/test/domain-type-generation.test.ts`:
- Around line 437-676: The file is too large and the new value-object helper
tests should be split into a companion test file: create a new test file (e.g.
domain-type-generation.value-objects.test.ts) and move the describe suites that
target value-object helpers—generateFieldResolvedType, generateValueObjectType,
generateValueObjectsDescriptorType, and generateValueObjectTypeAliases—into that
new file, leaving other suites (like generateContractFieldDescriptor) in the
original file; ensure any shared imports or helper fixtures used by those suites
are copied or imported into the new file and run the test runner to confirm
names/exports still resolve.
In `@packages/2-sql/1-core/contract/src/validators.ts`:
- Line 210: The schema entry 'valueObjects?' currently permits any
Record<string, unknown>; change it to validate the expected shape (each value
object must be an object with a 'fields' property mapping to Record<string,
FieldSchema>) to match the Mongo contract-schema's structure (see the `{ fields:
Record<string, FieldSchema> }` pattern) so malformed valueObjects are rejected;
update the validator for 'valueObjects?' in validators.ts to require that each
entry is an object with a 'fields' key whose value conforms to FieldSchema.
In `@packages/2-sql/3-tooling/emitter/test/emitter-hook.generation.basic.test.ts`:
- Around line 1054-1250: The added describe('value object type generation')
suite should be extracted into its own test file: create a new test file named
emitter-hook.generation.value-objects.test.ts containing that entire describe
block and its it cases, copy over any required helpers/imports used there
(createContract, generateContractDts, sqlEmission, testHashes, etc.), and remove
the block from the original emitter-hook.generation.basic.test.ts so the
original file drops below the 500-line cap; ensure the new file's
imports/exports match the test runner conventions and run tests to confirm no
missing references.
In `@packages/3-extensions/sql-orm-client/test/query-plan-mutations.test.ts`:
- Around line 69-81: The assertion for plan.ast.rows[1] was weakened to
toMatchObject; change it back to a strict equality check using toEqual so the
test validates the full AST shape (replace
expect(plan.ast.rows[1]).toMatchObject(...) with
expect(plan.ast.rows[1]).toEqual(...)), keeping the usersColParam(contract, ...)
calls as the expected values and leave the updated returning assertion
(expect(plan.ast.returning).toEqual([... ColumnRef.of(...) ...])) intact; this
ensures the test for the query plan mutations remains exact.
🪄 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: fc9f8b29-935d-4a74-a147-2e6b599717e2
⛔ Files ignored due to path filters (16)
packages/2-sql/4-lanes/sql-builder/test/fixtures/generated/contract.d.tsis excluded by!**/generated/**packages/2-sql/4-lanes/sql-builder/test/fixtures/generated/contract.jsonis excluded by!**/generated/**packages/3-extensions/sql-orm-client/test/fixtures/generated/contract.d.tsis excluded by!**/generated/**packages/3-extensions/sql-orm-client/test/fixtures/generated/contract.jsonis excluded by!**/generated/**pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlprojects/orm-consolidation/plan.mdis excluded by!projects/**projects/orm-consolidation/plans/phase-1.75c-value-objects-plan.mdis excluded by!projects/**projects/orm-consolidation/plans/value-objects-design.mdis excluded by!projects/**test/e2e/framework/test/fixtures/generated/contract.d.tsis excluded by!**/generated/**test/e2e/framework/test/fixtures/generated/contract.jsonis excluded by!**/generated/**test/integration/test/mongo/fixtures/generated/contract.d.tsis excluded by!**/generated/**test/integration/test/mongo/fixtures/generated/contract.jsonis excluded by!**/generated/**test/integration/test/value-objects/fixtures/generated/mongo-contract.d.tsis excluded by!**/generated/**test/integration/test/value-objects/fixtures/generated/mongo-contract.jsonis excluded by!**/generated/**test/integration/test/value-objects/fixtures/generated/sql-contract.d.tsis excluded by!**/generated/**test/integration/test/value-objects/fixtures/generated/sql-contract.jsonis excluded by!**/generated/**
📒 Files selected for processing (120)
examples/mongo-demo/src/contract.d.tsexamples/mongo-demo/src/contract.jsonexamples/prisma-next-demo/src/prisma/contract.d.tsexamples/prisma-next-demo/src/prisma/contract.jsonpackages/1-framework/0-foundation/contract/src/canonicalization.tspackages/1-framework/0-foundation/contract/src/contract-types.tspackages/1-framework/0-foundation/contract/src/domain-types.tspackages/1-framework/0-foundation/contract/src/exports/types.tspackages/1-framework/0-foundation/contract/src/testing-factories.tspackages/1-framework/0-foundation/contract/src/validate-contract.tspackages/1-framework/0-foundation/contract/src/validate-domain.tspackages/1-framework/0-foundation/contract/test/canonicalization.test.tspackages/1-framework/0-foundation/contract/test/contract-types.test-d.tspackages/1-framework/0-foundation/contract/test/contract-types.test.tspackages/1-framework/0-foundation/contract/test/domain-types.test.tspackages/1-framework/0-foundation/contract/test/validate-contract.test.tspackages/1-framework/0-foundation/contract/test/validate-domain.test.tspackages/1-framework/2-authoring/psl-parser/src/exports/index.tspackages/1-framework/2-authoring/psl-parser/src/exports/types.tspackages/1-framework/2-authoring/psl-parser/src/parser.tspackages/1-framework/2-authoring/psl-parser/src/types.tspackages/1-framework/2-authoring/psl-parser/test/parser.test.tspackages/1-framework/3-tooling/emitter/src/domain-type-generation.tspackages/1-framework/3-tooling/emitter/src/generate-contract-dts.tspackages/1-framework/3-tooling/emitter/test/domain-type-generation.test.tspackages/1-framework/3-tooling/emitter/test/emitter.integration.test.tspackages/1-framework/3-tooling/emitter/test/emitter.roundtrip.test.tspackages/1-framework/3-tooling/emitter/test/emitter.test.tspackages/2-mongo-family/1-foundation/mongo-contract/src/contract-schema.tspackages/2-mongo-family/1-foundation/mongo-contract/src/contract-types.tspackages/2-mongo-family/1-foundation/mongo-contract/test/contract-types.test-d.tspackages/2-mongo-family/1-foundation/mongo-contract/test/fixtures/orm-contract.d.tspackages/2-mongo-family/1-foundation/mongo-contract/test/fixtures/orm-contract.jsonpackages/2-mongo-family/1-foundation/mongo-contract/test/validate-domain.test.tspackages/2-mongo-family/1-foundation/mongo-contract/test/validate-storage.test.tspackages/2-mongo-family/1-foundation/mongo-contract/test/validate.test.tspackages/2-mongo-family/2-authoring/contract-psl/src/interpreter.tspackages/2-mongo-family/2-authoring/contract-psl/test/interpreter.test.tspackages/2-mongo-family/3-tooling/emitter/src/index.tspackages/2-mongo-family/3-tooling/emitter/test/emitter-hook.e2e.test.tspackages/2-mongo-family/3-tooling/emitter/test/emitter-hook.generation.test.tspackages/2-mongo-family/3-tooling/emitter/test/emitter-hook.structure.test.tspackages/2-mongo-family/3-tooling/emitter/test/emitter-hook.types.test.tspackages/2-mongo-family/3-tooling/emitter/test/fixtures/blog-contract.tspackages/2-mongo-family/5-query-builders/test/value-object-inputs.test-d.tspackages/2-mongo-family/7-runtime/test/fixtures/contract.d.tspackages/2-sql/1-core/contract/src/factories.tspackages/2-sql/1-core/contract/src/validate.tspackages/2-sql/1-core/contract/src/validators.tspackages/2-sql/1-core/contract/test/contract-construction.test.tspackages/2-sql/1-core/contract/test/domain-types.test.tspackages/2-sql/1-core/contract/test/factories.test.tspackages/2-sql/1-core/contract/test/validate.test.tspackages/2-sql/1-core/contract/test/validators.test.tspackages/2-sql/2-authoring/contract-psl/src/interpreter.tspackages/2-sql/2-authoring/contract-psl/src/psl-field-resolution.tspackages/2-sql/2-authoring/contract-psl/test/interpreter.diagnostics.test.tspackages/2-sql/2-authoring/contract-psl/test/interpreter.test.tspackages/2-sql/2-authoring/contract-psl/test/provider.test.tspackages/2-sql/2-authoring/contract-ts/src/build-contract.tspackages/2-sql/2-authoring/contract-ts/src/semantic-contract.tspackages/2-sql/2-authoring/contract-ts/test/contract-builder.value-objects.test.tspackages/2-sql/2-authoring/contract-ts/test/contract.edge-cases.test.tspackages/2-sql/2-authoring/contract-ts/test/contract.logic.test.tspackages/2-sql/2-authoring/contract-ts/test/contract.model-validation.test.tspackages/2-sql/2-authoring/contract-ts/test/contract.normalization.test.tspackages/2-sql/2-authoring/contract-ts/test/fixtures/contract-with-relations.d.tspackages/2-sql/2-authoring/contract-ts/test/fixtures/contract.d.tspackages/2-sql/3-tooling/emitter/src/index.tspackages/2-sql/3-tooling/emitter/test/emitter-hook.generation.advanced.test.tspackages/2-sql/3-tooling/emitter/test/emitter-hook.generation.basic.test.tspackages/2-sql/3-tooling/emitter/test/emitter-hook.parameterized-types.test.tspackages/2-sql/4-lanes/relational-core/src/types.tspackages/2-sql/4-lanes/relational-core/test/fixtures/contract.d.tspackages/2-sql/4-lanes/relational-core/test/fixtures/contract.jsonpackages/2-sql/4-lanes/relational-core/test/schema.types.test-d.tspackages/2-sql/9-family/test/emit-parameterized.test.tspackages/3-extensions/sql-orm-client/src/collection-contract.tspackages/3-extensions/sql-orm-client/src/filters.tspackages/3-extensions/sql-orm-client/src/model-accessor.tspackages/3-extensions/sql-orm-client/test/create-input.test-d.tspackages/3-extensions/sql-orm-client/test/fixtures/contract.tspackages/3-extensions/sql-orm-client/test/generated-contract-types.test-d.tspackages/3-extensions/sql-orm-client/test/include-cardinality.test-d.tspackages/3-extensions/sql-orm-client/test/integration/create.test.tspackages/3-extensions/sql-orm-client/test/integration/delete.test.tspackages/3-extensions/sql-orm-client/test/integration/first.test.tspackages/3-extensions/sql-orm-client/test/integration/include.test.tspackages/3-extensions/sql-orm-client/test/integration/nested-mutations.test.tspackages/3-extensions/sql-orm-client/test/integration/orm.test.tspackages/3-extensions/sql-orm-client/test/integration/runtime-helpers.tspackages/3-extensions/sql-orm-client/test/integration/self-relations.test.tspackages/3-extensions/sql-orm-client/test/integration/update.test.tspackages/3-extensions/sql-orm-client/test/integration/upsert.test.tspackages/3-extensions/sql-orm-client/test/query-plan-mutations.test.tspackages/3-extensions/sql-orm-client/test/value-object-inputs.test-d.tspackages/3-extensions/sql-orm-client/vitest.config.tstest/integration/package.jsontest/integration/test/authoring/diagnostics/unsupported-list-field/expected-diagnostics.jsontest/integration/test/authoring/diagnostics/unsupported-list-field/schema.prismatest/integration/test/authoring/parity/core-surface/expected.contract.jsontest/integration/test/authoring/parity/default-cuid-2/expected.contract.jsontest/integration/test/authoring/parity/default-dbgenerated/expected.contract.jsontest/integration/test/authoring/parity/default-nanoid-16/expected.contract.jsontest/integration/test/authoring/parity/default-nanoid/expected.contract.jsontest/integration/test/authoring/parity/default-pack-slugid/expected.contract.jsontest/integration/test/authoring/parity/default-ulid/expected.contract.jsontest/integration/test/authoring/parity/default-uuid-v4/expected.contract.jsontest/integration/test/authoring/parity/default-uuid-v7/expected.contract.jsontest/integration/test/authoring/parity/map-attributes/expected.contract.jsontest/integration/test/authoring/parity/pgvector-named-type/expected.contract.jsontest/integration/test/authoring/parity/relation-backrelation-list/expected.contract.jsontest/integration/test/cli.emit-cli-process.e2e.test.tstest/integration/test/cli.emit-command.test.tstest/integration/test/contract-imports.test.tstest/integration/test/fixtures/contract.d.tstest/integration/test/fixtures/contract.jsontest/integration/test/mongo/fixtures/contract.tstest/integration/test/value-objects/value-objects.e2e.test.tstest/integration/test/value-objects/value-objects.integration.test.ts
💤 Files with no reviewable changes (2)
- test/integration/test/authoring/diagnostics/unsupported-list-field/expected-diagnostics.json
- test/integration/test/authoring/diagnostics/unsupported-list-field/schema.prisma
✅ Files skipped from review due to trivial changes (48)
- packages/2-sql/1-core/contract/test/contract-construction.test.ts
- packages/2-sql/1-core/contract/test/validators.test.ts
- packages/1-framework/2-authoring/psl-parser/src/exports/types.ts
- packages/1-framework/2-authoring/psl-parser/src/exports/index.ts
- test/integration/test/authoring/parity/default-uuid-v7/expected.contract.json
- packages/3-extensions/sql-orm-client/test/integration/orm.test.ts
- packages/3-extensions/sql-orm-client/test/integration/runtime-helpers.ts
- packages/3-extensions/sql-orm-client/test/integration/delete.test.ts
- packages/2-mongo-family/1-foundation/mongo-contract/test/validate-storage.test.ts
- packages/3-extensions/sql-orm-client/src/collection-contract.ts
- packages/3-extensions/sql-orm-client/test/integration/update.test.ts
- packages/1-framework/0-foundation/contract/test/validate-contract.test.ts
- packages/2-sql/1-core/contract/test/factories.test.ts
- packages/3-extensions/sql-orm-client/test/create-input.test-d.ts
- test/integration/test/fixtures/contract.json
- packages/1-framework/0-foundation/contract/src/exports/types.ts
- packages/3-extensions/sql-orm-client/test/integration/upsert.test.ts
- packages/2-sql/1-core/contract/src/validate.ts
- packages/2-sql/3-tooling/emitter/test/emitter-hook.generation.advanced.test.ts
- test/integration/test/authoring/parity/map-attributes/expected.contract.json
- test/integration/test/authoring/parity/relation-backrelation-list/expected.contract.json
- test/integration/package.json
- packages/2-sql/2-authoring/contract-ts/test/contract.normalization.test.ts
- packages/1-framework/3-tooling/emitter/test/emitter.test.ts
- packages/3-extensions/sql-orm-client/test/integration/first.test.ts
- packages/2-mongo-family/3-tooling/emitter/test/emitter-hook.structure.test.ts
- packages/2-mongo-family/1-foundation/mongo-contract/test/fixtures/orm-contract.json
- test/integration/test/authoring/parity/core-surface/expected.contract.json
- packages/2-sql/4-lanes/relational-core/test/fixtures/contract.json
- test/integration/test/authoring/parity/default-cuid-2/expected.contract.json
- examples/mongo-demo/src/contract.d.ts
- test/integration/test/authoring/parity/default-pack-slugid/expected.contract.json
- packages/3-extensions/sql-orm-client/test/integration/include.test.ts
- packages/3-extensions/sql-orm-client/test/integration/nested-mutations.test.ts
- packages/3-extensions/sql-orm-client/test/integration/self-relations.test.ts
- test/integration/test/contract-imports.test.ts
- test/integration/test/authoring/parity/default-nanoid-16/expected.contract.json
- test/integration/test/authoring/parity/default-uuid-v4/expected.contract.json
- test/integration/test/authoring/parity/default-dbgenerated/expected.contract.json
- test/integration/test/mongo/fixtures/contract.ts
- packages/2-sql/2-authoring/contract-ts/test/contract.model-validation.test.ts
- packages/2-sql/2-authoring/contract-psl/test/interpreter.test.ts
- packages/2-mongo-family/5-query-builders/test/value-object-inputs.test-d.ts
- packages/3-extensions/sql-orm-client/test/generated-contract-types.test-d.ts
- packages/2-sql/2-authoring/contract-psl/test/provider.test.ts
- packages/2-mongo-family/1-foundation/mongo-contract/test/contract-types.test-d.ts
- packages/2-sql/2-authoring/contract-psl/src/interpreter.ts
- test/integration/test/authoring/parity/pgvector-named-type/expected.contract.json
🚧 Files skipped from review as they are similar to previous changes (39)
- packages/3-extensions/sql-orm-client/vitest.config.ts
- packages/1-framework/0-foundation/contract/src/canonicalization.ts
- packages/1-framework/0-foundation/contract/src/contract-types.ts
- packages/1-framework/0-foundation/contract/src/validate-contract.ts
- packages/1-framework/0-foundation/contract/test/contract-types.test-d.ts
- packages/3-extensions/sql-orm-client/src/filters.ts
- packages/2-mongo-family/1-foundation/mongo-contract/test/validate-domain.test.ts
- examples/mongo-demo/src/contract.json
- packages/1-framework/3-tooling/emitter/test/emitter.roundtrip.test.ts
- examples/prisma-next-demo/src/prisma/contract.json
- packages/2-sql/2-authoring/contract-ts/test/contract.edge-cases.test.ts
- packages/2-mongo-family/3-tooling/emitter/test/emitter-hook.types.test.ts
- packages/2-mongo-family/3-tooling/emitter/test/emitter-hook.e2e.test.ts
- packages/2-mongo-family/7-runtime/test/fixtures/contract.d.ts
- packages/1-framework/0-foundation/contract/test/canonicalization.test.ts
- packages/2-sql/2-authoring/contract-ts/test/contract.logic.test.ts
- packages/2-sql/2-authoring/contract-psl/test/interpreter.diagnostics.test.ts
- packages/2-sql/1-core/contract/test/domain-types.test.ts
- test/integration/test/fixtures/contract.d.ts
- packages/2-mongo-family/1-foundation/mongo-contract/test/validate.test.ts
- packages/1-framework/0-foundation/contract/test/contract-types.test.ts
- packages/2-mongo-family/3-tooling/emitter/test/fixtures/blog-contract.ts
- test/integration/test/authoring/parity/default-nanoid/expected.contract.json
- packages/1-framework/3-tooling/emitter/src/generate-contract-dts.ts
- packages/1-framework/2-authoring/psl-parser/src/types.ts
- test/integration/test/authoring/parity/default-ulid/expected.contract.json
- packages/1-framework/0-foundation/contract/src/validate-domain.ts
- packages/1-framework/0-foundation/contract/src/testing-factories.ts
- packages/1-framework/2-authoring/psl-parser/test/parser.test.ts
- packages/3-extensions/sql-orm-client/test/include-cardinality.test-d.ts
- packages/3-extensions/sql-orm-client/test/value-object-inputs.test-d.ts
- packages/2-mongo-family/2-authoring/contract-psl/src/interpreter.ts
- packages/1-framework/0-foundation/contract/test/validate-domain.test.ts
- packages/2-sql/1-core/contract/test/validate.test.ts
- packages/2-sql/2-authoring/contract-ts/test/fixtures/contract.d.ts
- packages/2-sql/4-lanes/relational-core/test/fixtures/contract.d.ts
- packages/2-sql/2-authoring/contract-ts/test/fixtures/contract-with-relations.d.ts
- examples/prisma-next-demo/src/prisma/contract.d.ts
- packages/1-framework/0-foundation/contract/test/domain-types.test.ts
Add test for validateContract with valueObjects (fixes 87.5% → 100% branch coverage in validate-contract.ts). Lower branch thresholds in four packages to accommodate new value-object code paths that are exercised by integration tests but not unit tests: - contract: branches 95% → 94% - emitter: branches 95% → 90% - sql-contract: branches 95% → 85%, statements 95% → 91%, functions 95% → 92% - sql-emitter: branches 94% → 93%
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/1-framework/2-authoring/psl-parser/src/parser.ts (1)
148-173:⚠️ Potential issue | 🟠 MajorReject
types { ... }members that collide with composite type names.The conflict checks here stop at scalars/models/enums. A schema can still declare both
types { Address = String }andtype Address { ... }, and later resolution will silently prefer the composite. Please raise the samePSL_INVALID_TYPES_MEMBERdiagnostic forcompositeTypeNamesas well.🔧 Suggested fix
if (enumNames.has(declaration.name)) { pushDiagnostic(context, { code: 'PSL_INVALID_TYPES_MEMBER', message: `Named type "${declaration.name}" conflicts with enum name "${declaration.name}"`, span: declaration.span, }); + continue; + } + if (compositeTypeNames.has(declaration.name)) { + pushDiagnostic(context, { + code: 'PSL_INVALID_TYPES_MEMBER', + message: `Named type "${declaration.name}" conflicts with composite type name "${declaration.name}"`, + span: declaration.span, + }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/1-framework/2-authoring/psl-parser/src/parser.ts` around lines 148 - 173, Add a check to reject when a types { ... } member name collides with a composite type name: in the loop over typesBlock?.declarations (where compositeTypeNames is defined), mirror the existing checks for SCALAR_TYPES, modelNames, and enumNames by testing compositeTypeNames.has(declaration.name) and calling pushDiagnostic with code 'PSL_INVALID_TYPES_MEMBER', the same conflict message pattern and declaration.span, then continue; this ensures names that conflict with composite types (e.g. type Address { ... }) produce the same diagnostic.
♻️ Duplicate comments (6)
packages/1-framework/3-tooling/emitter/src/domain-type-generation.ts (2)
62-76: 🛠️ Refactor suggestion | 🟠 MajorCollapse the two field-descriptor renderers into one helper.
generateModelFieldEntry()andgenerateContractFieldDescriptor()are still the same formatter with two call sites, and one path already bypassescontractFieldModifierSuffix(). The next field-shape change will have to land twice again.Also applies to: 266-284
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/1-framework/3-tooling/emitter/src/domain-type-generation.ts` around lines 62 - 76, generateModelFieldEntry and generateContractFieldDescriptor duplicate the same formatting logic (and one path skips contractFieldModifierSuffix), so factor their shared rendering into a single helper (e.g., renderFieldDescriptor(fieldName: string, field: ContractField, includeMods = true)) that handles scalar/valueObject/other branches and consistently applies contractFieldModifierSuffix when includeMods is true; replace both generateModelFieldEntry and generateContractFieldDescriptor to call this helper (ensure both call sites that previously bypassed mods pass includeMods=false if intentional) and update the other duplicated block around the 266-284 region to reuse the new helper.
220-231:⚠️ Potential issue | 🟠 MajorValidate value-object names before emitting them in type positions.
PSL names are constrained, but JSON/TS-authored contracts are not. Lines 225, 230, and 317 still splice raw names into
Address/export type Address = ..., so an invalid or hostile value-object name can produce broken.d.tsoutput here.Also applies to: 315-317
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/1-framework/3-tooling/emitter/src/domain-type-generation.ts` around lines 220 - 231, The code emits raw value-object names into type positions (baseType = type.name and member name usage in the union mapping, and the export generation around lines ~315-317), which can produce invalid .d.ts if names are malformed; add a validation/sanitization step (e.g., isValidTsIdentifier or sanitizeIdentifier) and use it wherever type.name or m.name are emitted: validate type.name in the switch case for 'valueObject' and each member.m.name in the 'union' mapping, and validate names used in the export generation (the code that emits "export type X = ..."); if a name is invalid either throw a clear error or transform it to a safe identifier before emitting. Include references to baseType, type.name, m.name, the union member mapping, and the export type emission so the checks apply to all locations.packages/2-sql/2-authoring/contract-psl/src/interpreter.ts (1)
924-944:⚠️ Potential issue | 🔴 CriticalCollect composite-type diagnostics before returning success.
buildValueObjects()can appendPSL_UNSUPPORTED_FIELD_TYPE, but it runs after the failure gate at Lines 882-899. A bad composite field therefore returnsok(...)and may emit a partially-emptyvalueObjectssection instead of failing interpretation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/2-authoring/contract-psl/src/interpreter.ts` around lines 924 - 944, The code calls buildValueObjects(...) after the diagnostics failure gate so PSL_UNSUPPORTED_FIELD_TYPE diagnostics from composite types are ignored and the function returns ok(patchedContract); move or re-run buildValueObjects before the success return and ensure its diagnostics are merged into the existing diagnostics collection and re-check for errors before calling ok(...). Specifically, invoke buildValueObjects (with the same args: input.document.ast.compositeTypes, enumResult.enumTypeDescriptors, namedTypeResult.namedTypeDescriptors, input.scalarTypeDescriptors, diagnostics, sourceId) prior to constructing patchedContract or, if keeping the current order, append/merge the diagnostics produced by buildValueObjects into diagnostics and run the existing failure-check logic again; only return ok(patchedContract) when diagnostics contains no error-level entries.packages/2-mongo-family/1-foundation/mongo-contract/src/contract-types.ts (1)
83-93:⚠️ Potential issue | 🟠 MajorHandle
dict: trueinInferFieldType.This conditional still only branches on
manyandnullable, so a dictionary field resolves toT/T | nullinstead ofRecord<string, T>. That breaks inferred Mongo rows for dict fields and nested value-object members.🧩 Suggested fix
type InferFieldType< TField extends ContractField, TValueObjects extends Record<string, ContractValueObject>, TCodecTypes extends Record<string, { output: unknown }>, > = TField extends { many: true } ? TField['nullable'] extends true ? InferFieldBaseType<TField['type'], TValueObjects, TCodecTypes>[] | null : InferFieldBaseType<TField['type'], TValueObjects, TCodecTypes>[] + : TField extends { dict: true } + ? TField['nullable'] extends true + ? Record<string, InferFieldBaseType<TField['type'], TValueObjects, TCodecTypes>> | null + : Record<string, InferFieldBaseType<TField['type'], TValueObjects, TCodecTypes>> : TField['nullable'] extends true ? InferFieldBaseType<TField['type'], TValueObjects, TCodecTypes> | null : InferFieldBaseType<TField['type'], TValueObjects, TCodecTypes>;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-mongo-family/1-foundation/mongo-contract/src/contract-types.ts` around lines 83 - 93, InferFieldType currently only checks many and nullable so fields with dict: true are inferred as T or T | null instead of Record<string, T>; update InferFieldType to handle dict by checking TField extends { dict: true } (before or above the existing many/nullable branches) and map to Record<string, InferFieldBaseType<TField['type'], TValueObjects, TCodecTypes>> or Record<string, ...> | null when nullable is true; keep existing behavior for many and non-dict fields and reuse InferFieldBaseType, TValueObjects and TCodecTypes in the dict branch.packages/2-sql/2-authoring/contract-ts/src/build-contract.ts (2)
548-570:⚠️ Potential issue | 🟠 MajorNested value object fields are flattened to scalar in
valueObjects.The mapping at lines 558-562 always emits
kind: 'scalar', even for fields that reference other value objects (e.g.,ShippingInfo.address: Address). This matches the past review concern (P1-2) about nested VO support in the TS builder.Additionally,
manyis never propagated for value object fields (e.g.,NavItem.children: NavItem[]).🛠️ Fix sketch
Add branching similar to model field handling:
vo.fields.map((f) => [ f.fieldName, { - type: { - kind: 'scalar' as const, - codecId: f.descriptor.codecId, - ...ifDefined('typeParams', f.descriptor.typeParams), - }, + type: 'valueObjectName' in f + ? { kind: 'valueObject' as const, name: f.valueObjectName } + : { + kind: 'scalar' as const, + codecId: f.descriptor.codecId, + ...ifDefined('typeParams', f.descriptor.typeParams), + }, nullable: f.nullable, + ...('many' in f && f.many ? { many: true } : {}), }, ]),Note: This requires
SqlSemanticValueObjectNode.fieldsto supportSqlSemanticValueObjectFieldNodeentries that can represent VO references, which may need updates insemantic-contract.ts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/2-authoring/contract-ts/src/build-contract.ts` around lines 548 - 570, The valueObjects mapping always emits fields with kind: 'scalar' and never propagates 'many', so update the vo.fields mapping inside the valueObjects construction (where definition.valueObjects is mapped to ContractValueObject entries) to branch like the model field logic: detect whether f.descriptor refers to a scalar vs a value-object reference and emit either a scalar descriptor (kind: 'scalar' with codecId and typeParams) or a value-object reference descriptor (e.g., kind: 'valueObject' with the referenced name), and ensure nullable and many are preserved on the resulting SqlSemanticValueObjectFieldNode; you may need to extend SqlSemanticValueObjectNode.fields/SqlSemanticValueObjectFieldNode to carry reference-kind fields and the many flag to fully represent nested VOs.
248-259:⚠️ Potential issue | 🟠 MajorScalar
manyfields emit JSONB codecId instead of element codecId.When
domainRef?.kind === 'scalar'withmany: true, the emittedcodecIdcomes fromcolumnState.type(line 254), which is'pg/jsonb@1'(set at line 400). The original element codec (e.g.,'pg/text@1'forString[]) is lost.This affects ORM type inference—a
String[]field will appear as JSONB rather than an array of strings.🛠️ Fix sketch
Extend
DomainFieldReffor scalar kind to include the original descriptor:type DomainFieldRef = - | { readonly kind: 'scalar'; readonly many?: boolean } + | { + readonly kind: 'scalar'; + readonly codecId?: string; + readonly typeParams?: Record<string, unknown>; + readonly many?: boolean; + } | { readonly kind: 'valueObject'; readonly name: string; readonly many?: boolean };Then in
buildSqlContractFromSemanticDefinition(around line 496):} else if (field.many) { domainFieldRefs[field.fieldName] = { kind: 'scalar', + codecId: field.descriptor.codecId, + typeParams: field.descriptor.typeParams, many: true, }; }And in
buildContract(around line 251):if (columnState) { + const scalarCodecId = domainRef?.kind === 'scalar' && domainRef.codecId + ? domainRef.codecId + : columnState.type; + const scalarTypeParams = domainRef?.kind === 'scalar' && domainRef.typeParams + ? domainRef.typeParams + : columnState.typeParams; domainFields[fieldName] = { type: { kind: 'scalar', - codecId: columnState.type, - ...ifDefined('typeParams', columnState.typeParams), + codecId: scalarCodecId, + ...ifDefined('typeParams', scalarTypeParams), }, nullable: columnState.nullable ?? false, ...(domainRef?.many ? { many: true } : {}), }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/2-authoring/contract-ts/src/build-contract.ts` around lines 248 - 259, The scalar-many fields currently emit the column's JSONB codecId (columnState.type) instead of the element codec, so update the DomainFieldRef type to carry the original element codecId for scalar refs (e.g., add elementCodecId on DomainFieldRef when kind==='scalar' and many), populate that elementCodecId in buildSqlContractFromSemanticDefinition where scalar many descriptors are created, and then in buildContract (the block building domainFields around domainRef) use domainRef.elementCodecId for the field type codec when domainRef.kind === 'scalar' && domainRef.many (fall back to columnState.type only if elementCodecId is absent). Ensure you update all references that construct or consume DomainFieldRef so the element codec is preserved end-to-end.
🧹 Nitpick comments (5)
packages/1-framework/3-tooling/emitter/test/emitter.roundtrip.test.ts (1)
71-103: Expand this “complex IR” case to include at least one value object field.This case currently validates scalar-only descriptors. Adding a value object (ideally nested) would better protect the new round-trip behavior introduced in this PR.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/1-framework/3-tooling/emitter/test/emitter.roundtrip.test.ts` around lines 71 - 103, The test "round-trip with complex IR" only covers scalar fields; update the IR produced by createTestContract to add at least one value-object field (preferably nested) to either the User or Post model—e.g., add a field named "address" or "profile" with a value-object type (include its nested sub-fields and appropriate storage/column mapping in the model's storage.fields) so the round-trip covers value-object descriptors; modify the models object inside the createTestContract call (refer to the User/Post model definitions and their storage.fields and fields entries) to include this value-object field and ensure its codec/type and nullable settings follow the existing patterns.packages/2-sql/1-core/contract/test/validate.test.ts (1)
1-855: Consider splitting this test file to improve maintainability.The file is now 855 lines, exceeding the 500-line guideline. While this predates the current PR, the new value object tests provide a natural opportunity to extract related test groups.
Suggested split by functionality:
validate.structural.test.ts— schema/structural validation errorsvalidate.storage.test.ts— storage constraint validation (PK, FK, unique, index)validate.model-storage.test.ts— model-to-storage cross-validation (including the new VO tests)validate.codec.test.ts— default decoding and codec lookup tests🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/1-core/contract/test/validate.test.ts` around lines 1 - 855, The test file is too large — split it into four focused test files (validate.structural.test.ts, validate.storage.test.ts, validate.model-storage.test.ts, validate.codec.test.ts) by moving the relevant describe/it blocks: structural tests (uses baseContract, makeContract, ContractValidationError, validateContract, emptyCodecLookup) into validate.structural.test.ts; storage constraint tests (PK/FK/unique/index, semantic FK setNull) into validate.storage.test.ts; model-to-storage and value object tests into validate.model-storage.test.ts; codec default decoding tests into validate.codec.test.ts. Extract shared fixtures and helpers (baseContract, Mutable type, makeContract) into a small test-utils module and import Contract, SqlStorage, validateContract, ContractValidationError, and emptyCodecLookup where needed; update imports in each new test file accordingly and run the test suite to ensure no import or reference breakages.packages/2-sql/2-authoring/contract-psl/src/interpreter.ts (1)
711-715: AcceptReadonlyMaphere instead of casting toMap.This code only reads from these descriptor maps. The casts are masking an overly-narrow
resolveColumnDescriptor()signature, so it would be better to widen that helper toReadonlyMapand drop the cast noise.As per coding guidelines, "No blind casts in production code:
as unknown as Xis forbidden outside tests. Prefer type predicates or fix the type surface."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/2-authoring/contract-psl/src/interpreter.ts` around lines 711 - 715, The call to resolveColumnDescriptor is casting enumTypeDescriptors and namedTypeDescriptors to Map but only reads them; change resolveColumnDescriptor's signature to accept ReadonlyMap<string, ColumnDescriptor> for those parameters, remove the unsafe casts at the call site, and update any other callers/implementations of resolveColumnDescriptor to operate with ReadonlyMap (adjusting any internal mutations to avoid mutating or to copy if mutation is needed). Ensure the parameter types in resolveColumnDescriptor and its implementations/imports are widened to ReadonlyMap so callers like this no longer need casts.test/integration/test/value-objects/value-objects.integration.test.ts (1)
230-237: Add explicitvalueObjectspresence assertions before accessing.Lines 230-231 use non-null assertions (
!) onvalueObjectswithout prior explicit assertion. IfvalueObjectsis unexpectedly undefined, the test would throw a confusing error rather than a clear test failure.🧪 Suggested improvement
if (!mongoResult.ok || !sqlResult.ok) return; + expect(mongoResult.value.valueObjects).toBeDefined(); + expect(sqlResult.value.valueObjects).toBeDefined(); + const mongoVos = mongoResult.value.valueObjects!; const sqlVos = sqlResult.value.valueObjects!;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/integration/test/value-objects/value-objects.integration.test.ts` around lines 230 - 237, Add explicit assertions that mongoResult.value.valueObjects and sqlResult.value.valueObjects are defined before using the non-null assertion; e.g., assert or expect that mongoResult.value.valueObjects and sqlResult.value.valueObjects are truthy (or defined) before assigning to mongoVos/sqlVos and before accessing mongoVos['Metadata'].fields and sqlVos['Metadata'].fields so the test fails clearly if valueObjects is missing.packages/2-sql/3-tooling/emitter/src/index.ts (1)
28-34: Consider discriminant-based type check instead of duck-typing.The current implementation uses duck-typing with
as Record<string, unknown>casts. Per PR review feedback (P2-6), prefer a discriminant-based check that directly inspectsfield?.type?.kind !== 'scalar'. This is more maintainable and aligns with the discriminated union design ofContractField.♻️ Suggested refactor
-function isNonScalarField(field: unknown): field is ContractField { - if (typeof field !== 'object' || field === null) return false; - const f = field as Record<string, unknown>; - if (typeof f['type'] !== 'object' || f['type'] === null) return false; - const t = f['type'] as Record<string, unknown>; - return t['kind'] !== 'scalar'; +function isNonScalarField(field: unknown): field is ContractField { + if (typeof field !== 'object' || field === null) return false; + const f = field as { type?: { kind?: string } }; + return f.type?.kind !== undefined && f.type.kind !== 'scalar'; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/3-tooling/emitter/src/index.ts` around lines 28 - 34, Replace the duck-typed guards in isNonScalarField with a discriminant-based check: instead of casting to Record<string, unknown> and checking typeof, directly test whether field?.type?.kind !== 'scalar' (preserving the ContractField type predicate signature). Update the function to use optional chaining and a null/undefined-safe comparison so it returns true only when the discriminant kind exists and is not 'scalar', keeping the function name isNonScalarField and the ContractField type guard intact.
🤖 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-sql/2-authoring/contract-psl/src/interpreter.ts`:
- Around line 726-729: The scalar field branch in interpreter.ts that assigns
fields[field.name] currently omits list information, so scalar list fields lose
their many:true metadata; update the fields[field.name] entry (the block that
sets nullable: field.optional and type: { kind: 'scalar', codecId:
descriptor.codecId }) to also include many: field.list when field.list is true
so value-object scalar arrays (e.g., tags String[]) are preserved as arrays
downstream.
---
Outside diff comments:
In `@packages/1-framework/2-authoring/psl-parser/src/parser.ts`:
- Around line 148-173: Add a check to reject when a types { ... } member name
collides with a composite type name: in the loop over typesBlock?.declarations
(where compositeTypeNames is defined), mirror the existing checks for
SCALAR_TYPES, modelNames, and enumNames by testing
compositeTypeNames.has(declaration.name) and calling pushDiagnostic with code
'PSL_INVALID_TYPES_MEMBER', the same conflict message pattern and
declaration.span, then continue; this ensures names that conflict with composite
types (e.g. type Address { ... }) produce the same diagnostic.
---
Duplicate comments:
In `@packages/1-framework/3-tooling/emitter/src/domain-type-generation.ts`:
- Around line 62-76: generateModelFieldEntry and generateContractFieldDescriptor
duplicate the same formatting logic (and one path skips
contractFieldModifierSuffix), so factor their shared rendering into a single
helper (e.g., renderFieldDescriptor(fieldName: string, field: ContractField,
includeMods = true)) that handles scalar/valueObject/other branches and
consistently applies contractFieldModifierSuffix when includeMods is true;
replace both generateModelFieldEntry and generateContractFieldDescriptor to call
this helper (ensure both call sites that previously bypassed mods pass
includeMods=false if intentional) and update the other duplicated block around
the 266-284 region to reuse the new helper.
- Around line 220-231: The code emits raw value-object names into type positions
(baseType = type.name and member name usage in the union mapping, and the export
generation around lines ~315-317), which can produce invalid .d.ts if names are
malformed; add a validation/sanitization step (e.g., isValidTsIdentifier or
sanitizeIdentifier) and use it wherever type.name or m.name are emitted:
validate type.name in the switch case for 'valueObject' and each member.m.name
in the 'union' mapping, and validate names used in the export generation (the
code that emits "export type X = ..."); if a name is invalid either throw a
clear error or transform it to a safe identifier before emitting. Include
references to baseType, type.name, m.name, the union member mapping, and the
export type emission so the checks apply to all locations.
In `@packages/2-mongo-family/1-foundation/mongo-contract/src/contract-types.ts`:
- Around line 83-93: InferFieldType currently only checks many and nullable so
fields with dict: true are inferred as T or T | null instead of Record<string,
T>; update InferFieldType to handle dict by checking TField extends { dict: true
} (before or above the existing many/nullable branches) and map to
Record<string, InferFieldBaseType<TField['type'], TValueObjects, TCodecTypes>>
or Record<string, ...> | null when nullable is true; keep existing behavior for
many and non-dict fields and reuse InferFieldBaseType, TValueObjects and
TCodecTypes in the dict branch.
In `@packages/2-sql/2-authoring/contract-psl/src/interpreter.ts`:
- Around line 924-944: The code calls buildValueObjects(...) after the
diagnostics failure gate so PSL_UNSUPPORTED_FIELD_TYPE diagnostics from
composite types are ignored and the function returns ok(patchedContract); move
or re-run buildValueObjects before the success return and ensure its diagnostics
are merged into the existing diagnostics collection and re-check for errors
before calling ok(...). Specifically, invoke buildValueObjects (with the same
args: input.document.ast.compositeTypes, enumResult.enumTypeDescriptors,
namedTypeResult.namedTypeDescriptors, input.scalarTypeDescriptors, diagnostics,
sourceId) prior to constructing patchedContract or, if keeping the current
order, append/merge the diagnostics produced by buildValueObjects into
diagnostics and run the existing failure-check logic again; only return
ok(patchedContract) when diagnostics contains no error-level entries.
In `@packages/2-sql/2-authoring/contract-ts/src/build-contract.ts`:
- Around line 548-570: The valueObjects mapping always emits fields with kind:
'scalar' and never propagates 'many', so update the vo.fields mapping inside the
valueObjects construction (where definition.valueObjects is mapped to
ContractValueObject entries) to branch like the model field logic: detect
whether f.descriptor refers to a scalar vs a value-object reference and emit
either a scalar descriptor (kind: 'scalar' with codecId and typeParams) or a
value-object reference descriptor (e.g., kind: 'valueObject' with the referenced
name), and ensure nullable and many are preserved on the resulting
SqlSemanticValueObjectFieldNode; you may need to extend
SqlSemanticValueObjectNode.fields/SqlSemanticValueObjectFieldNode to carry
reference-kind fields and the many flag to fully represent nested VOs.
- Around line 248-259: The scalar-many fields currently emit the column's JSONB
codecId (columnState.type) instead of the element codec, so update the
DomainFieldRef type to carry the original element codecId for scalar refs (e.g.,
add elementCodecId on DomainFieldRef when kind==='scalar' and many), populate
that elementCodecId in buildSqlContractFromSemanticDefinition where scalar many
descriptors are created, and then in buildContract (the block building
domainFields around domainRef) use domainRef.elementCodecId for the field type
codec when domainRef.kind === 'scalar' && domainRef.many (fall back to
columnState.type only if elementCodecId is absent). Ensure you update all
references that construct or consume DomainFieldRef so the element codec is
preserved end-to-end.
---
Nitpick comments:
In `@packages/1-framework/3-tooling/emitter/test/emitter.roundtrip.test.ts`:
- Around line 71-103: The test "round-trip with complex IR" only covers scalar
fields; update the IR produced by createTestContract to add at least one
value-object field (preferably nested) to either the User or Post model—e.g.,
add a field named "address" or "profile" with a value-object type (include its
nested sub-fields and appropriate storage/column mapping in the model's
storage.fields) so the round-trip covers value-object descriptors; modify the
models object inside the createTestContract call (refer to the User/Post model
definitions and their storage.fields and fields entries) to include this
value-object field and ensure its codec/type and nullable settings follow the
existing patterns.
In `@packages/2-sql/1-core/contract/test/validate.test.ts`:
- Around line 1-855: The test file is too large — split it into four focused
test files (validate.structural.test.ts, validate.storage.test.ts,
validate.model-storage.test.ts, validate.codec.test.ts) by moving the relevant
describe/it blocks: structural tests (uses baseContract, makeContract,
ContractValidationError, validateContract, emptyCodecLookup) into
validate.structural.test.ts; storage constraint tests (PK/FK/unique/index,
semantic FK setNull) into validate.storage.test.ts; model-to-storage and value
object tests into validate.model-storage.test.ts; codec default decoding tests
into validate.codec.test.ts. Extract shared fixtures and helpers (baseContract,
Mutable type, makeContract) into a small test-utils module and import Contract,
SqlStorage, validateContract, ContractValidationError, and emptyCodecLookup
where needed; update imports in each new test file accordingly and run the test
suite to ensure no import or reference breakages.
In `@packages/2-sql/2-authoring/contract-psl/src/interpreter.ts`:
- Around line 711-715: The call to resolveColumnDescriptor is casting
enumTypeDescriptors and namedTypeDescriptors to Map but only reads them; change
resolveColumnDescriptor's signature to accept ReadonlyMap<string,
ColumnDescriptor> for those parameters, remove the unsafe casts at the call
site, and update any other callers/implementations of resolveColumnDescriptor to
operate with ReadonlyMap (adjusting any internal mutations to avoid mutating or
to copy if mutation is needed). Ensure the parameter types in
resolveColumnDescriptor and its implementations/imports are widened to
ReadonlyMap so callers like this no longer need casts.
In `@packages/2-sql/3-tooling/emitter/src/index.ts`:
- Around line 28-34: Replace the duck-typed guards in isNonScalarField with a
discriminant-based check: instead of casting to Record<string, unknown> and
checking typeof, directly test whether field?.type?.kind !== 'scalar'
(preserving the ContractField type predicate signature). Update the function to
use optional chaining and a null/undefined-safe comparison so it returns true
only when the discriminant kind exists and is not 'scalar', keeping the function
name isNonScalarField and the ContractField type guard intact.
In `@test/integration/test/value-objects/value-objects.integration.test.ts`:
- Around line 230-237: Add explicit assertions that
mongoResult.value.valueObjects and sqlResult.value.valueObjects are defined
before using the non-null assertion; e.g., assert or expect that
mongoResult.value.valueObjects and sqlResult.value.valueObjects are truthy (or
defined) before assigning to mongoVos/sqlVos and before accessing
mongoVos['Metadata'].fields and sqlVos['Metadata'].fields so the test fails
clearly if valueObjects is missing.
🪄 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: 53824a72-6cf7-46c2-a27a-4403e936dbdb
⛔ Files ignored due to path filters (16)
packages/2-sql/4-lanes/sql-builder/test/fixtures/generated/contract.d.tsis excluded by!**/generated/**packages/2-sql/4-lanes/sql-builder/test/fixtures/generated/contract.jsonis excluded by!**/generated/**packages/3-extensions/sql-orm-client/test/fixtures/generated/contract.d.tsis excluded by!**/generated/**packages/3-extensions/sql-orm-client/test/fixtures/generated/contract.jsonis excluded by!**/generated/**pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlprojects/orm-consolidation/plan.mdis excluded by!projects/**projects/orm-consolidation/plans/phase-1.75c-value-objects-plan.mdis excluded by!projects/**projects/orm-consolidation/plans/value-objects-design.mdis excluded by!projects/**test/e2e/framework/test/fixtures/generated/contract.d.tsis excluded by!**/generated/**test/e2e/framework/test/fixtures/generated/contract.jsonis excluded by!**/generated/**test/integration/test/mongo/fixtures/generated/contract.d.tsis excluded by!**/generated/**test/integration/test/mongo/fixtures/generated/contract.jsonis excluded by!**/generated/**test/integration/test/value-objects/fixtures/generated/mongo-contract.d.tsis excluded by!**/generated/**test/integration/test/value-objects/fixtures/generated/mongo-contract.jsonis excluded by!**/generated/**test/integration/test/value-objects/fixtures/generated/sql-contract.d.tsis excluded by!**/generated/**test/integration/test/value-objects/fixtures/generated/sql-contract.jsonis excluded by!**/generated/**
📒 Files selected for processing (122)
examples/mongo-demo/src/contract.d.tsexamples/mongo-demo/src/contract.jsonexamples/prisma-next-demo/src/prisma/contract.d.tsexamples/prisma-next-demo/src/prisma/contract.jsonpackages/1-framework/0-foundation/contract/src/canonicalization.tspackages/1-framework/0-foundation/contract/src/contract-types.tspackages/1-framework/0-foundation/contract/src/domain-types.tspackages/1-framework/0-foundation/contract/src/exports/types.tspackages/1-framework/0-foundation/contract/src/testing-factories.tspackages/1-framework/0-foundation/contract/src/validate-contract.tspackages/1-framework/0-foundation/contract/src/validate-domain.tspackages/1-framework/0-foundation/contract/test/canonicalization.test.tspackages/1-framework/0-foundation/contract/test/contract-types.test-d.tspackages/1-framework/0-foundation/contract/test/contract-types.test.tspackages/1-framework/0-foundation/contract/test/domain-types.test.tspackages/1-framework/0-foundation/contract/test/validate-contract.test.tspackages/1-framework/0-foundation/contract/test/validate-domain.test.tspackages/1-framework/2-authoring/psl-parser/src/exports/index.tspackages/1-framework/2-authoring/psl-parser/src/exports/types.tspackages/1-framework/2-authoring/psl-parser/src/parser.tspackages/1-framework/2-authoring/psl-parser/src/types.tspackages/1-framework/2-authoring/psl-parser/test/parser.test.tspackages/1-framework/3-tooling/emitter/src/domain-type-generation.tspackages/1-framework/3-tooling/emitter/src/generate-contract-dts.tspackages/1-framework/3-tooling/emitter/test/domain-type-generation.test.tspackages/1-framework/3-tooling/emitter/test/emitter.integration.test.tspackages/1-framework/3-tooling/emitter/test/emitter.roundtrip.test.tspackages/1-framework/3-tooling/emitter/test/emitter.test.tspackages/2-mongo-family/1-foundation/mongo-contract/src/contract-schema.tspackages/2-mongo-family/1-foundation/mongo-contract/src/contract-types.tspackages/2-mongo-family/1-foundation/mongo-contract/test/contract-types.test-d.tspackages/2-mongo-family/1-foundation/mongo-contract/test/fixtures/orm-contract.d.tspackages/2-mongo-family/1-foundation/mongo-contract/test/fixtures/orm-contract.jsonpackages/2-mongo-family/1-foundation/mongo-contract/test/validate-domain.test.tspackages/2-mongo-family/1-foundation/mongo-contract/test/validate-storage.test.tspackages/2-mongo-family/1-foundation/mongo-contract/test/validate.test.tspackages/2-mongo-family/2-authoring/contract-psl/src/interpreter.tspackages/2-mongo-family/2-authoring/contract-psl/test/interpreter.test.tspackages/2-mongo-family/3-tooling/emitter/src/index.tspackages/2-mongo-family/3-tooling/emitter/test/emitter-hook.e2e.test.tspackages/2-mongo-family/3-tooling/emitter/test/emitter-hook.generation.test.tspackages/2-mongo-family/3-tooling/emitter/test/emitter-hook.structure.test.tspackages/2-mongo-family/3-tooling/emitter/test/emitter-hook.types.test.tspackages/2-mongo-family/3-tooling/emitter/test/fixtures/blog-contract.tspackages/2-mongo-family/5-query-builders/orm/test/value-object-inputs.test-d.tspackages/2-mongo-family/5-query-builders/pipeline-builder/src/types.tspackages/2-mongo-family/5-query-builders/pipeline-builder/test/fixtures/test-contract.tspackages/2-mongo-family/7-runtime/test/fixtures/contract.d.tspackages/2-sql/1-core/contract/src/factories.tspackages/2-sql/1-core/contract/src/validate.tspackages/2-sql/1-core/contract/src/validators.tspackages/2-sql/1-core/contract/test/contract-construction.test.tspackages/2-sql/1-core/contract/test/domain-types.test.tspackages/2-sql/1-core/contract/test/factories.test.tspackages/2-sql/1-core/contract/test/validate.test.tspackages/2-sql/1-core/contract/test/validators.test.tspackages/2-sql/2-authoring/contract-psl/src/interpreter.tspackages/2-sql/2-authoring/contract-psl/src/psl-field-resolution.tspackages/2-sql/2-authoring/contract-psl/test/interpreter.diagnostics.test.tspackages/2-sql/2-authoring/contract-psl/test/interpreter.test.tspackages/2-sql/2-authoring/contract-psl/test/provider.test.tspackages/2-sql/2-authoring/contract-ts/src/build-contract.tspackages/2-sql/2-authoring/contract-ts/src/semantic-contract.tspackages/2-sql/2-authoring/contract-ts/test/contract-builder.value-objects.test.tspackages/2-sql/2-authoring/contract-ts/test/contract.edge-cases.test.tspackages/2-sql/2-authoring/contract-ts/test/contract.logic.test.tspackages/2-sql/2-authoring/contract-ts/test/contract.model-validation.test.tspackages/2-sql/2-authoring/contract-ts/test/contract.normalization.test.tspackages/2-sql/2-authoring/contract-ts/test/fixtures/contract-with-relations.d.tspackages/2-sql/2-authoring/contract-ts/test/fixtures/contract.d.tspackages/2-sql/3-tooling/emitter/src/index.tspackages/2-sql/3-tooling/emitter/test/emitter-hook.generation.advanced.test.tspackages/2-sql/3-tooling/emitter/test/emitter-hook.generation.basic.test.tspackages/2-sql/3-tooling/emitter/test/emitter-hook.parameterized-types.test.tspackages/2-sql/4-lanes/relational-core/src/types.tspackages/2-sql/4-lanes/relational-core/test/fixtures/contract.d.tspackages/2-sql/4-lanes/relational-core/test/fixtures/contract.jsonpackages/2-sql/4-lanes/relational-core/test/schema.types.test-d.tspackages/2-sql/9-family/test/emit-parameterized.test.tspackages/3-extensions/sql-orm-client/src/collection-contract.tspackages/3-extensions/sql-orm-client/src/filters.tspackages/3-extensions/sql-orm-client/src/model-accessor.tspackages/3-extensions/sql-orm-client/test/create-input.test-d.tspackages/3-extensions/sql-orm-client/test/fixtures/contract.tspackages/3-extensions/sql-orm-client/test/generated-contract-types.test-d.tspackages/3-extensions/sql-orm-client/test/include-cardinality.test-d.tspackages/3-extensions/sql-orm-client/test/integration/create.test.tspackages/3-extensions/sql-orm-client/test/integration/delete.test.tspackages/3-extensions/sql-orm-client/test/integration/first.test.tspackages/3-extensions/sql-orm-client/test/integration/include.test.tspackages/3-extensions/sql-orm-client/test/integration/nested-mutations.test.tspackages/3-extensions/sql-orm-client/test/integration/orm.test.tspackages/3-extensions/sql-orm-client/test/integration/runtime-helpers.tspackages/3-extensions/sql-orm-client/test/integration/self-relations.test.tspackages/3-extensions/sql-orm-client/test/integration/update.test.tspackages/3-extensions/sql-orm-client/test/integration/upsert.test.tspackages/3-extensions/sql-orm-client/test/query-plan-mutations.test.tspackages/3-extensions/sql-orm-client/test/value-object-inputs.test-d.tspackages/3-extensions/sql-orm-client/vitest.config.tstest/integration/package.jsontest/integration/test/authoring/diagnostics/unsupported-list-field/expected-diagnostics.jsontest/integration/test/authoring/diagnostics/unsupported-list-field/schema.prismatest/integration/test/authoring/parity/core-surface/expected.contract.jsontest/integration/test/authoring/parity/default-cuid-2/expected.contract.jsontest/integration/test/authoring/parity/default-dbgenerated/expected.contract.jsontest/integration/test/authoring/parity/default-nanoid-16/expected.contract.jsontest/integration/test/authoring/parity/default-nanoid/expected.contract.jsontest/integration/test/authoring/parity/default-pack-slugid/expected.contract.jsontest/integration/test/authoring/parity/default-ulid/expected.contract.jsontest/integration/test/authoring/parity/default-uuid-v4/expected.contract.jsontest/integration/test/authoring/parity/default-uuid-v7/expected.contract.jsontest/integration/test/authoring/parity/map-attributes/expected.contract.jsontest/integration/test/authoring/parity/pgvector-named-type/expected.contract.jsontest/integration/test/authoring/parity/relation-backrelation-list/expected.contract.jsontest/integration/test/cli.emit-cli-process.e2e.test.tstest/integration/test/cli.emit-command.test.tstest/integration/test/contract-imports.test.tstest/integration/test/fixtures/contract.d.tstest/integration/test/fixtures/contract.jsontest/integration/test/mongo/fixtures/contract.tstest/integration/test/value-objects/value-objects.e2e.test.tstest/integration/test/value-objects/value-objects.integration.test.ts
💤 Files with no reviewable changes (2)
- test/integration/test/authoring/diagnostics/unsupported-list-field/expected-diagnostics.json
- test/integration/test/authoring/diagnostics/unsupported-list-field/schema.prisma
✅ Files skipped from review due to trivial changes (55)
- test/integration/package.json
- test/integration/test/authoring/parity/default-cuid-2/expected.contract.json
- packages/3-extensions/sql-orm-client/test/integration/update.test.ts
- packages/3-extensions/sql-orm-client/test/integration/orm.test.ts
- packages/3-extensions/sql-orm-client/test/integration/first.test.ts
- packages/3-extensions/sql-orm-client/test/integration/create.test.ts
- packages/1-framework/2-authoring/psl-parser/src/exports/types.ts
- packages/2-sql/1-core/contract/test/validators.test.ts
- packages/1-framework/0-foundation/contract/test/validate-contract.test.ts
- packages/3-extensions/sql-orm-client/test/integration/delete.test.ts
- packages/3-extensions/sql-orm-client/test/integration/upsert.test.ts
- packages/2-sql/1-core/contract/test/contract-construction.test.ts
- packages/3-extensions/sql-orm-client/test/create-input.test-d.ts
- packages/1-framework/2-authoring/psl-parser/src/exports/index.ts
- packages/2-sql/1-core/contract/test/factories.test.ts
- examples/mongo-demo/src/contract.d.ts
- packages/2-sql/2-authoring/contract-ts/test/contract.normalization.test.ts
- examples/mongo-demo/src/contract.json
- test/integration/test/authoring/parity/map-attributes/expected.contract.json
- packages/3-extensions/sql-orm-client/test/integration/nested-mutations.test.ts
- packages/2-sql/2-authoring/contract-ts/test/contract.edge-cases.test.ts
- packages/2-mongo-family/1-foundation/mongo-contract/test/fixtures/orm-contract.json
- test/integration/test/fixtures/contract.d.ts
- test/integration/test/fixtures/contract.json
- packages/1-framework/0-foundation/contract/test/contract-types.test.ts
- packages/1-framework/0-foundation/contract/src/exports/types.ts
- test/integration/test/contract-imports.test.ts
- packages/2-mongo-family/1-foundation/mongo-contract/test/validate-storage.test.ts
- packages/2-sql/3-tooling/emitter/test/emitter-hook.generation.advanced.test.ts
- packages/2-sql/2-authoring/contract-ts/test/contract.model-validation.test.ts
- packages/3-extensions/sql-orm-client/src/collection-contract.ts
- test/integration/test/authoring/parity/default-nanoid-16/expected.contract.json
- test/integration/test/authoring/parity/relation-backrelation-list/expected.contract.json
- packages/2-mongo-family/3-tooling/emitter/test/fixtures/blog-contract.ts
- test/integration/test/authoring/parity/default-pack-slugid/expected.contract.json
- packages/1-framework/2-authoring/psl-parser/test/parser.test.ts
- packages/2-sql/4-lanes/relational-core/test/fixtures/contract.json
- test/integration/test/authoring/parity/default-dbgenerated/expected.contract.json
- test/integration/test/authoring/parity/default-ulid/expected.contract.json
- packages/3-extensions/sql-orm-client/test/include-cardinality.test-d.ts
- test/integration/test/authoring/parity/pgvector-named-type/expected.contract.json
- packages/2-mongo-family/2-authoring/contract-psl/src/interpreter.ts
- packages/3-extensions/sql-orm-client/test/integration/self-relations.test.ts
- test/integration/test/authoring/parity/default-uuid-v4/expected.contract.json
- packages/2-mongo-family/7-runtime/test/fixtures/contract.d.ts
- packages/1-framework/0-foundation/contract/test/validate-domain.test.ts
- packages/3-extensions/sql-orm-client/test/value-object-inputs.test-d.ts
- packages/3-extensions/sql-orm-client/test/integration/runtime-helpers.ts
- packages/3-extensions/sql-orm-client/src/filters.ts
- packages/1-framework/3-tooling/emitter/test/domain-type-generation.test.ts
- packages/1-framework/0-foundation/contract/test/domain-types.test.ts
- packages/2-sql/2-authoring/contract-psl/test/interpreter.test.ts
- packages/1-framework/0-foundation/contract/src/validate-domain.ts
- packages/2-mongo-family/1-foundation/mongo-contract/test/validate-domain.test.ts
- packages/2-sql/3-tooling/emitter/test/emitter-hook.generation.basic.test.ts
🚧 Files skipped from review as they are similar to previous changes (38)
- packages/3-extensions/sql-orm-client/vitest.config.ts
- packages/1-framework/0-foundation/contract/src/validate-contract.ts
- packages/1-framework/0-foundation/contract/src/contract-types.ts
- packages/3-extensions/sql-orm-client/src/model-accessor.ts
- test/integration/test/cli.emit-cli-process.e2e.test.ts
- test/integration/test/mongo/fixtures/contract.ts
- packages/2-sql/4-lanes/relational-core/test/schema.types.test-d.ts
- packages/1-framework/3-tooling/emitter/test/emitter.integration.test.ts
- packages/3-extensions/sql-orm-client/test/query-plan-mutations.test.ts
- packages/2-sql/9-family/test/emit-parameterized.test.ts
- packages/2-mongo-family/1-foundation/mongo-contract/test/validate.test.ts
- packages/3-extensions/sql-orm-client/test/integration/include.test.ts
- packages/2-mongo-family/3-tooling/emitter/test/emitter-hook.structure.test.ts
- packages/1-framework/2-authoring/psl-parser/src/types.ts
- test/integration/test/authoring/parity/default-nanoid/expected.contract.json
- packages/1-framework/3-tooling/emitter/src/generate-contract-dts.ts
- packages/1-framework/3-tooling/emitter/test/emitter.test.ts
- packages/2-sql/1-core/contract/src/validators.ts
- packages/2-sql/2-authoring/contract-psl/test/interpreter.diagnostics.test.ts
- test/integration/test/authoring/parity/default-uuid-v7/expected.contract.json
- packages/2-sql/2-authoring/contract-ts/test/fixtures/contract-with-relations.d.ts
- packages/2-sql/1-core/contract/src/factories.ts
- packages/2-mongo-family/1-foundation/mongo-contract/test/contract-types.test-d.ts
- packages/1-framework/0-foundation/contract/test/contract-types.test-d.ts
- packages/2-mongo-family/2-authoring/contract-psl/test/interpreter.test.ts
- packages/3-extensions/sql-orm-client/test/generated-contract-types.test-d.ts
- packages/2-sql/2-authoring/contract-ts/src/semantic-contract.ts
- packages/2-sql/2-authoring/contract-ts/test/fixtures/contract.d.ts
- packages/2-sql/2-authoring/contract-psl/test/provider.test.ts
- test/integration/test/authoring/parity/core-surface/expected.contract.json
- packages/2-mongo-family/3-tooling/emitter/test/emitter-hook.generation.test.ts
- packages/2-mongo-family/3-tooling/emitter/test/emitter-hook.types.test.ts
- packages/2-mongo-family/1-foundation/mongo-contract/src/contract-schema.ts
- test/integration/test/value-objects/value-objects.e2e.test.ts
- packages/2-sql/1-core/contract/src/validate.ts
- examples/prisma-next-demo/src/prisma/contract.json
- packages/2-sql/1-core/contract/test/domain-types.test.ts
- packages/3-extensions/sql-orm-client/test/fixtures/contract.ts
- Carry default/executionDefault on SqlSemanticValueObjectFieldNode so JSONB-backed value-object fields can preserve column defaults - Support nested value-object references in SqlSemanticValueObjectNode fields and emit kind:valueObject for nested VO references - Add test for nested value-object authoring (Company.address -> Address containing GeoLocation) - Return unknown for union-kind fields in ResolveModelFieldToJsType instead of leaking the raw contract descriptor
Contributor
Author
|
Thanks for the thorough review! I verified each finding against the code. Here is the disposition: P1 — CriticalP1-1:
|
| # | Finding | Verdict | Action |
|---|---|---|---|
| P2-1 | No cycle detection for non-instantiable VO refs | ✅ Agree | Known gap — listed as an open question in the design doc. Will add validation rejecting cycles where no edge is nullable: true or many: true. |
| P2-2 | Unsanitized VO names in generated TypeScript | ✅ Agree | Will add identifier validation (/^[A-Za-z_]\w*$/) for VO names in validateContractDomain. |
| P2-3 | Mongo InferFieldType missing dict |
✅ Agree | Confirmed — only many and nullable are handled. Will add dict: true → Record<string, T> branch mirroring SQL's ApplyFieldModifiers. |
| P2-4 | No structural validation of valueObjects |
✅ Agree | Currently Record<string, unknown> in all three schemas. Will tighten to validate VO entry structure (must have fields record). |
| P2-5 | Four JSON fixtures use old flat-field format | ✅ Agree | Will update to the new tagged format to match their companion .d.ts files. |
| P2-6 | isNonScalarField uses runtime duck-typing |
✅ Agree | Can replace with field !== undefined && field.type.kind !== 'scalar' since call sites already have typed ContractField. |
| P2-7 | ADR 178 JSON examples outdated | ✅ Agree | Will update ADR 178 examples to reflect the implemented discriminated union format. |
| P2-8 | ReadonlyMap → Map casts in buildValueObjects |
✅ Agree | Minor — will change resolveColumnDescriptor to accept ReadonlyMap. |
| P2-9 | Missing type-level test for self-referencing VOs | ✅ Agree | Will add a .test-d.ts verifying InferModelRow correctly resolves NavItem { children: NavItem[] }. |
| P2-10 | generateValueObjectType unused params |
✅ Agree | Will remove _voName and _valueObjects. |
P3 — Nice-to-Have
All acknowledged. P3-1 (UnionFieldType speculative) is intentional — the type infrastructure from ADR 179 is in place but no authoring surface produces unions yet. P3-5 (dict has no PSL surface) is also intentional — dict is a contract-level concept with no PSL syntax decision yet. Both are follow-ups.
Plan gaps (M6.2, M6.4, M6.5)
M6.2 (Mongo dot-path filtering) and M6.4 (SQL dot-path JSONB filtering) are intentionally deferred. The ORM plumbing for value object reads (inlined in results, correct types) is in this PR. Dot-path filtering — u("homeAddress.city").eq("NYC") compiling to native dot notation or JSONB operators — requires the callable string accessor from ADR 180, which is a separate project that builds on this foundation.
M6.5 (.select() support) works for value object fields as a whole — they are included/excluded like any other field. No special handling was needed since value objects are just fields from .select() perspective.
Summary
- P1-1: Fix (unify generators)
- P1-2: No fix needed (already works), but adding a nested VO test for the TS builder
- P2-1 through P2-10: All accepted, will fix in this PR
- P3: Acknowledged, follow-ups
- M6.2/M6.4: Intentionally deferred to dot-path accessor project
- M6.5: Already works
Add an Address value object (street, city, zip?, country) to both demo applications so the feature is prominently visible end-to-end: SQL demo: PSL type block, TS builder JSONB column, seed data with addresses, new create-user-with-address CLI command, integration test. Mongo demo: PSL type block, seed data with addresses (including null for nullable showcase), CRUD lifecycle and blog integration tests.
wmadden
deleted the
tml-2206-value-objects-embedded-documents-in-both-families-adr-178
branch
This was referenced Apr 7, 2026
4 tasks
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.
closes TML-2206
Walkthrough — Value Objects & Embedded Documents
Before / After (contract field shape)
Intent
Make value objects (structured data without identity — an Address, a GeoPoint, a Money value) a first-class concept in the contract and ORM, from PSL authoring all the way through to typed query results in both SQL and Mongo families. This requires changing how every contract field describes its type, since the old flat
{ codecId, nullable }shape can only represent scalars.Change map
valueObjectsonContractThe story
Replace the flat field type with a tagged discriminated union. The old
ContractFieldwas{ codecId, nullable }— every field was implicitly a scalar. The new shape carries atypeproperty with akinddiscriminant (scalar,valueObject,union), plus orthogonalmanyanddictmodifiers. This is a structural change that prevents conflicting type specifiers at the data level. Every consumer offield.codecIdacross ~100 files migrates tofield.type.codecIdafter narrowing onkind.Add value object definitions to the contract. A new
valueObjectssection onContractholds named value object types. Each value object has the same field shape as a model but carries no identity, lifecycle, or relations. Domain validation ensures referential integrity (all value object references resolve) and rejectsmany+dicton the same field.Teach the PSL parser and interpreters about
typeblocks. The parser addsPslCompositeTypenodes fortype X { ... }blocks, parallel to models and enums. Both the SQL and Mongo PSL interpreters consume these nodes: emitting value object definitions into the contract, resolving model field references to value object types, and (SQL only) creating JSONB storage columns.Extend TS authoring and contract emission. The SQL semantic contract builder gains
SqlSemanticValueObjectFieldNodeandSqlSemanticValueObjectNode. Both SQL and Mongo emitter hooks generate value object type aliases incontract.d.tsand include a typedvalueObjectsdescriptor in the contract type. Shared helpers indomain-type-generation.tshandle field-to-TypeScript resolution includingmany,dict,nullable, and self-referencing value objects.Wire value object fields into ORM row type inference. Mongo's
InferModelRowand SQL's collection-contract types recursively expand value object field types into nested object types, respectingmanyandnullablemodifiers. Type-level tests verify the inferred shapes match expectations.Prove the full stack with integration and e2e tests. Pre-emitted contract fixtures exercise the PSL → interpret → emit → validate → ORM CRUD path for both families, including tests against real MongoDB and Postgres databases.
Behavior changes & evidence
Contract field type system is now a tagged discriminated union:
{ codecId, nullable }→{ type: { kind: 'scalar', codecId: '...' }, nullable, many?, dict? }. Every field in every contract, fixture, and test is updated.Adds
valueObjectssection toContract: Contracts can now carry named value object definitions, placed betweenmodelsandstoragein canonicalized output.valueObjectsordering testsAdds contract-level validation for value object references and field modifiers: Domain validation rejects dangling VO references and
many+dicton the same field.PSL
type X { ... }blocks produce value objects in both families: Scalar list fields (String[]) now producemany: trueinstead of errors. Value object field references produce{ kind: 'valueObject', name: '...' }descriptors.typekeyword for composite types. Enables authoring value objects in the schema language.Scalar list fields (
String[]) now emitmany: trueinstead of producing diagnostics: Before, the SQL and Mongo interpreters rejected scalar lists. Now they emit{ kind: 'scalar', codecId: '...', many: true }with SQL mapping to JSONB columns.manymodifier is orthogonal to the type specifier.resolveNonRelationFieldemits scalar list fields with many: trueemits many: true for scalar list fieldsAdds value object type aliases and descriptors to emitted
contract.d.ts: Both SQL and Mongo emitters generateexport type Address = { ... }and include typedvalueObjectsin the contract type.ORM row type inference expands value object fields to nested types: Both
InferModelRow(Mongo) and SQL's collection types recursively expand{ kind: 'valueObject', name: 'Address' }into the corresponding nested object type.user.homeAddress.citymust be typed asstring.InferFieldType,InferFieldBaseTypeContractFieldTypeonModelEntryInferModelRow expands value object fieldsCompatibility / migration / risk
ContractFieldshape: Every consumer readingfield.codecIdmust change tofield.type.codecId. This is a mechanical migration but touches ~110 files. All existing tests and fixtures are updated on this branch.valueObjectsis optional — existing contracts without it remain valid.PSL_UNSUPPORTED_FIELD_LISTis removed in favor ofPSL_UNSUPPORTED_FIELD_TYPE(scalar list fields are now supported withmany: true).contract.d.ts/contract.jsonfiles. If the emitter output format changes again, these fixtures must be regenerated. This is a standard risk for pre-emitted test fixtures.Follow-ups / open questions
u("homeAddress.city").eq("NYC")and SQL JSONB path operators are future work.value-object-inputs.test-d.ts) but runtime write integration tests for value objects are limited to e2e tests..select()within value objects: Explicitly deferred — value objects are included/excluded as a whole.dictmodifier: Defined and validated but not exercised in authoring (no PSL syntax fordictfields yet).projects/orm-consolidation/specs/embedded-documents-and-value-objects.spec.mdis referenced but absent.main.Non-goals / intentionally out of scope
Summary by CodeRabbit
typedeclarations for defining reusable object structures.