Updated Branches: refs/heads/master bb255c0a1 -> e16e5fb50
TAP5-1007: When Tapestry is loading templates or other files on case-insensitive OSs (Windows) it should trigger an error if the file name case is incorrect (which will result in a runtime failure on case-sensitive OSs, such as Linux) Project: http://git-wip-us.apache.org/repos/asf/tapestry-5/repo Commit: http://git-wip-us.apache.org/repos/asf/tapestry-5/commit/e16e5fb5 Tree: http://git-wip-us.apache.org/repos/asf/tapestry-5/tree/e16e5fb5 Diff: http://git-wip-us.apache.org/repos/asf/tapestry-5/diff/e16e5fb5 Branch: refs/heads/master Commit: e16e5fb50555545e36960767568ac9520322fc9b Parents: bb255c0 Author: Howard M. Lewis Ship <[email protected]> Authored: Mon Aug 12 18:29:19 2013 -0700 Committer: Howard M. Lewis Ship <[email protected]> Committed: Mon Aug 12 18:29:19 2013 -0700 ---------------------------------------------------------------------- .../internal/services/AssetSourceImpl.java | 121 +++++++++++-------- .../internal/services/ContextResource.java | 11 +- .../ContextResourceSymbolProviderTest.java | 10 +- .../internal/services/AssetSourceImplTest.java | 20 +-- .../internal/services/ContextResourceTest.java | 12 +- .../ioc/internal/util/AbstractResource.java | 73 +++++++++++ .../ioc/internal/util/ClasspathResource.java | 7 +- .../ioc/specs/ClasspathResourceSpec.groovy | 14 +++ 8 files changed, 192 insertions(+), 76 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/tapestry-5/blob/e16e5fb5/tapestry-core/src/main/java/org/apache/tapestry5/internal/services/AssetSourceImpl.java ---------------------------------------------------------------------- diff --git a/tapestry-core/src/main/java/org/apache/tapestry5/internal/services/AssetSourceImpl.java b/tapestry-core/src/main/java/org/apache/tapestry5/internal/services/AssetSourceImpl.java index ac83ddb..9bcc065 100644 --- a/tapestry-core/src/main/java/org/apache/tapestry5/internal/services/AssetSourceImpl.java +++ b/tapestry-core/src/main/java/org/apache/tapestry5/internal/services/AssetSourceImpl.java @@ -19,6 +19,8 @@ import org.apache.tapestry5.ComponentResources; import org.apache.tapestry5.internal.AssetConstants; import org.apache.tapestry5.internal.TapestryInternalUtils; import org.apache.tapestry5.internal.services.assets.ResourceChangeTracker; +import org.apache.tapestry5.ioc.Invokable; +import org.apache.tapestry5.ioc.OperationTracker; import org.apache.tapestry5.ioc.Resource; import org.apache.tapestry5.ioc.annotations.PostInjection; import org.apache.tapestry5.ioc.internal.util.CollectionFactory; @@ -54,13 +56,16 @@ public class AssetSourceImpl extends LockSupport implements AssetSource private final AtomicBoolean firstWarning = new AtomicBoolean(true); + private final OperationTracker tracker; + public AssetSourceImpl(ThreadLocale threadLocale, - Map<String, AssetFactory> configuration, SymbolSource symbolSource, Logger logger) + Map<String, AssetFactory> configuration, SymbolSource symbolSource, Logger logger, OperationTracker tracker) { this.threadLocale = threadLocale; this.symbolSource = symbolSource; this.logger = logger; + this.tracker = tracker; Map<Class, AssetFactory> byResourceClass = CollectionFactory.newMap(); @@ -80,7 +85,8 @@ public class AssetSourceImpl extends LockSupport implements AssetSource } @PostInjection - public void clearCacheWhenResourcesChange(ResourceChangeTracker tracker) { + public void clearCacheWhenResourcesChange(ResourceChangeTracker tracker) + { tracker.clearOnInvalidation(cache); } @@ -114,79 +120,88 @@ public class AssetSourceImpl extends LockSupport implements AssetSource return getUnlocalizedAsset(symbolSource.expandSymbols(path)); } - public Asset getComponentAsset(ComponentResources resources, String path) + public Asset getComponentAsset(final ComponentResources resources, final String path) { - assert resources != null; + assert resources != null; assert InternalUtils.isNonBlank(path); - // First, expand symbols: + return tracker.invoke(String.format("Resolving '%s' for component %s", path, resources.getCompleteId() + ), + new Invokable<Asset>() + { + public Asset invoke() + { + // First, expand symbols: - String expanded = symbolSource.expandSymbols(path); + String expanded = symbolSource.expandSymbols(path); - int dotx = expanded.indexOf(':'); + int dotx = expanded.indexOf(':'); - // We special case the hell out of 'classpath:' so that we can provide warnings today (5.4) and - // blow up in a useful fashion tomorrow (5.5). + // We special case the hell out of 'classpath:' so that we can provide warnings today (5.4) and + // blow up in a useful fashion tomorrow (5.5). - if (dotx > 0 && !expanded.substring(0, dotx).equalsIgnoreCase(AssetConstants.CLASSPATH)) - { - return getAssetInLocale(resources.getBaseResource(), expanded, resources.getLocale()); - } + if (dotx > 0 && !expanded.substring(0, dotx).equalsIgnoreCase(AssetConstants.CLASSPATH)) + { + return getAssetInLocale(resources.getBaseResource(), expanded, resources.getLocale()); + } - // No prefix, so implicitly classpath:, or explicitly classpath: + // No prefix, so implicitly classpath:, or explicitly classpath: - String restOfPath = expanded.substring(dotx + 1); + String restOfPath = expanded.substring(dotx + 1); - // This is tricky, because a relative path (including "../") is ok in 5.3, since its just somewhere - // else on the classpath (though you can "stray" out of the "safe" zone). In 5.4, under /META-INF/assets/ - // it's possible to "stray" out beyond the safe zone more easily, into parts of the classpath that can't be - // represented in the URL. + // This is tricky, because a relative path (including "../") is ok in 5.3, since its just somewhere + // else on the classpath (though you can "stray" out of the "safe" zone). In 5.4, under /META-INF/assets/ + // it's possible to "stray" out beyond the safe zone more easily, into parts of the classpath that can't be + // represented in the URL. + // Ends with trailing slash: + String metaRoot = "META-INF/assets/" + toPathPrefix(resources.getComponentModel().getLibraryName()); - // Ends with trailing slash: - String metaRoot = "META-INF/assets/" + toPathPrefix(resources.getComponentModel().getLibraryName()); + String metaPath = metaRoot + (restOfPath.startsWith("/") + ? restOfPath.substring(1) + : restOfPath); - String metaPath = metaRoot + (restOfPath.startsWith("/") - ? restOfPath.substring(1) - : restOfPath); + // Based on the path, metaResource is where it should exist in a 5.4 and beyond world ... unless the expanded + // path was a bit too full of ../ sequences, in which case the expanded path is not valid and we adjust the + // error we write. - // Based on the path, metaResource is where it should exist in a 5.4 and beyond world ... unless the expanded - // path was a bit too full of ../ sequences, in which case the expanded path is not valid and we adjust the - // error we write. + Resource metaResource = findLocalizedResource(null, metaPath, resources.getLocale()); - Resource metaResource = findLocalizedResource(null, metaPath, resources.getLocale()); + Asset result = getComponentAsset(resources, expanded, metaResource); - Asset result = getComponentAsset(resources, expanded, metaResource); + // This is the best way to tell if the result is an asset for a Classpath resource. - // This is the best way to tell if the result is an asset for a Classpath resource. + Resource resultResource = result.getResource(); - Resource resultResource = result.getResource(); + if (!resultResource.equals(metaResource)) + { + if (firstWarning.getAndSet(false)) + { + logger.error("Packaging of classpath assets has changed in release 5.4; " + + "Assets should no longer be on the main classpath, " + + "but should be moved to 'META-INF/assets/' or a sub-folder. Future releases of Tapestry may " + + "no longer support assets on the main classpath."); + } - if (!resultResource.equals(metaResource)) - { - if (firstWarning.getAndSet(false)) - { - logger.error("Packaging of classpath assets has changed in release 5.4; " + - "Assets should no longer be on the main classpath, " + - "but should be moved to 'META-INF/assets/' or a sub-folder. Future releases of Tapestry may " + - "no longer support assets on the main classpath."); - } + if (metaResource.getFolder().startsWith(metaRoot)) + { + logger.error(String.format("Classpath asset '/%s' should be moved to folder '/%s/'.", + resultResource.getPath(), + metaResource.getFolder())); + } else + { + logger.error(String.format("Classpath asset '/%s' should be moved under folder '/%s', and the relative path adjusted.", + resultResource.getPath(), + metaRoot)); + } + } - if (metaResource.getFolder().startsWith(metaRoot)) - { - logger.error(String.format("Classpath asset '/%s' should be moved to folder '/%s/'.", - resultResource.getPath(), - metaResource.getFolder())); - } else - { - logger.error(String.format("Classpath asset '/%s' should be moved under folder '/%s', and the relative path adjusted.", - resultResource.getPath(), - metaRoot)); - } - } + return result; + } + } - return result; + ); } private Asset getComponentAsset(ComponentResources resources, String expandedPath, Resource metaResource) http://git-wip-us.apache.org/repos/asf/tapestry-5/blob/e16e5fb5/tapestry-core/src/main/java/org/apache/tapestry5/internal/services/ContextResource.java ---------------------------------------------------------------------- diff --git a/tapestry-core/src/main/java/org/apache/tapestry5/internal/services/ContextResource.java b/tapestry-core/src/main/java/org/apache/tapestry5/internal/services/ContextResource.java index f5337b2..26f22a4 100644 --- a/tapestry-core/src/main/java/org/apache/tapestry5/internal/services/ContextResource.java +++ b/tapestry-core/src/main/java/org/apache/tapestry5/internal/services/ContextResource.java @@ -1,4 +1,4 @@ -// Copyright 2006, 2008, 2010, 2012 The Apache Software Foundation +// Copyright 2006-2013 The Apache Software Foundation // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -63,6 +63,7 @@ public class ContextResource extends AbstractResource try { acquireReadLock(); + if (!urlResolved) { resolveURL(); @@ -104,7 +105,11 @@ public class ContextResource extends AbstractResource try { url = file.toURI().toURL(); + + validateURL(url); + urlResolved = true; + return; } catch (MalformedURLException ex) { @@ -116,8 +121,12 @@ public class ContextResource extends AbstractResource // URL we get ... but reloading won't work. url = context.getResource(contextPath); + + validateURL(url); + urlResolved = true; + } finally { downgradeWriteLockToReadLock(); http://git-wip-us.apache.org/repos/asf/tapestry-5/blob/e16e5fb5/tapestry-core/src/test/java/org/apache/tapestry5/internal/ContextResourceSymbolProviderTest.java ---------------------------------------------------------------------- diff --git a/tapestry-core/src/test/java/org/apache/tapestry5/internal/ContextResourceSymbolProviderTest.java b/tapestry-core/src/test/java/org/apache/tapestry5/internal/ContextResourceSymbolProviderTest.java index 483658e..b9a772b 100644 --- a/tapestry-core/src/test/java/org/apache/tapestry5/internal/ContextResourceSymbolProviderTest.java +++ b/tapestry-core/src/test/java/org/apache/tapestry5/internal/ContextResourceSymbolProviderTest.java @@ -1,4 +1,4 @@ -// Copyright 2009 The Apache Software Foundation +// Copyright 2009-2013 The Apache Software Foundation // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -25,8 +25,6 @@ public class ContextResourceSymbolProviderTest extends InternalBaseTestCase { private static final String CONTENT = "homer=simpson\r\nmonty=burns"; - private static final String PATH = "bar/foo.properties"; - @Test public void access() throws Exception { @@ -36,11 +34,11 @@ public class ContextResourceSymbolProviderTest extends InternalBaseTestCase Context context = mockContext(); - expect(context.getRealFile("/" + PATH)).andReturn(f); + expect(context.getRealFile("/bar/" + f.getName())).andReturn(f); replay(); - ContextResourceSymbolProvider provider = new ContextResourceSymbolProvider(context, PATH); + ContextResourceSymbolProvider provider = new ContextResourceSymbolProvider(context, "bar/" + f.getName()); /* test general access */ assertEquals(provider.valueForSymbol("homer"), "simpson"); @@ -64,7 +62,5 @@ public class ContextResourceSymbolProviderTest extends InternalBaseTestCase fos.write(CONTENT.getBytes()); fos.close(); - - fos = null; } } http://git-wip-us.apache.org/repos/asf/tapestry-5/blob/e16e5fb5/tapestry-core/src/test/java/org/apache/tapestry5/internal/services/AssetSourceImplTest.java ---------------------------------------------------------------------- diff --git a/tapestry-core/src/test/java/org/apache/tapestry5/internal/services/AssetSourceImplTest.java b/tapestry-core/src/test/java/org/apache/tapestry5/internal/services/AssetSourceImplTest.java index 904217e..bf57ab9 100644 --- a/tapestry-core/src/test/java/org/apache/tapestry5/internal/services/AssetSourceImplTest.java +++ b/tapestry-core/src/test/java/org/apache/tapestry5/internal/services/AssetSourceImplTest.java @@ -1,4 +1,4 @@ -// Copyright 2006, 2007, 2008, 2010, 2012 The Apache Software Foundation +// Copyright 2006-2013 The Apache Software Foundation // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -16,7 +16,9 @@ package org.apache.tapestry5.internal.services; import org.apache.tapestry5.Asset; import org.apache.tapestry5.internal.test.InternalBaseTestCase; +import org.apache.tapestry5.ioc.OperationTracker; import org.apache.tapestry5.ioc.Resource; +import org.apache.tapestry5.ioc.internal.QuietOperationTracker; import org.apache.tapestry5.ioc.internal.util.ClasspathResource; import org.apache.tapestry5.ioc.services.SymbolSource; import org.apache.tapestry5.ioc.services.ThreadLocale; @@ -35,6 +37,8 @@ public class AssetSourceImplTest extends InternalBaseTestCase private final Resource rootResource = new ClasspathResource("/"); + private final OperationTracker tracker = new QuietOperationTracker(); + @Test public void relative_asset() { @@ -52,7 +56,7 @@ public class AssetSourceImplTest extends InternalBaseTestCase replay(); - AssetSource source = new AssetSourceImpl(threadLocale, configuration, null, null); + AssetSource source = new AssetSourceImpl(threadLocale, configuration, null, null, tracker); // First try creates it: @@ -82,7 +86,7 @@ public class AssetSourceImplTest extends InternalBaseTestCase replay(); - AssetSource source = new AssetSourceImpl(threadLocale, configuration, null, null); + AssetSource source = new AssetSourceImpl(threadLocale, configuration, null, null, tracker); // First try creates it: @@ -112,7 +116,7 @@ public class AssetSourceImplTest extends InternalBaseTestCase replay(); - AssetSource source = new AssetSourceImpl(null, configuration, symbolSource, null); + AssetSource source = new AssetSourceImpl(null, configuration, symbolSource, null, tracker); // First try creates it: @@ -142,7 +146,7 @@ public class AssetSourceImplTest extends InternalBaseTestCase replay(); - AssetSource source = new AssetSourceImpl(threadLocale, configuration, null, null); + AssetSource source = new AssetSourceImpl(threadLocale, configuration, null, null, tracker); assertSame(source.getClasspathAsset("org/apache/tapestry5/internal/services/SimpleComponent.properties"), asset); @@ -167,7 +171,7 @@ public class AssetSourceImplTest extends InternalBaseTestCase replay(); - AssetSource source = new AssetSourceImpl(threadLocale, configuration, null, null); + AssetSource source = new AssetSourceImpl(threadLocale, configuration, null, null, tracker); assertSame(source.getAsset(baseResource, "classpath:org/apache/tapestry5/internal/services/SimpleComponent.properties", Locale.UK), asset); @@ -189,7 +193,7 @@ public class AssetSourceImplTest extends InternalBaseTestCase replay(); - AssetSource source = new AssetSourceImpl(threadLocale, configuration, null, null); + AssetSource source = new AssetSourceImpl(threadLocale, configuration, null, null, tracker); try { @@ -214,7 +218,7 @@ public class AssetSourceImplTest extends InternalBaseTestCase replay(); - AssetSource source = new AssetSourceImpl(threadLocale, configuration, null, null); + AssetSource source = new AssetSourceImpl(threadLocale, configuration, null, null, tracker); try { http://git-wip-us.apache.org/repos/asf/tapestry-5/blob/e16e5fb5/tapestry-core/src/test/java/org/apache/tapestry5/internal/services/ContextResourceTest.java ---------------------------------------------------------------------- diff --git a/tapestry-core/src/test/java/org/apache/tapestry5/internal/services/ContextResourceTest.java b/tapestry-core/src/test/java/org/apache/tapestry5/internal/services/ContextResourceTest.java index 08a032b..0823c24 100644 --- a/tapestry-core/src/test/java/org/apache/tapestry5/internal/services/ContextResourceTest.java +++ b/tapestry-core/src/test/java/org/apache/tapestry5/internal/services/ContextResourceTest.java @@ -1,4 +1,4 @@ -// Copyright 2006, 2007, 2008 The Apache Software Foundation +// Copyright 2006-2013 The Apache Software Foundation // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -27,18 +27,18 @@ public class ContextResourceTest extends InternalBaseTestCase @Test public void get_url_no_real_file() throws Exception { - String path = "/foo/Bar.txt"; + String path = "/foo/ContextResourceTest.class"; URL url = getClass().getResource("ContextResourceTest.class"); Context context = mockContext(); expect(context.getRealFile(path)).andReturn(null); - expect(context.getResource("/foo/Bar.txt")).andReturn(url); + expect(context.getResource("/foo/ContextResourceTest.class")).andReturn(url); replay(); - Resource r = new ContextResource(context, "foo/Bar.txt"); + Resource r = new ContextResource(context, "foo/ContextResourceTest.class"); assertSame(r.toURL(), url); @@ -50,7 +50,7 @@ public class ContextResourceTest extends InternalBaseTestCase { File f = File.createTempFile("Bar", ".txt"); - String path = "/foo/Bar.txt"; + String path = "/foo/" + f.getName(); Context context = mockContext(); @@ -58,7 +58,7 @@ public class ContextResourceTest extends InternalBaseTestCase replay(); - Resource r = new ContextResource(context, "foo/Bar.txt"); + Resource r = new ContextResource(context, "foo/" + f.getName()); assertEquals(r.toURL(), f.toURL()); http://git-wip-us.apache.org/repos/asf/tapestry-5/blob/e16e5fb5/tapestry-ioc/src/main/java/org/apache/tapestry5/ioc/internal/util/AbstractResource.java ---------------------------------------------------------------------- diff --git a/tapestry-ioc/src/main/java/org/apache/tapestry5/ioc/internal/util/AbstractResource.java b/tapestry-ioc/src/main/java/org/apache/tapestry5/ioc/internal/util/AbstractResource.java index feed8cf..23773cd 100644 --- a/tapestry-ioc/src/main/java/org/apache/tapestry5/ioc/internal/util/AbstractResource.java +++ b/tapestry-ioc/src/main/java/org/apache/tapestry5/ioc/internal/util/AbstractResource.java @@ -18,8 +18,10 @@ import org.apache.tapestry5.ioc.Resource; import org.apache.tapestry5.ioc.util.LocalizedNameGenerator; import java.io.BufferedInputStream; +import java.io.File; import java.io.IOException; import java.io.InputStream; +import java.net.URISyntaxException; import java.net.URL; import java.util.List; import java.util.Locale; @@ -69,6 +71,11 @@ public abstract class AbstractResource extends LockSupport implements Resource public final String getFile() { + return extractFile(path); + } + + private static String extractFile(String path) + { int slashx = path.lastIndexOf('/'); return path.substring(slashx + 1); @@ -264,7 +271,9 @@ public abstract class AbstractResource extends LockSupport implements Resource URL url = toURL(); if (url == null) + { return null; + } return new BufferedInputStream(url.openStream()); } @@ -273,4 +282,68 @@ public abstract class AbstractResource extends LockSupport implements Resource * Factory method provided by subclasses. */ protected abstract Resource newResource(String path); + + /** + * Validates that the URL is correct; at this time, a correct URL is one of: + * <ul><li>null</li> + * <li>a non-file: URL</li> + * <li>a file: URL where the case of the file matches the corresponding path element</li> + * </ul> + * See <a href="https://issues.apache.org/jira/browse/TAP5-1007";>TAP5-1007</a> + * + * @param url + * to validate + * @since 5.4 + */ + protected void validateURL(URL url) + { + if (url == null) + { + return; + } + + // Don't have to be concerned with the ClasspathURLConverter since this is intended as a + // runtime check during development; it's about ensuring that what works in development on + // a case-insensitive file system will work in production on the classpath (or other case sensitive + // file system). + + if (!url.getProtocol().equals("file")) + { + return; + } + + File file = toFile(url); + + String expectedFileName = null; + + try + { + expectedFileName = extractFile(file.getCanonicalPath()); + } catch (IOException e) + { + return; + } + + String actualFileName = getFile(); + + if (actualFileName.equals(expectedFileName)) + { + return; + } + + throw new IllegalStateException(String.format("Resource %s does not match the case of the actual file name, '%s'.", + this, expectedFileName)); + + } + + private File toFile(URL url) + { + try + { + return new File(url.toURI()); + } catch (URISyntaxException ex) + { + return new File(url.getPath()); + } + } } http://git-wip-us.apache.org/repos/asf/tapestry-5/blob/e16e5fb5/tapestry-ioc/src/main/java/org/apache/tapestry5/ioc/internal/util/ClasspathResource.java ---------------------------------------------------------------------- diff --git a/tapestry-ioc/src/main/java/org/apache/tapestry5/ioc/internal/util/ClasspathResource.java b/tapestry-ioc/src/main/java/org/apache/tapestry5/ioc/internal/util/ClasspathResource.java index a7e613d..84e44f6 100644 --- a/tapestry-ioc/src/main/java/org/apache/tapestry5/ioc/internal/util/ClasspathResource.java +++ b/tapestry-ioc/src/main/java/org/apache/tapestry5/ioc/internal/util/ClasspathResource.java @@ -1,4 +1,4 @@ -// Copyright 2006, 2007, 2008, 2012 The Apache Software Foundation +// Copyright 2006-2013 The Apache Software Foundation // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -77,12 +77,17 @@ public final class ClasspathResource extends AbstractResource if (!urlResolved) { url = classLoader.getResource(getPath()); + + validateURL(url); + urlResolved = true; } } finally { downgradeWriteLockToReadLock(); } + + } @Override http://git-wip-us.apache.org/repos/asf/tapestry-5/blob/e16e5fb5/tapestry-ioc/src/test/groovy/ioc/specs/ClasspathResourceSpec.groovy ---------------------------------------------------------------------- diff --git a/tapestry-ioc/src/test/groovy/ioc/specs/ClasspathResourceSpec.groovy b/tapestry-ioc/src/test/groovy/ioc/specs/ClasspathResourceSpec.groovy index 2442bf3..a99336d 100644 --- a/tapestry-ioc/src/test/groovy/ioc/specs/ClasspathResourceSpec.groovy +++ b/tapestry-ioc/src/test/groovy/ioc/specs/ClasspathResourceSpec.groovy @@ -24,6 +24,20 @@ class ClasspathResourceSpec extends Specification { content(r) == RESOURCE_TXT_CONTENT } + def "case-mismatch on file name is detected"() { + def r = new ClasspathResource("$FOLDER/Resource.Txt") + + when: + + r.toURL() + + then: + + IllegalStateException e = thrown() + + e.message == "Resource classpath:org/apache/tapestry5/ioc/internal/util/Resource.Txt does not match the case of the actual file name, 'resource.txt'." + } + def "expand path from root resource yields same URL as full path"() { def r = new ClasspathResource("").forFile(PATH)
