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 d9aef6fc8f6..ea03f6bc75e 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 @@ -99,7 +99,7 @@ public final class ProducesRequestCondition extends AbstractRequestCondition getMediaTypes() { Set result = new LinkedHashSet(); - for (ProduceMediaTypeExpression expression : expressions) { + for (ProduceMediaTypeExpression expression : getContent()) { result.add(expression.getMediaType()); } return result; @@ -113,8 +113,8 @@ public final class ProducesRequestCondition extends AbstractRequestCondition getContent() { - return expressions; + protected List getContent() { + return this.expressions.isEmpty() ? DEFAULT_EXPRESSIONS : this.expressions; } @Override @@ -158,73 +158,91 @@ public final class ProducesRequestCondition extends AbstractRequestCondition - *
  • Sorts the request 'Accept' header media types by quality value via - * {@link MediaType#sortByQualityValue(List)} and iterates over the sorted types. - *
  • Compares the sorted request media types against the media types of each - * Produces condition via {@link MediaType#includes(MediaType)}. - *
  • A "produces" condition with a matching media type listed earlier wins. - *
  • If both conditions have a matching media type at the same index, the - * media types are further compared by specificity and quality. + *
  • Sort 'Accept' header media types by quality value via + * {@link MediaType#sortByQualityValue(List)} and iterate the list. + *
  • Get the lowest index of matching media types from each "produces" + * condition first matching with {@link MediaType#equals(Object)} and + * then with {@link MediaType#includes(MediaType)}. + *
  • If a lower index is found, the "produces" condition wins. + *
  • If both indexes are equal, the media types at the index are + * compared further with {@link MediaType#SPECIFICITY_COMPARATOR}. * * - *

    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); + List acceptedMediaTypes = getAcceptedMediaTypes(request); MediaType.sortByQualityValue(acceptedMediaTypes); for (MediaType acceptedMediaType : acceptedMediaTypes) { - if (acceptedMediaType.equals(MediaType.ALL)) { - if (isOneEmptyButNotBoth(other)) { - return this.isEmpty() ? -1 : 1; - } + int thisIndex = this.indexOfEqualMediaType(acceptedMediaType); + int otherIndex = other.indexOfEqualMediaType(acceptedMediaType); + int result = compareMatchingMediaTypes(this, thisIndex, other, otherIndex); + if (result != 0) { + return result; } - int thisIndex = this.indexOfMediaType(acceptedMediaType); - int otherIndex = other.indexOfMediaType(acceptedMediaType); - if (thisIndex != otherIndex) { - return otherIndex - thisIndex; - } else if (thisIndex != -1 && otherIndex != -1) { - ProduceMediaTypeExpression thisExpr = this.expressions.get(thisIndex); - ProduceMediaTypeExpression otherExpr = other.expressions.get(otherIndex); - int result = thisExpr.compareTo(otherExpr); - if (result != 0) { - return result; - } - } - } - - if (acceptedMediaTypes.isEmpty()) { - if (isOneEmptyButNotBoth(other)) { - return this.isEmpty() ? -1 : 1; + thisIndex = this.indexOfIncludedMediaType(acceptedMediaType); + otherIndex = other.indexOfIncludedMediaType(acceptedMediaType); + result = compareMatchingMediaTypes(this, thisIndex, other, otherIndex); + if (result != 0) { + return result; } } return 0; } - private int indexOfMediaType(MediaType mediaType) { - for (int i = 0; i < expressions.size(); i++) { - if (mediaType.includes(expressions.get(i).getMediaType())) { + private static List getAcceptedMediaTypes(HttpServletRequest request) { + String acceptHeader = request.getHeader("Accept"); + if (StringUtils.hasLength(acceptHeader)) { + return MediaType.parseMediaTypes(acceptHeader); + } + else { + return Collections.singletonList(MediaType.ALL); + } + } + + private int indexOfEqualMediaType(MediaType mediaType) { + for (int i = 0; i < getContent().size(); i++) { + if (mediaType.equals(getContent().get(i).getMediaType())) { return i; } } return -1; } - private boolean isOneEmptyButNotBoth(ProducesRequestCondition other) { - return ((this.isEmpty() || other.isEmpty()) && (this.expressions.size() != other.expressions.size())); + private int indexOfIncludedMediaType(MediaType mediaType) { + for (int i = 0; i < getContent().size(); i++) { + if (mediaType.includes(getContent().get(i).getMediaType())) { + return i; + } + } + return -1; } + + private static int compareMatchingMediaTypes(ProducesRequestCondition condition1, int index1, + ProducesRequestCondition condition2, int index2) { + int result = 0; + if (index1 != index2) { + result = index2 - index1; + } + else if (index1 != -1 && index2 != -1) { + ProduceMediaTypeExpression expr1 = condition1.getContent().get(index1); + ProduceMediaTypeExpression expr2 = condition2.getContent().get(index2); + result = expr1.compareTo(expr2); + result = (result != 0) ? result : expr1.getMediaType().compareTo(expr2.getMediaType()); + } + return result; + } + + private static final List DEFAULT_EXPRESSIONS = + Collections.singletonList(new ProduceMediaTypeExpression("*/*")); /** * Parses and matches a single media type expression to a request's 'Accept' header. @@ -250,15 +268,6 @@ public final class ProducesRequestCondition extends AbstractRequestCondition getAcceptedMediaTypes(HttpServletRequest request) { - String acceptHeader = request.getHeader("Accept"); - if (StringUtils.hasLength(acceptHeader)) { - return MediaType.parseMediaTypes(acceptHeader); - } - else { - return Collections.singletonList(MediaType.ALL); - } - } } } 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 91a8dc71410..f9f7da667ff 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 @@ -175,33 +175,54 @@ public class ProducesRequestConditionTests { assertTrue("Invalid comparison result: " + result, result < 0); } - @Test - public void compareToEmptyCondition() { - MockHttpServletRequest request = new MockHttpServletRequest(); - request.addHeader("Accept", "*/*"); + // SPR-8536 + @Test + public void compareToMediaTypeAll() { + 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); + assertTrue("Should have picked '*/*' condition as an exact match", + condition1.compareTo(condition2, request) < 0); + assertTrue("Should have picked '*/*' condition as an exact match", + condition2.compareTo(condition1, request) > 0); - result = condition2.compareTo(condition1, request); - assertTrue("Invalid comparison result: " + result, result > 0); + condition1 = new ProducesRequestCondition("*/*"); + condition2 = new ProducesRequestCondition("application/json"); + + assertTrue(condition1.compareTo(condition2, request) < 0); + assertTrue(condition2.compareTo(condition1, request) > 0); + + request.addHeader("Accept", "*/*"); + + condition1 = new ProducesRequestCondition(); + condition2 = new ProducesRequestCondition("application/json"); + + assertTrue(condition1.compareTo(condition2, request) < 0); + assertTrue(condition2.compareTo(condition1, request) > 0); + + condition1 = new ProducesRequestCondition("*/*"); + condition2 = new ProducesRequestCondition("application/json"); + + assertTrue(condition1.compareTo(condition2, request) < 0); + assertTrue(condition2.compareTo(condition1, request) > 0); } @Test - public void compareToWithoutAcceptHeader() { + public void compareToEqualMatch() { MockHttpServletRequest request = new MockHttpServletRequest(); + request.addHeader("Accept", "text/*"); - ProducesRequestCondition condition1 = new ProducesRequestCondition(); - ProducesRequestCondition condition2 = new ProducesRequestCondition("application/json"); + ProducesRequestCondition condition1 = new ProducesRequestCondition("text/plain"); + ProducesRequestCondition condition2 = new ProducesRequestCondition("text/xhtml"); int result = condition1.compareTo(condition2, request); - assertTrue("Invalid comparison result: " + result, result < 0); + assertTrue("Should have used MediaType.equals(Object) to break the match", result < 0); result = condition2.compareTo(condition1, request); - assertTrue("Invalid comparison result: " + result, result > 0); + assertTrue("Should have used MediaType.equals(Object) to break the match", result > 0); } @Test diff --git a/org.springframework.web.servlet/src/test/java/org/springframework/web/servlet/mvc/method/RequestMappingInfoTests.java b/org.springframework.web.servlet/src/test/java/org/springframework/web/servlet/mvc/method/RequestMappingInfoTests.java index 9fad86f395c..135786f264f 100644 --- a/org.springframework.web.servlet/src/test/java/org/springframework/web/servlet/mvc/method/RequestMappingInfoTests.java +++ b/org.springframework.web.servlet/src/test/java/org/springframework/web/servlet/mvc/method/RequestMappingInfoTests.java @@ -51,8 +51,8 @@ public class RequestMappingInfoTests { assertEquals(0, info.getPatternsCondition().getPatterns().size()); assertEquals(0, info.getMethodsCondition().getMethods().size()); - assertEquals(0, info.getConsumesCondition().getMediaTypes().size()); - assertEquals(0, info.getProducesCondition().getMediaTypes().size()); + assertEquals(true, info.getConsumesCondition().isEmpty()); + assertEquals(true, info.getProducesCondition().isEmpty()); assertNotNull(info.getParamsCondition()); assertNotNull(info.getHeadersCondition()); assertNull(info.getCustomCondition());