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

Revert change for SPR-10402 that allowed treating empty values as missing values [SPR-10578] #15207

Closed
spring-issuemaster opened this issue May 21, 2013 · 6 comments

Comments

Projects
None yet
2 participants
@spring-issuemaster
Copy link
Collaborator

commented May 21, 2013

Alfred Staflinger opened SPR-10578 and commented

Today, I have downloaded Spring Framework 3.2.3.

Now, handleMissingValue is called, even if the @RequestParam attribute required = false!

IMHO, this new behaviour is suboptimal. We have many cases, where an empty string parameter value (which has been converted to null by the property editor) should not be handled as missing parameter (even if required = true)! And I guess that many others who relied on the prior behaviour will be confused, too.


Affects: 3.2.3

Issue Links:

  • #15213 Optional parameter in web method now raises error, breaks compatibility with the past ("is duplicated by")
  • #15221 spring 3.2.3 @RequestParam(value="username", required=false) bug ("is duplicated by")
  • #15035 Request @RequestParam not enforced with empty values

Referenced from: commits abfb439, 2d8315f

6 votes, 10 watchers

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented May 21, 2013

Rossen Stoyanchev commented

Apologies for the regression.

Now, handleMissingValue is called, even if the @RequestParam attribute required = false!

That was not intended and can be fixed.

IMHO, this new behaviour is suboptimal. We have many cases, where an empty string parameter value (which has been converted to null by the property editor) should not be handled as missing parameter (even if required = true)

I can see that semantically an empty string satisfies required=true. However, if then converted to null effectively you don't require it. At least from the controller method's perspective, the logic must accommodate the possibility of null. The distinction that there was a "" is gone. So just trying to understand what is the reason for doing this?

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented May 21, 2013

Alfred Staflinger commented

We have got this @ControllerAdvice ...

import org.springframework.beans.propertyeditors.StringTrimmerEditor;
import org.springframework.web.bind.WebDataBinder;
import org.springframework.web.bind.annotation.ControllerAdvice;
import org.springframework.web.bind.annotation.InitBinder;

@ControllerAdvice
public class GlobalControllerAdvice {

	@InitBinder
	public void initBinder(WebDataBinder binder) {
		binder.registerCustomEditor(String.class, new StringTrimmerEditor(
				"\u0000\u0001\u0002\u0003\u0004\u0005\u0006\u0007\u0008\u000b\u000c\u000e\u000f\u0010\u0011\u0012\u0013\u0014\u0015\u0016\u0017\u0018\u0019\u001a\u001b\u001c\u001d\u001e\u001f\u007f",
				true));
	}

}

... because we want to have null values instead of empty strings througout the whole application.

For example, we have got two request mappings within the same @Controller:

@RequestMapping(value = "/foo", params = "!param2")
public String method1(@RequestParam String param1)  {
	...
}
@RequestMapping(value = "/foo", params = "param2")
public String method2(@RequestParam String param1, @RequestParam String param2)  {
	...
	if (StringUtils.hasText(param2)) {
		...
	} else {
		...
	}
	...
}

While method1 should handle this request: /foo?param1=value1 (param2 not part of query string),
... method2 should handle /foo?param1=value1&param2= (param2 is empty string)
and /foo?param1=value1&param2=value2 (param2 is non-empty string)

Now, with Spring Framework 3.2.3, we would have to check thousands of methods (like method2 in the example above) because we get HTTP Status 400 (Bad Request) where it has been no problem up to Spring Framework 2.3.2.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented May 21, 2013

Alfred Staflinger commented

I changed the Priority to Blocker because Spring Framework 3.2.3 is not backward compatible to the previous versions (changed behaviour causes problems: HTTP Response Status 400: Bad Request).

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented May 21, 2013

Rossen Stoyanchev commented

I see. So technically "param2" could be marked required=false but obviously the problem is with the change of behavior. I guess we'll need to roll this back.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 3, 2013

david landis commented

No unit tests failed as a result of such a massively breaking change? I've grown accustomed over many years to not seeing such sloppiness from spring source like this regression.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 20, 2013

Rossen Stoyanchev commented

The change for #15035 has been reversed with commit 2d8315.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.