Skip to content

Support various ways to encode collections in URL parameters for declarative HTTP interfaces #33154

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
doljae opened this issue Jul 7, 2024 · 10 comments
Assignees
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) status: superseded An issue that has been superseded by another type: enhancement A general enhancement

Comments

@doljae
Copy link
Contributor

doljae commented Jul 7, 2024

I've been making good use of the declarative HTTP interface feature that was added in the latest spring release.
I would like HTTP interface to be a bit more versatile and support different encoding methods for @RequestParam.

Currently, the encoding method for @RequestParam of type collection seems to be repeating the same key with &, which seems to be related to the way RequestParamArgumentResolver is implemented.

interface TestRepository {
	@GetExchange("/test")
	Repository test(@RequestParam List<String> param);
}

@Service
@RequiredArgsConstructor
public class TestService {
        private TestRepository repository;

        public void test() {
                List<String> param = List.of("1", "2", "3");
                repository.test(param); // Generated URI path -> GET /test?param=1&param=2&param=3
        }
}

Of course Modern web frameworks expect @RequestParam to be deserialized to collection type automatically, either by repeating the same key value as &, or by parsing strings joined by certain delimiters.
However, if this is not possible due to limitations of older web frameworks or API specs, we have to join the strings with a specific delimiter before the request and then pass the strings as @RequestParam, which is not cool.

It would be nice to have the ability to encode @RequestParam via @CollectionFormat, which is supported by Spring Cloud OpenFeign. I think users who use declarative HTTP interface will have experience with Spring Cloud OpenFeign and will expect a smooth transition.

interface TestRepository {
	@GetExchange("/test1")
	Repository test1(@RequestParam @CollectionFormat(CSV) List<String> param);

	@GetExchange("/test2")
	Repository test2(@RequestParam @CollectionFormat(SSV) List<String> param);
}

@Service
@RequiredArgsConstructor
public class TestService {
        private TestRepository repository;

        public void test() {
                List<String> param = List.of("1", "2", "3");
                repository.test1(param); // Generated URI path -> GET /test1?param=1,2,3
                repository.test2(param); // Generated URI path -> GET /test2?param=1 2 3
        }
}

I'm curious to know what the maintainers think about this 🙂

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Jul 7, 2024
@doljae
Copy link
Contributor Author

doljae commented Jul 7, 2024

BTW, I created a PR that simply adds the @CollectionFormat feature 🙂

@snicoll snicoll added the in: web Issues in web modules (web, webmvc, webflux, websocket) label Jul 9, 2024
@rstoyanchev
Copy link
Contributor

rstoyanchev commented Jul 9, 2024

The example is with HTTP GET parameters in the query, but @RequestParam also applies to HTTP POST with form data in the request body. Is this intended to apply to form data too?

On the server side we apply the ConversionService to method parameters, and it makes it possible to inject comma-separated values into a List. We could do something similar on the client side as well. In fact, RequestParamArgumentResolver already has a ConversionService but it's not actually used because the multiValued argument here is set to true. If I set that to false, then the ConversionService is applied and comma-separated works as expected. We could expose this as a configuration property on RequestParamArgumentResolver, e.g. boolean formatAsSingleValue or something similar, and you could configure the resolver.

@doljae
Copy link
Contributor Author

doljae commented Jul 10, 2024

@rstoyanchev Thanks for the explanation 🙂

Is this intended to apply to form data too?

It is not intended to extend to form data. It's a bit embarrassing, but I just realized that we can bind form data with a combination of @PostMapping and @RequestParam 😅

This issue and the PR I created were initially thought of from a client side perspective only. We often concatenated collection type parameters into strings with a pre-request delimiter to meet certain API specifications, which I thought was uncool, and Spring Cloud OpenFeign is proposing to migrate to the Spring Framework's declarative HTTP interface as feature-complete.

So my personal opinion is that it wouldn't be a bad idea to look at it from a client-side perspective, i.e. to support this feature only on HTTP interfaces using HTTP clients, including RestClient, as a starting point.

@doljae
Copy link
Contributor Author

doljae commented Jul 10, 2024

Also, @RequestParam is a very stable, high-impact annotation that's used in a lot of places.
So I thought it would have fewer side effects to add a new annotation and have the RequestParamArgumentResolver do a little extra logic if it has this annotation.

