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

HEAD/GET in MVC not fully backward-compatible [SPR-14383] #18956

Closed
spring-projects-issues opened this issue Jun 20, 2016 · 14 comments
Closed

HEAD/GET in MVC not fully backward-compatible [SPR-14383] #18956

spring-projects-issues opened this issue Jun 20, 2016 · 14 comments
Assignees
Milestone

Comments

@spring-projects-issues
Copy link
Collaborator

@spring-projects-issues spring-projects-issues commented Jun 20, 2016

Francisco Lozano opened SPR-14383 and commented

I have a controller that looks more or less like this (just relevant parts):

@RequestMapping("/abcd/{myID}/stuff")
public class StuffController extends AbstractBaseController {
	/* ... */
	@ResponseBody
	@ResponseStatus(value = HttpStatus.NO_CONTENT)
	@RequestMapping(value = "/*", method = HEAD)
	public void checkExists(@PathVariable MyID myID) throws StuffNotFoundException {
		stuffService.checkExists(myID, parsePath());
	}


	@ResponseBody
	@RequestMapping(value = "/*", method = GET, produces = StuffRetrievalResponse.JSON_MIME_TYPE)
	public StuffRetrievalResponse get(@PathVariable MyID myID) throws StuffNotFoundException {
		Stuff stuff = stuffService.get(myID, parsePath());
		StuffRetrievalResponse res = new StuffRetrievalResponse();
		fill(res, stuff);
		return res;
	}

}

Now, always GET gets invoked instead of HEAD. We are still debugging but we're thinking that the compareTo method in RequestMethodsRequestCondition is related to the issue.


Affects: 4.3 GA

Referenced from: commits 058279b

1 votes, 3 watchers

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jun 20, 2016

Francisco Lozano commented

Maybe related to #18753 / 7cdcc10 ?

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jun 20, 2016

Rossen Stoyanchev commented

It sounds exactly like #18753 which was fixed in 4.3 RC2. Are you sure you're not running with an older RC by any chance?

I can't reproduce it with 4.3 GA and the above controller:
https://github.com/spring-projects/spring-framework-issues/tree/master/SPR-14383

[master]$ curl -v http://localhost:8080/abcd/123/stuff/morestuff
*   Trying 127.0.0.1...
* Connected to localhost (127.0.0.1) port 8080 (#0)
> GET /abcd/123/stuff/morestuff HTTP/1.1
> Host: localhost:8080
> User-Agent: curl/7.47.0
> Accept: */*
> 
< HTTP/1.1 200 OK
< Date: Mon, 20 Jun 2016 17:27:55 GMT
< Content-Length: 0
< Server: Jetty(9.3.3.v20150827)
< 
* Connection #0 to host localhost left intact

[master]$ curl -v --head http://localhost:8080/abcd/123/stuff/morestuff
*   Trying 127.0.0.1...
* Connected to localhost (127.0.0.1) port 8080 (#0)
> HEAD /abcd/123/stuff/morestuff HTTP/1.1
> Host: localhost:8080
> User-Agent: curl/7.47.0
> Accept: */*
> 
< HTTP/1.1 204 No Content
HTTP/1.1 204 No Content
< Date: Mon, 20 Jun 2016 17:28:00 GMT
Date: Mon, 20 Jun 2016 17:28:00 GMT
< Server: Jetty(9.3.3.v20150827)
Server: Jetty(9.3.3.v20150827)

< 
* Connection #0 to host localhost left intact
@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jun 20, 2016

Francisco Lozano commented

I'm 100% sure we're running 4.3.0 final, not RC:

$ unzip ../webapps/ROOT.war |grep spring
  inflating: WEB-INF/lib/spring-webmvc-4.3.0.RELEASE.jar
  inflating: WEB-INF/lib/spring-tx-4.3.0.RELEASE.jar
  inflating: WEB-INF/lib/spring-xmemcached-999.1-SNAPSHOT.jar
  inflating: WEB-INF/lib/spring-initializer-999.1-SNAPSHOT.jar
  inflating: WEB-INF/lib/spring-expression-4.3.0.RELEASE.jar
  inflating: WEB-INF/lib/spring-test-4.3.0.RELEASE.jar
  inflating: WEB-INF/lib/spring-aop-4.3.0.RELEASE.jar
  inflating: WEB-INF/lib/spring-context-4.3.0.RELEASE.jar
  inflating: WEB-INF/lib/spring-context-support-4.3.0.RELEASE.jar
  inflating: WEB-INF/lib/spring-web-4.3.0.RELEASE.jar
  inflating: WEB-INF/lib/spring-jdbc-4.3.0.RELEASE.jar
  inflating: WEB-INF/lib/spring-beans-4.3.0.RELEASE.jar
  inflating: WEB-INF/lib/spring-core-4.3.0.RELEASE.jar
  inflating: WEB-INF/lib/spring-retry-1.1.2.RELEASE.jar
  inflating: WEB-INF/lib/spring-client-netty-999.1-SNAPSHOT.jar
  inflating: WEB-INF/lib/future-converter-spring-java8-0.3.0.jar
  inflating: WEB-INF/lib/hazelcast-spring-3.6.3.jar
  inflating: WEB-INF/lib/spring-data-redis-1.7.1.RELEASE.jar
  inflating: WEB-INF/lib/spring-ldap-core-2.1.0.RELEASE.jar
  inflating: WEB-INF/lib/spring-consul-999.1-SNAPSHOT.jar
  inflating: WEB-INF/lib/spring-rabbit-1.6.0.RELEASE.jar
  inflating: WEB-INF/lib/spring-data-keyvalue-1.1.1.RELEASE.jar
  inflating: WEB-INF/lib/spring-oxm-4.3.0.RELEASE.jar
  inflating: WEB-INF/lib/spring-amqp-1.6.0.RELEASE.jar
  inflating: WEB-INF/lib/spring-messaging-4.3.0.RELEASE.jar
  inflating: WEB-INF/lib/spring-data-commons-1.12.1.RELEASE.jar

I will try to isolate the behaviour, the controller is pretty complex (code pasted above is just a subset).

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jun 20, 2016

Rossen Stoyanchev commented

Okay I figured it was pretty unlikely but worth checking.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jun 20, 2016

Francisco Lozano commented

OK, something strange:
I'm debugging my code to try to figure out what's going on with it. I have found that, if my controller looks like the one I pasted initially (head without "produces"), the "get" method wins and the request goes to it. However, if I add a "produces" to the method handler:

@RequestMapping(value = "/*", method = HEAD, produces = "application/json")

the code goes to the head, as it used to in 4.2.

But this doesn't explain why you can't reproduce with the example above... :/

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jun 20, 2016

Rossen Stoyanchev commented

Okay that's the part I was missing. If I add an Accept header I can see the issue:

curl -v --head -H "Accept:application/json" http://localhost:8080/abcd/123/stuff/morestuff

I presume you have that?

So I guess In RequestMappingInfo#compareTo we look at the produces condition before the HTTP methods, and in this case the produces condition always resolves in favor of the method that has a matching produces condition.

I'm wondering now if this is right or wrong. Perhaps we need to update RequestMappingInfo#compareTo to check the HTTP method earlier in case it is an HTTP HEAD request.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jun 20, 2016

Francisco Lozano commented

yes, our client adds "Accept" header

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jun 20, 2016

Rossen Stoyanchev commented

For the "right or wrong" comment what I meant is that maybe it's behaving as it should be (forgetting for a moment the fact it's a regression). After all HEAD is meant to be the same as GET minus the body. However an explicit mapping to HTTP HEAD clearly should take precedence.

