From 39786e479080a337691a3a7d0490547e37a900e3 Mon Sep 17 00:00:00 2001 From: James Yuzawa Date: Fri, 24 Feb 2023 08:01:26 -0500 Subject: [PATCH] Improve attribute handling in RequestPredicates This commit changes the way request attributes are handled in RequestPredicates. Previously, the AND/OR/NOT predicates copied all attributes in a map, and restored that when the delegate predicate(s) failed. Now, we only set the attributes when all delegates have succeeded. Closes gh-30028 --- .../function/server/RequestPredicates.java | 189 +++++++++++++----- .../RequestPredicateAttributesTests.java | 14 +- .../servlet/function/RequestPredicates.java | 189 +++++++++++++----- 3 files changed, 278 insertions(+), 114 deletions(-) diff --git a/spring-webflux/src/main/java/org/springframework/web/reactive/function/server/RequestPredicates.java b/spring-webflux/src/main/java/org/springframework/web/reactive/function/server/RequestPredicates.java index 284ea7e0d5..6bebef9db4 100644 --- a/spring-webflux/src/main/java/org/springframework/web/reactive/function/server/RequestPredicates.java +++ b/spring-webflux/src/main/java/org/springframework/web/reactive/function/server/RequestPredicates.java @@ -21,7 +21,6 @@ import java.net.URI; import java.security.Principal; import java.util.Arrays; import java.util.Collections; -import java.util.HashMap; import java.util.LinkedHashMap; import java.util.LinkedHashSet; import java.util.List; @@ -296,11 +295,6 @@ public abstract class RequestPredicates { } } - private static void restoreAttributes(ServerRequest request, Map attributes) { - request.attributes().clear(); - request.attributes().putAll(attributes); - } - private static Map mergePathVariables(Map oldVariables, Map newVariables) { @@ -432,6 +426,81 @@ public abstract class RequestPredicates { } + /** + * A boolean result with a state-changing commit step to be conditionally + * applied by a caller. + */ + static abstract class Evaluation { + static final Evaluation TRUE = new NopEvaluation(true); + static final Evaluation FALSE = new NopEvaluation(false); + + private final boolean value; + + Evaluation(boolean value) { + this.value = value; + } + + abstract void doCommit(); + + + private static final class NopEvaluation extends Evaluation { + private NopEvaluation(boolean value) { + super(value); + } + + @Override + void doCommit() { + // pass + } + } + } + + + /** + * Evaluates a {@link ServerRequest} and returns an {@link Evaluation}. + */ + private static abstract class Evaluator { + private static Evaluator of(RequestPredicate requestPredicate) { + if (requestPredicate instanceof EvaluatorRequestPredicate evaluatorRequestPredicate) { + return evaluatorRequestPredicate; + } + // Wrap the RequestPredicate with an Evaluator + return new RequestPredicateEvaluator(requestPredicate); + } + + abstract Evaluation apply(ServerRequest request); + + + private static final class RequestPredicateEvaluator extends Evaluator { + private final RequestPredicate requestPredicate; + + private RequestPredicateEvaluator(RequestPredicate requestPredicate) { + this.requestPredicate = requestPredicate; + } + + @Override + Evaluation apply(ServerRequest request) { + return this.requestPredicate.test(request) ? Evaluation.TRUE : Evaluation.FALSE; + } + } + } + + /** + * A {@link RequestPredicate} which may modify the request. + */ + static abstract class EvaluatorRequestPredicate extends Evaluator implements RequestPredicate { + @Override + public final boolean test(ServerRequest request) { + Evaluation result = apply(request); + boolean value = result.value; + if (value) { + result.doCommit(); + } + return value; + } + } + + private static class HttpMethodPredicate implements RequestPredicate { private final Set httpMethods; @@ -482,7 +551,7 @@ public abstract class RequestPredicates { } - private static class PathPatternPredicate implements RequestPredicate, ChangePathPatternParserVisitor.Target { + private static class PathPatternPredicate extends EvaluatorRequestPredicate implements ChangePathPatternParserVisitor.Target { private PathPattern pattern; @@ -492,29 +561,28 @@ public abstract class RequestPredicates { } @Override - public boolean test(ServerRequest request) { + public Evaluation apply(ServerRequest request) { PathContainer pathContainer = request.requestPath().pathWithinApplication(); PathPattern.PathMatchInfo info = this.pattern.matchAndExtract(pathContainer); traceMatch("Pattern", this.pattern.getPatternString(), request.path(), info != null); - if (info != null) { - mergeAttributes(request, info.getUriVariables(), this.pattern); - return true; + if (info == null) { + return Evaluation.FALSE; } - else { - return false; - } - } + return new Evaluation(true) { + @Override + void doCommit() { + Map attributes = request.attributes(); + Map pathVariables = mergePathVariables(request.pathVariables(), + info.getUriVariables()); + attributes.put(RouterFunctions.URI_TEMPLATE_VARIABLES_ATTRIBUTE, + Collections.unmodifiableMap(pathVariables)); - private static void mergeAttributes(ServerRequest request, Map variables, - PathPattern pattern) { - Map pathVariables = mergePathVariables(request.pathVariables(), variables); - request.attributes().put(RouterFunctions.URI_TEMPLATE_VARIABLES_ATTRIBUTE, - Collections.unmodifiableMap(pathVariables)); - - pattern = mergePatterns( - (PathPattern) request.attributes().get(RouterFunctions.MATCHING_PATTERN_ATTRIBUTE), - pattern); - request.attributes().put(RouterFunctions.MATCHING_PATTERN_ATTRIBUTE, pattern); + PathPattern newPattern = mergePatterns( + (PathPattern) attributes.get(RouterFunctions.MATCHING_PATTERN_ATTRIBUTE), + PathPatternPredicate.this.pattern); + attributes.put(RouterFunctions.MATCHING_PATTERN_ATTRIBUTE, newPattern); + } + }; } @Override @@ -756,28 +824,40 @@ public abstract class RequestPredicates { * {@link RequestPredicate} for where both {@code left} and {@code right} predicates * must match. */ - static class AndRequestPredicate implements RequestPredicate, ChangePathPatternParserVisitor.Target { + static class AndRequestPredicate extends EvaluatorRequestPredicate implements ChangePathPatternParserVisitor.Target { private final RequestPredicate left; + private final Evaluator leftEvaluator; private final RequestPredicate right; + private final Evaluator rightEvaluator; public AndRequestPredicate(RequestPredicate left, RequestPredicate right) { Assert.notNull(left, "Left RequestPredicate must not be null"); Assert.notNull(right, "Right RequestPredicate must not be null"); this.left = left; + this.leftEvaluator = Evaluator.of(left); this.right = right; + this.rightEvaluator = Evaluator.of(right); } @Override - public boolean test(ServerRequest request) { - Map oldAttributes = new HashMap<>(request.attributes()); - - if (this.left.test(request) && this.right.test(request)) { - return true; + public Evaluation apply(ServerRequest request) { + Evaluation leftResult = this.leftEvaluator.apply(request); + if (!leftResult.value) { + return leftResult; } - restoreAttributes(request, oldAttributes); - return false; + Evaluation rightResult = this.rightEvaluator.apply(request); + if (!rightResult.value) { + return rightResult; + } + return new Evaluation(true) { + @Override + void doCommit() { + leftResult.doCommit(); + rightResult.doCommit(); + } + }; } @Override @@ -814,23 +894,26 @@ public abstract class RequestPredicates { /** * {@link RequestPredicate} that negates a delegate predicate. */ - static class NegateRequestPredicate implements RequestPredicate, ChangePathPatternParserVisitor.Target { + static class NegateRequestPredicate extends EvaluatorRequestPredicate implements ChangePathPatternParserVisitor.Target { private final RequestPredicate delegate; + private final Evaluator delegateEvaluator; public NegateRequestPredicate(RequestPredicate delegate) { Assert.notNull(delegate, "Delegate must not be null"); this.delegate = delegate; + this.delegateEvaluator = Evaluator.of(delegate); } @Override - public boolean test(ServerRequest request) { - Map oldAttributes = new HashMap<>(request.attributes()); - boolean result = !this.delegate.test(request); - if (!result) { - restoreAttributes(request, oldAttributes); - } - return result; + public Evaluation apply(ServerRequest request) { + Evaluation result = this.delegateEvaluator.apply(request); + return new Evaluation(!result.value) { + @Override + void doCommit() { + result.doCommit(); + } + }; } @Override @@ -858,34 +941,30 @@ public abstract class RequestPredicates { * {@link RequestPredicate} where either {@code left} or {@code right} predicates * may match. */ - static class OrRequestPredicate implements RequestPredicate, ChangePathPatternParserVisitor.Target { + static class OrRequestPredicate extends EvaluatorRequestPredicate implements ChangePathPatternParserVisitor.Target { private final RequestPredicate left; + private final Evaluator leftEvaluator; private final RequestPredicate right; + private final Evaluator rightEvaluator; public OrRequestPredicate(RequestPredicate left, RequestPredicate right) { Assert.notNull(left, "Left RequestPredicate must not be null"); Assert.notNull(right, "Right RequestPredicate must not be null"); this.left = left; + this.leftEvaluator = Evaluator.of(left); this.right = right; + this.rightEvaluator = Evaluator.of(right); } @Override - public boolean test(ServerRequest request) { - Map oldAttributes = new HashMap<>(request.attributes()); - - if (this.left.test(request)) { - return true; + public Evaluation apply(ServerRequest request) { + Evaluation leftResult = this.leftEvaluator.apply(request); + if (leftResult.value) { + return leftResult; } - else { - restoreAttributes(request, oldAttributes); - if (this.right.test(request)) { - return true; - } - } - restoreAttributes(request, oldAttributes); - return false; + return this.rightEvaluator.apply(request); } @Override diff --git a/spring-webflux/src/test/java/org/springframework/web/reactive/function/server/RequestPredicateAttributesTests.java b/spring-webflux/src/test/java/org/springframework/web/reactive/function/server/RequestPredicateAttributesTests.java index f20e3678a4..184c45457e 100644 --- a/spring-webflux/src/test/java/org/springframework/web/reactive/function/server/RequestPredicateAttributesTests.java +++ b/spring-webflux/src/test/java/org/springframework/web/reactive/function/server/RequestPredicateAttributesTests.java @@ -23,6 +23,8 @@ import org.junit.jupiter.api.Test; import org.springframework.core.codec.StringDecoder; import org.springframework.http.codec.DecoderHttpMessageReader; +import org.springframework.web.reactive.function.server.RequestPredicates.Evaluation; +import org.springframework.web.reactive.function.server.RequestPredicates.EvaluatorRequestPredicate; import org.springframework.web.testfixture.http.server.reactive.MockServerHttpRequest; import org.springframework.web.testfixture.server.MockServerWebExchange; @@ -182,7 +184,7 @@ public class RequestPredicateAttributesTests { } - private static class AddAttributePredicate implements RequestPredicate { + private static class AddAttributePredicate extends EvaluatorRequestPredicate { private boolean result; @@ -197,9 +199,13 @@ public class RequestPredicateAttributesTests { } @Override - public boolean test(ServerRequest request) { - request.attributes().put(key, value); - return this.result; + public Evaluation apply(ServerRequest request) { + return new Evaluation(result) { + @Override + void doCommit() { + request.attributes().put(key, value); + } + }; } } diff --git a/spring-webmvc/src/main/java/org/springframework/web/servlet/function/RequestPredicates.java b/spring-webmvc/src/main/java/org/springframework/web/servlet/function/RequestPredicates.java index fae4532aa6..6e38198145 100644 --- a/spring-webmvc/src/main/java/org/springframework/web/servlet/function/RequestPredicates.java +++ b/spring-webmvc/src/main/java/org/springframework/web/servlet/function/RequestPredicates.java @@ -23,7 +23,6 @@ import java.security.Principal; import java.time.Instant; import java.util.Arrays; import java.util.Collections; -import java.util.HashMap; import java.util.LinkedHashMap; import java.util.LinkedHashSet; import java.util.List; @@ -295,11 +294,6 @@ public abstract class RequestPredicates { } } - private static void restoreAttributes(ServerRequest request, Map attributes) { - request.attributes().clear(); - request.attributes().putAll(attributes); - } - private static Map mergePathVariables(Map oldVariables, Map newVariables) { @@ -431,6 +425,81 @@ public abstract class RequestPredicates { } + /** + * A boolean result with a state-changing commit step to be conditionally + * applied by a caller. + */ + static abstract class Evaluation { + static final Evaluation TRUE = new NopEvaluation(true); + static final Evaluation FALSE = new NopEvaluation(false); + + private final boolean value; + + Evaluation(boolean value) { + this.value = value; + } + + abstract void doCommit(); + + + private static final class NopEvaluation extends Evaluation { + private NopEvaluation(boolean value) { + super(value); + } + + @Override + void doCommit() { + // pass + } + } + } + + + /** + * Evaluates a {@link ServerRequest} and returns an {@link Evaluation}. + */ + private static abstract class Evaluator { + private static Evaluator of(RequestPredicate requestPredicate) { + if (requestPredicate instanceof EvaluatorRequestPredicate evaluatorRequestPredicate) { + return evaluatorRequestPredicate; + } + // Wrap the RequestPredicate with an Evaluator + return new RequestPredicateEvaluator(requestPredicate); + } + + abstract Evaluation apply(ServerRequest request); + + + private static final class RequestPredicateEvaluator extends Evaluator { + private final RequestPredicate requestPredicate; + + private RequestPredicateEvaluator(RequestPredicate requestPredicate) { + this.requestPredicate = requestPredicate; + } + + @Override + Evaluation apply(ServerRequest request) { + return this.requestPredicate.test(request) ? Evaluation.TRUE : Evaluation.FALSE; + } + } + } + + /** + * A {@link RequestPredicate} which may modify the request. + */ + static abstract class EvaluatorRequestPredicate extends Evaluator implements RequestPredicate { + @Override + public final boolean test(ServerRequest request) { + Evaluation result = apply(request); + boolean value = result.value; + if (value) { + result.doCommit(); + } + return value; + } + } + + private static class HttpMethodPredicate implements RequestPredicate { private final Set httpMethods; @@ -481,7 +550,7 @@ public abstract class RequestPredicates { } - private static class PathPatternPredicate implements RequestPredicate, ChangePathPatternParserVisitor.Target { + private static class PathPatternPredicate extends EvaluatorRequestPredicate implements ChangePathPatternParserVisitor.Target { private PathPattern pattern; @@ -491,29 +560,28 @@ public abstract class RequestPredicates { } @Override - public boolean test(ServerRequest request) { + public Evaluation apply(ServerRequest request) { PathContainer pathContainer = request.requestPath().pathWithinApplication(); PathPattern.PathMatchInfo info = this.pattern.matchAndExtract(pathContainer); traceMatch("Pattern", this.pattern.getPatternString(), request.path(), info != null); - if (info != null) { - mergeAttributes(request, info.getUriVariables(), this.pattern); - return true; + if (info == null) { + return Evaluation.FALSE; } - else { - return false; - } - } + return new Evaluation(true) { + @Override + void doCommit() { + Map attributes = request.attributes(); + Map pathVariables = mergePathVariables(request.pathVariables(), + info.getUriVariables()); + attributes.put(RouterFunctions.URI_TEMPLATE_VARIABLES_ATTRIBUTE, + Collections.unmodifiableMap(pathVariables)); - private static void mergeAttributes(ServerRequest request, Map variables, - PathPattern pattern) { - Map pathVariables = mergePathVariables(request.pathVariables(), variables); - request.attributes().put(RouterFunctions.URI_TEMPLATE_VARIABLES_ATTRIBUTE, - Collections.unmodifiableMap(pathVariables)); - - pattern = mergePatterns( - (PathPattern) request.attributes().get(RouterFunctions.MATCHING_PATTERN_ATTRIBUTE), - pattern); - request.attributes().put(RouterFunctions.MATCHING_PATTERN_ATTRIBUTE, pattern); + PathPattern newPattern = mergePatterns( + (PathPattern) attributes.get(RouterFunctions.MATCHING_PATTERN_ATTRIBUTE), + PathPatternPredicate.this.pattern); + attributes.put(RouterFunctions.MATCHING_PATTERN_ATTRIBUTE, newPattern); + } + }; } @Override @@ -755,28 +823,40 @@ public abstract class RequestPredicates { * {@link RequestPredicate} for where both {@code left} and {@code right} predicates * must match. */ - static class AndRequestPredicate implements RequestPredicate, ChangePathPatternParserVisitor.Target { + static class AndRequestPredicate extends EvaluatorRequestPredicate implements ChangePathPatternParserVisitor.Target { private final RequestPredicate left; + private final Evaluator leftEvaluator; private final RequestPredicate right; + private final Evaluator rightEvaluator; public AndRequestPredicate(RequestPredicate left, RequestPredicate right) { Assert.notNull(left, "Left RequestPredicate must not be null"); Assert.notNull(right, "Right RequestPredicate must not be null"); this.left = left; + this.leftEvaluator = Evaluator.of(left); this.right = right; + this.rightEvaluator = Evaluator.of(right); } @Override - public boolean test(ServerRequest request) { - Map oldAttributes = new HashMap<>(request.attributes()); - - if (this.left.test(request) && this.right.test(request)) { - return true; + public Evaluation apply(ServerRequest request) { + Evaluation leftResult = this.leftEvaluator.apply(request); + if (!leftResult.value) { + return leftResult; } - restoreAttributes(request, oldAttributes); - return false; + Evaluation rightResult = this.rightEvaluator.apply(request); + if (!rightResult.value) { + return rightResult; + } + return new Evaluation(true) { + @Override + void doCommit() { + leftResult.doCommit(); + rightResult.doCommit(); + } + }; } @Override @@ -813,23 +893,26 @@ public abstract class RequestPredicates { /** * {@link RequestPredicate} that negates a delegate predicate. */ - static class NegateRequestPredicate implements RequestPredicate, ChangePathPatternParserVisitor.Target { + static class NegateRequestPredicate extends EvaluatorRequestPredicate implements ChangePathPatternParserVisitor.Target { private final RequestPredicate delegate; + private final Evaluator delegateEvaluator; public NegateRequestPredicate(RequestPredicate delegate) { Assert.notNull(delegate, "Delegate must not be null"); this.delegate = delegate; + this.delegateEvaluator = Evaluator.of(delegate); } @Override - public boolean test(ServerRequest request) { - Map oldAttributes = new HashMap<>(request.attributes()); - boolean result = !this.delegate.test(request); - if (!result) { - restoreAttributes(request, oldAttributes); - } - return result; + public Evaluation apply(ServerRequest request) { + Evaluation result = this.delegateEvaluator.apply(request); + return new Evaluation(!result.value) { + @Override + void doCommit() { + result.doCommit(); + } + }; } @Override @@ -857,34 +940,30 @@ public abstract class RequestPredicates { * {@link RequestPredicate} where either {@code left} or {@code right} predicates * may match. */ - static class OrRequestPredicate implements RequestPredicate, ChangePathPatternParserVisitor.Target { + static class OrRequestPredicate extends EvaluatorRequestPredicate implements ChangePathPatternParserVisitor.Target { private final RequestPredicate left; + private final Evaluator leftEvaluator; private final RequestPredicate right; + private final Evaluator rightEvaluator; public OrRequestPredicate(RequestPredicate left, RequestPredicate right) { Assert.notNull(left, "Left RequestPredicate must not be null"); Assert.notNull(right, "Right RequestPredicate must not be null"); this.left = left; + this.leftEvaluator = Evaluator.of(left); this.right = right; + this.rightEvaluator = Evaluator.of(right); } @Override - public boolean test(ServerRequest request) { - Map oldAttributes = new HashMap<>(request.attributes()); - - if (this.left.test(request)) { - return true; + public Evaluation apply(ServerRequest request) { + Evaluation leftResult = this.leftEvaluator.apply(request); + if (leftResult.value) { + return leftResult; } - else { - restoreAttributes(request, oldAttributes); - if (this.right.test(request)) { - return true; - } - } - restoreAttributes(request, oldAttributes); - return false; + return this.rightEvaluator.apply(request); } @Override