From 39786e479080a337691a3a7d0490547e37a900e3 Mon Sep 17 00:00:00 2001 From: James Yuzawa Date: Fri, 24 Feb 2023 08:01:26 -0500 Subject: [PATCH 1/2] 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 284ea7e0d53..6bebef9db46 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 f20e3678a4a..184c45457ea 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 fae4532aa66..6e381981452 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 From c5c843696b50a16077cb6027da127b937c0df6a9 Mon Sep 17 00:00:00 2001 From: Arjen Poutsma Date: Mon, 11 Sep 2023 16:35:53 +0200 Subject: [PATCH 2/2] Polishing external contribution --- .../function/server/RequestPredicates.java | 258 ++++++++++-------- .../RequestPredicateAttributesTests.java | 19 +- .../servlet/function/RequestPredicates.java | 240 ++++++++-------- 3 files changed, 272 insertions(+), 245 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 6bebef9db46..152a9554fe8 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 @@ -427,77 +427,83 @@ public abstract class RequestPredicates { /** - * A boolean result with a state-changing commit step to be conditionally - * applied by a caller. + * Extension of {@code RequestPredicate} that can modify the {@code ServerRequest}. */ - 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(); + static abstract class RequestModifyingPredicate implements RequestPredicate { - private static final class NopEvaluation extends Evaluation { - private NopEvaluation(boolean value) { - super(value); + public static RequestModifyingPredicate of(RequestPredicate requestPredicate) { + if (requestPredicate instanceof RequestModifyingPredicate modifyingPredicate) { + return modifyingPredicate; } - - @Override - void doCommit() { - // pass + else { + return new RequestModifyingPredicate() { + @Override + protected Result testInternal(ServerRequest request) { + return Result.of(requestPredicate.test(request)); + } + }; } } - } - /** - * 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; + Result result = testInternal(request); + boolean value = result.value(); if (value) { - result.doCommit(); + result.modify(request); } return value; } + + protected abstract Result testInternal(ServerRequest request); + + + protected static final class Result { + + private static final Result TRUE = new Result(true, null); + + private static final Result FALSE = new Result(false, null); + + + private final boolean value; + + @Nullable + private final Consumer modify; + + + private Result(boolean value, @Nullable Consumer modify) { + this.value = value; + this.modify = modify; + } + + + public static Result of(boolean value) { + return of(value, null); + } + + public static Result of(boolean value, @Nullable Consumer commit) { + if (commit == null) { + return value ? TRUE : FALSE; + } + else { + return new Result(value, commit); + } + } + + + public boolean value() { + return this.value; + } + + public void modify(ServerRequest request) { + if (this.modify != null) { + this.modify.accept(request); + } + } + } + } @@ -507,7 +513,7 @@ public abstract class RequestPredicates { public HttpMethodPredicate(HttpMethod httpMethod) { Assert.notNull(httpMethod, "HttpMethod must not be null"); - this.httpMethods = Collections.singleton(httpMethod); + this.httpMethods = Set.of(httpMethod); } public HttpMethodPredicate(HttpMethod... httpMethods) { @@ -551,38 +557,41 @@ public abstract class RequestPredicates { } - private static class PathPatternPredicate extends EvaluatorRequestPredicate implements ChangePathPatternParserVisitor.Target { + private static class PathPatternPredicate extends RequestModifyingPredicate + implements ChangePathPatternParserVisitor.Target { private PathPattern pattern; + public PathPatternPredicate(PathPattern pattern) { Assert.notNull(pattern, "'pattern' must not be null"); this.pattern = pattern; } + @Override - public Evaluation apply(ServerRequest request) { + protected Result testInternal(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) { - return Evaluation.FALSE; + if (info != null) { + return Result.of(true, serverRequest -> mergeAttributes(serverRequest, info.getUriVariables())); } - 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)); + else { + return Result.of(false); + } + } - PathPattern newPattern = mergePatterns( - (PathPattern) attributes.get(RouterFunctions.MATCHING_PATTERN_ATTRIBUTE), - PathPatternPredicate.this.pattern); - attributes.put(RouterFunctions.MATCHING_PATTERN_ATTRIBUTE, newPattern); - } - }; + private void mergeAttributes(ServerRequest request, Map variables) { + Map attributes = request.attributes(); + Map pathVariables = mergePathVariables(request.pathVariables(), variables); + attributes.put(RouterFunctions.URI_TEMPLATE_VARIABLES_ATTRIBUTE, + Collections.unmodifiableMap(pathVariables)); + + PathPattern pattern = mergePatterns( + (PathPattern) attributes.get(RouterFunctions.MATCHING_PATTERN_ATTRIBUTE), + this.pattern); + attributes.put(RouterFunctions.MATCHING_PATTERN_ATTRIBUTE, pattern); } @Override @@ -824,40 +833,42 @@ public abstract class RequestPredicates { * {@link RequestPredicate} for where both {@code left} and {@code right} predicates * must match. */ - static class AndRequestPredicate extends EvaluatorRequestPredicate implements ChangePathPatternParserVisitor.Target { + static class AndRequestPredicate extends RequestModifyingPredicate + implements ChangePathPatternParserVisitor.Target { private final RequestPredicate left; - private final Evaluator leftEvaluator; + + private final RequestModifyingPredicate leftModifying; private final RequestPredicate right; - private final Evaluator rightEvaluator; + + private final RequestModifyingPredicate rightModifying; + 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.leftModifying = of(left); this.right = right; - this.rightEvaluator = Evaluator.of(right); + this.rightModifying = of(right); } + @Override - public Evaluation apply(ServerRequest request) { - Evaluation leftResult = this.leftEvaluator.apply(request); - if (!leftResult.value) { + protected Result testInternal(ServerRequest request) { + Result leftResult = this.leftModifying.testInternal(request); + if (!leftResult.value()) { return leftResult; } - Evaluation rightResult = this.rightEvaluator.apply(request); - if (!rightResult.value) { + Result rightResult = this.rightModifying.testInternal(request); + if (!rightResult.value()) { return rightResult; } - return new Evaluation(true) { - @Override - void doCommit() { - leftResult.doCommit(); - rightResult.doCommit(); - } - }; + return Result.of(true, serverRequest -> { + leftResult.modify(serverRequest); + rightResult.modify(serverRequest); + }); } @Override @@ -876,11 +887,11 @@ public abstract class RequestPredicates { @Override public void changeParser(PathPatternParser parser) { - if (this.left instanceof ChangePathPatternParserVisitor.Target leftTarget) { - leftTarget.changeParser(parser); + if (this.left instanceof ChangePathPatternParserVisitor.Target target) { + target.changeParser(parser); } - if (this.right instanceof ChangePathPatternParserVisitor.Target rightTarget) { - rightTarget.changeParser(parser); + if (this.right instanceof ChangePathPatternParserVisitor.Target target) { + target.changeParser(parser); } } @@ -894,26 +905,25 @@ public abstract class RequestPredicates { /** * {@link RequestPredicate} that negates a delegate predicate. */ - static class NegateRequestPredicate extends EvaluatorRequestPredicate implements ChangePathPatternParserVisitor.Target { + static class NegateRequestPredicate extends RequestModifyingPredicate + implements ChangePathPatternParserVisitor.Target { private final RequestPredicate delegate; - private final Evaluator delegateEvaluator; + + private final RequestModifyingPredicate delegateModifying; + public NegateRequestPredicate(RequestPredicate delegate) { Assert.notNull(delegate, "Delegate must not be null"); this.delegate = delegate; - this.delegateEvaluator = Evaluator.of(delegate); + this.delegateModifying = of(delegate); } + @Override - public Evaluation apply(ServerRequest request) { - Evaluation result = this.delegateEvaluator.apply(request); - return new Evaluation(!result.value) { - @Override - void doCommit() { - result.doCommit(); - } - }; + protected Result testInternal(ServerRequest request) { + Result result = this.delegateModifying.testInternal(request); + return Result.of(!result.value(), result::modify); } @Override @@ -941,30 +951,36 @@ public abstract class RequestPredicates { * {@link RequestPredicate} where either {@code left} or {@code right} predicates * may match. */ - static class OrRequestPredicate extends EvaluatorRequestPredicate implements ChangePathPatternParserVisitor.Target { + static class OrRequestPredicate extends RequestModifyingPredicate + implements ChangePathPatternParserVisitor.Target { private final RequestPredicate left; - private final Evaluator leftEvaluator; + + private final RequestModifyingPredicate leftModifying; private final RequestPredicate right; - private final Evaluator rightEvaluator; + + private final RequestModifyingPredicate rightModifying; + 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.leftModifying = of(left); this.right = right; - this.rightEvaluator = Evaluator.of(right); + this.rightModifying = of(right); } @Override - public Evaluation apply(ServerRequest request) { - Evaluation leftResult = this.leftEvaluator.apply(request); - if (leftResult.value) { + protected Result testInternal(ServerRequest request) { + Result leftResult = this.leftModifying.testInternal(request); + if (leftResult.value()) { return leftResult; } - return this.rightEvaluator.apply(request); + else { + return this.rightModifying.testInternal(request); + } } @Override @@ -989,11 +1005,11 @@ public abstract class RequestPredicates { @Override public void changeParser(PathPatternParser parser) { - if (this.left instanceof ChangePathPatternParserVisitor.Target leftTarget) { - leftTarget.changeParser(parser); + if (this.left instanceof ChangePathPatternParserVisitor.Target target) { + target.changeParser(parser); } - if (this.right instanceof ChangePathPatternParserVisitor.Target rightTarget) { - rightTarget.changeParser(parser); + if (this.right instanceof ChangePathPatternParserVisitor.Target target) { + target.changeParser(parser); } } 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 184c45457ea..faef1bc9091 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,8 +23,6 @@ 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; @@ -184,28 +182,25 @@ public class RequestPredicateAttributesTests { } - private static class AddAttributePredicate extends EvaluatorRequestPredicate { + private static class AddAttributePredicate extends RequestPredicates.RequestModifyingPredicate { - private boolean result; + private final boolean result; private final String key; private final String value; - private AddAttributePredicate(boolean result, String key, String value) { + + public AddAttributePredicate(boolean result, String key, String value) { this.result = result; this.key = key; this.value = value; } + @Override - public Evaluation apply(ServerRequest request) { - return new Evaluation(result) { - @Override - void doCommit() { - request.attributes().put(key, value); - } - }; + protected Result testInternal(ServerRequest request) { + return Result.of(this.result, serverRequest -> serverRequest.attributes().put(this.key, this.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 6e381981452..906dbcce748 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 @@ -426,77 +426,83 @@ public abstract class RequestPredicates { /** - * A boolean result with a state-changing commit step to be conditionally - * applied by a caller. + * Extension of {@code RequestPredicate} that can modify the {@code ServerRequest}. */ - 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 abstract class RequestModifyingPredicate implements RequestPredicate { - private static final class NopEvaluation extends Evaluation { - private NopEvaluation(boolean value) { - super(value); + public static RequestModifyingPredicate of(RequestPredicate requestPredicate) { + if (requestPredicate instanceof RequestModifyingPredicate modifyingPredicate) { + return modifyingPredicate; } - - @Override - void doCommit() { - // pass + else { + return new RequestModifyingPredicate() { + @Override + protected Result testInternal(ServerRequest request) { + return Result.of(requestPredicate.test(request)); + } + }; } } - } - /** - * 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; + Result result = testInternal(request); + boolean value = result.value(); if (value) { - result.doCommit(); + result.modify(request); } return value; } + + protected abstract Result testInternal(ServerRequest request); + + + protected static final class Result { + + private static final Result TRUE = new Result(true, null); + + private static final Result FALSE = new Result(false, null); + + + private final boolean value; + + @Nullable + private final Consumer modify; + + + private Result(boolean value, @Nullable Consumer modify) { + this.value = value; + this.modify = modify; + } + + + public static Result of(boolean value) { + return of(value, null); + } + + public static Result of(boolean value, @Nullable Consumer commit) { + if (commit == null) { + return value ? TRUE : FALSE; + } + else { + return new Result(value, commit); + } + } + + + public boolean value() { + return this.value; + } + + public void modify(ServerRequest request) { + if (this.modify != null) { + this.modify.accept(request); + } + } + } + } @@ -550,38 +556,41 @@ public abstract class RequestPredicates { } - private static class PathPatternPredicate extends EvaluatorRequestPredicate implements ChangePathPatternParserVisitor.Target { + private static class PathPatternPredicate extends RequestModifyingPredicate + implements ChangePathPatternParserVisitor.Target { private PathPattern pattern; + public PathPatternPredicate(PathPattern pattern) { Assert.notNull(pattern, "'pattern' must not be null"); this.pattern = pattern; } + @Override - public Evaluation apply(ServerRequest request) { + protected Result testInternal(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) { - return Evaluation.FALSE; + if (info != null) { + return Result.of(true, serverRequest -> mergeAttributes(serverRequest, info.getUriVariables())); } - 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)); + else { + return Result.of(false); + } + } - PathPattern newPattern = mergePatterns( - (PathPattern) attributes.get(RouterFunctions.MATCHING_PATTERN_ATTRIBUTE), - PathPatternPredicate.this.pattern); - attributes.put(RouterFunctions.MATCHING_PATTERN_ATTRIBUTE, newPattern); - } - }; + private void mergeAttributes(ServerRequest request, Map variables) { + Map attributes = request.attributes(); + Map pathVariables = mergePathVariables(request.pathVariables(), variables); + attributes.put(RouterFunctions.URI_TEMPLATE_VARIABLES_ATTRIBUTE, + Collections.unmodifiableMap(pathVariables)); + + PathPattern pattern = mergePatterns( + (PathPattern) attributes.get(RouterFunctions.MATCHING_PATTERN_ATTRIBUTE), + this.pattern); + attributes.put(RouterFunctions.MATCHING_PATTERN_ATTRIBUTE, pattern); } @Override @@ -823,40 +832,42 @@ public abstract class RequestPredicates { * {@link RequestPredicate} for where both {@code left} and {@code right} predicates * must match. */ - static class AndRequestPredicate extends EvaluatorRequestPredicate implements ChangePathPatternParserVisitor.Target { + static class AndRequestPredicate extends RequestModifyingPredicate + implements ChangePathPatternParserVisitor.Target { private final RequestPredicate left; - private final Evaluator leftEvaluator; + + private final RequestModifyingPredicate leftModifying; private final RequestPredicate right; - private final Evaluator rightEvaluator; + + private final RequestModifyingPredicate rightModifying; + 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.leftModifying = of(left); this.right = right; - this.rightEvaluator = Evaluator.of(right); + this.rightModifying = of(right); } + @Override - public Evaluation apply(ServerRequest request) { - Evaluation leftResult = this.leftEvaluator.apply(request); - if (!leftResult.value) { + protected Result testInternal(ServerRequest request) { + Result leftResult = this.leftModifying.testInternal(request); + if (!leftResult.value()) { return leftResult; } - Evaluation rightResult = this.rightEvaluator.apply(request); - if (!rightResult.value) { + Result rightResult = this.rightModifying.testInternal(request); + if (!rightResult.value()) { return rightResult; } - return new Evaluation(true) { - @Override - void doCommit() { - leftResult.doCommit(); - rightResult.doCommit(); - } - }; + return Result.of(true, serverRequest -> { + leftResult.modify(serverRequest); + rightResult.modify(serverRequest); + }); } @Override @@ -893,26 +904,25 @@ public abstract class RequestPredicates { /** * {@link RequestPredicate} that negates a delegate predicate. */ - static class NegateRequestPredicate extends EvaluatorRequestPredicate implements ChangePathPatternParserVisitor.Target { + static class NegateRequestPredicate extends RequestModifyingPredicate + implements ChangePathPatternParserVisitor.Target { private final RequestPredicate delegate; - private final Evaluator delegateEvaluator; + + private final RequestModifyingPredicate delegateModifying; + public NegateRequestPredicate(RequestPredicate delegate) { Assert.notNull(delegate, "Delegate must not be null"); this.delegate = delegate; - this.delegateEvaluator = Evaluator.of(delegate); + this.delegateModifying = of(delegate); } + @Override - public Evaluation apply(ServerRequest request) { - Evaluation result = this.delegateEvaluator.apply(request); - return new Evaluation(!result.value) { - @Override - void doCommit() { - result.doCommit(); - } - }; + protected Result testInternal(ServerRequest request) { + Result result = this.delegateModifying.testInternal(request); + return Result.of(!result.value(), result::modify); } @Override @@ -940,30 +950,36 @@ public abstract class RequestPredicates { * {@link RequestPredicate} where either {@code left} or {@code right} predicates * may match. */ - static class OrRequestPredicate extends EvaluatorRequestPredicate implements ChangePathPatternParserVisitor.Target { + static class OrRequestPredicate extends RequestModifyingPredicate + implements ChangePathPatternParserVisitor.Target { private final RequestPredicate left; - private final Evaluator leftEvaluator; + + private final RequestModifyingPredicate leftModifying; private final RequestPredicate right; - private final Evaluator rightEvaluator; + + private final RequestModifyingPredicate rightModifying; + 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.leftModifying = of(left); this.right = right; - this.rightEvaluator = Evaluator.of(right); + this.rightModifying = of(right); } @Override - public Evaluation apply(ServerRequest request) { - Evaluation leftResult = this.leftEvaluator.apply(request); - if (leftResult.value) { + protected Result testInternal(ServerRequest request) { + Result leftResult = this.leftModifying.testInternal(request); + if (leftResult.value()) { return leftResult; } - return this.rightEvaluator.apply(request); + else { + return this.rightModifying.testInternal(request); + } } @Override