To explain the current behavior of compareTo order, the only way that the HTTP method can be a deal breaker is if one controller method was declared with an explicit HTTP method and the other with none (i.e. matching to any method). This is why we check methods late, after all other conditions which are based on explicit declarations only. However with HTTP HEAD support we've introduced a new case for HTTP method comparison.

For HTTP HEAD we need to check the HTTP method first so that an explicit mapping to HEAD always wins over any other mapping. For all other cases, the HTTP method comparison needs to remain as it is now after all other conditions. That means also that an explicit HTTP HEAD mapping would take precedence over more specific URL patterns, which is really generalization of the case here with a produces condition.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jun 20, 2016

Francisco Lozano commented

I understand the fact that HEAD is supposed to be the same as GET. In this case, we took advantage of the different verb to implement a little different behaviour: we allow to check for existence of a resource (with HEAD) but we only allow to retrieve its content to some principals (with GET). That's how we found this situation, because in a test 404 was expected but 401 was returned instead.

To me, intuitively HTTP method is "naturally" the first piece to check, but I didn't consider the possibility of having no-HTTP-method mappings... sounds tricky.

I will workaround it for now by adding "produces" to our HEAD mappings, which is not incorrect in any way and is not a big effort. Thanks a lot for taking a look at this issue!

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jun 20, 2016

Rossen Stoyanchev commented

Come to think of it, Isn't having the produces condition on the HEAD mapping necessary? If the Accept header is not application/json the response for the GET should be a 415 instead and the HEAD should match that.

This doesn't change my thinking that we need a fix in RequestMappingInfo so that explicit HEAD mapping has precedence. It's just that as a result it becomes the application responsibility to ensure that the resulting HEAD responses are consistent with GET.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jun 21, 2016

Francisco Lozano commented

I think it's OK to let the developer be responsible of that. Developer is already responsible for that in 4.2, and to me it follows the principle of least surprise.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jun 21, 2016

Rossen Stoyanchev commented

I've pushed a fix and confirmed it with the repro project but would be great if you could confirm also after this build is done.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jun 22, 2016

Francisco Lozano commented

My test passes now with latest 4.3.1 SNAPSHOT, thank you!

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jun 22, 2016

Rossen Stoyanchev commented

Thanks for confirming.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants