Updates the module name validation check for using CompileModule on a project. 1) Loosen the requirement that the last part of the module name be a Java identifier. Users were creating modules with non-java identifer names but since they weren't top level modules, they were never checked. Those modules were then disallowed when presented to the compiler as top level modules for CompileModule. 2) Perform the loosened name validation check on all modules. Adds some unit tests on the module naming. Review at http://gwt-code-reviews.appspot.com/1467810 git-svn-id: https://google-web-toolkit.googlecode.com/svn/trunk@10415 8db76d5a-ed1c-0410-87a9-c151d255dfc7
diff --git a/dev/core/src/com/google/gwt/dev/cfg/ModuleDef.java b/dev/core/src/com/google/gwt/dev/cfg/ModuleDef.java index 92d0e7d..f0accba 100644 --- a/dev/core/src/com/google/gwt/dev/cfg/ModuleDef.java +++ b/dev/core/src/com/google/gwt/dev/cfg/ModuleDef.java
@@ -78,8 +78,13 @@ }; public static boolean isValidModuleName(String moduleName) { + // Check for an empty string between two periods. + if (moduleName.contains("..")) { + return false; + } + // Insure the package name components are a valid Java ident. String[] parts = moduleName.split("\\."); - for (int i = 0; i < parts.length; i++) { + for (int i = 0; i < parts.length - 1; i++) { String part = parts[i]; if (!Util.isValidJavaIdent(part)) { return false;
diff --git a/dev/core/src/com/google/gwt/dev/cfg/ModuleDefLoader.java b/dev/core/src/com/google/gwt/dev/cfg/ModuleDefLoader.java index 256ba6e..f214a3a 100644 --- a/dev/core/src/com/google/gwt/dev/cfg/ModuleDefLoader.java +++ b/dev/core/src/com/google/gwt/dev/cfg/ModuleDefLoader.java
@@ -239,6 +239,12 @@ + moduleName + "'", null); alreadyLoadedModules.add(moduleName); + if (!ModuleDef.isValidModuleName(moduleName)) { + logger.log(TreeLogger.ERROR, "Invalid module name: '" + moduleName + "'", + null); + throw new UnableToCompleteException(); + } + // Find the specified module using the classpath. // String slashedModuleName = moduleName.replace('.', '/'); @@ -308,11 +314,6 @@ */ private ModuleDef doLoadModule(TreeLogger logger, String moduleName) throws UnableToCompleteException { - if (!ModuleDef.isValidModuleName(moduleName)) { - logger.log(TreeLogger.ERROR, "Invalid module name: '" + moduleName + "'", - null); - throw new UnableToCompleteException(); - } ModuleDef moduleDef = new ModuleDef(moduleName); Event moduleLoadEvent = SpeedTracerLogger.start(CompilerEventType.MODULE_DEF,
diff --git a/dev/core/test/com/google/gwt/dev/cfg/ModuleDefLoaderTest.java b/dev/core/test/com/google/gwt/dev/cfg/ModuleDefLoaderTest.java index 0df405b..6f57071 100644 --- a/dev/core/test/com/google/gwt/dev/cfg/ModuleDefLoaderTest.java +++ b/dev/core/test/com/google/gwt/dev/cfg/ModuleDefLoaderTest.java
@@ -16,6 +16,8 @@ package com.google.gwt.dev.cfg; import com.google.gwt.core.ext.TreeLogger; +import com.google.gwt.core.ext.UnableToCompleteException; +import com.google.gwt.dev.util.UnitTestTreeLogger; import junit.framework.TestCase; @@ -52,4 +54,59 @@ assertNull(three.findSourceFile("com/google/gwt/dev/cfg/testdata/merging/client/Toxic.java")); } + /** + * The top level module has an invalid name. + */ + public void testModuleNamingInvalid() { + UnitTestTreeLogger.Builder builder = new UnitTestTreeLogger.Builder(); + builder.setLowestLogLevel(TreeLogger.ERROR); + builder.expectError("Invalid module name: 'com.google.gwt.dev.cfg.testdata.naming.Invalid..Foo'", null); + UnitTestTreeLogger logger = builder.createLogger(); + try { + ModuleDefLoader.loadFromClassPath(logger, + "com.google.gwt.dev.cfg.testdata.naming.Invalid..Foo", false); + fail("Expected exception from invalid module name."); + } catch (UnableToCompleteException expected) { + } + logger.assertLogEntriesContainExpected(); + } + + public void testModuleNamingValid() throws Exception { + TreeLogger logger = TreeLogger.NULL; + //TreeLogger logger = new PrintWriterTreeLogger(); + + ModuleDef module; + module = ModuleDefLoader.loadFromClassPath(logger, + "com.google.gwt.dev.cfg.testdata.naming.Foo-test", false); + assertNotNull(module.findSourceFile("com/google/gwt/dev/cfg/testdata/naming/client/Mock.java")); + + module = ModuleDefLoader.loadFromClassPath(logger, + "com.google.gwt.dev.cfg.testdata.naming.7Foo", false); + assertNotNull(module.findSourceFile("com/google/gwt/dev/cfg/testdata/naming/client/Mock.java")); + + module = ModuleDefLoader.loadFromClassPath(logger, + "com.google.gwt.dev.cfg.testdata.naming.Nested7Foo", false); + assertNotNull(module.findSourceFile("com/google/gwt/dev/cfg/testdata/naming/client/Mock.java")); + + module = ModuleDefLoader.loadFromClassPath(logger, + "com.google.gwt.dev.cfg.testdata.naming.Nested7Foo", false); + assertNotNull(module.findSourceFile("com/google/gwt/dev/cfg/testdata/naming/client/Mock.java")); + } + + /** + * The top level module has a valid name, but the inherited one does not. + */ + public void testModuleNestedNamingInvalid() { + UnitTestTreeLogger.Builder builder = new UnitTestTreeLogger.Builder(); + builder.setLowestLogLevel(TreeLogger.ERROR); + builder.expectError("Invalid module name: 'com.google.gwt.dev.cfg.testdata.naming.Invalid..Foo'", null); + UnitTestTreeLogger logger = builder.createLogger(); + try { + ModuleDefLoader.loadFromClassPath(logger, + "com.google.gwt.dev.cfg.testdata.naming.NestedInvalid", false); + fail("Expected exception from invalid module name."); + } catch (UnableToCompleteException expected) { + } + logger.assertLogEntriesContainExpected(); + } }
diff --git a/dev/core/test/com/google/gwt/dev/cfg/ModuleDefTest.java b/dev/core/test/com/google/gwt/dev/cfg/ModuleDefTest.java index 5be688c..9827bd4 100644 --- a/dev/core/test/com/google/gwt/dev/cfg/ModuleDefTest.java +++ b/dev/core/test/com/google/gwt/dev/cfg/ModuleDefTest.java
@@ -237,4 +237,24 @@ assertEquals(Arrays.asList(expectedClasses), new ArrayList<Class<? extends Linker>>(def.getActiveLinkers())); } + + public void testValidModuleName() { + // Package names must contain valid Java identifiers. + assertFalse(ModuleDef.isValidModuleName("com.foo..")); + assertFalse(ModuleDef.isValidModuleName("com..Foo")); + assertFalse(ModuleDef.isValidModuleName("com.7.Foo")); + assertFalse(ModuleDef.isValidModuleName("com.7foo.Foo")); + + assertTrue(ModuleDef.isValidModuleName("com.foo.Foo")); + assertTrue(ModuleDef.isValidModuleName("com.$foo.Foo")); + assertTrue(ModuleDef.isValidModuleName("com._foo.Foo")); + assertTrue(ModuleDef.isValidModuleName("com.foo7.Foo")); + + // For legacy reasons, allow the last part of the name is not + // required to be a valid ident. In the past, naming rules + // were enforced for top level modules, but not nested modules. + assertTrue(ModuleDef.isValidModuleName("com.foo.F-oo")); + assertTrue(ModuleDef.isValidModuleName("com.foo.7Foo")); + assertTrue(ModuleDef.isValidModuleName("com.foo.+Foo")); + } }
diff --git a/dev/core/test/com/google/gwt/dev/cfg/testdata/naming/7Foo.gwt.xml b/dev/core/test/com/google/gwt/dev/cfg/testdata/naming/7Foo.gwt.xml new file mode 100644 index 0000000..76635d8 --- /dev/null +++ b/dev/core/test/com/google/gwt/dev/cfg/testdata/naming/7Foo.gwt.xml
@@ -0,0 +1,3 @@ +<module> + <source path="client" /> +</module> \ No newline at end of file
diff --git a/dev/core/test/com/google/gwt/dev/cfg/testdata/naming/Foo-test.gwt.xml b/dev/core/test/com/google/gwt/dev/cfg/testdata/naming/Foo-test.gwt.xml new file mode 100644 index 0000000..76635d8 --- /dev/null +++ b/dev/core/test/com/google/gwt/dev/cfg/testdata/naming/Foo-test.gwt.xml
@@ -0,0 +1,3 @@ +<module> + <source path="client" /> +</module> \ No newline at end of file
diff --git a/dev/core/test/com/google/gwt/dev/cfg/testdata/naming/Invalid..Foo.gwt.xml b/dev/core/test/com/google/gwt/dev/cfg/testdata/naming/Invalid..Foo.gwt.xml new file mode 100644 index 0000000..76635d8 --- /dev/null +++ b/dev/core/test/com/google/gwt/dev/cfg/testdata/naming/Invalid..Foo.gwt.xml
@@ -0,0 +1,3 @@ +<module> + <source path="client" /> +</module> \ No newline at end of file
diff --git a/dev/core/test/com/google/gwt/dev/cfg/testdata/naming/Nested7Foo.gwt.xml b/dev/core/test/com/google/gwt/dev/cfg/testdata/naming/Nested7Foo.gwt.xml new file mode 100644 index 0000000..66d06e9 --- /dev/null +++ b/dev/core/test/com/google/gwt/dev/cfg/testdata/naming/Nested7Foo.gwt.xml
@@ -0,0 +1,3 @@ +<module> + <inherits name="com.google.gwt.dev.cfg.testdata.naming.7Foo"/> +</module> \ No newline at end of file
diff --git a/dev/core/test/com/google/gwt/dev/cfg/testdata/naming/NestedInvalid.gwt.xml b/dev/core/test/com/google/gwt/dev/cfg/testdata/naming/NestedInvalid.gwt.xml new file mode 100644 index 0000000..5b4a068 --- /dev/null +++ b/dev/core/test/com/google/gwt/dev/cfg/testdata/naming/NestedInvalid.gwt.xml
@@ -0,0 +1,3 @@ +<module> + <inherits name="com.google.gwt.dev.cfg.testdata.naming.Invalid..Foo"/> +</module> \ No newline at end of file
diff --git a/dev/core/test/com/google/gwt/dev/cfg/testdata/naming/client/Mock.java b/dev/core/test/com/google/gwt/dev/cfg/testdata/naming/client/Mock.java new file mode 100644 index 0000000..9bbfe12 --- /dev/null +++ b/dev/core/test/com/google/gwt/dev/cfg/testdata/naming/client/Mock.java
@@ -0,0 +1,4 @@ +package com.google.gwt.dev.cfg.testdata.naming.client; + +public class Mock { +}
diff --git a/dev/core/test/com/google/gwt/dev/util/UnitTestTreeLogger.java b/dev/core/test/com/google/gwt/dev/util/UnitTestTreeLogger.java index af4aaeb..0c2bcaf 100644 --- a/dev/core/test/com/google/gwt/dev/util/UnitTestTreeLogger.java +++ b/dev/core/test/com/google/gwt/dev/util/UnitTestTreeLogger.java
@@ -45,8 +45,7 @@ return new UnitTestTreeLogger(expected, loggableTypes); } - public void expect(TreeLogger.Type type, String msg, - Class<? extends Throwable> caught) { + public void expect(TreeLogger.Type type, String msg, Class<? extends Throwable> caught) { expected.add(new LogEntry(type, msg, caught)); } @@ -102,8 +101,7 @@ private final String msg; private final Type type; - public LogEntry(TreeLogger.Type type, String msg, - Class<? extends Throwable> caught) { + public LogEntry(TreeLogger.Type type, String msg, Class<? extends Throwable> caught) { assert (type != null); this.type = type; this.msg = msg; @@ -139,28 +137,37 @@ } return sb.toString(); } + + private boolean matches(LogEntry other) { + if (!type.equals(other.type)) { + return false; + } + if (!msg.equals(other.msg)) { + return false; + } + if ((caught == null) != (other.caught == null)) { + return false; + } + if (caught != null && !caught.isAssignableFrom(other.caught)) { + return false; + } + return true; + } } private static void assertCorrectLogEntry(LogEntry expected, LogEntry actual) { - Assert.assertEquals("Log types do not match", expected.getType(), - actual.getType()); - Assert.assertEquals("Log messages do not match", expected.getMessage(), - actual.getMessage()); + Assert.assertEquals("Log types do not match", expected.getType(), actual.getType()); + Assert.assertEquals("Log messages do not match", expected.getMessage(), actual.getMessage()); if (expected.getCaught() == null) { - Assert.assertNull("Actual log exception type should have been null", - actual.getCaught()); + Assert.assertNull("Actual log exception type should have been null", actual.getCaught()); } else { - Assert.assertNotNull( - "Actual log exception type should not have been null", - actual.getCaught()); - Assert.assertTrue("Actual log exception type (" - + actual.getCaught().getName() + Assert.assertNotNull("Actual log exception type should not have been null", actual + .getCaught()); + Assert.assertTrue("Actual log exception type (" + actual.getCaught().getName() + ") cannot be assigned to expected log exception type (" - + expected.getCaught().getName() + ")", - expected.getCaught().isAssignableFrom(actual.getCaught())); + + expected.getCaught().getName() + ")", expected.getCaught().isAssignableFrom( + actual.getCaught())); } - Assert.assertEquals("Log types do not match", expected.getType(), - actual.getType()); } private final List<LogEntry> actualEntries = new ArrayList<LogEntry>(); @@ -168,20 +175,22 @@ private final EnumSet<TreeLogger.Type> loggableTypes; - public UnitTestTreeLogger(List<LogEntry> expectedEntries, - EnumSet<TreeLogger.Type> loggableTypes) { + public UnitTestTreeLogger(List<LogEntry> expectedEntries, EnumSet<TreeLogger.Type> loggableTypes) { this.expectedEntries.addAll(expectedEntries); this.loggableTypes = loggableTypes; // Sanity check that all expected entries are actually loggable. for (LogEntry entry : expectedEntries) { Type type = entry.getType(); - Assert.assertTrue("Cannot expect an entry of a non-loggable type!", - isLoggable(type)); + Assert.assertTrue("Cannot expect an entry of a non-loggable type!", isLoggable(type)); loggableTypes.add(type); } } + /** + * Asserts that all expected log entries were logged in the correct order and + * no other entries were logged. + */ public void assertCorrectLogEntries() { if (expectedEntries.size() != actualEntries.size()) { Assert.fail("Wrong log count: expected=" + expectedEntries + ", actual=" + actualEntries); @@ -191,9 +200,27 @@ } } + /** + * A more loose check than {@link #assertCorrectLogEntries} that just checks + * to see that the expected log messages are somewhere in the actual logged + * messages. + */ + public void assertLogEntriesContainExpected() { + for (LogEntry expectedEntry : expectedEntries) { + boolean found = false; + for (LogEntry actualEntry : actualEntries) { + if (expectedEntry.matches(actualEntry)) { + found = true; + break; + } + } + Assert.assertTrue("No match for expected=" + expectedEntry + " in actual=" + actualEntries, + found); + } + } + @Override - public TreeLogger branch(Type type, String msg, Throwable caught, - HelpInfo helpInfo) { + public TreeLogger branch(Type type, String msg, Throwable caught, HelpInfo helpInfo) { log(type, msg, caught); return this; }