[bugfix] [typescript-fetch] add oneOf string union & array support (#19909) #20193

thesn10 · 2024-11-27T01:30:45Z

Fixes #19909

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 || exit
./bin/generate-samples.sh ./bin/configs/*.yaml || exit
./bin/utils/export_docs_generators.sh || exit
```
(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.x.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.

@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)

…ls#19909)

wing328 · 2024-11-27T09:11:45Z

modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultCodegen.java

+    private static String getEnumStringLiteralUnionType(Schema interfaceSchema) {
+        return (String) interfaceSchema.getEnum().stream()
+                .map(enumName -> "\"" + (String)enumName + "\"")
+                .collect(Collectors.joining(" | "));


is that the same as https://github.com/OpenAPITools/openapi-generator/blob/master/modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/AbstractTypeScriptClientCodegen.java#L1123 ?

if overriding toOneOfName won't meet your requirement, can you please elaborate?

cc @OpenAPITools/generator-core-team

@wing328 Yea, they are pretty similar. The core difference is that toOneOfName seems to build the whole oneOf type, while mine only builds the string literal part. It would be nice to refactor them both into one method.

The Problem is, toOneOfName seems to be completely unused in the oneOf model generation.
Currently, the different oneOf union parts are just put in an array and are appended together inside the mustache, not using toOneOfName at all.

Refactoring it to build the full oneOf type inside the toOneOfName instead of inside the mustache seems to be a major change to how the code generation works and i need some input if this is the correct way, or if i misunderstood something, as i am new to the codebase

joscha · 2024-11-28T09:24:43Z

samples/client/petstore/typescript-fetch/builds/oneOf/apis/DefaultApi.ts

+    /**
+     */
+    async testArrayRaw(initOverrides?: RequestInit | runtime.InitOverrideFunction): Promise<runtime.ApiResponse<TestArrayResponse>> {
+        const queryParameters: any = {};


can this be at least:

Suggested change

const queryParameters: any = {};

const queryParameters: Record<string, any> = {};

?

Yes, in fact, it can even be runtime.HTTPQuery

But i think this is outside of the scope of this PR as I did not make any changes to the api mustache, i just added a new testcase.
This PR is only about fixing the model mustache, not improving the api mustache, so I would recommend opening a new PR for this

ah, sorry, I assumed because it was a newly generated block, it originated from your change, too.

joscha · 2024-11-28T09:29:37Z

samples/client/petstore/typescript-fetch/builds/oneOf/models/TestArrayResponse.ts

+    if (!(json instanceof Object)){
+        return {} as any;
+    }
+    if (Array.isArray(json) && json.every(item => item instanceof String)) {


if these were type guards, Typescript can infer without the cast.
In this case for example:

function isStringArray(x: any): x is Array<string> { return Array.isArray(x) && x.every(item => item instanceof String); }

see playground

Yes great idea, i'll implement it

item instanceof String is incorrect. you need typeof item === 'string' instead.
you can check it in the console:

> "" instanceof String < false > typeof "" === 'string' < true

and also modern TS (5.5 or higher) is smart enough to infer type guards, you don't need a custom type guard here.
here is a modified playground: https://www.typescriptlang.org/play/?moduleResolution=99&target=7&module=6&resolvePackageJsonImports=true&allowArbitraryExtensions=false#code/MYewdgzgLgBAHjAvDA2gIgIZoDRoEZoC6MGEJYAngNwBQNAlgGYAUAggE7sYUB09EHLhWZwAlDABkE+DwCmAN1nth9KLIC2SAHwwoFAA6yQjGKo1JEyAOTR29MAHMro8QG8aMT-A9eA9L5gAPQB+GgBfIA

joscha · 2024-11-28T09:32:27Z

samples/client/petstore/typescript-fetch/builds/oneOf/models/TestArrayResponse.ts

+    return {} as any;
+}
+
+export function TestArrayResponseToJSON(json: any): any {


can the return type be more narrow here?

I think the best we can do is runtime.Json but this is also just an alias for any so not really helpful.

Also I was also wondering why we have two methods: one typed named ToJSONTyped and one untyped named ToJSON. Do you think we can remove the untyped ToJSON?

I am not sure why, maybe a backwards compat thing? We can alias one to the other. Anything but unknown and never is a subsitute for any, so as long as we don't produce that we can remove the untyped variant implementation?

^ @macjohnny probably knows more?

joscha · 2024-11-28T09:32:31Z

samples/client/petstore/typescript-fetch/builds/oneOf/models/TestArrayResponse.ts

+    return TestArrayResponseToJSONTyped(json, false);
+}
+
+export function TestArrayResponseToJSONTyped(value?: TestArrayResponse | null, ignoreDiscriminator: boolean = false): any {


joscha · 2024-11-28T09:34:48Z

samples/client/petstore/typescript-fetch/builds/oneOf/models/TestResponse.ts

+    }
+    if (!(value instanceof Object)){
+        return {};
+    }
    if (instanceOfTestA(value)) {


Independent from your changes:

mental note to look at the generated instanceOf... functions to make them type guards, so we can lose some casts.

amakhrov · 2024-11-29T00:26:53Z

samples/client/petstore/typescript-fetch/builds/oneOf/models/TestArrayResponse.ts

+    if (Array.isArray(json) && json.every(item => item instanceof String)) {
+        return json as Array<string>;
+    }
+    if (Array.isArray(json) && json.every(item => item instanceof Object)) {


same here: instanceof Object is probably not what you need.
e.g. [] instanceof Object // true
use typeof instead

amakhrov · 2024-11-29T00:29:40Z

samples/client/petstore/typescript-fetch/builds/oneOf/models/TestArrayResponse.ts

+        return json as Array<string>;
+    }
+    if (Array.isArray(json) && json.every(item => item instanceof Object)) {
+        if (json.every(item => instanceOfTestA(item as Object))) {


once you have changed instanceof Object to typeof x === 'object, you will not need this type assertion (as Object)

thesn10 added 2 commits November 27, 2024 02:23

[typescript-fetch] add oneOf string union & array support (OpenAPIToo…

17dac43

…ls#19909)

[typescript-fetch] add oneOf string union & array support (OpenAPIToo…

2dd6b47

…ls#19909)

wing328 reviewed Nov 27, 2024

View reviewed changes

joscha reviewed Nov 28, 2024

View reviewed changes

amakhrov reviewed Nov 29, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[bugfix] [typescript-fetch] add oneOf string union & array support (#19909) #20193

[bugfix] [typescript-fetch] add oneOf string union & array support (#19909) #20193

thesn10 commented Nov 27, 2024

wing328 Nov 27, 2024 •

edited

Loading

thesn10 Nov 27, 2024

joscha Nov 28, 2024

thesn10 Nov 28, 2024

joscha Nov 28, 2024

joscha Nov 28, 2024

thesn10 Nov 28, 2024

amakhrov Nov 29, 2024

amakhrov Nov 29, 2024 •

edited

Loading

joscha Nov 28, 2024

thesn10 Nov 28, 2024

joscha Nov 28, 2024

joscha Nov 28, 2024

joscha Nov 28, 2024

joscha Nov 28, 2024 •

edited

Loading

amakhrov Nov 29, 2024

amakhrov Nov 29, 2024

	const queryParameters: any = {};
	const queryParameters: Record<string, any> = {};

[bugfix] [typescript-fetch] add oneOf string union & array support (#19909) #20193

Are you sure you want to change the base?

[bugfix] [typescript-fetch] add oneOf string union & array support (#19909) #20193

Conversation

thesn10 commented Nov 27, 2024

PR checklist

wing328 Nov 27, 2024 • edited Loading

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

amakhrov Nov 29, 2024 • edited Loading

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

joscha Nov 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wing328 Nov 27, 2024 •

edited

Loading

amakhrov Nov 29, 2024 •

edited

Loading

joscha Nov 28, 2024 •

edited

Loading