-
Notifications
You must be signed in to change notification settings - Fork 131
Support URL form encoded request bodies #182
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
Comments
Hi @benfrearson, welcome to the project! Thanks for taking the time to file the issue with so much great detail! You're right, Swift OpenAPI Generator doesn't yet support URL encoded form bodies as a structured type, but it's definitely something we'd accept a PR for! It just hasn't been implemented yet as we're still working through other foundational OpenAPI features. Since adding support for a new kind of a structured body will not be a trivial amount of work, the best way might be for you to try to prototype it (even including some shortcuts) and open a draft PR that we can then discuss the approach in. I don't want you to spend time polishing a PR until we confirm that the overall approach is likely to be accepted. If this sounds good to you, I'll be looking forward to your PR and if you have any questions, you can ask under this issue. Thanks! 🙏 |
Also, this piece of documentation could be helpful, you'll potentially need to expand the set of helper functions: https://swiftpackageindex.com/apple/swift-openapi-generator/0.1.8/documentation/swift-openapi-generator/converting-between-data-and-swift-types |
Thanks! Yes, I was anticipating using the Converter to handle this. How does this sound as a general approach from the Client interface side:
Assumptions:
Considerations/scope:
I think for a first-pass at this, we should focus on conventional url form key-value pairs while maintaining the existing implementation. I assume this would cover a large percentage of endpoints that use url forms. We could extend this in the future with helpers or options for other encoding types. |
Thanks for writing up the assumptions and considerations, that's really the main thing we have to agree on, the implementation should hopefully most flow out of that.
That might be the case, but let's first explore what it'd take to support all the payloads that RFC 3986 covers (I'm assuming URL encoded forms with the I'd like us to see if we can use the The benefit of this is that we wouldn't limit support to flag key-value pairs, but we'd support any arbitrary content that can be describes with JSON schema and encoded as If we find this is too difficult to achieve, then we can scale back and talk about only supporting a subset.
Can you say more about this? Today, you can specify
Not sure what you mean by this, can you provide an example?
If we use the custom Encoder/Decoder approach, and find multiple values for a non-array type (as for arrays, this would be the standard way to encode it when
For sure, hopefully how we generate Codable structs today would be sufficient. It works well for JSON, I don't currently see why it wouldn't be enough for URL encoded forms as well.
Can you clarify what you mean by "must allow". It could mean: Currently with JSON, we do A when
This isn't a requirement! We're still pre-1.0, and we already have a few breaking API changes queued up behind feature flags that we'll enable unconditionally in 0.2.0 (next breaking version). I think that with support of So don't worry about backwards compatibility too much. If it's not-API breaking, it can land in the current minor release. If it's API breaking, we'd land it disabled behind a feature flag and enable in the next minor (API-breaking) release. Either way, the goal here is to make the support as good as possible for adopters, not to retain an API that we are continuing to evolve. 🙂
Please elaborate on this as well, ideally with an example, I haven't worked with URL encoded forms very much yet. To summarize: let's try for putting URL encoded form support on the same level as JSON by trying to build a custom Encoder/Decoder and use the existing generated Codable types. Backwards API compatibility is a non-goal pre-1.0, so let's design the best solution, and whether it's API breaking only affects when exactly it lands. If all this sounds good to you, I think a good next step would be a prototype of this approach to discover all the unknown unknowns. |
On the queries about arbitrary field names: I thought it feasible—but probably very bad practice—that you could design an endpoint which would accept any field name, regardless of whether it's included in a schema. For example, you might have a
Again, I don't have any concrete examples of this, and it would be fairly odd imo, but I think it would be possible to write an endpoint that did this (however weird)... We can absolutely aim for parity with JSON support, and that would be preferable in the particular instance that drew me here: I agree that we want as much type safety as possible. My only hesitation with this approach is that we're making the assumption that every form request is equally as uniform as a JSON object. I'm happy to move ahead with this approach though and see if it causes any actual issues. The edge cases I suggested are feasible, but perhaps minimal, and it may be good to get the ball rolling with covering the best practices first. As an example of things assuming a certain conformance in this space, Postman rigidly won't accept anything other than key-value pairs for url forms, so I would assume that means most people don't try and do weird stuff with them: |
That's actually already supported in JSON Schema: type: object
additionalProperties:
type: boolean That will generate you a The only thing to keep in mind is that the author of the OpenAPI document has to explicitly specify it, but I think that's reasonable and so far nobody has complained about that behavior for JSON. |
ok great! that answers that question then! 👍🏻 |
Thinking about this more, creating a proper implementation of a URL form encoder and decoder (for To that end, I'd suggest we focus on that problem first - can we build a Once we have a decent encoder and decoder in the runtime library, including unit tests, we can discuss how to a) add support for the new content types in the generator itself (should be straightforward at that point) and b) how to migrate the existing query item and header encoding logic over to this new infra (can be done separately, but should result in a bunch of code removed). This might take a few PRs, so we could introduce it in the runtime library under I took a brief look at Foundation's JSONEncoder/JSONDecoder implementation and I think they have a good structure we could get inspired by, with the difference that the URLFormEncoder/Decoder should be even simpler, as we really only have 4 fundamental types (string, array, dictionary, and null), whereas JSON has a few more. How does this approach sound? |
Splitting the creation of the new type into a separate issue, to split it from actually supporting the new content type in bodies: #192 |
I've just put up a first pass attempt at implementing a rudimentary version with hardcoded converters, but I fully agree that having a custom encoder is preferable and something that we can work on next. I'm happy to continue with that. I have a few more thoughts, but will add these to #192 |
An update from my side: the URI coder will be landing in apple/swift-openapi-runtime#41. Once that lands, I'll move all of the parameters to it (already WIP), deprecate a bunch of code, and we'll release a breaking 0.2.0 hopefully some time next week. After that's released, all the pieces should be in place for you to pick this work up again, and add URL form encoded body support. I'll let you know when it's ready, I'd like to avoid creating a bunch of merge conflicts in on your branches, so it's better to wait. |
[Generator] Integrate the new URI and String coders ### Motivation Depends on runtime changes from apple/swift-openapi-runtime#45. Up until now, we relied on a series of marker and helper protocols `_StringConvertible` and `_AutoLosslessStringConvertible` to handle converting between various types and their string representation. This has been very manual and required a non-trivial amount of work to support any extra type, especially `Date` and generated string enums. Well, turns out this was an unnecessarily difficult way to approach the problem - we had a better solution available for a long time - `Codable`. Since all the generated types and all the built-in types we reference are already `Codable`, there is no need to reinvent a way to serialize and deserialize types, and we should just embrace it. While a JSON encoder and decoder already exists in Foundation, we didn't have one handy for encoding to and from URIs (used by headers, query and path parameters), and raw string representation (using `LosslessStringConvertible`). We created those in the runtime library in PRs apple/swift-openapi-runtime#44 and apple/swift-openapi-runtime#41, and integrated them into our helper functions (which got significantly simplified this way) in apple/swift-openapi-runtime#45. Out of scope of this PR, but this also opens the door to supporting URL form encoded bodies (#182), multipart (#36), and base64 (#11). While this should be mostly invisible to our adopters, this refactoring creates space for implementing more complex features and overall simplifies our serialization story. ### Modifications - Updated the generator to use the new helper functions. - Updated the article about serialization, shows how we reduced the number of helper functions by moving to `Codable`. - Set the `lineLength` to 120 on the formatter configuration, it was inconsistent with our `.swift-format` file, and lead to the soundness script trying to update the reference files, but then the reference tests were failing. Since we're planning to sync these in #40, this is a step closer to it, but it means that it's probably best to review this PR's diff with whitespace ignored. ### Result Now the generated code uses the new helper functions, allowing us to delete all the deprecated helpers in 0.2.0. ### Test Plan Updated file-based reference, snippet, and unit tests. Reviewed by: glbrntt Builds: ✔︎ pull request validation (5.8) - Build finished. ✔︎ pull request validation (5.9) - Build finished. ✔︎ pull request validation (docc test) - Build finished. ✔︎ pull request validation (integration test) - Build finished. ✔︎ pull request validation (nightly) - Build finished. ✔︎ pull request validation (soundness) - Build finished. #226
Sweet, thanks! I've been away for the past week, but I'll take a look at this later in the week. |
### Motivation [Issue 182](apple/swift-openapi-generator#182) Supporting runtime update for [add URL form encoder & decoder](apple/swift-openapi-generator#283) ### Modifications Add converter methods for Codable <--> URLEncodedForm. **Note:** Trying to encode a primitive collection (e.g. an array of `String`) form a Codable type fails because the URIEncoder rejects nested object types. I didn't make any changes to the Encoder here, as I think there may be a larger discussion needed about how to approach this. It would be beneficial to support this, as there may be a form which contains multiple values for the same key. Decoding an exploded key from to a Codable type containing an array works already, but I've not included the test in this PR, as that should be included in the above. ### Result Converter methods for URLEncodedForms are now available for use in generated content. ### Test Plan Update tests for encoders and decoders. --------- Co-authored-by: benfrearson <[email protected]> Co-authored-by: bfrearson <> Co-authored-by: Honza Dvorsky <[email protected]>
### Motivation #182 Currently, the generated interface for request bodies that use URLEncodedForms accepts a `Data` object. This PR uses the URIEncoder/Decoder to instead provide an interface using codable types in the same way that a JSON body would. ### Modifications Adds a new coding strategy that works with the corresponding [runtime converter updates](apple/swift-openapi-runtime#53). **Note:** This PR does not support encoding collections of primitives (i.e. an array of string values for a single key). This should be addressed in a future update, but it raised questions about the correct use of URIEncoder, so I didn't include it in this PR. Further discussion of the issue is in the [runtime PR](apple/swift-openapi-runtime#53). ### Result Generated interfaces now directly accept URLEncodedForm Codable types rather than untyped `Data`. ### Test Plan Updated reference tests to include a request using a form body. --------- Co-authored-by: benfrearson <[email protected]> Co-authored-by: bfrearson <> Co-authored-by: Honza Dvorsky <[email protected]>
Landed behind the feature flag |
Released in 0.2.3 of the generator, behind the |
There may be a legitimate reason why this isn't already implemented, or perhaps I'm not using the generator for its intended use case - if so, I'd appreciate some clarity.
It appears to me that URL form encoded requests have their data structure erased in the generated Input.body type, which blindly expects
Data
.The issue here is that using the corresponding
Client
method now is unaware of the required url form fields. This is obviously fairly easy to work around, but it obfuscates the expected data from the users of theClient
in a way that using a JSON encoded body does not.It would be better if the generated Input type could handle encoding form bodies itself, such as by:
Input.body
initializer that accepts arguments as dictated by the form fields.converter.setRequiredRequestBodyAsBinary()
method.I would be happy to contribute to this issue, but I'd like to confirm that this is something that is needed/wanted and aligns with the project's direction.
The text was updated successfully, but these errors were encountered: