Skip to content

[typescript-fetch] to fix incorrect parsing with additional properties #20923

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
macjohnny merged 4 commits into from
Mar 24, 2025
+103 −16
Merged

[typescript-fetch] to fix incorrect parsing with additional properties #20923

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e950e2c
Fix issue #20195
DavidGrath Mar 18, 2025
4c82037
Ran export_docs_generators.sh
DavidGrath Mar 18, 2025
0d6041f
Generated path separators fixed
DavidGrath Mar 19, 2025
3207c6b
Trigger Build
DavidGrath Mar 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion modules/openapi-generator/src/main/resources/typescript-fetch/apis.mustache
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -265,7 +265,7 @@ export class {{classname}} extends runtime.BaseAPI {
{{/isEnumRef}}
{{^isEnumRef}}
{{^withoutRuntimeChecks}}
formParams.append('{{baseName}}', new Blob([JSON.stringify({{{dataType}}}ToJSON(requestParameters['{{paramName}}']))], { type: "application/json", }));
formParams.append('{{baseName}}', new Blob([JSON.stringify({{returnType}}ToJSON(requestParameters['{{paramName}}']))], { type: "application/json", }));
Copy link
Contributor

@joscha joscha Mar 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we add a fixture (generated sample) to the repository that shows this change? Or have the samples not been regenerated?

Copy link
Contributor Author

@DavidGrath DavidGrath Mar 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the only real changes I got from running generate-samples were CRLF/LF changes.
Tried again recently and this is pretty much all the output:
image

Copy link
Contributor Author

@DavidGrath DavidGrath Mar 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joscha Okay, so I retried on WSL and it seems the package-lock.json was the only real change

Copy link

@paras-fundwave paras-fundwave Jul 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems to be only working when additionalProperties exists

it is causing an issue where the param (paramName here) has it's own schema

for the case shown below, it puts in RequestPayload but instead it should be RequestContext

e.g.

requestBody:
  description: entity for which custom field will be added such as fund, deal, asset, investor etc
  content:
    multipart/form-data:
      schema:
        $ref: "#/components/schemas/RequestPayload"

...

RequestPayload:
  type: object
  properties:
    context:
      $ref: "#/components/schemas/RequestContext"
  ...

RequestContext:
  type: object
  properties:
    ...

@macjohnny @joscha

Copy link
Member

@macjohnny macjohnny Jul 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@paras-fundwave thanks for reporting! would you want to send a PR to fix that?

Copy link

@paras-fundwave paras-fundwave Jul 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure thing!

while I'll familiarise myself with the codebase, can you help confirm the approach I have in mind

we'll need to have 2 outputs (for this line) depending on whether additionalProperties exists

from this PR I can see I'll need to add test-cases too, anything else I'm missing that can help but might not be mentioned in the contributing.md?

Copy link
Member

@macjohnny macjohnny Jul 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sg, no objections, although i didnt look into the issue in detail

Copy link

@paras-fundwave paras-fundwave Jul 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like how datatype is processed has changed too in b/w v7.0.0 and v7.1.0

reverting to dataType is still leads to malformed code - broken v7.1.0 onwards

Copy link
Contributor Author

@DavidGrath DavidGrath Jul 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see. It seems like an inverted mustache section might come in handy. Were you able to spot any other issues around this? Thanks for pointing this out

Copy link
Contributor

@adrianhj adrianhj Jul 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seeing similar issues on 7.14.0 with a discriminated schema where returnType is null and ends up generating invalid code: new Blob([JSON.stringify(ToJSON(...)], ...), dataType generates as expected with new Blob([JSON.stringify(MyDiscriminatedSchemaToJSON(...)], ...)

7.12.0 generates OK, 7.13.0 onwards is broken.

I haven't read up on the original issue fully -- but as the line in question is part of building up the request, using the return type for serializing request parameters seems wrong?

Copy link
Contributor

@adrianhj adrianhj Jul 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Diving into the test I don't think the assertion is right.

The schema for the request body is:

"multipart/form-data": {
  "schema": {
    "type": "object",
    "properties": {
      "paRes": {
        "type": "object",
        "additionalProperties": {
          "type": "string"
        },
        "description": "Form sent for Authentication"
      }
    }
  },
  "encoding": {
    "paRes": {
      "style": "form"
    }
  }
}

ResponseOfString is a schema used as the response:

"responses": {
  "200": {
    "description": "OK",
    "content": {
      "application/json": {
        "schema": {
          "$ref": "#/components/schemas/ResponseOfString"
        }
      }
    }
  }
}

The code that gets generated to append the part for the form in the request:

if (requestParameters['paRes'] != null) {
            formParams.append('paRes', new Blob([JSON.stringify(ResponseOfStringToJSON(requestParameters['paRes']))], { type: "application/json", }));
                    }

But that is surely wrong, as ResponseOfString is the response of the operation, not related to the request body?

I have raised a PR to revert: #21542 (FYI @macjohnny)

{{/withoutRuntimeChecks}}{{#withoutRuntimeChecks}}
formParams.append('{{baseName}}', new Blob([JSON.stringify(requestParameters['{{paramName}}'])], { type: "application/json", }));
{{/withoutRuntimeChecks}}
Expand Down
25 changes: 25 additions & 0 deletions ...test/java/org/openapitools/codegen/typescript/fetch/TypeScriptFetchClientCodegenTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -347,6 +347,31 @@ public void testGeneratedFilenamesInCamelCaseWithAdditionalModelPrefix() throws
TestUtils.assertFileExists(Paths.get(output + "/apis/petControllerApi.ts"));
}

@Test(description = "Issue #20195")
public void givenObjectHasAdditionalPropertiesWhenGenerateThenIndexSignatureNotUsedToGenerateMethodName() throws IOException {
File output = Files.createTempDirectory("test").toFile().getCanonicalFile();
output.deleteOnExit();

TypeScriptFetchClientCodegen clientCodegen = new TypeScriptFetchClientCodegen();
clientCodegen.setWithoutRuntimeChecks(false);
clientCodegen.setOutputDir(output.getAbsolutePath());

Map<String, Object> properties = new HashMap<>();
properties.put(TypeScriptFetchClientCodegen.WITH_INTERFACES, true);
properties.put(CodegenConstants.ENUM_PROPERTY_NAMING, "original");
clientCodegen.additionalProperties().putAll(properties);

DefaultGenerator defaultGenerator = new DefaultGenerator();
defaultGenerator.opts(
new ClientOptInput().openAPI(TestUtils.parseSpec("src/test/resources/bugs/issue_20195.json"))
.config(clientCodegen)
).generate();

String outputPath = output.getAbsolutePath();
Path exampleApiPath = Paths.get(outputPath + "/apis/ExampleApi.ts");
TestUtils.assertFileContains(exampleApiPath, "new Blob([JSON.stringify(ResponseOfStringToJSON");
}

private static File generate(Map<String, Object> properties) throws IOException {
File output = Files.createTempDirectory("test").toFile();
output.deleteOnExit();
Expand Down
77 changes: 77 additions & 0 deletions modules/openapi-generator/src/test/resources/bugs/issue_20195.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
{
"openapi": "3.0.1",
"info": {
"title": "Sample API",
"description": "This is sample api",
"version": "v1"
},
"paths": {
"/example_api_101": {
"post": {
"tags": [
"Example"
],
"summary": "Process Auth",
"parameters": [
{
"name": "Id",
"in": "query",
"schema": {
"type": "integer",
"format": "int32"
}
}
],
"requestBody": {
"content": {
"multipart/form-data": {
"schema": {
"type": "object",
"properties": {
"paRes": {
"type": "object",
"additionalProperties": {
"type": "string"
},
"description": "Form sent for Authentication"
}
}
},
"encoding": {
"paRes": {
"style": "form"
}
}
}
}
},
"responses": {
"200": {
"description": "OK",
"content": {
"application/json": {
"schema": {
"$ref": "#/components/schemas/ResponseOfString"
}
}
}
}
}
}
}
},
"components": {
"schemas": {
"ResponseOfString": {
"type": "object",
"properties": {
"message": {
"type": "string",
"nullable": true
}
},
"additionalProperties": false
}
}
}
}
15 changes: 0 additions & 15 deletions samples/client/petstore/typescript-fetch/tests/default/package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading