[Go][client] make structure members pointers, provide custom marshalling #3113

bkabrda · 2019-06-06T12:02:11Z

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.

Description of the PR

@antihax (2017/11) @bvwells (2017/12) @grokify (2018/07) @kemokemo (2018/09

This is my shot at fixing #522. It's still WIP, but I wanted to gather some initial comments to see if it makes sense to take it all the way to the end.

Basically, this PR makes all the members of generated structures pointers and it adds getters/setters for them. Additionally, it provides custom marshalling to check that required/nullable attributes are set properly in JSON when marshalling. Todo list of things that need to be done before this could be considered good to go:

Not sure about this, but I think I should change the basic types for struct members in Java code (typeMappings).
- It actually turns out to be much cleaner to do this purely on the template level, so that's what I did.
This will need update of documentations as well.
- I updated to model_doc.mustache to reflect the changes.
I need to take a closer look at enums and see if something needs to be adapted for them to work ok.
- This seems to not require any changes for enums.
One of the lines (for getting the "zero" value of a type) is duplicated in two places; it'd be ideal to hide it behind some sort of macro. Quick google search didn't reveal if mustache provides something like this, I need to take a look again.
- Turns out that the easiest way to do this is to create an uninitialized var of that specific type and return that - no macro needed.
Not sure if getters should return empty instance or nil for structure members that point to other structures. I guess nil would be more in line with what's going on here generally, but not sure - I'm trying to mimic gen-accessors.go referenced in [1], which returns empty structures, but I'm not 100 % sure why that's a good thing to do.
- I think this is ok as long as it's documented, which it is now.
I don't think custom unmarshalling needs to be created, but not sure.
- This would probably only be required if we wanted to add Get{{name}}ExplicitNull method to nullable struct members. I don't think we need that right now. If we ever need that, we can add it without introducing and API breakage.
Fix tests

Any comments would be appreciated.
Thanks!

[1] #522 (comment)

…ools#2934) * kotlin spring : add reactivity via kotlin's coroutines * add kotlin spring boot reactive samples * bug : fix spring version and import for coroutines * remove exception handler for reactive (webflux doesn't support it) * add spring milestone repository to maven pom * add reactive type for list in Api and ApiImpl methodes for mathching body responsive parameter * fix baseType for ArraySchema * regenerate samples * updating documentation

Generates samples to match with code introduced in OpenAPITools#2934.

Update CI badges to 4.1.x

* edit fix(java-jersey2): Fixing javadocs warnings * fix(java-jersey): Updating pet project

…OpenAPITools#2848) * Remove default parameters in JS client, as it's an ES6 feature * update js petstore samples

…penAPITools#3029) * feat(java-okhttpgson): Making API response headers case insensitive * feat(java-okhttpgson): Adding documentation * feat(java-okhttpgson): Removing tabs * feat(java-okhttpgson): Removing tabs

…PITools#3028) * refactor(golang): Use http constants for methods * regenerate samples * fix: Only import strings when needed * regenerate samples * Only import fmt and strings when needed * regenerate samples

Change local json.hpp include path to correct value. Update samples.

…pilable (OpenAPITools#3056) * feat(java-jersey2): Adding http response headers and making example compilable * feat(java-jersey2): Updating pet project * feat(java-jersey2): Removing uncessary lines from Readme * feat(java-jersey2): Updating pet projects * feat(java-jersey2): Updating pet projects

* Fixed case where invalid comma is added to consumes/produces list in case last element is empty. * Changed default HttpStatus.OK response to match first response code in definition. Allowing also other responses 201, 202 ... * Changed default HttpStatus.OK response to match first response code in definition. Allowing also other responses 201, 202 ... * run ./bin/kotlin-springboot-petstore-server.sh Updated APIs

* update csharp dependency * fix appveyor test * skip build error * update nunit version * fix nunit path * update petstore test * various fix

* mark nodejs-server as deprecated * update opeanpi3 script * add new file

…ols#3038) * [KOTLIN Spring] fix generation with modelNamePrefix/modelNameSuffix * fix indent * fix indent * fix indent * fix indent * fix indent * fix indent

…Tools#3072) * feat(java-jersey2): Making headers case-insensitive * feat(java-jersey2): Updating documentation

* general support to add scopes for bearer auth too implemented authorize workflow in aspnet core too * petstore update * fix missing ) * multi roles fix * null pointer error prevention * null point exception fixes * null pointer fixes * npe fix * solved line break issue

…CI (OpenAPITools#3087) * test plugin in travis, move jobs to circle CI * Revert "[maven-plugin] fix strictSpec parameter (OpenAPITools#3071)" This reverts commit 8c9a151. * Revert "Revert "[maven-plugin] fix strictSpec parameter (OpenAPITools#3071)"" This reverts commit c3e5723. * test with jdk8

…nAPITools#3012)

`URI.encode` is obsolete. `CGI.escape`, `URI.encode_www_form` or `URI.encode_www_form_component` are recommended instead. https://ruby-doc.org/stdlib-2.6/libdoc/uri/rdoc/URI/Escape.html#method-i-escape URI.encode has different behaviour to CGI.escape: ```ruby URI.encode('hello/world?test%string') => "hello/world?test%25string" CGI.escape('hello/world?test%string') => "hello%2Fworld%3Ftest%25string" ``` I recently raised pull request OpenAPITools#3039 201cbdc That pull request escapes path items at insertion. Before either pull request, the path item 'hello?world' would go into the URL as 'hello?world'. That behaviour was insecure as if an attacker could control the path item value, they could change the URL the application connected to. After OpenAPITools#3039 'hello?world' would go in as 'hello%253Fworld'. This was safer than before, but it's still not correct. If I'd realised at the time, I would have made it correct at the time. What this pull request does is make it go in as 'hello%35world', which is correct. ApiClient::build_request_url was URI.encoding the whole path. This wasn't protecting against all undesirable characters in the path items, but was escaping % characters a 2nd time which was unhelpful. I have additionally removed URI.encode from Configuration::base_url as I can't see any benefit it could be bringing. There is no justification for it in the commit where it was originally added: 47c8597

) * Revert "[maven-plugin] fix strictSpec parameter (OpenAPITools#3071)" This reverts commit 8c9a151. * [maven-plugin] fix strictSpec parameter without alias

* Link query parameter to model object Must fix OpenAPITools#2655 * Fix import

* update java client depedency to the latest version * update java samples

…ing. Fixes OpenAPITools#522

bkabrda · 2019-06-13T10:39:53Z

All right, so I think this is in a pretty good shape to get reviewed right now. I've regenerated the v2 code and fixed all tests but one - TestFindPetsByTag. It seems to me like the testing server is broken right now (returning 500 when I run the test locally) and my change isn't really touching anything related to that test, so I think I didn't break it.

I haven't included the changes from running ./bin/openapi3/go-petstore.sh, since it seems this hasn't been run for a while and would include other changes as well - probably a separate PR would be good to address that (I'm happy to do that once this is merged).

I'll continue looking into this and will be adding further changes as additional commits in case someone starts reviewing, so that it's clear what's been reviewed and what hasn't.

antihax · 2019-06-13T19:26:48Z

This looks like a good start. I would be concerned with collisions in the name space down the line but so far it looks like that is avoided.

bkabrda · 2019-06-17T12:49:59Z

Allright, I added documentation for the included utility functions. From my POV everything seems to be fine, so getting a full review with list of things to fix/improve would be great.

@antihax yeah, I think the naming collisions are avoided (at least I can't think of any collisions happening right now).

jimschubert

I like the direction of providing utilities for pointer conversions. I'm by no means a Go expert, but in the little I've worked with it, I was very confused by optional properties in JSON needing to be pointers.

The PR's changes look like a pretty significant client-side breaking change, I would suggest targeting the 4.1.x branch rather than master.

jimschubert · 2019-06-25T17:09:30Z

modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/GoClientCodegen.java

+            if (v instanceof CodegenModel) {
+                CodegenModel model = (CodegenModel) v;
+                if (!model.isEnum) {
+                    imports.add(createMapping("import", "encoding/json"));


This and the import below have a potential to break in the case that someone invoke with --import-mappings=import=encoding/json,import=error (or however the mappings are done for golang). See CodegenConfigurator line 540.

Ok, thanks, I'll take a look.

But the same argument would apply to current AbstractGoCodegen.postProcessModels which adds some imports too, right? So it's a generic issue that's already there AFAICS. (And I think the best course of action would be to deduplicate the imports list after processing is done, but I think that's out of scope of this PR).

modules/openapi-generator/src/main/resources/go/README.mustache

wing328 · 2019-06-28T10:31:46Z

@bkabrda looks like a rebase has failed and as a result, this PR contains commits not authored by you. Can you please create a new PR by cherrying picking the commits authored by you?

It's hard to review as this PR contains many changes not related to the Go generator.

wing328 · 2019-06-28T11:24:13Z

It also seems to me it's a breaking change without fallback but I'll review again once a new PR is submitted

bkabrda · 2019-06-28T11:55:17Z

Yeah, I'll do a new PR. This functionality does contain backwards incompatible changes, which is why I was advised by Jim Schubert to rebase it against 4.1.x. Let's discuss at the new PR, I'll open it shortly and link it here.

bkabrda · 2019-06-28T12:01:09Z

New PR opened: #3241

sylvainmoindron and others added 27 commits June 2, 2019 15:50

Updates sample generated content (OpenAPITools#3068)

740325f

Generates samples to match with code introduced in OpenAPITools#2934.

Update CI badges to 4.1.x (OpenAPITools#3069)

ded0bd6

Update CI badges to 4.1.x

fix(java-jersey2): Fixing javadocs warnings (OpenAPITools#3040)

25dce23

* edit fix(java-jersey2): Fixing javadocs warnings * fix(java-jersey): Updating pet project

[JavaScript] Remove default parameters syntax, as it's an ES6 feature (…

b512174

…OpenAPITools#2848) * Remove default parameters in JS client, as it's an ES6 feature * update js petstore samples

refactor(golang): Use http provided constants for http methods (OpenA…

8d9eb5b

…PITools#3028) * refactor(golang): Use http constants for methods * regenerate samples * fix: Only import strings when needed * regenerate samples * Only import fmt and strings when needed * regenerate samples

Fix wrong include path in api-header template (OpenAPITools#3062)

1eca97f

Change local json.hpp include path to correct value. Update samples.

Update C# client dependency (OpenAPITools#2678)

e06a408

* update csharp dependency * fix appveyor test * skip build error * update nunit version * fix nunit path * update petstore test * various fix

[maven-plugin] fix strictSpec parameter (OpenAPITools#3071)

8c9a151

Use 4.0.2-SNAPSHOT version in gradle samples (OpenAPITools#3085)

89d0c01

Mark nodejs-server as deprecated (OpenAPITools#3083)

c6207c0

* mark nodejs-server as deprecated * update opeanpi3 script * add new file

[KOTLIN Spring] fix generation with modelNamePrefix/Suffix (OpenAPITo…

57560d3

…ols#3038) * [KOTLIN Spring] fix generation with modelNamePrefix/modelNameSuffix * fix indent * fix indent * fix indent * fix indent * fix indent * fix indent

feat(java-jersey2): Making response headers case-insensitive (OpenAPI…

dc35439

…Tools#3072) * feat(java-jersey2): Making headers case-insensitive * feat(java-jersey2): Updating documentation

[Golang][client] Allow generating go client code as a submodule. (Ope…

66bf0dc

…nAPITools#3012)

[maven-plugin] fix strictSpec parameter without alias (OpenAPITools#3095

ad06496

) * Revert "[maven-plugin] fix strictSpec parameter (OpenAPITools#3071)" This reverts commit 8c9a151. * [maven-plugin] fix strictSpec parameter without alias

scala-play-server: fix API doc url (OpenAPITools#3096)

6379022

fix NPE in go generator (OpenAPITools#3104)

86e53d6

Add @Sunn to openapi generator core team (OpenAPITools#3105)

e68426f

scala-play-server: fix enum names for reserved words (OpenAPITools#3080)

0d701b7

Link query parameter to model object (OpenAPITools#2710)

146c1fb

* Link query parameter to model object Must fix OpenAPITools#2655 * Fix import

[Java][okhttp-gson] Update dependencies (OpenAPITools#3103)

19f47d0

* update java client depedency to the latest version * update java samples

bkabrda mentioned this pull request Jun 10, 2019

[Go] Optional values should be pointers #522

Closed

bkabrda force-pushed the go-structure-members-pointers branch 2 times, most recently from 2ce9b26 to ef202a5 Compare June 13, 2019 07:59

bkabrda force-pushed the go-structure-members-pointers branch 2 times, most recently from b07e02c to 0baf3f0 Compare June 13, 2019 10:07

[Go][client] make structure members pointers, provide custom marshall…

09568ce

…ing. Fixes OpenAPITools#522

bkabrda force-pushed the go-structure-members-pointers branch from 0baf3f0 to 09568ce Compare June 13, 2019 10:34

bkabrda force-pushed the go-structure-members-pointers branch from 0322d32 to fc606ff Compare June 17, 2019 15:32

Document utility functions

8502b2e

bkabrda force-pushed the go-structure-members-pointers branch from fc606ff to 8502b2e Compare June 17, 2019 16:28

jimschubert reviewed Jun 25, 2019

View reviewed changes

Prefix all util methods with 'Ptr' to better avoid naming collisions

2f69d39

bkabrda changed the base branch from master to 4.1.x June 26, 2019 03:10

bkabrda closed this Jun 28, 2019

bkabrda mentioned this pull request Jun 28, 2019

Go struct members pointers #3241

Closed

4 tasks

bkabrda mentioned this pull request Jul 16, 2019

[Go][experimental-client]: make struct members pointers, provide custom marshalling #3371

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Go][client] make structure members pointers, provide custom marshalling #3113

[Go][client] make structure members pointers, provide custom marshalling #3113

bkabrda commented Jun 6, 2019 •

edited

Loading

bkabrda commented Jun 13, 2019

antihax commented Jun 13, 2019

bkabrda commented Jun 17, 2019

jimschubert left a comment

jimschubert Jun 25, 2019

bkabrda Jun 25, 2019

bkabrda Jun 25, 2019

wing328 commented Jun 28, 2019 •

edited

Loading

wing328 commented Jun 28, 2019

bkabrda commented Jun 28, 2019

bkabrda commented Jun 28, 2019

[Go][client] make structure members pointers, provide custom marshalling #3113

[Go][client] make structure members pointers, provide custom marshalling #3113

Conversation

bkabrda commented Jun 6, 2019 • edited Loading

PR checklist

Description of the PR

bkabrda commented Jun 13, 2019

antihax commented Jun 13, 2019

bkabrda commented Jun 17, 2019

jimschubert left a comment

Choose a reason for hiding this comment

jimschubert Jun 25, 2019

Choose a reason for hiding this comment

bkabrda Jun 25, 2019

Choose a reason for hiding this comment

bkabrda Jun 25, 2019

Choose a reason for hiding this comment

wing328 commented Jun 28, 2019 • edited Loading

wing328 commented Jun 28, 2019

bkabrda commented Jun 28, 2019

bkabrda commented Jun 28, 2019

bkabrda commented Jun 6, 2019 •

edited

Loading

wing328 commented Jun 28, 2019 •

edited

Loading