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

[Generator] Add support of deepObject style in query params #538

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

kstefanou52
Copy link

@kstefanou52 kstefanou52 commented Mar 7, 2024

Motivation

The generator changes for:
apple/swift-openapi-generator

Depends on apple/swift-openapi-runtime#100
landing first and getting released, and the version dependency being bumped here.

Modifications

Added deepObject style to serializer & parser in order to support nested keys on query parameters.

Result

Support nested keys on query parameters.

Test Plan

Adapted snippet tests (SnippetBasedReferenceTests)

### Motivation

apple#259

~Depends on apple/swift-openapi-runtime#100
landing first and getting released, and the version dependency being
bumped here.~

### Modifications

Added `deepObject` style to serializer & parser in order to support nested keys on query parameters.

### Result

Support nested keys on query parameters.

### Test Plan

Adapted snippet tests (SnippetBasedReferenceTests)
@simonjbeaumont
Copy link
Collaborator

Thanks @kstefanou52 for taking the time to tackle this. We'll park this PR review until we land the runtime PR.

@simonjbeaumont simonjbeaumont added the status/blocked Waiting for another issue. label Mar 7, 2024
@czechboy0
Copy link
Collaborator

@kstefanou52 Ok please bump the runtime dependency to 1.4.0 in Package.swift and you should be able to finish the generator changes.

@czechboy0 czechboy0 removed the status/blocked Waiting for another issue. label Apr 16, 2024
@kstefanou52
Copy link
Author

@czechboy0 Done! I have created some tests, but I'm not sure if you would like to add more. Could you please suggest any additional tests you think might be necessary? Thank you!

Copy link
Collaborator

@czechboy0 czechboy0 left a comment

Choose a reason for hiding this comment

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

One code change suggested, and regarding the extra tests - you're right 🙂

Start by adding a similar parameter like you did in the snippet tests, but this time to petstore.yaml, which makes it available in generated form in PetstoreConsumerTests. There, please extend one client and one server unit test to verify that the serialization/deserialization is correctly piped through all the way. The relevant test classes are Test_Client and Test_Server.

@simonjbeaumont
Copy link
Collaborator

Just checking in on this one. Is it ready for review again? (Thanks for the effort here!)

@kstefanou52
Copy link
Author

Hi @simonjbeaumont! Unfortunately some tests are failing, I'll work on this on weekend and I'll let you know. Cheers!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants