From 4b24bcb79983dce73c0d957ab9f9ccb37c1ca955 Mon Sep 17 00:00:00 2001 From: Rossen Stoyanchev Date: Wed, 2 Jan 2019 14:32:50 -0500 Subject: [PATCH] More accurate checks for presence of MediaType.ALL Typically a straight up equals as well as Collections#contains checks for MediaType.ALL is susceptible to the presence of media type parameters. This commits adds equalsTypeAndSubtype as well as an isPresentIn(Collection) methods to MimeType to faciliate with checks for MediaType.ALL. Issue: SPR-17550 --- .../org/springframework/util/MimeType.java | 34 ++++++++++++- .../AbstractHttpMessageConverter.java | 4 +- .../BufferedImageHttpMessageConverter.java | 4 +- .../ByteArrayHttpMessageConverter.java | 4 +- .../result/HandlerResultHandlerSupport.java | 8 +-- .../condition/ProducesRequestCondition.java | 20 +++++++- .../ProducesRequestConditionTests.java | 51 +++++++++++-------- .../condition/ProducesRequestCondition.java | 4 +- ...stractMessageConverterMethodProcessor.java | 7 +-- .../ProducesRequestConditionTests.java | 13 ++++- 10 files changed, 111 insertions(+), 38 deletions(-) diff --git a/spring-core/src/main/java/org/springframework/util/MimeType.java b/spring-core/src/main/java/org/springframework/util/MimeType.java index 4fbc08eb4ae..f6d3703c20b 100644 --- a/spring-core/src/main/java/org/springframework/util/MimeType.java +++ b/spring-core/src/main/java/org/springframework/util/MimeType.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2018 the original author or authors. + * Copyright 2002-2019 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -19,6 +19,7 @@ package org.springframework.util; import java.io.Serializable; import java.nio.charset.Charset; import java.util.BitSet; +import java.util.Collection; import java.util.Collections; import java.util.Comparator; import java.util.Iterator; @@ -383,6 +384,37 @@ public class MimeType implements Comparable, Serializable { return false; } + /** + * Similar to {@link #equals(Object)} but based on the type and subtype + * only, i.e. ignoring parameters. + * @param other the other mime type to compare to + * @return whether the two mime types have the same type and subtype + * @since 5.1.4 + */ + public boolean equalsTypeAndSubtype(@Nullable MimeType other) { + if (other == null) { + return false; + } + return this.type.equalsIgnoreCase(other.type) && this.subtype.equalsIgnoreCase(other.subtype); + } + + /** + * Unlike {@link Collection#contains(Object)} which relies on + * {@link MimeType#equals(Object)}, this method only checks the type and the + * subtype, but otherwise ignores parameters. + * @param mimeTypes the list of mime types to perform the check against + * @return whether the list contains the given mime type + * @since 5.1.4 + */ + public boolean isPresentIn(Collection mimeTypes) { + for (MimeType mimeType : mimeTypes) { + if (mimeType.equalsTypeAndSubtype(this)) { + return true; + } + } + return false; + } + @Override public boolean equals(Object other) { diff --git a/spring-web/src/main/java/org/springframework/http/converter/AbstractHttpMessageConverter.java b/spring-web/src/main/java/org/springframework/http/converter/AbstractHttpMessageConverter.java index 75a5477782e..c88c60a009e 100644 --- a/spring-web/src/main/java/org/springframework/http/converter/AbstractHttpMessageConverter.java +++ b/spring-web/src/main/java/org/springframework/http/converter/AbstractHttpMessageConverter.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2018 the original author or authors. + * Copyright 2002-2019 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -177,7 +177,7 @@ public abstract class AbstractHttpMessageConverter implements HttpMessageConv * or if the media type is {@code null} */ protected boolean canWrite(@Nullable MediaType mediaType) { - if (mediaType == null || MediaType.ALL.equals(mediaType)) { + if (mediaType == null || MediaType.ALL.equalsTypeAndSubtype(mediaType)) { return true; } for (MediaType supportedMediaType : getSupportedMediaTypes()) { diff --git a/spring-web/src/main/java/org/springframework/http/converter/BufferedImageHttpMessageConverter.java b/spring-web/src/main/java/org/springframework/http/converter/BufferedImageHttpMessageConverter.java index e174888dfee..258ee3aba96 100644 --- a/spring-web/src/main/java/org/springframework/http/converter/BufferedImageHttpMessageConverter.java +++ b/spring-web/src/main/java/org/springframework/http/converter/BufferedImageHttpMessageConverter.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2018 the original author or authors. + * Copyright 2002-2019 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -150,7 +150,7 @@ public class BufferedImageHttpMessageConverter implements HttpMessageConverter imageWriters = ImageIO.getImageWritersByMIMEType(mediaType.toString()); diff --git a/spring-web/src/main/java/org/springframework/http/converter/ByteArrayHttpMessageConverter.java b/spring-web/src/main/java/org/springframework/http/converter/ByteArrayHttpMessageConverter.java index 8bbc5b27557..bf892df24f2 100644 --- a/spring-web/src/main/java/org/springframework/http/converter/ByteArrayHttpMessageConverter.java +++ b/spring-web/src/main/java/org/springframework/http/converter/ByteArrayHttpMessageConverter.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2016 the original author or authors. + * Copyright 2002-2019 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -42,7 +42,7 @@ public class ByteArrayHttpMessageConverter extends AbstractHttpMessageConverter< * Create a new instance of the {@code ByteArrayHttpMessageConverter}. */ public ByteArrayHttpMessageConverter() { - super(new MediaType("application", "octet-stream"), MediaType.ALL); + super(MediaType.APPLICATION_OCTET_STREAM, MediaType.ALL); } diff --git a/spring-webflux/src/main/java/org/springframework/web/reactive/result/HandlerResultHandlerSupport.java b/spring-webflux/src/main/java/org/springframework/web/reactive/result/HandlerResultHandlerSupport.java index 34706f26813..68c38b23fe6 100644 --- a/spring-webflux/src/main/java/org/springframework/web/reactive/result/HandlerResultHandlerSupport.java +++ b/spring-webflux/src/main/java/org/springframework/web/reactive/result/HandlerResultHandlerSupport.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2018 the original author or authors. + * Copyright 2002-2019 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -17,6 +17,7 @@ package org.springframework.web.reactive.result; import java.util.ArrayList; +import java.util.Arrays; import java.util.Comparator; import java.util.LinkedHashSet; import java.util.List; @@ -47,7 +48,8 @@ import org.springframework.web.server.ServerWebExchange; */ public abstract class HandlerResultHandlerSupport implements Ordered { - private static final MediaType MEDIA_TYPE_APPLICATION_ALL = new MediaType("application"); + private static final List ALL_APPLICATION_MEDIA_TYPES = + Arrays.asList(MediaType.ALL, new MediaType("application")); protected final Log logger = LogFactory.getLog(getClass()); @@ -147,7 +149,7 @@ public abstract class HandlerResultHandlerSupport implements Ordered { selected = mediaType; break; } - else if (mediaType.equals(MediaType.ALL) || mediaType.equals(MEDIA_TYPE_APPLICATION_ALL)) { + else if (mediaType.isPresentIn(ALL_APPLICATION_MEDIA_TYPES)) { selected = MediaType.APPLICATION_OCTET_STREAM; break; } diff --git a/spring-webflux/src/main/java/org/springframework/web/reactive/result/condition/ProducesRequestCondition.java b/spring-webflux/src/main/java/org/springframework/web/reactive/result/condition/ProducesRequestCondition.java index ac3ee39833d..faec761208d 100644 --- a/spring-webflux/src/main/java/org/springframework/web/reactive/result/condition/ProducesRequestCondition.java +++ b/spring-webflux/src/main/java/org/springframework/web/reactive/result/condition/ProducesRequestCondition.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2018 the original author or authors. + * Copyright 2002-2019 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -33,6 +33,7 @@ import org.springframework.web.reactive.accept.RequestedContentTypeResolver; import org.springframework.web.reactive.accept.RequestedContentTypeResolverBuilder; import org.springframework.web.server.NotAcceptableStatusException; import org.springframework.web.server.ServerWebExchange; +import org.springframework.web.server.UnsupportedMediaTypeStatusException; /** * A logical disjunction (' || ') request condition to match a request's 'Accept' header @@ -48,6 +49,8 @@ public final class ProducesRequestCondition extends AbstractRequestCondition mediaTypeAllList = Collections.singletonList(new ProduceMediaTypeExpression(MediaType.ALL_VALUE)); @@ -192,7 +195,20 @@ public final class ProducesRequestCondition extends AbstractRequestCondition result = new LinkedHashSet<>(this.expressions); result.removeIf(expression -> !expression.match(exchange)); - return (!result.isEmpty() ? new ProducesRequestCondition(result, this.contentTypeResolver) : null); + if (!result.isEmpty()) { + return new ProducesRequestCondition(result, this.contentTypeResolver); + } + else { + try { + if (MediaType.ALL.isPresentIn(getAcceptedMediaTypes(exchange))) { + return EMPTY_CONDITION; + } + } + catch (NotAcceptableStatusException | UnsupportedMediaTypeStatusException ex) { + // Ignore + } + } + return null; } /** diff --git a/spring-webflux/src/test/java/org/springframework/web/reactive/result/condition/ProducesRequestConditionTests.java b/spring-webflux/src/test/java/org/springframework/web/reactive/result/condition/ProducesRequestConditionTests.java index decb13d342c..5af60ab6ef3 100644 --- a/spring-webflux/src/test/java/org/springframework/web/reactive/result/condition/ProducesRequestConditionTests.java +++ b/spring-webflux/src/test/java/org/springframework/web/reactive/result/condition/ProducesRequestConditionTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2018 the original author or authors. + * Copyright 2002-2019 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -39,7 +39,7 @@ import static org.springframework.mock.http.server.reactive.test.MockServerHttpR public class ProducesRequestConditionTests { @Test - public void match() throws Exception { + public void match() { MockServerWebExchange exchange = MockServerWebExchange.from(get("/").header("Accept", "text/plain")); ProducesRequestCondition condition = new ProducesRequestCondition("text/plain"); @@ -47,7 +47,7 @@ public class ProducesRequestConditionTests { } @Test - public void matchNegated() throws Exception { + public void matchNegated() { MockServerWebExchange exchange = MockServerWebExchange.from(get("/").header("Accept", "text/plain")); ProducesRequestCondition condition = new ProducesRequestCondition("!text/plain"); @@ -55,13 +55,13 @@ public class ProducesRequestConditionTests { } @Test - public void getProducibleMediaTypes() throws Exception { + public void getProducibleMediaTypes() { ProducesRequestCondition condition = new ProducesRequestCondition("!application/xml"); assertEquals(Collections.emptySet(), condition.getProducibleMediaTypes()); } @Test - public void matchWildcard() throws Exception { + public void matchWildcard() { MockServerWebExchange exchange = MockServerWebExchange.from(get("/").header("Accept", "text/plain")); ProducesRequestCondition condition = new ProducesRequestCondition("text/*"); @@ -69,7 +69,7 @@ public class ProducesRequestConditionTests { } @Test - public void matchMultiple() throws Exception { + public void matchMultiple() { MockServerWebExchange exchange = MockServerWebExchange.from(get("/").header("Accept", "text/plain")); ProducesRequestCondition condition = new ProducesRequestCondition("text/plain", "application/xml"); @@ -77,7 +77,7 @@ public class ProducesRequestConditionTests { } @Test - public void matchSingle() throws Exception { + public void matchSingle() { MockServerWebExchange exchange = MockServerWebExchange.from(get("/").header("Accept", "application/xml")); ProducesRequestCondition condition = new ProducesRequestCondition("text/plain"); @@ -85,7 +85,7 @@ public class ProducesRequestConditionTests { } @Test - public void matchParseError() throws Exception { + public void matchParseError() { MockServerWebExchange exchange = MockServerWebExchange.from(get("/").header("Accept", "bogus")); ProducesRequestCondition condition = new ProducesRequestCondition("text/plain"); @@ -93,15 +93,26 @@ public class ProducesRequestConditionTests { } @Test - public void matchParseErrorWithNegation() throws Exception { + public void matchParseErrorWithNegation() { MockServerWebExchange exchange = MockServerWebExchange.from(get("/").header("Accept", "bogus")); ProducesRequestCondition condition = new ProducesRequestCondition("!text/plain"); assertNull(condition.getMatchingCondition(exchange)); } + @Test // SPR-17550 + public void matchWithNegationAndMediaTypeAllWithQualityParameter() { + ProducesRequestCondition condition = new ProducesRequestCondition("!application/json"); + + MockServerWebExchange exchange = MockServerWebExchange.from(get("/").header("Accept", + "text/html,application/xhtml+xml,application/xml;q=0.9,image/webp,image/apng,*/*;q=0.8")); + + assertNotNull(condition.getMatchingCondition(exchange)); + } + + @Test - public void compareTo() throws Exception { + public void compareTo() { ProducesRequestCondition html = new ProducesRequestCondition("text/html"); ProducesRequestCondition xml = new ProducesRequestCondition("application/xml"); ProducesRequestCondition none = new ProducesRequestCondition(); @@ -136,7 +147,7 @@ public class ProducesRequestConditionTests { } @Test - public void compareToWithSingleExpression() throws Exception { + public void compareToWithSingleExpression() { MockServerWebExchange exchange = MockServerWebExchange.from(get("/").header("Accept", "text/plain")); ProducesRequestCondition condition1 = new ProducesRequestCondition("text/plain"); @@ -150,7 +161,7 @@ public class ProducesRequestConditionTests { } @Test - public void compareToMultipleExpressions() throws Exception { + public void compareToMultipleExpressions() { ProducesRequestCondition condition1 = new ProducesRequestCondition("*/*", "text/plain"); ProducesRequestCondition condition2 = new ProducesRequestCondition("text/*", "text/plain;q=0.7"); @@ -164,7 +175,7 @@ public class ProducesRequestConditionTests { } @Test - public void compareToMultipleExpressionsAndMultipleAcceptHeaderValues() throws Exception { + public void compareToMultipleExpressionsAndMultipleAcceptHeaderValues() { ProducesRequestCondition condition1 = new ProducesRequestCondition("text/*", "text/plain"); ProducesRequestCondition condition2 = new ProducesRequestCondition("application/*", "application/xml"); @@ -190,7 +201,7 @@ public class ProducesRequestConditionTests { // SPR-8536 @Test - public void compareToMediaTypeAll() throws Exception { + public void compareToMediaTypeAll() { MockServerWebExchange exchange = MockServerWebExchange.from(get("/")); ProducesRequestCondition condition1 = new ProducesRequestCondition(); @@ -226,7 +237,7 @@ public class ProducesRequestConditionTests { // SPR-9021 @Test - public void compareToMediaTypeAllWithParameter() throws Exception { + public void compareToMediaTypeAllWithParameter() { MockServerWebExchange exchange = MockServerWebExchange.from(get("/").header("Accept", "*/*;q=0.9")); ProducesRequestCondition condition1 = new ProducesRequestCondition(); @@ -237,7 +248,7 @@ public class ProducesRequestConditionTests { } @Test - public void compareToEqualMatch() throws Exception { + public void compareToEqualMatch() { MockServerWebExchange exchange = MockServerWebExchange.from(get("/").header("Accept", "text/*")); ProducesRequestCondition condition1 = new ProducesRequestCondition("text/plain"); @@ -251,7 +262,7 @@ public class ProducesRequestConditionTests { } @Test - public void combine() throws Exception { + public void combine() { ProducesRequestCondition condition1 = new ProducesRequestCondition("text/plain"); ProducesRequestCondition condition2 = new ProducesRequestCondition("application/xml"); @@ -260,7 +271,7 @@ public class ProducesRequestConditionTests { } @Test - public void combineWithDefault() throws Exception { + public void combineWithDefault() { ProducesRequestCondition condition1 = new ProducesRequestCondition("text/plain"); ProducesRequestCondition condition2 = new ProducesRequestCondition(); @@ -269,7 +280,7 @@ public class ProducesRequestConditionTests { } @Test - public void instantiateWithProducesAndHeaderConditions() throws Exception { + public void instantiateWithProducesAndHeaderConditions() { String[] produces = new String[] {"text/plain"}; String[] headers = new String[]{"foo=bar", "accept=application/xml,application/pdf"}; ProducesRequestCondition condition = new ProducesRequestCondition(produces, headers); @@ -278,7 +289,7 @@ public class ProducesRequestConditionTests { } @Test - public void getMatchingCondition() throws Exception { + public void getMatchingCondition() { MockServerWebExchange exchange = MockServerWebExchange.from(get("/").header("Accept", "text/plain")); ProducesRequestCondition condition = new ProducesRequestCondition("text/plain", "application/xml"); diff --git a/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/condition/ProducesRequestCondition.java b/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/condition/ProducesRequestCondition.java index a3c816adb54..6995fc229e2 100644 --- a/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/condition/ProducesRequestCondition.java +++ b/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/condition/ProducesRequestCondition.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2018 the original author or authors. + * Copyright 2002-2019 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -207,7 +207,7 @@ public final class ProducesRequestCondition extends AbstractRequestCondition WHITELISTED_MEDIA_BASE_TYPES = new HashSet<>( Arrays.asList("audio", "image", "video")); - private static final MediaType MEDIA_TYPE_APPLICATION = new MediaType("application"); + private static final List ALL_APPLICATION_MEDIA_TYPES = + Arrays.asList(MediaType.ALL, new MediaType("application")); private static final Type RESOURCE_REGION_LIST_TYPE = new ParameterizedTypeReference>() { }.getType(); @@ -257,7 +258,7 @@ public abstract class AbstractMessageConverterMethodProcessor extends AbstractMe selectedMediaType = mediaType; break; } - else if (mediaType.equals(MediaType.ALL) || mediaType.equals(MEDIA_TYPE_APPLICATION)) { + else if (mediaType.isPresentIn(ALL_APPLICATION_MEDIA_TYPES)) { selectedMediaType = MediaType.APPLICATION_OCTET_STREAM; break; } diff --git a/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/condition/ProducesRequestConditionTests.java b/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/condition/ProducesRequestConditionTests.java index 3d532b43a55..51ffb59ecce 100644 --- a/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/condition/ProducesRequestConditionTests.java +++ b/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/condition/ProducesRequestConditionTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2018 the original author or authors. + * Copyright 2002-2019 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -128,6 +128,17 @@ public class ProducesRequestConditionTests { assertNotNull(condition.getMatchingCondition(request)); } + @Test // SPR-17550 + public void matchWithNegationAndMediaTypeAllWithQualityParameter() { + ProducesRequestCondition condition = new ProducesRequestCondition("!application/json"); + + MockHttpServletRequest request = new MockHttpServletRequest(); + request.addHeader("Accept", + "text/html,application/xhtml+xml,application/xml;q=0.9,image/webp,image/apng,*/*;q=0.8"); + + assertNotNull(condition.getMatchingCondition(request)); + } + @Test public void compareTo() { ProducesRequestCondition html = new ProducesRequestCondition("text/html");