Move builder methods to VersionResourceResolver

This change moves the VersionStrategy builder-style methods from
ResourceHandlerRegistration to VersionResourceResolver.

This makes the methods more universally usable and also makes use of
ResourceHandlerRegistration more readable, i.e. simply a sequence of
addResource and addTransformer calls.

spring-webmvc/src/main/java/org/springframework/web/servlet/config/annotation/ResourceHandlerRegistration.java

@@ -17,9 +17,7 @@
 package org.springframework.web.servlet.config.annotation;
 
 import java.util.ArrayList;
-import java.util.HashMap;
 import java.util.List;
-import java.util.Map;
 
 import org.springframework.cache.Cache;
 import org.springframework.cache.concurrent.ConcurrentMapCache;
@@ -63,10 +61,12 @@ public class ResourceHandlerRegistration {
 
 	private List<ResourceResolver> customResolvers = new ArrayList<ResourceResolver>();
 
-	private VersionResourceResolver versionResolver;
-
 	private List<ResourceTransformer> customTransformers = new ArrayList<ResourceTransformer>();
 
+	private boolean hasVersionResolver;
+
+	private boolean hasCssLinkTransformer;
+
 	private boolean isDevMode = false;
 
 	private Cache resourceCache;
@@ -123,6 +123,9 @@ public class ResourceHandlerRegistration {
 	public ResourceHandlerRegistration addResolver(ResourceResolver resolver) {
 		Assert.notNull(resolver, "The provided ResourceResolver should not be null");
 		this.customResolvers.add(resolver);
+		if (resolver instanceof VersionResourceResolver) {
+			this.hasVersionResolver = true;
+		}
 		return this;
 	}
 
@@ -149,70 +152,8 @@ public class ResourceHandlerRegistration {
 	public ResourceHandlerRegistration addTransformer(ResourceTransformer transformer) {
 		Assert.notNull(transformer, "The provided ResourceTransformer should not be null");
 		this.customTransformers.add(transformer);
-		return this;
-	}
-
-	/**
-	 * Apply Resource Versioning on the matching resources using a {@link FixedVersionStrategy}.
-	 * <p>This strategy uses that fixed version string and adds it as a prefix in the resource path,
-	 * e.g. {@code fixedversion/js/main.js}.</p>
-	 * <p>There are many ways to get a version string for your application:</p>
-	 * <ul>
-	 *     <li>create a string using the current date, a source of random numbers at runtime</li>
-	 *     <li>fetch a version string from a property source or an Env variable, using SpEL or @Value</li>
-	 * </ul>
-	 * <p>Note that a {@link CssLinkResourceTransformer} will be automatically registered to
-	 * support versioned resources in CSS files.</p>
-	 * @param fixedVersion a version string
-	 * @param pathPatterns one or more resource URL path patterns
-	 * @return the same {@link ResourceHandlerRegistration} instance for chained method invocation
-	 * @see VersionResourceResolver
-	 * @see FixedVersionStrategy
-	 * @since 4.1
-	 */
-	public ResourceHandlerRegistration addFixedVersionStrategy(String fixedVersion, String... pathPatterns) {
-		addVersionStrategy(new FixedVersionStrategy(fixedVersion), pathPatterns);
-		return this;
-	}
-
-	/**
-	 * Apply Resource Versioning on the matching resources using a {@link ContentVersionStrategy}.
-	 * <p>This strategy uses the content of the Resource to create a String hash and adds it
-	 * in the resource filename, e.g. {@code css/main-e36d2e05253c6c7085a91522ce43a0b4.css}.</p>
-	 * <p>Note that a {@link CssLinkResourceTransformer} will be automatically registered to
-	 * support versioned resources in CSS files.</p>
-	 * @param pathPatterns one or more resource URL path patterns
-	 * @return the same {@link ResourceHandlerRegistration} instance for chained method invocation
-	 * @see VersionResourceResolver
-	 * @see ContentVersionStrategy
-	 * @since 4.1
-	 */
-	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.
-	 * <p>Note that a {@link CssLinkResourceTransformer} will be automatically registered to
-	 * support versioned resources in CSS files.</p>
-	 * @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);
+		if (transformer instanceof CssLinkResourceTransformer) {
+			this.hasCssLinkTransformer = true;
 		}
 		return this;
 	}
@@ -273,10 +214,20 @@ public class ResourceHandlerRegistration {
 		}
 		List<ResourceTransformer> transformers = new ArrayList<ResourceTransformer>();
 		ResourceTransformer first = this.customTransformers.get(0);
-		if (!ClassUtils.isAssignable(CachingResourceTransformer.class, first.getClass()) && !this.isDevMode) {
-			transformers.add(new CachingResourceTransformer(getDefaultResourceCache()));
+		ResourceTransformer cachingTransformer = null;
+		if (!this.isDevMode) {
+			if (ClassUtils.isAssignable(CachingResourceTransformer.class, first.getClass())) {
+				cachingTransformer = first;
+			}
+			else {
+				cachingTransformer = new CachingResourceTransformer(getDefaultResourceCache());
+				transformers.add(cachingTransformer);
+			}
 		}
 		transformers.addAll(this.customTransformers);
+		if (this.hasVersionResolver && !this.hasCssLinkTransformer) {
+			transformers.add(cachingTransformer != null ? 1 : 0, new CssLinkResourceTransformer());
+		}
 		return transformers;
 	}
 

spring-webmvc/src/main/java/org/springframework/web/servlet/resource/VersionResourceResolver.java

@@ -24,23 +24,26 @@ import org.springframework.util.AntPathMatcher;
 import org.springframework.util.StringUtils;
 
 /**
- * A {@code ResourceResolver} that resolves request paths containing a version
- * string, i.e. version information about the resource being requested.
- * This resolver can be useful to set up HTTP caching strategies by changing
- * resources' URLs as they are updated.
+ * Resolves request paths containing a version string that can be used as part
+ * of an HTTP caching strategy in which a resource is cached with a far future
+ * date (e.g. 1 year) and cached until the version, and therefore the URL, is
+ * changed.
  *
- * <p>Because resource versioning depends on the resource types, this {@code ResourceResolver}
- * needs to be configured with at least one {@link VersionStrategy}. The process of matching
- * and generating version strings is delegated to the {@code VersionStrategy}.
+ * <p>Different versioning strategies exist and this resolver must be configured
+ * with one or more such strategies along with path mappings to indicate which
+ * strategy applies to which resources.
  *
- * <p>When resolving resources, this resolver will first delegate to the chain to locate
- * an existing resource and then attempt to extract a version string from the request path
- * and then find a resource that matches that version.
+ * <p>{@code ContentVersionStrategy} is a good default choice except in cases
+ * where it cannot be used. Most notably the {@code ContentVersionStrategy}
+ * cannot be combined with JavaScript module loaders. For such cases the
+ * {@code FixedVersionStrategy} is a better choice.
  *
- * <p>When resolving URLs, this resolver will, if necessary, add a version string in the
- * request path.
+ * <p>Note that using this resolver to serve CSS files means the
+ * {@link CssLinkResourceTransformer} should also be used in order to modify
+ * links within CSS files to also contain versions.
  *
  * @author Brian Clozel
+ * @author Rossen Stoyanchev
  * @since 4.1
  * @see VersionStrategy
  */
@@ -69,10 +72,59 @@ public class VersionResourceResolver extends AbstractResourceResolver {
 	/**
 	 * Return the map with version strategies keyed by path pattern.
 	 */
-	public Map<String, VersionStrategy> getVersionStrategyMap() {
+	public Map<String, VersionStrategy> getStrategyMap() {
 		return this.versionStrategyMap;
 	}
 
+	/**
+	 * Insert a content-based version in resource URLs that match the given path
+	 * patterns. The version is computed from the content of the file, e.g.
+	 * {@code "css/main-e36d2e05253c6c7085a91522ce43a0b4.css"}. This is a good
+	 * default strategy to use except when it cannot be, for example when using
+	 * JavaScript module loaders, use {@link #addFixedVersionStrategy} instead
+	 * for serving JavaScript files.
+	 * @param pathPatterns one or more resource URL path patterns
+	 * @return the current instance for chained method invocation
+	 * @see ContentVersionStrategy
+	 */
+	public VersionResourceResolver addContentVersionStrategy(String... pathPatterns) {
+		addVersionStrategy(new ContentVersionStrategy(), pathPatterns);
+		return this;
+	}
+
+	/**
+	 * Insert a fixed, prefix-based version in resource URLs that match the given
+	 * path patterns, e.g. {@code "{version}/js/main.js"}. This is useful (vs
+	 * content-based versions) when using JavaScript module loaders.
+	 * <p>The version may be a random number, the current date, fetched from a
+	 * git commit sha, a property file, environment variable, and set with SpEL
+	 * expressions in the configuration (e.g. see {@code @Value} in Java config).
+	 * @param version a version string
+	 * @param pathPatterns one or more resource URL path patterns
+	 * @return the current instance for chained method invocation
+	 * @see FixedVersionStrategy
+	 */
+	public VersionResourceResolver addFixedVersionStrategy(String version, String... pathPatterns) {
+		addVersionStrategy(new FixedVersionStrategy(version), pathPatterns);
+		return this;
+	}
+
+	/**
+	 * Register a custom VersionStrategy to apply to resource URLs that match the
+	 * given path patterns.
+	 * @param strategy the custom strategy
+	 * @param pathPatterns one or more resource URL path patterns
+	 * @return the current instance for chained method invocation
+	 * @see VersionStrategy
+	 */
+	public VersionResourceResolver addVersionStrategy(VersionStrategy strategy, String... pathPatterns) {
+		for(String pattern : pathPatterns) {
+			getStrategyMap().put(pattern, strategy);
+		}
+		return this;
+	}
+
+
 	@Override
 	protected Resource resolveResourceInternal(HttpServletRequest request, String requestPath,
 			List<? extends Resource> locations, ResourceResolverChain chain) {

spring-webmvc/src/test/java/org/springframework/web/servlet/config/annotation/ResourceHandlerRegistryTests.java

@@ -143,11 +143,12 @@ public class ResourceHandlerRegistryTests {
 
 	@Test
 	public void versionResourceChain() throws Exception {
-		this.registration
-				.addTransformer(new AppCacheManifestTransfomer())
+		VersionResourceResolver versionResolver = new VersionResourceResolver()
 				.addFixedVersionStrategy("fixed", "/**/*.js")
 				.addContentVersionStrategy("/**");
 
+		this.registration.addResolver(versionResolver).addTransformer(new AppCacheManifestTransfomer());
+
 		ResourceHttpRequestHandler handler = getHandler("/resources/**");
 		List<ResourceResolver> resolvers = handler.getResourceResolvers();
 		assertThat(resolvers.toString(), resolvers, Matchers.hasSize(3));
@@ -164,21 +165,24 @@ public class ResourceHandlerRegistryTests {
 		List<ResourceTransformer> transformers = handler.getResourceTransformers();
 		assertThat(transformers, Matchers.hasSize(3));
 		assertThat(transformers.get(0), Matchers.instanceOf(CachingResourceTransformer.class));
-		assertThat(transformers.get(1), Matchers.instanceOf(AppCacheManifestTransfomer.class));
-		assertThat(transformers.get(2), Matchers.instanceOf(CssLinkResourceTransformer.class));
+		assertThat(transformers.get(1), Matchers.instanceOf(CssLinkResourceTransformer.class));
+		assertThat(transformers.get(2), Matchers.instanceOf(AppCacheManifestTransfomer.class));
 	}
 
 	@Test
 	public void customResourceChain() throws Exception {
+		VersionResourceResolver versionResolver = new VersionResourceResolver()
+				.addFixedVersionStrategy("fixed", "/**/*.js")
+				.addContentVersionStrategy("/**");
+
 		CachingResourceResolver cachingResolver = Mockito.mock(CachingResourceResolver.class);
 		CachingResourceTransformer cachingTransformer = Mockito.mock(CachingResourceTransformer.class);
 		this.registration
-				.addTransformer(cachingTransformer)
-				.addTransformer(new AppCacheManifestTransfomer())
 				.addResolver(cachingResolver)
-				.addFixedVersionStrategy("fixed", "/**/*.js")
-				.addContentVersionStrategy("/**")
+				.addResolver(versionResolver)
 				.addResolver(new CustomPathResourceResolver())
+				.addTransformer(cachingTransformer)
+				.addTransformer(new AppCacheManifestTransfomer())
 				.setCachePeriod(3600);
 
 		ResourceHttpRequestHandler handler = getHandler("/resources/**");
@@ -191,8 +195,8 @@ public class ResourceHandlerRegistryTests {
 		List<ResourceTransformer> transformers = handler.getResourceTransformers();
 		assertThat(transformers, Matchers.hasSize(3));
 		assertThat(transformers.get(0), Matchers.equalTo(cachingTransformer));
-		assertThat(transformers.get(1), Matchers.instanceOf(AppCacheManifestTransfomer.class));
-		assertThat(transformers.get(2), Matchers.instanceOf(CssLinkResourceTransformer.class));
+		assertThat(transformers.get(1), Matchers.instanceOf(CssLinkResourceTransformer.class));
+		assertThat(transformers.get(2), Matchers.instanceOf(AppCacheManifestTransfomer.class));
 	}
 
 	private ResourceHttpRequestHandler getHandler(String pathPattern) {

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/")
-				.addContentVersionStrategy("/**");
+				.addResolver(new VersionResourceResolver().addContentVersionStrategy("/**"));
 		}
 	}