Enhance interaction between UriBuilder and MultivaluedMap

[commodis]

UriInfo provides essential information about the URI like the query parameters via the Methode UriInfo.getQueryParameters(). Its return type is MultivaluedMap<String, String> which is basically a Map<String, List<String>>. Thus a single entry inside this Map is Entry<String, List<String>>.

This type is incompatible with the class UriBuilder. Its method signature is

public abstract UriBuilder queryParam(String name, Object... values);

Following code does not work

final var parameters = uriInfo.getQueryParameters();
final var searchParameterValue = parameters.get("search");

final var baseUri = URI.create("https://github.com");
UriBuilder.fromUri(baseUri)
     .queryParam("search", searchParameterValue)
     .build();

This limitation could be resolved by adding a method like

public abstract UriBuilder queryParam(String name, List<String> values);

[chkal]

I guess your example could be fixed by adding .toArray(), or am I missing something?

UriBuilder.fromUri(baseUri)
     .queryParam("search", searchParameterValue.toArray())
     .build();

[commodis]

You are correct.

The question is, why would I need to do that in the first place within a single API? I guess it is the same reasoning for classes like WebTarget which could accept more object types than a common String

[chkal]

I basically agree that providing another method for setting query parameters using a list could be beneficial. I was just trying to understand what users currently have to do as a workaround.

[commodis]

My usecase was to proxy the query parameters from my current location to a different target where I had to modify a single parameter like for example

https://current.github.com?a=1&b=2

to

https://**after**.github.com?a=1&b=**3**

Was I missing something obvious rather than build up the new query myself?

[chkal]

Well, wouldn't it be easy to get the MultivaluedMap for the current request, replace the single query parameter and then loop over the multimap and call UriBuilder.queryParam(String, Object...) for each entry?

[commodis]

The MultivaluedMap is immutable which disallows operations like put but yes, basically I am doing this. I am not telling that this is impossible and no workaround exists. It is just weird that we are not having a consistent API within the same specification. Obviously, it is due to backwards compatibility and based on other specifications.

[chkal]

I'm still not sure if I fully understand what you mean with "consistent API". If you want to get the query parameters of the current request, modify it and pass it to a UriBuilder, you can do something like this:

UriInfo uriInfo = ...;
UriBuilder uriBuilder = ...;

var params = new HashMap<>( uriInfo.getQueryParameters() );
params.put( "b", Collections.singletonList( "3" ) );
params.forEach( (name, value) -> uriBuilder.queryParam( name, value.toArray() ) );

I agree that you could get rid of .toArray() if UriBuilder would allow to provide the query parameters as a list, but you would still end up with three lines of code for your example. And your use case seems to be quite special, so IMO this is acceptable.

[commodis]

Leaving my special usecase aside it is just about the required conversion needed from List<String to Object[] when using UriInfo as a source to produce an URI via UriBuilder.

requiring the step params.forEach( (name, value) -> uriBuilder.queryParam( name, value.toArray() ) ); feels unnecessary.

[chkal]

So you think that something like UriBuilder.queryParams(MultivaluedMap<String,​String>) would help? In this case we would also have to add UriBuilder.replaceQueryParams(MultivaluedMap<String,​String>) for consistency with other methods.

To be honest, I'm not super convinced that having these additional methods bring much benefit. Especially, because there are workarounds which are also just one line of code as shown above.

However, maybe other @eclipse-ee4j/ee4j-jaxrs-committers have a different opinion?

[commodis]

In my opinion, an API should have a consistent layout so people using that API are not re-inventing the wheel each and every time. JDK 16 added Stream.toList() even though .collect(Collectors.toList()) already exists. If both would either use Object[] or List<String> this would all be fine

[andymc12]

I don't have a strong opinion either way. If it makes life easier or more consistent for users, then it's probably a good thing. I think that if we added these methods, most implementors will basically just delegate to the existing method performing the same toArray() approach previously discussed.

[chkal]

@eclipse-ee4j/ee4j-jaxrs-committers Other thoughts?