feat(sql): add per-codec temporal presets with execution-default arguments (TML-3036)#1003
Open
wmadden-electric wants to merge 17 commits into
Open
feat(sql): add per-codec temporal presets with execution-default arguments (TML-3036)#1003wmadden-electric wants to merge 17 commits into
wmadden-electric wants to merge 17 commits into
Conversation
…sion to authoring framework Dispatch 1 (framework layer) of the per-codec temporal presets slice: - AuthoringArgumentDescriptor gains an `option` kind (values: readonly string[]) - validateAuthoringArgument enforces option membership - AuthoringArgRef gains `map` for declarative token -> template lookup, with `default` bypassing `map` - executionDefaults phase templates that resolve to undefined now omit the phase (and executionDefaults itself when both phases omit) instead of throwing - resolveAuthoringStorageTypeTemplate normalizes an empty resolved typeParams to omitted No preset changes; temporalAuthoringPresets and existing descriptors are unaffected (regression-tested). Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
Adds the `option` case to parsePslAuthoringArgumentValue: a bare identifier token parses to its string value; anything else (quoted string, number, object) is rejected. Membership validation stays with validateAuthoringArgument (framework layer) — this is syntax only. Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
…ptors optional tuple slots
ArgTypeFromDescriptor gains an option branch typing to the literal union
of the descriptors values. TupleFromArgumentDescriptors is rewritten
recursively so a descriptor marked optional: true gets an optional tuple
slot instead of a required one, closing the gap where a helper with only
optional args (e.g. nanoid) could not be called with zero arguments even
though the runtime already accepted omission.
Known conflict (see PR/dispatch notes): this recursive rewrite makes the
sole-optional-arg case ambiguous under the FieldHelperFunctionWithNamedConstraint
overload (unchanged, per design-spec 4.2) - `field.id.nanoid({ size })`
with no trailing name option now infers the object as the named-constraint
options argument instead of the preset arg, breaking one pre-existing
call site in contract-builder.dsl.helpers.test.ts. The two-argument form
(`field.id.nanoid({ size }, { name })`) is unaffected and is covered by a
new regression test.
Signed-off-by: willbot <w.a.madden+machine@gmail.com>
Signed-off-by: Will Madden <madden@prisma.io>
…ad pair
Replaces the single rest-tuple signature (trailing `options?`) with an
intersection of two call signatures per design-spec 4.3: a no-options
signature first, then an options-required signature.
Once optional descriptors yield optional tuple slots, Params can infer as
the empty tuple, so the old single signature bound a lone preset argument
to the trailing optional options parameter — breaking the pre-existing
public call `field.id.nanoid({ size: 16 })`. That call site now compiles
unmodified, which is the test of this fix.
Known gap (spec 4.3 row 3, reported to the spec owner):
`field.id.nanoid({ name: "x" })` still resolves via signature 1 and yields
no constraint name. The spec assumed signature 1 would reject `{name}`,
but ObjectArgumentType builds `{} & { size?: number }`, and the empty-object
constituent defeats weak-type rejection, so `{name}` is assignable to the
size-arg object. That spelling was a compile error before the optional-slot
change, so it is not a regression against working code, but it now fails
silently instead of loudly. Pinned by a test that documents actual behavior.
Signed-off-by: willbot <w.a.madden+machine@gmail.com>
Signed-off-by: Will Madden <madden@prisma.io>
When every declared property is optional, ObjectArgumentType built
`{} & { size?: number }`. The `{}` constituent is not itself a weak type,
so weak-type detection never fired for the intersection and a foreign-keyed
object was assignable to it. Emit the optional-only mapped type directly in
that case, keeping the intersection only when required keys exist.
This closes a silent-wrong-result hole introduced by this slice. The TS
authoring surface runs no runtime argument validation by design
(buildFieldPreset calls instantiateAuthoringFieldPreset directly), so the
type system is the only check: combined with optional tuple slots,
`field.id.nanoid({ name: "pk" })` compiled and silently produced a
length-21 nanoid with an UNNAMED primary key -- wrong DDL, no diagnostic at
any layer. On baseline f44cbcb that call was a compile error, so this is a
loud error that this slice had turned into a silent wrong result.
With the plain weak type, `{ name: "x" }` is no longer assignable to the
size argument, signature 1 falls through, and design-spec 4.3 row 3 resolves
via signature 2 with constraint name "x" -- delivering row 3 in the same
change. Verified: weak-type detection does fire.
Signed-off-by: willbot <w.a.madden+machine@gmail.com>
Signed-off-by: Will Madden <madden@prisma.io>
Adds temporalCodecPresetWithPrecision (for codecs taking a precision param: pg/timestamp@1, pg/timestamptz@1) and temporalCodecPreset (for codecs without params: sqlite/datetime@1), exported from family-sql/control alongside the untouched temporalAuthoringPresets. One field preset per codec; arguments change field properties, never the codec. All args are optional: omitting precision omits typeParams entirely, and omitting a phase omits that phase (both omitted omits executionDefaults). The `now` token is mapped declaratively to the timestampNow generator id via the arg-ref map, so the generator id never appears in a user spelling (ADR 169). Literal types are preserved end-to-end (const type params + as const satisfies); a type test pins the codec id, native type, option value union, and arg-ref indices against silent widening. Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
…poral.datetime on sqlite The existing `temporal:` entry becomes a spread of temporalAuthoringPresets plus the new per-codec siblings, so createdAt/updatedAt survive unchanged alongside them. Preset names follow the codec base name: pg/timestamp@1 -> timestamp, pg/timestamptz@1 -> timestamptz, sqlite/datetime@1 -> datetime. Sqlite uses the no-precision factory because sqlite/datetime@1 declares voidParamsSchema, while both pg timestamp codecs declare precisionParamsSchema. Each registration is asserted against the factory output, so the target packs cannot drift from family-sql. Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
…ree-way parity Interpreter tests assert every row of design-spec section 8 as a whole column shape and whole execution-defaults list (not spot fields), including that temporal.timestamp() omits the typeParams key entirely and that timestamp(3) carries no executionDefaults key. Diagnostics cover every row of the section 5 table plus the free positional-then-named collision case timestamp(3, precision: 4); all assert code PSL_INVALID_ATTRIBUTE_ARGUMENT and a message substring. Parity covers the three-way byte-identity between PSL temporal.updatedAt(), PSL temporal.timestamptz(onCreate: now, onUpdate: now), and the TS equivalent, plus named-args ≡ positional spelling and the sqlite temporal.datetime case. The contract-psl and contract-ts fixtures mirror the family-sql factories by hand because neither package depends on family-sql. The mirrors are anchored on both sides: the factory shapes are asserted in family-sql, and each target registration is asserted against the factory output. Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
Review finding F1. contract-psl and contract-ts hand-mirror the temporal preset factories because neither can import family-sql (family-sql depends on contract-ts in production and contract-psl in dev, so the reverse is a cycle). Those mirrors were pinned by nothing. The target-pack registration tests could not serve as the anchor: both sides of `expect(registration).toEqual(factory(...))` derive from the factory, so they are invariant under factory changes. A new arg or a renamed map key would go red only in family-sql`s literal test; once that literal was updated, the mirrors would stay stale and the whole section 8 output table plus all three parity assertions would keep passing against a preset that no longer ships -- suite green, coverage silently gone. Invert the direction instead, which the dependency graph already allows: consolidate the mirrors into contract-psl/test/fixtures.ts and a new contract-ts/test/temporal-preset-mirror.ts, and assert in family-sql that each deep-equals the factory output. The factory is generic over codecId/nativeType, so contract-ts`s portable sql/timestamp@1 mirror anchors the same way, letting one family-sql file anchor all four mirrors. Verified non-vacuous: renaming the map key now -> current fails all four anchors; adding a timezone arg to the precision factory fails the three that use it (the sqlite mirror correctly stays green). Also corrects the comments that claimed the mirrors were "anchored on both sides" to name the assertion that actually anchors them, and pins the section 2 spelling field.temporal.timestamp(3) -- precision present, execution defaults absent -- which was advertised but untested on the TS surface. Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
Review finding F2 — F1 named a class of defect and the first fix swept only
the instances that dispatch added. This sweeps the rest of the class in the
contract-psl fixtures.
The inline temporal.createdAt/updatedAt entries in both contract-psl fixture
packs mirror temporalAuthoringPresets(...) and were pinned by nothing. That
matters more than the per-codec mirrors did: the slice states three-way
byte-identity as a done-condition, and both buildTsConvenienceForm() and
interpretTemporalPsl("temporal.updatedAt()") route through these mirrors
while the timestamptz side is anchored. A field added to the real
updatedAt output would leave all three parity assertions green while proving
a fiction of updatedAt byte-identical to the real timestamptz — the shipped
shorthand guarantee broken, and the test written to guarantee it silent.
Consolidates them into temporalConvenienceMirrors in fixtures.ts, consumed
by both packs via spread, and anchors both against temporalAuthoringPresets
in family-sql alongside the existing per-codec anchors.
Verified non-vacuous twice: changing createdAt now() -> CURRENT_TIMESTAMP
fails both new anchors, and adding nullable:false to updatedAt output fails
both.
Also documents that contract-ts test temporal.createdAt/updatedAt are
independent portable fixtures (sql/timestamp@1 + CURRENT_TIMESTAMP), NOT
mirrors of temporalAuthoringPresets (which hardcodes now()), so they cannot
and should not be anchored.
Signed-off-by: willbot <w.a.madden+machine@gmail.com>
Signed-off-by: Will Madden <madden@prisma.io>
Scope note: this is not temporal preset work. It closes the last instance of
the mirror-drift class F1/F2 named, in the one place where this slice`s own
fix depends on the mirrored value.
contract-ts hand-mirrors sqlFamilyAuthoringFieldPresets.{nanoid, id.nanoid}
in three test files and cannot import family-sql (family-sql depends on
contract-ts in production, so the reverse is a cycle). Those mirrors were
pinned by nothing.
They became load-bearing in dispatch 1. Section 4.4 -- our fix for the
silent-wrong-DDL hole this slice opened -- rests entirely on nanoid`s options
object being all-optional: that is what makes ObjectArgumentType emit a plain
weak type, and weak-type detection is the only check on the TS surface, which
runs no runtime argument validation. If size became required upstream,
ObjectArgumentType would take its intersection branch, the protection would
evaporate, section 4.3 row 3 would change behavior -- and every test written
for that fix would stay green against the stale mirror.
Consolidates the three copies into test/nanoid-preset-mirror.ts and anchors
them in family-sql/test/authoring-field-presets.test.ts (the home of
sqlFamilyAuthoringFieldPresets; the temporal file stays temporal-scoped). The
args[0] assertion is called out as the one carrying the weight.
Verified non-vacuous with the exact perturbation the anchor exists for:
dropping optional:true from the shipping size property fails all three
anchors.
Signed-off-by: willbot <w.a.madden+machine@gmail.com>
Signed-off-by: Will Madden <madden@prisma.io>
…ments no-transient-project-ids-in-code (always-apply) forbids prose attributions to plan/spec documents in source, comments, and test names. /wip/* is gitignored, so every "design-spec §N" reference these tests carried pointed at a document that never lands -- the code would start lying the moment the spec was rewritten or deleted. Describes the property each test pins instead: which keys must be absent from the lowered column, that the option kind adds no diagnostic code of its own, that updatedAt and timestamptz are separately authored descriptors held equal only by the assertions, and so on. No behavior change; the durable home for the reasoning is ADR 234. Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
…uthoring helpers Records the four decisions behind the per-codec temporal presets: the option argument kind (bare token in PSL following @relation(onDelete: Cascade), literal union in TS, mirroring the existing PslBlockParamOption vocabulary); the map node on argument refs, which keeps preset vocabulary and generator ids as separate vocabularies related by data (ADR 169 keeps timestampNow preset-only); and the two omission rules. Carries the findings from the build loop that a future descriptor author cannot infer from the code: - the naming rule (per-codec preset name = codec base name) leads, since it is what lets the per-codec presets share the temporal namespace - which check protects which surface: PSL validates option membership at runtime; TS relies on the literal union at compile time and has no runtime argument validation at all, leaving the map throw as the only backstop for type-bypassing callers - which check protects which argument object: all-optional gets weak-type protection, required-bearing relies on the excess-property check - the two omission rules carry each other — updatedAt() equals timestamptz(now, now) only because empty-typeParams normalization makes the unfilled {precision} template vanish - parity tests prove the spellings agree, not that the output is correct; the absolute output assertions do that job - the shorthand is test-enforced, not structural: two separately authored descriptors sharing no code, held equal only by named tests Every snippet, symbol, path, and test name re-verified against landed code rather than copied from the spec. Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
adr-writing asks that consequences not be narrated as project events. Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
Contributor
📝 WalkthroughWalkthroughAdds ChangesAuthoring contracts and temporal preset lowering
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant AuthoringHelper
participant FrameworkAuthoring
participant TemporalPreset
participant SQLContract
AuthoringHelper->>FrameworkAuthoring: resolve option and select templates
FrameworkAuthoring->>TemporalPreset: resolve timestamp generator mapping
TemporalPreset->>SQLContract: emit codec, typeParams, and execution defaults
FrameworkAuthoring->>SQLContract: omit undefined phases and empty typeParams
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
@prisma-next/extension-author-tools
@prisma-next/mongo-runtime
@prisma-next/family-mongo
@prisma-next/sql-runtime
@prisma-next/family-sql
@prisma-next/extension-arktype-json
@prisma-next/middleware-cache
@prisma-next/mongo
@prisma-next/extension-paradedb
@prisma-next/extension-pgvector
@prisma-next/extension-postgis
@prisma-next/postgres
@prisma-next/sql-orm-client
@prisma-next/sqlite
@prisma-next/extension-supabase
@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/ts-render
@prisma-next/contract-authoring
@prisma-next/ids
@prisma-next/psl-parser
@prisma-next/psl-printer
@prisma-next/cli
@prisma-next/cli-telemetry
@prisma-next/config-loader
@prisma-next/emitter
@prisma-next/language-server
@prisma-next/migration-tools
prisma-next
@prisma-next/vite-plugin-contract-emit
@prisma-next/mongo-codec
@prisma-next/mongo-contract
@prisma-next/mongo-value
@prisma-next/mongo-contract-psl
@prisma-next/mongo-contract-ts
@prisma-next/mongo-emitter
@prisma-next/mongo-schema-ir
@prisma-next/mongo-query-ast
@prisma-next/mongo-orm
@prisma-next/mongo-query-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/target-sqlite
@prisma-next/adapter-postgres
@prisma-next/adapter-sqlite
@prisma-next/driver-postgres
@prisma-next/driver-sqlite
commit: |
size-limit report 📦
|
![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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/2-sql/9-family/src/core/timestamp-now-generator.ts`:
- Around line 77-83: Update TEMPORAL_PRECISION_ARG to enforce a maximum
precision of 6, rejecting values above 6 at the authoring boundary. Add coverage
for out-of-range precision values supplied both positionally and by the named
argument.
🪄 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: 738dacf1-d7ac-466e-bbc0-6ae93a82b129
📒 Files selected for processing (24)
docs/architecture docs/ADR-INDEX.mddocs/architecture docs/adrs/ADR 234 - Option arguments and value-mapped templates for authoring helpers.mdpackages/1-framework/1-core/framework-components/src/shared/framework-authoring.tspackages/1-framework/1-core/framework-components/test/framework-components.authoring.test.tspackages/2-sql/2-authoring/contract-psl/src/psl-authoring-arguments.tspackages/2-sql/2-authoring/contract-psl/test/fixtures.tspackages/2-sql/2-authoring/contract-psl/test/interpreter.defaults.test.tspackages/2-sql/2-authoring/contract-psl/test/psl-authoring-arguments.test.tspackages/2-sql/2-authoring/contract-psl/test/ts-psl-parity.test.tspackages/2-sql/2-authoring/contract-ts/src/authoring-type-utils.tspackages/2-sql/2-authoring/contract-ts/test/authoring-helper-runtime.test.tspackages/2-sql/2-authoring/contract-ts/test/authoring-type-utils.test-d.tspackages/2-sql/2-authoring/contract-ts/test/contract-builder.dsl.helpers.test.tspackages/2-sql/2-authoring/contract-ts/test/nanoid-preset-mirror.tspackages/2-sql/2-authoring/contract-ts/test/temporal-preset-mirror.tspackages/2-sql/9-family/src/core/timestamp-now-generator.tspackages/2-sql/9-family/src/exports/control.tspackages/2-sql/9-family/test/authoring-field-presets.test.tspackages/2-sql/9-family/test/temporal-codec-presets.test-d.tspackages/2-sql/9-family/test/temporal-codec-presets.test.tspackages/3-targets/3-targets/postgres/src/core/authoring.tspackages/3-targets/3-targets/postgres/test/authoring-field-presets.test.tspackages/3-targets/3-targets/sqlite/src/core/authoring.tspackages/3-targets/3-targets/sqlite/test/authoring-field-presets.test.ts
Comment on lines
+77
to
+83
| const TEMPORAL_PRECISION_ARG = { | ||
| name: 'precision', | ||
| kind: 'number', | ||
| optional: true, | ||
| integer: true, | ||
| minimum: 0, | ||
| } as const; |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target file around the reported lines.
sed -n '1,240p' packages/2-sql/9-family/src/core/timestamp-now-generator.ts
echo '--- SEARCH: precision validation / temporal.timestamp ---'
rg -n "TEMPORAL_PRECISION_ARG|temporal\.timestamp|precision.*maximum|maxim|timestamp.*precision|typeParams" packages/2-sql/9-family/src -g '*.ts' -g '*.tsx'
echo '--- SEARCH: PostgreSQL timestamp precision handling elsewhere ---'
rg -n "precision.*6|maximum:\s*6|timestamp precision|datetime" packages -g '*.ts' -g '*.tsx'Repository: prisma/prisma-next
Length of output: 24966
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- relational-core sql-codecs.ts ---'
sed -n '1,140p' packages/2-sql/4-lanes/relational-core/src/ast/sql-codecs.ts
echo '--- postgres codec descriptor ---'
sed -n '1,180p' packages/3-targets/3-targets/postgres/src/core/codecs.ts
echo '--- temporal codec preset tests ---'
sed -n '1,260p' packages/2-sql/9-family/test/temporal-codec-presets.test.ts
echo '--- temporal DSL helper test around temporal.timestamp ---'
sed -n '740,840p' packages/2-sql/2-authoring/contract-ts/test/contract-builder.dsl.helpers.test.tsRepository: prisma/prisma-next
Length of output: 22194
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- search preset descriptor schema consumption ---'
rg -n "AuthoringFieldPresetDescriptor|args: \[TEMPORAL_PRECISION_ARG|kind: 'fieldPreset'|validate.*args|paramsSchema|StandardSchemaV1" packages -g '*.ts' -g '*.tsx'
echo '--- inspect authoring-field-presets.ts ---'
sed -n '1,260p' packages/2-sql/9-family/src/core/authoring-field-presets.ts
echo '--- inspect contract-ts DSL helper around temporal.timestamp implementation ---'
rg -n "temporal\.timestamp|temporalCodecPresetWithPrecision|timestamp-now-generator" packages/2-sql/2-authoring/contract-ts/src packages/2-sql/2-authoring/contract-ts/test -g '*.ts' -g '*.tsx'Repository: prisma/prisma-next
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect authoring argument validation for number descriptors.
sed -n '940,1165p' packages/1-framework/1-core/framework-components/src/shared/framework-authoring.ts
echo '--- runtime helper path ---'
sed -n '1,180p' packages/2-sql/2-authoring/contract-ts/src/authoring-helper-runtime.ts
echo '--- authoring type utils around number args ---'
rg -n "minimum|maximum|integer|kind: 'number'|validateAuthoringArgument" packages/2-sql/2-authoring/contract-ts/src packages/1-framework/1-core/framework-components/src/shared/framework-authoring.ts -g '*.ts'Repository: prisma/prisma-next
Length of output: 15890
Reject PostgreSQL precision values above 6
TEMPORAL_PRECISION_ARG still accepts temporal.timestamp(7) at the authoring boundary; add maximum: 6 and cover both positional and named out-of-range arguments.
Proposed fix
const TEMPORAL_PRECISION_ARG = {
name: 'precision',
kind: 'number',
optional: true,
integer: true,
minimum: 0,
+ maximum: 6,
} as const;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const TEMPORAL_PRECISION_ARG = { | |
| name: 'precision', | |
| kind: 'number', | |
| optional: true, | |
| integer: true, | |
| minimum: 0, | |
| } as const; | |
| const TEMPORAL_PRECISION_ARG = { | |
| name: 'precision', | |
| kind: 'number', | |
| optional: true, | |
| integer: true, | |
| minimum: 0, | |
| maximum: 6, | |
| } as const; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/2-sql/9-family/src/core/timestamp-now-generator.ts` around lines 77
- 83, Update TEMPORAL_PRECISION_ARG to enforce a maximum precision of 6,
rejecting values above 6 at the authoring boundary. Add coverage for
out-of-range precision values supplied both positionally and by the named
argument.
Ground the example in what the spellings do (runtime-written execution defaults) and show a real option declaration; move the enforcement table after map so its third row is no longer a forward reference; promote the TS-surface typing story (optional slots, overload pair, weak-type mechanism) out of Consequences; replace change-narration ("gains", "untouched", "rather than throwing") with statements about the system as it is; add the structural-derivation alternative for the convenience presets.
Signed-off-by: willbot <w.a.madden+machine@gmail.com>
Signed-off-by: Will Madden <madden@prisma.io>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
docs/architecture docs/adrs/ADR 234 - Option arguments and value-mapped templates for authoring helpers.md (1)
120-127: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftKeep the ADR focused on architectural decisions.
These sections include resolver step-by-step behavior, normalization code, test mechanics, and TypeScript implementation details. Move those implementation recipes to subsystem/package documentation, retaining only the architectural constraints and rationale here to reduce future drift.
Based on learnings, ADRs should document architectural decisions and key constraints, not implementation algorithms or emitter derivation logic.
Also applies to: 147-160, 193-204, 208-234
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/architecture` docs/adrs/ADR 234 - Option arguments and value-mapped templates for authoring helpers.md around lines 120 - 127, Refocus the ADR on the architectural decision and rationale by removing resolver step-by-step behavior, normalization and test mechanics, and TypeScript/emitter implementation details from the referenced sections. Retain only the constraints that defaults are output-space values, mapped references are excluded from builder-state positions, and temporal presets use mapping only in execution defaults; move implementation recipes to the relevant subsystem or package documentation.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@docs/architecture` docs/adrs/ADR 234 - Option arguments and value-mapped
templates for authoring helpers.md:
- Around line 120-127: Refocus the ADR on the architectural decision and
rationale by removing resolver step-by-step behavior, normalization and test
mechanics, and TypeScript/emitter implementation details from the referenced
sections. Retain only the constraints that defaults are output-space values,
mapped references are excluded from builder-state positions, and temporal
presets use mapping only in execution defaults; move implementation recipes to
the relevant subsystem or package documentation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: e77a1230-8444-4c00-bad6-ef7f305a1744
📒 Files selected for processing (1)
docs/architecture docs/adrs/ADR 234 - Option arguments and value-mapped templates for authoring helpers.md
…tion type
A map-bearing arg ref was not a reference: it inserted a value selected by
the argument rather than the argument itself, one kind covering two
operations. And the legal-token enumeration lived twice (the option
argument's values and the map keys) with nothing coupling them before a
runtime throw.
Selection is now its own template node: {kind: 'select', index, path?,
cases}. Arg refs go back to pure substitution. Case coverage is validated
against the referenced option argument at pack-registration time
(set equality, from assertNoCrossRegistryCollisions, so both composition
seams inherit it). The option concept is one shared AuthoringOption type
used by both the block-parameter and helper-argument vocabularies.
No emitted contract changes; fixtures:check clean. Anchor non-vacuity:
reverting one downstream mirror to the old shape fails the family-sql
anchor test.
Signed-off-by: willbot <w.a.madden+machine@gmail.com>
Signed-off-by: Will Madden <madden@prisma.io>
Selection is a first-class template node rather than a map property on the argument reference; the option concept is one shared type across block parameters and helper arguments; case coverage is a registration-time check. The rejected map-on-ref shape moves to alternatives considered. Renamed to match: value-mapped templates -> select templates. Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
docs/architecture docs/adrs/ADR 234 - Option arguments and select templates for authoring helpers.md (1)
130-132: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove the resolution algorithm out of the ADR.
Keep the semantic guarantees here, but move the step-by-step lookup algorithm and
assertNoCrossRegistryCollisionsimplementation seam to framework documentation.Based on learnings, ADRs should document architectural decisions and constraints, not implementation algorithms or step-by-step resolution rules.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/architecture` docs/adrs/ADR 234 - Option arguments and select templates for authoring helpers.md around lines 130 - 132, Move the step-by-step select-node lookup and recursive resolution procedure from this ADR to the relevant framework documentation. Retain only the semantic guarantees and architectural constraints, including undefined omission, valid case-key requirements, and pack-registration coverage validation; move the assertNoCrossRegistryCollisions implementation seam there as well.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@packages/1-framework/1-core/framework-components/src/shared/framework-authoring.ts`:
- Around line 896-929: The select-reference validation flow around
collectSelectRefs and validateSelectRefsAgainstArgs must retain each node’s
template path and reject select nodes located under codecId, nullable, id, or
unique, as prohibited by ADR 234 and unsupported by ResolveTemplateValue. Update
registration validation to report these forbidden placements, and add
registration coverage for each field while preserving valid select placements.
- Around line 417-449: Replace the production `as` casts in
`isAuthoringSelectRef` and `readTemplateArgumentValue` with the existing
`blindCast` helper, or strengthen the type guards so those casts are
unnecessary. Preserve the current validation and nested argument lookup
behavior.
---
Nitpick comments:
In `@docs/architecture` docs/adrs/ADR 234 - Option arguments and select templates
for authoring helpers.md:
- Around line 130-132: Move the step-by-step select-node lookup and recursive
resolution procedure from this ADR to the relevant framework documentation.
Retain only the semantic guarantees and architectural constraints, including
undefined omission, valid case-key requirements, and pack-registration coverage
validation; move the assertNoCrossRegistryCollisions implementation seam there
as well.
🪄 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: 87fc19d1-a8a1-4dcd-9d06-a001a7a84b68
📒 Files selected for processing (11)
docs/architecture docs/ADR-INDEX.mddocs/architecture docs/adrs/ADR 234 - Option arguments and select templates for authoring helpers.mdpackages/1-framework/1-core/framework-components/src/exports/authoring.tspackages/1-framework/1-core/framework-components/src/shared/framework-authoring.tspackages/1-framework/1-core/framework-components/src/shared/option-descriptor.tspackages/1-framework/1-core/framework-components/src/shared/psl-extension-block.tspackages/1-framework/1-core/framework-components/test/framework-components.authoring.test.tspackages/2-sql/2-authoring/contract-psl/test/fixtures.tspackages/2-sql/2-authoring/contract-ts/test/temporal-preset-mirror.tspackages/2-sql/9-family/src/core/timestamp-now-generator.tspackages/2-sql/9-family/test/temporal-codec-presets.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- docs/architecture docs/ADR-INDEX.md
- packages/2-sql/2-authoring/contract-ts/test/temporal-preset-mirror.ts
- packages/2-sql/9-family/test/temporal-codec-presets.test.ts
- packages/2-sql/9-family/src/core/timestamp-now-generator.ts
Comment on lines
+417
to
+449
| function isAuthoringSelectRef(value: unknown): value is AuthoringSelectRef { | ||
| if ( | ||
| typeof value !== 'object' || | ||
| value === null || | ||
| (value as { kind?: unknown }).kind !== 'select' | ||
| ) { | ||
| return false; | ||
| } | ||
| const { index, path, cases } = value as { index?: unknown; path?: unknown; cases?: unknown }; | ||
| if (typeof index !== 'number' || !Number.isInteger(index) || index < 0) { | ||
| return false; | ||
| } | ||
| if (path !== undefined && (!Array.isArray(path) || path.some((s) => typeof s !== 'string'))) { | ||
| return false; | ||
| } | ||
| return typeof cases === 'object' && cases !== null && !Array.isArray(cases); | ||
| } | ||
|
|
||
| function isAuthoringTemplateRecord(value: unknown): value is Record<string, unknown> { | ||
| return typeof value === 'object' && value !== null && !Array.isArray(value); | ||
| } | ||
|
|
||
| function readTemplateArgumentValue( | ||
| args: readonly unknown[], | ||
| index: number, | ||
| path: readonly string[] | undefined, | ||
| ): unknown { | ||
| let value = args[index]; | ||
| for (const segment of path ?? []) { | ||
| if (!isAuthoringTemplateRecord(value) || !Object.hasOwn(value, segment)) { | ||
| return undefined; | ||
| } | ||
| value = (value as Record<string, unknown>)[segment]; |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C2 "from '`@prisma-next/utils/casts`'" packages/1-framework -g '*.ts'Repository: prisma/prisma-next
Length of output: 19428
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE='packages/1-framework/1-core/framework-components/src/shared/framework-authoring.ts'
echo '--- target file around cited lines ---'
sed -n '400,470p' "$FILE" | cat -n
echo
echo '--- select-related symbols in target file ---'
rg -n -C3 'collectSelectRefs|validateSelectRefsAgainstArgs|isAuthoringSelectRef|codecId|nullable|unique|id' "$FILE"
echo
echo '--- repo-wide search for placement checks on select refs ---'
rg -n -C2 'codecId|nullable|unique|id' packages/1-framework/1-core/framework-components/src/shared packages/1-framework/1-core/framework-components/src/controlRepository: prisma/prisma-next
Length of output: 50377
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE='packages/1-framework/1-core/framework-components/src/shared/framework-authoring.ts'
echo '--- exact cited range ---'
sed -n '417,449p' "$FILE" | cat -n
echo
echo '--- surrounding select helpers ---'
rg -n -C4 'collectSelectRefs|validateSelectRefsAgainstArgs|isAuthoringSelectRef|readTemplateArgumentValue|isAuthoringTemplateRecord' "$FILE"
echo
echo '--- any ADR-234 or placement-enforcement code in the package ---'
rg -n -C3 'ADR 234|placement|codecId|nullable|unique|id' packages/1-framework/1-core/framework-components/src/shared packages/1-framework/1-core/framework-components/src/controlRepository: prisma/prisma-next
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE='packages/1-framework/1-core/framework-components/src/shared/framework-authoring.ts'
echo '--- exact cited range ---'
nl -ba "$FILE" | sed -n '417,449p'
echo
echo '--- select helpers in this file only ---'
rg -n -C3 'collectSelectRefs|validateSelectRefsAgainstArgs|isAuthoringSelectRef|readTemplateArgumentValue|isAuthoringTemplateRecord|codecId|nullable|unique|id' "$FILE"
echo
echo '--- any runtime enforcement of forbidden select positions in this file ---'
rg -n -C2 'Must not be used in the `codecId`/`nullable`/`id`/`unique` positions|ADR 234|forbidden select|placement' "$FILE"Repository: prisma/prisma-next
Length of output: 222
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE='packages/1-framework/1-core/framework-components/src/shared/framework-authoring.ts'
echo '--- exact cited range ---'
sed -n '417,449p' "$FILE" | cat -n
echo
echo '--- select helpers in this file only ---'
rg -n -C3 'collectSelectRefs|validateSelectRefsAgainstArgs|isAuthoringSelectRef|readTemplateArgumentValue|isAuthoringTemplateRecord|codecId|nullable|unique|id' "$FILE"
echo
echo '--- any runtime enforcement of forbidden select positions in this file ---'
rg -n -C2 'Must not be used in the `codecId`/`nullable`/`id`/`unique` positions|ADR 234|forbidden select|placement' "$FILE"Repository: prisma/prisma-next
Length of output: 24438
Replace the remaining bare casts with blindCast
isAuthoringSelectRef and readTemplateArgumentValue still use production as casts. Use the existing cast helper or tighten the guard so the extra cast disappears.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@packages/1-framework/1-core/framework-components/src/shared/framework-authoring.ts`
around lines 417 - 449, Replace the production `as` casts in
`isAuthoringSelectRef` and `readTemplateArgumentValue` with the existing
`blindCast` helper, or strengthen the type guards so those casts are
unnecessary. Preserve the current validation and nested argument lookup
behavior.
Source: Coding guidelines
Comment on lines
+896
to
+929
| function collectSelectRefs(value: unknown, found: AuthoringSelectRef[]): void { | ||
| if (typeof value !== 'object' || value === null) { | ||
| return; | ||
| } | ||
| if (isAuthoringSelectRef(value)) { | ||
| found.push(value); | ||
| for (const caseTemplate of Object.values(value.cases)) { | ||
| collectSelectRefs(caseTemplate, found); | ||
| } | ||
| return; | ||
| } | ||
| if (isAuthoringArgRef(value)) { | ||
| collectSelectRefs(value.default, found); | ||
| return; | ||
| } | ||
| if (Array.isArray(value)) { | ||
| for (const entry of value) { | ||
| collectSelectRefs(entry, found); | ||
| } | ||
| return; | ||
| } | ||
| for (const entry of Object.values(value)) { | ||
| collectSelectRefs(entry, found); | ||
| } | ||
| } | ||
|
|
||
| function validateSelectRefsAgainstArgs( | ||
| label: string, | ||
| helperPath: string, | ||
| args: readonly AuthoringArgumentDescriptor[] | undefined, | ||
| templateRoot: unknown, | ||
| ): void { | ||
| const selects: AuthoringSelectRef[] = []; | ||
| collectSelectRefs(templateRoot, selects); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Enforce the documented select-placement constraint at registration.
The collector discards each node’s output path, so validation accepts select under codecId, nullable, id, or unique. Those positions are explicitly unsupported by ResolveTemplateValue, allowing a registered descriptor to break builder-state inference.
Collect the template path and reject these positions, with registration tests for each forbidden field.
Based on learnings, ADR-declared behavior is authoritative; ADR 234 explicitly prohibits select nodes in these output positions.
Also applies to: 976-1005
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@packages/1-framework/1-core/framework-components/src/shared/framework-authoring.ts`
around lines 896 - 929, The select-reference validation flow around
collectSelectRefs and validateSelectRefsAgainstArgs must retain each node’s
template path and reject select nodes located under codecId, nullable, id, or
unique, as prohibited by ADR 234 and unsupported by ResolveTemplateValue. Update
registration validation to report these forbidden placements, and add
registration coverage for each field while preserving valid select placements.
Source: Learnings
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
1 participant
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.
Overview
A contract describing a database that Prisma ORM created needs
timestamp(3)columns — that is what ORM migrations generate forDateTime @updatedAt. Prisma Next could spell the column type (DateTime @db.Timestamp(3)) or the auto-update behavior (temporal.updatedAt(), which hardcodestimestamptz), but never both:@updatedAtwas removed deliberately, and@default(timestampNow())is rejected by design becausetimestampNowis preset-only (ADR 169). Correct type or correct behavior, pick one.This adds per-codec temporal presets whose arguments carry the execution-default behavior, so both can be spelled together:
The governing rule is one preset per codec; arguments change properties of the field, never its codec.
temporal.updatedAt()survives unchanged, as pure convenience — shorthand fortemporal.timestamptz(onCreate: now, onUpdate: now). Don't liketimestamptz? Write the full form with the codec you want.Refs: TML-3036
The spelling, and why
onUpdate: nowis a bare token, following@relation(onDelete: Cascade)— the established way PSL spells an enumerated argument. No grammar change was needed: bare identifiers already parse in argument position, and theoptiondescriptor kind mirrors thePslBlockParamOptionvocabulary that extension blocks already use. On TypeScript the same argument types as the literal union'now', so the legal set is autocompleted and compiler-enforced.nowis preset vocabulary, dispatched to the internal generator descriptor by aselecttemplate node whose cases are validated against the option's legal values at pack-registration time —timestampNownever becomes user-facing spelling, per ADR 169. The option concept itself is one shared type across the extension-block parameter vocabulary and the helper-argument vocabulary.ADR 234 is the decision record and the best place to start reviewing. It covers the four decisions, the alternatives rejected (booleans, quoted strings, variant-selecting args,
@db.*composition), and several non-obvious properties of the result that are easy to break without knowing they exist.Changes
Framework — Core (
packages/1-framework/1-core/framework-components, shared plane)AuthoringOptiontype (kind: 'option',values) used by both the helper-argument union andPslBlockParamOption, with membership validation on the PSL path.selecttemplate node ({ kind: 'select', index, cases }) — token → template dispatch; case coverage is checked against the targeted option argument'svaluesat pack registration (set equality: no uncovered token, no unreachable case), fromassertNoCrossRegistryCollisions, so both composition seams inherit it.undefinedis omitted (neverexecutionDefaults: {}), and an empty resolvedtypeParamsproduces no key at all.SQL — Authoring (
contract-psl,contract-ts)SQL — Family (
family-sql):temporalCodecPresetWithPrecision/temporalCodecPresetfactories.Targets — Postgres / SQLite: register
temporal.timestamp+temporal.timestamptz(postgres) andtemporal.datetime(sqlite). A per-codec preset is named for its codec's base name, which is what lets them share thetemporalnamespace with the convenience presets.No IR, runtime, or adapter changes: the
timestampNowgenerator and theexecution.mutations.defaultsplumbing are reused as-is, and{ precision }typeParams already flow through DDL rendering. No emitted contract changes —fixtures:checkis clean, which is also the evidence thattemporal.updatedAt()is byte-identical to what it produced before.Non-obvious bits worth a reviewer's attention
Two type-level fixes were forced by the optional tuple slots, and both are in ADR 234:
id/uniqueneeds an overload pair — once the argument tuple can infer as empty, a single signature with a trailingoptions?binds the first real argument tooptions, which brokefield.id.nanoid({ size: 16 }).ObjectArgumentTypedrops its empty-object constituent for all-optional descriptors.{}is not a weak type, so intersecting with it silently disabled TypeScript's weak-type check — and because the TS authoring surface performs no runtime argument validation by design, the type system is the only check there. Without this,field.id.nanoid({ name: 'pk' })compiled into a length-21 nanoid with an unnamed primary key. Silent wrong DDL, no diagnostic at any layer. That call was a compile error before this branch, so nothing regressed — but a loud error becoming a silent wrong result is worse, and this closes it.Test fixtures that mirror shipping descriptors are now anchored. Several test packs hand-copy a real preset's shape because the dependency graph forbids importing it (
family-sqldepends oncontract-ts/contract-psl, not the reverse). Those copies were inert until this branch made their fidelity load-bearing — theupdatedAt()byte-identity guarantee routes through one. They're now anchored by deep-equal assertions infamily-sql, so a factory change can't leave a stale copy quietly passing. This is whyfamily-sql's tests reach into two other packages' test directories;lint:depsis clean.Notes for the reviewer
contract-builder.dsl.helpers.test.ts,fixtures.ts,framework-components.authoring.test.ts). All three were already over onmain. Splitting them is a mechanical change that would bury a subtle type-system diff, so it belongs in its own PR — flagging pre-emptively rather than have it come up in review.temporal.createdAt()is untouched. It's a storage default (now()in DDL) — a different mechanism from execution defaults, and deliberately not expressible viaonCreate: now.@db.*native-type attributes are untouched. Whether they should go is a much larger question and a separate ticket.Verification
pnpm build,pnpm typecheck,pnpm fixtures:check,pnpm lint:deps— all green.pnpm test:packages— 12,195 passing; the failures are the knownconfig-loader/var→/private/varsymlink family and a CLI cross-package parallel-run flake (the CLI package passes 1332/1332 in isolation), neither related to these changes.Coverage pins every row of the lowering table as whole column shapes, every diagnostic spelling, three-way byte-identity between
temporal.updatedAt()and both full forms, and literal-type survival (a widened codec id or option union would silently cost the TS surface its enforcement). Note that the parity tests prove the four spellings agree; correctness is proved separately by absolute assertions, since all four funnel through the same resolver.Summary by CodeRabbit