First of all, I identified that it would be nice to have such a feature as an issue and PR which is not perfect, but it can be implemented without much modification.

If you have any ideas for a different, more generalized approach to implementing this feature (which you've outlined), and if it's possible, I'd be happy to work on it 🙂

@rstoyanchev
Copy link
Contributor

rstoyanchev commented Jul 11, 2024

HTTP POST of form data with @RequestParam is supported on both the client and server side, and generally the intent with @HttExchange methods is to keep the programming model neutral to client or server.

I propose that we start by making RequestParamArgumentResolver customizable so that it formats collection values to a String through the ConversionService. An application can then configure that as a custom argument resolver on the HttpServiceProxyFactory.Builder. If you want to start from there. We also need to make sure that this is not done for form data nor multipart requests. One way to do it is to check the HttpExchange annotation contentType attribute.

@rstoyanchev rstoyanchev self-assigned this Jul 11, 2024
@rstoyanchev rstoyanchev added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Jul 11, 2024
@rstoyanchev rstoyanchev added this to the 6.x Backlog milestone Jul 11, 2024
@doljae
Copy link
Contributor Author

doljae commented Jul 12, 2024

@rstoyanchev

I analyzed the code according to your guide and understood the logic behind the integration of RequestParamArgumentResolver and ConversionService.

I have a few questions before I start working on it.

  1. How to trigger the multiValued field of the NamedValueInfo returned by RequestParamArgumentResolver.createNamedValueInfo() to be false, using the @CollectionFormat annotation I mentioned before?
    Please let me know if you have any other methods in mind 🙂

  2. HttpExchange.contentType() can be read directly from the XXXExchange annotation, but I think it should also take into account the value of the HttpExchange annotation in the method, class, etc.
    Is this the way you intended to work? Seems HttpExchangeReflectiveProcessor is related to this logic and it determine the contentType value from various contentType() from @HttpExchange 🤔

@HttpExchange(contentType = "application/json")
private interface TestApiClient {

	@HttpExchange(contentType = "application/x-www-form-urlencoded;charset=UTF-8")
	@PostExchange(contentType = "application/json")
	void test();
}
  1. How would we enable a converter that supports more delimiters than just commas?
    CollectionToStringConverter seems to be the core class for turning a collection into a string with a comma delimiter.
    Based on your answers to questions 1 and 2, it seems like it could be implemented to support for comma delimiters.
    But how would we extend the Converter class to read @CollectionFormat and convert to that delimiter?

@rstoyanchev
Copy link
Contributor

@doljae I am suggesting that we start with the ConversionService approach as a transparent, automated feature, without introducing an @CollectionFormat annotation. In other words, for a given HttpServiceProxyFactory you choose whether to have collection values formatted or not. If this is sufficient then it's easier than having to repeat the same annotation on every request parameter.

@doljae
Copy link
Contributor Author

doljae commented Jul 13, 2024

@rstoyanchev
you mean that make some modification so that we can use new feature like this way?

@Configuration
class RestClientConfiguration {
    @Bean
    fun testApiClient(conversionService: ConversionService): TestApiClient {
        val restClient =
            RestClient.builder()
                .baseUrl("http://127.0.0.1:8080")
                .build()
        val adapter = RestClientAdapter.create(restClient)
        val factory = HttpServiceProxyFactory.builderFor(adapter)
            .customArgumentResolver(RequestParamArgumentResolver(conversionService, true)) // new boolean field, formatAsSingleValue
            .build()

        return factory.createClient(TestApiClient::class.java)
    }
}

@doljae
Copy link
Contributor Author

doljae commented Jul 15, 2024

@rstoyanchev

I make draft PR. Please check I'm working in the right direction 🙂

@rstoyanchev
Copy link
Contributor

I'll close this as superseded by #33220. Let's continue there.

@rstoyanchev rstoyanchev closed this as not planned Won't fix, can't repro, duplicate, stale Jul 19, 2024
@rstoyanchev rstoyanchev removed this from the 6.x Backlog milestone Jul 19, 2024
@rstoyanchev rstoyanchev added the status: superseded An issue that has been superseded by another label Jul 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) status: superseded An issue that has been superseded by another type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants