From 7453c0d0cb4646ddea56182ec3511ceeaf179b1f Mon Sep 17 00:00:00 2001 From: Rossen Stoyanchev Date: Tue, 21 Jan 2020 11:06:49 +0000 Subject: [PATCH 1/4] Deprecate config options to match by path extension See gh-24179 --- .../annotation/PathMatchConfigurer.java | 61 +++++++++++++------ .../WebMvcConfigurationSupport.java | 3 +- .../condition/PatternsRequestCondition.java | 39 +++++++++--- .../mvc/method/RequestMappingInfo.java | 25 +++++++- .../RequestMappingHandlerMapping.java | 40 ++++++++++-- 5 files changed, 134 insertions(+), 34 deletions(-) diff --git a/spring-webmvc/src/main/java/org/springframework/web/servlet/config/annotation/PathMatchConfigurer.java b/spring-webmvc/src/main/java/org/springframework/web/servlet/config/annotation/PathMatchConfigurer.java index fc20853ebe..50d6716eb9 100644 --- a/spring-webmvc/src/main/java/org/springframework/web/servlet/config/annotation/PathMatchConfigurer.java +++ b/spring-webmvc/src/main/java/org/springframework/web/servlet/config/annotation/PathMatchConfigurer.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2018 the original author or authors. + * Copyright 2002-2020 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. @@ -22,6 +22,7 @@ import java.util.function.Predicate; import org.springframework.lang.Nullable; import org.springframework.util.PathMatcher; +import org.springframework.web.servlet.mvc.method.annotation.RequestMappingHandlerMapping; import org.springframework.web.util.UrlPathHelper; /** @@ -37,7 +38,7 @@ import org.springframework.web.util.UrlPathHelper; * * @author Brian Clozel * @since 4.0.3 - * @see org.springframework.web.servlet.mvc.method.annotation.RequestMappingHandlerMapping + * @see RequestMappingHandlerMapping * @see org.springframework.web.servlet.handler.SimpleUrlHandlerMapping */ public class PathMatchConfigurer { @@ -46,10 +47,10 @@ public class PathMatchConfigurer { private Boolean suffixPatternMatch; @Nullable - private Boolean trailingSlashMatch; + private Boolean registeredSuffixPatternMatch; @Nullable - private Boolean registeredSuffixPatternMatch; + private Boolean trailingSlashMatch; @Nullable private UrlPathHelper urlPathHelper; @@ -66,22 +67,16 @@ public class PathMatchConfigurer { * requests. If enabled a method mapped to "/users" also matches to "/users.*". *

By default this is set to {@code true}. * @see #registeredSuffixPatternMatch + * @deprecated as of 5.2.4. See class-level note in + * {@link RequestMappingHandlerMapping} on the deprecation of path extension + * config options. */ + @Deprecated public PathMatchConfigurer setUseSuffixPatternMatch(Boolean suffixPatternMatch) { this.suffixPatternMatch = suffixPatternMatch; return this; } - /** - * Whether to match to URLs irrespective of the presence of a trailing slash. - * If enabled a method mapped to "/users" also matches to "/users/". - *

The default value is {@code true}. - */ - public PathMatchConfigurer setUseTrailingSlashMatch(Boolean trailingSlashMatch) { - this.trailingSlashMatch = trailingSlashMatch; - return this; - } - /** * Whether suffix pattern matching should work only against path extensions * explicitly registered when you @@ -90,12 +85,26 @@ public class PathMatchConfigurer { * avoid issues such as when a "." appears in the path for other reasons. *

By default this is set to "false". * @see WebMvcConfigurer#configureContentNegotiation + * @deprecated as of 5.2.4. See class-level note in + * {@link RequestMappingHandlerMapping} on the deprecation of path extension + * config options. */ + @Deprecated public PathMatchConfigurer setUseRegisteredSuffixPatternMatch(Boolean registeredSuffixPatternMatch) { this.registeredSuffixPatternMatch = registeredSuffixPatternMatch; return this; } + /** + * Whether to match to URLs irrespective of the presence of a trailing slash. + * If enabled a method mapped to "/users" also matches to "/users/". + *

The default value is {@code true}. + */ + public PathMatchConfigurer setUseTrailingSlashMatch(Boolean trailingSlashMatch) { + this.trailingSlashMatch = trailingSlashMatch; + return this; + } + /** * Set the UrlPathHelper to use for resolution of lookup paths. *

Use this to override the default UrlPathHelper with a custom subclass, @@ -137,21 +146,35 @@ public class PathMatchConfigurer { } + /** + * Whether to use registered suffixes for pattern matching. + * @deprecated as of 5.2.4. See class-level note in + * {@link RequestMappingHandlerMapping} on the deprecation of path extension + * config options. + */ @Nullable + @Deprecated public Boolean isUseSuffixPatternMatch() { return this.suffixPatternMatch; } + /** + * Whether to use registered suffixes for pattern matching. + * @deprecated as of 5.2.4. See class-level note in + * {@link RequestMappingHandlerMapping} on the deprecation of path extension + * config options. + */ + @Nullable + @Deprecated + public Boolean isUseRegisteredSuffixPatternMatch() { + return this.registeredSuffixPatternMatch; + } + @Nullable public Boolean isUseTrailingSlashMatch() { return this.trailingSlashMatch; } - @Nullable - public Boolean isUseRegisteredSuffixPatternMatch() { - return this.registeredSuffixPatternMatch; - } - @Nullable public UrlPathHelper getUrlPathHelper() { return this.urlPathHelper; diff --git a/spring-webmvc/src/main/java/org/springframework/web/servlet/config/annotation/WebMvcConfigurationSupport.java b/spring-webmvc/src/main/java/org/springframework/web/servlet/config/annotation/WebMvcConfigurationSupport.java index d8f93bf4e5..1000fe12b7 100644 --- a/spring-webmvc/src/main/java/org/springframework/web/servlet/config/annotation/WebMvcConfigurationSupport.java +++ b/spring-webmvc/src/main/java/org/springframework/web/servlet/config/annotation/WebMvcConfigurationSupport.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2019 the original author or authors. + * Copyright 2002-2020 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. @@ -275,6 +275,7 @@ public class WebMvcConfigurationSupport implements ApplicationContextAware, Serv * requests to annotated controllers. */ @Bean + @SuppressWarnings("deprecation") public RequestMappingHandlerMapping requestMappingHandlerMapping( @Qualifier("mvcContentNegotiationManager") ContentNegotiationManager contentNegotiationManager, @Qualifier("mvcConversionService") FormattingConversionService conversionService, diff --git a/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/condition/PatternsRequestCondition.java b/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/condition/PatternsRequestCondition.java index 1c1fd482c5..f5df906880 100644 --- a/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/condition/PatternsRequestCondition.java +++ b/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/condition/PatternsRequestCondition.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2019 the original author or authors. + * Copyright 2002-2020 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.util.AntPathMatcher; import org.springframework.util.PathMatcher; import org.springframework.util.StringUtils; import org.springframework.web.servlet.HandlerMapping; +import org.springframework.web.servlet.mvc.method.annotation.RequestMappingHandlerMapping; import org.springframework.web.util.UrlPathHelper; /** @@ -58,23 +59,42 @@ public final class PatternsRequestCondition extends AbstractRequestCondition fileExtensions) { diff --git a/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/RequestMappingInfo.java b/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/RequestMappingInfo.java index fe79fd341e..7d72839996 100644 --- a/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/RequestMappingInfo.java +++ b/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/RequestMappingInfo.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2019 the original author or authors. + * Copyright 2002-2020 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. @@ -35,6 +35,7 @@ import org.springframework.web.servlet.mvc.condition.ProducesRequestCondition; import org.springframework.web.servlet.mvc.condition.RequestCondition; import org.springframework.web.servlet.mvc.condition.RequestConditionHolder; import org.springframework.web.servlet.mvc.condition.RequestMethodsRequestCondition; +import org.springframework.web.servlet.mvc.method.annotation.RequestMappingHandlerMapping; import org.springframework.web.util.UrlPathHelper; /** @@ -505,6 +506,7 @@ public final class RequestMappingInfo implements RequestConditionBy default this is set to 'true'. * @see #setRegisteredSuffixPatternMatch(boolean) + * @deprecated as of 5.2.4. See class-level note in + * {@link RequestMappingHandlerMapping} on the deprecation of path + * extension config options. */ + @Deprecated public void setSuffixPatternMatch(boolean suffixPatternMatch) { this.suffixPatternMatch = suffixPatternMatch; } /** * Return whether to apply suffix pattern matching in PatternsRequestCondition. + * @deprecated as of 5.2.4. See class-level note in + * {@link RequestMappingHandlerMapping} on the deprecation of path + * extension config options. */ + @Deprecated public boolean useSuffixPatternMatch() { return this.suffixPatternMatch; } @@ -618,7 +628,12 @@ public final class RequestMappingInfo implements RequestCondition getFileExtensions() { if (useRegisteredSuffixPatternMatch() && this.contentNegotiationManager != null) { return this.contentNegotiationManager.getAllFileExtensions(); diff --git a/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/RequestMappingHandlerMapping.java b/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/RequestMappingHandlerMapping.java index 3dadc61a37..5b6d42ed49 100644 --- a/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/RequestMappingHandlerMapping.java +++ b/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/RequestMappingHandlerMapping.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2019 the original author or authors. + * Copyright 2002-2020 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. @@ -59,6 +59,18 @@ import org.springframework.web.servlet.mvc.method.RequestMappingInfoHandlerMappi * {@link RequestMapping @RequestMapping} annotations in * {@link Controller @Controller} classes. * + *

Note:

In 5.2.4, + * {@link #setUseSuffixPatternMatch(boolean) useSuffixPatternMatch} and + * {@link #setUseRegisteredSuffixPatternMatch(boolean) useRegisteredSuffixPatternMatch} + * are deprecated in order to discourage use of path extensions for request + * mapping and for content negotiation (with similar deprecations in + * {@link ContentNegotiationManager}). For further context, please read issue + * #24719. + * + *

In 5.3, {@link #setUseRegisteredSuffixPatternMatch(boolean) useRegisteredSuffixPatternMatch} + * switches to being on by default so that path matching becomes constrained + * to registered suffixes only. + * * @author Arjen Poutsma * @author Rossen Stoyanchev * @author Sam Brannen @@ -89,7 +101,10 @@ public class RequestMappingHandlerMapping extends RequestMappingInfoHandlerMappi *

The default value is {@code true}. *

Also see {@link #setUseRegisteredSuffixPatternMatch(boolean)} for * more fine-grained control over specific suffixes to allow. + * @deprecated as of 5.2.4. See class level comment about deprecation of + * path extension config options. */ + @Deprecated public void setUseSuffixPatternMatch(boolean useSuffixPatternMatch) { this.useSuffixPatternMatch = useSuffixPatternMatch; } @@ -100,7 +115,11 @@ public class RequestMappingHandlerMapping extends RequestMappingInfoHandlerMappi * is generally recommended to reduce ambiguity and to avoid issues such as * when a "." appears in the path for other reasons. *

By default this is set to "false". + * @deprecated as of 5.2.4. See class level comment about deprecation of + * path extension config options note also that in 5.3 the default for this + * property will switch from {@code false} to {@code true}. */ + @Deprecated public void setUseRegisteredSuffixPatternMatch(boolean useRegisteredSuffixPatternMatch) { this.useRegisteredSuffixPatternMatch = useRegisteredSuffixPatternMatch; this.useSuffixPatternMatch = (useRegisteredSuffixPatternMatch || this.useSuffixPatternMatch); @@ -159,13 +178,14 @@ public class RequestMappingHandlerMapping extends RequestMappingInfoHandlerMappi } @Override + @SuppressWarnings("deprecation") public void afterPropertiesSet() { this.config = new RequestMappingInfo.BuilderConfiguration(); this.config.setUrlPathHelper(getUrlPathHelper()); this.config.setPathMatcher(getPathMatcher()); - this.config.setSuffixPatternMatch(this.useSuffixPatternMatch); - this.config.setTrailingSlashMatch(this.useTrailingSlashMatch); - this.config.setRegisteredSuffixPatternMatch(this.useRegisteredSuffixPatternMatch); + this.config.setSuffixPatternMatch(useSuffixPatternMatch()); + this.config.setTrailingSlashMatch(useTrailingSlashMatch()); + this.config.setRegisteredSuffixPatternMatch(useRegisteredSuffixPatternMatch()); this.config.setContentNegotiationManager(getContentNegotiationManager()); super.afterPropertiesSet(); @@ -173,15 +193,21 @@ public class RequestMappingHandlerMapping extends RequestMappingInfoHandlerMappi /** - * Whether to use suffix pattern matching. + * Whether to use registered suffixes for pattern matching. + * @deprecated as of 5.2.4. See class-level note on the deprecation of path + * extension config options. */ + @Deprecated public boolean useSuffixPatternMatch() { return this.useSuffixPatternMatch; } /** * Whether to use registered suffixes for pattern matching. + * @deprecated as of 5.2.4. See class-level note on the deprecation of path + * extension config options. */ + @Deprecated public boolean useRegisteredSuffixPatternMatch() { return this.useRegisteredSuffixPatternMatch; } @@ -195,8 +221,12 @@ public class RequestMappingHandlerMapping extends RequestMappingInfoHandlerMappi /** * Return the file extensions to use for suffix pattern matching. + * @deprecated as of 5.2.4. See class-level note on the deprecation of path + * extension config options. */ @Nullable + @Deprecated + @SuppressWarnings("deprecation") public List getFileExtensions() { return this.config.getFileExtensions(); } From 214ba631279461c6fe4a40de354c675231d118bc Mon Sep 17 00:00:00 2001 From: Rossen Stoyanchev Date: Tue, 21 Jan 2020 21:25:36 +0000 Subject: [PATCH 2/4] Deprecate config options for content negotiation by path extension See gh-24179 --- .../ContentNegotiationManagerFactoryBean.java | 27 ++++++++++++------- .../ContentNegotiationConfigurer.java | 14 +++++++--- 2 files changed, 29 insertions(+), 12 deletions(-) diff --git a/spring-web/src/main/java/org/springframework/web/accept/ContentNegotiationManagerFactoryBean.java b/spring-web/src/main/java/org/springframework/web/accept/ContentNegotiationManagerFactoryBean.java index f49cff6395..91c3193934 100644 --- a/spring-web/src/main/java/org/springframework/web/accept/ContentNegotiationManagerFactoryBean.java +++ b/spring-web/src/main/java/org/springframework/web/accept/ContentNegotiationManagerFactoryBean.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2019 the original author or authors. + * Copyright 2002-2020 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. @@ -81,15 +81,18 @@ import org.springframework.web.context.ServletContextAware; * * * - *

As of 5.0 you can set the exact strategies to use via + *

Alternatively you can avoid use of the above convenience builder + * methods and set the exact strategies to use via * {@link #setStrategies(List)}. * - *

Note: if you must use URL-based content type resolution, - * the use of a query parameter is simpler and preferable to the use of a path - * extension since the latter can cause issues with URI variables, path - * parameters, and URI decoding. Consider setting {@link #setFavorPathExtension} - * to {@literal false} or otherwise set the strategies to use explicitly via - * {@link #setStrategies(List)}. + *

Note: As of 5.2.4, + * {@link #setFavorPathExtension(boolean) favorPathExtension} and + * {@link #setIgnoreUnknownPathExtensions(boolean) ignoreUnknownPathExtensions} + * are deprecated in order to discourage use of path extensions for content + * negotiation and for request mapping (with similar deprecations in + * {@link org.springframework.web.servlet.mvc.method.annotation.RequestMappingHandlerMapping + * RequestMappingHandlerMapping}). For further context, please read issue + * #24719. * * @author Rossen Stoyanchev * @author Brian Clozel @@ -145,7 +148,10 @@ public class ContentNegotiationManagerFactoryBean *

By default this is set to {@code true} in which case a request * for {@code /hotels.pdf} will be interpreted as a request for * {@code "application/pdf"} regardless of the 'Accept' header. + * @deprecated as of 5.2.4. See class-level note on the deprecation of path + * extension config options. */ + @Deprecated public void setFavorPathExtension(boolean favorPathExtension) { this.favorPathExtension = favorPathExtension; } @@ -199,7 +205,10 @@ public class ContentNegotiationManagerFactoryBean * to any media type. Setting this to {@code false} will result in an * {@code HttpMediaTypeNotAcceptableException} if there is no match. *

By default this is set to {@code true}. + * @deprecated as of 5.2.4. See class-level note on the deprecation of path + * extension config options. */ + @Deprecated public void setIgnoreUnknownPathExtensions(boolean ignore) { this.ignoreUnknownPathExtensions = ignore; } @@ -303,7 +312,7 @@ public class ContentNegotiationManagerFactoryBean } /** - * Actually build the {@link ContentNegotiationManager}. + * Create and initialize a {@link ContentNegotiationManager} instance. * @since 5.0 */ public ContentNegotiationManager build() { diff --git a/spring-webmvc/src/main/java/org/springframework/web/servlet/config/annotation/ContentNegotiationConfigurer.java b/spring-webmvc/src/main/java/org/springframework/web/servlet/config/annotation/ContentNegotiationConfigurer.java index 6b0c2522ea..b1ce073255 100644 --- a/spring-webmvc/src/main/java/org/springframework/web/servlet/config/annotation/ContentNegotiationConfigurer.java +++ b/spring-webmvc/src/main/java/org/springframework/web/servlet/config/annotation/ContentNegotiationConfigurer.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2019 the original author or authors. + * Copyright 2002-2020 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. @@ -32,7 +32,6 @@ import org.springframework.web.accept.ContentNegotiationStrategy; import org.springframework.web.accept.FixedContentNegotiationStrategy; import org.springframework.web.accept.HeaderContentNegotiationStrategy; import org.springframework.web.accept.ParameterContentNegotiationStrategy; -import org.springframework.web.accept.PathExtensionContentNegotiationStrategy; /** * Creates a {@code ContentNegotiationManager} and configures it with @@ -52,7 +51,8 @@ import org.springframework.web.accept.PathExtensionContentNegotiationStrategy; * * {@link #favorPathExtension} * true - * {@link PathExtensionContentNegotiationStrategy} + * {@link org.springframework.web.accept.PathExtensionContentNegotiationStrategy + * PathExtensionContentNegotiationStrategy} * Enabled * * @@ -129,7 +129,11 @@ public class ContentNegotiationConfigurer { *

By default this is set to {@code true} in which case a request * for {@code /hotels.pdf} will be interpreted as a request for * {@code "application/pdf"} regardless of the 'Accept' header. + * @deprecated as of 5.2.4. See class-level note in + * {@link ContentNegotiationManagerFactoryBean} on the deprecation of path + * extension config options. */ + @Deprecated public ContentNegotiationConfigurer favorPathExtension(boolean favorPathExtension) { this.factory.setFavorPathExtension(favorPathExtension); return this; @@ -183,7 +187,11 @@ public class ContentNegotiationConfigurer { * to any media type. Setting this to {@code false} will result in an * {@code HttpMediaTypeNotAcceptableException} if there is no match. *

By default this is set to {@code true}. + * @deprecated as of 5.2.4. See class-level note in + * {@link ContentNegotiationManagerFactoryBean} on the deprecation of path + * extension config options. */ + @Deprecated public ContentNegotiationConfigurer ignoreUnknownPathExtensions(boolean ignore) { this.factory.setIgnoreUnknownPathExtensions(ignore); return this; From 542e1878317c66cbcdd0e1129441a94d12848cdb Mon Sep 17 00:00:00 2001 From: Rossen Stoyanchev Date: Wed, 22 Jan 2020 12:32:52 +0000 Subject: [PATCH 3/4] Expose media type mappings in ContentNegotiationManager ContentNegotiationManagerFactoryBean now ensures that ContentNegotiationManager contains the MediaType mappings even if the path extension and the parameter strategies are off. There are also minor fixes to ensure the media type mappings in ContentNegotiationManagerFactoryBean aren't polluted when mapping keys are not lowercase, and likewise MappingMediaTypeFileExtensionResolver filters out duplicates in the list of all file extensions. See gh-24179 --- .../web/accept/ContentNegotiationManager.java | 51 +++++++-- .../ContentNegotiationManagerFactoryBean.java | 65 ++++++----- ...MappingMediaTypeFileExtensionResolver.java | 6 +- ...entNegotiationManagerFactoryBeanTests.java | 102 ++++++++++++++---- ...ngMediaTypeFileExtensionResolverTests.java | 35 +++--- 5 files changed, 186 insertions(+), 73 deletions(-) diff --git a/spring-web/src/main/java/org/springframework/web/accept/ContentNegotiationManager.java b/spring-web/src/main/java/org/springframework/web/accept/ContentNegotiationManager.java index 8ad0110deb..5f3b76d7a4 100644 --- a/spring-web/src/main/java/org/springframework/web/accept/ContentNegotiationManager.java +++ b/spring-web/src/main/java/org/springframework/web/accept/ContentNegotiationManager.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2018 the original author or authors. + * Copyright 2002-2020 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. @@ -20,13 +20,17 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; import java.util.Collections; +import java.util.HashMap; import java.util.LinkedHashSet; import java.util.List; +import java.util.Map; import java.util.Set; +import java.util.function.Function; import org.springframework.http.MediaType; import org.springframework.lang.Nullable; import org.springframework.util.Assert; +import org.springframework.util.CollectionUtils; import org.springframework.web.HttpMediaTypeNotAcceptableException; import org.springframework.web.context.request.NativeWebRequest; @@ -132,11 +136,7 @@ public class ContentNegotiationManager implements ContentNegotiationStrategy, Me @Override public List resolveFileExtensions(MediaType mediaType) { - Set result = new LinkedHashSet<>(); - for (MediaTypeFileExtensionResolver resolver : this.resolvers) { - result.addAll(resolver.resolveFileExtensions(mediaType)); - } - return new ArrayList<>(result); + return doResolveExtensions(resolver -> resolver.resolveFileExtensions(mediaType)); } /** @@ -152,11 +152,44 @@ public class ContentNegotiationManager implements ContentNegotiationStrategy, Me */ @Override public List getAllFileExtensions() { - Set result = new LinkedHashSet<>(); + return doResolveExtensions(MediaTypeFileExtensionResolver::getAllFileExtensions); + } + + private List doResolveExtensions(Function> extractor) { + List result = null; for (MediaTypeFileExtensionResolver resolver : this.resolvers) { - result.addAll(resolver.getAllFileExtensions()); + List extensions = extractor.apply(resolver); + if (CollectionUtils.isEmpty(extensions)) { + continue; + } + result = (result != null ? result : new ArrayList<>(4)); + for (String extension : extensions) { + if (!result.contains(extension)) { + result.add(extension); + } + } } - return new ArrayList<>(result); + return (result != null ? result : Collections.emptyList()); + } + + /** + * Return all registered lookup key to media type mappings by iterating + * {@link MediaTypeFileExtensionResolver}s. + * @since 5.2.4 + */ + public Map getMediaTypeMappings() { + Map result = null; + for (MediaTypeFileExtensionResolver resolver : this.resolvers) { + if (resolver instanceof MappingMediaTypeFileExtensionResolver) { + Map map = ((MappingMediaTypeFileExtensionResolver) resolver).getMediaTypes(); + if (CollectionUtils.isEmpty(map)) { + continue; + } + result = (result != null ? result : new HashMap<>(4)); + result.putAll(map); + } + } + return (result != null ? result : Collections.emptyMap()); } } diff --git a/spring-web/src/main/java/org/springframework/web/accept/ContentNegotiationManagerFactoryBean.java b/spring-web/src/main/java/org/springframework/web/accept/ContentNegotiationManagerFactoryBean.java index 91c3193934..42f5159141 100644 --- a/spring-web/src/main/java/org/springframework/web/accept/ContentNegotiationManagerFactoryBean.java +++ b/spring-web/src/main/java/org/springframework/web/accept/ContentNegotiationManagerFactoryBean.java @@ -89,7 +89,7 @@ import org.springframework.web.context.ServletContextAware; * {@link #setFavorPathExtension(boolean) favorPathExtension} and * {@link #setIgnoreUnknownPathExtensions(boolean) ignoreUnknownPathExtensions} * are deprecated in order to discourage use of path extensions for content - * negotiation and for request mapping (with similar deprecations in + * negotiation as well as for request mapping (with similar deprecations in * {@link org.springframework.web.servlet.mvc.method.annotation.RequestMappingHandlerMapping * RequestMappingHandlerMapping}). For further context, please read issue * #24719. @@ -157,46 +157,52 @@ public class ContentNegotiationManagerFactoryBean } /** - * Add a mapping from a key, extracted from a path extension or a query - * parameter, to a MediaType. This is required in order for the parameter - * strategy to work. Any extensions explicitly registered here are also - * whitelisted for the purpose of Reflected File Download attack detection - * (see Spring Framework reference documentation for more details on RFD - * attack protection). - *

The path extension strategy will also try to use + * Add a mapping from a key to a MediaType where the key are normalized to + * lowercase and may have been extracted from a path extension, a filename + * extension, or passed as a query parameter. + *

The {@link #setFavorParameter(boolean) parameter strategy} requires + * such mappings in order to work while the {@link #setFavorPathExtension(boolean) + * path extension strategy} can fall back on lookups via * {@link ServletContext#getMimeType} and - * {@link org.springframework.http.MediaTypeFactory} to resolve path extensions. + * {@link org.springframework.http.MediaTypeFactory}. + *

Note: Mappings registered here may be accessed via + * {@link ContentNegotiationManager#getMediaTypeMappings()} and may be used + * not only in the parameter and path extension strategies. For example, + * with the Spring MVC config, e.g. {@code @EnableWebMvc} or + * {@code }, the media type mappings are also plugged + * in to: + *

* @param mediaTypes media type mappings * @see #addMediaType(String, MediaType) * @see #addMediaTypes(Map) */ public void setMediaTypes(Properties mediaTypes) { if (!CollectionUtils.isEmpty(mediaTypes)) { - mediaTypes.forEach((key, value) -> { - String extension = ((String) key).toLowerCase(Locale.ENGLISH); - MediaType mediaType = MediaType.valueOf((String) value); - this.mediaTypes.put(extension, mediaType); - }); + mediaTypes.forEach((key, value) -> + addMediaType((String) key, MediaType.valueOf((String) value))); } } /** - * An alternative to {@link #setMediaTypes} for use in Java code. - * @see #setMediaTypes - * @see #addMediaTypes + * An alternative to {@link #setMediaTypes} for programmatic registrations. */ - public void addMediaType(String fileExtension, MediaType mediaType) { - this.mediaTypes.put(fileExtension, mediaType); + public void addMediaType(String key, MediaType mediaType) { + this.mediaTypes.put(key.toLowerCase(Locale.ENGLISH), mediaType); } /** - * An alternative to {@link #setMediaTypes} for use in Java code. - * @see #setMediaTypes - * @see #addMediaType + * An alternative to {@link #setMediaTypes} for programmatic registrations. */ public void addMediaTypes(@Nullable Map mediaTypes) { if (mediaTypes != null) { - this.mediaTypes.putAll(mediaTypes); + mediaTypes.forEach(this::addMediaType); } } @@ -315,6 +321,7 @@ public class ContentNegotiationManagerFactoryBean * Create and initialize a {@link ContentNegotiationManager} instance. * @since 5.0 */ + @SuppressWarnings("deprecation") public ContentNegotiationManager build() { List strategies = new ArrayList<>(); @@ -336,7 +343,6 @@ public class ContentNegotiationManagerFactoryBean } strategies.add(strategy); } - if (this.favorParameter) { ParameterContentNegotiationStrategy strategy = new ParameterContentNegotiationStrategy(this.mediaTypes); strategy.setParameterName(this.parameterName); @@ -348,17 +354,24 @@ public class ContentNegotiationManagerFactoryBean } strategies.add(strategy); } - if (!this.ignoreAcceptHeader) { strategies.add(new HeaderContentNegotiationStrategy()); } - if (this.defaultNegotiationStrategy != null) { strategies.add(this.defaultNegotiationStrategy); } } this.contentNegotiationManager = new ContentNegotiationManager(strategies); + + // Ensure media type mappings are available via ContentNegotiationManager#getMediaTypeMappings() + // independent of path extension or parameter strategies. + + if (!CollectionUtils.isEmpty(this.mediaTypes) && !this.favorPathExtension && !this.favorParameter) { + this.contentNegotiationManager.addFileExtensionResolvers( + new MappingMediaTypeFileExtensionResolver(this.mediaTypes)); + } + return this.contentNegotiationManager; } diff --git a/spring-web/src/main/java/org/springframework/web/accept/MappingMediaTypeFileExtensionResolver.java b/spring-web/src/main/java/org/springframework/web/accept/MappingMediaTypeFileExtensionResolver.java index 25147843b7..a0ee49a569 100644 --- a/spring-web/src/main/java/org/springframework/web/accept/MappingMediaTypeFileExtensionResolver.java +++ b/spring-web/src/main/java/org/springframework/web/accept/MappingMediaTypeFileExtensionResolver.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2019 the original author or authors. + * Copyright 2002-2020 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. @@ -18,9 +18,11 @@ package org.springframework.web.accept; import java.util.ArrayList; import java.util.Collections; +import java.util.HashSet; import java.util.List; import java.util.Locale; import java.util.Map; +import java.util.Set; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ConcurrentMap; import java.util.concurrent.CopyOnWriteArrayList; @@ -53,7 +55,7 @@ public class MappingMediaTypeFileExtensionResolver implements MediaTypeFileExten */ public MappingMediaTypeFileExtensionResolver(@Nullable Map mediaTypes) { if (mediaTypes != null) { - List allFileExtensions = new ArrayList<>(); + Set allFileExtensions = new HashSet<>(mediaTypes.size()); mediaTypes.forEach((extension, mediaType) -> { String lowerCaseExtension = extension.toLowerCase(Locale.ENGLISH); this.mediaTypes.put(lowerCaseExtension, mediaType); diff --git a/spring-web/src/test/java/org/springframework/web/accept/ContentNegotiationManagerFactoryBeanTests.java b/spring-web/src/test/java/org/springframework/web/accept/ContentNegotiationManagerFactoryBeanTests.java index 1b5ac13d79..2c0169c997 100644 --- a/spring-web/src/test/java/org/springframework/web/accept/ContentNegotiationManagerFactoryBeanTests.java +++ b/spring-web/src/test/java/org/springframework/web/accept/ContentNegotiationManagerFactoryBeanTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2019 the original author or authors. + * Copyright 2002-2020 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. @@ -21,6 +21,7 @@ import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.Properties; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -70,21 +71,29 @@ public class ContentNegotiationManagerFactoryBeanTests { this.servletRequest.setRequestURI("/flower.gif"); - assertThat(manager.resolveMediaTypes(this.webRequest)).as("Should be able to resolve file extensions by default").isEqualTo(Collections.singletonList(MediaType.IMAGE_GIF)); + assertThat(manager.resolveMediaTypes(this.webRequest)) + .as("Should be able to resolve file extensions by default") + .isEqualTo(Collections.singletonList(MediaType.IMAGE_GIF)); this.servletRequest.setRequestURI("/flower.foobarbaz"); - assertThat(manager.resolveMediaTypes(this.webRequest)).as("Should ignore unknown extensions by default").isEqualTo(ContentNegotiationStrategy.MEDIA_TYPE_ALL_LIST); + assertThat(manager.resolveMediaTypes(this.webRequest)) + .as("Should ignore unknown extensions by default") + .isEqualTo(ContentNegotiationStrategy.MEDIA_TYPE_ALL_LIST); this.servletRequest.setRequestURI("/flower"); this.servletRequest.setParameter("format", "gif"); - assertThat(manager.resolveMediaTypes(this.webRequest)).as("Should not resolve request parameters by default").isEqualTo(ContentNegotiationStrategy.MEDIA_TYPE_ALL_LIST); + assertThat(manager.resolveMediaTypes(this.webRequest)) + .as("Should not resolve request parameters by default") + .isEqualTo(ContentNegotiationStrategy.MEDIA_TYPE_ALL_LIST); this.servletRequest.setRequestURI("/flower"); this.servletRequest.addHeader("Accept", MediaType.IMAGE_GIF_VALUE); - assertThat(manager.resolveMediaTypes(this.webRequest)).as("Should resolve Accept header by default").isEqualTo(Collections.singletonList(MediaType.IMAGE_GIF)); + assertThat(manager.resolveMediaTypes(this.webRequest)) + .as("Should resolve Accept header by default") + .isEqualTo(Collections.singletonList(MediaType.IMAGE_GIF)); } @Test @@ -101,29 +110,33 @@ public class ContentNegotiationManagerFactoryBeanTests { this.servletRequest.setRequestURI("/flower"); this.servletRequest.addParameter("format", "bar"); - assertThat(manager.resolveMediaTypes(this.webRequest)).isEqualTo(Collections.singletonList(new MediaType("application", "bar"))); + assertThat(manager.resolveMediaTypes(this.webRequest)) + .isEqualTo(Collections.singletonList(new MediaType("application", "bar"))); } @Test public void favorPath() throws Exception { this.factoryBean.setFavorPathExtension(true); - this.factoryBean.addMediaTypes(Collections.singletonMap("bar", new MediaType("application", "bar"))); + this.factoryBean.addMediaType("bar", new MediaType("application", "bar")); this.factoryBean.afterPropertiesSet(); ContentNegotiationManager manager = this.factoryBean.getObject(); this.servletRequest.setRequestURI("/flower.foo"); - assertThat(manager.resolveMediaTypes(this.webRequest)).isEqualTo(Collections.singletonList(new MediaType("application", "foo"))); + assertThat(manager.resolveMediaTypes(this.webRequest)) + .isEqualTo(Collections.singletonList(new MediaType("application", "foo"))); this.servletRequest.setRequestURI("/flower.bar"); - assertThat(manager.resolveMediaTypes(this.webRequest)).isEqualTo(Collections.singletonList(new MediaType("application", "bar"))); + assertThat(manager.resolveMediaTypes(this.webRequest)) + .isEqualTo(Collections.singletonList(new MediaType("application", "bar"))); this.servletRequest.setRequestURI("/flower.gif"); - assertThat(manager.resolveMediaTypes(this.webRequest)).isEqualTo(Collections.singletonList(MediaType.IMAGE_GIF)); + assertThat(manager.resolveMediaTypes(this.webRequest)) + .isEqualTo(Collections.singletonList(MediaType.IMAGE_GIF)); } @Test // SPR-10170 - public void favorPathWithIgnoreUnknownPathExtensionTurnedOff() throws Exception { + public void favorPathWithIgnoreUnknownPathExtensionTurnedOff() { this.factoryBean.setFavorPathExtension(true); this.factoryBean.setIgnoreUnknownPathExtensions(false); this.factoryBean.afterPropertiesSet(); @@ -139,10 +152,7 @@ public class ContentNegotiationManagerFactoryBeanTests { @Test public void favorParameter() throws Exception { this.factoryBean.setFavorParameter(true); - - Map mediaTypes = new HashMap<>(); - mediaTypes.put("json", MediaType.APPLICATION_JSON); - this.factoryBean.addMediaTypes(mediaTypes); + this.factoryBean.addMediaType("json", MediaType.APPLICATION_JSON); this.factoryBean.afterPropertiesSet(); ContentNegotiationManager manager = this.factoryBean.getObject(); @@ -150,11 +160,12 @@ public class ContentNegotiationManagerFactoryBeanTests { this.servletRequest.setRequestURI("/flower"); this.servletRequest.addParameter("format", "json"); - assertThat(manager.resolveMediaTypes(this.webRequest)).isEqualTo(Collections.singletonList(MediaType.APPLICATION_JSON)); + assertThat(manager.resolveMediaTypes(this.webRequest)) + .isEqualTo(Collections.singletonList(MediaType.APPLICATION_JSON)); } @Test // SPR-10170 - public void favorParameterWithUnknownMediaType() throws HttpMediaTypeNotAcceptableException { + public void favorParameterWithUnknownMediaType() { this.factoryBean.setFavorParameter(true); this.factoryBean.afterPropertiesSet(); ContentNegotiationManager manager = this.factoryBean.getObject(); @@ -162,8 +173,52 @@ public class ContentNegotiationManagerFactoryBeanTests { this.servletRequest.setRequestURI("/flower"); this.servletRequest.setParameter("format", "invalid"); - assertThatExceptionOfType(HttpMediaTypeNotAcceptableException.class).isThrownBy(() -> - manager.resolveMediaTypes(this.webRequest)); + assertThatExceptionOfType(HttpMediaTypeNotAcceptableException.class) + .isThrownBy(() -> manager.resolveMediaTypes(this.webRequest)); + } + + @Test + public void mediaTypeMappingsWithoutPathAndParameterStrategies() { + + this.factoryBean.setFavorPathExtension(false); + this.factoryBean.setFavorParameter(false); + + Properties properties = new Properties(); + properties.put("JSon", "application/json"); + + this.factoryBean.setMediaTypes(properties); + this.factoryBean.addMediaType("pdF", MediaType.APPLICATION_PDF); + this.factoryBean.addMediaTypes(Collections.singletonMap("xML", MediaType.APPLICATION_XML)); + + ContentNegotiationManager manager = this.factoryBean.build(); + assertThat(manager.getMediaTypeMappings()) + .hasSize(3) + .containsEntry("json", MediaType.APPLICATION_JSON) + .containsEntry("pdf", MediaType.APPLICATION_PDF) + .containsEntry("xml", MediaType.APPLICATION_XML); + } + + @Test + public void fileExtensions() { + + this.factoryBean.setFavorPathExtension(false); + this.factoryBean.setFavorParameter(false); + + Properties properties = new Properties(); + properties.put("json", "application/json"); + properties.put("pdf", "application/pdf"); + properties.put("xml", "application/xml"); + this.factoryBean.setMediaTypes(properties); + + this.factoryBean.addMediaType("jsON", MediaType.APPLICATION_JSON); + this.factoryBean.addMediaType("pdF", MediaType.APPLICATION_PDF); + + this.factoryBean.addMediaTypes(Collections.singletonMap("JSon", MediaType.APPLICATION_JSON)); + this.factoryBean.addMediaTypes(Collections.singletonMap("xML", MediaType.APPLICATION_XML)); + + ContentNegotiationManager manager = this.factoryBean.build(); + assertThat(manager.getAllFileExtensions()).containsExactlyInAnyOrder("json", "xml", "pdf"); + } @Test @@ -175,7 +230,8 @@ public class ContentNegotiationManagerFactoryBeanTests { this.servletRequest.setRequestURI("/flower"); this.servletRequest.addHeader("Accept", MediaType.IMAGE_GIF_VALUE); - assertThat(manager.resolveMediaTypes(this.webRequest)).isEqualTo(ContentNegotiationStrategy.MEDIA_TYPE_ALL_LIST); + assertThat(manager.resolveMediaTypes(this.webRequest)) + .isEqualTo(ContentNegotiationStrategy.MEDIA_TYPE_ALL_LIST); } @Test @@ -210,10 +266,12 @@ public class ContentNegotiationManagerFactoryBeanTests { this.factoryBean.afterPropertiesSet(); ContentNegotiationManager manager = this.factoryBean.getObject(); - assertThat(manager.resolveMediaTypes(this.webRequest)).isEqualTo(Collections.singletonList(MediaType.APPLICATION_JSON)); + assertThat(manager.resolveMediaTypes(this.webRequest)) + .isEqualTo(Collections.singletonList(MediaType.APPLICATION_JSON)); this.servletRequest.addHeader("Accept", MediaType.ALL_VALUE); - assertThat(manager.resolveMediaTypes(this.webRequest)).isEqualTo(Collections.singletonList(MediaType.APPLICATION_JSON)); + assertThat(manager.resolveMediaTypes(this.webRequest)) + .isEqualTo(Collections.singletonList(MediaType.APPLICATION_JSON)); } diff --git a/spring-web/src/test/java/org/springframework/web/accept/MappingMediaTypeFileExtensionResolverTests.java b/spring-web/src/test/java/org/springframework/web/accept/MappingMediaTypeFileExtensionResolverTests.java index 873f521a9e..7f1907dba5 100644 --- a/spring-web/src/test/java/org/springframework/web/accept/MappingMediaTypeFileExtensionResolverTests.java +++ b/spring-web/src/test/java/org/springframework/web/accept/MappingMediaTypeFileExtensionResolverTests.java @@ -17,6 +17,7 @@ package org.springframework.web.accept; import java.util.Collections; +import java.util.HashMap; import java.util.List; import java.util.Map; @@ -34,12 +35,14 @@ import static org.assertj.core.api.Assertions.assertThat; */ public class MappingMediaTypeFileExtensionResolverTests { - private final Map mapping = Collections.singletonMap("json", MediaType.APPLICATION_JSON); - private final MappingMediaTypeFileExtensionResolver resolver = new MappingMediaTypeFileExtensionResolver(this.mapping); + private static final Map DEFAULT_MAPPINGS = + Collections.singletonMap("json", MediaType.APPLICATION_JSON); + @Test public void resolveExtensions() { - List extensions = this.resolver.resolveFileExtensions(MediaType.APPLICATION_JSON); + List extensions = new MappingMediaTypeFileExtensionResolver(DEFAULT_MAPPINGS) + .resolveFileExtensions(MediaType.APPLICATION_JSON); assertThat(extensions).hasSize(1); assertThat(extensions.get(0)).isEqualTo("json"); @@ -47,20 +50,24 @@ public class MappingMediaTypeFileExtensionResolverTests { @Test public void resolveExtensionsNoMatch() { - List extensions = this.resolver.resolveFileExtensions(MediaType.TEXT_HTML); - - assertThat(extensions).isEmpty(); + assertThat(new MappingMediaTypeFileExtensionResolver(DEFAULT_MAPPINGS) + .resolveFileExtensions(MediaType.TEXT_HTML)).isEmpty(); } - /** - * Unit test for SPR-13747 - ensures that reverse lookup of media type from media - * type key is case-insensitive. - */ - @Test + @Test // SPR-13747 public void lookupMediaTypeCaseInsensitive() { - MediaType mediaType = this.resolver.lookupMediaType("JSON"); - - assertThat(mediaType).isEqualTo(MediaType.APPLICATION_JSON); + assertThat(new MappingMediaTypeFileExtensionResolver(DEFAULT_MAPPINGS).lookupMediaType("JSON")) + .isEqualTo(MediaType.APPLICATION_JSON); } + @Test + public void allFileExtensions() { + Map mappings = new HashMap<>(); + mappings.put("json", MediaType.APPLICATION_JSON); + mappings.put("JsOn", MediaType.APPLICATION_JSON); + mappings.put("jSoN", MediaType.APPLICATION_JSON); + + MappingMediaTypeFileExtensionResolver resolver = new MappingMediaTypeFileExtensionResolver(mappings); + assertThat(resolver.getAllFileExtensions()).containsExactly("json"); + } } From c69703ffdb032f70e640755de1b3fc270bea9920 Mon Sep 17 00:00:00 2001 From: Rossen Stoyanchev Date: Wed, 22 Jan 2020 13:34:27 +0000 Subject: [PATCH 4/4] Deprecate path extension strategies This commit deprecates PathExtensionContentNegotiationStrategy and ServletPathExtensionContentNegotiationStrategy and also updates code that depends on them internally to remove that dependence. See gh-24179 --- .../setup/StandaloneMockMvcBuilder.java | 7 +- ...thExtensionContentNegotiationStrategy.java | 6 +- ...thExtensionContentNegotiationStrategy.java | 6 +- .../annotation/ResourceHandlerRegistry.java | 3 +- ...stractMessageConverterMethodProcessor.java | 39 ++---- .../resource/ResourceHttpRequestHandler.java | 112 +++++++++++++----- .../ResourceHttpRequestHandlerTests.java | 18 +-- src/docs/asciidoc/web/webmvc.adoc | 20 +++- 8 files changed, 138 insertions(+), 73 deletions(-) diff --git a/spring-test/src/main/java/org/springframework/test/web/servlet/setup/StandaloneMockMvcBuilder.java b/spring-test/src/main/java/org/springframework/test/web/servlet/setup/StandaloneMockMvcBuilder.java index 46209caddd..554d7e85da 100644 --- a/spring-test/src/main/java/org/springframework/test/web/servlet/setup/StandaloneMockMvcBuilder.java +++ b/spring-test/src/main/java/org/springframework/test/web/servlet/setup/StandaloneMockMvcBuilder.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2019 the original author or authors. + * Copyright 2002-2020 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. @@ -310,7 +310,11 @@ public class StandaloneMockMvcBuilder extends AbstractMockMvcBuilderThe default value is {@code true}. + * @deprecated as of 5.2.4. See class-level note in + * {@link RequestMappingHandlerMapping} on the deprecation of path extension + * config options. */ + @Deprecated public StandaloneMockMvcBuilder setUseSuffixPatternMatch(boolean useSuffixPatternMatch) { this.useSuffixPatternMatch = useSuffixPatternMatch; return this; @@ -442,6 +446,7 @@ public class StandaloneMockMvcBuilder extends AbstractMockMvcBuilder safeExtensions = new HashSet<>(); @@ -133,17 +132,10 @@ public abstract class AbstractMessageConverterMethodProcessor extends AbstractMe super(converters, requestResponseBodyAdvice); this.contentNegotiationManager = (manager != null ? manager : new ContentNegotiationManager()); - this.pathStrategy = initPathStrategy(this.contentNegotiationManager); this.safeExtensions.addAll(this.contentNegotiationManager.getAllFileExtensions()); this.safeExtensions.addAll(WHITELISTED_EXTENSIONS); } - private static PathExtensionContentNegotiationStrategy initPathStrategy(ContentNegotiationManager manager) { - Class clazz = PathExtensionContentNegotiationStrategy.class; - PathExtensionContentNegotiationStrategy strategy = manager.getStrategy(clazz); - return (strategy != null ? strategy : new PathExtensionContentNegotiationStrategy()); - } - /** * Creates a new {@link HttpOutputMessage} from the given {@link NativeWebRequest}. @@ -481,26 +473,21 @@ public abstract class AbstractMessageConverterMethodProcessor extends AbstractMe return true; } } - return safeMediaTypesForExtension(new ServletWebRequest(request), extension); + MediaType mediaType = resolveMediaType(request, extension); + return (mediaType != null && (safeMediaType(mediaType))); } - private boolean safeMediaTypesForExtension(NativeWebRequest request, String extension) { - List mediaTypes = null; - try { - mediaTypes = this.pathStrategy.resolveMediaTypeKey(request, extension); + @Nullable + private MediaType resolveMediaType(ServletRequest request, String extension) { + MediaType result = null; + String rawMimeType = request.getServletContext().getMimeType("file." + extension); + if (StringUtils.hasText(rawMimeType)) { + result = MediaType.parseMediaType(rawMimeType); } - catch (HttpMediaTypeNotAcceptableException ex) { - // Ignore + if (result == null || MediaType.APPLICATION_OCTET_STREAM.equals(result)) { + result = MediaTypeFactory.getMediaType("file." + extension).orElse(null); } - if (CollectionUtils.isEmpty(mediaTypes)) { - return false; - } - for (MediaType mediaType : mediaTypes) { - if (!safeMediaType(mediaType)) { - return false; - } - } - return true; + return result; } private boolean safeMediaType(MediaType mediaType) { diff --git a/spring-webmvc/src/main/java/org/springframework/web/servlet/resource/ResourceHttpRequestHandler.java b/spring-webmvc/src/main/java/org/springframework/web/servlet/resource/ResourceHttpRequestHandler.java index 6dcf31c1d5..186c79d7bb 100644 --- a/spring-webmvc/src/main/java/org/springframework/web/servlet/resource/ResourceHttpRequestHandler.java +++ b/spring-webmvc/src/main/java/org/springframework/web/servlet/resource/ResourceHttpRequestHandler.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2019 the original author or authors. + * Copyright 2002-2020 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. @@ -24,6 +24,7 @@ import java.util.ArrayList; import java.util.Collections; import java.util.HashMap; import java.util.List; +import java.util.Locale; import java.util.Map; import java.util.stream.Collectors; @@ -43,6 +44,7 @@ import org.springframework.http.HttpHeaders; import org.springframework.http.HttpMethod; import org.springframework.http.HttpRange; import org.springframework.http.MediaType; +import org.springframework.http.MediaTypeFactory; import org.springframework.http.converter.ResourceHttpMessageConverter; import org.springframework.http.converter.ResourceRegionHttpMessageConverter; import org.springframework.http.server.ServletServerHttpRequest; @@ -56,8 +58,6 @@ import org.springframework.util.StringUtils; import org.springframework.util.StringValueResolver; import org.springframework.web.HttpRequestHandler; import org.springframework.web.accept.ContentNegotiationManager; -import org.springframework.web.accept.PathExtensionContentNegotiationStrategy; -import org.springframework.web.accept.ServletPathExtensionContentNegotiationStrategy; import org.springframework.web.context.request.ServletWebRequest; import org.springframework.web.cors.CorsConfiguration; import org.springframework.web.cors.CorsConfigurationSource; @@ -129,8 +129,7 @@ public class ResourceHttpRequestHandler extends WebContentGenerator @Nullable private ContentNegotiationManager contentNegotiationManager; - @Nullable - private PathExtensionContentNegotiationStrategy contentNegotiationStrategy; + private final Map mediaTypes = new HashMap<>(4); @Nullable private CorsConfiguration corsConfiguration; @@ -262,7 +261,11 @@ public class ResourceHttpRequestHandler extends WebContentGenerator * media types for resources being served. If the manager contains a path * extension strategy it will be checked for registered file extension. * @since 4.3 + * @deprecated as of 5.2.4 in favor of using {@link #setMediaTypes(Map)} + * with mappings possibly obtained from + * {@link ContentNegotiationManager#getMediaTypeMappings()}. */ + @Deprecated public void setContentNegotiationManager(@Nullable ContentNegotiationManager contentNegotiationManager) { this.contentNegotiationManager = contentNegotiationManager; } @@ -270,12 +273,38 @@ public class ResourceHttpRequestHandler extends WebContentGenerator /** * Return the configured content negotiation manager. * @since 4.3 + * @deprecated as of 5.2.4. */ @Nullable + @Deprecated public ContentNegotiationManager getContentNegotiationManager() { return this.contentNegotiationManager; } + /** + * Add mappings between file extensions, extracted from the filename of a + * static {@link Resource}, and corresponding media type to set on the + * response. + *

Use of this method is typically not necessary since mappings are + * otherwise determined via + * {@link javax.servlet.ServletContext#getMimeType(String)} or via + * {@link MediaTypeFactory#getMediaType(Resource)}. + * @param mediaTypes media type mappings + * @since 5.2.4 + */ + public void setMediaTypes(Map mediaTypes) { + mediaTypes.forEach((ext, mediaType) -> + this.mediaTypes.put(ext.toLowerCase(Locale.ENGLISH), mediaType)); + } + + /** + * Return the {@link #setMediaTypes(Map) configured} media types. + * @since 5.2.4 + */ + public Map getMediaTypes() { + return this.mediaTypes; + } + /** * Specify the CORS configuration for resources served by this handler. *

By default this is not set in which allows cross-origin requests. @@ -344,7 +373,17 @@ public class ResourceHttpRequestHandler extends WebContentGenerator this.resourceRegionHttpMessageConverter = new ResourceRegionHttpMessageConverter(); } - this.contentNegotiationStrategy = initContentNegotiationStrategy(); + ContentNegotiationManager manager = getContentNegotiationManager(); + if (manager != null) { + setMediaTypes(manager.getMediaTypeMappings()); + } + + @SuppressWarnings("deprecation") + org.springframework.web.accept.PathExtensionContentNegotiationStrategy strategy = + initContentNegotiationStrategy(); + if (strategy != null) { + setMediaTypes(strategy.getMediaTypes()); + } } private void resolveResourceLocations() { @@ -412,26 +451,20 @@ public class ResourceHttpRequestHandler extends WebContentGenerator } /** - * Initialize the content negotiation strategy depending on the {@code ContentNegotiationManager} - * setup and the availability of a {@code ServletContext}. - * @see ServletPathExtensionContentNegotiationStrategy - * @see PathExtensionContentNegotiationStrategy + * Initialize the strategy to use to determine the media type for a resource. + * @deprecated as of 5.2.4 this method returns {@code null}, and if a + * sub-class returns an actual instance,the instance is used only as a + * source of media type mappings, if it contains any. Please, use + * {@link #setMediaTypes(Map)} instead, or if you need to change behavior, + * you can override {@link #getMediaType(HttpServletRequest, Resource)}. */ - protected PathExtensionContentNegotiationStrategy initContentNegotiationStrategy() { - Map mediaTypes = null; - if (getContentNegotiationManager() != null) { - PathExtensionContentNegotiationStrategy strategy = - getContentNegotiationManager().getStrategy(PathExtensionContentNegotiationStrategy.class); - if (strategy != null) { - mediaTypes = new HashMap<>(strategy.getMediaTypes()); - } - } - return (getServletContext() != null ? - new ServletPathExtensionContentNegotiationStrategy(getServletContext(), mediaTypes) : - new PathExtensionContentNegotiationStrategy(mediaTypes)); + @Nullable + @Deprecated + @SuppressWarnings("deprecation") + protected org.springframework.web.accept.PathExtensionContentNegotiationStrategy initContentNegotiationStrategy() { + return null; } - /** * Processes a resource request. *

Checks for the existence of the requested resource in the configured list of locations. @@ -659,17 +692,40 @@ public class ResourceHttpRequestHandler extends WebContentGenerator /** * Determine the media type for the given request and the resource matched - * to it. This implementation tries to determine the MediaType based on the - * file extension of the Resource via - * {@link ServletPathExtensionContentNegotiationStrategy#getMediaTypeForResource}. + * to it. This implementation tries to determine the MediaType using one of + * the following lookups based on the resource filename and its path + * extension: + *

    + *
  1. {@link javax.servlet.ServletContext#getMimeType(String)} + *
  2. {@link #getMediaTypes()} + *
  3. {@link MediaTypeFactory#getMediaType(String)} + *
* @param request the current request * @param resource the resource to check * @return the corresponding media type, or {@code null} if none found */ @Nullable protected MediaType getMediaType(HttpServletRequest request, Resource resource) { - return (this.contentNegotiationStrategy != null ? - this.contentNegotiationStrategy.getMediaTypeForResource(resource) : null); + MediaType result = null; + String mimeType = request.getServletContext().getMimeType(resource.getFilename()); + if (StringUtils.hasText(mimeType)) { + result = MediaType.parseMediaType(mimeType); + } + if (result == null || MediaType.APPLICATION_OCTET_STREAM.equals(result)) { + MediaType mediaType = null; + String filename = resource.getFilename(); + String ext = StringUtils.getFilenameExtension(filename); + if (ext != null) { + mediaType = this.mediaTypes.get(ext.toLowerCase(Locale.ENGLISH)); + } + if (mediaType == null) { + mediaType = MediaTypeFactory.getMediaType(filename).orElse(null); + } + if (mediaType != null) { + result = mediaType; + } + } + return result; } /** diff --git a/spring-webmvc/src/test/java/org/springframework/web/servlet/resource/ResourceHttpRequestHandlerTests.java b/spring-webmvc/src/test/java/org/springframework/web/servlet/resource/ResourceHttpRequestHandlerTests.java index 62ff42be89..9af18827f9 100644 --- a/spring-webmvc/src/test/java/org/springframework/web/servlet/resource/ResourceHttpRequestHandlerTests.java +++ b/spring-webmvc/src/test/java/org/springframework/web/servlet/resource/ResourceHttpRequestHandlerTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2019 the original author or authors. + * Copyright 2002-2020 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. @@ -74,13 +74,15 @@ public class ResourceHttpRequestHandlerTests { paths.add(new ClassPathResource("testalternatepath/", getClass())); paths.add(new ClassPathResource("META-INF/resources/webjars/")); + TestServletContext servletContext = new TestServletContext(); + this.handler = new ResourceHttpRequestHandler(); this.handler.setLocations(paths); this.handler.setCacheSeconds(3600); - this.handler.setServletContext(new TestServletContext()); + this.handler.setServletContext(servletContext); this.handler.afterPropertiesSet(); - this.request = new MockHttpServletRequest("GET", ""); + this.request = new MockHttpServletRequest(servletContext, "GET", ""); this.response = new MockHttpServletResponse(); } @@ -283,15 +285,12 @@ public class ResourceHttpRequestHandlerTests { @Test // SPR-14368 public void getResourceWithMediaTypeResolvedThroughServletContext() throws Exception { + MockServletContext servletContext = new MockServletContext() { @Override public String getMimeType(String filePath) { return "foo/bar"; } - @Override - public String getVirtualServerName() { - return ""; - } }; List paths = Collections.singletonList(new ClassPathResource("test/", getClass())); @@ -300,8 +299,9 @@ public class ResourceHttpRequestHandlerTests { handler.setLocations(paths); handler.afterPropertiesSet(); - this.request.setAttribute(HandlerMapping.PATH_WITHIN_HANDLER_MAPPING_ATTRIBUTE, "foo.css"); - handler.handleRequest(this.request, this.response); + MockHttpServletRequest request = new MockHttpServletRequest(servletContext, "GET", ""); + request.setAttribute(HandlerMapping.PATH_WITHIN_HANDLER_MAPPING_ATTRIBUTE, "foo.css"); + handler.handleRequest(request, this.response); assertThat(this.response.getContentType()).isEqualTo("foo/bar"); assertThat(this.response.getContentAsString()).isEqualTo("h1 { color:red; }"); diff --git a/src/docs/asciidoc/web/webmvc.adoc b/src/docs/asciidoc/web/webmvc.adoc index 6de7b41083..af1a836792 100644 --- a/src/docs/asciidoc/web/webmvc.adoc +++ b/src/docs/asciidoc/web/webmvc.adoc @@ -1683,6 +1683,18 @@ the issues that come with file extensions. Alternatively, if you must use file e restricting them to a list of explicitly registered extensions through the `mediaTypes` property of <>. +[INFO] +==== +Starting in 5.2.4, path extension related options for request mapping in +{api-spring-framework}/org/springframework/web/servlet/mvc/method/annotation/RequestMappingHandlerMapping.java[RequestMappingHandlerMapping] +and for content negotiation in +{api-spring-framework}/org.springframework.web.accept/ContentNegotiationManagerFactoryBean.java[ContentNegotiationManagerFactoryBean] +are deprecated. See Spring Framework issue +https://github.com/spring-projects/spring-framework/issues/24179[#24179] and related +issues for further plans. +==== + + [[mvc-ann-requestmapping-rfd]] ==== Suffix Match and RFD @@ -5779,13 +5791,11 @@ The following example shows how to customize path matching in Java configuration @Override public void configurePathMatch(PathMatchConfigurer configurer) { configurer - .setUseSuffixPatternMatch(true) .setUseTrailingSlashMatch(false) .setUseRegisteredSuffixPatternMatch(true) .setPathMatcher(antPathMatcher()) .setUrlPathHelper(urlPathHelper()) - .addPathPrefix("/api", - HandlerTypePredicate.forAnnotation(RestController.class)); + .addPathPrefix("/api", HandlerTypePredicate.forAnnotation(RestController.class)); } @Bean @@ -5813,8 +5823,7 @@ The following example shows how to customize path matching in Java configuration .setUseRegisteredSuffixPatternMatch(true) .setPathMatcher(antPathMatcher()) .setUrlPathHelper(urlPathHelper()) - .addPathPrefix("/api", - HandlerTypePredicate.forAnnotation(RestController::class.java)) + .addPathPrefix("/api", HandlerTypePredicate.forAnnotation(RestController::class.java)) } @Bean @@ -5835,7 +5844,6 @@ The following example shows how to achieve the same configuration in XML: ----