Exceptions for missing request values should expose information when they are missing after conversion

[drahkrub]

In Spring 5.3 it is possible that a MissingServletRequestParameterException is thrown although a required parameter is provided (and therefore not missing). The same holds for a required path variable - even it is provided a MissingPathVariableException may be thrown.

Moreover the MissingServletRequestParameterException triggers a response status 400 in contrast to the MissingPathVariableException which ends up in a response status 500.

The former happens due to the changes made in #23939: If a provided parameter (request parameter or path variable) is converted to null it is handled as a missing value, see AbstractNamedValueMethodArgumentResolver.

I would like to propose:

1. Adjust the response status.
2. Throw a different exception (e.g. IllegalArgumentException) if a required parameter (request parameter or path variable) is provided but converted to null.

In a lengthy discussion with @rstoyanchev in #26088 (I thank his patience) I gave an example to explain that the current status is problematic:

1. two users load the same book entity via its id, let's say 42
2. one user deletes the book entity
3. the other user continues using URLs with the id 42

I attached a small spring boot app with several test cases (github, spring_sandbox-master.zip).

With

@GetMapping(path = "/rp/book")
public Book getBookByRequestParam(@RequestParam("id") Book book) {
    ...

calling /rp/book?id=42 leads to a MissingServletRequestParameterException which is also thrown if /edit is called without a parameter or with an empty parameter - these cases can not be distinguished in an elegant way, see the tests

• noRequestParamThrowsMissingServletRequestParameterException
• emptyRequestParamThrowsMissingServletRequestParameterException
• idOfDeletedBookThrowsMissingServletRequestParameterException

In contrast calling

@GetMapping({"/pv/book/{id}"})
public Book getBookByPathVariable(@PathVariable("id") Book book) {
    ...

with /pv/book/42 "only" leads to a misleading MissingPathVariableException, but the method can not be called with no or an empty parameter (if /pv/book is not mapped to a different method it is handled by the ResourceHttpRequestHandler, see the test missingValueNotHandledByController).

But a conflicting case can be constructed this way:

@GetMapping({"/cc/book", "/cc/book/{id}"})
public Book constructedCase(@PathVariable("id") Book book) {
    ...

Here calling /cc/book and /cc/book/42 both lead to a MissingPathVariableException - but as I said this is constructed resp. bad programming.

[drahkrub]

And a general remark: For me the changes made in Spring 5.3 feel like a strange (not to say wrong) mix of concepts.

@RequestParam(required = true) always ment to me that the presence of the request param is required, not that its value is guaranteed to not result in null.

To express "guaranteed to not result in null" I would see or create something like @RequestParam(nullable = false).

In my discussion with @rstoyanchev in #26088 there are some inaccuracies on my side but interested readers can find more arguments over there (from an old school programmer who is using Spring for around 15 years now and who is picking up innovations (not only in the java programming language) only slowly ;-)).

[rstoyanchev]

@drahkrub indeed at the point of raising the MissingServletRequestParameterException we do know if the parameter is present and non-empty. We can update the exception with a property such as conversionFailed and an error message indicative of the the fact the value was present but was converted to null. This would make it easy to differentiate and handle the error differently with an @ExceptionHandler either within a controller or across controllers via @ControllerAdvice.

Beyond that there are more options. For example in your own Converter (as in your sample), you can return null, raise a different exception (e.g. entity not found), or create a default value. Or to handle it locally within a controller method, declare the argument as @Nullable or Optional. In any case when an entity is null, I imagine either a 404 or a 400 is in order so an @ExceptionHandler method is probably the way to go.

We've debated how @RequestParam or @PathVariable handle opting out of being required, and I would like to add that it is consistent with how the same is done across the Spring Framework for annotation-based handler methods and for dependency injection, including in constructor args. There is an established pattern for that already and we're not going to add a nullable attribute to annotations.

[drahkrub]

@rstoyanchev Related to the three paragraphs you wrote:

1. That would be a step forward! Although a special exception (maybe extending MissingServletRequestParameterException) would be more practical I think.
2. "raise a different exception (e.g. entity not found)" - as you wrote, that only works in my own Converters, not in e.g. Spring Datas DomainClassConverter. All other options (including @Nullable and Optional) also have disadvantages...
3. Ok, I didn't seriously expect that either, but I just couldn't resist to raise this point one more time (and for the last time) again ;-)

What about the different response status? Are they intentional?

[rstoyanchev]

I've added a missingAfterConversion flag with a common base class for all exceptions related to missing request values. Due to the number of exceptions it's not feasible to go with a different exception (as a sub-class) approach.

[Stexxen]

Just wanted to make a note that this change appeared during an upgrade from Spring Boot 2.4.4 to 2.4.5 and it broke some of our tests where we are checking the exceptions getMessage() after it has been converted to a json response.

Should a point release dramatically change an exceptions message is what I'm asking here? I could understand this appearing in an upgrade of 5.3.x to 5.4.0 but a point release.... I'm not so sure...

[skaba]

Will this be backported to Spring 5?