-
Notifications
You must be signed in to change notification settings - Fork 116
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
base: main
Are you sure you want to change the base?
Conversation
Thanks @kstefanou52 for taking the time to tackle this. We'll park this PR review until we land the runtime PR. |
@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 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! |
There was a problem hiding this 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
.
Sources/_OpenAPIGeneratorCore/Translator/Parameters/TypedParameter.swift
Outdated
Show resolved
Hide resolved
Just checking in on this one. Is it ready for review again? (Thanks for the effort here!) |
Hi @simonjbeaumont! Unfortunately some tests are failing, I'll work on this on weekend and I'll let you know. Cheers! |
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. |
@czechboy0 I'm apologising for my late response. I'll try to catch up and complete the PR by the end of the week. |
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)
4d6f297
to
7c84475
Compare
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. |
Never mind, found your other comment. |
Tests/OpenAPIGeneratorReferenceTests/SnippetBasedReferenceTests.swift
Outdated
Show resolved
Hide resolved
…s.swift Co-authored-by: Honza Dvorsky <[email protected]>
### 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`.
Hey @czechboy0, I just opened the runtime PR. Please let me know if there are any additional tests we should add. Thanks! |
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)