Support JDK8 Optional MethodArgumentResolver and HandlerMethodReturnValueHandler [SPR-12997]

[spring-projects-issues]

cemo koc opened SPR-12997 and commented

I think that it would be nice to have OptionalMethodArgumentResolver and OptionalHandlerMethodReturnValueHandler. This would be ideal and complementary to Optional injection which is already addressed before.

---

No further details from SPR-12997

[spring-projects-issues]

cemo koc commented

@Rossen Given that end of life of JDK7 was last month, supporting JDK8 features are more and more essential. Any chance to include in this release?

[spring-projects-issues]

Rossen Stoyanchev commented

Could you explain a little more about what you have in mind? I'm having trouble seeing the idea. With a HandlerMethodArgumentResolver what's the actual value in the Optional? And with HandlerMethodReturnValueHandler I presume you have a service that's returning an Optional which is what you want to return and have it treated as a response body or model attribute?

[spring-projects-issues]

cemo koc commented

I have a UserMethodArgumentResolver. If User is logged in, It injects logged in user to Controller parameter.

@RequestMapping("test")
public void test(User loggedInUser) {
}

This is nice but I like this much more:

@RequestMapping("test")
public void test(Optional<User> loggedInUser) {
}

This implies loggedInUser might be empty optional. This is forcing developers to think about emptiness of LoggedInUser.

What do you think?

[spring-projects-issues]

Rossen Stoyanchev commented

I presume UserMethodArgumentResolver is your own? If so you have to change it to support Optional.

[spring-projects-issues]

cemo koc commented

Yes its my argument resolver but there are other built in resolvers as well. Supporting all of them in the first place is making sense for me. For example, Spring security is using AuthenticationPrincipalArgumentResolver for resolving authenticated user. I can not change their implementation.

Optional method argument resolver makes sense almost in all contexts. Another example would be org.springframework.web.servlet.mvc.method.annotation.ServletRequestMethodArgumentResolver. I think that Injecting Optional<HttpSession> is better than HttpSession. If user does not have a session resolving this as empty optional is much more readable.

Injecting Optional instead of null is also removing ambiguity. A null value might indicate an error as well. However having an empty optional instance removes ambiguity.

Hope that It makes sense.

[spring-projects-issues]

Rossen Stoyanchev commented

Hm, I see where you're going. Not sure I agree though. We don't know what can be combined with Optional. For example Optional<HttpServletRequest> doesn't make sense, same for @PathVariable arguments which are not allowed to be null. It's pretty much what we've done with our existing resolvers, we've added java.util.Optional support selectively where it makes sense.

[spring-projects-issues]

cemo koc commented

I was considering resolving OptionalMethodArgumentResolver first than resolving generic parameter to resolve second part. In this second part you are free to implement your logic. If your resolver does not let you to return null values, as PathVariable, you are free to throw exception. OptionalMethodArgumentResolver does not restrict to return a value from a MethodArgumentResolver. It does only improve semantic and lets you warn other developers to not fall in pitfall by suggesting an optional can also be Empty.

[spring-projects-issues]

Rossen Stoyanchev commented

Right but resolvers support specific argument types while java.util.Optional is more of an orthogonal concern that applies to resolvers that allow null values. I think if we move away from how it is to be implemented, what you're asking is can java.util.Optional be supported in a cross-cutting way such that individual HandlerMethodArgumentResolver's don't have to do it themselves?

I'm not sure if that's feasible or not yet. In the very least though each resolver would have to be involved at least to indicate whether Optional even makes sense and that can vary with each call to resolveArgument since in some cases a single resolver can resolve many different types, some optional, some not (e.g. ServletRequestMethodArgumentResolver).

What did you have in mind on the return value side by the way?

[spring-projects-issues]

cemo koc commented

Yes, exactly. I would like to not implement java.util.Optional in each HandlerMethodArgumentResolver. Applying same logic to all HandlerMethodArgumentResolver is totally fine for me. If a HandlerMethodArgumentResolver can not be empty, Developer is free to throw an exception.

I think that each resolver should not be aware of directly Optional support. They can return null as currently they do. If they don't, throwing an exception or handling in another way is fine. Optional argument should only know value returning from another HandlerMethodArgumentResolver.

On the return value side, my thoughts were not so much concrete. But at first I was considering to return no content in case a empty value. But this might be not best implementation.

[spring-projects-issues]

Rossen Stoyanchev commented

In order to provide transparent support for java.util.Optional argument resolution, most likely we'd have to replace the MethodParameter just before calling the resolver (i.e. in InvocableHandlerMethod) with another that represents the parameterized type inside the Optional. In other words let the resolver do its work unaware of the Optional wrapper. Then wrap the returned value in java.util.Optional. Or am I missing some other implementation ideas?

This sounds alright. However consider something like @RequestParam Optional<String> foo as a replacement for @RequestParam(required=false) String foo. When Optional is used you no longer need to set required=false. Now if we made the resolver unaware of the Optional wrapper by switching the MethodParameter then what the resolver would "see" is @RequestParam String foo which is equivalent to required=true and would result in an exception.

So really the presence of a java.util.Optional wrapper is vital information to a resolver, since it replaces the need for a required flag on a parameter annotation. Given all this I'd like to circle back to the question about motivation.

From an implementation perspective adding java.util.Optional support is trivial. First detect its presence and then wrap. We also provide an ObjectToOptionalConverter so resolvers that do type conversion (like the @RequestParam, @RequestHeader, etc) have even less to do, simply check if the Optional wrapper is present as an indicator of whether the argument is required.

[spring-projects-issues]

cemo koc commented

You are not missing something. I was considering same.
The only difference I have to state that I am not considering @RequestParam Optional<String> foo as a replacement of @RequestParam(required=false) String foo. If developer is aware of consequences of required=true, having an Optional means not so much. Because Optional will be never empty. I can not see any problem apart from semantically meaningless Optional usage.

I think that supporting Optional values by default for all MethodArgumentResolvers would be a good asset for project.

[spring-projects-issues]

Rossen Stoyanchev commented

Well, the use of java.uitil.Optional is equivalent to required=false is how we have supported this since 4.0. We are not going to change that and I don't think see the point of combining required=true with java.util.Optional. What is the reason for have Optional at all in that case?

[spring-projects-issues]

cemo koc commented

Of course Optional support seems a little bid unrelated for such handlers but resolves problem from a single place and supports all other resolvers as well. required=false and Optional support together makes sense for earlier version of JDK8. It is not totally absurd. :)

By the way I have currently changed our implementation as you pointed. It was pretty easy as you stated.
Thanks

[spring-projects-issues]

Rossen Stoyanchev commented

Good to know, thanks.