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

Request @RequestParam not enforced with empty values [SPR-10402] #15035

Closed
spring-issuemaster opened this issue Mar 20, 2013 · 16 comments
Assignees
Milestone

Comments

@spring-issuemaster
Copy link
Collaborator

@spring-issuemaster spring-issuemaster commented Mar 20, 2013

Michael Osipov opened SPR-10402 and commented

I have defined this method signature:

@RequestMapping(method = RequestMethod.GET, produces = { MediaType.APPLICATION_XML_VALUE,
  "application/json;charset=UTF-8" })
public ResponseEntity<Object> searchProjects(@RequestParam(value = "q") String query,
  @RequestParam("attributes") String[] attributes,
  @RequestParam(value = "outputType", required = false, defaultValue = "hash") OutputType outputType)
  throws MissingServletRequestParameterException

When I invoke this method either with:

GET /rest?q&attributes=one

or

GET /rest?q=&attributes=one

The query variable is always passed as empty string/null although its defined as required. MissingServletRequestParameterException is not thrown.

I have attached a screenshot of the debugging session where one can inspect that case. I think the else if in lines 95 to 97 are incomplete.


Affects: 3.2.2

Attachments:

Issue Links:

  • #15213 Optional parameter in web method now raises error, breaks compatibility with the past
  • #15207 Revert change for SPR-10402 that allowed treating empty values as missing values

Referenced from: commits 2d8315f, e39fe18, abfb439, d3eda09

0 votes, 7 watchers

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Mar 20, 2013

Phil Webb commented

I believe that this is by design. The MissingServletRequestParameterException is thrown when the parameter is missing, in your example the parameter is declared, just without any value.

The @RequestMapping javadoc:

Whether the parameter is required.
Default is true, leading to an exception thrown in case of the parameter missing in the request.
Switch this to false if you prefer a null in case of the parameter missing.

Rossen Stoyanchev or Arjen Poutsma can probably confirm this.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Mar 21, 2013

Michael Osipov commented

Phil, how can this be by design? My request param is required thus no value has been provided by ?q or ?q=. As the JavaDoc says, this should lead to an exception.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Mar 21, 2013

Phil Webb commented

Hi Micheal,

Exceptions are raised only when the parameter is missing. In your case the parameter is present and the value is treated as an empty string:

GET /rest?q=&attributes=one

This will not throw a MissingServletRequestParameterException because the q parameter is present (albeit an empty String).

GET /rest?attributes=one

This will throw a MissingServletRequestParameterException because the q parameter is missing entirely.

This is probably not the most intuitive behavior and the javadoc could be improved but unfortunately I don't see an easy way to change the current design without breaking existing applications that may depend on this behavior.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Mar 21, 2013

Michael Osipov commented

Unfortunately, I do not consider an empty value a valid value. I have reported a similar issue already #14813 which was fixed in 3.2.1. I think this one fall under the same edge case. Never tested.

Do you really think that people really care whether the value is not provided or an empty string? I don't think so.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Mar 25, 2013

Rossen Stoyanchev commented

Obviously in your case you don't care about empty vs null values. That's not necessarily the case for everyone. The key here is that the "" is passed through type conversion, which by default turns it to null. We don't want to take away the option to treat "" as something else. However, if the value is still null after type conversion and the argument is marked as required, we can treat it as a missing parameter at that point.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Mar 25, 2013

Michael Osipov commented

Rossen, that sounds fair enough. Looking forward to 3.2.3. Thanks.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented May 10, 2013

Rossen Stoyanchev commented

This is now resolved so that if the request parameter is "" and following type conversion it is null, it is treated as a missing value.

Note however, that the ConversionService leaves empty strings alone by default. So you must choose to treat them the same null by registering a converter or property editor that does that. See for example StringTrimmerEditor and also #12201 is relevant.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented May 20, 2013

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

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster 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

@spring-issuemaster spring-issuemaster commented May 21, 2013

Rossen Stoyanchev commented

I have opened #15207 and am closing this one since 3.2.3 is already out.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented May 21, 2013

Rossen Stoyanchev commented

As much as I dislike doing so, I'm afraid I'll need to roll this change back as it introduced a regression that is not acceptable. Michael Osipov, that means you'll probably need to find another way to treat empty values as invalid input. You could do so by registering a PropertyEditor or Converter that rejects empty values by throwing an exception.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented May 21, 2013

Michael Osipov commented

Rossen, fine. Can we postpone this to 4.x with a refined logic to fill gaps like these?

I will evaluate the org.springframework.beans.propertyeditors.StringTrimmerEditor class.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented May 22, 2013

Michael Osipov commented

Unfortunately, situation remains as-is. I deploy a workaround in my controller:

// Workaround for https://jira.springsource.org/browse/SPR-10402
if (StringUtils.isEmpty(query))
  throw new MissingServletRequestParameterException("q", "String");
if (ArrayUtils.isEmpty(attributes))
  throw new MissingServletRequestParameterException("attributes", "String[]");
@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Aug 14, 2013

Stefan Gheorghiu commented

This bug is still there in 3.2.3.RELEASE. Rolling back to 3.2.2 helps. Please fix it in 3.2.4.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Aug 14, 2013

Rossen Stoyanchev commented

I'm guessing you're referring to the bug as described in #15213? It was fixed in 3.2.4.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Aug 14, 2013

Stefan Gheorghiu commented

Exactly. I just used a wrong browser tab. Sorry for that.

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