Skip to content

Commit 39d689d

Browse files
bclozelbclozel
committed
Fix conditional requests support for HttpEntity
Prior to this commit, `HttpEntityMethodProcessor` would rely on `ServletWebRequest` to process conditional requests and with incoming `"If-Modified-Since"` / `"If-None-Match"` request headers. This approach is problematic since in that class: * response is wrapped in a `ServletServerHttpResponse` * this wrapped response does not write response headers right away * `ServletWebRequest.checkNotModified` methods can't apply their logic with incomplete response headers This solution adds some minimal code duplication and applies the conditional request logic within the Processor. A possible alternative would be to improve the `ServletServerHttpResponse$ServletResponseHttpHeaders` implementation with write methods - but this solution would only work for Servlet 3.x applications. Issue: SPR-13090
1 parent 338a18e commit 39d689d

File tree

4 files changed

+88
-48
lines changed

4 files changed

+88
-48
lines changed

‎spring-web/src/main/java/org/springframework/web/context/request/ServletWebRequest.java

+15-21
Original file line numberDiff line numberDiff line change
@@ -181,31 +181,19 @@ public boolean isSecure() {
181181
public boolean checkNotModified(long lastModifiedTimestamp) {
182182
HttpServletResponse response = getResponse();
183183
if (lastModifiedTimestamp >= 0 && !this.notModified) {
184-
if (response == null || isResponseCompatibleWithConditional(response, HEADER_LAST_MODIFIED)) {
184+
if (response == null || HttpStatus.valueOf(response.getStatus()).is2xxSuccessful()) {
185185
this.notModified = isTimeStampNotModified(lastModifiedTimestamp);
186186
if (response != null) {
187187
if (this.notModified && supportsNotModifiedStatus()) {
188188
response.setStatus(HttpServletResponse.SC_NOT_MODIFIED);
189189
}
190-
response.setHeader(HEADER_LAST_MODIFIED, formatDate(lastModifiedTimestamp));
191-
}
192-
}
193-
}
194-
return this.notModified;
195-
}
196-
197-
private boolean isResponseCompatibleWithConditional(HttpServletResponse response, String... headers) {
198-
if (response != null) {
199-
if (HttpStatus.valueOf(response.getStatus()).is2xxSuccessful()) {
200-
for (String header : headers) {
201-
if (response.containsHeader(header)) {
202-
return false;
190+
if(response.getHeader(HEADER_LAST_MODIFIED) == null) {
191+
response.setHeader(HEADER_LAST_MODIFIED, formatDate(lastModifiedTimestamp));
203192
}
204193
}
205-
return true;
206194
}
207195
}
208-
return false;
196+
return this.notModified;
209197
}
210198

211199
@SuppressWarnings("deprecation")
@@ -235,14 +223,16 @@ private boolean isTimeStampNotModified(long lastModifiedTimestamp) {
235223
public boolean checkNotModified(String etag) {
236224
HttpServletResponse response = getResponse();
237225
if (StringUtils.hasLength(etag) && !this.notModified) {
238-
if (response == null || isResponseCompatibleWithConditional(response, HEADER_ETAG)) {
226+
if (response == null || HttpStatus.valueOf(response.getStatus()).is2xxSuccessful()) {
239227
etag = addEtagPadding(etag);
240228
this.notModified = isETagNotModified(etag);
241229
if (response != null) {
242230
if (this.notModified && supportsNotModifiedStatus()) {
243231
response.setStatus(HttpServletResponse.SC_NOT_MODIFIED);
244232
}
245-
response.setHeader(HEADER_ETAG, etag);
233+
if(response.getHeader(HEADER_ETAG) == null) {
234+
response.setHeader(HEADER_ETAG, etag);
235+
}
246236
}
247237
}
248238
}
@@ -283,15 +273,19 @@ private boolean supportsNotModifiedStatus() {
283273
public boolean checkNotModified(String etag, long lastModifiedTimestamp) {
284274
HttpServletResponse response = getResponse();
285275
if (StringUtils.hasLength(etag) && !this.notModified) {
286-
if (response == null || isResponseCompatibleWithConditional(response, HEADER_ETAG, HEADER_LAST_MODIFIED)) {
276+
if (response == null || HttpStatus.valueOf(response.getStatus()).is2xxSuccessful()) {
287277
etag = addEtagPadding(etag);
288278
this.notModified = isETagNotModified(etag) && isTimeStampNotModified(lastModifiedTimestamp);
289279
if (response != null) {
290280
if (this.notModified && supportsNotModifiedStatus()) {
291281
response.setStatus(HttpServletResponse.SC_NOT_MODIFIED);
292282
}
293-
response.setHeader(HEADER_ETAG, etag);
294-
response.setHeader(HEADER_LAST_MODIFIED, formatDate(lastModifiedTimestamp));
283+
if(response.getHeader(HEADER_ETAG) == null) {
284+
response.setHeader(HEADER_ETAG, etag);
285+
}
286+
if(response.getHeader(HEADER_LAST_MODIFIED) == null) {
287+
response.setHeader(HEADER_LAST_MODIFIED, formatDate(lastModifiedTimestamp));
288+
}
295289
}
296290
}
297291
}

‎spring-web/src/test/java/org/springframework/web/context/request/ServletWebRequestHttpMethodsTests.java

+2-2
Original file line numberDiff line numberDiff line change
@@ -93,8 +93,8 @@ public void checkNotModifiedHeaderAlreadySet() {
9393
servletRequest.addHeader("If-Modified-Since", epochTime);
9494
servletResponse.addHeader("Last-Modified", CURRENT_TIME);
9595

96-
assertFalse(request.checkNotModified(epochTime));
97-
assertEquals(200, servletResponse.getStatus());
96+
assertTrue(request.checkNotModified(epochTime));
97+
assertEquals(304, servletResponse.getStatus());
9898
assertEquals(1, servletResponse.getHeaders("Last-Modified").size());
9999
assertEquals(CURRENT_TIME, servletResponse.getHeader("Last-Modified"));
100100
}

‎spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/HttpEntityMethodProcessor.java

+43-22
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@
2727
import org.springframework.http.HttpEntity;
2828
import org.springframework.http.HttpHeaders;
2929
import org.springframework.http.HttpMethod;
30+
import org.springframework.http.HttpStatus;
3031
import org.springframework.http.RequestEntity;
3132
import org.springframework.http.ResponseEntity;
3233
import org.springframework.http.converter.HttpMessageConverter;
@@ -163,32 +164,22 @@ public void handleReturnValue(Object returnValue, MethodParameter returnType,
163164

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

170168
HttpHeaders entityHeaders = responseEntity.getHeaders();
171-
169+
if (!entityHeaders.isEmpty()) {
170+
outputMessage.getHeaders().putAll(entityHeaders);
171+
}
172172
Object body = responseEntity.getBody();
173173
if (responseEntity instanceof ResponseEntity) {
174-
for (String headerName : entityHeaders.keySet()) {
175-
if(!HttpHeaders.LAST_MODIFIED.equals(headerName)
176-
&& !HttpHeaders.ETAG.equals(headerName)) {
177-
outputMessage.getHeaders().put(headerName, entityHeaders.get(headerName));
178-
}
179-
}
180-
if (isResourceNotModified(webRequest, (ResponseEntity<?>) responseEntity)) {
174+
outputMessage.setStatusCode(((ResponseEntity<?>) responseEntity).getStatusCode());
175+
if (isResourceNotModified(inputMessage, outputMessage)) {
176+
outputMessage.setStatusCode(HttpStatus.NOT_MODIFIED);
181177
// Ensure headers are flushed, no body should be written.
182178
outputMessage.flush();
183179
// Skip call to converters, as they may update the body.
184180
return;
185181
}
186182
}
187-
else {
188-
if (!entityHeaders.isEmpty()) {
189-
outputMessage.getHeaders().putAll(entityHeaders);
190-
}
191-
}
192183

193184
// Try even with null body. ResponseBodyAdvice could get involved.
194185
writeWithMessageConverters(body, returnType, inputMessage, outputMessage);
@@ -197,22 +188,52 @@ public void handleReturnValue(Object returnValue, MethodParameter returnType,
197188
outputMessage.flush();
198189
}
199190

200-
private boolean isResourceNotModified(NativeWebRequest webRequest, ResponseEntity<?> responseEntity) {
201-
String eTag = responseEntity.getHeaders().getETag();
202-
long lastModified = responseEntity.getHeaders().getLastModified();
191+
private boolean isResourceNotModified(ServletServerHttpRequest inputMessage, ServletServerHttpResponse outputMessage) {
192+
193+
List<String> ifNoneMatch = inputMessage.getHeaders().getIfNoneMatch();
194+
long ifModifiedSince = inputMessage.getHeaders().getIfModifiedSince();
195+
String eTag = addEtagPadding(outputMessage.getHeaders().getETag());
196+
long lastModified = outputMessage.getHeaders().getLastModified();
203197
boolean notModified = false;
198+
204199
if (lastModified != -1 && StringUtils.hasLength(eTag)) {
205-
notModified = webRequest.checkNotModified(eTag, lastModified);
200+
notModified = isETagNotModified(ifNoneMatch, eTag) && isTimeStampNotModified(ifModifiedSince, lastModified);
206201
}
207202
else if (lastModified != -1) {
208-
notModified = webRequest.checkNotModified(lastModified);
203+
notModified = isTimeStampNotModified(ifModifiedSince, lastModified);
209204
}
210205
else if (StringUtils.hasLength(eTag)) {
211-
notModified = webRequest.checkNotModified(eTag);
206+
notModified = isETagNotModified(ifNoneMatch, eTag);
212207
}
213208
return notModified;
214209
}
215210

211+
private boolean isETagNotModified(List<String> ifNoneMatch, String etag) {
212+
if (StringUtils.hasLength(etag)) {
213+
for (String clientETag : ifNoneMatch) {
214+
// compare weak/strong ETags as per https://tools.ietf.org/html/rfc7232#section-2.3
215+
if (StringUtils.hasLength(clientETag) &&
216+
(clientETag.replaceFirst("^W/", "").equals(etag.replaceFirst("^W/", ""))
217+
|| clientETag.equals("*"))) {
218+
return true;
219+
}
220+
}
221+
}
222+
return false;
223+
}
224+
225+
private boolean isTimeStampNotModified(long ifModifiedSince, long lastModifiedTimestamp) {
226+
return (ifModifiedSince >= (lastModifiedTimestamp / 1000 * 1000));
227+
}
228+
229+
private String addEtagPadding(String etag) {
230+
if (StringUtils.hasLength(etag) &&
231+
(!(etag.startsWith("\"") || etag.startsWith("W/\"")) || !etag.endsWith("\"")) ) {
232+
etag = "\"" + etag + "\"";
233+
}
234+
return etag;
235+
}
236+
216237
@Override
217238
protected Class<?> getReturnValueType(Object returnValue, MethodParameter returnType) {
218239
if (returnValue != null) {

‎spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/HttpEntityMethodProcessorMockTests.java

+28-3
Original file line numberDiff line numberDiff line change
@@ -336,7 +336,7 @@ public void responseHeaderAndBody() throws Exception {
336336
public void handleReturnTypeLastModified() throws Exception {
337337
long currentTime = new Date().getTime();
338338
long oneMinuteAgo = currentTime - (1000 * 60);
339-
servletRequest.addHeader(HttpHeaders.IF_MODIFIED_SINCE, currentTime);
339+
servletRequest.addHeader(HttpHeaders.IF_MODIFIED_SINCE, dateFormat.format(currentTime));
340340
HttpHeaders responseHeaders = new HttpHeaders();
341341
responseHeaders.setDate(HttpHeaders.LAST_MODIFIED, oneMinuteAgo);
342342
ResponseEntity<String> returnValue = new ResponseEntity<String>("body", responseHeaders, HttpStatus.OK);
@@ -380,7 +380,7 @@ public void handleReturnTypeETagAndLastModified() throws Exception {
380380
long currentTime = new Date().getTime();
381381
long oneMinuteAgo = currentTime - (1000 * 60);
382382
String etagValue = "\"deadb33f8badf00d\"";
383-
servletRequest.addHeader(HttpHeaders.IF_MODIFIED_SINCE, currentTime);
383+
servletRequest.addHeader(HttpHeaders.IF_MODIFIED_SINCE, dateFormat.format(currentTime));
384384
servletRequest.addHeader(HttpHeaders.IF_NONE_MATCH, etagValue);
385385
HttpHeaders responseHeaders = new HttpHeaders();
386386
responseHeaders.setDate(HttpHeaders.LAST_MODIFIED, oneMinuteAgo);
@@ -402,13 +402,38 @@ public void handleReturnTypeETagAndLastModified() throws Exception {
402402
assertEquals(0, servletResponse.getContentAsByteArray().length);
403403
}
404404

405+
@Test
406+
public void handleReturnTypeNotModified() throws Exception {
407+
long currentTime = new Date().getTime();
408+
long oneMinuteAgo = currentTime - (1000 * 60);
409+
String etagValue = "\"deadb33f8badf00d\"";
410+
HttpHeaders responseHeaders = new HttpHeaders();
411+
responseHeaders.setDate(HttpHeaders.LAST_MODIFIED, oneMinuteAgo);
412+
responseHeaders.set(HttpHeaders.ETAG, etagValue);
413+
ResponseEntity<String> returnValue = new ResponseEntity<String>("body", responseHeaders, HttpStatus.NOT_MODIFIED);
414+
415+
given(messageConverter.canWrite(String.class, null)).willReturn(true);
416+
given(messageConverter.getSupportedMediaTypes()).willReturn(Collections.singletonList(MediaType.TEXT_PLAIN));
417+
given(messageConverter.canWrite(String.class, MediaType.TEXT_PLAIN)).willReturn(true);
418+
419+
processor.handleReturnValue(returnValue, returnTypeResponseEntity, mavContainer, webRequest);
420+
421+
assertTrue(mavContainer.isRequestHandled());
422+
assertEquals(HttpStatus.NOT_MODIFIED.value(), servletResponse.getStatus());
423+
assertEquals(1, servletResponse.getHeaderValues(HttpHeaders.LAST_MODIFIED).size());
424+
assertEquals(dateFormat.format(oneMinuteAgo), servletResponse.getHeader(HttpHeaders.LAST_MODIFIED));
425+
assertEquals(1, servletResponse.getHeaderValues(HttpHeaders.ETAG).size());
426+
assertEquals(etagValue, servletResponse.getHeader(HttpHeaders.ETAG));
427+
assertEquals(0, servletResponse.getContentAsByteArray().length);
428+
}
429+
405430
@Test
406431
public void handleReturnTypeChangedETagAndLastModified() throws Exception {
407432
long currentTime = new Date().getTime();
408433
long oneMinuteAgo = currentTime - (1000 * 60);
409434
String etagValue = "\"deadb33f8badf00d\"";
410435
String changedEtagValue = "\"changed-etag-value\"";
411-
servletRequest.addHeader(HttpHeaders.IF_MODIFIED_SINCE, currentTime);
436+
servletRequest.addHeader(HttpHeaders.IF_MODIFIED_SINCE, dateFormat.format(currentTime));
412437
servletRequest.addHeader(HttpHeaders.IF_NONE_MATCH, etagValue);
413438
HttpHeaders responseHeaders = new HttpHeaders();
414439
responseHeaders.setDate(HttpHeaders.LAST_MODIFIED, oneMinuteAgo);

0 commit comments

Comments
 (0)