Skip to content

Conversation

czechboy0
Copy link
Contributor

@czechboy0 czechboy0 commented May 27, 2023

Motivation

When we fixed plain text bodies in apple/swift-openapi-runtime#9, we surfaced an existing issue now tracked by #43. Coincidentally, the generator integration test actually contained an instance of this uncommon pair of content-type: application/json + body: fragment string. So the test was broken before this PR.

This PR can be considered a follow-up of apple/swift-openapi-runtime#9.

Modifications

Until #43 is addressed, we can't both have plain text responses working correctly, and also continue to support the case of a JSON string as a body, so I changed the reference test to instead use the (more common) plain text response. Updated accordingly, all the changes are in tests only.

Result

The reference test now passes again with the latest version of the runtime library.

Test Plan

This whole PR is to fix a test, so ensured all tests pass.

@czechboy0
Copy link
Contributor Author

@swift-server-bot test this please

@simonjbeaumont
Copy link
Collaborator

simonjbeaumont commented May 29, 2023

So IIUC we fixed a bug in apple/swift-openapi-runtime#9 and introduced a new one and are making the choice of which one we want to live with in the short term.

Specifically, we are trading the the ability to correctly encode...

            application/json:
              schema:
                type: string

...for the ability to correctly encode...

            text/plain:
              schema:
                type: string

When we incorrectly encode the text/plain string, we get "value" (extraneous quotes). When we incorrectly encode the application/json string, we get value (no quotes, which is not valid JSON).

Coincidentally, the generator integration test actually contained an instance of this uncommon pair of content-type: application/json + body: fragment string.
...
Until #43 is addressed, we can't both have plain text responses working correctly, and also continue to support the case of a JSON string as a body, so I changed the reference test to instead use the (more common) plain text response.

I sampled the handful of OpenAPI documents I had to hand and the JSON string fragment actually appeared more times than the plain-text string, including in the canonical Petstore API that the OpenAPI initiative maintain0.

I think this tells us that it's not completely niche to use a JSON string and IMO we need to prioritise a fix for #43—do you agree?

On the subject of which is best in the short term, I'd like us to go into this change eyes-open.

Is it the case that before apple/swift-openapi-runtime#9 we handled all the application/json schemas correctly, but that we sometimes had a "false positive" and encoded as JSON when we shouldn't?

If this is the case, then moving in the proposed direction (favouring the plain string and regressing the JSON string) means we go from supporting all JSON schemas and having some issues with non-JSON response bodies, to a much more complex narrative, where we support most of JSON schemas and some of the text schemas.

Additionally we should account for the fact that a content-type of application/json indicates (valid) JSON and text/plain is not formally structured, so it's arguably more likely that a JSON response will be forwarded to automation than the plain version, which may be more likely to be viewed or processed manually.

Viewing a plain string with an extraneous set of quotes around it might cause less unwanted side effects than not correctly quoting the JSON string (invalid JSON), which might break automation.

With this in mind, do you still think we should be favouring the plain-text string over the JSON string in the short term?

@czechboy0
Copy link
Contributor Author

Closing this without merging, let's just address #43 🙂

@czechboy0 czechboy0 closed this May 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants