From fbfb7a3c482443c39a74058446e5d1a9a2c73807 Mon Sep 17 00:00:00 2001 From: Rossen Stoyanchev Date: Wed, 27 Aug 2014 13:30:43 -0400 Subject: [PATCH] Add PathResourceResolver conditionally The ResourceHandlerRegistration now checks if the last resolver is an instance of PathResourceResolver and if so it skips adding it. This change also creates and adds the VersionResourceResolver (as well as CssLinkTransformer) the first time any VersionStrategy is registered. This ensures that custom resolvers (including an extension of PathResourceResolver) may be added both before and after the VersionResourceResolver. Lastly this change renames addVersion and addVersionHash to be consistent with addVersionStrategy. Issue: SPR-12124 --- .../ResourceHandlerRegistration.java | 102 +++++++++--------- .../resource/PathResourceResolver.java | 23 +++- .../resource/VersionResourceResolver.java | 16 ++- .../ResourceHandlerRegistryTests.java | 32 +++--- .../ResourceUrlProviderJavaConfigTests.java | 2 +- 5 files changed, 99 insertions(+), 76 deletions(-) diff --git a/spring-webmvc/src/main/java/org/springframework/web/servlet/config/annotation/ResourceHandlerRegistration.java b/spring-webmvc/src/main/java/org/springframework/web/servlet/config/annotation/ResourceHandlerRegistration.java index 478e8c6905..8173259073 100644 --- a/spring-webmvc/src/main/java/org/springframework/web/servlet/config/annotation/ResourceHandlerRegistration.java +++ b/spring-webmvc/src/main/java/org/springframework/web/servlet/config/annotation/ResourceHandlerRegistration.java @@ -63,14 +63,15 @@ public class ResourceHandlerRegistration { private List customResolvers = new ArrayList(); - private List customTransformers = new ArrayList(); + private VersionResourceResolver versionResolver; - private Map versionStrategies = new HashMap(); + private List customTransformers = new ArrayList(); private boolean isDevMode = false; private Cache resourceCache; + /** * Create a {@link ResourceHandlerRegistration} instance. * @param resourceLoader a resource loader for turning a String location into a {@link Resource} @@ -82,6 +83,7 @@ public class ResourceHandlerRegistration { this.pathPatterns = pathPatterns; } + /** * Add one or more resource locations from which to serve static content. Each location must point to a valid * directory. Multiple locations may be specified as a comma-separated list, and the locations will be checked @@ -150,25 +152,6 @@ public class ResourceHandlerRegistration { return this; } - /** - * Apply Resource Versioning on the matching resources; this will update resources' URLs to include - * a version string calculated by a {@link VersionStrategy}. This is often used for cache busting. - *

Note that a {@link CssLinkResourceTransformer} will be automatically registered to - * support versioned resources in CSS files.

- * @param strategy the versioning strategy to use - * @param pathPatterns one or more resource URL path patterns - * @return the same {@link ResourceHandlerRegistration} instance for chained method invocation - * @see VersionResourceResolver - * @see VersionStrategy - * @since 4.1 - */ - public ResourceHandlerRegistration addVersionStrategy(VersionStrategy strategy, String... pathPatterns) { - for(String pattern : pathPatterns) { - this.versionStrategies.put(pattern, strategy); - } - return this; - } - /** * Apply Resource Versioning on the matching resources using a {@link FixedVersionStrategy}. *

This strategy uses that fixed version string and adds it as a prefix in the resource path, @@ -187,7 +170,7 @@ public class ResourceHandlerRegistration { * @see FixedVersionStrategy * @since 4.1 */ - public ResourceHandlerRegistration addVersion(String fixedVersion, String... pathPatterns) { + public ResourceHandlerRegistration addFixedVersionStrategy(String fixedVersion, String... pathPatterns) { addVersionStrategy(new FixedVersionStrategy(fixedVersion), pathPatterns); return this; } @@ -204,11 +187,36 @@ public class ResourceHandlerRegistration { * @see ContentVersionStrategy * @since 4.1 */ - public ResourceHandlerRegistration addVersionHash(String... pathPatterns) { + public ResourceHandlerRegistration addContentVersionStrategy(String... pathPatterns) { addVersionStrategy(new ContentVersionStrategy(), pathPatterns); return this; } + + /** + * Apply Resource Versioning on the matching resources; this will update resources' URLs to include + * a version string calculated by a {@link VersionStrategy}. This is often used for cache busting. + *

Note that a {@link CssLinkResourceTransformer} will be automatically registered to + * support versioned resources in CSS files.

+ * @param strategy the versioning strategy to use + * @param pathPatterns one or more resource URL path patterns + * @return the same {@link ResourceHandlerRegistration} instance for chained method invocation + * @see VersionResourceResolver + * @see VersionStrategy + * @since 4.1 + */ + public ResourceHandlerRegistration addVersionStrategy(VersionStrategy strategy, String... pathPatterns) { + if (this.versionResolver == null) { + this.versionResolver = new VersionResourceResolver(); + this.customResolvers.add(this.versionResolver); + this.customTransformers.add(new CssLinkResourceTransformer()); + } + for(String pattern : pathPatterns) { + this.versionResolver.getVersionStrategyMap().put(pattern, strategy); + } + return this; + } + /** * Disable automatic registration of caching Resolver/Transformer, thus disabling {@code Resource} caching * if no caching Resolver/Transformer was manually registered. @@ -243,44 +251,32 @@ public class ResourceHandlerRegistration { } protected List getResourceResolvers() { - List resolvers = new ArrayList(); - boolean hasCachingResolver = false; - if(!this.customResolvers.isEmpty()) { - if(ClassUtils.isAssignable(CachingResourceResolver.class, this.customResolvers.get(0).getClass())) { - hasCachingResolver = true; - } + if (this.customResolvers.isEmpty()) { + return null; } - if(!hasCachingResolver && !this.isDevMode) { + List resolvers = new ArrayList(); + ResourceResolver first = this.customResolvers.get(0); + if (!ClassUtils.isAssignable(CachingResourceResolver.class, first.getClass()) && !this.isDevMode) { resolvers.add(new CachingResourceResolver(getDefaultResourceCache())); } resolvers.addAll(this.customResolvers); - if(!this.versionStrategies.isEmpty()) { - VersionResourceResolver versionResolver = new VersionResourceResolver(); - versionResolver.setStrategyMap(this.versionStrategies); - resolvers.add(versionResolver); + ResourceResolver last = this.customResolvers.get(this.customResolvers.size() - 1); + if (!ClassUtils.isAssignable(PathResourceResolver.class, last.getClass())) { + resolvers.add(new PathResourceResolver()); } - resolvers.add(new PathResourceResolver()); return resolvers; } protected List getResourceTransformers() { - List transformers = new ArrayList(); - boolean hasCachingTransformer = false; - if(!this.customTransformers.isEmpty() || !this.versionStrategies.isEmpty()) { - if(!this.customTransformers.isEmpty()) { - if(ClassUtils.isAssignable(CachingResourceTransformer.class, this.customTransformers.get(0).getClass())) { - hasCachingTransformer = true; - } - } - if(!hasCachingTransformer && !this.isDevMode) { - transformers.add(new CachingResourceTransformer(getDefaultResourceCache())); - } - transformers.addAll(this.customTransformers); - if(!this.versionStrategies.isEmpty()) { - int cssLinkTransformerPosition = (hasCachingTransformer || !this.isDevMode) ? 1 : 0; - transformers.add(cssLinkTransformerPosition, new CssLinkResourceTransformer()); - } + if (this.customTransformers.isEmpty()) { + return null; } + List transformers = new ArrayList(); + ResourceTransformer first = this.customTransformers.get(0); + if (!ClassUtils.isAssignable(CachingResourceTransformer.class, first.getClass()) && !this.isDevMode) { + transformers.add(new CachingResourceTransformer(getDefaultResourceCache())); + } + transformers.addAll(this.customTransformers); return transformers; } @@ -291,11 +287,11 @@ public class ResourceHandlerRegistration { Assert.isTrue(!CollectionUtils.isEmpty(locations), "At least one location is required for resource handling."); ResourceHttpRequestHandler requestHandler = new ResourceHttpRequestHandler(); List resourceResolvers = getResourceResolvers(); - if (!resourceResolvers.isEmpty()) { + if (!CollectionUtils.isEmpty(resourceResolvers)) { requestHandler.setResourceResolvers(resourceResolvers); } List resourceTransformers = getResourceTransformers(); - if (!resourceTransformers.isEmpty()) { + if (!CollectionUtils.isEmpty(resourceTransformers)) { requestHandler.setResourceTransformers(resourceTransformers); } requestHandler.setLocations(this.locations); diff --git a/spring-webmvc/src/main/java/org/springframework/web/servlet/resource/PathResourceResolver.java b/spring-webmvc/src/main/java/org/springframework/web/servlet/resource/PathResourceResolver.java index d3b4ea8040..0522cc3d8d 100644 --- a/spring-webmvc/src/main/java/org/springframework/web/servlet/resource/PathResourceResolver.java +++ b/spring-webmvc/src/main/java/org/springframework/web/servlet/resource/PathResourceResolver.java @@ -46,20 +46,20 @@ public class PathResourceResolver extends AbstractResourceResolver { } @Override - protected String resolveUrlPathInternal(String resourceUrlPath, List locations, + protected String resolveUrlPathInternal(String resourcePath, List locations, ResourceResolverChain chain) { - return (getResource(resourceUrlPath, locations) != null ? resourceUrlPath : null); + return (getResource(resourcePath, locations) != null ? resourcePath : null); } - private Resource getResource(String path, List locations) { + private Resource getResource(String resourcePath, List locations) { for (Resource location : locations) { try { if (logger.isTraceEnabled()) { logger.trace("Checking location=[" + location + "]"); } - Resource resource = location.createRelative(path); - if (resource.exists() && resource.isReadable()) { + Resource resource = getResource(resourcePath, location); + if (resource != null) { if (logger.isTraceEnabled()) { logger.trace("Found match"); } @@ -76,4 +76,17 @@ public class PathResourceResolver extends AbstractResourceResolver { return null; } + /** + * Find the resource under the given location. + *

The default implementation checks if there is a readable + * {@code Resource} for the given path relative to the location. + * @param resourcePath the path to the resource + * @param location the location to check + * @return the resource or {@code null} + */ + protected Resource getResource(String resourcePath, Resource location) throws IOException { + Resource resource = location.createRelative(resourcePath); + return (resource.exists() && resource.isReadable() ? resource : null); + } + } diff --git a/spring-webmvc/src/main/java/org/springframework/web/servlet/resource/VersionResourceResolver.java b/spring-webmvc/src/main/java/org/springframework/web/servlet/resource/VersionResourceResolver.java index bd972c15f0..13ed4016c4 100644 --- a/spring-webmvc/src/main/java/org/springframework/web/servlet/resource/VersionResourceResolver.java +++ b/spring-webmvc/src/main/java/org/springframework/web/servlet/resource/VersionResourceResolver.java @@ -48,7 +48,8 @@ public class VersionResourceResolver extends AbstractResourceResolver { private AntPathMatcher pathMatcher = new AntPathMatcher(); - private Map versionStrategyMap = Collections.emptyMap(); + /** Map from path pattern -> VersionStrategy */ + private final Map versionStrategyMap = new HashMap(); /** @@ -58,12 +59,19 @@ public class VersionResourceResolver extends AbstractResourceResolver { *

Supports direct URL matches and Ant-style pattern matches. For syntax * details, see the {@link org.springframework.util.AntPathMatcher} javadoc. * - * @param versionStrategyMap map with URLs as keys and version strategies as values + * @param map map with URLs as keys and version strategies as values */ - public void setStrategyMap(Map versionStrategyMap) { - this.versionStrategyMap = versionStrategyMap; + public void setStrategyMap(Map map) { + this.versionStrategyMap.clear(); + this.versionStrategyMap.putAll(map); } + /** + * Return the map with version strategies keyed by path pattern. + */ + public Map getVersionStrategyMap() { + return this.versionStrategyMap; + } @Override protected Resource resolveResourceInternal(HttpServletRequest request, String requestPath, diff --git a/spring-webmvc/src/test/java/org/springframework/web/servlet/config/annotation/ResourceHandlerRegistryTests.java b/spring-webmvc/src/test/java/org/springframework/web/servlet/config/annotation/ResourceHandlerRegistryTests.java index 00f18904d5..e209a1df59 100644 --- a/spring-webmvc/src/test/java/org/springframework/web/servlet/config/annotation/ResourceHandlerRegistryTests.java +++ b/spring-webmvc/src/test/java/org/springframework/web/servlet/config/annotation/ResourceHandlerRegistryTests.java @@ -109,14 +109,13 @@ public class ResourceHandlerRegistryTests { @Test public void simpleResourceChain() throws Exception { - ResourceResolver mockResolver = Mockito.mock(ResourceResolver.class); ResourceTransformer mockTransformer = Mockito.mock(ResourceTransformer.class); this.registration.addResolver(mockResolver).addTransformer(mockTransformer); ResourceHttpRequestHandler handler = getHandler("/resources/**"); List resolvers = handler.getResourceResolvers(); - assertThat(resolvers, Matchers.hasSize(3)); + assertThat(resolvers.toString(), resolvers, Matchers.hasSize(3)); assertThat(resolvers.get(0), Matchers.instanceOf(CachingResourceResolver.class)); CachingResourceResolver cachingResolver = (CachingResourceResolver) resolvers.get(0); assertThat(cachingResolver.getCache(), Matchers.instanceOf(ConcurrentMapCache.class)); @@ -146,15 +145,16 @@ public class ResourceHandlerRegistryTests { public void versionResourceChain() throws Exception { this.registration .addTransformer(new AppCacheManifestTransfomer()) - .addVersion("fixed", "/**/*.js") - .addVersionHash("/**"); + .addFixedVersionStrategy("fixed", "/**/*.js") + .addContentVersionStrategy("/**"); ResourceHttpRequestHandler handler = getHandler("/resources/**"); List resolvers = handler.getResourceResolvers(); - assertThat(resolvers, Matchers.hasSize(3)); + assertThat(resolvers.toString(), resolvers, Matchers.hasSize(3)); assertThat(resolvers.get(0), Matchers.instanceOf(CachingResourceResolver.class)); assertThat(resolvers.get(1), Matchers.instanceOf(VersionResourceResolver.class)); DirectFieldAccessor fieldAccessor = new DirectFieldAccessor(resolvers.get(1)); + @SuppressWarnings("unchecked") Map strategies = (Map) fieldAccessor.getPropertyValue("versionStrategyMap"); assertNotNull(strategies.get("/**/*.js")); @@ -164,8 +164,8 @@ public class ResourceHandlerRegistryTests { List transformers = handler.getResourceTransformers(); assertThat(transformers, Matchers.hasSize(3)); assertThat(transformers.get(0), Matchers.instanceOf(CachingResourceTransformer.class)); - assertThat(transformers.get(1), Matchers.instanceOf(CssLinkResourceTransformer.class)); - assertThat(transformers.get(2), Matchers.instanceOf(AppCacheManifestTransfomer.class)); + assertThat(transformers.get(1), Matchers.instanceOf(AppCacheManifestTransfomer.class)); + assertThat(transformers.get(2), Matchers.instanceOf(CssLinkResourceTransformer.class)); } @Test @@ -176,22 +176,23 @@ public class ResourceHandlerRegistryTests { .addTransformer(cachingTransformer) .addTransformer(new AppCacheManifestTransfomer()) .addResolver(cachingResolver) - .addVersion("fixed", "/**/*.js") - .addVersionHash("/**") + .addFixedVersionStrategy("fixed", "/**/*.js") + .addContentVersionStrategy("/**") + .addResolver(new CustomPathResourceResolver()) .setCachePeriod(3600); ResourceHttpRequestHandler handler = getHandler("/resources/**"); List resolvers = handler.getResourceResolvers(); - assertThat(resolvers, Matchers.hasSize(3)); + assertThat(resolvers.toString(), resolvers, Matchers.hasSize(3)); assertThat(resolvers.get(0), Matchers.equalTo(cachingResolver)); assertThat(resolvers.get(1), Matchers.instanceOf(VersionResourceResolver.class)); - assertThat(resolvers.get(2), Matchers.instanceOf(PathResourceResolver.class)); + assertThat(resolvers.get(2), Matchers.instanceOf(CustomPathResourceResolver.class)); List transformers = handler.getResourceTransformers(); assertThat(transformers, Matchers.hasSize(3)); assertThat(transformers.get(0), Matchers.equalTo(cachingTransformer)); - assertThat(transformers.get(1), Matchers.instanceOf(CssLinkResourceTransformer.class)); - assertThat(transformers.get(2), Matchers.instanceOf(AppCacheManifestTransfomer.class)); + assertThat(transformers.get(1), Matchers.instanceOf(AppCacheManifestTransfomer.class)); + assertThat(transformers.get(2), Matchers.instanceOf(CssLinkResourceTransformer.class)); } private ResourceHttpRequestHandler getHandler(String pathPattern) { @@ -199,4 +200,9 @@ public class ResourceHandlerRegistryTests { return (ResourceHttpRequestHandler) handlerMapping.getUrlMap().get(pathPattern); } + + private static class CustomPathResourceResolver extends PathResourceResolver { + + } + } diff --git a/spring-webmvc/src/test/java/org/springframework/web/servlet/resource/ResourceUrlProviderJavaConfigTests.java b/spring-webmvc/src/test/java/org/springframework/web/servlet/resource/ResourceUrlProviderJavaConfigTests.java index f4a8ee5b1d..4c0ae115c0 100644 --- a/spring-webmvc/src/test/java/org/springframework/web/servlet/resource/ResourceUrlProviderJavaConfigTests.java +++ b/spring-webmvc/src/test/java/org/springframework/web/servlet/resource/ResourceUrlProviderJavaConfigTests.java @@ -113,7 +113,7 @@ public class ResourceUrlProviderJavaConfigTests { public void addResourceHandlers(ResourceHandlerRegistry registry) { registry.addResourceHandler("/resources/**") .addResourceLocations("classpath:org/springframework/web/servlet/resource/test/") - .addVersionHash("/**"); + .addContentVersionStrategy("/**"); } }