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

[POWERSHELL] fix: keep array context when converting to json #19535

Merged
merged 1 commit into from
Oct 3, 2024
Merged

[POWERSHELL] fix: keep array context when converting to json #19535

merged 1 commit into from
Oct 3, 2024

Conversation

syd-besco
Copy link
Contributor

@syd-besco syd-besco commented Sep 5, 2024

This PR aims to correct Issue #18427 . The fix submitted by @condorcorde in PR #19262 seems to give a different behavior than the one expected.

When using $LocalVarBodyParameter = ,$User | ConvertTo-Json -Depth 100, $User being a 1-element array, the output will look something like this :
{ "Value" : _content of $User_, "Length" : 1 }

Which is different from the expected behavior, that is to only get the array itself. This PR fixes this by forcing the cast of $User to an array using the @() syntax before converting it to Json.

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. @wing328

@syd-besco syd-besco marked this pull request as ready for review September 5, 2024 13:45
@wing328
Copy link
Member

wing328 commented Sep 7, 2024

Thanks for the PR but your commit (as shown in the Commits tab) is not linked to your Github account, which means this PR won't count as your contribution in https://github.com/OpenAPITools/openapi-generator/graphs/contributors.

Let me know if you need help fixing it.

Ref: https://github.com/OpenAPITools/openapi-generator/wiki/FAQ#how-can-i-update-commits-that-are-not-linked-to-my-github-account

@wing328
Copy link
Member

wing328 commented Sep 7, 2024

@syd-besco thanks for the PR

@canadaduane can you please review and test the PR locally when you've time?

@condorcorde
Copy link
Contributor

condorcorde commented Sep 9, 2024 via email

@condorcorde
Copy link
Contributor

For the record, this link provides an explanation of the issue: Why does powershell give different result in one-liner than two-liner when converting JSON?

The System.Array type - the base type for all arrays - has a .Count property defined for it via PowerShell's ETS (Extended Type System - see Get-Help about_Types.ps1xml) in Windows PowerShell, which causes ConvertTo-Json to include that property in the JSON string it creates, with the array elements included in a sibling value property.

The problem will continue to plague Windows PowerShell (whose latest and last version is v5.1), but PowerShell (Core) 7+ is not affected.

@syd-besco
Copy link
Contributor Author

Hi, thank you @condorcorde for your answers.

The fix I submitted aims to fix this issue for both versions, so I think it would be preferable to include it in the next release of OpenApi generator.
Regarding the other issue you submitted on this thread, maybe creating a proper issue will help solving and allow @wing328 to close this one ? :)

@wing328
Copy link
Member

wing328 commented Sep 24, 2024

Regarding the other issue you submitted on this thread, maybe creating a proper issue will help solving

Agreed

@wing328
Copy link
Member

wing328 commented Sep 28, 2024

if no further feedback/question, i'll merge this PR coming week before v7.8.0 release

@wing328 wing328 merged commit 9f2bd31 into OpenAPITools:master Oct 3, 2024
15 checks passed
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.

3 participants