Pageable and Sort arguments with empty qualifier value shouldn't be prefixed with delimiter [DATACMNS-1827]

[spring-projects-issues]

Vedran Pavic opened DATACMNS-1827 and commented

If Pageable or Sort controller method arguments are annotated with an empty string (the default value for Qualifier#value) the request parameters end up being prefixed with only qualifier delimiter, e.g. \_page and \_size (this assumes a default delimiter of \_).

Our use case is that we need to have multiple Pageable arguments resolved from the request, but have one that shouldn't be prefixed. Since having multiple arguments requires qualifying them, the logical option is to qualify one that shouldn't be prefixed with an empty qualifier value. However, this yields a bit awkward result, as explained above.

I classified this as a bug, because it feels like one, but I'd understand if it was reclassified as an enhancement. Either way, if you agree with the assessment that qualifier delimiter should be omitted in the case of empty qualifier value, I'd like to provide a PR to address this

---

Referenced from: pull request #473

Backported to: 2.4.1 (2020.0.1)

[spring-projects-issues]

Vedran Pavic commented

I've opened a PR to address this

[spring-projects-issues]

Mark Paluch commented

Concatenating with an empty value and adding the delimiter is indeed inconvenient. What's the reason for adding a @Qualifier to the argument without a value?

[spring-projects-issues]

Vedran Pavic commented

Hi Mark Paluch, thanks for looking into this.

We have a situation where some resources our service exposes need to provide additional data, a that data needs to be paginated as well. We cannot change the original (could be considered parent in this case) pagination parameters as that would be a breaking change.

Either way, it strikes me as reasonable to have a situation where multiple pagination controls need to be resolved from the request, with one of those being considered primary, or parent, to the other ones

[spring-projects-issues]

Mark Paluch commented

It makes sense to omit the delimiter. I was curious about the use of a qualifier annotation without a value since that's unusual. Looking at the code that does the qualifier handling, we don't consider value() if @Qualifier is used as meta-annotation since we're not using Spring's MergedAnnotations

[spring-projects-issues]

Vedran Pavic commented

I agree that a qualifier with an empty value is a bit unusual, but at the same time that's the default value so it should be handled somehow. Also, I'm not sure what would be a better way of supporting default (i.e. unqualified) pageable since the use of qualifiers is mandatory if there are more than one.

I'm open to extending the PR if necessary, would you like it to take MergedAnnotations into consideration as well?

[spring-projects-issues]

Mark Paluch commented

Thanks for the background. In fact, using @Qualifier without a value is only recommended when used as meta annotation. I'm going to merge and amend the PR to use MergedAnnotations to ensure we align with the intended annotation use

[spring-projects-issues]

Mark Paluch commented

That's fixed now along with considering meta-annotations. Care to retest against 2.4.1 snapshots as we plan to ship a release today?

[spring-projects-issues]

Vedran Pavic commented

I've managed to give 2.4.1-SNAPSHOT a spin, and it looks good. Thanks!