fix(sdk): drop create-only fields from read types in TypeScript emitter#4605
Conversation
📝 WalkthroughWalkthroughAdds a response-reachability-based read-visibility filtering mechanism to the TypeSpec TypeScript emitter. A new ChangesTypeSpec emitter: read-visibility filtering
Generated client contract updates
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
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 |
Greptile SummaryThis PR updates the TypeScript SDK emitter to remove create-only fields from generated read shapes. The main changes are:
Confidence Score: 4/5The generated request input path needs a fix before merging.
api/spec/packages/typespec-typescript/src/interface-types.ts, api/spec/packages/typespec-typescript/src/input-variants.ts, api/spec/packages/typespec-typescript/src/utils.tsx Important Files Changed
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
api/spec/packages/typespec-typescript/src/interface-types.ts:50
**Input Fields Become Read Fields**
`interfaceBody` also emits `...Input` variants, but this call now uses `bodyProperties()`, which filters create/update-only properties from any response-reachable model. For models used in both responses and requests, such as `CreditGrant` and `SubscriptionAddon`, valid request fields like `expires_after`, `key`, and `timing` disappear from the generated input types, so SDK callers cannot type valid create/update payloads without casts.
Reviews (1): Last reviewed commit: "fix(sdk): drop create-only fields from r..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (isHttpEnvelopeProperty(program, prop)) { | ||
| continue | ||
| } | ||
| for (const prop of bodyProperties(program, model)) { |
There was a problem hiding this comment.
interfaceBody also emits ...Input variants, but this call now uses bodyProperties(), which filters create/update-only properties from any response-reachable model. For models used in both responses and requests, such as CreditGrant and SubscriptionAddon, valid request fields like expires_after, key, and timing disappear from the generated input types, so SDK callers cannot type valid create/update payloads without casts.
Context Used: api/spec/AGENTS.md (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: api/spec/packages/typespec-typescript/src/interface-types.ts
Line: 50
Comment:
**Input Fields Become Read Fields**
`interfaceBody` also emits `...Input` variants, but this call now uses `bodyProperties()`, which filters create/update-only properties from any response-reachable model. For models used in both responses and requests, such as `CreditGrant` and `SubscriptionAddon`, valid request fields like `expires_after`, `key`, and `timing` disappear from the generated input types, so SDK callers cannot type valid create/update payloads without casts.
**Context Used:** api/spec/AGENTS.md ([source](https://app.greptile.com/openmeter/github/openmeterio/openmeter/-/custom-context?memory=28ba6068-00f9-4629-9b78-8e49cc802858))
How can I resolve this? If you propose a fix, please make it concise.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@api/spec/packages/typespec-typescript/src/utils.tsx`:
- Around line 110-114: The shared bodyProperties() helper is incorrectly
applying read-visibility filtering, which removes create-only properties from
input generation and can suppress ...Input variants. Update
bodyProperties(program, type) in utils.tsx to keep only the shared envelope
filtering, and move the isReadVisible logic to the response/output-specific call
sites instead. Make sure interface-types.ts and input-variants.ts continue to
use bodyProperties() for the common body shape, while any response-only
visibility filtering is applied separately where output models are produced.
In `@api/spec/packages/typespec-typescript/src/visibility.ts`:
- Around line 89-94: The recursive walk in visit is incorrectly traversing
type.derivedModels from every visited model, which pulls sibling discriminator
subtypes into the reachable read set. Update the traversal so it only follows
the actual returned subtype path (for example, by limiting derived-model descent
to the concrete response subtype or otherwise avoiding expanding all siblings
when visiting a base like Animal) while keeping the existing visibility
filtering logic in visibility.ts intact.
🪄 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.yaml
Review profile: CHILL
Plan: Pro
Run ID: 05aa73be-c9df-414a-8912-489628bf67bd
📒 Files selected for processing (7)
api/spec/packages/aip-client-javascript/src/models/schemas.tsapi/spec/packages/aip-client-javascript/src/models/types.tsapi/spec/packages/typespec-typescript/src/emitter.tsxapi/spec/packages/typespec-typescript/src/input-variants.tsapi/spec/packages/typespec-typescript/src/interface-types.tsapi/spec/packages/typespec-typescript/src/utils.tsxapi/spec/packages/typespec-typescript/src/visibility.ts
💤 Files with no reviewable changes (2)
- api/spec/packages/aip-client-javascript/src/models/types.ts
- api/spec/packages/aip-client-javascript/src/models/schemas.ts
| export function bodyProperties(program: Program, type: Model): ModelProperty[] { | ||
| return [...type.properties.values()].filter( | ||
| (prop) => !isHttpEnvelopeProperty(program, prop), | ||
| (prop) => | ||
| !isHttpEnvelopeProperty(program, prop) && | ||
| isReadVisible(program, type, prop), |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Keep read-visibility filtering out of the shared bodyProperties() helper.
This helper is still used by input-mode paths too: api/spec/packages/typespec-typescript/src/interface-types.ts, Line 175 builds ...Input bodies from it, and api/spec/packages/typespec-typescript/src/input-variants.ts, Line 62 uses it for divergence detection. Once a response-reachable model hides create-only props here, those props disappear from generated input types as well, and models whose only input/output difference is visibility can stop getting an ...Input variant. Splitting the read filter from the shared envelope filter would avoid that.
🤖 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 `@api/spec/packages/typespec-typescript/src/utils.tsx` around lines 110 - 114,
The shared bodyProperties() helper is incorrectly applying read-visibility
filtering, which removes create-only properties from input generation and can
suppress ...Input variants. Update bodyProperties(program, type) in utils.tsx to
keep only the shared envelope filtering, and move the isReadVisible logic to the
response/output-specific call sites instead. Make sure interface-types.ts and
input-variants.ts continue to use bodyProperties() for the common body shape,
while any response-only visibility filtering is applied separately where output
models are produced.
| // Descend into subclasses too: a model-form `@discriminator` hierarchy | ||
| // returned as a response is reached through its base, and each concrete | ||
| // subtype carries its own read-side properties to filter. | ||
| for (const derived of type.derivedModels) { | ||
| visit(derived) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Avoid pulling sibling subtypes into the reachable read set.
Returning one concrete subtype will currently mark its siblings reachable too. For example, visit(Dog) climbs to Animal, then Animal.derivedModels pulls in Cat, so Cat starts getting read-visibility filtering even though it never appears in a success response.
🤖 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 `@api/spec/packages/typespec-typescript/src/visibility.ts` around lines 89 -
94, The recursive walk in visit is incorrectly traversing type.derivedModels
from every visited model, which pulls sibling discriminator subtypes into the
reachable read set. Update the traversal so it only follows the actual returned
subtype path (for example, by limiting derived-model descent to the concrete
response subtype or otherwise avoiding expanding all siblings when visiting a
base like Animal) while keeping the existing visibility filtering logic in
visibility.ts intact.
2 participants
Overview
Fixes #(issue)
Notes for reviewer
Summary by CodeRabbit
New Features
Bug Fixes