fix(orm): exclude Unsupported fields from ORM client#2468
Open
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 <[email protected]>
Contributor
|
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 ✅ |
Contributor
📝 WalkthroughWalkthroughThis pull request introduces support for filtering unsupported fields throughout the ORM. The system now detects models with required unsupported fields lacking defaults and conditionally disables create operations, replacing delegate-model-based filtering with create-ability conditions across type definitions, runtime validation, Zod schema generation, and client APIs. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate 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 |
![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: 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
Contributor
|
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 -
|
Contributor
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
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.
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
Tests