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

Provide mechanism to control trimming in StringToArrayConverter and StringToCollectionConverter #23850

Open
SingleShot opened this issue Oct 22, 2019 · 7 comments
Labels
in: core Issues in core modules (aop, beans, core, context, expression) status: pending-design-work Needs design work before any code can be developed type: enhancement A general enhancement

Comments

@SingleShot
Copy link

SingleShot commented Oct 22, 2019

@RequestParams (i.e. HTTP query parameters) with leading or trailing spaces, including single parameters or delimited parameters, are trimmed when mapping them to a collection. It is likely this happens for other cases. A query parameter with a leading/trailing space is a perfectly reasonable construct provided it is encoded correctly. However by the time it gets into a controller, the space(s) have been trimmed.

I can see this happening in at least two Boot classes, and there are probably more:

  • StringToCollectionConverter
  • DelimitedStringToCollectionConverter

Simply removing the trim() calls is not the right choice as these are also used for parsing application.yml, and possibly have other uses. I suggest these converters should accept a trim? parameter at construction time, and the WebConversionService be given its own instances of the converters with trim=false so query params with leading/trailing spaces can be properly passed. Something like that anyway...

@snicoll snicoll transferred this issue from spring-projects/spring-boot Oct 23, 2019
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Oct 23, 2019
@rstoyanchev
Copy link
Contributor

We can't really change the default behavior in web applications. Regardless of one's opinion (and from prior discussions, something like this will have opinions on both sides) existing applications already rely on the current behavior. Adding a trim property to those converters however does seem reasonable to me.

@rstoyanchev
Copy link
Contributor

/cc @jhoeller, @sbrannen, seems straight forward to me, but just in case you have any comments.

@rstoyanchev rstoyanchev added in: core Issues in core modules (aop, beans, core, context, expression) type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Nov 29, 2019
@rstoyanchev rstoyanchev added this to the 5.2.2 milestone Nov 29, 2019
@rstoyanchev
Copy link
Contributor

This applies to StringToArrayConverter and StringToCollectionConverter from the Spring Framework side. DelimitedStringToCollectionConverter is in Spring Boot.

@rstoyanchev rstoyanchev changed the title Leading/Trailing Spaces Trimmed When Mapping RequestParams to Collections Property to control trimming in StringToArrayConverter and StringToCollectionConverter Nov 29, 2019
@sbrannen
Copy link
Member

sbrannen commented Nov 29, 2019

This applies to StringToArrayConverter and StringToCollectionConverter from the Spring Framework side.

I think it's reasonable to add opt-in trimming support (enabled by default, of course) to those converters, especially since they would then align with the existing trimValues support in StringArrayPropertyEditor.

... except that those two classes are not public. So I don't see how a user would register one with a custom trimValues = false option.

@rstoyanchev
Copy link
Contributor

rstoyanchev commented Nov 29, 2019

Ah, so they are. That makes the question even more pertinent. How is one even supposed to control this? In a web application it is possible to register a PropertyEditor via @InitBinder method, possibly in an @ControllerAdvice but this isn't easy to discover, and should be at least documented as such, if there isn't a good route via the global ConversionService. And we don't have a property editor for collections.

@sbrannen
Copy link
Member

sbrannen commented Nov 29, 2019

In the web, if you're only concerned with converting from a String to a String[], you could register a StringArrayPropertyEditor with the trimValues flag set to false; however, as far as I know, we don't have a PropertyEditor for generic collections. In addition, StringArrayPropertyEditor does not support arbitrary object arrays: it only supports string arrays.

If we make StringToArrayConverter and StringToCollectionConverter public, users could then register their custom-configured variants via WebMvcConfigurer.addFormatters(FormatterRegistry).

Otherwise, users would have to implement their own variants of StringToArrayConverter and StringToCollectionConverter (for example, via copy-n-paste but removing the .trim() invocations or making the trimming optional).

To be honest, I've never understood why StringToArrayConverter, StringToCollectionConverter, and other such built-in converters are package private classes.

@rstoyanchev rstoyanchev modified the milestones: 5.2.2, 5.2.3 Nov 29, 2019
@sbrannen sbrannen added status: pending-design-work Needs design work before any code can be developed and removed for: team-attention labels Dec 10, 2019
@sbrannen sbrannen modified the milestones: 5.2.3, 5.3 M1 Dec 10, 2019
@sbrannen sbrannen changed the title Property to control trimming in StringToArrayConverter and StringToCollectionConverter Provide mechanism to control trimming in StringToArrayConverter and StringToCollectionConverter Dec 10, 2019
@sbrannen
Copy link
Member

Team Decisions:

Investigate how to make the trimming in such converters configurable without making such converters public or how to make the functionality required to implement a custom converter by reusing the core functionality of such built-in converters — for example, by making the core functionality available via a public utility method.

Alternatively, we may decide to make certain built-in converters directly public or indirectly public via static factory methods in a yet-to-be-defined utility class.

We may consider introducing a StringCollectionPropertyEditor analogous to the existing StringArrayPropertyEditor and whether it makes sense to have such a StringCollectionPropertyEditor support arbitrary object arrays as opposed to only string arrays like StringArrayPropertyEditor.


If we make the trimming support configurable, we would need to assess how best to apply that to existing default registered converters — for an entire ApplicationContext or specifically for use in Spring MVC and Spring Webflux. For example, one may wish to disable trimming for StringToArrayConverter while retaining trimming for StringToCollectionConverter. Similarly, for some converters, it would appear that trimming may always make sense (e.g., StringToBooleanConverter, StringToNumber, etc.).

If we extract the existing core functionality from StringToArrayConverter and StringToCollectionConverter, we should build the trimValues support into that and delegate to that from StringToArrayConverter and StringToCollectionConverter with trimValues = true by default.

In any case, we must keep in mind that StringToArrayConverter and StringToCollectionConverter rely on the ConversionService in order to convert strings to the appropriate target type.

@jhoeller jhoeller modified the milestones: 6.x Backlog, General Backlog Oct 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: core Issues in core modules (aop, beans, core, context, expression) status: pending-design-work Needs design work before any code can be developed type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

5 participants