[Generator] Add support of deepObject style in query params #659
Conversation
|
We discussed this offline, this is the PR #538 reopened again, so that we can finish it up even while the original author is busy 🙂 Thanks @arthurcro for adding you both to the commit, it's important that we give credit to @kstefanou52 for the work already done! |
|
Kicked off CI to see what state we're in. |
| switch style { | ||
| case .form, .deepObject: | ||
| break | ||
| default: |
There was a problem hiding this comment.
| switch style { | |
| case .form, .deepObject: | |
| break | |
| default: | |
| switch style { | |
| case .form: | |
| break | |
| case .deepObject where explode: | |
| break | |
| default: |
There was a problem hiding this comment.
style: deepObject + explode: true is supported, but not with explode: false, so let's catch it here and provide a descriptive error.
See in the runtime changes that we only support explode: true: https://github.com/apple/swift-openapi-runtime/pull/100/files#diff-a382c8525d6209dfbc3e90daaf0262692d9df73f1b7a914b9eebe143f04c287dR76
| public init( | ||
| single: Swift.String? = nil, | ||
| manyExploded: [Swift.String]? = nil, | ||
| manyUnexploded: [Swift.String]? = nil | ||
| manyUnexploded: [Swift.String]? = nil, | ||
| sort: Operations.get_sol_foo.Input.Query.sortPayload |
There was a problem hiding this comment.
| sort: Operations.get_sol_foo.Input.Query.sortPayload | |
| sort: Operations.get_sol_foo.Input.Query.sortPayload = nil |
This should end up being defaulted to nil as well. It's possible it'll require improvements in inspecting the nested object and whether it can be initialized with .init().
There was a problem hiding this comment.
That parameter has required: true though, I don't think it should be defaulted tonil. It should be defaulted to .init() instead I think?
There was a problem hiding this comment.
Oh that's interesting! What does it mean for a query parameter of type object to be required, when all properties are optional?
On the wire, it won't look any different to the whole parameter being optional.
Maybe we should just change the sample to have an optional parameter? I think that's more common. Alternatively, make one of the properties required. Either is fine.
There was a problem hiding this comment.
Right, that's interesting. Updating the sample to have an optional parameter would resolve the issues you highlighted in the review with this test.
What if all the properties of the object have default values though? I tried getting an idea of what it would look like by updating the tests but I can get any default value to show in the generated swift types.
| } | ||
| } | ||
| public var query: Operations.get_sol_foo.Input.Query | ||
| public init(query: Operations.get_sol_foo.Input.Query = .init()) { | ||
| public init(query: Operations.get_sol_foo.Input.Query) { |
There was a problem hiding this comment.
| public init(query: Operations.get_sol_foo.Input.Query) { | |
| public init(query: Operations.get_sol_foo.Input.Query = .init()) { |
Yeah this should not go away, since all the query items are still optional, so it should be possible to init the Input with just .init().
|
Thank you for the review @czechboy0! I'll start taking a look at those changes asap! 🙂 |
|
Sorry, I couldn’t find the time to complete this PR yet. I made some progress, but there seems to be an issue with the unit tests—possibly something with the runtime library. I’ll have some time this weekend, so if @arthurcro would like any help, please let me know. |
Thanks for the update @kstefanou52! I've got some availability this weekend too and would be happy to help . If you'd prefer to complete it yourself given the progress you've made, that's completely fine - just let me know what works best for you! |
Thank you so much for the understanding @arthurcro. I'm just giving it a try and I will let you know about the outcome until Sunday end of the day. |
Motivation
The generator changes for: #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)