From cadc58c4d8aa7b181fc3c22cec5f41a6aeb3214e Mon Sep 17 00:00:00 2001 From: divcon Date: Thu, 10 Nov 2022 22:33:01 +0900 Subject: [PATCH 1/4] remove return statements of ServletWebRequest.validateIfUnmodifiedSince and DefaultServerWebExchange.validateIfUnmodifiedSince The return values of ServletWebRequest.validateIfUnmodifiedSince and DefaultServerWebExchange.validateIfUnmodifiedSince don't seem to be in used. So I think that it is better to remove return statement. --- .../web/context/request/ServletWebRequest.java | 7 +++---- .../web/server/adapter/DefaultServerWebExchange.java | 7 +++---- 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/spring-web/src/main/java/org/springframework/web/context/request/ServletWebRequest.java b/spring-web/src/main/java/org/springframework/web/context/request/ServletWebRequest.java index 0e070c3c806c..ab69349fb2d1 100644 --- a/spring-web/src/main/java/org/springframework/web/context/request/ServletWebRequest.java +++ b/spring-web/src/main/java/org/springframework/web/context/request/ServletWebRequest.java @@ -330,17 +330,16 @@ private boolean validateIfUnmodifiedSince(long lastModifiedTimestamp) { return true; } - private boolean validateIfModifiedSince(long lastModifiedTimestamp) { + private void validateIfModifiedSince(long lastModifiedTimestamp) { if (lastModifiedTimestamp < 0) { - return false; + return; } long ifModifiedSince = parseDateHeader(HttpHeaders.IF_MODIFIED_SINCE); if (ifModifiedSince == -1) { - return false; + return; } // We will perform this validation... this.notModified = ifModifiedSince >= (lastModifiedTimestamp / 1000 * 1000); - return true; } private void updateResponseIdempotent(String eTag, long lastModifiedTimestamp) { diff --git a/spring-web/src/main/java/org/springframework/web/server/adapter/DefaultServerWebExchange.java b/spring-web/src/main/java/org/springframework/web/server/adapter/DefaultServerWebExchange.java index 06b949f955e8..ef813bbdd236 100644 --- a/spring-web/src/main/java/org/springframework/web/server/adapter/DefaultServerWebExchange.java +++ b/spring-web/src/main/java/org/springframework/web/server/adapter/DefaultServerWebExchange.java @@ -401,17 +401,16 @@ private boolean validateIfUnmodifiedSince(Instant lastModified) { return true; } - private boolean validateIfModifiedSince(Instant lastModified) { + private void validateIfModifiedSince(Instant lastModified) { if (lastModified.isBefore(Instant.EPOCH)) { - return false; + return; } long ifModifiedSince = getRequestHeaders().getIfModifiedSince(); if (ifModifiedSince == -1) { - return false; + return; } // We will perform this validation... this.notModified = ChronoUnit.SECONDS.between(lastModified, Instant.ofEpochMilli(ifModifiedSince)) >= 0; - return true; } @Override From 8c50fc3a7bc606dcbef4967438c8d524593121b8 Mon Sep 17 00:00:00 2001 From: divcon Date: Thu, 10 Nov 2022 22:35:40 +0900 Subject: [PATCH 2/4] Add @Nullable annotation. --- .../web/context/request/ServletWebRequest.java | 6 +++--- .../web/server/adapter/DefaultServerWebExchange.java | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/spring-web/src/main/java/org/springframework/web/context/request/ServletWebRequest.java b/spring-web/src/main/java/org/springframework/web/context/request/ServletWebRequest.java index ab69349fb2d1..607a189a2492 100644 --- a/spring-web/src/main/java/org/springframework/web/context/request/ServletWebRequest.java +++ b/spring-web/src/main/java/org/springframework/web/context/request/ServletWebRequest.java @@ -309,7 +309,7 @@ private boolean eTagWeakMatch(@Nullable String first, @Nullable String second) { return first.equals(second); } - private void updateResponseStateChanging(String eTag, long lastModifiedTimestamp) { + private void updateResponseStateChanging(@Nullable String eTag, long lastModifiedTimestamp) { if (this.notModified && getResponse() != null) { getResponse().setStatus(HttpStatus.PRECONDITION_FAILED.value()); } @@ -342,7 +342,7 @@ private void validateIfModifiedSince(long lastModifiedTimestamp) { this.notModified = ifModifiedSince >= (lastModifiedTimestamp / 1000 * 1000); } - private void updateResponseIdempotent(String eTag, long lastModifiedTimestamp) { + private void updateResponseIdempotent(@Nullable String eTag, long lastModifiedTimestamp) { if (getResponse() != null) { boolean isHttpGetOrHead = SAFE_METHODS.contains(getRequest().getMethod()); if (this.notModified) { @@ -353,7 +353,7 @@ private void updateResponseIdempotent(String eTag, long lastModifiedTimestamp) { } } - private void addCachingResponseHeaders(String eTag, long lastModifiedTimestamp) { + private void addCachingResponseHeaders(@Nullable String eTag, long lastModifiedTimestamp) { if (SAFE_METHODS.contains(getRequest().getMethod())) { if (lastModifiedTimestamp > 0 && parseDateValue(getResponse().getHeader(HttpHeaders.LAST_MODIFIED)) == -1) { getResponse().setDateHeader(HttpHeaders.LAST_MODIFIED, lastModifiedTimestamp); diff --git a/spring-web/src/main/java/org/springframework/web/server/adapter/DefaultServerWebExchange.java b/spring-web/src/main/java/org/springframework/web/server/adapter/DefaultServerWebExchange.java index ef813bbdd236..277382918727 100644 --- a/spring-web/src/main/java/org/springframework/web/server/adapter/DefaultServerWebExchange.java +++ b/spring-web/src/main/java/org/springframework/web/server/adapter/DefaultServerWebExchange.java @@ -346,7 +346,7 @@ private boolean eTagWeakMatch(@Nullable String first, @Nullable String second) { return first.equals(second); } - private void updateResponseStateChanging(String eTag, Instant lastModified) { + private void updateResponseStateChanging(@Nullable String eTag, Instant lastModified) { if (this.notModified) { getResponse().setStatusCode(HttpStatus.PRECONDITION_FAILED); } From ebf4097ef28330d5447c9669a54ffd973e6f0f52 Mon Sep 17 00:00:00 2001 From: divcon Date: Thu, 10 Nov 2022 23:38:40 +0900 Subject: [PATCH 3/4] Simplify if statements --- .../context/request/ServletWebRequest.java | 19 ++++------------- .../adapter/DefaultServerWebExchange.java | 21 ++++++------------- 2 files changed, 10 insertions(+), 30 deletions(-) diff --git a/spring-web/src/main/java/org/springframework/web/context/request/ServletWebRequest.java b/spring-web/src/main/java/org/springframework/web/context/request/ServletWebRequest.java index 607a189a2492..0ec0cc985a63 100644 --- a/spring-web/src/main/java/org/springframework/web/context/request/ServletWebRequest.java +++ b/spring-web/src/main/java/org/springframework/web/context/request/ServletWebRequest.java @@ -233,10 +233,7 @@ else if (validateIfUnmodifiedSince(lastModifiedTimestamp)) { private boolean validateIfMatch(@Nullable String eTag) { Enumeration ifMatchHeaders = getRequest().getHeaders(HttpHeaders.IF_MATCH); - if (SAFE_METHODS.contains(getRequest().getMethod())) { - return false; - } - if (!ifMatchHeaders.hasMoreElements()) { + if (SAFE_METHODS.contains(getRequest().getMethod()) || !ifMatchHeaders.hasMoreElements()) { return false; } this.notModified = matchRequestedETags(ifMatchHeaders, eTag, false); @@ -319,11 +316,8 @@ private void updateResponseStateChanging(@Nullable String eTag, long lastModifie } private boolean validateIfUnmodifiedSince(long lastModifiedTimestamp) { - if (lastModifiedTimestamp < 0) { - return false; - } long ifUnmodifiedSince = parseDateHeader(HttpHeaders.IF_UNMODIFIED_SINCE); - if (ifUnmodifiedSince == -1) { + if (lastModifiedTimestamp < 0 || ifUnmodifiedSince == -1) { return false; } this.notModified = (ifUnmodifiedSince < (lastModifiedTimestamp / 1000 * 1000)); @@ -331,15 +325,10 @@ private boolean validateIfUnmodifiedSince(long lastModifiedTimestamp) { } private void validateIfModifiedSince(long lastModifiedTimestamp) { - if (lastModifiedTimestamp < 0) { - return; - } long ifModifiedSince = parseDateHeader(HttpHeaders.IF_MODIFIED_SINCE); - if (ifModifiedSince == -1) { - return; + if (lastModifiedTimestamp >= 0 && ifModifiedSince != -1) { + this.notModified = ifModifiedSince >= (lastModifiedTimestamp / 1000 * 1000); } - // We will perform this validation... - this.notModified = ifModifiedSince >= (lastModifiedTimestamp / 1000 * 1000); } private void updateResponseIdempotent(@Nullable String eTag, long lastModifiedTimestamp) { diff --git a/spring-web/src/main/java/org/springframework/web/server/adapter/DefaultServerWebExchange.java b/spring-web/src/main/java/org/springframework/web/server/adapter/DefaultServerWebExchange.java index 277382918727..5e7aee7a9ba1 100644 --- a/spring-web/src/main/java/org/springframework/web/server/adapter/DefaultServerWebExchange.java +++ b/spring-web/src/main/java/org/springframework/web/server/adapter/DefaultServerWebExchange.java @@ -278,10 +278,8 @@ else if (validateIfUnmodifiedSince(lastModified)) { private boolean validateIfMatch(@Nullable String eTag) { try { - if (SAFE_METHODS.contains(getRequest().getMethod())) { - return false; - } - if (CollectionUtils.isEmpty(getRequest().getHeaders().get(HttpHeaders.IF_MATCH))) { + if (SAFE_METHODS.contains(getRequest().getMethod()) + || CollectionUtils.isEmpty(getRequest().getHeaders().get(HttpHeaders.IF_MATCH))) { return false; } this.notModified = matchRequestedETags(getRequestHeaders().getIfMatch(), eTag, false); @@ -389,11 +387,8 @@ private void addCachingResponseHeaders(@Nullable String eTag, Instant lastModifi } private boolean validateIfUnmodifiedSince(Instant lastModified) { - if (lastModified.isBefore(Instant.EPOCH)) { - return false; - } long ifUnmodifiedSince = getRequestHeaders().getIfUnmodifiedSince(); - if (ifUnmodifiedSince == -1) { + if (lastModified.isBefore(Instant.EPOCH) || ifUnmodifiedSince == -1) { return false; } Instant sinceInstant = Instant.ofEpochMilli(ifUnmodifiedSince); @@ -402,15 +397,11 @@ private boolean validateIfUnmodifiedSince(Instant lastModified) { } private void validateIfModifiedSince(Instant lastModified) { - if (lastModified.isBefore(Instant.EPOCH)) { - return; - } long ifModifiedSince = getRequestHeaders().getIfModifiedSince(); - if (ifModifiedSince == -1) { - return; + if (!(lastModified.isBefore(Instant.EPOCH) || ifModifiedSince == -1)) { + // We will perform this validation... + this.notModified = ChronoUnit.SECONDS.between(lastModified, Instant.ofEpochMilli(ifModifiedSince)) >= 0; } - // We will perform this validation... - this.notModified = ChronoUnit.SECONDS.between(lastModified, Instant.ofEpochMilli(ifModifiedSince)) >= 0; } @Override From 974b5d0694c4e7728633aa4390ecef654709aea6 Mon Sep 17 00:00:00 2001 From: divcon Date: Tue, 22 Nov 2022 15:02:42 +0900 Subject: [PATCH 4/4] remove unnecessary parsing of the date header --- .../web/context/request/ServletWebRequest.java | 11 +++++++++-- .../server/adapter/DefaultServerWebExchange.java | 16 ++++++++++++---- 2 files changed, 21 insertions(+), 6 deletions(-) diff --git a/spring-web/src/main/java/org/springframework/web/context/request/ServletWebRequest.java b/spring-web/src/main/java/org/springframework/web/context/request/ServletWebRequest.java index 0ec0cc985a63..13ad367026d1 100644 --- a/spring-web/src/main/java/org/springframework/web/context/request/ServletWebRequest.java +++ b/spring-web/src/main/java/org/springframework/web/context/request/ServletWebRequest.java @@ -316,8 +316,11 @@ private void updateResponseStateChanging(@Nullable String eTag, long lastModifie } private boolean validateIfUnmodifiedSince(long lastModifiedTimestamp) { + if (lastModifiedTimestamp < 0) { + return false; + } long ifUnmodifiedSince = parseDateHeader(HttpHeaders.IF_UNMODIFIED_SINCE); - if (lastModifiedTimestamp < 0 || ifUnmodifiedSince == -1) { + if (ifUnmodifiedSince == -1) { return false; } this.notModified = (ifUnmodifiedSince < (lastModifiedTimestamp / 1000 * 1000)); @@ -325,8 +328,12 @@ private boolean validateIfUnmodifiedSince(long lastModifiedTimestamp) { } private void validateIfModifiedSince(long lastModifiedTimestamp) { + if (lastModifiedTimestamp < 0) { + return; + } long ifModifiedSince = parseDateHeader(HttpHeaders.IF_MODIFIED_SINCE); - if (lastModifiedTimestamp >= 0 && ifModifiedSince != -1) { + if (ifModifiedSince != -1) { + // We will perform this validation... this.notModified = ifModifiedSince >= (lastModifiedTimestamp / 1000 * 1000); } } diff --git a/spring-web/src/main/java/org/springframework/web/server/adapter/DefaultServerWebExchange.java b/spring-web/src/main/java/org/springframework/web/server/adapter/DefaultServerWebExchange.java index 5e7aee7a9ba1..43b7462fb91f 100644 --- a/spring-web/src/main/java/org/springframework/web/server/adapter/DefaultServerWebExchange.java +++ b/spring-web/src/main/java/org/springframework/web/server/adapter/DefaultServerWebExchange.java @@ -278,8 +278,10 @@ else if (validateIfUnmodifiedSince(lastModified)) { private boolean validateIfMatch(@Nullable String eTag) { try { - if (SAFE_METHODS.contains(getRequest().getMethod()) - || CollectionUtils.isEmpty(getRequest().getHeaders().get(HttpHeaders.IF_MATCH))) { + if (SAFE_METHODS.contains(getRequest().getMethod())) { + return false; + } + if (CollectionUtils.isEmpty(getRequest().getHeaders().get(HttpHeaders.IF_MATCH))) { return false; } this.notModified = matchRequestedETags(getRequestHeaders().getIfMatch(), eTag, false); @@ -387,8 +389,11 @@ private void addCachingResponseHeaders(@Nullable String eTag, Instant lastModifi } private boolean validateIfUnmodifiedSince(Instant lastModified) { + if (lastModified.isBefore(Instant.EPOCH)) { + return false; + } long ifUnmodifiedSince = getRequestHeaders().getIfUnmodifiedSince(); - if (lastModified.isBefore(Instant.EPOCH) || ifUnmodifiedSince == -1) { + if (ifUnmodifiedSince == -1) { return false; } Instant sinceInstant = Instant.ofEpochMilli(ifUnmodifiedSince); @@ -397,8 +402,11 @@ private boolean validateIfUnmodifiedSince(Instant lastModified) { } private void validateIfModifiedSince(Instant lastModified) { + if (lastModified.isBefore(Instant.EPOCH)) { + return; + } long ifModifiedSince = getRequestHeaders().getIfModifiedSince(); - if (!(lastModified.isBefore(Instant.EPOCH) || ifModifiedSince == -1)) { + if (ifModifiedSince != -1) { // We will perform this validation... this.notModified = ChronoUnit.SECONDS.between(lastModified, Instant.ofEpochMilli(ifModifiedSince)) >= 0; }