feign
feign copied to clipboard
OpenFeign
Support Conditional Parameter Expansion
Hi!
We define our Spring MVC APIs with @RequestParam Optional<Type> something.
It looks like Feign doesn't handle it well and uses "toString()" version so that Spring fails to convert String -> Optional
hey @spencergibb, I'm afraid this is not the same, #294 was about the decoder, and it's not hard to use a custom decoder to make it work, but, in case of the request parameters, I don't know any good way to make it work :(
Yeah, I specifically didn't say it was a duplicate, just regarding Optional.
oh.. you are asking about spring mvc annotations. sorry I was answering about feign.Param.Expander
if one were hacking things.. they could change the contract so that it populates MethodMetadata.indexToExpanderClass (this is what the default contract does, handling Param.Expander.
Same here, optional parameter is very useful if you want to reset optional parameter, otherwise you just can't do that.
It's already supported by Spring MVC:
@GetMapping(path = "/test")
public void test(
@RequestParam(value = "name", required = false) Optional<String> name
) {
System.out.println("is present " + name.isPresent());
}
Request set name http://localhost:8132/test?name=hello will have true.
Reset name request http://localhost:8132/test?name will have true.
No name reset request http://localhost:8132/test? will have false.
I think I was missing something, it seems that contract in Spring MVC is next:
Request set name http://localhost:8132/test?name=hello will have hello.
Reset name request http://localhost:8132/test?name will have `` - empty String.
No name reset request http://localhost:8132/test? will have null - null String.
Seems like optional adds nothing in my case.
For my case I think I need custom class that will be able to show when parameter actually is in request. Like ResponseEntity for returned body.
Example:
@GetMapping(path = "/test")
public void test(
@RequestParam(value = "name", required = false) RequestParamEntity<String> name
) {
System.out.println("is present " + name.isPresent());
System.out.println("has value " + name.hasValue());
}
For request http://localhost:8132/test?name=hello output will be:
is present true
has value true
For request http://localhost:8132/test?name= (questionable) output will be:
is present true
has value true
For request http://localhost:8132/test?name output will be:
is present true
has value false
For request http://localhost:8132/test? output will be:
is present false
has value false
But this also should be implemented in Spring MVC.
We can support Optional<T> as a return type. but we do not support is as a @Param. I'll add this to the backlog for us to consider, however; feel free to submit a PR for this if you cannot wait.
I would like to help add Optional<T> support in @Param, just need some direction on where to start looking
I think that we will first need to agree on a specification. These should focus on the result of Optional.isPresent(). We would also need to limit the scope to Feign's default contract and annotation, @Param, which does not support a required attribute. Given these restrictions, I see the following use cases for us to define:
-
isPresentistrue -
isPresentisfalse - Parameter is not provided or
null
Thoughts?
@kdavisk6 I think Optional will adds nothing from what actually needed, check my previous post for suggested custom class RequestParamEntity(name is questionable) and suggested behaviour of it.
That behaviour you can't implement using Optional.
Apologies. I will update the title of this issue to avoid any further confusion.
Reading through it again, the behavior you are looking for is to have more control over what the resulting expanded request parameter name/value pair is. A recent update, not yet released as part of #778, standardizes how Feign handles Query String, Headers and Body template parameter expansion. The logic now adheres to RFC 6570 - URI Templates Level 1 Expansion. Resolved Query String variables result in a name=value expansion and unresolved variables result in the name/value pair being removed from the resulting uri.
The part of your request that is challenging is the conditional logic applied if the parameter cannot be resolved. This feels a lot like what was discussed in #726. One possible approach would be to extend the Expander interface that can be used on a @Param, allowing for expansion of the entire pair and not just the value, but that would be a breaking change.
@yami12376
We are considering adding support for Level 2-4 uri template expression which will address most of what is discussed in this request. Specifically, we will adhere to the RFC rules regarding what to do when the value is undefined, which in our case is an explicit null. Empty Strings are not considered undefined, just empty, which are still included.
For example, using form style expressions http://localhost:8132/test{?name}, the result would be as follows:
name is present
http://localhost:8132/test?name=value
name is present and empty
http://localhost:8132/test?name=
name is not present
http://localhost:8132/test
The only situation this will not handle is the case where a user may want to override this behavior for a single expression. Using the example above where the desired result is the variable on the query string without the = sign, we would need provide a facility to override the expansion logic. We may also consider extending the capability of the Expander concept for this; however for Feign 11, we are going to focus primarily on the template levels.
We can support
Optional<T>as a return type. but we do not support is as a@Param. I'll add this to the backlog for us to consider, however; feel free to submit a PR for this if you cannot wait.
Has support to Optional<T> @Param been implemented in Feign?
this has just bitten me in 2023. i was expecting optional empty to supply no parameter, but nope gives ?param=
