fix(orm): exclude Unsupported fields from ORM client#2468
Conversation
Unsupported fields (e.g., Unsupported("xml")) are database-specific types
that cannot be meaningfully read or written through the ORM. This change
excludes them from TypeScript types, Zod validation schemas, and SQL
queries. Models with required Unsupported fields (no default) have their
create/upsert operations disabled since values cannot be provided.
- Add FieldIsUnsupported and ModelHasRequiredUnsupportedField type utils
- Exclude Unsupported fields from result, where, select, omit, orderBy,
create, update, distinct, aggregate, and groupBy types
- Skip Unsupported fields in all Zod schema generation
- Skip Unsupported fields in SELECT queries and schema pusher
- Disable create/upsert operations for models with required Unsupported
fields (both at type level and runtime)
- Add isUnsupportedField shared helper in query-utils
- Add comprehensive type tests, Zod tests, and ORM call tests
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Claude finished @ymc9's task —— View job PR Review: Exclude Unsupported Fields from ORM ClientThis is a well-architected and comprehensive solution for handling Code Quality Assessment✅ Excellent Type Safety:
✅ Runtime Implementation:
Performance & Security✅ Performance Considerations:
✅ Security Assessment:
Test Coverage✅ Comprehensive Testing:
Architectural Strengths✅ Design Decisions:
Minor Recommendations
SummaryThis PR demonstrates excellent engineering practices:
The implementation is production-ready and should be merged. The approach of completely excluding Recommendation: APPROVE ✅ |
|
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:
📝 WalkthroughWalkthroughAdds support for excluding "Unsupported" fields from type-level and runtime ORM surfaces, and gates create-related operations for models with required unsupported fields (without defaults) via new type- and runtime-checks. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/orm/src/client/executor/name-mapper.ts (1)
69-74:⚠️ Potential issue | 🟠 MajorRaw
*selections can still leak unsupported columns.These changes only filter unsupported fields once the query has already been expanded into an explicit field list. The expansion decision still depends on mapped columns / mapped enums, so a model whose only special case is an
Unsupportedfield will keep rawselect */returning *, and the unsupported column still comes back. Unsupported-field presence needs to be another reason to expand*.Also applies to: 563-575, 626-636
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/orm/src/client/executor/name-mapper.ts` around lines 69 - 74, The code that decides whether to keep raw `select *` must also detect models that contain Unsupported fields and force expansion; when iterating model fields (see getModelFields, getMappedName and the loop that populates fieldToColumnMap) check each fieldDef for Unsupported (or the existing isUnsupportedField predicate) and set a flag (e.g., forceExpandForUnsupported) stored alongside the model mapping; update the logic that currently allows raw `*` (the decision points referenced around the other ranges 563-575 and 626-636) to consult this flag and expand `*` if any Unsupported field is present so unsupported columns cannot be returned.packages/orm/src/client/query-utils.ts (1)
75-96:⚠️ Potential issue | 🟠 Major
getModelFields()now changes existing callers' semantics.This flips the helper from “complete field list unless a caller filters it down” to “supported fields only unless a caller opts back in”.
packages/plugins/policy/src/policy-handler.ts:860-880still usesgetModelFields(..., { inherited: true })to build a values table over model fields; with this default it silently drops unsupported columns, so downstream logic that expects the full table shape now sees something different. Please either keep the old default and opt out only at the call sites that need exclusion, or audit/update completeness-oriented callers to pass{ unsupported: true }.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/orm/src/client/query-utils.ts` around lines 75 - 96, getModelFields currently treats unspecified options as false, causing it to drop fields (like type 'Unsupported') by default; revert to the original "include everything unless explicitly excluded" behavior by changing the exclusion checks in getModelFields (function getModelFields) to only exclude when the option is explicitly false (e.g., use options?.relations === false, options?.computed === false, options?.inherited === false, options?.unsupported === false) so callers that rely on the full field list (e.g., the policy handler usage that passes { inherited: true }) continue to see the complete schema without requiring callers to opt-in.
🤖 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/orm/src/client/crud/operations/base.ts`:
- Around line 2512-2521: The guard checkNoRequiredUnsupportedFields() currently
always checks this.model, which misses nested writes; update the logic to accept
a model parameter (e.g., checkNoRequiredUnsupportedFields(modelName: string)) or
add an overload so callers can pass the target model, then use
requireModel(modelName) / iterate modelDef.fields and run the same
isUnsupportedField + !fieldDef.optional + !fieldHasDefaultValue check and throw
createNotSupportedError with the fieldName; call the updated check from all
mutation helpers that accept a model parameter (create, createMany,
connectOrCreate, upsert and any nested write handlers) passing the local model
argument instead of relying on this.model.
In `@packages/orm/src/client/type-utils.ts`:
- Around line 10-25: The type-level guard ModelHasRequiredUnsupportedField does
not consider fields marked with updatedAt as having an implicit default, causing
a mismatch with runtime fieldHasDefaultValue; update the conditional inside
ModelHasRequiredUnsupportedField (the branch that currently checks "'default'
extends keyof F") to also treat fields with an updatedAt marker as having a
default (e.g., check for 'updatedAt' existing on F or F['updatedAt'] being true)
so that GetModel/FieldDef logic treats `@updatedAt` Unsupported fields the same at
type-level as at runtime.
In `@packages/orm/src/client/zod/factory.ts`:
- Around line 1173-1181: Top-level create/upsert Zod schemas (built by
makeCreateSchema, makeCreateManySchema, makeCreateManyAndReturnSchema,
makeUpsertSchema) remain permissive for models that should be blocked because
canCreateModel() is only applied to nested relation shapes; update the top-level
schema builders to consult canCreateModel(model) and throw or return a
never/forbidden Zod schema when creation is not allowed rather than silently
omitting unsupported fields via makeCreateDataSchema; specifically, modify the
code paths that call makeCreateDataSchema (and the loops using getModelFields,
nextOptions, withoutFields) so they first check canCreateModel(model) and
produce a rejecting schema (or add a .refine/.superRefine) for blocked models,
ensuring consistency with runtime/client restrictions for GeoRecord and the
other affected ranges.
---
Outside diff comments:
In `@packages/orm/src/client/executor/name-mapper.ts`:
- Around line 69-74: The code that decides whether to keep raw `select *` must
also detect models that contain Unsupported fields and force expansion; when
iterating model fields (see getModelFields, getMappedName and the loop that
populates fieldToColumnMap) check each fieldDef for Unsupported (or the existing
isUnsupportedField predicate) and set a flag (e.g., forceExpandForUnsupported)
stored alongside the model mapping; update the logic that currently allows raw
`*` (the decision points referenced around the other ranges 563-575 and 626-636)
to consult this flag and expand `*` if any Unsupported field is present so
unsupported columns cannot be returned.
In `@packages/orm/src/client/query-utils.ts`:
- Around line 75-96: getModelFields currently treats unspecified options as
false, causing it to drop fields (like type 'Unsupported') by default; revert to
the original "include everything unless explicitly excluded" behavior by
changing the exclusion checks in getModelFields (function getModelFields) to
only exclude when the option is explicitly false (e.g., use options?.relations
=== false, options?.computed === false, options?.inherited === false,
options?.unsupported === false) so callers that rely on the full field list
(e.g., the policy handler usage that passes { inherited: true }) continue to see
the complete schema without requiring callers to opt-in.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7c46f86f-e799-4ed1-bee8-bffd070eaf10
📒 Files selected for processing (17)
packages/orm/src/client/contract.tspackages/orm/src/client/crud/dialects/base-dialect.tspackages/orm/src/client/crud/operations/base.tspackages/orm/src/client/crud/operations/create.tspackages/orm/src/client/crud/operations/update.tspackages/orm/src/client/executor/name-mapper.tspackages/orm/src/client/helpers/schema-db-pusher.tspackages/orm/src/client/query-utils.tspackages/orm/src/client/type-utils.tspackages/orm/src/client/zod/factory.tspackages/schema/src/schema.tstests/e2e/orm/client-api/unsupported.test-d.tstests/e2e/orm/client-api/unsupported.test.tstests/e2e/orm/schemas/unsupported/input.tstests/e2e/orm/schemas/unsupported/models.tstests/e2e/orm/schemas/unsupported/schema.tstests/e2e/orm/schemas/unsupported/schema.zmodel
|
Claude finished @ymc9's task —— View job PR Review: Exclude Unsupported Fields from ORM ClientConducting comprehensive code review to analyze:
Review SummaryThis is a well-architected and comprehensive solution for handling Code Quality Assessment ✅Excellent Type Safety
Runtime Implementation
Security & Performance ✅Security Assessment
Performance Considerations
Test Coverage ✅Comprehensive Testing Strategy
Issues Identified 🚨However, CodeRabbit identified several legitimate issues that should be addressed: 1. Critical Issue - Nested Write Operations Not ProtectedLocation: The Risk Level: High - Data integrity violation 2. Type/Runtime Mismatch -
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/orm/src/client/type-utils.ts (1)
18-20:⚠️ Potential issue | 🟠 MajorTreat
@updatedAtas an implicit default here too.Line 18 only checks for an explicit
default. The runtime default-value path also treatsupdatedAtfields as satisfiable, so a model whose only requiredUnsupportedfield is@updatedAtwill losecreate/upsertat the type level even though runtime can still allow it.Suggested fix
: 'default' extends keyof F ? false - : true + : 'updatedAt' extends keyof F + ? false + : true#!/bin/bash set -euo pipefail # Verify the type-level guard only checks `default`, while the runtime default-value # logic also accounts for `updatedAt`. rg -n -C2 'ModelHasRequiredUnsupportedField|fieldHasDefaultValue|updatedAt' \ packages/orm/src/client/type-utils.ts \ packages/orm/src/client/query-utils.ts \ packages/schema/src/schema.ts🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/orm/src/client/type-utils.ts` around lines 18 - 20, The type-level check in ModelHasRequiredUnsupportedField (in type-utils.ts) only treats fields with an explicit "default" key as satisfiable, but runtime also accepts fields marked with `@updatedAt`; update the conditional (where it currently uses "'default' extends keyof F ? false : true") to also consider fields with the updatedAt marker as having a default. Concretely, extend the guard to check both "'default' extends keyof F" OR an "updatedAt" indicator on F (match whatever metadata key/type you use for `@updatedAt` in your schema types) so that fields with `@updatedAt` are treated as having a default at the type level, keeping consistency with fieldHasDefaultValue/runtime behavior.
🧹 Nitpick comments (1)
tests/e2e/orm/client-api/unsupported.test.ts (1)
255-276: Make the nested-write failures more specific.In
packages/testtools/src/vitest-ext.ts, Lines 105-125,toBeRejectedByValidation()can assert expected messages. Right now both tests only assert a generic validation rejection, so any unrelatedINVALID_INPUTfromdb.geoParent.createwould still satisfy them.🧪 Suggested hardening
it('rejects nested create for model with required Unsupported field', async () => { + await expect(db.geoParent.create({ data: { name: 'parent-ok' } })).resolves.toMatchObject({ + name: 'parent-ok', + }); await expect( db.geoParent.create({ data: { name: 'parent', // `@ts-expect-error` create should not be allowed for GeoRecord (required Unsupported) records: { create: { title: 'test' } }, }, }), ).toBeRejectedByValidation(); }); it('rejects nested connectOrCreate for model with required Unsupported field', async () => { + await expect( + db.geoParent.create({ data: { name: 'parent-connect-or-create-ok' } }), + ).resolves.toMatchObject({ name: 'parent-connect-or-create-ok' }); await expect( db.geoParent.create({ data: { name: 'parent', // `@ts-expect-error` connectOrCreate should not be allowed for GeoRecord (required Unsupported) records: { connectOrCreate: { where: { id: 1 }, create: { title: 'test' } } }, }, }), ).toBeRejectedByValidation(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/orm/client-api/unsupported.test.ts` around lines 255 - 276, Update the two tests that call db.geoParent.create to assert a specific validation message instead of the generic rejection: use the toBeRejectedByValidation helper (defined in packages/testtools/src/vitest-ext.ts) with an expected message that matches the validation error for nested writes on a required Unsupported field (mentioning the field name "records" and/or model "GeoRecord" or the relevant INVALID_INPUT substring your validator emits). Change the two assertions in the 'rejects nested create...' and 'rejects nested connectOrCreate...' tests to pass the expected error string to toBeRejectedByValidation so the test fails on unrelated INVALID_INPUT errors.
🤖 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/orm/src/client/contract.ts`:
- Line 887: The new exported type OperationsRequiringCreate replaces a
previously exported alias OperationsIneligibleForDelegateModels, which will
break downstream consumers; restore a compatibility alias by re-exporting the
old name (OperationsIneligibleForDelegateModels) to point to the new type
(OperationsRequiringCreate) so both names exist for at least one release cycle,
and add a short comment marking the old alias as deprecated/temporary.
In `@tests/e2e/orm/client-api/unsupported.test.ts`:
- Around line 243-253: The test needs to cover the remaining create-capability
branches for models with Required Unsupported fields (with and without
defaults): update the assertions around db.geoRecord to also check
presence/absence of createMany and createManyAndReturn in addition to create and
upsert, and ensure upsert is asserted present when the Unsupported field has a
default; specifically, add assertions mirroring the existing create/upsert
checks for db.geoRecord.createMany and db.geoRecord.createManyAndReturn, and
adjust the branch that expects upsert to be defined when the Unsupported field
has a default so the test reflects the intended behavior implemented in
client-impl.ts (preserve upsert when Unsupported has default).
---
Duplicate comments:
In `@packages/orm/src/client/type-utils.ts`:
- Around line 18-20: The type-level check in ModelHasRequiredUnsupportedField
(in type-utils.ts) only treats fields with an explicit "default" key as
satisfiable, but runtime also accepts fields marked with `@updatedAt`; update the
conditional (where it currently uses "'default' extends keyof F ? false : true")
to also consider fields with the updatedAt marker as having a default.
Concretely, extend the guard to check both "'default' extends keyof F" OR an
"updatedAt" indicator on F (match whatever metadata key/type you use for
`@updatedAt` in your schema types) so that fields with `@updatedAt` are treated as
having a default at the type level, keeping consistency with
fieldHasDefaultValue/runtime behavior.
---
Nitpick comments:
In `@tests/e2e/orm/client-api/unsupported.test.ts`:
- Around line 255-276: Update the two tests that call db.geoParent.create to
assert a specific validation message instead of the generic rejection: use the
toBeRejectedByValidation helper (defined in
packages/testtools/src/vitest-ext.ts) with an expected message that matches the
validation error for nested writes on a required Unsupported field (mentioning
the field name "records" and/or model "GeoRecord" or the relevant INVALID_INPUT
substring your validator emits). Change the two assertions in the 'rejects
nested create...' and 'rejects nested connectOrCreate...' tests to pass the
expected error string to toBeRejectedByValidation so the test fails on unrelated
INVALID_INPUT errors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 915f8be7-bb71-47a3-8313-e510d71116f6
📒 Files selected for processing (11)
packages/clients/tanstack-query/src/common/types.tspackages/orm/src/client/client-impl.tspackages/orm/src/client/contract.tspackages/orm/src/client/crud-types.tspackages/orm/src/client/crud/operations/base.tspackages/orm/src/client/type-utils.tstests/e2e/orm/client-api/unsupported.test.tstests/e2e/orm/schemas/unsupported/input.tstests/e2e/orm/schemas/unsupported/models.tstests/e2e/orm/schemas/unsupported/schema.tstests/e2e/orm/schemas/unsupported/schema.zmodel
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/e2e/orm/schemas/unsupported/models.ts
- tests/e2e/orm/schemas/unsupported/schema.zmodel
- packages/orm/src/client/crud/operations/base.ts
- tests/e2e/orm/schemas/unsupported/input.ts
|
Does this basically make it possible to handle vector field types like prisma does? https://www.prisma.io/docs/postgres/database/postgres-extensions |
|
Claude finished @ymc9's task —— View job PR Review: Exclude Unsupported Fields from ORM ClientConducting comprehensive code review to analyze:
Review SummaryThis is a well-architected and comprehensive solution for handling Code Quality Assessment ✅Excellent Type Safety
Runtime Implementation
Security & Performance ✅Security Assessment
Performance Considerations
Test Coverage ✅Comprehensive Testing Strategy
Issues Identified 🚨However, CodeRabbit identified several legitimate issues that should be addressed: 1. Critical Issue - Nested Write Operations Not ProtectedLocation: The Risk Level: High - Data integrity violation 2. Type/Runtime Mismatch -
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
packages/orm/src/client/contract.ts (1)
897-897:⚠️ Potential issue | 🟠 MajorKeep the old exported alias for one compatibility cycle.
Renaming this public type is a compile break for consumers importing
OperationsIneligibleForDelegateModels. Please keep the old name as a deprecated alias toOperationsRequiringCreatefor at least one release.Suggested compatibility shim
export type OperationsRequiringCreate = 'create' | 'createMany' | 'createManyAndReturn' | 'upsert'; + +/** `@deprecated` Use `OperationsRequiringCreate` instead. */ +export type OperationsIneligibleForDelegateModels = OperationsRequiringCreate;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/orm/src/client/contract.ts` at line 897, The exported type was renamed to OperationsRequiringCreate, but consumers still import OperationsIneligibleForDelegateModels; add a deprecated alias export so the old name continues to resolve. Specifically, create an exported type alias named OperationsIneligibleForDelegateModels = OperationsRequiringCreate, add a JSDoc/@deprecated comment above it mentioning to migrate to OperationsRequiringCreate and keep this shim for one release; place the alias next to the new type declaration so it’s easy to remove later.packages/orm/src/client/zod/factory.ts (1)
1225-1231:⚠️ Potential issue | 🟠 MajorThis now makes blocked top-level create schemas validate.
Because
modelFieldsnow excludesUnsupportedfields,makeCreateDataSchema()produces a normal object schema even for models that should be non-creatable. The nested relation path is guarded later, but the top-levelmakeCreate*andmakeUpsertSchemaentry points still call this helper unconditionally, so$zodremains out of sync with the client contract/runtime restriction.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/orm/src/client/zod/factory.ts` around lines 1225 - 1231, The helper that builds create schemas is omitting Unsupported fields (via getModelFields) which lets makeCreateDataSchema() and makeUpsertSchema() produce a normal object schema for models that should be non-creatable; change makeCreateDataSchema (and the entry points makeUpsertSchema) to detect when a model has Unsupported top-level fields (or when all creatable fields are blocked) and return a schema that rejects creation (e.g., z.never() or an explicit non-creatable schema) instead of proceeding, or adjust getModelFields usage so Unsupported fields are preserved for this check; locate the logic around modelFields, getModelFields, makeCreateDataSchema, and makeUpsertSchema and add the guard before iterating modelFields/producing the object schema.
🤖 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/orm/src/client/crud/dialects/base-dialect.ts`:
- Around line 1133-1137: The delegate-descendant JSON packing branch currently
iterates Object.keys(subModel.fields) and thus still includes Unsupported
fields; change that iteration to use getModelFields(this.schema, subModel, {
inherited: true, computed: true }) and, for each returned fieldDef, call
this.shouldOmitField(omit, subModel, fieldDef.name) to skip omitted/unsupported
fields before including them in the delegate packing logic (use the same
buildSelectField/field serialization path you use above so selection and
serialization rules are consistent).
In `@packages/orm/src/client/zod/factory.ts`:
- Around line 1355-1377: The code is enabling nested create-like ops based only
on canCreateModel(fieldType), but canCreateModel only checks for required
Unsupported fields and can mistakenly allow create/connectOrCreate/createMany
for delegate models; update the guards around constructing 'create',
'connectOrCreate', and 'createMany' (the calls to makeCreateDataSchema,
makeConnectOrCreateDataSchema, makeCreateManyPayloadSchema) to also require the
public ModelAllowsCreate check (import/consume ModelAllowsCreate from
packages/orm/src/client/type-utils.ts or call the existing helper) so these
branches only run when both canCreateModel(fieldType) and
ModelAllowsCreate(fieldType) are true. Ensure the same fix is applied to the
other occurrence referenced (lines ~1406-1419).
---
Duplicate comments:
In `@packages/orm/src/client/contract.ts`:
- Line 897: The exported type was renamed to OperationsRequiringCreate, but
consumers still import OperationsIneligibleForDelegateModels; add a deprecated
alias export so the old name continues to resolve. Specifically, create an
exported type alias named OperationsIneligibleForDelegateModels =
OperationsRequiringCreate, add a JSDoc/@deprecated comment above it mentioning
to migrate to OperationsRequiringCreate and keep this shim for one release;
place the alias next to the new type declaration so it’s easy to remove later.
In `@packages/orm/src/client/zod/factory.ts`:
- Around line 1225-1231: The helper that builds create schemas is omitting
Unsupported fields (via getModelFields) which lets makeCreateDataSchema() and
makeUpsertSchema() produce a normal object schema for models that should be
non-creatable; change makeCreateDataSchema (and the entry points
makeUpsertSchema) to detect when a model has Unsupported top-level fields (or
when all creatable fields are blocked) and return a schema that rejects creation
(e.g., z.never() or an explicit non-creatable schema) instead of proceeding, or
adjust getModelFields usage so Unsupported fields are preserved for this check;
locate the logic around modelFields, getModelFields, makeCreateDataSchema, and
makeUpsertSchema and add the guard before iterating modelFields/producing the
object schema.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 984b684a-2aa0-4fcc-9477-d030388cd3bb
📒 Files selected for processing (9)
packages/orm/src/client/client-impl.tspackages/orm/src/client/contract.tspackages/orm/src/client/crud-types.tspackages/orm/src/client/crud/dialects/base-dialect.tspackages/orm/src/client/crud/operations/base.tspackages/orm/src/client/zod/factory.tspackages/schema/src/schema.tstests/e2e/orm/plugin-infra/ext-result/schema.tstests/e2e/orm/schemas/unsupported/schema.ts
💤 Files with no reviewable changes (1)
- packages/orm/src/client/crud/operations/base.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/orm/src/client/client-impl.ts
- tests/e2e/orm/schemas/unsupported/schema.ts
|
Claude finished @ymc9's task —— View job PR Review: Exclude Unsupported Fields from ORM ClientConducting comprehensive code review to analyze:
Review SummaryThis is a well-architected and comprehensive solution for handling Code Quality Assessment ✅Excellent Type Safety
Runtime Implementation
Security & Performance ✅Security Assessment
Performance Considerations
Test Coverage ✅Comprehensive Testing Strategy
Issues Identified 🚨However, CodeRabbit identified several legitimate issues that should be addressed: 1. Critical Issue - Nested Write Operations Not ProtectedLocation: The Risk Level: High - Data integrity violation 2. Type/Runtime Mismatch -
|
|
Claude finished @ymc9's task —— View job PR Review: Exclude Unsupported Fields from ORM ClientConducting comprehensive code review to analyze:
Review SummaryThis is a well-architected and comprehensive solution for handling Code Quality Assessment ✅Excellent Type Safety
Runtime Implementation
Security & Performance ✅Security Assessment
Performance Considerations
Test Coverage ✅Comprehensive Testing Strategy
Issues Identified 🚨However, CodeRabbit identified several legitimate issues that should be addressed: 1. Critical Issue - Nested Write Operations Not ProtectedLocation: The Risk Level: High - Data integrity violation 2. Type/Runtime Mismatch -
|
|
Claude finished @ymc9's task —— View job PR Review: Exclude Unsupported Fields from ORM ClientConducting comprehensive code review to analyze:
Review SummaryThis is a well-architected and comprehensive solution for handling Code Quality Assessment ✅Excellent Type Safety
Runtime Implementation
Security & Performance ✅Security Assessment
Performance Considerations
Test Coverage ✅Comprehensive Testing Strategy
Issues Identified 🚨However, CodeRabbit identified several legitimate issues that should be addressed: 1. Critical Issue - Nested Write Operations Not ProtectedLocation: The Risk Level: High - Data integrity violation 2. Type/Runtime Mismatch -
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/orm/src/client/zod/factory.ts (1)
141-143: Use the shared unsupported-field helper hereThis filter duplicates unsupported detection inline. Prefer the shared
isUnsupportedFieldhelper to keep one source of truth.♻️ Suggested refactor
import { fieldHasDefaultValue, getEnum, getTypeDef, getUniqueFields, + isUnsupportedField, isEnum, isTypeDef, requireField, requireModel, } from '../query-utils'; @@ - return Object.entries(modelDef.fields).filter(([, def]) => def.type !== 'Unsupported'); + return Object.entries(modelDef.fields).filter(([, def]) => !isUnsupportedField(def));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/orm/src/client/zod/factory.ts` around lines 141 - 143, The getModelFields method currently filters out unsupported fields by checking def.type !== 'Unsupported'; update it to use the shared helper by importing and calling isUnsupportedField and filtering with !isUnsupportedField(def) instead so unsupported detection is centralized; locate getModelFields and replace the inline check with the isUnsupportedField helper (and add the import if missing).
🤖 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/orm/src/client/zod/factory.ts`:
- Around line 2246-2248: The runtime create-gating logic (the
hasRequiredUnsupportedFields check in canCreateModel()) currently uses
fieldHasDefaultValue(fieldDef), which treats both default and updatedAt as
defaults and thus diverges from the type-level ModelHasRequiredUnsupportedField
rule; update the check to only consider an explicit default (e.g.,
fieldDef.default !== undefined) rather than fieldHasDefaultValue so that
required Unsupported fields with updatedAt: true but no default remain
disallowed for creation, keeping runtime behavior aligned with the type-level
rule.
---
Nitpick comments:
In `@packages/orm/src/client/zod/factory.ts`:
- Around line 141-143: The getModelFields method currently filters out
unsupported fields by checking def.type !== 'Unsupported'; update it to use the
shared helper by importing and calling isUnsupportedField and filtering with
!isUnsupportedField(def) instead so unsupported detection is centralized; locate
getModelFields and replace the inline check with the isUnsupportedField helper
(and add the import if missing).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 35a0d559-9b13-4730-a930-08dd12f55e3f
📒 Files selected for processing (1)
packages/orm/src/client/zod/factory.ts
|
Claude finished @ymc9's task —— View job PR Review: Exclude Unsupported Fields from ORM ClientConducting comprehensive code review to analyze:
Review SummaryThis is a well-architected and comprehensive solution for handling Code Quality Assessment ✅Excellent Type Safety
Runtime Implementation
Security & Performance ✅Security Assessment
Performance Considerations
Test Coverage ✅Comprehensive Testing Strategy
Issues Identified 🚨However, CodeRabbit identified several legitimate issues that should be addressed: 1. Critical Issue - Nested Write Operations Not ProtectedLocation: The Risk Level: High - Data integrity violation 2. Type/Runtime Mismatch -
|
2 participants
Summary
Unsupportedtype fields from all ORM client TypeScript types (result, where, select, omit, orderBy, create, update, distinct, aggregate, groupBy)Unsupportedfields in Zod validation schemas, SQL SELECT queries, and schema pusherUnsupportedfields that have no default valueisUnsupportedFieldhelper inquery-utils.tsandFieldIsUnsupported/ModelHasRequiredUnsupportedFieldtype utilities inschema.tsTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests