[Go][Client] Use configured scheme in requests #2947

johanbrandhorst · 2019-05-20T15:33:56Z

Ensure the Scheme option in the configuration is used.

PS. I ran ./bin/go-petstore.sh, ./bin/go-petstore-withxml.sh and ./bin/openapi3/go-petstore.sh. I assume the large amount of changes in the openapi3 directory is because it was slightly stale prior to my change.

@antihax @bvwells @grokify @kemokemo

wing328 · 2019-05-22T10:36:16Z

CI reports the following issues:

[ERROR] Error fetching link: /home/circleci/OpenAPITools/openapi-generator/modules/openapi-generator/home/circleci/OpenAPITools/openapi-generator/modules/openapi-generator-cli/target/apidocs. Ignored it.
# _/home/circleci/OpenAPITools/openapi-generator/samples/client/petstore/go/go-petstore
go-petstore/client.go:295:18: localVarRequest.Scheme undefined (type *http.Request has no field or method Scheme)
FAIL	_/home/circleci/OpenAPITools/openapi-generator/samples/client/petstore/go [build failed]

Please take a look when you've time.

johanbrandhorst · 2019-05-22T10:46:05Z

I've update the PR, and I've made a further change;

After reading the documentation surrounding the use of http.Request.Host, I think this was used incorrectly before; it wasn't setting the host in the request, it was only setting the Host used in the headers sent to the server, while the actual host connected to was whatever was part of the Base URL.

This fixes the behavior to overwrite the host in the parsed URL before the request is created.

References: http.Request documentation: https://golang.org/pkg/net/http/#Request

wing328 · 2019-05-23T02:12:20Z

Got the following new errors instead:

SLF4J: See http://www.slf4j.org/codes.html#StaticLoggerBinder for further details.
[ERROR] Error fetching link: /home/circleci/OpenAPITools/openapi-generator/modules/openapi-generator/home/circleci/OpenAPITools/openapi-generator/modules/openapi-generator-cli/target/apidocs. Ignored it.
=== RUN   TestOAuth2
--- FAIL: TestOAuth2 (0.11s)
	auth_test.go:46: Error while adding pet
	auth_test.go:47: Post http://testhost/v2/pet: dial tcp: lookup testhost on 169.254.169.254:53: no such host
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
	panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x10 pc=0x68fc1e]

goroutine 5 [running]:
testing.tRunner.func1(0xc4200fe0f0)
	/usr/local/go/src/testing/testing.go:711 +0x2d2
panic(0x6e1820, 0x8cf7e0)
	/usr/local/go/src/runtime/panic.go:491 +0x283
_/home/circleci/OpenAPITools/openapi-generator/samples/client/petstore/go.TestOAuth2(0xc4200fe0f0)
	/home/circleci/OpenAPITools/openapi-generator/samples/client/petstore/go/auth_test.go:49 +0x52e
testing.tRunner(0xc4200fe0f0, 0x74d7a0)
	/usr/local/go/src/testing/testing.go:746 +0xd0
created by testing.(*T).Run
	/usr/local/go/src/testing/testing.go:789 +0x2de
exit status 2
FAIL	_/home/circleci/OpenAPITools/openapi-generator/samples/client/petstore/go	0.119s
[ERROR] Failed to execute goal org.codehaus.mojo:exec-maven-plugin:1.2.1:exec (go-test) on project Goswagger: Command execution failed. Process exited with an error: 1 (Exit value: 1) -> [Help 1]
[ERROR]

johanbrandhorst · 2019-05-23T11:44:13Z

Thanks for following up @wing328, I've updated the tests, I think the existing test was actually never going to work if the Host was working properly (:thinking:). I've also refactored the tests to prevent them from panicking in the future.

I rebased the commits so that the first one regenerates the existing clients (since they were stale), so my changes are in commit 2 and 3.

johanbrandhorst · 2019-05-23T14:36:06Z

Not sure why the tests failed... they shouldn't be running against https anymore 🤔.

johanbrandhorst · 2019-05-23T15:21:02Z

@wing328 could you trigger a rerun of the tests? I don't know if they ran against the tip of the branch.

johanbrandhorst · 2019-05-23T15:22:45Z

Wait, I don't understand the last test failure at all 🤔

Previously we did not interrupt execution of a test case when an error was returned from an API call. This was causing the tests to crash as soon as we try to dereference the response. We now fail the tests as soon as the first API error is received.

Previously we were simply setting the Host header and completely ignoring the configured scheme. Fixes OpenAPITools#2943

johanbrandhorst · 2019-06-06T10:44:13Z

Rebased on master. Again, the meat of this change is here: fc3d0f4.

johanbrandhorst · 2019-06-06T13:27:20Z

I think the tests are failing because a change was merged that didn't regenerate all the appropriate files. I'll dig into it a bit.

johanbrandhorst · 2019-06-06T13:28:51Z

Yep, #3028 did not regenerate the pet store, and it is now failing because of it.

wing328 · 2019-06-06T13:28:57Z

Taking a look too...

johanbrandhorst · 2019-06-06T13:29:26Z

I can include the fix in this PR, no problems.

wing328 · 2019-06-06T13:31:02Z

@johanbrandhorst I would suggest merging the latest master, which includes some changes to the Go generator, before updating the samples.

johanbrandhorst · 2019-06-06T13:32:32Z

I've rebased this on latest master. The latest master is bugged, the generated files from api.mustache do not include the strings import like it should.

wing328 · 2019-06-06T13:34:21Z

I've rebased this on latest master. The latest master is bugged, the generated files from api.mustache do not include the strings import like it should.

But the tests (including Petstore) passed ...

johanbrandhorst · 2019-06-06T13:35:29Z

I've rebased this on latest master. The latest master is bugged, the generated files from api.mustache do not include the strings import like it should.

But the tests (including Petstore) passed ...

That's because the petstore was not regenerated with the new api.mustache file. They're failing here because I regenerated the files. I will fix it here as well, but there's a lack in CI coverage here since you have to manually regenerate the relevant files and if they're not, things are missed.

The strings package is still used for replacing strings in some places.

wing328 · 2019-06-06T13:36:13Z

That's because the petstore was not regenerated with the new api.mustache file.

We also have a script to ensure the Go petstore client is up-to-date. Let me double check on that.

johanbrandhorst · 2019-06-06T13:37:39Z

I've pushed an update to the api.mustache file. It's trivial to see that it is broken by searching the file for strings.: https://github.com/OpenAPITools/openapi-generator/blob/master/modules/openapi-generator/src/main/resources/go/api.mustache

wing328 · 2019-06-06T13:47:01Z

I ran ./bin/go-petstore.sh from the latest master but there's no change

$ git status
On branch master
Your branch is up to date with 'origin/master'.

nothing to commit, working tree clean

wing328 · 2019-06-06T13:48:43Z

I would recommend you create a new branch based on the latest master instead.

johanbrandhorst · 2019-06-06T13:57:41Z

Look, I will create another branch to really prove it to you, but you don't even need to regenerate the files to see the problem. Look at https://github.com/OpenAPITools/openapi-generator/blob/master/modules/openapi-generator/src/main/resources/go/api.mustache, which is the master version. It does not import "strings", but on line 84 we can see:

localVarPath = strings.Replace(localVarPath, "{"+"{{baseName}}"+"}", fmt.Sprintf("%v", {{paramName}}), -1){{/pathParams}}

This is trivially going to cause a compilation error. I don't know why your files aren't being generated, but feel free to compare the master version of api.mustache against the actual pet store files and you'll see that they're not up to date:

https://github.com/OpenAPITools/openapi-generator/blob/master/samples/client/petstore/go/go-petstore/api_pet.go

This is still importing strings.

wing328 · 2019-06-06T13:59:02Z

The hardcode import of strings has been (correctly) removed by another PR

wing328 · 2019-06-06T14:00:00Z

Reviewing the recent changes related to the Go client geneator may help : https://github.com/OpenAPITools/openapi-generator/pulls?utf8=%E2%9C%93&q=is%3Amerged+is%3Apr+label%3A%22Client%3A+Go%22+

johanbrandhorst · 2019-06-06T14:00:30Z

Removing the hardcoded import of strings is exactly what is causing the problem here. The strings import must stay for as long as line 80 is using strings.Replace:

https://github.com/OpenAPITools/openapi-generator/blob/master/modules/openapi-generator/src/main/resources/go/api.mustache#L80

johanbrandhorst · 2019-06-06T14:01:00Z

@mcristina422 Can you weigh in on this?

johanbrandhorst · 2019-06-06T14:05:29Z

I think the CI tests will pass with my recent change, and if they don't, it will probably be because there are others parts that haven't been regenerated.

wing328 · 2019-06-06T14:09:36Z

Please have a look at https://github.com/OpenAPITools/openapi-generator/pull/3028/files#diff-2ff536a1e1baf57d72cd845ad0765d2bR368

johanbrandhorst · 2019-06-06T14:12:20Z

Please have a look at https://github.com/OpenAPITools/openapi-generator/pull/3028/files#diff-2ff536a1e1baf57d72cd845ad0765d2bR368

I saw this change, but it's irrelevant, because this should always be imported, as is evident by the mustache file I linked. The strings package is used unconditionally, not just when using path parameters.

wing328 · 2019-06-06T14:20:42Z

OpenJDK Runtime Environment (build 1.8.0_141-8u141-b15-3~14.04-b15)
OpenJDK 64-Bit Server VM (build 25.141-b15, mixed mode)
SLF4J: Failed to load class "org.slf4j.impl.StaticLoggerBinder".
SLF4J: Defaulting to no-operation (NOP) logger implementation
SLF4J: See http://www.slf4j.org/codes.html#StaticLoggerBinder for further details.
[ERROR] Error fetching link: /home/circleci/OpenAPITools/openapi-generator/modules/openapi-generator/home/circleci/OpenAPITools/openapi-generator/modules/openapi-generator-cli/target/apidocs. Ignored it.
# _/home/circleci/OpenAPITools/openapi-generator/samples/client/petstore/go/go-petstore
go-petstore/api_another_fake.go:17:2: imported and not used: "strings"
go-petstore/api_fake.go:17:2: imported and not used: "strings"
go-petstore/api_fake_classname_tags123.go:17:2: imported and not used: "strings"
FAIL	_/home/circleci/OpenAPITools/openapi-generator/samples/client/petstore/go [build failed]
[ERROR] Failed to execute goal org.codehaus.mojo:exec-maven-plugin:1.2.1:exec (go-test) on project Goswagger: Command execution failed. Process exited with an error: 2 (Exit value: 2) -> [Help 1]
[ERROR]

Ref: https://circleci.com/gh/OpenAPITools/openapi-generator/7117#queue-placeholder/containers/1

Again I would suggest you start a new branch based on the latest master instead.

mcristina422 · 2019-06-06T14:49:00Z

The strings package is used unconditionally, not just when using path parameters.

That is incorrect. strings and fmt is only used when the operation has a path parameter.
{{#pathParams}} localVarPath := a.client.cfg.BasePath + "{{{path}}}"{{#pathParams}} localVarPath = strings.Replace(localVarPath, "{"+"{{baseName}}"+"}", fmt.Sprintf("%v", {{paramName}}), -1){{/pathParams}}

That's why there is an explicit check for operation.pathParams.size() > 0 in the java code. Encoding this logic into the mustache file was not trivial and ugly, and the fmt package already had the same issue.

I believe if you just rebase on master and add your scheme changes everything should test fine

johanbrandhorst · 2019-06-06T14:51:00Z

I was misled by old artifacts, I had to run mvn clean and regenerate to get the latest versions. Sorry for the confusion, I am not familiar with Java tooling.

wing328 added Client: Go Enhancement: General labels May 22, 2019

wing328 added this to the 4.0.1 milestone May 22, 2019

johanbrandhorst force-pushed the patch-1 branch from c7c41fa to a6e02cf Compare May 22, 2019 10:44

johanbrandhorst force-pushed the patch-1 branch 2 times, most recently from 671f5e6 to abda180 Compare May 23, 2019 11:42

johanbrandhorst force-pushed the patch-1 branch 2 times, most recently from 9cd245f to e4ac5c1 Compare May 23, 2019 13:34

wing328 modified the milestones: 4.0.1, 4.0.2 May 31, 2019

johanbrandhorst force-pushed the patch-1 branch 3 times, most recently from 9ac73f4 to b8ac09e Compare June 6, 2019 10:37

johanbrandhorst added 3 commits June 6, 2019 11:41

[Go][Client] Regenerate petstore examples

72b8066

[Go][Client] Use configured Scheme and Host in requests

fc3d0f4

Previously we were simply setting the Host header and completely ignoring the configured scheme. Fixes OpenAPITools#2943

johanbrandhorst force-pushed the patch-1 branch from b8ac09e to fc3d0f4 Compare June 6, 2019 10:41

[Go][Client] Add back strings import to api

71b8b52

The strings package is still used for replacing strings in some places.

johanbrandhorst closed this Jun 6, 2019

Uh oh!

[Go][Client] Use configured scheme in requests #2947

[Go][Client] Use configured scheme in requests #2947

Uh oh!

Conversation

johanbrandhorst commented May 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wing328 commented May 22, 2019

Uh oh!

johanbrandhorst commented May 22, 2019

Uh oh!

wing328 commented May 23, 2019

Uh oh!

johanbrandhorst commented May 23, 2019

Uh oh!

johanbrandhorst commented May 23, 2019

Uh oh!

johanbrandhorst commented May 23, 2019

Uh oh!

johanbrandhorst commented May 23, 2019

Uh oh!

johanbrandhorst commented Jun 6, 2019

Uh oh!

johanbrandhorst commented Jun 6, 2019

Uh oh!

johanbrandhorst commented Jun 6, 2019

Uh oh!

wing328 commented Jun 6, 2019

Uh oh!

johanbrandhorst commented Jun 6, 2019

Uh oh!

wing328 commented Jun 6, 2019

Uh oh!

johanbrandhorst commented Jun 6, 2019

Uh oh!

wing328 commented Jun 6, 2019

Uh oh!

johanbrandhorst commented Jun 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wing328 commented Jun 6, 2019

Uh oh!

johanbrandhorst commented Jun 6, 2019

Uh oh!

wing328 commented Jun 6, 2019

Uh oh!

wing328 commented Jun 6, 2019

Uh oh!

johanbrandhorst commented Jun 6, 2019

Uh oh!

wing328 commented Jun 6, 2019

Uh oh!

wing328 commented Jun 6, 2019

Uh oh!

johanbrandhorst commented Jun 6, 2019

Uh oh!

johanbrandhorst commented Jun 6, 2019

Uh oh!

johanbrandhorst commented Jun 6, 2019

Uh oh!

wing328 commented Jun 6, 2019

Uh oh!

johanbrandhorst commented Jun 6, 2019

Uh oh!

wing328 commented Jun 6, 2019

Uh oh!

mcristina422 commented Jun 6, 2019

Uh oh!

johanbrandhorst commented Jun 6, 2019

Uh oh!

Uh oh!

johanbrandhorst commented May 20, 2019 •

edited

Loading

johanbrandhorst commented Jun 6, 2019 •

edited

Loading