- edited description
Header values are split by default "\\s*,\\s*"
headers are getting split on commas, which breaks validation when sending a jsonObject as a header value. Is there anyway we can make this customizable? Looks like by default withHeader
always splits the header value. Also looks like it is bad practice to split header values by comma: https://stackoverflow.com/a/35192637/1359765
public Builder withHeader(final String name, final List<String> values) {
// available but not set headers are considered as empty
putValuesToMapOrDefault(headers, name, values, "", true);
return this;
}
static void putValuesToMapOrDefault(final Multimap<String, String> map, final String name,
final List<String> values, final String defaultIfNotSet,
final boolean splitValues) {
if (values == null || values.isEmpty()) {
map.putAll(name, splitValues ? splitHeaderValue(defaultIfNotSet) : Collections.singleton(null));
} else {
values.forEach(value -> map.putAll(name, splitValues ? splitHeaderValue(value) : Collections.singleton(value)));
}
}
Related Issue: https://bitbucket.org/atlassian/swagger-request-validator/issues/97 reviewed by @James Navin
Comments (16)
-
reporter -
reporter @James Navin Do you think this can be remedied?
-
Hi Jonathan - Im happy to review PRs, but I won’t have time to look at this myself for a few weeks.
From a brief look over the links from that stack overflow response it does indeed look like this is going to be non-trivial to solve in a consistent way.
Im open to suggestions here - a couple options that spring to mind are:
- Allowing consumers to register a ‘split strategy’ on a per-header basis, with the default being a simple CSV
- Implementing header-specific split strategies based on standard headers from the RFCs (could build on top of the first approach)
- ?
-
reporter I was thinking the first problem to solve is to not split header values by default.
-
reporter @James Navin if you have a second could you give a mini summary of some of the code I would need to consider changing? I was thinking it would be nice from the spring mvc project to be able to users to designate whether or not they want splitting to happen. I’m just not sure where to add that sort of functionality.
-
Just for my information:
What do you mean by ‘jsonObject’? Is it a string with JSON content?
Is this header described in your OpenAPI specification? Could you give a small example? -
reporter UserContext: type: object properties: id: type: integer role: type: string enum: [Guest, User, Admin] accountId: type: integer
parameters: - in: header name: usercontext schema: $ref: '#/components/schemas/UserContext' required: true
@Sven Döring As you can see the header value is a literal JsonObject. Does that help?
-
I see. I only know header as some kind of list. I haven’t thought of adding schemas. Yeah, why not.
Then I think a custom split strategy is not the right way.
The header shouldn’t be parsed at all in the
SimpleRequest
/SimpleResponse
. It shall be parsed within the validation process depending on the OpenAPI specification.
If the header is a schema definition it shall be validated like the body.
If the header is a defined list then it shall be splitted by the specified splitting character in the interface definition.
If it’s just a number, string, etc. it’s validated like that - no splitting after all. -
reporter Sven, does this mean you’ll be working on it. I would love to do it but I’m just not that familiar with the codebase and because of that don’t have the time to dive into it and update it
-
This decision has not been made, yet.
I would like to here @James Navin opinion about the former comment. Perhaps I haven’t got it right.
-
reporter For the most part, i think you got it right. According to the RFC I don’t think headers are supposed to be split. However, if a header is listed multiple times then the header values can be combined into a comma separated list.
-
Currently I’m looking at the OpenAPI documentation.
It says:
Header parameters always use the
simple
style, that is, comma-separated values. This corresponds to the{param_name}
URI template. An optionalexplode
keyword controls the object serialization.
Splitting the header values always at a comma seems about right.
Whole objects shall not be submitted as JSON strings but as exploded parameters.
-
reporter Yeah, I looked through that swagger openapi 3 specification and I have mixed feelings about it. It’s kind of going against the HTTP standards which i instantly have a knee-jerk reaction against doing. So it just depends on what standard we would want to follow. Plus, the http standard has specific formats for different header fields. For example, this is an example of an Accept header field:
Accept: foo/bar;p="A,B,C", bob/dole;x="apples,oranges"
which would be parsed incorrectly by the swagger validator.See: https://tools.ietf.org/html/rfc2616 https://tools.ietf.org/html/rfc7230 https://tools.ietf.org/html/rfc7231
-
- changed status to open
-
Thanks all for the input. I have revisited this issue while looking at fixing the behavior of array param validation.
After going back over the spec and the RFCs I am planning to do the following:
- Remove the header splitting in
SimpleRequest
altogether - as discussed above this is almost certainly the wrong behavior in almost all cases - Perform the csv splitting during array param validation instead, for
style
s that require it (e.g. for the defaultsimple
style) - For Swagger v2 specs, will support multi-valued headers when the
collectionFormat
ismulti
- For OAI v3 specs, will support multi-valued headers when the
style
isform
(even though this isn’t strictly supported in OAI specs I think it makes sense to allow it for consistency with swagger v2) - Changes will be released as a minor version bump
I have a branch open with most of the changes on it (just cleaning up some failing tests etc.) - https://bitbucket.org/atlassian/swagger-request-validator/branch/290-fix-array-params#diff
Feel free to comment further, otherwise I plan to get the changes out in the next few days.
- Remove the header splitting in
-
- changed status to resolved
Available in v2.11.0
- Log in to comment