Support field names which aren't legal typescript identifiers. - #24741
Open
brendandburns wants to merge 1 commit into
Open
Support field names which aren't legal typescript identifiers.#24741brendandburns wants to merge 1 commit into
brendandburns wants to merge 1 commit into
Conversation
- Add custom TLS server name support and update generated samples.\n- Fix the missing declaration.\n- Preserve unmapped baseName fields during TypeScript serialization.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
3 issues found across 23 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="samples/openapi3/client/petstore/typescript/builds/nullable-enum/models/ObjectSerializer.ts">
<violation number="1" location="samples/openapi3/client/petstore/typescript/builds/nullable-enum/models/ObjectSerializer.ts:183">
P2: When a schema contains a `constructor` field that is absent from the input, this fallback reads inherited `data.constructor` after `_constructor` is missing. Check that `baseName` is also an own property before reading it, so absent fields remain `undefined`.</violation>
</file>
<file name="samples/openapi3/client/petstore/typescript/builds/composed-schemas/models/ObjectSerializer.ts">
<violation number="1" location="samples/openapi3/client/petstore/typescript/builds/composed-schemas/models/ObjectSerializer.ts:204">
P2: When an optional schema property is named `constructor` and has an array type, an unset model reads inherited `data.constructor` here and serialization throws because functions are not iterable. Check `baseName` ownership before falling back so missing optional fields remain `undefined`.</violation>
</file>
<file name="modules/openapi-generator/src/main/resources/typescript/model/ObjectSerializer.mustache">
<violation number="1" location="modules/openapi-generator/src/main/resources/typescript/model/ObjectSerializer.mustache:208">
P2: When the same object carries both the sanitized `name` key and the raw `baseName` key with different values, this reads `data[name]` first and silently drops the `baseName` value. That happens with property-name collisions, which the generator produces: a legal field `foo_bar` gets `name = "foo_bar"`, while an illegal field `foo-bar` is sanitized to the same `name = "foo_bar"` (see `toVarName`/`sanitizeName` in `TypeScriptClientCodegen`). The lookup on `attributeType.name` is then satisfied by the other field's value and serialized under `foo-bar`, emitting incorrect data. Since `serialize` outputs under `attributeType.baseName`, prefer the `baseName` key and fall back to `name`, which also matches the wire-format key and both file patterns for consistency.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| instance[attributeType.baseName] = ObjectSerializer.serialize(data[attributeType.name], attributeType.type, attributeType.format); | ||
| const value = Object.prototype.hasOwnProperty.call(data, attributeType.name) | ||
| ? data[attributeType.name] | ||
| : data[attributeType.baseName]; |
Contributor
There was a problem hiding this comment.
P2: When a schema contains a constructor field that is absent from the input, this fallback reads inherited data.constructor after _constructor is missing. Check that baseName is also an own property before reading it, so absent fields remain undefined.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At samples/openapi3/client/petstore/typescript/builds/nullable-enum/models/ObjectSerializer.ts, line 183:
<comment>When a schema contains a `constructor` field that is absent from the input, this fallback reads inherited `data.constructor` after `_constructor` is missing. Check that `baseName` is also an own property before reading it, so absent fields remain `undefined`.</comment>
<file context>
@@ -178,7 +178,10 @@ export class ObjectSerializer {
- instance[attributeType.baseName] = ObjectSerializer.serialize(data[attributeType.name], attributeType.type, attributeType.format);
+ const value = Object.prototype.hasOwnProperty.call(data, attributeType.name)
+ ? data[attributeType.name]
+ : data[attributeType.baseName];
+ instance[attributeType.baseName] = ObjectSerializer.serialize(value, attributeType.type, attributeType.format);
}
</file context>
Suggested change
| : data[attributeType.baseName]; | |
| : Object.prototype.hasOwnProperty.call(data, attributeType.baseName) | |
| ? data[attributeType.baseName] | |
| : undefined; |
| instance[attributeType.baseName] = ObjectSerializer.serialize(data[attributeType.name], attributeType.type, attributeType.format); | ||
| const value = Object.prototype.hasOwnProperty.call(data, attributeType.name) | ||
| ? data[attributeType.name] | ||
| : data[attributeType.baseName]; |
Contributor
There was a problem hiding this comment.
P2: When an optional schema property is named constructor and has an array type, an unset model reads inherited data.constructor here and serialization throws because functions are not iterable. Check baseName ownership before falling back so missing optional fields remain undefined.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At samples/openapi3/client/petstore/typescript/builds/composed-schemas/models/ObjectSerializer.ts, line 204:
<comment>When an optional schema property is named `constructor` and has an array type, an unset model reads inherited `data.constructor` here and serialization throws because functions are not iterable. Check `baseName` ownership before falling back so missing optional fields remain `undefined`.</comment>
<file context>
@@ -199,7 +199,10 @@ export class ObjectSerializer {
- instance[attributeType.baseName] = ObjectSerializer.serialize(data[attributeType.name], attributeType.type, attributeType.format);
+ const value = Object.prototype.hasOwnProperty.call(data, attributeType.name)
+ ? data[attributeType.name]
+ : data[attributeType.baseName];
+ instance[attributeType.baseName] = ObjectSerializer.serialize(value, attributeType.type, attributeType.format);
}
</file context>
Comment on lines
+208
to
+210
| const value = Object.prototype.hasOwnProperty.call(data, attributeType.name) | ||
| ? data[attributeType.name] | ||
| : data[attributeType.baseName]; |
Contributor
There was a problem hiding this comment.
P2: When the same object carries both the sanitized name key and the raw baseName key with different values, this reads data[name] first and silently drops the baseName value. That happens with property-name collisions, which the generator produces: a legal field foo_bar gets name = "foo_bar", while an illegal field foo-bar is sanitized to the same name = "foo_bar" (see toVarName/sanitizeName in TypeScriptClientCodegen). The lookup on attributeType.name is then satisfied by the other field's value and serialized under foo-bar, emitting incorrect data. Since serialize outputs under attributeType.baseName, prefer the baseName key and fall back to name, which also matches the wire-format key and both file patterns for consistency.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At modules/openapi-generator/src/main/resources/typescript/model/ObjectSerializer.mustache, line 208:
<comment>When the same object carries both the sanitized `name` key and the raw `baseName` key with different values, this reads `data[name]` first and silently drops the `baseName` value. That happens with property-name collisions, which the generator produces: a legal field `foo_bar` gets `name = "foo_bar"`, while an illegal field `foo-bar` is sanitized to the same `name = "foo_bar"` (see `toVarName`/`sanitizeName` in `TypeScriptClientCodegen`). The lookup on `attributeType.name` is then satisfied by the other field's value and serialized under `foo-bar`, emitting incorrect data. Since `serialize` outputs under `attributeType.baseName`, prefer the `baseName` key and fall back to `name`, which also matches the wire-format key and both file patterns for consistency.</comment>
<file context>
@@ -205,7 +205,10 @@ export class ObjectSerializer {
let instance: {[index: string]: any} = {};
for (let attributeType of attributeTypes) {
- instance[attributeType.baseName] = ObjectSerializer.serialize(data[attributeType.name], attributeType.type, attributeType.format);
+ const value = Object.prototype.hasOwnProperty.call(data, attributeType.name)
+ ? data[attributeType.name]
+ : data[attributeType.baseName];
</file context>
Suggested change
| const value = Object.prototype.hasOwnProperty.call(data, attributeType.name) | |
| ? data[attributeType.name] | |
| : data[attributeType.baseName]; | |
| const value = Object.prototype.hasOwnProperty.call(data, attributeType.baseName) | |
| ? data[attributeType.baseName] | |
| : data[attributeType.name]; |
Member
|
thanks for the PR cc @TiFu (2017/07) @taxpon (2017/07) @sebastianhaas (2017/07) @kenisteward (2017/07) @Vrolijkx (2017/09) @macjohnny (2018/01) @topce (2018/10) @akehir (2019/07) @petejohansonxo (2019/11) @amakhrov (2020/02) @davidgamero (2022/03) @mkusaka (2022/04) @joscha (2024/10) @KannaKim (2026/07) |
Contributor
Author
|
@wing328 do we need anything more here before this can get merged? |
Member
|
@brendandburns did you have a chance to review the feedback from cubic-dev-ai? are those feedbacks valid? |
Contributor
There was a problem hiding this comment.
Copilot review overview
Review tier: Balanced
Findings: 2
New issues introduced by this change (3)
| Severity | Finding |
|---|---|
 |
modules/openapi-generator/src/main/resources/typescript/model/ObjectSerializer.mustache — The fallback must also require baseName to be an own property. For example, this generator… |
|
modules/openapi-generator/src/main/resources/typescript-node/models.mustache — The new fallback reads inherited values when baseName is not an own property. A schema/name… |
 |
modules/openapi-generator/src/test/resources/integrationtests/typescript/objectsWithEnums-expected/model/models.ts — This golden-file update does not exercise the fallback: the objectsWithEnums spec only has… |
What changed in this PR
Updates TypeScript serializers to preserve wire-format fields with names that are invalid TypeScript identifiers, addressing Kubernetes client issue #2962.
Changes:
- Falls back from generated property names to JSON
baseNamevalues. - Regenerates affected TypeScript samples and integration expectations.
| File | Description |
|---|---|
samples/openapi3/client/petstore/typescript/builds/object_params/models/ObjectSerializer.ts |
Regenerates serializer fallback. |
samples/openapi3/client/petstore/typescript/builds/nullable-enum/models/ObjectSerializer.ts |
Regenerates serializer fallback. |
samples/openapi3/client/petstore/typescript/builds/jquery/models/ObjectSerializer.ts |
Regenerates serializer fallback. |
samples/openapi3/client/petstore/typescript/builds/inversify/models/ObjectSerializer.ts |
Regenerates serializer fallback. |
samples/openapi3/client/petstore/typescript/builds/explode-query/models/ObjectSerializer.ts |
Regenerates serializer fallback. |
samples/openapi3/client/petstore/typescript/builds/deno/models/ObjectSerializer.ts |
Regenerates serializer fallback. |
samples/openapi3/client/petstore/typescript/builds/deno_object_params/models/ObjectSerializer.ts |
Regenerates serializer fallback. |
samples/openapi3/client/petstore/typescript/builds/default/models/ObjectSerializer.ts |
Regenerates serializer fallback. |
samples/openapi3/client/petstore/typescript/builds/composed-schemas/models/ObjectSerializer.ts |
Regenerates serializer fallback. |
samples/openapi3/client/petstore/typescript/builds/browser/models/ObjectSerializer.ts |
Regenerates serializer fallback. |
samples/client/petstore/typescript-node/npm/model/models.ts |
Updates generated Node serializer. |
samples/client/petstore/typescript-node/default/model/models.ts |
Updates generated Node serializer. |
samples/client/petstore/typescript-node/3_0/model/models.ts |
Updates generated Node serializer. |
samples/client/others/typescript/encode-decode/build/models/ObjectSerializer.ts |
Regenerates serializer fallback. |
samples/client/others/typescript/builds/with-unique-items/models/ObjectSerializer.ts |
Regenerates serializer fallback. |
samples/client/others/typescript/builds/null-types-simple/models/ObjectSerializer.ts |
Regenerates serializer fallback. |
samples/client/others/typescript/builds/enum-single-value/models/ObjectSerializer.ts |
Regenerates serializer fallback. |
samples/client/others/typescript/builds/array-of-lists/models/ObjectSerializer.ts |
Regenerates serializer fallback. |
samples/client/others/typescript-node/encode-decode/build/model/models.ts |
Updates generated Node serializer. |
samples/client/echo_api/typescript/build/models/ObjectSerializer.ts |
Regenerates serializer fallback. |
modules/openapi-generator/src/test/resources/integrationtests/typescript/objectsWithEnums-expected/model/models.ts |
Updates integration golden output. |
modules/openapi-generator/src/main/resources/typescript/model/ObjectSerializer.mustache |
Implements TypeScript fallback logic. |
modules/openapi-generator/src/main/resources/typescript-node/models.mustache |
Implements TypeScript Node fallback logic. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+208
to
+210
| const value = Object.prototype.hasOwnProperty.call(data, attributeType.name) | ||
| ? data[attributeType.name] | ||
| : data[attributeType.baseName]; |
Comment on lines
+171
to
+173
| const value = Object.prototype.hasOwnProperty.call(data, attributeType.name) | ||
| ? data[attributeType.name] | ||
| : data[attributeType.baseName]; |
Comment on lines
+98
to
+101
| const value = Object.prototype.hasOwnProperty.call(data, attributeType.name) | ||
| ? data[attributeType.name] | ||
| : data[attributeType.baseName]; | ||
| instance[attributeType.baseName] = ObjectSerializer.serialize(value, attributeType.type); |
Member
|
@brendandburns please also take a look at the feedback from copilot when you've time. thank you. |
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
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.
See:
kubernetes-client/javascript#2962
Summary by cubic
Preserves fields whose JSON names aren’t legal TypeScript identifiers by falling back to the field’s baseName during serialization. Previously,
ObjectSerializeronly readattributeType.name, which dropped values stored underbaseName.typescript-node/models.mustacheandtypescript/model/ObjectSerializer.mustacheto usedata[name]when it’s an own property; otherwise usedata[baseName]. Samples and one test expectation updated accordingly.nameandbaseNameexist,namewins. Uses own-property check to avoid prototype pollution. Deserialization is unchanged.Written for commit 8505f22. Summary will update on new commits.