UriBuilder - Overload queryParam to accept Optional and add only if value is present

[robinroos]

This addresses a use-case in which a REST endpoint (A) has received an optional query param (as type Optional) and is delegating through WebClient to another service (B), probably with a similar uri structure.

Rather than have service (A) interrogate the parameter and branch its execution flow, it can merely include it in the chain of .queryParam() invocations on UriBuilder. The parameter will be added in the downstream call to service (B) only if a value is present.

Without this change:

        Function<UriBuilder, URI> f = (b) -> {
            b = b.path("/eventdates");
            if(context.isPresent()) {
                b = b.queryParam("context", context);
            }
            b = b.queryParam("limit", 3);
            return b.build();
        };
        WebClient.ResponseSpec body = service.get().uri(f).accept(MediaType.APPLICATION_JSON).retrieve();

With this change (queryParam for "context" is passed an Optional):

        WebClient.ResponseSpec body = service.get().uri(b -> b.path("/eventdates").queryParam("context", context).queryParam("limit", 3).build()).accept(MediaType.APPLICATION_JSON).retrieve();

[pivotal-issuemaster]

@robinroos Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

[pivotal-issuemaster]

@robinroos Thank you for signing the Contributor License Agreement!

[robinroos]

It would be readlly good to see this included in the up-coming Spring Boot 5.4.0 release. I am currently testing with Spring Boot 5.4.0-M3.

[rstoyanchev]

As per the Javadoc of uriBuilder#queryParam it is possible to pass no values at all which results in a query parameter without a value. To be consistent a queryParam method that takes an Optional would have to behave in the same way to be consistent.

We also need to consider what should happen if a value passed into the existing queryParam is Optional? Here it clearly needs to be interpreted as if null was passed. So an obvious place to start is to change the existing queryParam methods to do something meaningful with Optional values. Once that's in place I think there is no longer a need for an overloaded method that explicitly takes Optional.

If we wanted to have another method that does not add a query param at all if there is no value, that would have to be named differently in order to differentiate the semantics. But I am wondering if the existing method behavior, if extended to support Optional, would get you far enough?

[robinroos]

Thanks for your comment.

So an obvious place to start is to change the existing queryParam methods to do something meaningful with Optional values.

Whereas my PR adds an overloading, my purposes would be equally well-served by effecting the change under the existing API.

What is important for me is that the caller (my code) should not have to interrogate the Optional for isPresent(), and that the query param should NOT be added in the "absent" case.

Would you like me to craft an alternative PR which does not extend the API at all but which works within the existing methods?

[robinroos]

In addition to the above, my personal feeling is that it would acceptable for the documentation to reflect that the parameter name will NOT be added to the url if the argument passed is Optional.empty().

Remember it was not present on the inbound request, which is why the query parameter was mapped to Optional.empty(). The delegation to a downstream service would be different in perhaps subtle ways if the parameter name was included albeit with no value.

[rstoyanchev]

The existing queryParam methods have established behavior. Documenting that it works one way for null or no value and a another way for no value via Optional won't make it consistent there are more than one ways in which a user of such an API can be surprised, depending on whether you're exposed to using it via null or Optional first. Even after you learn, you have to then remember which is which or always go back to look it up.

Remember it was not present on the inbound request, which is why the query parameter was mapped to Optional.empty().

I do understand that your scenario does not fit this existing behavior. For that we can add a new method that is named differently and free to behave in a different way, e.g. queryParamIfPresent(String, Optional<?>).

[robinroos]

I have raised an alternative (actually better) implementation as #25950.

[robinroos]

@rstoyanchev , I'm just now catching up with the most recent comment.

I would be interested to hear your thoughts after you have read the PR description for #25950 - specifically about how Optional is currently treated.

[robinroos]

Once you have read the other PR, if you definitely want to proceed with a new method name then I will happily implement that as a third PR. May I suggest the method name be "optionalQueryParam" ?

[rstoyanchev]

@robinroos I'm not sure what else there is to say. The additional PR only adds what I've already said we won't do.

[robinroos]

No problem. I'll implement an explicit optionalQueryParam method.

[robinroos]

I have raised #25951

Is it ok if I now close this one and also close #25950 ?