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 5 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)

@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!

@czechboy0
Copy link
Collaborator

Hi @kstefanou52 - do you expect to be able to come back to finish this? 🙂 It's also okay if you can't and someone else will pick up where you left off.

@kstefanou52
Copy link
Author

@czechboy0 I'm apologising for my late response. I'll try to catch up and complete the PR by the end of the week.

@czechboy0
Copy link
Collaborator

Hi @kstefanou52 - let us know if you need any help with this? I think it just needs to be updated from the main branch and a conflict resolved, but otherwise might be good to go?

apple#259

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

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

Support nested keys on query parameters.

Adapted snippet tests (SnippetBasedReferenceTests)
@kstefanou52
Copy link
Author

Hello @czechboy0 and @arthurcro, I've made some progress, but I'm facing an issue with one of the unit tests. The code appears to work as expected, but I suspect there may be a problem with the runtime library. I’d appreciate your thoughts on this; more details can be found here.

@czechboy0
Copy link
Collaborator

czechboy0 commented Oct 26, 2024

Can you elaborate? The link doesn't really say what the issue is - what are you seeing and what are you expecting?

Never mind, found your other comment.

kstefanou52 added a commit to kstefanou52/swift-openapi-runtime that referenced this pull request Oct 29, 2024
### Motivation

As discussed in [[Generator] Add support of deepObject style in query params #538](apple/swift-openapi-generator#538 (comment)), there is a misimplementation of the `decodeRootIfPresent` method in `URIValueFromNodeDecoder`. When using the `deepObject` style, the node for `rootValue` is correctly empty, containing only the sub-objects.

Without this PR, the tests for [Add support of deepObject style in query params #538](apple/swift-openapi-generator#538) are failing.

### Modifications

- Updated the `decodeRootIfPresent(_ type:) throws -> T` method to ignore whether `currentElementAsArray` is empty or not.

### Result

- Enables the tests in the [Generator part of the PR](apple/swift-openapi-generator#538) to pass successfully.

### Test Plan

This PR includes unit tests that validate the change to `decodeRootIfPresent(_ type:) throws -> T` within `Test_Converter+Server` as well as in `Test_serverConversionExtensions`.
@kstefanou52
Copy link
Author

Hey @czechboy0, I just opened the runtime PR. Please let me know if there are any additional tests we should add. Thanks!

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.

3 participants