Skip to content
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.

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

[typescript-fetch] Fixed issue where unique arrays (sets) of primitive values aren't initialized properly #19521

Conversation

davidomid
Copy link
Contributor

@davidomid davidomid commented Sep 4, 2024

Resolves #19520

This PR aims to fix the above bug, by ensuring that set properties from API responses are correctly initialized as sets instead of being passed directly as arrays.

Explanation of the modelGeneric.mustache change

isPrimitiveType appears to be true for arrays which contain primitives, not just for primitives themselves. Because of the template conditions, this means that arrays of primitives were being passed verbatim from the API response.

This logic is fine for regular arrays, but for sets this causes a problem, as the values aren't being converted to a Set after being retrieved from the API. This leads to issues where the client thinks the property is a Set, but it's actually an array. The outcome of this is that accessing Set members such as size won't work at runtime, but trying to access length will cause a TypeScript compilation error.

This PR aims to fix this problem by adjusting the logic in modelGeneric.mustache to correctly handle arrays of primitives which contain unique items.

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.

@davidomid davidomid force-pushed the users/davidomid/primitive-array-set-fix branch from b91326c to e9ab486 Compare September 4, 2024 01:59
@davidomid davidomid changed the title [typescript-fetch] Fix for https://github.com/OpenAPITools/openapi-generator/issues/19520 [typescript-fetch] Fixed issue where unique arrays (sets) of primitive values aren't assigned properly Sep 4, 2024
@davidomid davidomid changed the title [typescript-fetch] Fixed issue where unique arrays (sets) of primitive values aren't assigned properly [typescript-fetch] Fixed issue where unique arrays (sets) of primitive values aren't initialized properly Sep 4, 2024
@davidomid davidomid marked this pull request as ready for review September 5, 2024 11:47
@davidomid
Copy link
Contributor Author

davidomid commented Sep 5, 2024

As it's my first time contributing to this project, I'm a bit unsure on the best way of testing this change. I think ideally it would be great if there were some assertions for validating that Set members (size, has, etc.) are actually accessible on a generated Set property, as that's the bug being fixed. After reading the docs in this repository, I'm still a bit confused about how/where a test like that is meant to be written and would really appreciate some guidance.

cc: @TiFu @taxpon @sebastianhaas @kenisteward @Vrolijkx @macjohnny @topce @akehir @petejohansonxo @amakhrov @davidgamero @mkusaka as the technical committee for this area.

@macjohnny
Copy link
Member

@davidomid you can add a test of the generated typescript, running typescript code, in https://github.com/OpenAPITools/openapi-generator/blob/master/samples/client/petstore/typescript-fetch/tests/default/test/PetApi.ts

or alternatively, you can add a java test in https://github.com/OpenAPITools/openapi-generator/blob/master/modules/openapi-generator/src/test/java/org/openapitools/codegen/typescript/fetch/TypeScriptFetchModelTest.java

but in this case, i guess its fine without writing a new test, since the generated samples are used as manual verification by human reviewers

Copy link
Member

@macjohnny macjohnny left a comment

Choose a reason for hiding this comment

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

thanks for your contribution!

the type in the interface is Set, so your change fixes that in the deserialization:

@macjohnny macjohnny merged commit 2bc0e5f into OpenAPITools:master Sep 5, 2024
15 checks passed
@wing328 wing328 added this to the 7.9.0 milestone Oct 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] [typescript-fetch] Unique item arrays (i.e. sets) containing primitive types aren't initialized properly
3 participants