fix(typescript): preserve base-properties on undiscriminated unions (TS + Python) - #17666
fix(typescript): preserve base-properties on undiscriminated unions (TS + Python)#17666devin-ai-integration[bot] wants to merge 3 commits into
Conversation
…nions Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
![nitpickybot[bot]](https://avatars.githubusercontent.com/in/2901896?s=60&v=4){kind=small}
There was a problem hiding this comment.
AI Review Summary
The PR merges undiscriminated-union baseProperties into object-like members for the TS type/schema generators and the Python v1 generators. The approach looks sound overall, but there are a few consistency gaps: the TS schema raw type uses wire keys while the parsed schema uses the casing-aware key (fine), yet noOptionalProperties handling and read/write-only variants differ between the type and schema paths; and the Python generator ignores union member extends/alias-of-object nuances in the typeddict should_export path (creating locally-exported helper classes). Also a couple of smaller correctness concerns flagged below.
- 🟡 6 warning(s)
- 🔵 2 suggestion(s)
To request another review, comment /ai-review on this pull request.
| return ( | ||
| this.noOptionalProperties | ||
| ? context.coreUtilities.zurg.objectWithoutOptionalProperties | ||
| : context.coreUtilities.zurg.object | ||
| )(basePropertySchemas).extend( | ||
| context.typeSchema.getSchemaOfNamedType(resolved.name, { isGeneratingSchema: true }) | ||
| ); | ||
| }) |
There was a problem hiding this comment.
🟡 warning
The parsed schema is built as object(baseProps).extend(memberSchema). extend in zurg produces the union of properties, but the raw key mapping for base props here uses raw: wireValue / parsed: getBasePropertyKey(...), while generateRawTypeDeclaration below emits the raw property with getPropertyKey(getWireValue(...)) — consistent. However, the ordering matters for conflicts: if a member object already declares a property with the same wire key as a base property, extend will have the member's schema win at runtime while the TS type intersection (type generator) yields A & B (intersection of both). Worth confirming duplicate wire keys between base properties and member properties can't produce a schema/type mismatch (the Python side explicitly dedupes; TS doesn't).
| return ts.factory.createParenthesizedType( | ||
| ts.factory.createIntersectionTypeNode([ | ||
| memberNode, | ||
| ts.factory.createTypeLiteralNode( | ||
| baseProperties.map((property) => | ||
| ts.factory.createPropertySignature( | ||
| undefined, | ||
| ts.factory.createIdentifier(property.name), | ||
| property.hasQuestionToken ? ts.factory.createToken(ts.SyntaxKind.QuestionToken) : undefined, | ||
| selectTypeNode(property) | ||
| ) | ||
| ) | ||
| ) | ||
| ]) | ||
| ); | ||
| } |
There was a problem hiding this comment.
🟡 warning
intersectWithBaseProperties recomputes getBasePropertyNodes(context) for every member. For a union with N members and M base properties this is N*M getReferenceToType calls per generated variant (x3 for normal/request/response). Hoist it out of the per-member loop (e.g. compute once in generateTypeNodes / lazily memoize) to avoid the redundant work.
| return false; | ||
| } | ||
| const resolved = context.type.resolveTypeReference(member.type); | ||
| return resolved.type === "named" && resolved.shape === FernIr.ShapeType.Object; |
There was a problem hiding this comment.
🟡 warning
appliesBasePropertiesToMember only accepts members whose resolved shape is Object. The Python counterpart (_get_object_type_id) also follows aliases-to-objects. Does context.type.resolveTypeReference already unwrap named aliases here? If not, TS and Python will disagree on which members receive base properties (alias-to-object members get them in Python, not TS), which means the same IR generates divergent SDKs.
| const baseRawType = ts.factory.createTypeLiteralNode( | ||
| (this.shape.baseProperties ?? []).map((property) => { | ||
| const type = context.typeSchema.getReferenceToRawType(property.valueType); | ||
| return ts.factory.createPropertySignature( | ||
| undefined, | ||
| ts.factory.createIdentifier(getPropertyKey(getWireValue(property.name))), | ||
| type.isOptional ? ts.factory.createToken(ts.SyntaxKind.QuestionToken) : undefined, | ||
| type.typeNodeWithoutUndefined | ||
| ); | ||
| }) | ||
| ); |
There was a problem hiding this comment.
🔵 suggestion
baseRawType is computed unconditionally even when no member is applicable (and even when baseProperties is empty, producing an empty type literal that's never used). Cheap, but guard it with this.shape.baseProperties?.length for clarity and to avoid pointless getReferenceToRawType calls.
| def _get_object_type_id(self, type_id: ir_types.TypeId) -> Optional[ir_types.TypeId]: | ||
| """Returns the type id of the object declaration this named type resolves to, if any.""" | ||
| shape = self._context.get_declaration_for_type_id(type_id).shape.get_as_union() | ||
| if shape.type == "object": | ||
| return type_id | ||
| if shape.type == "alias": | ||
| resolved = shape.resolved_type.get_as_union() | ||
| if resolved.type == "named": | ||
| return self._get_object_type_id(resolved.name.type_id) | ||
| return None |
There was a problem hiding this comment.
🟡 warning
_get_object_type_id recurses through named aliases with no cycle guard. A self-referential or mutually-referential alias chain (which the surrounding code explicitly acknowledges via CycleAwareMemberType) would recurse infinitely. Track visited type ids and bail out:
| def _get_object_type_id(self, type_id: ir_types.TypeId) -> Optional[ir_types.TypeId]: | |
| """Returns the type id of the object declaration this named type resolves to, if any.""" | |
| shape = self._context.get_declaration_for_type_id(type_id).shape.get_as_union() | |
| if shape.type == "object": | |
| return type_id | |
| if shape.type == "alias": | |
| resolved = shape.resolved_type.get_as_union() | |
| if resolved.type == "named": | |
| return self._get_object_type_id(resolved.name.type_id) | |
| return None | |
| def _get_object_type_id( | |
| self, type_id: ir_types.TypeId, _seen: Optional[Set[ir_types.TypeId]] = None | |
| ) -> Optional[ir_types.TypeId]: | |
| """Returns the type id of the object declaration this named type resolves to, if any.""" | |
| seen = _seen if _seen is not None else set() | |
| if type_id in seen: | |
| return None | |
| seen.add(type_id) | |
| shape = self._context.get_declaration_for_type_id(type_id).shape.get_as_union() | |
| if shape.type == "object": | |
| return type_id | |
| if shape.type == "alias": | |
| resolved = shape.resolved_type.get_as_union() | |
| if resolved.type == "named": | |
| return self._get_object_type_id(resolved.name.type_id, seen) | |
| return None |
(requires Set in the typing import)
| member_name = resolve_name(member_union.name).pascal_case.safe_name | ||
| base_property_wire_names = {get_wire_value(property.name) for property in self._base_properties} | ||
| return MemberWithBaseProperties( | ||
| class_name=f"{self._get_class_name(as_request=as_request)}{member_name}", |
There was a problem hiding this comment.
🟡 warning
class_name is f"{UnionName}{MemberName}" with no collision check. Two distinct members whose pascal-cased names collide (e.g. foo_bar and fooBar, or an existing type literally named UnionWithBasePropertiesNamedMetadata) will produce duplicate class declarations in the same file. Consider deduping/uniquifying via the context's name registry.
| if member_with_base is not None: | ||
| return AST.TypeHint(self._generate_member_with_base_properties(member_with_base)) | ||
| return self._context.get_type_hint_for_type_reference( | ||
| member.type, as_if_type_checking_import=member.is_circular_reference |
There was a problem hiding this comment.
🟡 warning
When the member is a circular reference (member.is_circular_reference), the base-properties path drops as_if_type_checking_import entirely and instead inlines a locally-declared Pydantic model whose fields reference the member's properties directly. For a genuinely recursive member this can reintroduce the import cycle / forward-ref problem the as_if_type_checking_import flag exists to avoid. Consider skipping the merge (falling back to the plain reference) when member.is_circular_reference is true, or verify with a recursive-union fixture.
| source_file=self._source_file, | ||
| class_name=member_with_base.class_name, | ||
| original_type_id=self._name.type_id, | ||
| should_export=True, |
There was a problem hiding this comment.
🔵 suggestion
should_export=True exports the synthetic per-member TypedDict into the public API surface (as seen in the regenerated __init__.py files). That's a new public name derived from an implementation detail; if it's not intentional, prefer should_export=False. Note the pydantic generator's FernAwarePydanticModel call doesn't pass an equivalent flag, so the two paths may already differ in export behavior.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Devin Review found 3 potential issues.
1 flag not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)
| members.map(({ member, ref }) => | ||
| this.intersectWithBaseProperties(context, member, ref.typeNode, "normal") | ||
| ) | ||
| ), | ||
| requestTypeNode: ts.factory.createUnionTypeNode( |
There was a problem hiding this comment.
🟡 Generated snippets omit shared fields
After intersectWithBaseProperties adds required fields, example builders still emit only the original member. Generated TypeScript and Python snippets no longer satisfy the union.
Prompt for agents
Update TypeScript and Python example/snippet generation for undiscriminated unions with base properties. TypeScript GeneratedUndiscriminatedUnionTypeImpl.buildExample and Python AbstractUndiscriminatedUnionSnippetGenerator currently build only example.singleUnionType. For an applicable object member, merge the union example's base-property values and instantiate or emit the new merged member shape. Ensure generated endpoint snippets include required base fields and use the generated Python local member class where necessary. Add seed assertions for both languages.
Was this helpful? React with 👍 or 👎 to provide feedback.
SDK Generation Benchmark ResultsComparing PR branch against median of 5 nightly run(s) on Full benchmark table (click to expand)
main (generator): generator-only time via --skip-scripts (includes Docker image build, container startup, IR parsing, and code generation — this is the same Docker-based flow customers use via |
…e base/member serde keys Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
|
Addressed in e83d682: |
|
Addressed in e83d682: base properties whose wire key already exists on the resolved member object (including extended properties, via |
|
Not fixable in this PR: |
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
1 participant
Description
Linear ticket: Refs
When an OpenAPI
oneOfhas siblingproperties/required, the importer correctly places the shared fields inUndiscriminatedUnionTypeDeclaration.baseProperties. Both the TypeScript and Python (v1) SDK generators ignored that field and emitted onlymembers, so every shared property vanished from the generated model (e.g.MedicationOrderPayloadlost 9 of 11 properties in a customer spec).Base properties are now merged into every union member that resolves to a named object declaration; other members (primitives, optionals, maps, aliases to non-objects, ...) are left untouched, and unions without
basePropertiesgenerate exactly as before.TypeScript —
GeneratedUndiscriminatedUnionTypeImpl/GeneratedUndiscriminatedUnionTypeSchemaImplGeneratedUndiscriminatedUnionTypegainsappliesBasePropertiesToMember(context, member)andgetBasePropertyKey({ propertyWireKey })so the schema generator shares the type generator's applicability/casing logic.Python (v1) —
AbstractUndiscriminatedUnionGeneratorcomputesMemberWithBaseProperties(base props first, then the member's own properties incl. extensions, deduped by wire name). The pydantic and TypedDict generators emit a local<Union><Member>class per applicable member and reference it from the union alias:Changes Made
object(baseProps).extend(memberSchema)+ raw type intersection for object-like members.serde-layeroutput to theundiscriminated-unionsts-sdk seed fixture so the schema path is covered; regeneratedundiscriminated-unionsfor ts-sdk and python-sdk.typescript/sdkandpython/sdk.Testing
type-schema-generatormock extended for the new interface methods;type-generator,type-schema-generator,sdk-generatorvitest suites pass.seed test --generator ts-sdk --fixture undiscriminated-unions(3/3) and--generator python-sdk(1/1).pnpm compile,pnpm lint:biome,pnpm formatclean;poetry run mypy .(343 files) andpoetry run pytest(399 passed) ingenerators/python.MedicationOrderPayloadnow carries all 9 shared fields on both the human and veterinary branches in TS and Python; generated Python passes mypy.Link to Devin session: https://app.devin.ai/sessions/d7d3ec59fed14ea3ab2943cc09080757
Open in Devin Desktop: https://app.devin.ai/desktop/session/d7d3ec59fed14ea3ab2943cc09080757?variant=devin