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