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

[go] Serialize multipart array of complex objects as JSON #2965

Merged
merged 3 commits into from
Jun 28, 2019

Conversation

thiagoarrais
Copy link
Contributor

@thiagoarrais thiagoarrais commented May 21, 2019

PR checklist

  • Read the contribution guidelines.
  • Ran the shell script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh, ./bin/openapi3/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in .\bin\windows\. If contributing template-only or documentation-only changes which will change sample output, be sure to build the project first.
  • Filed the PR against the correct branch: master, 4.1.x, 5.0.x. Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

cc @antihax @bvwells @grokify @kemokemo

Description of the PR

Fixes #2964 by forcing multipart items that are complex objects to be rendered as json.

I'm not really sure this is the correct approach architecturally. Just let me know if you need any changes. And please also help me see the many hidden edge cases I'm not finding.

Update: Fixes #3043

@thiagoarrais thiagoarrais changed the title Serialize multipart array of complex objects as JSON [go] Serialize multipart array of complex objects as JSON May 23, 2019
@thiagoarrais
Copy link
Contributor Author

@wing328: the bot didn't label this one (probably because I've edited the title to include after the fact). Can you add the Client: Go label please?

@@ -138,6 +138,10 @@ func parameterToString(obj interface{}, collectionFormat string) string {
delimiter = "\t"
case "csv":
delimiter = ","
case "json": {
jsonBuf, _ := json.Marshal(obj)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@antihax: changed as per your suggestion. Question: do you think we need to check err here?

Copy link
Member

Choose a reason for hiding this comment

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

If checking the err here will result in better error handling (e.g. users get notified of the errors instead of errors being silently discarded), then please do so.

Copy link
Contributor

Choose a reason for hiding this comment

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

The function may need to be altered to return the error but it would be a good idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. I will add the error handling.

And since this is the only case that needs error handling (and this function was already being distorted anyway), I will also move JSON serialization code to the point of call instead of changing parameterToString.

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 updated the code to add error handling. Please let me know what you think.

Copy link
Member

Choose a reason for hiding this comment

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

@antihax can you please have another look when you've time?

@thiagoarrais
Copy link
Contributor Author

Friendly bump: can we merge this?

@wing328 wing328 modified the milestones: 4.0.2, 4.0.3 Jun 20, 2019
@wing328
Copy link
Member

wing328 commented Jun 25, 2019

@thiagoarrais sorry for the delay as we were busy with another release.

Can you please resolve the merge conflicts and I'll merge this PR into accordingly?

@thiagoarrais
Copy link
Contributor Author

@wing328: 70ff797 should be good to go

@wing328 wing328 merged commit e46bd7d into OpenAPITools:master Jun 28, 2019
@thiagoarrais
Copy link
Contributor Author

🎉

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