Update URIValueFromNodeDecoder for DeepObject.#125
Closed
kstefanou52 wants to merge 1 commit intoapple:mainfrom
kstefanou52:feature/deep_object_style_decode_fix
Closed
Update URIValueFromNodeDecoder for DeepObject.#125kstefanou52 wants to merge 1 commit intoapple:mainfrom kstefanou52:feature/deep_object_style_decode_fix
kstefanou52 wants to merge 1 commit intoapple:mainfrom
kstefanou52:feature/deep_object_style_decode_fix
Conversation
### 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`.
Contributor
|
@kstefanou52 When reviewing this change I realized we'll need a few more changes here, which I'll look into doing next week. I'll let you know when they're ready. |
Contributor
Author
Sure! Let me know if you would like some help on this! |
Contributor
|
Okay I have the new PR coming up shortly, I ended up completely refactoring URIParser to make this more maintainable and allow handling deepObject values correctly. I copied some of the test cases from your PR - thanks for those! Once we land my PR, you should be unblocked again to finish the deepObject support in the generator repo. |
Contributor
|
Ok @kstefanou52 here's my PR: #127 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
As discussed in [Generator] Add support of deepObject style in query params #538, there is a misimplementation of the
decodeRootIfPresentmethod inURIValueFromNodeDecoder. When using thedeepObjectstyle, the node forrootValueis correctly empty, containing only the sub-objects.Without this PR, the tests for Add support of deepObject style in query params #538 are failing.
Modifications
decodeRootIfPresent(_ type:) throws -> Tmethod to ignore whethercurrentElementAsArrayis empty or not.Result
Test Plan
This PR includes unit tests that validate the change to
decodeRootIfPresent(_ type:) throws -> TwithinTest_Converter+Serveras well as inTest_serverConversionExtensions.