Indeterministic UnsatisfiedServletRequestParameterException#paramConditions [SPR-12854]

[spring-projects-issues]

Adam Michalik opened SPR-12854 and commented

Consider a REST controller with the following request mappings on methods:

@RequestMapping(method = GET, params = "!myParam")
public void methodA(){...}

@RequestMapping(method = GET, params = "myParam=a")
public void methodB(){...}

When a request is made with myParam=b it does not match either of the mappings and an UnsatisfiedServletRequestParameterException is thrown in RequestMappingInfoHandlerMapping. However, the message of the exception is indeterministic - sometimes it's

Parameter conditions "myParam=a" not met for actual request parameters: myParam={b}

and sometimes

Parameter conditions "!myParam" not met for actual request parameters: myParam={b}

In my case it depended on which method was invoked first with a correct request after the server restart.
This is due to the logic in RequestMappingInfoHandlerMapping#getRequestParams where only first matching RequestMappingInfo is processed and the set of partialMatches is a HashSet with indeterministic order.

---

Affects: 4.1.6

Referenced from: commits b6449ba

[spring-projects-issues]

Juergen Hoeller commented

So if I understand correctly, the easy way to make it deterministic would be to use a LinkedHashSet there. I suppose that makes sense in general.

That said, what we really should be showing is an exception message for all the conditions that failed to apply...

Juergen

[spring-projects-issues]

Adam Michalik commented

I think it's not as easy as this - the Set<RequestMappingInfo> partialMatches parameter that is passed to RequestMappingInfoHandlerMapping#getRequestParams and that is the cause of the problem is now a HashSet, so changing it to a LinkedHashSet is one thing, but it is filled in iteratively from the Set<RequestMappingInfo> requestMappingInfos in RequestMappingInfoHandlerMapping#handleNoMatch. These come from AbstractHandlerMethodMapping#handlerMethods which is filled in AbstractHandlerMethodMapping#registerHandlerMethod. That is filled in AbstractHandlerMethodMapping#detectHandlerMethods by HandlerMethodSelector#selectMethods which indeed returns a LinkedHashSet but the order of the methods there depends on the order returned by java.lang.Class#getDeclaredMethods which is indeterministic.

All in all, the simplest solution would be to start working with a SortedSet at some point. However, my first impression was the same as your last sentence - why not display all of the mismatched partialMatches? Then the order in which they appear would not be so important.

[spring-projects-issues]

Rossen Stoyanchev commented

Listing all, collected from multiple handler methods, is misleading IMO. It implies all are required but isn't it a set of potential choices and there is no way of telling what the user's intent was. For the same reason does listing one consistently over the other change anything? Perhaps it makes it possible to write a test assertion but it's a random pick still.

When it comes down to two methods that are so alike down to the request parameters it's hard to say what would be helpful to show to the user? The only thing I can think of is relaxing the message on UnsatisfiedServletRequestParameterException to be more along the lines of: "one or more of the following parameter conditions..." vs "parameter conditions.

[spring-projects-issues]

Adam Michalik commented

I don't agree that it implies all are required - it depends how you put it. The example was my real-life case, but I assume we would have the same problem with simply

@RequestMapping(method = GET, params = "myParam=a")
public void methodA(){...}
 
@RequestMapping(method = GET, params = "myParam=b")
public void methodB(){...}

We should be able to tell from the exception that the valid options are myParam=a OR myParam=b. However, if we have

@RequestMapping(method = GET, params = {"myParam=a", "yourParam=b"})
public void methodAB(){...}
 
@RequestMapping(method = GET, params = "myParam=c")
public void methodC(){...}

we should be able to tell that the valid options are ((myParam=a AND yourParam=b) OR myParam=c).
It is the current implementation is misleading, since it would inform the caller only about one of the request mappings. Unfortunately, to support that, the UnsatisfiedServletRequestParameterException#paramConditions cannot be a flat String array.

[spring-projects-issues]

Rossen Stoyanchev commented

Unfortunately, to support that, the UnsatisfiedServletRequestParameterException#paramConditions cannot be a flat String array.

That's where I was going with this as well. So taking what you said, perhaps what we should do is change UnsatisfiedServletRequestParameterException to have contain a list of parameter condition groups, i.e. List<String[]>, which could then be expressed correctly in the exception message. The current getParamConditions method would basically return the first in the list.

[spring-projects-issues]

Adam Michalik commented

That would be a very reasonable solution.

[spring-projects-issues]

Juergen Hoeller commented

Let's revise this properly for 4.2, exposing condition metadata through new accessors if necessary. It's more of an improvement than a hard bug anyway, and we should restrict 4.1.7 to regression fixes and other must-haves for that branch.

Juergen