Skip to content

[Go] return errors that happen while unmarshalling objects#16525

Merged
wing328 merged 4 commits intoOpenAPITools:masterfrom
ctreatma:go-return-unmarshal-errors
Sep 12, 2023
Merged

[Go] return errors that happen while unmarshalling objects#16525
wing328 merged 4 commits intoOpenAPITools:masterfrom
ctreatma:go-return-unmarshal-errors

Conversation

@ctreatma
Copy link
Contributor

@ctreatma ctreatma commented Sep 6, 2023

This updates the Go client templates so that the UnmarshalJSON function for a model always returns errors that occur during unmarshalling.

Given the following spec (pseudocode, not necessarily a fully-valid spec):

components:
  schemas:
    ItemType:
      type: string
      enum:
        - foo
        - bar
        - baz
    Item:
      type: object
      properties:
        type:
          $ref: '#/ItemType'

and the following JSON for an Item:

{
  "type": "invalid"
}

The generated Go model should throw an error in UnmarshalJSON because the value of the type field does not match the spec.

cc/ Go committe: @antihax @grokify @kemokemo @jirikuncar @ph4r5h4d @lwj5

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/utils/export_docs_generators.sh
    
    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*.
    For Windows users, please run the script in Git BASH.
  • In case you are adding a new generator, run the following additional script :
    ./bin/utils/ensure-up-to-date
    
    Commit all changed files.
  • File the PR against the correct branch: master (7.0.1 - patch release), 7.1.x (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.

@ctreatma
Copy link
Contributor Author

ctreatma commented Sep 6, 2023

CI says samples are not up-to-date, but the changes it's finding are unrelated to my PR; I'm not sure how to address this.

Example diff reported by CI job:

-// HasType returns a boolean if a field has been set.
-func (o *PropertyNameMapping) HasType() bool {
-	if o != nil && !IsNil(o.Type) {
+// HasTypeWithUnderscore returns a boolean if a field has been set.
+func (o *PropertyNameMapping) HasTypeWithUnderscore() bool {
+	if o != nil && !IsNil(o.TypeWithUnderscore) {
 		return true
 	}
 
 	return false
 }

@wing328
Copy link
Member

wing328 commented Sep 7, 2023

CircleCI reported the following errors:

        go-petstore/model_property_name_mapping.go:91:31: other declaration of GetTypeOk
go-petstore/model_property_name_mapping.go:163:31: PropertyNameMapping.HasType redeclared in this block
        go-petstore/model_property_name_mapping.go:99:31: other declaration of HasType
go-petstore/model_property_name_mapping.go:172:31: PropertyNameMapping.SetType redeclared in this block
        go-petstore/model_property_name_mapping.go:108:31: other declaration of SetType
go-petstore/model_property_name_mapping.go:172:31: too many errors
[ERROR] Failed to execute goal org.codehaus.mojo:exec-maven-plugin:1.2.1:exec (go-test) on project GoOAS3Petstore: Command execution failed.: Process exited with an error: 2 (Exit value: 2) -> [Help 1]
[ERROR] 
[ERROR] To see the full stack trace of the errors, re-run Maven with the -e switch.
[ERROR] Re-run Maven using the -X switch to enable full debug logging.

ref: https://app.circleci.com/pipelines/github/OpenAPITools/openapi-generator/25568/workflows/098b15c0-fac5-43d1-84d1-6d769962cf3a/jobs/77623

Can you please take a look when you've time?

@wing328
Copy link
Member

wing328 commented Sep 7, 2023

for the updated samples, can you please merge the latest master into your branch and regenerate the samples again?

@ctreatma ctreatma force-pushed the go-return-unmarshal-errors branch from 452e416 to 10e0e85 Compare September 7, 2023 16:27
// https://github.com/OpenAPITools/openapi-generator/issues/1292
if regexp.
MustCompile(`^parsing time.+cannot parse "\+0000"" as "Z07:00"$`).
MustCompile(`as "Z07:00"$`).
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change matches a change that was made in November 2022 in samples/client/petstore/go/store_api_test.go.

I wouldn't expect my template change to impact this test...was this broken before or did my template change break it?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@ctreatma ctreatma Sep 8, 2023

Choose a reason for hiding this comment

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

I did run the tests locally to fix the issue but hadn't run them against master locally to confirm the initial state. It took me a minute to connect the new test failure to my change because I didn't change anything about time parsing. The test was passing before because errors weren't being returned at all, so there was no time parse error to ignore; with my change to return errors, the test started failing because the regex it was using to identify time parse errors was wrong. (Go playground example for posterity)

Copy link
Member

Choose a reason for hiding this comment

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

@ctreatma thanks for the details. Can you please PM me vis Slack when you've time?

https://join.slack.com/t/openapi-generator/shared_invite/zt-12jxxd7p2-XUeQM~4pzsU9x~eGLQqX2g

@wing328 wing328 added this to the 7.0.1 milestone Sep 8, 2023
@wing328 wing328 merged commit 5b4d970 into OpenAPITools:master Sep 12, 2023
@ctreatma ctreatma deleted the go-return-unmarshal-errors branch January 14, 2025 19:25
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.

2 participants