ETag/If-None-Match logic in HttpEntityMethodProcessor should not affect methods other than HTTP GET [SPR-13496]

[spring-projects-issues]

Francisco Lozano opened SPR-13496 and commented

A new etag-related logic has been introduced that breaks backward compatibility.

In HttpEntityMethodProcessor, the line:

|| clientETag.equals("*"))) {

maybe acceptable for GET, but definitely not for PUT/POST/PATCH verbs, because if I do:

PUT /some/resource
If-None-Match: *

the intention is to create the resource only if it doesn't exist previously. Forcing 304 not modified makes absolutely no sense here, because I want to return 201 created (which I set in my ResponseEntity<MyResponseObject>).

I haven't find any obvious way to disable this magic etag handling, that's why I'm marking this issue as blocker (feel free to downgrade if a workaround is available).

I think all this logic may make sense in some applications, but it can introduce heavy incompatibilities in existing REST APIs.

I wish I had found this when in RC - but I couldn't because of other issues of my components with 4.2 that only now I have been able to fix.

Full changes on this class:

git diff v4.1.7.RELEASE..v4.2.1.RELEASE -- spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/HttpEntityMethodProcessor.java

diff --git a/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/HttpEntityMethodProcessor.java b/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/HttpEntityMethodProcessor.java
index 26d06ef..22ebdbc 100644
--- a/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/HttpEntityMethodProcessor.java
+++ b/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/HttpEntityMethodProcessor.java
@@ -19,18 +19,22 @@ package org.springframework.web.servlet.mvc.method.annotation;
 import java.io.IOException;
 import java.lang.reflect.ParameterizedType;
 import java.lang.reflect.Type;
+import java.net.URI;
 import java.util.List;

 import org.springframework.core.MethodParameter;
 import org.springframework.core.ResolvableType;
 import org.springframework.http.HttpEntity;
 import org.springframework.http.HttpHeaders;
+import org.springframework.http.HttpMethod;
+import org.springframework.http.HttpStatus;
 import org.springframework.http.RequestEntity;
 import org.springframework.http.ResponseEntity;
 import org.springframework.http.converter.HttpMessageConverter;
 import org.springframework.http.server.ServletServerHttpRequest;
 import org.springframework.http.server.ServletServerHttpResponse;
 import org.springframework.util.Assert;
+import org.springframework.util.StringUtils;
 import org.springframework.web.HttpMediaTypeNotSupportedException;
 import org.springframework.web.accept.ContentNegotiationManager;
 import org.springframework.web.bind.support.WebDataBinderFactory;
@@ -48,31 +52,59 @@ import org.springframework.web.method.support.ModelAndViewContainer;
  *
  * @author Arjen Poutsma
  * @author Rossen Stoyanchev
+ * @author Brian Clozel
  * @since 3.1
  */
 public class HttpEntityMethodProcessor extends AbstractMessageConverterMethodProcessor {

-       public HttpEntityMethodProcessor(List<HttpMessageConverter<?>> messageConverters) {
-               super(messageConverters);
+
+       /**
+        * Basic constructor with converters only. Suitable for resolving
+        * {@code HttpEntity}. For handling {@code ResponseEntity} consider also
+        * providing a {@code ContentNegotiationManager}.
+        */
+       public HttpEntityMethodProcessor(List<HttpMessageConverter<?>> converters) {
+               super(converters);
+       }
+
+       /**
+        * Basic constructor with converters and {@code ContentNegotiationManager}.
+        * Suitable for resolving {@code HttpEntity} and handling {@code ResponseEntity}
+        * without {@code Request~} or {@code ResponseBodyAdvice}.
+        */
+       public HttpEntityMethodProcessor(List<HttpMessageConverter<?>> converters,
+                       ContentNegotiationManager manager) {
+
+               super(converters, manager);
        }

-       public HttpEntityMethodProcessor(List<HttpMessageConverter<?>> messageConverters,
-                       ContentNegotiationManager contentNegotiationManager) {
+       /**
+        * Complete constructor for resolving {@code HttpEntity} method arguments.
+        * For handling {@code ResponseEntity} consider also providing a
+        * {@code ContentNegotiationManager}.
+        * @since 4.2
+        */
+       public HttpEntityMethodProcessor(List<HttpMessageConverter<?>> converters,
+                       List<Object> requestResponseBodyAdvice) {

-               super(messageConverters, contentNegotiationManager);
+               super(converters, null, requestResponseBodyAdvice);
        }

-       public HttpEntityMethodProcessor(List<HttpMessageConverter<?>> messageConverters,
-                       ContentNegotiationManager contentNegotiationManager, List<Object> responseBodyAdvice) {
+       /**
+        * Complete constructor for resolving {@code HttpEntity} and handling
+        * {@code ResponseEntity}.
+        */
+       public HttpEntityMethodProcessor(List<HttpMessageConverter<?>> converters,
+                       ContentNegotiationManager manager, List<Object> requestResponseBodyAdvice) {

-               super(messageConverters, contentNegotiationManager, responseBodyAdvice);
+               super(converters, manager, requestResponseBodyAdvice);
        }


        @Override
        public boolean supportsParameter(MethodParameter parameter) {
-               return (HttpEntity.class.equals(parameter.getParameterType()) ||
-                               RequestEntity.class.equals(parameter.getParameterType()));
+               return (HttpEntity.class == parameter.getParameterType() ||
+                               RequestEntity.class == parameter.getParameterType());
        }

        @Override
@@ -90,9 +122,10 @@ public class HttpEntityMethodProcessor extends AbstractMessageConverterMethodPro
                Type paramType = getHttpEntityType(parameter);

                Object body = readWithMessageConverters(webRequest, parameter, paramType);
-               if (RequestEntity.class.equals(parameter.getParameterType())) {
-                       return new RequestEntity<Object>(body, inputMessage.getHeaders(),
-                                       inputMessage.getMethod(), inputMessage.getURI());
+               if (RequestEntity.class == parameter.getParameterType()) {
+                       URI url = inputMessage.getURI();
+                       HttpMethod httpMethod = inputMessage.getMethod();
+                       return new RequestEntity<Object>(body, inputMessage.getHeaders(), httpMethod, url);
                }
                else {
                        return new HttpEntity<Object>(body, inputMessage.getHeaders());
@@ -131,22 +164,74 @@ public class HttpEntityMethodProcessor extends AbstractMessageConverterMethodPro

                Assert.isInstanceOf(HttpEntity.class, returnValue);
                HttpEntity<?> responseEntity = (HttpEntity<?>) returnValue;
-               if (responseEntity instanceof ResponseEntity) {
-                       outputMessage.setStatusCode(((ResponseEntity<?>) responseEntity).getStatusCode());
-               }

                HttpHeaders entityHeaders = responseEntity.getHeaders();
                if (!entityHeaders.isEmpty()) {
                        outputMessage.getHeaders().putAll(entityHeaders);
                }
-
                Object body = responseEntity.getBody();
+               if (responseEntity instanceof ResponseEntity) {
+                       outputMessage.setStatusCode(((ResponseEntity<?>) responseEntity).getStatusCode());
+                       if (isResourceNotModified(inputMessage, outputMessage)) {
+                               outputMessage.setStatusCode(HttpStatus.NOT_MODIFIED);
+                               // Ensure headers are flushed, no body should be written.
+                               outputMessage.flush();
+                               // Skip call to converters, as they may update the body.
+                               return;
+                       }
+               }

                // Try even with null body. ResponseBodyAdvice could get involved.
                writeWithMessageConverters(body, returnType, inputMessage, outputMessage);

                // Ensure headers are flushed even if no body was written.
-               outputMessage.getBody();
+               outputMessage.flush();
+       }
+
+       private boolean isResourceNotModified(ServletServerHttpRequest inputMessage, ServletServerHttpResponse outputMessage) {
+
+               List<String> ifNoneMatch = inputMessage.getHeaders().getIfNoneMatch();
+               long ifModifiedSince = inputMessage.getHeaders().getIfModifiedSince();
+               String eTag = addEtagPadding(outputMessage.getHeaders().getETag());
+               long lastModified = outputMessage.getHeaders().getLastModified();
+               boolean notModified = false;
+
+               if (lastModified != -1 && StringUtils.hasLength(eTag)) {
+                       notModified = isETagNotModified(ifNoneMatch, eTag) && isTimeStampNotModified(ifModifiedSince, lastModified);
+               }
+               else if (lastModified != -1) {
+                       notModified = isTimeStampNotModified(ifModifiedSince, lastModified);
+               }
+               else if (StringUtils.hasLength(eTag)) {
+                       notModified = isETagNotModified(ifNoneMatch, eTag);
+               }
+               return notModified;
+       }
+
+       private boolean isETagNotModified(List<String> ifNoneMatch, String etag) {
+               if (StringUtils.hasLength(etag)) {
+                       for (String clientETag : ifNoneMatch) {
+                               // compare weak/strong ETags as per https://tools.ietf.org/html/rfc7232#section-2.3
+                               if (StringUtils.hasLength(clientETag) &&
+                                               (clientETag.replaceFirst("^W/", "").equals(etag.replaceFirst("^W/", ""))
+                                                               || clientETag.equals("*"))) {
+                                       return true;
+                               }
+                       }
+               }
+               return false;
+       }
+
+       private boolean isTimeStampNotModified(long ifModifiedSince, long lastModifiedTimestamp) {
+               return (ifModifiedSince >= (lastModifiedTimestamp / 1000 * 1000));
+       }
+
+       private String addEtagPadding(String etag) {
+               if (StringUtils.hasLength(etag) &&
+                               (!(etag.startsWith("\"") || etag.startsWith("W/\"")) || !etag.endsWith("\"")) ) {
+                       etag = "\"" + etag + "\"";
+               }
+               return etag;
        }

        @Override

---

Affects: 4.2 GA, 4.2.1

Issue Links:

• ServletWebRequest.checkNotModified(…) writes Last-Modified header in invalid format [SPR-13090] #17681 ServletWebRequest.checkNotModified(…) writes Last-Modified header in invalid format
• Support for conditional PUT in Web MVC (using If-Unmodified-Since header) [SPR-13863] #18436 Support for conditional PUT in Web MVC (using If-Unmodified-Since header)
• Improve and/or provide control over ETag/If-None-Match logic in HttpEntityMethodProcessor [SPR-13626] #18204 Improve and/or provide control over ETag/If-None-Match logic in HttpEntityMethodProcessor

Referenced from: commits 583a48a

1 votes, 5 watchers

[spring-projects-issues]

Brian Clozel commented

Thanks for reporting this!
We're now triggering that code path only for GET requests.

[spring-projects-issues]

Francisco Lozano commented

Thanks for the fix ... anyway, it'd be great if all this magic etag handling was fully optional somehow.

[spring-projects-issues]

Brian Clozel commented

Francisco Lozano It's always hard to draw the line between "giving options, not enforcing strong opinions" and "being easy to configure".

If you think this conditional requests support can be problematic, you should definitely open a new enhancement issue for you and other developers to describe their use cases and vote for this request.

[spring-projects-issues]

Francisco Lozano commented

well it's a quite drastic change that may pass unnoticed in many web apps, but for sure affects any REST API implementation that is already using etags and handling these internally. If you add a new behaviour that silently breaks stuff, I think a way to revert that should be provided... and that's not an "enhancement", that's just "allow to optionally not-break existing stuff".

[spring-projects-issues]

Rossen Stoyanchev commented

After the fix for this issue to constrain the behavior to HTTP GET, is there anything else that still breaks for you? Could you elaborate? Thanks.

[spring-projects-issues]

Francisco Lozano commented

The remaining issue is that older REST clients that don't handle caching correctly may send If-Modified-Since / If-None-Match when doing GET but still not support conditional request/response flow correctly.

I have clients with broken cache/conditional behaviour like that, unfortunately. Those clients are working well without this feature, but they will break immediately if I release my server with Spring 4.2. Browsers are open and flexible, but poorly written HTTP clients are not and simple changes like these can break them.

I understand the new behaviour is RFC-compliant, but it can break existing apps - it would break my app. I think an opt-out for it is necessary.

[spring-projects-issues]

Brian Clozel commented

I'm confused - are you using ResponseEntity.eTag and ResponseEntity.lastModified in your implementation?

Not using those methods should be the simplest way to opt out; if REST clients have a poor conditional request implementation, then those response headers are useless and could confuse them.

[spring-projects-issues]

Francisco Lozano commented

Yes, I am using those headers, especially eTag, I use heavily for conditional updates and such - that's why I got affected by this issue in the first place... I use by directly writing headers, not using the specific methods in ResponseEntity. I don't think when I started writing my code these existed (not sure now).

I cannot not-use those headers as they're part of my API. However, I have clients that are currently working (since Spring 3.0), that have fragile conditional request implementation and that will fail if the server ever returns 304.

That's the exact part that I need to opt-out, the GET returning 304 and skipping the body: I have clients that I don't control and that will break with this behaviour.

My point is that this change enforces me to change my API's contract with no possibility to opt-out, and I don't think that's fair at all.

[spring-projects-issues]

Brian Clozel commented

We could provide an option to disable this: you'd have to configure your own RequestMappingHandlerAdapter bean and use the setReturnValueHandlers method on it to override the default list of ReturnValueHandlers.

This would probably make your application configuration more complex, as I believe this would prevent the use of @EnableWebMvc or require the use of a BeanPostProcessor.

Thoughts?

[spring-projects-issues]

Francisco Lozano commented

That would be painfully complex configuration, right? mostly code copy&paste...

[spring-projects-issues]

Francisco Lozano commented

Is this going to stay like this definitely? My API is broken with 4.2 and I cannot do anything about it other than copy&paste a lot of Spring internals and modify myself.

[spring-projects-issues]

Juergen Hoeller commented

Brian Clozel, Rossen Stoyanchev, I suppose we should create a follow-up JIRA issue that's specifically about a convenient opt-out variant?

[spring-projects-issues]

Brian Clozel commented

My previous comment described a possible workaround for this, but it seems that this solution is not acceptable for you. Where would you see this new configuration?

Could you elaborate on those HTTP clients, as knowing their name/popularity could tell us how big is the audience for this?

[spring-projects-issues]

Francisco Lozano commented

My previous comment described a possible workaround for this, but it seems that this solution is not acceptable for you. Where would you see this new configuration?

The workaround described, as I answered, involves copying Spring internal code that I, as a framework user, should have no business touching (let alone copying into my classes). My configuration code would brutally explode just for disabling a new feature.

I think one possibility would be to allow to configure (disable) the behaviour in the WebMvcConfigurationSupport class - like the configuration methods that allow you to customize view resolvers or cors mappings from there.

Could you elaborate on those HTTP clients, as knowing their name/popularity could tell us how big is the audience for this?

When I say client I mean a client application (mobile app, iot device) with an existing software running and a set of expectations, not a specific http client library. I can show you an example of code that uses RestTemplate that becomes broken with this change (extracted from integration test):

		// Headers
		HttpHeaders headers = new HttpHeaders();
		headers.set(HttpHeaderConst.AUTHORIZATION, "Bearer " + token;
		headers.setIfNoneMatch("*");

		// Entity
		final HttpEntity<?> requestEntity = new HttpEntity<>(headers);

		// Get object body via REST
		final ResponseEntity<InputStream> resObjBody = restTemplate.exchange(
				url, HttpMethod.GET, requestEntity, InputStream.class);

		byte[] obtained = IOUtils.toByteArray(resObjBody.getBody());

With 4.2 in the server, a NPE will happen in the last line.

Any app client that has above expectation will fail, no matter what client technology is used. I could force clients to update if I controlled them (that would be bad), but I don't control them and thus I cannot force anything (I can just break my API).

You could say above code is "wrong" as per RFC (that's debatable), but it's a compatibility break with no possible opt-out: It worked with a server based in 4.1, and cannot work with a server based in 4.2 because the new code enforces the server to answer with 3xx and the response body is null. It could be acceptable (again, debatable) in a pure Web/browser environment, but definitely it's not in an environment in which clients can be applications or devices.

[spring-projects-issues]

Brian Clozel commented

Overloading an existing MVC Bean is a well described solution (see Javadoc) for specific needs that are rare or behavior that we tend to warn against.

WebMvcConfigurationSupport is used to expose high-level customization methods that should be easily found and usable; adding an enable/disable method there for each small MVC new feature added in a dot release would be messy really quickly.

Since we're talking specific applications, not libraries, this makes even a stronger case for a configuration flag that will be rarely used.
The code you're showing would effectively fail for any response with an empty body - 404s, 204...

You could say above code is "wrong" as per RFC (that's debatable), but it's a compatibility break with no possible opt-out: It worked with a server based in 4.1, and cannot work with a server based in 4.2 because the new code enforces the server to answer with 3xx and the response body is null. It could be acceptable (again, debatable) in a pure Web/browser environment, but definitely it's not in an environment in which clients can be applications or devices.

This behavior shows an HTTP client asking for potential HTTP 304 and breaking on empty bodies. I don't think this new feature qualifies as a compatibility break; this is in fact a very small feature dealing with a very old, battle-tested, ubiquitous spec: HTTP. HTTP clients (not only browsers) sending conditional requests should know how to deal with them.

Now I totally understand (and sympathize) the fact that you can't change those clients and that you have to deal with a production application.
Judging from our discussion here, I can see three ways to deal with this:

1. Writing a custom servlet Filter or WebRequestInterceptor in your app: wrap requests sent by those particular user agents, and delete those request headers. This way your app can still handle conditional requests for other clients, or future versions of those clients, while still being very easy to configure and maintain
2. Implementing the workaround we discussed
3. Creating a new feature request issue with an easier way to configure this, and ask the community to describe their own use cases and vote on this issue

[spring-projects-issues]

Francisco Lozano commented

I guess it's not my call and you are not changing your mind about this, but I would not consider conditional request handling a "small MVC new feature" - it's maybe small with regard to implementation, but the surface impact it has is enormous.

Why CORS deserves configuration in WebMvcConfigurationSupport and conditional request handling does not?

About the possible solutions:

1. is not robust enough, I cannot endorse that solution I'm afraid.

2. I understand and agree that overloading a bean is a good way to override and customize behaviour, but the thing is that HttpEntityMethodProcessor is not a bean AFAIK, the whole RequestMappingHandlerAdapter is, so it's not as easy as "just override a bean" (HttpEntityMethodProcessor instance), it's override a bean (RequestMappingHandlerAdapter) and implement behaviour on top of it, looking at its source code and copying/manipulating stuff. It doesn't look like an acceptable workaround to me.

It would be OK to have my own HttpEntityMethodProcessor, but having my own RequestMappingHandlerAdapter and reimplement getDefaultArgumentResolvers() goes beyond reasonable "customization", as that method really does a lot of very heterogeneous stuff. Moreover, that method is private and uses private state (requestResponseBodyAdvice) that I could not touch. I could override afterPropertiesSet and call super() methods to initialize the original state and then initialize a manipulated version... but I guess you can see how complex and unreliable this could be.

If you could make it a little easier to customize RequestMappingHandlerAdapter, it could be a good way to solve it, but in current state I don't think that it can be considered practical to customize RequestMappingHandlerAdapter beyond the few setters that there are.

3. With all due respect and gratefulness, this is where I whole-heartedly disagree with the approach taken in this issue. You really think that a "new feature" whose purpose is "to disable another new feature that can potentially break existing stuff" makes any sense? If a new feature has the potential of breaking stuff that has been working before, it should not be mandatory, and "a new feature" should not be needed to make it optional. I have explained this a few times in my comments, should I understand that the Spring Framework developers disagrees on this point of view about backward compatibility?

[spring-projects-issues]

Brian Clozel commented

I'm always open to new arguments and happy to change my mind.

Right now, I don't think this use case qualifies for a configuration facility at the WebMvcConfigurationSupport level.
This feature adds up to our existing conditional request support, as you can see in the ref doc. Such a configuration flag would only work for ResponseEntity support, not for all the other cases (WebRequest, static resources, etc). Modifying RequestMappingHandlerAdapter method signatures would be a breaking change.

I agree that #2 is quite involved and harder to maintain, but that's the best solution I can come up with right now on our side. I still believe that a Filter/Interceptor solution is the best choice, but that's not my call.

Again, involving the community is a way forward - gathering more use cases and traction behind this would only help.

[spring-projects-issues]

Francisco Lozano commented

Thanks for your comments and explanation. I don't think this is an acceptable answer, but I am giving up fighting it.

[spring-projects-issues]

Juergen Hoeller commented

Francisco Lozano, please don't give up. We want you to be able to upgrade to 4.2+ without too much concern... In particular, opening up RequestMappingHandlerAdapter is something that I'm pretty keen on, so if you have a concrete proposal, please contribute it - as a pull request or as a patch. As long as it does not break existing public/protected signatures, we could even roll this into 4.2.3 still.

Juergen

[spring-projects-issues]

Rossen Stoyanchev commented

There have been several ideas mentioned. We are simply trying to find some common ground. So indeed don't give up, your feedback is valuable.

A slightly different direction but looking at the example above with GET + If-None-Match: * and matching to the spec:

The meaning of "If-None-Match: *" is that the method MUST NOT be performed if the representation selected by the origin server (or by a cache, possibly using the Vary mechanism, see section 14.44) exists, and SHOULD be performed if the representation does not exist. This feature is intended to be useful in preventing races between PUT operations.

Isn't this always a 304? The "*" is clearly not intended for GET. Perhaps you omitted some other headers for brevity? If this is the exact situation you have then there might be a simple solution. For once Brian's suggestion to wrap the request to suppress the "If-None-Match" can be an effective workaround. We can also address this in HttpEntityMethodProcessor itself. If we see a GET with If-None-Match: * (and no other validation headers) ignore "If-None-Match" and proceed as usual.

Even the spec has clarifications for undefined combinations of headers, for example:

The result of a request having both an If-None-Match header field and either an If-Match or an If-Unmodified-Since header fields is undefined by this specification.

So if we encounter something we can't make sense of, we simply ignore it and effectively treat it as invalid (the client doesn't know what they're doing). So could it be as simple as that?

[spring-projects-issues]

Rossen Stoyanchev commented

I've updated the title to reflect the actual fix for this ticket in 4.2.2 (something we can no longer change). I've also created a separate ticket #18204 to discuss the remaining questions.