[typescript][typescript-node] Serialize/deserialize maps and nullable types correctly #19362

simon-abbott · 2024-08-15T18:11:57Z

There was an edge case in the ObjectSerializer in both the typescript and typescript-node generators where schema objects (object types defined in schema.components) were not properly deserialized if they were contained within a map (an object with additionalProperties) or a nullable array (an array where every entry is either a schema object or null). In these cases the ObjectSerializer would short-circuit and not actually deserialize the object. This results in the snake_cased keys in the API not being converted to the camelCase variants actually documented in the generated typescript types.

I have included tests that cover this (and other) potential encoding and decoding edge cases. I'm sure I didn't cover every possible example, but I did confirm that as-written it will prevent a regression of this issue.

Note: I also removed the TS config setting suppressImplicitAnyIndexErrors from the templates. This was done because suppressImplicitAnyIndexErrors is deprecated, and trying to build with it causes the typescript compiler to error, which was blocking the typescript-node tests from running.

PR checklist

Read the contribution guidelines.
Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
Run the following to build the project and update samples:
```
./mvnw clean package 
./bin/generate-samples.sh ./bin/configs/*.yaml
./bin/utils/export_docs_generators.sh
```
(For Windows users, please run the script in Git BASH)
Commit all changed files.
This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
These must match the expectations made by your contribution.
You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/configs/java*.
IMPORTANT: Do NOT purge/delete any folders/files (e.g. tests) when regenerating the samples as manually written tests may be removed.
File the PR against the correct branch: master (upcoming 7.6.0 minor release - breaking changes with fallbacks), 8.0.x (breaking changes without fallbacks)
If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

Mentioning the TS committee members as per the checklist:

@TiFu @taxpon @sebastianhaas @kenisteward @Vrolijkx @macjohnny @topce @akehir @petejohansonxo @amakhrov @davidgamero @mkusaka

This fixes deserializing of models that are composed with null, undefined, or are inside of a map.

macjohnny

thanks for your contribution! can you please update the code according to the comments (and apply the same changes also to the other parts of your PR)?

macjohnny · 2024-08-19T12:33:44Z

modules/openapi-generator/src/main/resources/typescript-node/models.mustache

@@ -68,6 +68,16 @@ let typeMap: {[index: string]: any} = {
    {{/models}}
 }

