spring-projects / spring-framework

Spring Framework
https://spring.io/projects/spring-framework
Apache License 2.0
56.8k stars 38.17k forks source link

Provide mechanism to control trimming in StringToArrayConverter and StringToCollectionConverter #23850

Open SingleShot opened 5 years ago

SingleShot commented 5 years ago

@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:

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...

rstoyanchev commented 5 years ago

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 commented 5 years ago

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

rstoyanchev commented 5 years ago

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

sbrannen commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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.

sbrannen commented 4 years ago

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.