Skip to content

Add support for @RequestParam, @RequestHeader, @PathVariable at field level #32597

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
vcruzmj opened this issue Apr 9, 2024 · 5 comments
Closed
Labels
status: duplicate A duplicate of another issue

Comments

@vcruzmj
Copy link

vcruzmj commented Apr 9, 2024

This was discussed already in:

  1. Allow @RequestParam, @PathVariable, and their siblings on fields #23618
  2. Provide mechanism to map request parameters to fields of DTO object #23094
  3. Add support for @BeanParam like JAX-RS in Spring MVC [SPR-17237] #21770

But it was closed by @jhoeller alongside this comment.

We have no plans to introduce field-level request binding annotations. @RequestParam, @PathVariable and co are meant to be used at the parameter level, that's a pretty central part of our annotated handler method arrangement. Binding to objects is a separate mechanism based on bean property setters, property-style fields or constructor arguments, matched against request parameters and path variables. If different binding names are intended there, you could use constructor binding and declare the constructor parameter names with the abbreviated binding names, or with setter methods and declare the setter method names according to your binding names, all while keeping the field names themselves readable.

But IMHO, this answer is just a business decision instead of a technical decision; it could be a feature request that can be handled with a low priority, but discarding it just because "it is not in plans" is not a good approach at all.

First of all, why do we need this? Mainly because some GET endpoints start to get so many arguments that they surpass an acceptable amount. So, creating a DTO is a must, but when you need to specify a different name or property (like required) in every field, this starts to get messy.

Our current solution uses @ConstructorProperties, but this approach leads to human errors and a considerable degradation of maintainability. (I'm going to use Kotlin examples, but applies to Java also)

Example 1:

Expected with the requested feature
data class DTOWithRequestParams(
    @field:RequestParam(name= "some_field_a", required = true)
    val someFieldA: String,

    @field:RequestParam(name= "some_field_b", defaultValue = "B")
    val someFieldB: String?,

    @field:RequestParam(name= "some_field_c", required = true)
    val someFieldC: String?,

    // ... more fields in between

    @field:RequestParam(name= "some_field_x", required = true)
    val someFieldX: String?,

    @field:RequestParam(name= "some_field_y", required = true)
    val someFieldY: String?,

    @field:RequestParam(name= "some_field_z", required = true)
    val someFieldZ: String?,
)
Actual option with `@ConstructorProperties`
data class DTOWithRequestParams
@ConstructorProperties(
    "some_field_a",
    "some_field_b",
    "some_field_c",
    // more field names
    "some_field_x",
    "some_field_y",
    "some_field_z",
)
constructor(
    val someFieldA: String,

    val someFieldB: String?,

    val someFieldC: String?,

    // ... more fields in between

    val someFieldX: String?,

    val someFieldY: String?,

    val someFieldZ: String?,
)

Clearly, with the current approach, you need to always consider the order of the fields. If you don't, the code will compile and be run anyway, but for example, you can have the value of someFieldA inside someFieldB without notice until it fails in other code ahead.

With a new approach at the field level, you can have the annotation with the name and the other properties attached to the field, and it is hard to write something wrong and easier to maintain.

Please consider this feature, even with low priority, and leave the community with an opinion on this matter.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Apr 9, 2024
@snicoll
Copy link
Member

snicoll commented Apr 9, 2024

But IMHO, this answer is just a business decision instead of a technical decision;

I disagree. For a start, it clearly states we have no intention of doing this. As a team, we can't justify having a feature open with low priority hanging around to not have to deal with report like yours. How would you benefit from having an issue rotting in the issue tracker?

We did consider the feature and decided we don't want to act on it. We do our best to listen to the community and provide justifications, and keeping this issue open when we know we don't want to do it isn't fair to the community.

@snicoll snicoll closed this as not planned Won't fix, can't repro, duplicate, stale Apr 9, 2024
@snicoll snicoll added status: duplicate A duplicate of another issue and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Apr 9, 2024
@vcruzmj
Copy link
Author

vcruzmj commented Apr 9, 2024

So everything summarizes as: "We don't want to do it," right? Just for the record.

The justification for having this feature speaks for itself, but you (as a team, according to your comment) decided not to act on it.

The problem now is "why?" Is it too expensive to make? Is it breaking any standard? Hits performance severely?

As developers, we (looking at the other related rejected and closed issues) need a justification for why your team is dropping this.

@snicoll
Copy link
Member

snicoll commented Apr 9, 2024

So everything summarizes as: "We don't want to do it," right? Just for the record.

Not at all. Juergen's initial comment reads as a complete and well-formed justification to me. You seem to dismiss the entire thing as a "business justification" which it isn't only IMO. I understand that you feel differently but we're the ones having to deal with the extra complexity and edge cases this will generate.

And again, having an issue rotting in the issue tracker is not better. You're asking for this to be open with low priority but that isn't going to achieve anything.

@bclozel
Copy link
Member

bclozel commented Apr 9, 2024

Juergen's comment points already to the fact that we are responsible for designing the web programming model and that we think that introducing this variant competes with another approach that we think is a better alternative. Introducing variants increases the complexity for developers and maintainers.

We sympathize with your use case but cannot justify implementing this in the current state of things. We did reconsider issues in the past and reopened them. As Stephane points out, keeping issues opened gives a false impression to the community and makes it harder to prioritize other work.

@vcruzmj
Copy link
Author

vcruzmj commented Apr 9, 2024

Even if I disagree, thanks for your replies; I hope this can be reconsidered in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: duplicate A duplicate of another issue
Projects
None yet
Development

No branches or pull requests

4 participants