From 3c7e44ada41a26cd1ac243015af893fe2b5411ba Mon Sep 17 00:00:00 2001 From: Rossen Stoyanchev Date: Wed, 3 Aug 2011 11:22:46 +0000 Subject: [PATCH] SPR-8536 When two methods match a request for any content type (i.e. Accept=*/* or no Accept header), choose the one that doesn't define any 'Accept' media types by default. This addresses an important use case where methods serving browsers typically don't specify Accept media types. --- .../condition/AbstractRequestCondition.java | 3 +- .../condition/ConsumesRequestCondition.java | 64 ++++++----- .../condition/HeadersRequestCondition.java | 38 +++--- .../mvc/condition/MediaTypeExpression.java | 6 +- .../mvc/condition/ParamsRequestCondition.java | 26 ++--- .../condition/PatternsRequestCondition.java | 69 +++++------ .../condition/ProducesRequestCondition.java | 108 +++++++++++------- .../mvc/condition/RequestCondition.java | 36 +++--- .../RequestMethodsRequestCondition.java | 25 ++-- .../ServletAnnotationControllerTests.java | 61 +++++++++- .../ProducesRequestConditionTests.java | 49 +++++--- ...nnotationControllerHandlerMethodTests.java | 49 ++++++++ 12 files changed, 348 insertions(+), 186 deletions(-) diff --git a/org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/mvc/condition/AbstractRequestCondition.java b/org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/mvc/condition/AbstractRequestCondition.java index 72c3dae280b..d6c74efa5f7 100644 --- a/org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/mvc/condition/AbstractRequestCondition.java +++ b/org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/mvc/condition/AbstractRequestCondition.java @@ -20,7 +20,8 @@ import java.util.Collection; import java.util.Iterator; /** - * Base class for {@link RequestCondition}s. + * A base class for {@link RequestCondition} types providing implementations of + * {@link #equals(Object)}, {@link #hashCode()}, and {@link #toString()}. * * @author Rossen Stoyanchev * @since 3.1 diff --git a/org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/mvc/condition/ConsumesRequestCondition.java b/org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/mvc/condition/ConsumesRequestCondition.java index ef150a590de..a61e015e9cf 100644 --- a/org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/mvc/condition/ConsumesRequestCondition.java +++ b/org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/mvc/condition/ConsumesRequestCondition.java @@ -32,13 +32,11 @@ import org.springframework.web.bind.annotation.RequestMapping; import org.springframework.web.servlet.mvc.condition.HeadersRequestCondition.HeaderExpression; /** - * A logical disjunction (' || ') request condition to match requests against consumable media type expressions. - * - *

For details on the syntax of the expressions see {@link RequestMapping#consumes()}. If the condition is - * created with 0 consumable media type expressions, it matches to every request. - * - *

This request condition is also capable of parsing header expressions specifically selecting 'Content-Type' - * header expressions and converting them to consumable media type expressions. + * A logical disjunction (' || ') request condition to match a request's 'Content-Type' + * header to a list of media type expressions. Two kinds of media type expressions are + * supported, which are described in {@link RequestMapping#consumes()} and + * {@link RequestMapping#headers()} where the header name is 'Content-Type'. + * Regardless of which syntax is used, the semantics are the same. * * @author Arjen Poutsma * @author Rossen Stoyanchev @@ -49,26 +47,27 @@ public final class ConsumesRequestCondition extends AbstractRequestCondition expressions; /** - * Creates a {@link ConsumesRequestCondition} with the given consumable media type expressions. - * @param consumes the expressions to parse; if 0, the condition matches to every request + * Creates a new instance from 0 or more "consumes" expressions. + * @param consumes expressions with the syntax described in {@link RequestMapping#consumes()} + * if 0 expressions are provided, the condition will match to every request */ public ConsumesRequestCondition(String... consumes) { this(consumes, null); } /** - * Creates a {@link ConsumesRequestCondition} with the given header and consumes expressions. - * In addition to consume expressions, {@code "Content-Type"} header expressions are extracted - * and treated as consumable media type expressions. - * @param consumes the consumes expressions to parse; if 0, the condition matches to all requests - * @param headers the header expression to parse; if 0, the condition matches to all requests + * Creates a new instance with "consumes" and "header" expressions. "Header" expressions + * where the header name is not 'Content-Type' or have no header value defined are ignored. + * If 0 expressions are provided in total, the condition will match to every request + * @param consumes expressions with the syntax described in {@link RequestMapping#consumes()} + * @param headers expressions with the syntax described in {@link RequestMapping#headers()} */ public ConsumesRequestCondition(String[] consumes, String[] headers) { this(parseExpressions(consumes, headers)); } /** - * Private constructor. + * Private constructor accepting parsed media type expressions. */ private ConsumesRequestCondition(Collection expressions) { this.expressions = new ArrayList(expressions); @@ -96,7 +95,7 @@ public final class ConsumesRequestCondition extends AbstractRequestCondition getMediaTypes() { Set result = new LinkedHashSet(); @@ -107,7 +106,7 @@ public final class ConsumesRequestCondition extends AbstractRequestConditionExample: method-level "consumes" overrides type-level "consumes" condition. + * Returns the "other" instance if it has any expressions; returns "this" + * instance otherwise. Practically that means a method-level "consumes" + * overrides a type-level "consumes" condition. */ public ConsumesRequestCondition combine(ConsumesRequestCondition other) { return !other.expressions.isEmpty() ? other : this; } /** - * Checks if any of the consumable media type expressions match the given request and returns an - * instance that is guaranteed to contain matching media type expressions only. + * Checks if any of the contained media type expressions match the given + * request 'Accept' header and returns an instance that is guaranteed to + * contain matching expressions only. The match is performed via + * {@link MediaType#includes(MediaType)}. * * @param request the current request * * @return the same instance if the condition contains no expressions; - * or a new condition with matching expressions only; or {@code null} if no expressions match. + * or a new condition with matching expressions only; + * or {@code null} if no expressions match. */ public ConsumesRequestCondition getMatchingCondition(HttpServletRequest request) { if (isEmpty()) { @@ -159,12 +161,13 @@ public final class ConsumesRequestCondition extends AbstractRequestCondition *

  • 0 if the two conditions have the same number of expressions - *
  • Less than 1 if "this" has more in number or more specific consumable media type expressions - *
  • Greater than 1 if "other" has more in number or more specific consumable media type expressions + *
  • Less than 1 if "this" has more or more specific media type expressions + *
  • Greater than 1 if "other" has more or more specific media type expressions * * - *

    It is assumed that both instances have been obtained via {@link #getMatchingCondition(HttpServletRequest)} - * and each instance contains the matching consumable media type expression only or is otherwise empty. + *

    It is assumed that both instances have been obtained via + * {@link #getMatchingCondition(HttpServletRequest)} and each instance contains + * the matching consumable media type expression only or is otherwise empty. */ public int compareTo(ConsumesRequestCondition other, HttpServletRequest request) { if (expressions.isEmpty() && other.expressions.isEmpty()) { @@ -182,8 +185,7 @@ public final class ConsumesRequestCondition extends AbstractRequestConditionFor details on the syntax of the expressions see {@link RequestMapping#headers()}. If the condition is - * created with 0 header expressions, it will match to every request. - * - *

    Note: when parsing header expressions, {@code "Accept"} and {@code "Content-Type"} header expressions - * are filtered out. Those should be converted and used as "produces" and "consumes" conditions instead. - * See the constructors for {@link ProducesRequestCondition} and {@link ConsumesRequestCondition}. + *

    Expressions passed to the constructor with header names 'Accept' or + * 'Content-Type' are ignored. See {@link ConsumesRequestCondition} and + * {@link ProducesRequestCondition} for those. * * @author Arjen Poutsma * @author Rossen Stoyanchev @@ -44,13 +42,11 @@ public final class HeadersRequestCondition extends AbstractRequestCondition expressions; /** - * Create a {@link HeadersRequestCondition} with the given header expressions. - * - *

    Note: {@code "Accept"} and {@code "Content-Type"} header expressions are filtered out. - * Those should be converted and used as "produces" and "consumes" conditions instead. - * See the constructors for {@link ProducesRequestCondition} and {@link ConsumesRequestCondition}. - * - * @param headers 0 or more header expressions; if 0, the condition will match to every request. + * Create a new instance from the given header expressions. Expressions with + * header names 'Accept' or 'Content-Type' are ignored. See {@link ConsumesRequestCondition} + * and {@link ProducesRequestCondition} for those. + * @param headers media type expressions with syntax defined in {@link RequestMapping#headers()}; + * if 0, the condition will match to every request. */ public HeadersRequestCondition(String... headers) { this(parseExpressions(headers)); @@ -85,7 +81,8 @@ public final class HeadersRequestCondition extends AbstractRequestCondition set = new LinkedHashSet(this.expressions); @@ -94,7 +91,8 @@ public final class HeadersRequestCondition extends AbstractRequestConditionGreater than 1 if the "other" instance has more header expressions * * - *

    It is assumed that both instances have been obtained via {@link #getMatchingCondition(HttpServletRequest)} - * and each instance contains the matching header expression only or is otherwise empty. + *

    It is assumed that both instances have been obtained via + * {@link #getMatchingCondition(HttpServletRequest)} and each instance + * contains the matching header expression only or is otherwise empty. */ public int compareTo(HeadersRequestCondition other, HttpServletRequest request) { return other.expressions.size() - this.expressions.size(); } /** - * Parsing and request matching logic for header expressions. - * @see RequestMapping#headers() + * Parses and matches a single header expression to a request. */ static class HeaderExpression extends AbstractNameValueExpression { diff --git a/org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/mvc/condition/MediaTypeExpression.java b/org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/mvc/condition/MediaTypeExpression.java index 3d1f21beb9c..bc68537d8a9 100644 --- a/org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/mvc/condition/MediaTypeExpression.java +++ b/org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/mvc/condition/MediaTypeExpression.java @@ -51,12 +51,12 @@ abstract class MediaTypeExpression implements Comparable { isNegated = negated; } - public boolean match(HttpServletRequest request) { - boolean match = match(request, this.mediaType); + public final boolean match(HttpServletRequest request) { + boolean match = matchMediaType(request); return !isNegated ? match : !match; } - protected abstract boolean match(HttpServletRequest request, MediaType mediaType); + protected abstract boolean matchMediaType(HttpServletRequest request); MediaType getMediaType() { return mediaType; diff --git a/org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/mvc/condition/ParamsRequestCondition.java b/org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/mvc/condition/ParamsRequestCondition.java index b2891656a14..661f6ffe461 100644 --- a/org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/mvc/condition/ParamsRequestCondition.java +++ b/org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/mvc/condition/ParamsRequestCondition.java @@ -27,10 +27,8 @@ import org.springframework.web.bind.annotation.RequestMapping; import org.springframework.web.util.WebUtils; /** - * A logical conjunction (' && ') request condition that matches a request against a set parameter expressions. - * - *

    For details on the syntax of the expressions see {@link RequestMapping#params()}. If the condition is - * created with 0 parameter expressions, it will match to every request. + * A logical conjunction (' && ') request condition that matches a request against + * a set parameter expressions with syntax defined in {@link RequestMapping#params()}. * * @author Arjen Poutsma * @author Rossen Stoyanchev @@ -41,9 +39,9 @@ public final class ParamsRequestCondition extends AbstractRequestCondition expressions; /** - * Create a {@link ParamsRequestCondition} with the given param expressions. - * - * @param params 0 or more param expressions; if 0, the condition will match to every request. + * Create a new instance from the given param expressions. + * @param params expressions with syntax defined in {@link RequestMapping#params()}; + * if 0, the condition will match to every request. */ public ParamsRequestCondition(String... params) { this(parseExpressions(params)); @@ -74,7 +72,8 @@ public final class ParamsRequestCondition extends AbstractRequestCondition set = new LinkedHashSet(this.expressions); @@ -83,7 +82,8 @@ public final class ParamsRequestCondition extends AbstractRequestConditionGreater than 1 if the "other" instance has more parameter expressions * * - *

    It is assumed that both instances have been obtained via {@link #getMatchingCondition(HttpServletRequest)} - * and each instance contains the matching parameter expressions only or is otherwise empty. + *

    It is assumed that both instances have been obtained via + * {@link #getMatchingCondition(HttpServletRequest)} and each instance + * contains the matching parameter expressions only or is otherwise empty. */ public int compareTo(ParamsRequestCondition other, HttpServletRequest request) { return other.expressions.size() - this.expressions.size(); } /** - * Parsing and request matching logic for parameter expressions. - * @see RequestMapping#params() + * Parses and matches a single param expression to a request. */ static class ParamExpression extends AbstractNameValueExpression { diff --git a/org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/mvc/condition/PatternsRequestCondition.java b/org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/mvc/condition/PatternsRequestCondition.java index 1073b7b31c1..2e32d1d6e16 100644 --- a/org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/mvc/condition/PatternsRequestCondition.java +++ b/org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/mvc/condition/PatternsRequestCondition.java @@ -34,9 +34,8 @@ import org.springframework.util.StringUtils; import org.springframework.web.util.UrlPathHelper; /** - * A logical disjunction (' || ') request condition that matches a request against a set of URL path patterns. - * - *

    See Javadoc on individual methods for details on how URL patterns are matched, combined, and compared. + * A logical disjunction (' || ') request condition that matches a request + * against a set of URL path patterns. * * @author Rossen Stoyanchev * @since 3.1 @@ -52,9 +51,17 @@ public final class PatternsRequestCondition extends AbstractRequestCondition asList(String... patterns) { - return patterns != null ? Arrays.asList(patterns) : Collections.emptyList(); - } - - /** - * Private constructor. + * Private constructor accepting a collection of patterns. */ private PatternsRequestCondition(Collection patterns, UrlPathHelper urlPathHelper, @@ -93,6 +87,10 @@ public final class PatternsRequestCondition extends AbstractRequestCondition asList(String... patterns) { + return patterns != null ? Arrays.asList(patterns) : Collections.emptyList(); + } + private static Set prependLeadingSlash(Collection patterns) { if (patterns == null) { return Collections.emptySet(); @@ -128,7 +126,7 @@ public final class PatternsRequestCondition extends AbstractRequestConditionIf there are patterns in both instances, combine the patterns in "this" with * the patterns in "other" using {@link PathMatcher#combine(String, String)}. *

  • If only one instance has patterns, use them. - *
  • If neither instance has patterns, use "". + *
  • If neither instance has patterns, use an empty String (i.e. ""). * */ public PatternsRequestCondition combine(PatternsRequestCondition other) { @@ -153,21 +151,23 @@ public final class PatternsRequestCondition extends AbstractRequestConditionA matching pattern is obtained by making checks in the following order: *
      *
    • Direct match - *
    • A pattern match with ".*" appended assuming the pattern already doesn't contain "." - *
    • A pattern match - *
    • A pattern match with "/" appended assuming the patterns already end with "/" + *
    • Pattern match with ".*" appended if the pattern doesn't already contain a "." + *
    • Pattern match + *
    • Pattern match with "/" appended if the pattern doesn't already end in "/" *
    * * @param request the current request * * @return the same instance if the condition contains no patterns; - * or a new condition with sorted matching patterns; or {@code null} if no patterns match. + * or a new condition with sorted matching patterns; + * or {@code null} if no patterns match. */ public PatternsRequestCondition getMatchingCondition(HttpServletRequest request) { if (patterns.isEmpty()) { @@ -207,13 +207,16 @@ public final class PatternsRequestCondition extends AbstractRequestConditionIt is assumed that both instances have been obtained via {@link #getMatchingCondition(HttpServletRequest)} - * to ensure they contain only patterns that match the request and are sorted with the best matches on top. + *

    It is assumed that both instances have been obtained via + * {@link #getMatchingCondition(HttpServletRequest)} to ensure they + * contain only patterns that match the request and are sorted with + * the best matches on top. */ public int compareTo(PatternsRequestCondition other, HttpServletRequest request) { String lookupPath = urlPathHelper.getLookupPathForRequest(request); diff --git a/org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/mvc/condition/ProducesRequestCondition.java b/org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/mvc/condition/ProducesRequestCondition.java index 70f8b5b8294..d9aef6fc8f6 100644 --- a/org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/mvc/condition/ProducesRequestCondition.java +++ b/org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/mvc/condition/ProducesRequestCondition.java @@ -17,7 +17,6 @@ package org.springframework.web.servlet.mvc.condition; import java.util.ArrayList; -import java.util.Arrays; import java.util.Collection; import java.util.Collections; import java.util.Iterator; @@ -33,14 +32,11 @@ import org.springframework.web.bind.annotation.RequestMapping; import org.springframework.web.servlet.mvc.condition.HeadersRequestCondition.HeaderExpression; /** - * A logical disjunction (' || ') request condition to match requests against producible - * media type expressions. - * - *

    For details on the syntax of the expressions see {@link RequestMapping#consumes()}. - * If the condition is created without media type expressions, it matches to every request. - * - *

    This request condition is also capable of parsing header expressions by selecting - * 'Accept' header expressions and converting them to prodicuble media type expressions. + * A logical disjunction (' || ') request condition to match a request's 'Accept' header + * to a list of media type expressions. Two kinds of media type expressions are + * supported, which are described in {@link RequestMapping#produces()} and + * {@link RequestMapping#headers()} where the header name is 'Accept'. + * Regardless of which syntax is used, the semantics are the same. * * @author Arjen Poutsma * @author Rossen Stoyanchev @@ -51,26 +47,27 @@ public final class ProducesRequestCondition extends AbstractRequestCondition expressions; /** - * Creates a {@link ProducesRequestCondition} with the given producible media type expressions. - * @param produces the expressions to parse; if 0 the condition matches to every request + * Creates a new instance from 0 or more "produces" expressions. + * @param produces expressions with the syntax described in {@link RequestMapping#produces()} + * if 0 expressions are provided, the condition matches to every request */ public ProducesRequestCondition(String... produces) { - this(produces, null); + this(parseExpressions(produces, null)); } /** - * Creates a {@link ProducesRequestCondition} with the given header and produces expressions. - * In addition to produces expressions, {@code "Accept"} header expressions are extracted and treated as - * producible media type expressions. - * @param produces the produces expressions to parse; if 0, the condition matches to all requests - * @param headers the header expression to parse; if 0, the condition matches to all requests + * Creates a new instance with "produces" and "header" expressions. "Header" expressions + * where the header name is not 'Accept' or have no header value defined are ignored. + * If 0 expressions are provided in total, the condition matches to every request + * @param produces expressions with the syntax described in {@link RequestMapping#produces()} + * @param headers expressions with the syntax described in {@link RequestMapping#headers()} */ public ProducesRequestCondition(String[] produces, String[] headers) { this(parseExpressions(produces, headers)); } /** - * A private constructor. + * Private constructor accepting parsed media type expressions. */ private ProducesRequestCondition(Collection expressions) { this.expressions = new ArrayList(expressions); @@ -98,7 +95,7 @@ public final class ProducesRequestCondition extends AbstractRequestCondition getMediaTypes() { Set result = new LinkedHashSet(); @@ -109,7 +106,7 @@ public final class ProducesRequestCondition extends AbstractRequestConditionExample: method-level "produces" overrides type-level "produces" condition. + * Returns the "other" instance if it has any expressions; returns "this" + * instance otherwise. Practically that means a method-level "produces" + * overrides a type-level "produces" condition. */ public ProducesRequestCondition combine(ProducesRequestCondition other) { return !other.expressions.isEmpty() ? other : this; } /** - * Checks if any of the producible media type expressions match the given request and returns an instance that - * is guaranteed to contain matching media type expressions only. + * Checks if any of the contained media type expressions match the given + * request 'Content-Type' header and returns an instance that is guaranteed + * to contain matching expressions only. The match is performed via + * {@link MediaType#isCompatibleWith(MediaType)}. * * @param request the current request * - * @return the same instance if the condition contains no expressions; - * or a new condition with matching expressions; or {@code null} if no expressions match. + * @return the same instance if there are no expressions; + * or a new condition with matching expressions; + * or {@code null} if no expressions match. */ public ProducesRequestCondition getMatchingCondition(HttpServletRequest request) { if (isEmpty()) { @@ -158,22 +158,38 @@ public final class ProducesRequestCondition extends AbstractRequestCondition - *

  • 0 if the two conditions have the same number of expressions - *
  • Less than 1 if "this" has more in number or more specific producible media type expressions - *
  • Greater than 1 if "other" has more in number or more specific producible media type expressions - * + * Compares this and another Produces condition as follows: * - *

    It is assumed that both instances have been obtained via {@link #getMatchingCondition(HttpServletRequest)} - * and each instance contains the matching producible media type expression only or is otherwise empty. + *

      + *
    1. Sorts the request 'Accept' header media types by quality value via + * {@link MediaType#sortByQualityValue(List)} and iterates over the sorted types. + *
    2. Compares the sorted request media types against the media types of each + * Produces condition via {@link MediaType#includes(MediaType)}. + *
    3. A "produces" condition with a matching media type listed earlier wins. + *
    4. If both conditions have a matching media type at the same index, the + * media types are further compared by specificity and quality. + *
    + * + *

    If a request media type is {@link MediaType#ALL} or if there is no 'Accept' + * header, and therefore both conditions match, preference is given to one + * Produces condition if it is empty and the other one is not. + * + *

    It is assumed that both instances have been obtained via + * {@link #getMatchingCondition(HttpServletRequest)} and each instance + * contains the matching producible media type expression only or + * is otherwise empty. */ public int compareTo(ProducesRequestCondition other, HttpServletRequest request) { String acceptHeader = request.getHeader("Accept"); List acceptedMediaTypes = MediaType.parseMediaTypes(acceptHeader); MediaType.sortByQualityValue(acceptedMediaTypes); - + for (MediaType acceptedMediaType : acceptedMediaTypes) { + if (acceptedMediaType.equals(MediaType.ALL)) { + if (isOneEmptyButNotBoth(other)) { + return this.isEmpty() ? -1 : 1; + } + } int thisIndex = this.indexOfMediaType(acceptedMediaType); int otherIndex = other.indexOfMediaType(acceptedMediaType); if (thisIndex != otherIndex) { @@ -187,6 +203,13 @@ public final class ProducesRequestCondition extends AbstractRequestCondition acceptedMediaTypes = getAcceptedMediaTypes(request); for (MediaType acceptedMediaType : acceptedMediaTypes) { - if (mediaType.isCompatibleWith(acceptedMediaType)) { + if (getMediaType().isCompatibleWith(acceptedMediaType)) { return true; } } diff --git a/org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/mvc/condition/RequestCondition.java b/org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/mvc/condition/RequestCondition.java index caea6862d9c..d97f60ed6cc 100644 --- a/org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/mvc/condition/RequestCondition.java +++ b/org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/mvc/condition/RequestCondition.java @@ -23,11 +23,13 @@ import org.springframework.web.bind.annotation.RequestMapping; /** * The contract for request conditions. * - *

    Request conditions can be combined via {@link #combine(Object)}, matched to a request via - * {@link #getMatchingCondition(HttpServletRequest)}, and compared to each other via - * {@link #compareTo(Object, HttpServletRequest)} to determine which matches a request more closely. + *

    Request conditions can be combined via {@link #combine(Object)}, matched to + * a request via {@link #getMatchingCondition(HttpServletRequest)}, and compared + * to each other via {@link #compareTo(Object, HttpServletRequest)} to determine + * which matches a request more closely. * - * @param The type of objects that this RequestCondition can be compared to and combined with. + * @param The type of objects that this RequestCondition can be combined + * with compared to. * * @author Rossen Stoyanchev * @author Arjen Poutsma @@ -36,27 +38,33 @@ import org.springframework.web.bind.annotation.RequestMapping; public interface RequestCondition { /** - * Defines the rules for combining this condition (i.e. the current instance) with another condition. - * For example combining type- and method-level {@link RequestMapping} conditions. + * Defines the rules for combining this condition (i.e. the current instance) + * with another condition. For example combining type- and method-level + * {@link RequestMapping} conditions. * * @param other the condition to combine with. - * @returns a request condition instance that is the result of combining the two condition instances. + * @returns a request condition instance that is the result of combining + * the two condition instances. */ T combine(T other); /** - * Checks if this condition matches the given request and returns a potentially new request condition - * with content tailored to the current request. For example a condition with URL patterns might return - * a new condition that contains matching patterns sorted with best matching patterns on top. + * Checks if this condition matches the given request and returns a + * potentially new request condition with content tailored to the + * current request. For example a condition with URL patterns might + * return a new condition that contains matching patterns sorted + * with best matching patterns on top. * - * @return a condition instance in case of a match; or {@code null} if there is no match. + * @return a condition instance in case of a match; + * or {@code null} if there is no match. */ T getMatchingCondition(HttpServletRequest request); /** - * Compares this condition to another condition in the context of a specific request. This method assumes - * both instances have been obtained via {@link #getMatchingCondition(HttpServletRequest)} to ensure they - * have content relevant to current request only. + * Compares this condition to another condition in the context of + * a specific request. This method assumes both instances have + * been obtained via {@link #getMatchingCondition(HttpServletRequest)} + * to ensure they have content relevant to current request only. */ int compareTo(T other, HttpServletRequest request); diff --git a/org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/mvc/condition/RequestMethodsRequestCondition.java b/org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/mvc/condition/RequestMethodsRequestCondition.java index dfe9809e91b..1b4d1b61a56 100644 --- a/org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/mvc/condition/RequestMethodsRequestCondition.java +++ b/org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/mvc/condition/RequestMethodsRequestCondition.java @@ -28,9 +28,8 @@ import javax.servlet.http.HttpServletRequest; import org.springframework.web.bind.annotation.RequestMethod; /** - * A logical disjunction (' || ') request condition that matches a request against a set of {@link RequestMethod}s. - * - *

    If the condition is created with 0 HTTP request methods, it matches to every request. + * A logical disjunction (' || ') request condition that matches a request + * against a set of {@link RequestMethod}s. * * @author Arjen Poutsma * @author Rossen Stoyanchev @@ -41,8 +40,9 @@ public final class RequestMethodsRequestCondition extends AbstractRequestConditi private final Set methods; /** - * Create a {@link RequestMethodsRequestCondition} with the given request methods. - * @param requestMethods 0 or more HTTP request methods; if, 0 the condition will match to every request. + * Create a new instance with the given request methods. + * @param requestMethods 0 or more HTTP request methods; + * if, 0 the condition will match to every request. */ public RequestMethodsRequestCondition(RequestMethod... requestMethods) { this(asList(requestMethods)); @@ -77,7 +77,8 @@ public final class RequestMethodsRequestCondition extends AbstractRequestConditi } /** - * Returns a new instance with a union of the HTTP request methods from "this" and the "other" instance. + * Returns a new instance with a union of the HTTP request methods + * from "this" and the "other" instance. */ public RequestMethodsRequestCondition combine(RequestMethodsRequestCondition other) { Set set = new LinkedHashSet(this.methods); @@ -86,11 +87,12 @@ public final class RequestMethodsRequestCondition extends AbstractRequestConditi } /** - * Checks if any of the HTTP request methods match the given request and returns an instance that - * contain the matching request method. + * Checks if any of the HTTP request methods match the given request and returns + * an instance that contains the matching request method only. * @param request the current request * @return the same instance if the condition contains no request method; - * or a new condition with the matching request method; or {@code null} if no request methods match. + * or a new condition with the matching request method; + * or {@code null} if no request methods match. */ public RequestMethodsRequestCondition getMatchingCondition(HttpServletRequest request) { if (methods.isEmpty()) { @@ -113,8 +115,9 @@ public final class RequestMethodsRequestCondition extends AbstractRequestConditi *

  • Greater than 1 "other" has an HTTP request method but "this" doesn't. * * - *

    It is assumed that both instances have been obtained via {@link #getMatchingCondition(HttpServletRequest)} - * and therefore each instance contains the matching HTTP request method only or is otherwise empty. + *

    It is assumed that both instances have been obtained via + * {@link #getMatchingCondition(HttpServletRequest)} and therefore each instance + * contains the matching HTTP request method only or is otherwise empty. */ public int compareTo(RequestMethodsRequestCondition other, HttpServletRequest request) { return other.methods.size() - this.methods.size(); diff --git a/org.springframework.web.servlet/src/test/java/org/springframework/web/servlet/mvc/annotation/ServletAnnotationControllerTests.java b/org.springframework.web.servlet/src/test/java/org/springframework/web/servlet/mvc/annotation/ServletAnnotationControllerTests.java index b0386b5f945..f1450ed4ab7 100644 --- a/org.springframework.web.servlet/src/test/java/org/springframework/web/servlet/mvc/annotation/ServletAnnotationControllerTests.java +++ b/org.springframework.web.servlet/src/test/java/org/springframework/web/servlet/mvc/annotation/ServletAnnotationControllerTests.java @@ -16,6 +16,14 @@ package org.springframework.web.servlet.mvc.annotation; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertSame; +import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; + import java.beans.PropertyEditorSupport; import java.io.IOException; import java.io.Serializable; @@ -39,6 +47,7 @@ import java.util.List; import java.util.Locale; import java.util.Map; import java.util.Set; + import javax.servlet.ServletConfig; import javax.servlet.ServletContext; import javax.servlet.ServletException; @@ -51,7 +60,6 @@ import javax.validation.constraints.NotNull; import javax.xml.bind.annotation.XmlRootElement; import org.junit.Test; - import org.springframework.aop.framework.autoproxy.DefaultAdvisorAutoProxyCreator; import org.springframework.aop.interceptor.SimpleTraceInterceptor; import org.springframework.aop.support.DefaultPointcutAdvisor; @@ -133,8 +141,6 @@ import org.springframework.web.servlet.mvc.support.ControllerClassNameHandlerMap import org.springframework.web.servlet.view.InternalResourceViewResolver; import org.springframework.web.util.NestedServletException; -import static org.junit.Assert.*; - /** * @author Juergen Hoeller * @author Sam Brannen @@ -1869,6 +1875,40 @@ public class ServletAnnotationControllerTests { servlet.service(request, response); assertEquals(405, response.getStatus()); } + + // SPR-8536 + + @Test + public void testHeadersCondition() throws Exception { + initServlet(HeadersConditionController.class); + + // No "Accept" header + MockHttpServletRequest request = new MockHttpServletRequest("GET", "/"); + MockHttpServletResponse response = new MockHttpServletResponse(); + servlet.service(request, response); + + assertEquals(200, response.getStatus()); + assertEquals("home", response.getForwardedUrl()); + + // Accept "*/*" + request = new MockHttpServletRequest("GET", "/"); + request.addHeader("Accept", "*/*"); + response = new MockHttpServletResponse(); + servlet.service(request, response); + + assertEquals(200, response.getStatus()); + assertEquals("home", response.getForwardedUrl()); + + // Accept "application/json" + request = new MockHttpServletRequest("GET", "/"); + request.addHeader("Accept", "application/json"); + response = new MockHttpServletResponse(); + servlet.service(request, response); + + assertEquals(200, response.getStatus()); + assertEquals("application/json", response.getHeader("Content-Type")); + assertEquals("homeJson", response.getContentAsString()); + } /* @@ -3151,5 +3191,20 @@ public class ServletAnnotationControllerTests { writer.write("handle2"); } } + + @Controller + static class HeadersConditionController { + + @RequestMapping(value = "/", method = RequestMethod.GET) + public String home() { + return "home"; + } + + @RequestMapping(value = "/", method = RequestMethod.GET, headers="Accept=application/json") + @ResponseBody + public String homeJson() { + return "homeJson"; + } + } } diff --git a/org.springframework.web.servlet/src/test/java/org/springframework/web/servlet/mvc/condition/ProducesRequestConditionTests.java b/org.springframework.web.servlet/src/test/java/org/springframework/web/servlet/mvc/condition/ProducesRequestConditionTests.java index 4e57104a4c9..91a8dc71410 100644 --- a/org.springframework.web.servlet/src/test/java/org/springframework/web/servlet/mvc/condition/ProducesRequestConditionTests.java +++ b/org.springframework.web.servlet/src/test/java/org/springframework/web/servlet/mvc/condition/ProducesRequestConditionTests.java @@ -120,7 +120,7 @@ public class ProducesRequestConditionTests { } @Test - public void compareToSingle() { + public void compareToWithSingleExpression() { MockHttpServletRequest request = new MockHttpServletRequest(); request.addHeader("Accept", "text/plain"); @@ -135,7 +135,7 @@ public class ProducesRequestConditionTests { } @Test - public void compareToMultiple() { + public void compareToMultipleExpressions() { ProducesRequestCondition condition1 = new ProducesRequestCondition("*/*", "text/plain"); ProducesRequestCondition condition2 = new ProducesRequestCondition("text/*", "text/plain;q=0.7"); @@ -147,22 +147,10 @@ public class ProducesRequestConditionTests { result = condition2.compareTo(condition1, request); assertTrue("Invalid comparison result: " + result, result > 0); - - condition1 = new ProducesRequestCondition("*/*"); - condition2 = new ProducesRequestCondition("text/*"); - - request = new MockHttpServletRequest(); - request.addHeader("Accept", "text/*"); - - result = condition1.compareTo(condition2, request); - assertTrue("Invalid comparison result: " + result, result > 0); - - result = condition2.compareTo(condition1, request); - assertTrue("Invalid comparison result: " + result, result < 0); } @Test - public void compareToMultipleAccept() { + public void compareToMultipleExpressionsAndMultipeAcceptHeaderValues() { ProducesRequestCondition condition1 = new ProducesRequestCondition("text/*", "text/plain"); ProducesRequestCondition condition2 = new ProducesRequestCondition("application/*", "application/xml"); @@ -186,7 +174,36 @@ public class ProducesRequestConditionTests { result = condition2.compareTo(condition1, request); assertTrue("Invalid comparison result: " + result, result < 0); } - + + @Test + public void compareToEmptyCondition() { + MockHttpServletRequest request = new MockHttpServletRequest(); + request.addHeader("Accept", "*/*"); + + ProducesRequestCondition condition1 = new ProducesRequestCondition(); + ProducesRequestCondition condition2 = new ProducesRequestCondition("application/json"); + + int result = condition1.compareTo(condition2, request); + assertTrue("Invalid comparison result: " + result, result < 0); + + result = condition2.compareTo(condition1, request); + assertTrue("Invalid comparison result: " + result, result > 0); + } + + @Test + public void compareToWithoutAcceptHeader() { + MockHttpServletRequest request = new MockHttpServletRequest(); + + ProducesRequestCondition condition1 = new ProducesRequestCondition(); + ProducesRequestCondition condition2 = new ProducesRequestCondition("application/json"); + + int result = condition1.compareTo(condition2, request); + assertTrue("Invalid comparison result: " + result, result < 0); + + result = condition2.compareTo(condition1, request); + assertTrue("Invalid comparison result: " + result, result > 0); + } + @Test public void combine() { ProducesRequestCondition condition1 = new ProducesRequestCondition("text/plain"); diff --git a/org.springframework.web.servlet/src/test/java/org/springframework/web/servlet/mvc/method/annotation/ServletAnnotationControllerHandlerMethodTests.java b/org.springframework.web.servlet/src/test/java/org/springframework/web/servlet/mvc/method/annotation/ServletAnnotationControllerHandlerMethodTests.java index c3e198278f2..12ff2acf5bd 100644 --- a/org.springframework.web.servlet/src/test/java/org/springframework/web/servlet/mvc/method/annotation/ServletAnnotationControllerHandlerMethodTests.java +++ b/org.springframework.web.servlet/src/test/java/org/springframework/web/servlet/mvc/method/annotation/ServletAnnotationControllerHandlerMethodTests.java @@ -1419,6 +1419,40 @@ public class ServletAnnotationControllerHandlerMethodTests extends AbstractServl assertEquals(405, response.getStatus()); } + // SPR-8536 + + @Test + public void testHeadersCondition() throws Exception { + initServletWithControllers(HeadersConditionController.class); + + // No "Accept" header + MockHttpServletRequest request = new MockHttpServletRequest("GET", "/"); + MockHttpServletResponse response = new MockHttpServletResponse(); + getServlet().service(request, response); + + assertEquals(200, response.getStatus()); + assertEquals("home", response.getForwardedUrl()); + + // Accept "*/*" + request = new MockHttpServletRequest("GET", "/"); + request.addHeader("Accept", "*/*"); + response = new MockHttpServletResponse(); + getServlet().service(request, response); + + assertEquals(200, response.getStatus()); + assertEquals("home", response.getForwardedUrl()); + + // Accept "application/json" + request = new MockHttpServletRequest("GET", "/"); + request.addHeader("Accept", "application/json"); + response = new MockHttpServletResponse(); + getServlet().service(request, response); + + assertEquals(200, response.getStatus()); + assertEquals("application/json", response.getHeader("Content-Type")); + assertEquals("homeJson", response.getContentAsString()); + } + /* * Controllers @@ -2713,6 +2747,21 @@ public class ServletAnnotationControllerHandlerMethodTests extends AbstractServl } } + @Controller + static class HeadersConditionController { + + @RequestMapping(value = "/", method = RequestMethod.GET) + public String home() { + return "home"; + } + + @RequestMapping(value = "/", method = RequestMethod.GET, headers="Accept=application/json") + @ResponseBody + public String homeJson() { + return "homeJson"; + } + } + // Test cases deleted from the original SevletAnnotationControllerTests: // @Ignore("Controller interface => no method-level @RequestMapping annotation")