Allows different collection encodings - #543
Conversation
|
thanks for raising. per the HACKING file, we don't tend to add features ahead of multiple people requesting them. If we did, an enum might be a bit brittle or "guessy" as in we are guessing which formats people might want, and locking them into the old approach if not. Currently, we have Param.Extractor which allows people to implement their favorite toString'er for a parameter (memory fails me, but it might be possible to use this in lieu of this change). At any rate, something like that is more likely to reduce the amount of similar change requests. If you want to proceed with something like this, review the issues and see if you can find similar ones. We're looking for at least a few users as code like this usually lingers past the interest of the author (no offense) |
|
Hi Adrian,
Thank you for reviewing my issue. I understand about your concern about
feature-creep.
I was able to find one other user who ran into the same issue as me (
spring-cloud/spring-cloud-netflix#1634). He was
able to use a RequestInterceptor as a workaround. I could do that, too, but
for my use case (auto-generating client stubs from Swagger specs) the
annotation route is cleaner.
In any case, let's see if anyone else shows interest in that topic.
Thanks,
Ben
…On Mon, Mar 27, 2017 at 5:49 PM, Adrian Cole ***@***.***> wrote:
thanks for raising. per the HACKING file, we don't tend to add features
ahead of multiple people requesting them. If we did, an enum might be a bit
brittle or "guessy" as in we are guessing which formats people might want,
and locking them into the old approach if not. Currently, we have
Param.Extractor which allows people to implement their favorite toString'er
for a parameter (memory fails me, but it might be possible to use this in
lieu of this change). At any rate, something like that is more likely to
reduce the amount of similar change requests.
If you want to proceed with something like this, review the issues and see
if you can find similar ones. We're looking for at least a few users as
code like this usually lingers past the interest of the author (no offense)
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#543 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AB1PIkuZboDJlqKw_NfOe1rJVdZRFs8kks5rqFkTgaJpZM4Mqxu6>
.
|
|
How many votes does it take? And what do you consider votes? I'm assuming the thumbs-up icon is a vote for yes? |
rage-shadowman
left a comment
There was a problem hiding this comment.
I'm not a fan of switch statements. They are a nasty code smell IMO.
| /** Values separated with the pipe (|) character, eg foo=bar|baz */ | ||
| PIPES, | ||
| /** Parameter name repeated for each value, eg foo=bar&foo=baz */ | ||
| MULTI, |
There was a problem hiding this comment.
I would either add a package-local method in here for abstract String getSeparator(String field); to be called rather than the switch statement in RequestTemplate, or I'd drop the enum in favor of a ParamJoiner interface (which again has the String getSeparator(String field) method).
There was a problem hiding this comment.
I can use cases for CSV, SSV, PIPES and MULTI. I am concerned with TAB. Is the expectation that when that is used a TAB character is present on the query string? Is that considered safe?
There was a problem hiding this comment.
Regarding TABS, the raw tab character would not end up on the wire, it would be percent-encoded. The feign layer would read and write tabs since the underlying HTTP client would take care of percent encoding/decoding.
The same would be true for spaces as well, which shouldn't really be used in URL's either. You can see an example of the wire format with spaces percent-encoded in the current OpenAPI (née Swagger) spec.
Interestingly, it looks like current version (3.0) of the OpenAPI spec has removed tabs and only accepts commas, spaces, pipes and multi.
There was a problem hiding this comment.
Awesome. Thanks for the info! Good to know.
There was a problem hiding this comment.
@kdavisk6 My problem with switch statements (and large if/else if blocks) is that they are hard to maintain, so I try to avoid them if possible.
There often ends up multiple of these blocks of code, and they all must be kept in sync with every new bug-fix/feature-addition/etc, and the farther they are away from the actual determiner-of-logic, the harder it is to keep them up-to-date.
Really, the thing that knows how a MULTI or CSV or whatever format should behave, is the enum value itself, not the request template. So I'd just make each of the enum values implement the same method signature and then just call something like: collectionFormat.toJoinedString(fieldName, collection);.
If there absolutely must be a switch statement (sometimes they are unavoidable), then I'd stick it as close to the determiner-of-logic as possible, such as in a static method on the enum class, or in a utility class that is dedicated to CollectionFormat values. That way, when you fix a bug or add a new value to the enum, there are less places to overlook.
There was a problem hiding this comment.
@rage-shadowman @kdavisk6 I went ahead and refactored this a bit according to your suggestions, and I think it's a bit cleaner now. Instead of the switch statement, I added a join method to the enum that contains all the logic for joining together collections of values with the given field name. Now the joining logic in RequestTemplate is minimal.
I also renamed MULTI to EXPLODED, which is the term used in the newest edition of OpenAPI. Users will generally not see this value anyway, since that's the default.
|
I think after conflict being solved and the switch being removed, we can merge this. |
In the case where a parameter represents a collection of values, there are conflicting ways of encoding that collection. Common ways are repeating the parameter name (foo=bar&foo=baz) and using comma separated values (foo=bar,baz). The current behavior repeats the parameter name. This change introduces an additional RequestLine parameter that explicitly specifies the encoding type, one of CSV, TSV, space-delimited, pipe-delimited, and repeating the parameter name. The default value for this option is repeating the parameter name, so backwards compatibility is maintained.
78cdb06 to
93598e3
Compare
|
@velo I rebased to master and fixed the merge conflict. I also removed the switch statement in favor of moving the joining logic into the enum class. |
* Allows different collection encodings In the case where a parameter represents a collection of values, there are conflicting ways of encoding that collection. Common ways are repeating the parameter name (foo=bar&foo=baz) and using comma separated values (foo=bar,baz). The current behavior repeats the parameter name. This change introduces an additional RequestLine parameter that explicitly specifies the encoding type, one of CSV, TSV, space-delimited, pipe-delimited, and repeating the parameter name. The default value for this option is repeating the parameter name, so backwards compatibility is maintained. * Replace switch statement with enum method for joining values
* Allows different collection encodings In the case where a parameter represents a collection of values, there are conflicting ways of encoding that collection. Common ways are repeating the parameter name (foo=bar&foo=baz) and using comma separated values (foo=bar,baz). The current behavior repeats the parameter name. This change introduces an additional RequestLine parameter that explicitly specifies the encoding type, one of CSV, TSV, space-delimited, pipe-delimited, and repeating the parameter name. The default value for this option is repeating the parameter name, so backwards compatibility is maintained. * Replace switch statement with enum method for joining values
In the case where a parameter represents a collection of values, there are
conflicting ways of encoding that collection. Common ways are repeating the
parameter name (foo=bar&foo=baz) and using comma separated values (foo=bar,baz).
The current behavior repeats the parameter name. This change introduces an
additional RequestLine parameter that explicitly specifies the encoding type,
one of CSV, TSV, space-delimited, pipe-delimited, and repeating the parameter
name. The default value for this option is repeating the parameter name, so
backwards compatibility is maintained.
Possible implementation for #542.