From c53c8bfc5a7ff963e1762edfcbb6425fc5088035 Mon Sep 17 00:00:00 2001 From: Arjen Poutsma Date: Thu, 18 Jan 2018 11:26:16 +0100 Subject: [PATCH] Set 304 status on ServerResponse when ETag/LastModified match This commit checks the Etag/LastModified headers on the incoming request, and sets a 304 Not Modified status with no body when they match, by delegating to ServerWebExchange.checkNotModified. Issue: SPR-16348 --- .../server/DefaultEntityResponseBuilder.java | 6 +- .../DefaultRenderingResponseBuilder.java | 5 +- .../server/DefaultServerResponseBuilder.java | 31 +++++-- .../DefaultEntityResponseBuilderTests.java | 53 ++++++++++- .../server/DefaultRenderingResponseTests.java | 57 +++++++++++- .../DefaultServerResponseBuilderTests.java | 87 ++++++++++++++++--- 6 files changed, 208 insertions(+), 31 deletions(-) diff --git a/spring-webflux/src/main/java/org/springframework/web/reactive/function/server/DefaultEntityResponseBuilder.java b/spring-webflux/src/main/java/org/springframework/web/reactive/function/server/DefaultEntityResponseBuilder.java index 65b22bc3ee..31d74a5079 100644 --- a/spring-webflux/src/main/java/org/springframework/web/reactive/function/server/DefaultEntityResponseBuilder.java +++ b/spring-webflux/src/main/java/org/springframework/web/reactive/function/server/DefaultEntityResponseBuilder.java @@ -224,10 +224,8 @@ class DefaultEntityResponseBuilder implements EntityResponse.Builder { } @Override - public Mono writeTo(ServerWebExchange exchange, Context context) { - ServerHttpResponse response = exchange.getResponse(); - writeStatusAndHeaders(response); - return inserter().insert(response, new BodyInserter.Context() { + protected Mono writeToInternal(ServerWebExchange exchange, Context context) { + return inserter().insert(exchange.getResponse(), new BodyInserter.Context() { @Override public List> messageWriters() { return context.messageWriters(); diff --git a/spring-webflux/src/main/java/org/springframework/web/reactive/function/server/DefaultRenderingResponseBuilder.java b/spring-webflux/src/main/java/org/springframework/web/reactive/function/server/DefaultRenderingResponseBuilder.java index fb2618feac..a91fee78fd 100644 --- a/spring-webflux/src/main/java/org/springframework/web/reactive/function/server/DefaultRenderingResponseBuilder.java +++ b/spring-webflux/src/main/java/org/springframework/web/reactive/function/server/DefaultRenderingResponseBuilder.java @@ -35,7 +35,6 @@ import org.springframework.http.HttpHeaders; import org.springframework.http.HttpStatus; import org.springframework.http.MediaType; import org.springframework.http.ResponseCookie; -import org.springframework.http.server.reactive.ServerHttpResponse; import org.springframework.lang.Nullable; import org.springframework.util.Assert; import org.springframework.util.LinkedMultiValueMap; @@ -184,9 +183,7 @@ class DefaultRenderingResponseBuilder implements RenderingResponse.Builder { } @Override - public Mono writeTo(ServerWebExchange exchange, Context context) { - ServerHttpResponse response = exchange.getResponse(); - writeStatusAndHeaders(response); + protected Mono writeToInternal(ServerWebExchange exchange, Context context) { MediaType responseContentType = exchange.getResponse().getHeaders().getContentType(); Locale locale = LocaleContextHolder.getLocale(exchange.getLocaleContext()); Stream viewResolverStream = context.viewResolvers().stream(); diff --git a/spring-webflux/src/main/java/org/springframework/web/reactive/function/server/DefaultServerResponseBuilder.java b/spring-webflux/src/main/java/org/springframework/web/reactive/function/server/DefaultServerResponseBuilder.java index 0382ddb93c..a32acabb30 100644 --- a/spring-webflux/src/main/java/org/springframework/web/reactive/function/server/DefaultServerResponseBuilder.java +++ b/spring-webflux/src/main/java/org/springframework/web/reactive/function/server/DefaultServerResponseBuilder.java @@ -17,8 +17,10 @@ package org.springframework.web.reactive.function.server; import java.net.URI; +import java.time.Instant; import java.time.ZonedDateTime; import java.util.Arrays; +import java.util.EnumSet; import java.util.HashMap; import java.util.LinkedHashSet; import java.util.List; @@ -278,6 +280,8 @@ class DefaultServerResponseBuilder implements ServerResponse.BodyBuilder { static abstract class AbstractServerResponse implements ServerResponse { + private static final Set SAFE_METHODS = EnumSet.of(HttpMethod.GET, HttpMethod.HEAD); + final int statusCode; private final HttpHeaders headers; @@ -319,7 +323,21 @@ class DefaultServerResponseBuilder implements ServerResponse.BodyBuilder { return this.cookies; } - protected void writeStatusAndHeaders(ServerHttpResponse response) { + @Override + public final Mono writeTo(ServerWebExchange exchange, Context context) { + writeStatusAndHeaders(exchange.getResponse()); + + Instant lastModified = Instant.ofEpochMilli(headers().getLastModified()); + HttpMethod httpMethod = exchange.getRequest().getMethod(); + if (SAFE_METHODS.contains(httpMethod) && exchange.checkNotModified(headers().getETag(), lastModified)) { + return exchange.getResponse().setComplete(); + } + else { + return writeToInternal(exchange, context); + } + } + + private void writeStatusAndHeaders(ServerHttpResponse response) { if (response instanceof AbstractServerHttpResponse) { ((AbstractServerHttpResponse) response).setStatusCodeValue(this.statusCode); } @@ -335,6 +353,8 @@ class DefaultServerResponseBuilder implements ServerResponse.BodyBuilder { copy(this.cookies, response.getCookies()); } + protected abstract Mono writeToInternal(ServerWebExchange exchange, Context context); + private static void copy(MultiValueMap src, MultiValueMap dst) { if (!src.isEmpty()) { src.entrySet().stream() @@ -358,8 +378,7 @@ class DefaultServerResponseBuilder implements ServerResponse.BodyBuilder { } @Override - public Mono writeTo(ServerWebExchange exchange, Context context) { - writeStatusAndHeaders(exchange.getResponse()); + protected Mono writeToInternal(ServerWebExchange exchange, Context context) { return this.writeFunction.apply(exchange, context); } } @@ -381,10 +400,8 @@ class DefaultServerResponseBuilder implements ServerResponse.BodyBuilder { } @Override - public Mono writeTo(ServerWebExchange exchange, Context context) { - ServerHttpResponse response = exchange.getResponse(); - writeStatusAndHeaders(response); - return this.inserter.insert(response, new BodyInserter.Context() { + protected Mono writeToInternal(ServerWebExchange exchange, Context context) { + return this.inserter.insert(exchange.getResponse(), new BodyInserter.Context() { @Override public List> messageWriters() { return context.messageWriters(); diff --git a/spring-webflux/src/test/java/org/springframework/web/reactive/function/server/DefaultEntityResponseBuilderTests.java b/spring-webflux/src/test/java/org/springframework/web/reactive/function/server/DefaultEntityResponseBuilderTests.java index e889329fdf..d3e5715006 100644 --- a/spring-webflux/src/test/java/org/springframework/web/reactive/function/server/DefaultEntityResponseBuilderTests.java +++ b/spring-webflux/src/test/java/org/springframework/web/reactive/function/server/DefaultEntityResponseBuilderTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2017 the original author or authors. + * Copyright 2002-2018 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -18,6 +18,8 @@ package org.springframework.web.reactive.function.server; import java.nio.ByteBuffer; import java.time.ZonedDateTime; +import java.time.format.DateTimeFormatter; +import java.time.temporal.ChronoUnit; import java.util.Collections; import java.util.EnumSet; import java.util.List; @@ -44,6 +46,7 @@ import org.springframework.http.codec.EncoderHttpMessageWriter; import org.springframework.http.codec.HttpMessageWriter; import org.springframework.http.server.reactive.ServerHttpResponse; import org.springframework.mock.http.server.reactive.test.MockServerHttpRequest; +import org.springframework.mock.http.server.reactive.test.MockServerHttpResponse; import org.springframework.mock.web.test.server.MockServerWebExchange; import org.springframework.util.LinkedMultiValueMap; import org.springframework.util.MultiValueMap; @@ -232,4 +235,52 @@ public class DefaultEntityResponseBuilderTests { assertNotNull(exchange.getResponse().getBody()); } + @Test + public void notModifiedEtag() { + String etag = "\"foo\""; + EntityResponse responseMono = EntityResponse.fromObject("bar") + .eTag(etag) + .build() + .block(); + + MockServerHttpRequest request = MockServerHttpRequest.get("http://example.com") + .header(HttpHeaders.IF_NONE_MATCH, etag) + .build(); + MockServerWebExchange exchange = MockServerWebExchange.from(request); + + responseMono.writeTo(exchange, DefaultServerResponseBuilderTests.EMPTY_CONTEXT); + + MockServerHttpResponse response = exchange.getResponse(); + assertEquals(HttpStatus.NOT_MODIFIED, response.getStatusCode()); + StepVerifier.create(response.getBody()) + .expectError(IllegalStateException.class) + .verify(); + } + + + @Test + public void notModifiedLastModified() { + ZonedDateTime now = ZonedDateTime.now(); + ZonedDateTime oneMinuteBeforeNow = now.minus(1, ChronoUnit.MINUTES); + + EntityResponse responseMono = EntityResponse.fromObject("bar") + .lastModified(oneMinuteBeforeNow) + .build() + .block(); + + MockServerHttpRequest request = MockServerHttpRequest.get("http://example.com") + .header(HttpHeaders.IF_MODIFIED_SINCE, + DateTimeFormatter.RFC_1123_DATE_TIME.format(now)) + .build(); + MockServerWebExchange exchange = MockServerWebExchange.from(request); + + responseMono.writeTo(exchange, DefaultServerResponseBuilderTests.EMPTY_CONTEXT); + + MockServerHttpResponse response = exchange.getResponse(); + assertEquals(HttpStatus.NOT_MODIFIED, response.getStatusCode()); + StepVerifier.create(response.getBody()) + .expectError(IllegalStateException.class) + .verify(); + } + } diff --git a/spring-webflux/src/test/java/org/springframework/web/reactive/function/server/DefaultRenderingResponseTests.java b/spring-webflux/src/test/java/org/springframework/web/reactive/function/server/DefaultRenderingResponseTests.java index b664a8b4b8..1ffb88dd67 100644 --- a/spring-webflux/src/test/java/org/springframework/web/reactive/function/server/DefaultRenderingResponseTests.java +++ b/spring-webflux/src/test/java/org/springframework/web/reactive/function/server/DefaultRenderingResponseTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2017 the original author or authors. + * Copyright 2002-2018 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -16,6 +16,9 @@ package org.springframework.web.reactive.function.server; +import java.time.ZonedDateTime; +import java.time.format.DateTimeFormatter; +import java.time.temporal.ChronoUnit; import java.util.ArrayList; import java.util.Collections; import java.util.List; @@ -28,9 +31,11 @@ import reactor.core.publisher.Mono; import reactor.test.StepVerifier; import org.springframework.http.HttpHeaders; +import org.springframework.http.HttpStatus; import org.springframework.http.MediaType; import org.springframework.http.ResponseCookie; import org.springframework.mock.http.server.reactive.test.MockServerHttpRequest; +import org.springframework.mock.http.server.reactive.test.MockServerHttpResponse; import org.springframework.mock.web.test.server.MockServerWebExchange; import org.springframework.util.LinkedMultiValueMap; import org.springframework.util.MultiValueMap; @@ -40,7 +45,7 @@ import org.springframework.web.reactive.result.view.ViewResolver; import org.springframework.web.reactive.result.view.ViewResolverSupport; import org.springframework.web.server.ServerWebExchange; -import static org.junit.Assert.assertEquals; +import static org.junit.Assert.*; import static org.mockito.Mockito.*; /** @@ -181,4 +186,52 @@ public class DefaultRenderingResponseTests { } + @Test + public void notModifiedEtag() { + String etag = "\"foo\""; + RenderingResponse responseMono = RenderingResponse.create("bar") + .header(HttpHeaders.ETAG, etag) + .build() + .block(); + + MockServerHttpRequest request = MockServerHttpRequest.get("http://example.com") + .header(HttpHeaders.IF_NONE_MATCH, etag) + .build(); + MockServerWebExchange exchange = MockServerWebExchange.from(request); + + responseMono.writeTo(exchange, DefaultServerResponseBuilderTests.EMPTY_CONTEXT); + + MockServerHttpResponse response = exchange.getResponse(); + assertEquals(HttpStatus.NOT_MODIFIED, response.getStatusCode()); + StepVerifier.create(response.getBody()) + .expectError(IllegalStateException.class) + .verify(); + } + + @Test + public void notModifiedLastModified() { + ZonedDateTime now = ZonedDateTime.now(); + ZonedDateTime oneMinuteBeforeNow = now.minus(1, ChronoUnit.MINUTES); + + RenderingResponse responseMono = RenderingResponse.create("bar") + .header(HttpHeaders.LAST_MODIFIED, DateTimeFormatter.RFC_1123_DATE_TIME.format(oneMinuteBeforeNow)) + .build() + .block(); + + MockServerHttpRequest request = MockServerHttpRequest.get("http://example.com") + .header(HttpHeaders.IF_MODIFIED_SINCE, + DateTimeFormatter.RFC_1123_DATE_TIME.format(now)) + .build(); + MockServerWebExchange exchange = MockServerWebExchange.from(request); + + responseMono.writeTo(exchange, DefaultServerResponseBuilderTests.EMPTY_CONTEXT); + + MockServerHttpResponse response = exchange.getResponse(); + assertEquals(HttpStatus.NOT_MODIFIED, response.getStatusCode()); + StepVerifier.create(response.getBody()) + .expectError(IllegalStateException.class) + .verify(); + } + + } diff --git a/spring-webflux/src/test/java/org/springframework/web/reactive/function/server/DefaultServerResponseBuilderTests.java b/spring-webflux/src/test/java/org/springframework/web/reactive/function/server/DefaultServerResponseBuilderTests.java index a116cb6a30..af2ea5986c 100644 --- a/spring-webflux/src/test/java/org/springframework/web/reactive/function/server/DefaultServerResponseBuilderTests.java +++ b/spring-webflux/src/test/java/org/springframework/web/reactive/function/server/DefaultServerResponseBuilderTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2017 the original author or authors. + * Copyright 2002-2018 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -18,6 +18,8 @@ package org.springframework.web.reactive.function.server; import java.net.URI; import java.time.ZonedDateTime; +import java.time.format.DateTimeFormatter; +import java.time.temporal.ChronoUnit; import java.util.Collections; import java.util.EnumSet; import java.util.List; @@ -33,19 +35,33 @@ import org.springframework.http.HttpMethod; import org.springframework.http.HttpStatus; import org.springframework.http.MediaType; import org.springframework.http.ResponseCookie; +import org.springframework.http.codec.HttpMessageWriter; +import org.springframework.mock.http.server.reactive.test.MockServerHttpRequest; import org.springframework.mock.http.server.reactive.test.MockServerHttpResponse; +import org.springframework.mock.web.test.server.MockServerWebExchange; import org.springframework.util.LinkedMultiValueMap; import org.springframework.util.MultiValueMap; -import org.springframework.web.server.ServerWebExchange; +import org.springframework.web.reactive.result.view.ViewResolver; import static org.junit.Assert.*; -import static org.mockito.Mockito.*; /** * @author Arjen Poutsma */ public class DefaultServerResponseBuilderTests { + static final ServerResponse.Context EMPTY_CONTEXT = new ServerResponse.Context() { + @Override + public List> messageWriters() { + return Collections.emptyList(); + } + + @Override + public List viewResolvers() { + return Collections.emptyList(); + } + }; + @Test public void from() throws Exception { ServerResponse other = ServerResponse.ok().header("foo", "bar").build().block(); @@ -287,13 +303,12 @@ public class DefaultServerResponseBuilderTests { .header("MyKey", "MyValue") .cookie(cookie).build(); - ServerWebExchange exchange = mock(ServerWebExchange.class); - MockServerHttpResponse response = new MockServerHttpResponse(); - when(exchange.getResponse()).thenReturn(response); - ServerResponse.Context context = mock(ServerResponse.Context.class); + MockServerHttpRequest request = MockServerHttpRequest.get("http://example.com").build(); + MockServerWebExchange exchange = MockServerWebExchange.from(request); - result.flatMap(res -> res.writeTo(exchange, context)).block(); + result.flatMap(res -> res.writeTo(exchange, EMPTY_CONTEXT)).block(); + MockServerHttpResponse response = exchange.getResponse(); assertEquals(HttpStatus.CREATED, response.getStatusCode()); assertEquals("MyValue", response.getHeaders().getFirst("MyKey")); assertEquals("value", response.getCookies().getFirst("name").getValue()); @@ -305,13 +320,12 @@ public class DefaultServerResponseBuilderTests { Mono mono = Mono.empty(); Mono result = ServerResponse.ok().build(mono); - ServerWebExchange exchange = mock(ServerWebExchange.class); - MockServerHttpResponse response = new MockServerHttpResponse(); - when(exchange.getResponse()).thenReturn(response); - ServerResponse.Context context = mock(ServerResponse.Context.class); + MockServerHttpRequest request = MockServerHttpRequest.get("http://example.com").build(); + MockServerWebExchange exchange = MockServerWebExchange.from(request); - result.flatMap(res -> res.writeTo(exchange, context)).block(); + result.flatMap(res -> res.writeTo(exchange, EMPTY_CONTEXT)).block(); + MockServerHttpResponse response = exchange.getResponse(); StepVerifier.create(response.getBody()).expectComplete().verify(); } @@ -322,5 +336,52 @@ public class DefaultServerResponseBuilderTests { ServerResponse.ok().syncBody(mono); } + @Test + public void notModifiedEtag() { + String etag = "\"foo\""; + ServerResponse responseMono = ServerResponse.ok() + .eTag(etag) + .syncBody("bar") + .block(); + + MockServerHttpRequest request = MockServerHttpRequest.get("http://example.com") + .header(HttpHeaders.IF_NONE_MATCH, etag) + .build(); + MockServerWebExchange exchange = MockServerWebExchange.from(request); + + responseMono.writeTo(exchange, EMPTY_CONTEXT); + + MockServerHttpResponse response = exchange.getResponse(); + assertEquals(HttpStatus.NOT_MODIFIED, response.getStatusCode()); + StepVerifier.create(response.getBody()) + .expectError(IllegalStateException.class) + .verify(); + } + + @Test + public void notModifiedLastModified() { + ZonedDateTime now = ZonedDateTime.now(); + ZonedDateTime oneMinuteBeforeNow = now.minus(1, ChronoUnit.MINUTES); + + ServerResponse responseMono = ServerResponse.ok() + .lastModified(oneMinuteBeforeNow) + .syncBody("bar") + .block(); + + MockServerHttpRequest request = MockServerHttpRequest.get("http://example.com") + .header(HttpHeaders.IF_MODIFIED_SINCE, + DateTimeFormatter.RFC_1123_DATE_TIME.format(now)) + .build(); + MockServerWebExchange exchange = MockServerWebExchange.from(request); + + responseMono.writeTo(exchange, EMPTY_CONTEXT); + + MockServerHttpResponse response = exchange.getResponse(); + assertEquals(HttpStatus.NOT_MODIFIED, response.getStatusCode()); + StepVerifier.create(response.getBody()) + .expectError(IllegalStateException.class) + .verify(); + } + } \ No newline at end of file