Skip to content
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

Allow formatting of Collection values for @RequestParam with HTTP interface client #33220

Closed
wants to merge 4 commits into from

Conversation

doljae
Copy link
Contributor

@doljae doljae commented Jul 15, 2024

Motivation:

Make it possible to format Collection, @RequestParam arguments to a single String value. On the server side, the ConversionService parses a String value to the target method parameter type. On the client side, the reverse could be done to format the MethodParameter to a String.

See original request and discussion under #33154.

Modifications:

Add customization for handling collection types to RequestParamArgumentResolver

Additional info:

  • I plan to polish the code after checking with the maintainer to make sure I'm working in the direction I intended.
  • If we have a way to format it according to the checkstyle rules, please let me know 🙏

@doljae doljae marked this pull request as draft July 15, 2024 13:04
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Jul 15, 2024
@rstoyanchev rstoyanchev self-assigned this Jul 19, 2024
@rstoyanchev rstoyanchev changed the title feat: add customization for handling collection types to RequestParamArgumentResolver Allow formatting of Collection values for @RequestParam with HTTP interface client Jul 19, 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 19, 2024
@rstoyanchev rstoyanchev added this to the 6.2.0-M7 milestone Jul 19, 2024
Copy link
Contributor

@rstoyanchev rstoyanchev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the changes so far, I've decided to make the following update dcabddd to make it easier access to HttpExchange metadata in argument resolvers. Now RequestParamArgumentResolver can override the overloaded createNamedValueInfo method and get easy access to the HTTP method and content type.

See further comments below.

@rstoyanchev rstoyanchev added the in: web Issues in web modules (web, webmvc, webflux, websocket) label Jul 19, 2024
@doljae doljae requested a review from rstoyanchev July 20, 2024 07:00
@doljae
Copy link
Contributor Author

doljae commented Jul 20, 2024

On 95a76ee commit I tried to applying coding style which is used in the project. Please let me know if I need to make any additional changes.

One more thing, I was wondering what your thoughts are on supporting a variety of delimiters besides the comma that @Collectionformat supports.

@doljae doljae marked this pull request as ready for review July 20, 2024 22:50
Copy link
Contributor

@rstoyanchev rstoyanchev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates, I'll take it from here. For a different delimiter, you can customize the ConversionService.

@yvasyliev
Copy link

yvasyliev commented Sep 15, 2024

Hello @rstoyanchev, I need some clarification here.

I've got a case where I need to support different collection encoding methods within the same HTTP Interface:

@HttpExchange("/api/v1")
public interface MyClient {
    @GetExchange("/list")
    String sendList(List<String> params); // must be encoded as `params=value1&params=value2`

    @GetExchange("/csv")
    String sendCsv(List<String> params); // must be encoded as `params=value1,value2`
}

Could you please suggest how can I achieve it after f967f6f?

With Feign client I could do the following:

@FeignClient("myClient")
public interface MyClient {
    @GetMapping("/api/v1/list")
    String sendList(List<String> params); // will be encoded as `params=value1&params=value2`

    @CollectionFormat(feign.CollectionFormat.CSV)
    @GetMapping("/api/v1/csv")
    String sendCsv(List<String> params); // will be encoded as `params=value1,value2`
}

I would expect to see a @CollectionFormat alternative for HTTP Interface to be applied to each request parameter individually. It will provide users with high customization capabilities:

@HttpExchange("/api/v1")
public interface MyAnotherClient {
    @GetExchange("/list")
    String sendDifferentParams(
        List<String> params,
        @CollectionFormat(CSV) List<String> csvParams
    ); // will be encoded as `params=value1&params=value2&csvParams=value1,value2`
}

@doljae
Copy link
Contributor Author

doljae commented Sep 16, 2024

Hello @yvasyliev .
I'm not the one who you mentioned but I received an email from your comment.
You can use it with milestone version and please check my issue comment.

As I know, you're right. Currently we can decide only one way of handling collection values with @RequestParam annotation in single declarative HTTP interface.

I'm not the maintainer but I think Spring core already have an ArgumentResolver and it's derived class, we can decide the way to handle collection values with registering customized ArgumentResolver class. @CollectionFormat's implementation is different from the one in spring core and I'm guessing that their intent is to be cautious about adding new annotations. Again, this is just my opinion 🙂

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) type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants