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

Fix: Always serialise Map and Set to object and array in typescript-axios #17790

Conversation

adrianhelvikspond
Copy link
Contributor

@adrianhelvikspond adrianhelvikspond commented Feb 5, 2024

This produces type definitions that aren't compatible with the typescript-axios client:

        someField:
          uniqueItems: true
          type: array
          items:
            type: string

My fix serialises of Sets into arrays and Maps into objects, since otherwise they are not serialized with JSON.stringify, see https://stackoverflow.com/questions/31190885/json-stringify-a-set

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.1.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 @taxpon @sebastianhaas @kenisteward @Vrolijkx @macjohnny @topce @akehir @petejohansonxo @amakhrov @davidgamero @mkusaka

@adrianhelvikspond
Copy link
Contributor Author

Fixes the serialization part of #11746

@wing328
Copy link
Member

wing328 commented Feb 7, 2024

can you please follow step 3 to update the samples?

@adrianhelvikspond
Copy link
Contributor Author

adrianhelvikspond commented Feb 7, 2024

Done!

Sorry about missing the samples. Step 3 messed up some samples for all languages on Mac. Worked fine with Docker though. This script might be useful for Mac users if it happens to others:

docker run -it --rm --workdir /project -v $PWD:/project sapmachine:21-jdk-ubuntu sh -c './mvnw clean package && ./bin/generate-samples.sh ./bin/configs/*.yaml && ./bin/utils/export_docs_generators.sh'

@adrianhelvikspond
Copy link
Contributor Author

I replaced Object.entries and Object.fromEntries with ponyfills that are compatible with the target environment.

@wing328
Copy link
Member

wing328 commented Feb 28, 2024

can you please merge the latest master into your branch as the ts axios test failures have been fixed?

@adrianhelvikspond adrianhelvikspond force-pushed the fix-incorrect-serialisation-of-maps-and-sets branch from 4c41422 to 21794c0 Compare March 15, 2024 16:36
@adrianhelvikspond
Copy link
Contributor Author

Rebased from master now.

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.

LGTM

@macjohnny
Copy link
Member

@adrianhelvikspond can you fix the build issue?

@adrianhelvikspond
Copy link
Contributor Author

adrianhelvikspond commented Mar 18, 2024

I updated the tsconfig so it downlevels iterators to Symbol.iterator, which is needed for map iterators.

@@ -4,6 +4,7 @@
"target": "ES5",
"module": "commonjs",
"noImplicitAny": true,
"downlevelIteration": true,
Copy link
Member

Choose a reason for hiding this comment

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

can you rewrite the code so it does not need this change? some folks might be using the generated code without the generated tsconfig.json

Copy link
Contributor Author

@adrianhelvikspond adrianhelvikspond Mar 19, 2024

Choose a reason for hiding this comment

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

Yep. I'll use forEach instead. Shouldn't expose any details about iterators, and makes it compatible with IE11 without a Symbol.iterator polyfill.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've made the changes and updated the documentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And now I've rebased from master again.

@adrianhelvikspond adrianhelvikspond force-pushed the fix-incorrect-serialisation-of-maps-and-sets branch from 44c90da to c017caf Compare March 19, 2024 05:13
@adrianhelvikspond adrianhelvikspond force-pushed the fix-incorrect-serialisation-of-maps-and-sets branch from 6651ccc to 921ae3c Compare March 19, 2024 05:20
@macjohnny macjohnny merged commit 8152052 into OpenAPITools:master Mar 19, 2024
17 checks passed
zapodot pushed a commit to zapodot/openapi-generator that referenced this pull request Mar 21, 2024
@pauliusg
Copy link

pauliusg commented Apr 17, 2024

@macjohnny, @wing328, @adrianhelvikspond this introduced new issue. Now Date object is serialized to {}. Request:

{ testDate: new Date() }

After serialization becomes:

{ testDate: {} }

This might happen not only with dates. Looks like you have to check if object has toJSON method. And if has, execute it for final value: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/JSON/stringify

Had to downgrade from v7.5.0-SNAPSHOT to v7.4.0.

@wing328 wing328 added this to the 7.5.0 milestone Apr 17, 2024
@macjohnny
Copy link
Member

@adrianhelvikspond do you want to fix this?

@macjohnny
Copy link
Member

I reverted this PR in #18452
@pauliusg thanks for reporting the issue!

@ckoegel
Copy link
Contributor

ckoegel commented Apr 23, 2024

This PR also changed the response for binary media, prior to 7.5.0, my response looked like:

{
      type: 'Buffer',
      data: [
        255, 216, 255, 224,  0,  16,  74, 70,  73,  70,  0,  1,
          1,   0,   0,   1,  0,   1,   0,  0, 255, 219,  0, 67,
        ... 32003 more items
      ]
    }

but it now appears as:

{
      '0': 255,
      '1': 216,
      '2': 255,
      '3': 224,
      '4': 0,
      '5': 16,
      '6': 74,
...
}
This makes it more difficult to write this data to a file. Reverting this PR helps for the time being, but we'll still need a solution for the Maps and Sets to be serialized properly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants