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] report correctly the parameters with the deep object specification #13909

Merged
merged 16 commits into from
Nov 20, 2022

Conversation

parvit
Copy link
Contributor

@parvit parvit commented Nov 4, 2022

Hi all,
this change addresses the issue mentioned in #11401 and it should report correctly the parameters with the deep object specification.

Commits cherry-picked from previous branch to be based on current master

# Conflicts:
#	samples/client/petstore/go/go-petstore/model_200_response.go
#	samples/client/petstore/go/go-petstore/model_additional_properties_any_type.go
#	samples/client/petstore/go/go-petstore/model_client.go
@parvit
Copy link
Contributor Author

parvit commented Nov 4, 2022

@wing328 Here's the update PR you requested.

@wing328
Copy link
Member

wing328 commented Nov 4, 2022

can you please update the samples ?

https://github.com/OpenAPITools/openapi-generator/actions/runs/3391716534/jobs/5637208273

also I believe you've tested it locally, right?

Can you post the test result as well?

@parvit
Copy link
Contributor Author

parvit commented Nov 4, 2022

I'll need to update the mustache files first as now they don't generate correct code anymore.

@wing328
Copy link
Member

wing328 commented Nov 4, 2022

take your time. just let me know when it's ready. i'll review over the weekend or next week.

have a nice weekend.

@parvit
Copy link
Contributor Author

parvit commented Nov 4, 2022

Just a quick question:
image

Could you indicate how to make the "time" import generate in "model_one_of_primitive_types.go" ?

@wing328
Copy link
Member

wing328 commented Nov 4, 2022

again please make sure this PR is created from the latest master.

@parvit
Copy link
Contributor Author

parvit commented Nov 5, 2022

@wing328 master branch has the same issue, i've created a PR for that fix.

EDIT: Retesting from clean state i was not able to reproduce that issue, maybe it was just a local change that was throwing me off course

@parvit
Copy link
Contributor Author

parvit commented Nov 14, 2022

@wing328 Were you able to check the change?

@wing328
Copy link
Member

wing328 commented Nov 18, 2022

can you please resolve the merge conflicts when you've time?

I'll try to test again over the weekend.

@parvit
Copy link
Contributor Author

parvit commented Nov 18, 2022 via email

# Conflicts:
#	modules/openapi-generator/src/main/resources/go/model_simple.mustache
#	modules/openapi-generator/src/main/resources/go/utils.mustache
#	samples/client/petstore/go/go-petstore/model_animal.go
#	samples/client/petstore/go/go-petstore/model_category.go
#	samples/client/petstore/go/go-petstore/model_enum_test_.go
#	samples/client/petstore/go/go-petstore/model_format_test_.go
#	samples/client/petstore/go/go-petstore/model_name.go
#	samples/client/petstore/go/go-petstore/model_pet.go
#	samples/client/petstore/go/go-petstore/model_type_holder_default.go
#	samples/client/petstore/go/go-petstore/model_type_holder_example.go
#	samples/client/petstore/go/go-petstore/utils.go
#	samples/openapi3/client/extensions/x-auth-id-alias/go-experimental/utils.go
#	samples/openapi3/client/petstore/go/go-petstore/model_animal.go
#	samples/openapi3/client/petstore/go/go-petstore/model_apple_req.go
#	samples/openapi3/client/petstore/go/go-petstore/model_banana_req.go
#	samples/openapi3/client/petstore/go/go-petstore/model_category.go
#	samples/openapi3/client/petstore/go/go-petstore/model_duplicated_prop_parent.go
#	samples/openapi3/client/petstore/go/go-petstore/model_enum_test_.go
#	samples/openapi3/client/petstore/go/go-petstore/model_format_test_.go
#	samples/openapi3/client/petstore/go/go-petstore/model_name.go
#	samples/openapi3/client/petstore/go/go-petstore/model_pet.go
#	samples/openapi3/client/petstore/go/go-petstore/model_whale.go
#	samples/openapi3/client/petstore/go/go-petstore/model_zebra.go
#	samples/openapi3/client/petstore/go/go-petstore/utils.go
@parvit
Copy link
Contributor Author

parvit commented Nov 19, 2022

@wing328 the update is done

@wing328 wing328 added this to the 6.3.0 milestone Nov 20, 2022
@wing328
Copy link
Member

wing328 commented Nov 20, 2022

I did a test and got the following in tcpdump:

10:05:53.342677 IP localhost.52060 > localhost.http: Flags [P.], seq 3718:4248, ack 6277, win 6281, options [nop,nop,TS val 3234621852 ecr 4289403119], length 530: HTTP: GET /v2/fake/deep_object_test?inputOptions[F1]=1&inputOptions[F2]=teststring&inputOptions[F3]=null&inputOptions[id]=1&inputOptions[name]=TestCat&test_pet[F1]=1&test_pet[F2]=teststring&test_pet[F3]=null&test_pet[id]=1&test_pet[name]=Test&test_pet[photoUrls]=http%3A%2F%2Flocalhost&test_pet[tags][F1]=1&test_pet[tags][F2]=teststring&test_pet[tags][F3]=null&test_pet[tags][id]=2&test_pet[tags][name]=tag1 HTTP/1.1

What are those F1, F2, F3 ?

Here is a test I added to pet_api_test.go

// test deep object query parameter and verify via tcpdump
func TestDeepObjectQuery(t *testing.T) {
	newPet := (sw.Pet{Id: sw.PtrInt64(12830), Name: "gopher",
		PhotoUrls: []string{"http://1.com", "http://2.com"}, Status: sw.PtrString("pending"),
		Tags: []sw.Tag{{Id: sw.PtrInt64(1), Name: sw.PtrString("tag2")}}})
	newCategory := (sw.Category{Id: sw.PtrInt64(12830), Name: "cat"})
	configuration := sw.NewConfiguration()
	apiClient := sw.NewAPIClient(configuration)
	r, err := apiClient.FakeApi.TestQueryDeepObject(context.Background()).TestPet(newPet).InputOptions(newCategory).Execute()
	if err != nil {
		// for sure this will fail as the endpoint is fake
	}
	if r.StatusCode != 200 {
		t.Log(r)
	}
}

Can you please also include it in your PR so that we've a test ready for testing next time?

Thanks again for the PR.

@parvit
Copy link
Contributor Author

parvit commented Nov 20, 2022

The names come from the data in the AdditionalProperties map in the test i already made in fake_api_test.go, because i wanted to stress the deep reflection path of the code to confirm it works.

func TestQueryDeepObject(t *testing.T) {
	req := client.FakeApi.TestQueryDeepObject(context.Background())

	var id = int64(1)
	var idTag1 = int64(2)
	var nameTag1 = "tag1"
	req = req.TestPet(sw.Pet{
		Id: &id,
		Name: "Test",
		PhotoUrls: []string{ "http://localhost" },
		Tags: []sw.Tag{
			{
				Id: &idTag1,
				Name: &nameTag1,
				AdditionalProperties: map[string]interface{}{
					"F1": 1,
					"F2": "teststring",
					"F3": nil,
				},
			},
		},
		AdditionalProperties: map[string]interface{}{
			"F1": 1,
			"F2": "teststring",
			"F3": nil,
		},
	})
	var idcat = int64(1)
	req = req.InputOptions(sw.Category{
		Id: &idcat,
		Name: "TestCat",
		AdditionalProperties: map[string]interface{}{
			"F1": 1,
			"F2": "teststring",
			"F3": nil,
		},
	})

	r, _ := req.Execute()

	var expectedDeepObjectURL = testScheme + "://" + testHost + deepObjectURL

	assert.Equal(t,
		expectedDeepObjectURL,
		r.Request.URL.String() )
}

@wing328
Copy link
Member

wing328 commented Nov 20, 2022

Thanks for the explanation 👍 . I can confirm in my end the result is good.

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