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.

Sign up for GitHub

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

Jump to bottom

Choose the serialization method based on content type #43

Closed
czechboy0 opened this issue May 27, 2023 · 0 comments · Fixed by #48 or apple/swift-openapi-runtime#12
Closed

Choose the serialization method based on content type #43

czechboy0 opened this issue May 27, 2023 · 0 comments · Fixed by #48 or apple/swift-openapi-runtime#12
Assignees
@czechboy0
Labels
area/runtime Affects: the runtime library. kind/bug Feature doesn't work as expected. semver/patch No public API change. size/L Large task. (A couple of weeks of work.)

Comments

@czechboy0
Copy link
Collaborator

Currently, the generator chooses the serialization method (JSON, text-based, or raw data) based on the type used in the request/response body. Most of the time, that works correctly, but there are some corner cases, such as:

  • content type: application/json, the body is a fragment string (as opposed to a JSON container, like an object or an array).

In this case, we today serialize it using LosslessStringConvertible, and end up without the quotes. That's only correct if the content type is text/plain (and in fact was just fixed in apple/swift-openapi-runtime#9).

Action: add logic for choosing the serialization method based on the content type, instead of trying to deduce it from the Swift type.

Whilst implementing this change, I think that we may want to revisit the encoding logic, since I think the knowledge of whether to encode a value as binary data, JSON, or plain text, is at the place where the content-type is set, but we'll defer that to a subsequent PR.

From: apple/swift-openapi-runtime#10
@czechboy0 czechboy0 added semver/patch No public API change. area/runtime Affects: the runtime library. kind/bug Feature doesn't work as expected. labels May 27, 2023
This was referenced May 27, 2023
Closed
Closed
@czechboy0 czechboy0 self-assigned this May 30, 2023
@czechboy0 czechboy0 added the size/M Medium task. (A couple of days of work.) label May 30, 2023
This was referenced May 31, 2023
Merged
Merged
czechboy0 added a commit to czechboy0/swift-openapi-generator that referenced this issue Jun 6, 2023
@czechboy0
[Docs] Document the Converter type in preparation for apple#43
267c1f0
@czechboy0 czechboy0 mentioned this issue Jun 6, 2023
Merged
czechboy0 added a commit that referenced this issue Jun 7, 2023
@czechboy0
[Docs] Document the Converter type and all the cases it needs to supp…
7186fd5 
…ort (#50)

[Docs] Document the Converter type and all the cases it needs to support

### Motivation

I wrote down how the Converter works and all the cases we need to support. That'll help prepare ground for the refactor needed for #43.

Also created space for maintainer-focused docs.

### Modifications

Added docs, which I'll use in a subsequent PR to ensure all the cases listed are implemented and tested.

### Result

Converter now has higher level docs, and maintainers have a place to add more docs.

### Test Plan

Previewed locally.


Reviewed by: simonjbeaumont

Builds:
     ✔︎ pull request validation (5.8) - Build finished. 
     ✔︎ pull request validation (5.9) - Build finished. 
     ✔︎ pull request validation (nightly) - Build finished. 
     ✔︎ pull request validation (soundness) - Build finished. 

#50
@czechboy0 czechboy0 added size/L Large task. (A couple of weeks of work.) and removed size/M Medium task. (A couple of days of work.) labels Jun 8, 2023
czechboy0 added a commit to apple/swift-openapi-runtime that referenced this issue Jun 8, 2023
@czechboy0
[Runtime] Choose the serialization method based on content type (#12)
3c21ae3 
[Runtime] Choose the serialization method based on content type

### Motivation

Resolves apple/swift-openapi-generator#43. This PR also resolves apple/swift-openapi-generator#42. It's paired with apple/swift-openapi-generator#48.

This PR introduces the initial content-type-aware coding strategy, which is likely to get expanded in the future.

### Modifications

This PR covers the scope described in issue 43 - handling a `type: string` appropriately, based on whether it's wrapped in `application/json` or `text/plan` content.

It introduces new variants of the SPI helper functions, which now has variants per "coding strategy". It's best to read https://github.com/apple/swift-openapi-generator/pull/48/files?short_path=aa92e58#diff-aa92e58fb5a311512cfcd285827a4aa2e6634cae7fdfe0090bd2cfc7b0986140 for details.

Deprecated all of the SPI methods that didn't contain the coding strategy parameter, to be removed in the next breaking version.

### Result

We now have explicit control over the coding strategy for ambiguous types, such as `String`, which conform to both `Codable` and `LosslessStringConvertible`, to be taken advantage of by the generator (see the paired PR).

### Test Plan

Verified that the default behavior now matches version 0.1.0, and when paired with the updated generator, fully resolves issue 43.

Used code coverage to ensure that `Converter+{Common,Client,Server}.swift` are now 100% unit tested.


Reviewed by: simonjbeaumont

Builds:
     ✔︎ pull request validation (5.8) - Build finished. 
     ✔︎ pull request validation (5.9) - Build finished. 
     ✔︎ pull request validation (nightly) - Build finished. 
     ✔︎ pull request validation (soundness) - Build finished. 

#12
@czechboy0 czechboy0 mentioned this issue Jun 8, 2023
Closed
czechboy0 added a commit that referenced this issue Jun 8, 2023
@czechboy0
[Generator] Choose the serialization method based on content type (#48)
8b9c564 
[Generator] Choose the serialization method based on content type

### Motivation

Fixes #43. Depends on apple/swift-openapi-runtime#12 landing first and tagging a release.

### Modifications

Builds on top of the changes to the runtime library.

### Result

We now always specify a coding strategy with a Swift type, leading to deterministic and understandable conversion logic.

### Test Plan

Updated unit and integration tests, added a `500` case to one of the operations to explicitly test the missing plain text response body.


Reviewed by: simonjbeaumont

Builds:
     ✔︎ pull request validation (5.8) - Build finished. 
     ✔︎ pull request validation (5.9) - Build finished. 
     ✔︎ pull request validation (nightly) - Build finished. 
     ✔︎ pull request validation (soundness) - Build finished. 

#48
@czechboy0 czechboy0 mentioned this issue Jun 8, 2023
Merged
czechboy0 added a commit to apple/swift-openapi-runtime that referenced this issue Jun 8, 2023
@czechboy0
[Fix] Re-introduce deprecated methods (#15)
2c093b0 
[Fix] Re-introduce deprecated methods

### Motivation

Unfortunately, as part of landing apple/swift-openapi-generator#43, I made a mistake during backwards compatibility testing and forgot to add a few deprecated methods still used by generator 0.1.0.

That means that generator 0.1.0 doesn't work with runtime 0.1.1, currently.

This PR reintroduces them, so that generator 0.1.0 can work with a hotfix 0.1.2 again.

### Modifications

Re-added a few missing deprecated methods on Converter.

### Result

Someone using generator 0.1.0 with the latest runtime library (e.g. if they locked their generator version to 0.1.0 but didn't lock their runtime version) will have the generated code compiling again with runtime 0.1.2 (once this fix is landed).

### Test Plan

Checked locally by actually checking our the different tags of runtime and generator and built them together.


Reviewed by: simonjbeaumont

Builds:
     ✔︎ pull request validation (5.8) - Build finished. 
     ✔︎ pull request validation (5.9) - Build finished. 
     ✔︎ pull request validation (nightly) - Build finished. 
     ✔︎ pull request validation (soundness) - Build finished. 

#15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@czechboy0 czechboy0

Labels
area/runtime Affects: the runtime library. kind/bug Feature doesn't work as expected. semver/patch No public API change. size/L Large task. (A couple of weeks of work.)
Projects
None yet
Milestone
No milestone
1 participant
@czechboy0