[Runtime] Improve parameter handling of MIME types in content types #113
Conversation
| guard case .concrete(let substringType, let substringSubtype) = parsedSubstring.kind else { | ||
| // If the substring content type has a wildcard, just let it through. | ||
| // It's not well defined how such a case should behave, so be permissive. | ||
| return | ||
| } |
There was a problem hiding this comment.
Is this case correct?
The function is supposed to throw an error if the Accept header is present but incompatible with the provided content type. The comments here indicate that this case is handling where the header is wildcards, and there is such a case in the deleted part of the diff, but it looks like this handles any .concrete with arbitrary associated values for substringType and substringSubtype. Is this missing some use of .any values?
Currently, adding the following to the tests fails,
let cases: [(HTTPFields, String, Bool)] = [
+ ([.accept: "text/plain"], "application/*", false),There was a problem hiding this comment.
Yeah this case isn't handled correctly in this shape, I tried to explain that in the comment in the else block. In general we don't have clarity on what to do with e.g. application/* (a partial wildcard) being the content type in the OpenAPI doc (as opposed to wildcards being part of the Accept header, which is well-defined). That's why I opted to go more relaxed here, as I don't know what to back up with being stricter.
I guess it comes down to whether we think of application/* as a content type that we have any information about whatsoever (so far I've though the answer is no, and treated it equally to */*). Maybe that's not the right place to land, and we can come up with rules that constrain it more. We just run the risk of getting it slightly wrong at which point users won't have an escape hatch. But I could be convinced of going in either direction.
There was a problem hiding this comment.
I'd have thought that if we have a partial, we would validate that the top-level types match in the validate function.
I can see that a partial wildcard, e.g. application/*, is a strange thing to return in the Content-Type header field of a HTTP response, but it's clear that it doesn't match an Accept header of text/plain, or text/*, or anything that doesn't start with application/.
For this reason, I think the least surprising semantics of this validate function is to reject it.
To pull on this thread a little more though:
- Is a partial wildcard content-type for a response valid OpenAPI (according to OAS)?
- If yes, then is that just an oversight? I.e. if it is valid, is it typically always a mistake, or is there a genuine use case for it?
The answers to these two questions might yield some more validation and/or warning generation when the generator runs.
There was a problem hiding this comment.
OpenAPI does allow partial and full wildcards in the content type, but I've not come up with a way to treat that any differently to raw data that we know nothing about.
Agreed that we should do at least the type match. I'll update the PR.
There was a problem hiding this comment.
From chatting with folks in the team and consulting the RFCs, I think we need to adjust this differently.
For Content-Type: a/*, I suggested it should be permitted if Accept: a/b but we should only permit if Accept: a/* because we should treat a/* as a concrete MIME type with * being an unrecognised, but concrete (not wildcard) subtype, when in the Content-Type header field.
There was a problem hiding this comment.
Ok, updated with this more correct matching. Check out the unit tests, I can add more if there's a case I missed.
| guard let parsedAcceptValue = OpenAPIMIMEType(acceptValue) else { | ||
| throw RuntimeError.invalidExpectedContentType(acceptValue) | ||
| } |
There was a problem hiding this comment.
How permissive would we like to be here? If a client sends an Accept header with a valid content-type that the server supports, and an invalid one, should the server permit it? IIUC, this guard statement suggests not, but is that the desired behaviour?
There was a problem hiding this comment.
If we fail to even parse the header value in its entirety, I suspect something very wrong is happening and I think we should fail, yeah. If we skip it silently, it could lead to harder-to-debug cases where the user is getting unexpected content back, and we're not catching that on the server (even though we could).
Or do you have cases in mind where this could be too strict?
There was a problem hiding this comment.
I think we need to consider the case where someone is calling the server from a not-generated client and might make mistakes and provide a malformed content-type followed by a valid one in the accept header. Having them get back a response of the valid one seems like a nicer experience than failing the request altogether, but I appreciate your opinion might differ here.
There was a problem hiding this comment.
Yeah I genuinely wonder, if they're typing in a malformed content type, whether they'd want us to fall back to the other one, or to error out (since there's probably a reason they added it in the first place).
There was a problem hiding this comment.
Discussed more offline, we'll leave it this strict, but separately I also filed apple/swift-openapi-generator#644 so that we actually return 400, not 500 (as we'd return today when any error is thrown, including input validation error).
…api-runtime into hd-mime-type-params
Motivation
In service of an upcoming generator PR - to better handle parameters in MIME types.
This is needed for APIs such as Kubernetes, that has both
application/jsonandapplication/json; watch=truein a single operation.Modifications
Use the more modern
OpenAPIMIMETypetype for parsing and comparing content types, rather than the old naive logic that ignored parameter (mis)matches.Result
More accurate handling of content types.
Test Plan
Adapted unit tests.