[Runtime] Add support of deepObject style in query params

[kstefanou52]

Motivation

The runtime changes for:
apple/swift-openapi-generator#259

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

These are just the runtime changes, tested together with generated changes.

[simonjbeaumont]

First off, thanks @kstefanou52 for taking a stab at this. I've taken a brief look over the code and left some comments. I imagine @czechboy0 will have some to add too.

[czechboy0]

Thanks for kicking off this work, @kstefanou52! A few comments, but overall this is the right approach.

[alephao]

Hello, looks like all the test cases except for the dictionary are wrong since the behavior of non-deep objects is undefined. Am I missing something?

In this implementation it looks like the formDataExplode behavior is being used as a fallback for the undefined behavior from deepObjectExplode, is that intentional?

[czechboy0]

@alephao yes these were called out in the review, see the code comments and expand the resolved ones (they don't seem to actually be updated in code yet).

[alephao]

@czechboy0 Oh, sorry, I got a bit lost in the comments and missed that one!

@kstefanou52 since this been stuck for a few weeks, I sent a PR to your fork addressing most of the requested changes (not sure if it's all there cuz I got a bit lost in the comments) kstefanou52#1

This is a summary of the changes:

• fix URISerializer for deepObject and its tests (following the spec and not handling undefined behavior)
• add deepObject tests to URIParser (there were no tests here)
• fix URICodingRoundtrip tests (I ended up ignoring the cases for deepObject when the behavior is undefined since roundtrip doesn't make much sense to me in those cases)
• remove parseBetweenCharacters function (I used parseUpToCharacterOrEnd twice instead)
• add preconditionFailure to code that should not be reachable (serializeArray with deepObject)

[kstefanou52]

@czechboy0 @alephao Sorry I was away from keyboard the last 2 weeks due to vacations. I have fixed the majority of the comments and I'll push it at the of the day.

[czechboy0]

Thanks for the updates, a few more comments

[alephao]

Hi @kstefanou52, I'm sorry that I'm being annoying but I really want this change in the upstream and the PR seem to be stuck, I updated my PR to your fork with your new changes, it already addresses all the requested changes by @czechboy0, please have a look there or if you don't have time to keep working on this I'm happy to open another PR and I can put my focus on it and finish the work you've been doing.

Again, sorry to push, I just want this feature merged soon!

[kstefanou52]

@alephao I'll push my changes until the end of day.

[kstefanou52]

Hello @simonjbeaumont @czechboy0 ! I just pushed my changes. the only thing that I'm still missing are the comments you left about Test_URiParser with the square brackets. I think that the tests are correct because it tests the nested parsing. Am I missing something ? thank you so much for the guidance and sorry for the delay. cheers!

[alephao]

@kstefanou52 thanks for the updates! I'm going to try to explain @czechboy0's points since I do agree with the changes he has requested.

Following the matrix here, the spec doesn't define the behavior of "empty", "string" or "array" unless the document is using a spec extension.

My interpretation of it is that primitives like "foo=bar", "foo=1", should throw an error or at least be ignored and the runtime should not attempt to parse them, here is my reasoning:

Imagine the following concrete example:

I have an endpoint to update user's first and last name, I like to model my sever using the deepObject style, so the decoders on my server work on inputs like user[first_name]=foo&user[last_name]=bar. My server will not handle first_name=foo&last_name=bar and I specify this to the users of my api by writing on my openapi spec that the endpoint accepts only deepObject structures. So if you try to send anything other than a deepObject, my server will return an error.

Now imagine a mocked server generated by this library, if it accepts first_name=foo&last_name=bar, it is falling to correctly mock the server, since I specifically told in my openapi spec that I only accept deepObject structures. Once I finish development and plug my code to the real server, I'll have a surprise: my code doesn't work on the live server, it only works on the mock server, because the mock server successfully parses inputs that I specifically told I do not accept.

Now what that means in terms of code that needs to be changed here is that all the URIParser's tests should stop handling the "primitive=value" case, there should be an assertion that an empty dictionary was generated (meaning that the incorrect input was ignored) or it should assert that an error was thrown.

When looking at the empty test case of the style = simple, I'm deducing that we should do the first option: just ignore the invalid input and return an empty dictionary.

Please let me know if this makes any sense or if I'm completely wrong

[kstefanou52]

@alephao if I'm not wrong @czechboy0 requested to oarse using square braces and not ignore them. But to be honest I'm not completely sure. Let's wait for his response.

[alephao]

@kstefanou52 about the brackets check the suggestions below, there is also a request to create test cases for non-escaped brackets

[czechboy0]

Yeah @alephao is exactly right here - unsupported types of objects need to throw an error, but should not parse as if they were simple/form.

[czechboy0]

The code looks okay now, just the tests need updating based on @alephao and my comments. 🙏

[kstefanou52]

Hello @czechboy0 and @alephao, could you please take a look at Test_URICodingRoundtrip? I'm not entirely confident that all the test scenarios are correct, especially those involving a struct with a custom Codable implementation that forwards, and handling AnyOf cases. Thanks!

[alephao]

Hi @kstefanou52, let me know if I'm wrong, but I believe that the point of these roundtrip tests is to check that a successfully encoded value can also be decoded, all the tests expecting errors won't encode nor decode anything, so they end up missing the point.

I think would be nice if we could do something like this:

// Note: This code won't work, it's just an idea
deepObjectExplode: .custom(
   "obj[who]=fred",
   value: ["who": "fred"]
)

But that code won't work, I pulled your latest changes and spent a bit of time hammering a way to test the deepObject in the current setup (with the _test function), but I wasn't happy with anything I tried, code was getting spaghetti and hard to reason about.

So I tried writing a separate function to test deepObjects which ended up working well. It's almost the same as the generic code in _test.

// func definition
func _testDeepObject<T: Codable & Equatable>(key: String, value: T, encodedString string: String, file: StaticString = #file, line: UInt = #line) throws {
    let encoder = URIEncoder(configuration: .deepObjectExplode)
    let encodedString = try encoder.encode(value, forKey: key)
    XCTAssertEqual(encodedString, string, "Variant: deepObjectExplode", file: file, line: line)
    let decoder = URIDecoder(configuration: .deepObjectExplode)
    let decodedValue = try decoder.decode(T.self, forKey: key, from: encodedString[...])
    XCTAssertEqual(decodedValue, value, "Variant: deepObjectExplode", file: file, line: line)
}

// usage example
try _testDeepObject(key: "obj", value: ["root": "Hello World!"], encodedString: "obj%5Broot%5D=Hello%20World%21")

I'm not sure if it's the best approach, but it does work and it fulfills what I believe to be the goal of these tests

[kstefanou52]

Thank you @alephao for reviewing it. I completely agree with your comment. However, since we are only supporting encoding dictionaries and not primitive values, I thought it would be appropriate to throw the necessary error and test this behavior. Let's wait for @czechboy0 to confirm how we should proceed.

[czechboy0]

Yes let's only support dictionaries, by my read of the specification, that's the only type that has defined rules. It's better to throw an error for anything else, and if we figure out that there are other types we should support, we can always add them. But adding them is easier than removing them :)

[kstefanou52]

@czechboy0, are there any further changes needed? I apologize for being a bit lost with all these conversations. Thank you!

[czechboy0]

Ok looks good to me! Let's run the CI and get that to green now.

[czechboy0]

@swift-server-bot test this please

[czechboy0]

The API breakage failure is expected:

💔 API breakage: enumelement ParameterStyle.deepObject has been added as a new enum case

It's in the SPI that only generated code uses, so it's okay to allow this "breakage" (it's not really, it was designed to be extended).

[czechboy0]

Ok soundness failed, please run this locally and check in the changes:

./scripts/run-swift-format.sh --fix

[czechboy0]

@swift-server-bot test this please

[kstefanou52]

@czechboy0, I think the soundness check must be green now.

[czechboy0]

@swift-server-bot test this please