+// Check if a string starts with another string without using es6 features
+function startsWith(str: string, match: string): boolean {
+    return str.lastIndexOf(match, 0) === 0;


why not indexOf?

I was using the same logic that the existing code uses. But now that you mention it indexOf would make more sense.

That being said, is it really necessary to preserve pre-es6 compatibility here? startsWith and endsWith have been supported in Node.js and every major browser since 2015.

macjohnny · 2024-08-19T12:34:23Z

modules/openapi-generator/src/main/resources/typescript-node/models.mustache

+
+// Check if a string ends with another string without using es6 features
+function endsWith(str: string, match: string): boolean {
+    return str.length > match.length && str.substring(str.length - match.length) === match;


Suggested change

return str.length > match.length && str.substring(str.length - match.length) === match;

return str.length >= match.length && str.substring(str.length - match.length) === match;

Good catch. It would be fine without it because all of the matches being passed in are strict subsets (there is guaranteed to be more string than match), but still good to be correct!

macjohnny · 2024-08-26T13:56:38Z

modules/openapi-generator/src/main/resources/typescript-node/models.mustache

+            if (data === null) {
+                return data
+            } else {
+                let subType: string = type.substring(0, type.length - 7); // Type | null => Type


Suggested change

let subType: string = type.substring(0, type.length - 7); // Type | null => Type

let subType: string = type.substring(0, type.length - " | null".length); // Type | null => Type

and i would extract " | null" to a separate constant

similar for the below undefined case

macjohnny · 2024-08-26T14:04:07Z

modules/openapi-generator/src/main/resources/typescript-node/models.mustache

@@ -118,6 +142,17 @@ export class ObjectSerializer {
                transformedData.push(ObjectSerializer.serialize(datum, subType));
            }
            return transformedData;
+        } else if (startsWith(type, '{ [key: string]: ')) {
+            let subType: string = type.replace('{ [key: string]: ', ''); // { [key: string]: Type; } => Type; }


I would suggest a regex here. can Type be a map again, i.e. contain { [key: string]:?

I would suggest a regex here.

Why a regex? I don't see how that would improve this code at all. Though now that I think about this more a .slice would probably be more concise anyway.

can Type be a map again, i.e. contain { [key: string]:?

Yes, which is fine because this code is recursive.

simon-abbott · 2024-08-30T17:03:22Z

@macjohnny I've updated the PR to use string constants instead of magic numbers. Please re-review when you have time.

macjohnny · 2024-09-17T11:53:44Z

@simon-abbott can you please re-generate the samples?

simon-abbott · 2024-09-25T17:27:10Z

@macjohnny Samples regenerated.

simon-abbott added 4 commits August 15, 2024 09:39

Remove deprecated suppressImplicitAnyIndexErrors property from templates

4ef52a1

Add encoding / decoding tests for the typescript generator

2408117

Add encoding / decoding tests for the typescript-node generator

d3d6710

[Typescript][Typescript Node] fix deserializing complex types

79ff9e1

This fixes deserializing of models that are composed with null, undefined, or are inside of a map.

simon-abbott changed the title ~~[Typescript][Typescript Node] Serialize/deserialize maps and nullable types correctly~~ [typescript][typescript-node] Serialize/deserialize maps and nullable types correctly Aug 16, 2024

macjohnny reviewed Aug 26, 2024

View reviewed changes

simon-abbott added 3 commits August 30, 2024 11:27

Use more sensible startsWith implementation

7372556

Remove use of magic number offsets

a521a6c

Fix endsWith bounds checking

990659b

simon-abbott requested a review from macjohnny August 30, 2024 17:03

macjohnny approved these changes Sep 17, 2024

View reviewed changes

simon-abbott added 2 commits September 25, 2024 10:13

Merge branch 'master' into deserialize-objects-correctly

d8fb161

Regenerate samples

dba4a2f

macjohnny merged commit a6581e8 into OpenAPITools:master Sep 25, 2024
4 checks passed

macjohnny added the Client: TypeScript label Sep 26, 2024

simon-abbott deleted the deserialize-objects-correctly branch September 27, 2024 21:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[typescript][typescript-node] Serialize/deserialize maps and nullable types correctly #19362

[typescript][typescript-node] Serialize/deserialize maps and nullable types correctly #19362

simon-abbott commented Aug 15, 2024 •

edited

Loading

macjohnny left a comment

macjohnny Aug 19, 2024

simon-abbott Aug 30, 2024

simon-abbott Aug 30, 2024

macjohnny Aug 19, 2024

simon-abbott Aug 30, 2024

macjohnny Aug 26, 2024

macjohnny Aug 26, 2024

simon-abbott Aug 30, 2024

simon-abbott commented Aug 30, 2024

macjohnny commented Sep 17, 2024

simon-abbott commented Sep 25, 2024

	return str.length > match.length && str.substring(str.length - match.length) === match;
	return str.length >= match.length && str.substring(str.length - match.length) === match;

	let subType: string = type.substring(0, type.length - 7); // Type \| null => Type
	let subType: string = type.substring(0, type.length - " \| null".length); // Type \| null => Type

[typescript][typescript-node] Serialize/deserialize maps and nullable types correctly #19362

[typescript][typescript-node] Serialize/deserialize maps and nullable types correctly #19362

Conversation

simon-abbott commented Aug 15, 2024 • edited Loading

PR checklist

macjohnny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simon-abbott commented Aug 30, 2024

macjohnny commented Sep 17, 2024

simon-abbott commented Sep 25, 2024

simon-abbott commented Aug 15, 2024 •

edited

Loading