openapi-generator icon indicating copy to clipboard operation
openapi-generator copied to clipboard

issue #11401 - Go client generator doesn't support deepObject in query

Open parvit opened this issue 1 year ago • 12 comments

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

Please review and indicate me if there's anything more i should address.

parvit avatar Oct 08 '22 09:10 parvit

cc @antihax (2017/11) @grokify (2018/07) @kemokemo (2018/09) @jirikuncar (2021/01) @ph4r5h4d (2021/04)

wing328 avatar Oct 12 '22 03:10 wing328

Can you please update the samples by running ./bin/generate-samples.sh (using gitbash if you're on Windows) so that the CI can test the change ?

wing328 avatar Oct 12 '22 03:10 wing328

sure, in a moment

EDIT: i'm fixing the issue with samples

parvit avatar Oct 12 '22 04:10 parvit

i seem to have missed the functionality based on header parameters, i'll add it and push soon

parvit avatar Oct 12 '22 04:10 parvit

Now everything should be in place, i've also regenerated the samples and the tests produced fail in the same way as on master.

parvit avatar Oct 12 '22 21:10 parvit

@wing328 could you please check again the change again? thanks.

parvit avatar Oct 14 '22 20:10 parvit

CI failed:

go version go1.14 linux/amd64
go: found github.com/stretchr/testify/assert in github.com/stretchr/testify v1.8.0
go: github.com/stretchr/testify upgrade => v1.8.0
go: golang.org/x/oauth2 upgrade => v0.0.0-20221014153046-6fdb5e3db783
go: downloading golang.org/x/oauth2 v0.0.0-20221014153046-6fdb5e3db783
go: downloading golang.org/x/net v0.0.0-20221014081412-f15817d10f9b
go: found golang.org/x/net/context in golang.org/x/net v0.0.0-20221014081412-f15817d10f9b
go: golang.org/x/net upgrade => v0.0.0-20221014081412-f15817d10f9b
=== RUN   TestOAuth2
    TestOAuth2: auth_test.go:56: Error while deleting pet by id: 404 Not Found
--- FAIL: TestOAuth2 (0.23s)
=== RUN   TestBasicAuth
    TestBasicAuth: auth_test.go:91: Error while deleting pet by id: 404 Not Found
--- FAIL: TestBasicAuth (0.01s)
=== RUN   TestAccessToken
    TestAccessToken: auth_test.go:121: Error while deleting pet by id: 404 Not Found
--- FAIL: TestAccessToken (0.01s)
=== RUN   TestAPIKeyNoPrefix
    TestAPIKeyNoPrefix: auth_test.go:150: Error while deleting pet by id: 404 Not Found
--- FAIL: TestAPIKeyNoPrefix (0.00s)
=== RUN   TestAPIKeyWithPrefix
    TestAPIKeyWithPrefix: auth_test.go:185: Error while deleting pet by id: 404 Not Found
--- FAIL: TestAPIKeyWithPrefix (0.00s)
=== RUN   TestDefaultHeader
    TestDefaultHeader: auth_test.go:219: Error while deleting pet by id: 404 Not Found
--- FAIL: TestDefaultHeader (0.00s)
=== RUN   TestHostOverride
--- PASS: TestHostOverride (0.01s)
=== RUN   TestSchemeOverride
--- PASS: TestSchemeOverride (0.01s)
=== RUN   TestPutBodyWithFileSchema
--- PASS: TestPutBodyWithFileSchema (0.00s)
=== RUN   TestAddPet
--- PASS: TestAddPet (0.00s)
=== RUN   TestAddPetMock
--- PASS: TestAddPetMock (0.00s)
=== RUN   TestFindPetsByStatusWithMissingParam
--- PASS: TestFindPetsByStatusWithMissingParam (0.00s)
=== RUN   TestGetPetById
    TestGetPetById: pet_api_test.go:291: Error while getting pet by id: 404 Not Found
--- FAIL: TestGetPetById (0.00s)
=== RUN   TestGetPetByIdWithInvalidID
--- PASS: TestGetPetByIdWithInvalidID (0.00s)
=== RUN   TestUpdatePetWithForm
    TestUpdatePetWithForm: pet_api_test.go:89: Error while updating pet by id: 404 Not Found
--- FAIL: TestUpdatePetWithForm (0.00s)
=== RUN   TestFindPetsByTag
    TestFindPetsByTag: pet_api_test.go:108: Error no pets returned
    TestFindPetsByTag: pet_api_test.go:121: Error while getting pet by tag could not find 12830
--- FAIL: TestFindPetsByTag (0.00s)
=== RUN   TestFindPetsByStatus
    TestFindPetsByStatus: pet_api_test.go:137: Error no pets returned
--- FAIL: TestFindPetsByStatus (0.00s)
=== RUN   TestUploadFile
    TestUploadFile: pet_api_test.go:160: Error while uploading file: 404 Not Found
--- FAIL: TestUploadFile (0.01s)
=== RUN   TestUploadFileRequired
--- PASS: TestUploadFileRequired (0.00s)
=== RUN   TestDeletePet
    TestDeletePet: pet_api_test.go:190: Error while deleting pet by id: 404 Not Found
--- FAIL: TestDeletePet (0.00s)
=== RUN   TestPlaceOrder
    TestPlaceOrder: store_api_test.go:29: Skipping error for parsing time with `+0000` UTC offset as Petstore Test Server does not return valid RFC 3339 datetime
--- PASS: TestPlaceOrder (0.02s)
=== RUN   TestCreateUser
--- PASS: TestCreateUser (0.01s)
=== RUN   TestCreateUsersWithArrayInput
--- PASS: TestCreateUsersWithArrayInput (0.01s)
=== RUN   TestGetUserByName
    TestGetUserByName: user_api_test.go:86: Error while getting user by id: 404 Not Found
--- FAIL: TestGetUserByName (0.00s)
=== RUN   TestGetUserByNameWithInvalidID
--- PASS: TestGetUserByNameWithInvalidID (0.00s)
=== RUN   TestUpdateUser
    TestUpdateUser: user_api_test.go:137: Error while getting user by id: 404 Not Found
--- FAIL: TestUpdateUser (0.00s)
FAIL
exit status 1
FAIL	github.com/OpenAPITools/openapi-generator/samples/client/petstore/go	0.339s
[ERROR] Failed to execute goal org.codehaus.mojo:exec-maven-plugin:1.2.1:exec (go-test) on project GoPetstore: Command execution failed.: Process exited with an error: 1 (Exit value: 1) -> [Help 1]
[ERROR] 
[ERROR] To see the full stack trace of the errors, re-run Maven with the -e switch.

ref: https://app.circleci.com/pipelines/github/OpenAPITools/openapi-generator/20107/workflows/e33d6565-687e-4ca1-a786-bda17bdd7ac1/jobs/51746

wing328 avatar Oct 15 '22 02:10 wing328

For some reason using the "http" schema in the tests was broken and would only return error 405, changing it to "https" runs correct.

Then there were some actual issues in the code that previous result had covered, now locally the tests are green.

parvit avatar Oct 15 '22 10:10 parvit

You will need to run the petstore server locally: https://github.com/OpenAPITools/openapi-generator/wiki/Integration-Tests

wing328 avatar Oct 16 '22 02:10 wing328

You will need to run the petstore server locally: https://github.com/OpenAPITools/openapi-generator/wiki/Integration-Tests

Ok, however the tests already pass on CI so i think we can go ahead

parvit avatar Oct 16 '22 06:10 parvit

@wing328 Probably i'm missing something, could you please indicate what exactly is missing to merge the issue? thanks.

parvit avatar Oct 17 '22 15:10 parvit

@altitude I wonder if you can also test/review this change as you've a bug bounty on https://github.com/OpenAPITools/openapi-generator/issues/11401. Thanks.

wing328 avatar Oct 17 '22 16:10 wing328

No update on this?

parvit avatar Oct 22 '22 07:10 parvit

@altitude @wing328 Hi all, i'd really like to close this, is there anything i can do to accelerate the process? thanks.

parvit avatar Oct 25 '22 20:10 parvit

@parvit I did some tests locally with a new endpoint in petstore spec as follows:

  /fake/deep_object_test:
    get:
      tags:
        - fake
      operationId: test_query_deep_object
      parameters:
        - name: test_pet
          in: query
          required: false
          style: deepObject
          schema:
            $ref: '#/components/schemas/Pet'
          explode: true
        - name: inputOptions
          in: query
          required: false
          style: deepObject
          schema:
            $ref: '#/components/schemas/Category'
          explode: true
      responses:
        '200':
          description: OK

but here is the result with tcpdump:

14:48:08.484748 IP localhost.52322 > localhost.http: Flags [P.], seq 10209:10701, ack 7291, win 6265, options [nop,nop,TS val 477774952 ecr 729668614], length 492: HTTP: GET /v2/fake/deep_object_test?inputOptions%5Bid%5D=0xc000018590&inputOptions%5Bname%5D=gopher+world&test_pet%5Bid%5D=0xc000018580&test_pet%5Bname%5D=hello+gopher&test_pet%5BphotoUrls%5D%5B0%5D=http%3A%2F%2F1.com&test_pet%5BphotoUrls%5D%5B1%5D=http%3A%2F%2F2.com&test_pet%5Bstatus%5D=0xc000210a30&test_pet%5Btags%5D%5B0%5D=%7B0xc000018588+0xc000210a40+map%5B%5D%7D HTTP/1.1

I don't think that's the correct result.

Does the fix work for you locally?

wing328 avatar Oct 31 '22 06:10 wing328

The deepObject format in and of itself seems to be working when you decode the URL: image

What would you expect in place of the value of "inputOptions[id]=0xc000018590" for example?

parvit avatar Oct 31 '22 07:10 parvit

The deepObject format in and of itself seems to be working when you decode the URL:

I don't think the users expect these square brackets to be encoded in the URL.

Please refer to the following SO for more info:

https://stackoverflow.com/questions/56279024/openapi-parameter-with-brackets-and-variable-name https://stackoverflow.com/questions/46004257/swagger-openapi-spec-for-arrays-of-objects-in-url-query-parameter https://stackoverflow.com/questions/48491688/how-to-define-parameters-with-square-brackets-in-openapi-swagger

wing328 avatar Oct 31 '22 07:10 wing328

https://github.com/OpenAPITools/openapi-generator/issues/11401#issuecomment-1268147781 also contains an example.

Expected call /test?filter[search]=query, not /test?filter=%7B0x14000098dc0%7D

wing328 avatar Oct 31 '22 07:10 wing328

Ok i'll see if i can change it to not being encoded.

parvit avatar Oct 31 '22 08:10 parvit

Hi, i've managed to implement leaving the square brackets without escaping, also i've integrated your specification in a test that you can find in fake_api_test.go

But when i've pulled the master this error has come up related to changes to the workflow from master: image

Is it something we can safely ignore or is it related to the code somehow? thanks

parvit avatar Nov 02 '22 05:11 parvit

@parvit looks like the PR is messed up with 4000+ file changes.

Can you please file a new one based on the latest master by cherry-picking the commits?

wing328 avatar Nov 04 '22 05:11 wing328

There were merge conflicts with the master so i've pulled it and resolved them on the branch, but if you want to solve them manually i'll make the PR without it.

parvit avatar Nov 04 '22 05:11 parvit

totally ok that you try to resolve the merged conflicts, but if you review the files changed in this PR, many unrelated-changes are included.

wing328 avatar Nov 04 '22 06:11 wing328

If you review https://github.com/OpenAPITools/openapi-generator/pull/13643/commits/d0703afd72c6a84be8468e584fa849771018d166, you will find lots of changes unrelated to your PR.

wing328 avatar Nov 04 '22 06:11 wing328

Yeah when you pull master it tends to happen to pull unrelated changes, however now i'm cherrypicking the changes on a new branch.

Il giorno ven 4 nov 2022 alle ore 07:16 William Cheng < @.***> ha scritto:

totally ok that you try to resolve the merged conflicts, but if you review the files changed in this PR, many unrelated-changes are included.

— Reply to this email directly, view it on GitHub https://github.com/OpenAPITools/openapi-generator/pull/13643#issuecomment-1303024852, or unsubscribe https://github.com/notifications/unsubscribe-auth/AWGJD4AMRHOIARUFGBON4E3WGSS37ANCNFSM6AAAAAARAF5QR4 . You are receiving this because you modified the open/close state.Message ID: @.***>

-- Vittorio Parrella, Software Developer

parvit avatar Nov 04 '22 06:11 parvit