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] dont canonize headers #10779

Merged

Conversation

andrewkav
Copy link
Contributor

@andrewkav andrewkav commented Nov 4, 2021

Golang be default canonizes header names for example content-type will be converted to Content-Type, this PR allows to opt out this behaviour by setting additional property dontCanonizeHeaders=true.

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.
  • File the PR against the correct branch: master (5.3.0), 6.0.x
  • If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

@andrewkav
Copy link
Contributor Author

hey @wing328 @antihax @grokify @kemokemo @jirikuncar @ph4r5h4d , would appreciate if you could look at this PR :)

@wing328
Copy link
Member

wing328 commented Nov 5, 2021

@andrewkav thanks for the PR. What about fixing this issue without adding an option to start with as your fix looks to be the correct behaviour to me (plus we want to avoid too many options in the generator)?

@andrewkav
Copy link
Contributor Author

andrewkav commented Nov 5, 2021

@andrewkav thanks for the PR. What about fixing this issue without adding an option to start with as your fix looks to be the correct behaviour to me (plus we want to avoid too many options in the generator)?

@wing328
yeah, I don't mind making that standard behavior, however I was concerned that somebody might relying on the current implementation so made it an opt-out. Do you think we can make this default behavior?

@wing328
Copy link
Member

wing328 commented Nov 5, 2021

My take is to make this the default behaviour and we'll then find out if there's any valid use cases that must use the current/old behaviour.

@andrewkav andrewkav force-pushed the go/dont-canonize-headers-opt-out branch from 51f45f7 to 3659153 Compare November 5, 2021 20:12
@andrewkav
Copy link
Contributor Author

Alright, I removed additional parameter, defaulting to non canonized headers (will preserve what's in the OpenAPI spec)

@wing328 wing328 added this to the 5.3.1 milestone Nov 6, 2021
@wing328 wing328 changed the title [GO]: dont canonize headers when dontCanonizeHeaders additional prope… [GO]: dont canonize headers Nov 6, 2021
@wing328 wing328 changed the title [GO]: dont canonize headers [GO] dont canonize headers Nov 6, 2021
@wing328
Copy link
Member

wing328 commented Nov 9, 2021

Tested it locally and it broke the following tests:

=== RUN   TestAccessToken
--- PASS: TestAccessToken (0.01s)
=== RUN   TestAPIKeyNoPrefix
    auth_test.go:155: APIKey Authentication is missing
--- FAIL: TestAPIKeyNoPrefix (0.02s)
=== RUN   TestAPIKeyWithPrefix
    auth_test.go:190: APIKey Authentication is missing
--- FAIL: TestAPIKeyWithPrefix (0.02s)
=== RUN   TestDefaultHeader
--- PASS: TestDefaultHeader (0.01s)

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

Looks like 2 tests in auth_test.go need to be updated.

@andrewkav andrewkav force-pushed the go/dont-canonize-headers-opt-out branch from 050027a to 09e267d Compare November 9, 2021 09:22
Copy link
Contributor

@jirikuncar jirikuncar left a comment

Choose a reason for hiding this comment

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

I agree with not canonising headers. There must be a reason why someone would use non-canonic version in their OpenAPI spec and we should respect that. Users can update their specification to keep the "old" behavior. The only issue might be a consistency among other languages, but we can solve that separately.

@wing328
Copy link
Member

wing328 commented Nov 11, 2021

I reran the tests but it's now down to 1 failed test:

=== RUN   TestAPIKeyNoPrefix
--- PASS: TestAPIKeyNoPrefix (0.02s)
=== RUN   TestAPIKeyWithPrefix
    auth_test.go:190: APIKey Authentication is missing
--- FAIL: TestAPIKeyWithPrefix (0.03s)
=== RUN   TestDefaultHeader
--- PASS: TestDefaultHeader (0.01s)

@andrewkav can you please take another look when you've time?

Here is the command to run the tests:

mvn integration-test -f samples/client/petstore/go/pom.xml

@andrewkav
Copy link
Contributor Author

I'm getting some unrelated errors even when running from master

    auth_test.go:47: Error while adding pet: 405 Method Not Allowed
--- FAIL: TestOAuth2 (1.20s)
=== RUN   TestBasicAuth
    auth_test.go:82: Error while adding pet: 405 Method Not Allowed
--- FAIL: TestBasicAuth (0.39s)
=== RUN   TestAccessToken
    auth_test.go:112: Error while adding pet: 405 Method Not Allowed
--- FAIL: TestAccessToken (0.31s)

Am I doing something wrong?

@andrewkav andrewkav force-pushed the go/dont-canonize-headers-opt-out branch from 09e267d to 735ee4b Compare November 11, 2021 15:04
@andrewkav andrewkav force-pushed the go/dont-canonize-headers-opt-out branch from 735ee4b to e47fa0d Compare November 11, 2021 19:09
@wing328
Copy link
Member

wing328 commented Nov 14, 2021

@wing328
Copy link
Member

wing328 commented Nov 14, 2021

Tested locally and all tests passed.

@wing328 wing328 merged commit d91ff3a into OpenAPITools:master Nov 14, 2021
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.

None yet

3 participants