Skip to content
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

Introduce URIEncoder and URIDecoder types #41

Merged
merged 20 commits into from
Aug 29, 2023

Conversation

czechboy0
Copy link
Contributor

@czechboy0 czechboy0 commented Aug 21, 2023

Motivation

Fixes apple/swift-openapi-generator#192.

For refactoring how we encode:

  • path parameters
  • query parameters
  • headers
  • (new feature, coming later) application/x-www-form-urlencoded bodies

Supports:

  • form + explode
  • form + unexplode
  • simple + explode
  • simple + unexplode
  • form where space is encoded as + instead of %20 (for bodies) + explode
  • form where space is encoded as + instead of %20 (for bodies) + unexplode

Modifications

First step - introduce two new types: URIEncoder and URIDecoder.

They're configurable types that can handle the URI Template (RFC 6570) form and simple styles, refined by OpenAPI 3.0.3, and also the application/x-www-form-urlencoded format (mostly identical to URI Template).

Result

The types can be used now, subsequent PRs will integrate them.

Test Plan

Added unit tests for the individual parts of the coder, but also roundtrip tests.

@bfrearson
Copy link
Contributor

This looks great!

I think it's flexible enough to use wherever.

Do you think we should support percent encoding from the beginning? I can see a lot of things breaking if you included a literal = or & in the input.

@czechboy0
Copy link
Contributor Author

Do you think we should support percent encoding from the beginning? I can see a lot of things breaking if you included a literal = or & in the input.

Ah yes, I didn't mention this in the PR description - this doesn't do any percent escaping yet, I first wanted to get feedback on the architecture. I'll add escaping etc before moving this into proper review.

@czechboy0
Copy link
Contributor Author

@bfrearson Ok so I got it all working end to end now, without nesting (that's a future enhancement) - WDYT? This isn't polished yet, but tests are passing.

Copy link
Contributor

@bfrearson bfrearson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks really good - just that minor suggestion about using an enum for space encoding

@czechboy0 czechboy0 changed the title [Prototype] URIEncoder Introduce URIEncoder and URIDecoder types Aug 25, 2023
@czechboy0 czechboy0 marked this pull request as ready for review August 25, 2023 14:12
Copy link
Contributor

@glbrntt glbrntt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

czechboy0 added a commit that referenced this pull request Aug 29, 2023
Introduce StringEncoder and StringDecoder types

### Motivation

A sibling PR to #41, as part of moving from hand-crafted code that uses `LosslessStringConvertible` conformances on built-in and generated types, we need not just an URI coder, but also a raw string coder.

This will be used for encoding and decoding `text/plain` bodies, where only primitive types can be used, and are encoded as raw strings.

### Modifications

Introduced two types, `StringEncoder` and `StringDecoder`. It's very simple, doesn't support containers, only primitive types.

### Result

Once this lands, it'll allow us to get rid of our ugly marker protocols `_StringConvertible` and `_AutoLosslessStringConvertible` and always go through `Codable`. More on this in subsequent PRs integrating this logic.

### Test Plan

Added a rountrip unit test.


Reviewed by: glbrntt

Builds:
     ✔︎ pull request validation (5.8) - Build finished. 
     ✔︎ pull request validation (5.9) - Build finished. 
     ✔︎ pull request validation (api breakage) - 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. 

#44
@czechboy0 czechboy0 merged commit 0769bd0 into apple:main Aug 29, 2023
@czechboy0 czechboy0 deleted the hd-uri-encoder branch August 29, 2023 12:54
@czechboy0 czechboy0 added the 🔨 semver/patch No public API change. label Aug 30, 2023
czechboy0 added a commit to apple/swift-openapi-generator that referenced this pull request Aug 30, 2023
[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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔨 semver/patch No public API change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create a URI encoder and decoder that works with Codable types
3 participants