Fix JPA setup in Nomulus tool (#780)

* Fix JPA setup in Nomulus tool

Hibernate unnecessarily scans third-party classes in the Nomulus tool,
hitting a bug and fails to set up.

In this change we properly configured persistence.xml to include the orm mapping file (orm.xml) and disable 
auto detection, and provided a custom (NOOP) scanner
to work around Hibernate scanner bugs.

Also improved on the :core:registryIntegrationTest task to test for
JPA setup as well as dependency-packaging.

core/build.gradle

@@ -635,131 +635,6 @@ artifacts {
   testRuntime testJar
 }
 
-/**
- * We have to break out the test suites because some of the tests conflict
- * with one another, but unfortunately this breaks the "--tests" flag.  The
- * --tests flag only applies to the task named on the command line (usually
- * just "test"), not for all tasks of type "Test".
- *
- * As a better solution, FilteringTest sets testNameIncludePatterns (the
- * internal property that --tests sets) from the value of the "testFilter"
- * property, allowing us to filter across all the tests in core without
- * explicitly specifying a test task or causing errors because there are no
- * matching tests in the main task.
- *
- * To use it, define "testFilter" to be a comma-separated collection of class
- * names (wildcards are allowed):
- *
- *   ./gradlew test -P testFilter=*.FooBar,google.registry.tools.ShellCommandTest
- */
-class FilteringTest extends Test {
-
-  FilteringTest() {
-    useJUnitPlatform();
-  }
-
-  private void applyTestFilter() {
-    if (project.testFilter) {
-      testNameIncludePatterns = project.testFilter.split(',')
-
-      // By default, gradle test tasks will produce a failure if no tests
-      // match the include/exclude/filter rules.  Since test filtering allows us
-      // to select a set of tests from a particular task, we don't want this
-      // behavior.
-      filter.failOnNoMatchingTests = false
-    }
-  }
-
-  /**
-   * Set to false if you also want to include TestCase and TestSuite classes.
-   *
-   * <p>Must be defined before "test", if at all.
-   */
-  boolean excludeTestCases = true
-
-  void setTests(List<String> tests) {
-    // Common exclude pattern. See README in parent directory for explanation.
-    if (excludeTestCases) {
-      exclude "**/*TestCase.*", "**/*TestSuite.*"
-    }
-    include tests
-    applyTestFilter()
-  }
-
-  /**
-   * Include all of the tests (except Test{Case,TestSuite}).  This actually
-   * doesn't explicitly "include" anything, in which cast the Test class tries
-   * to include everything that is not explicitly excluded.
-   */
-  void includeAllTests() {
-    exclude "**/*TestCase.*", "**/*TestSuite.*"
-    applyTestFilter()
-  }
-}
-
-task fragileTest(type: FilteringTest) {
-  // Common exclude pattern. See README in parent directory for explanation.
-  tests = fragileTestPatterns
-
-  if (rootProject.findProperty("skipDockerIncompatibleTests") == "true") {
-    exclude dockerIncompatibleTestPatterns
-  }
-
-  // Run every test class in a freshly started process.
-  forkEvery 1
-
-  doFirst {
-    new File(screenshotsDir).deleteDir()
-  }
-}
-
-task outcastTest(type: FilteringTest) {
-  tests = outcastTestPatterns
-
-  // Sets the maximum number of test executors that may exist at the same time.
-  // Note that this number appears to contribute to NoClassDefFoundError
-  // exceptions on certain machines and distros. The root cause is unclear.
-  // Try reducing this number if you experience similar problems.
-  maxParallelForks 3
-}
-
-// Whitebox test verifying that RegistryTool can be instantiated. Note the
-// use of runtimeClasspath. This test emulates the logic in RegistryCli#run.
-// A to-do is added there to refactor.
-// TODO(weiminyu): Need a similar test for Registry server.
-task registryToolIntegrationTest {
-  dependsOn compileJava
-  doLast {
-    def classLoader =
-        new URLClassLoader(sourceSets.main.runtimeClasspath.collect {
-            it.toURI().toURL()
-        } as URL[])
-    def commandClasses =
-        (classLoader.loadClass('google.registry.tools.RegistryTool')
-            .getDeclaredField('COMMAND_MAP').get(null) as Map).values()
-
-    commandClasses.each {
-      try {
-        Constructor<?> c = ((Class<?>) it).getDeclaredConstructor()
-        c.setAccessible(true)
-        c.newInstance()
-      } catch (Throwable e) {
-        throw new RuntimeException("Failed to instantiate ${it}:\n ${e}")
-      }
-    }
-  }
-}
-
-// Dedicated test suite for schema-dependent tests.
-task sqlIntegrationTest(type: FilteringTest) {
-  // TestSuite still requires a JUnit 4 runner, which knows how to handle JUnit 5 tests.
-  // Here we need to override parent's choice of JUnit 5. If changing this, remember to
-  // change :integration:sqlIntegrationTest too.
-  useJUnit()
-  excludeTestCases = false
-  tests = ['google/registry/schema/integration/SqlIntegrationTestSuite.*']
-}
-
 task findGoldenImages(type: JavaExec) {
   classpath = sourceSets.test.runtimeClasspath
   main = 'google.registry.webdriver.GoldenImageFinder'
@@ -879,39 +754,6 @@ task generateGoldenImages(type: FilteringTest) {
 }
 generateGoldenImages.finalizedBy(findGoldenImages)
 
-task standardTest(type: FilteringTest) {
-  includeAllTests()
-  exclude fragileTestPatterns
-  exclude outcastTestPatterns
-  // See SqlIntegrationTestSuite.java
-  exclude '**/*BeforeSuiteTest.*', '**/*AfterSuiteTest.*'
-
-  if (rootProject.findProperty("skipDockerIncompatibleTests") == "true") {
-    exclude dockerIncompatibleTestPatterns
-  }
-
-  // Run every test class in its own process.
-  // Uncomment to unblock build while troubleshooting inexplicable test errors.
-  // This setting makes the build take 35 minutes, without it it takes about 10.
-  // forkEvery 1
-
-  // Sets the maximum number of test executors that may exist at the same time.
-  // Also, Gradle executes tests in 1 thread and some of our test infrastructures
-  // depend on that, e.g. DualDatabaseTestInvocationContextProvider injects
-  // different implementation of TransactionManager into TransactionManagerFactory.
-  maxParallelForks 5
-
-  systemProperty 'test.projectRoot', rootProject.projectRootDir
-  systemProperty 'test.resourcesDir', resourcesDir
-}
-
-test {
-  // Don't run any tests from this task, all testing gets done in the
-  // FilteringTest tasks.
-  exclude "**"
-  // TODO(weiminyu): Remove dependency on sqlIntegrationTest
-}.dependsOn(fragileTest, outcastTest, standardTest, registryToolIntegrationTest, sqlIntegrationTest)
-
 createUberJar('nomulus', 'nomulus', 'google.registry.tools.RegistryTool')
 
 // A jar with classes and resources from main sourceSet, excluding internal
@@ -1046,6 +888,147 @@ task runTestServer(dependsOn: copyJsFilesForTestServer, type: JavaExec) {
   classpath = sourceSets.test.runtimeClasspath
 }
 
+/**
+ * We have to break out the test suites because some of the tests conflict
+ * with one another, but unfortunately this breaks the "--tests" flag.  The
+ * --tests flag only applies to the task named on the command line (usually
+ * just "test"), not for all tasks of type "Test".
+ *
+ * As a better solution, FilteringTest sets testNameIncludePatterns (the
+ * internal property that --tests sets) from the value of the "testFilter"
+ * property, allowing us to filter across all the tests in core without
+ * explicitly specifying a test task or causing errors because there are no
+ * matching tests in the main task.
+ *
+ * To use it, define "testFilter" to be a comma-separated collection of class
+ * names (wildcards are allowed):
+ *
+ *   ./gradlew test -P testFilter=*.FooBar,google.registry.tools.ShellCommandTest
+ */
+class FilteringTest extends Test {
+
+  FilteringTest() {
+    useJUnitPlatform();
+  }
+
+  private void applyTestFilter() {
+    if (project.testFilter) {
+      testNameIncludePatterns = project.testFilter.split(',')
+
+      // By default, gradle test tasks will produce a failure if no tests
+      // match the include/exclude/filter rules.  Since test filtering allows us
+      // to select a set of tests from a particular task, we don't want this
+      // behavior.
+      filter.failOnNoMatchingTests = false
+    }
+  }
+
+  /**
+   * Set to false if you also want to include TestCase and TestSuite classes.
+   *
+   * <p>Must be defined before "test", if at all.
+   */
+  boolean excludeTestCases = true
+
+  void setTests(List<String> tests) {
+    // Common exclude pattern. See README in parent directory for explanation.
+    if (excludeTestCases) {
+      exclude "**/*TestCase.*", "**/*TestSuite.*"
+    }
+    include tests
+    applyTestFilter()
+  }
+
+  /**
+   * Include all of the tests (except Test{Case,TestSuite}).  This actually
+   * doesn't explicitly "include" anything, in which cast the Test class tries
+   * to include everything that is not explicitly excluded.
+   */
+  void includeAllTests() {
+    exclude "**/*TestCase.*", "**/*TestSuite.*"
+    applyTestFilter()
+  }
+}
+
+task fragileTest(type: FilteringTest) {
+  // Common exclude pattern. See README in parent directory for explanation.
+  tests = fragileTestPatterns
+
+  if (rootProject.findProperty("skipDockerIncompatibleTests") == "true") {
+    exclude dockerIncompatibleTestPatterns
+  }
+
+  // Run every test class in a freshly started process.
+  forkEvery 1
+
+  doFirst {
+    new File(screenshotsDir).deleteDir()
+  }
+}
+
+task outcastTest(type: FilteringTest) {
+  tests = outcastTestPatterns
+
+  // Sets the maximum number of test executors that may exist at the same time.
+  // Note that this number appears to contribute to NoClassDefFoundError
+  // exceptions on certain machines and distros. The root cause is unclear.
+  // Try reducing this number if you experience similar problems.
+  maxParallelForks 3
+}
+
+// Dedicated test suite for schema-dependent tests.
+task sqlIntegrationTest(type: FilteringTest) {
+  // TestSuite still requires a JUnit 4 runner, which knows how to handle JUnit 5 tests.
+  // Here we need to override parent's choice of JUnit 5. If changing this, remember to
+  // change :integration:sqlIntegrationTest too.
+  useJUnit()
+  excludeTestCases = false
+  tests = ['google/registry/schema/integration/SqlIntegrationTestSuite.*']
+}
+
+// Verifies that RegistryTool can be instantiated:
+// - All dependencies are packaged in nomulus.jar
+// - JPA setup succeeds.
+task registryToolIntegrationTest(dependsOn: nomulus, type: FilteringTest) {
+  tests = ['google/registry/tools/RegistryToolTest.*']
+  testClassesDirs = sourceSets.test.output.classesDirs
+  classpath = nomulus.outputs.files.plus(configurations.testRuntimeClasspath)
+      .plus(files(testClassesDirs))
+}
+
+task standardTest(type: FilteringTest) {
+  includeAllTests()
+  exclude fragileTestPatterns
+  exclude outcastTestPatterns
+  // See SqlIntegrationTestSuite.java
+  exclude '**/*BeforeSuiteTest.*', '**/*AfterSuiteTest.*'
+
+  if (rootProject.findProperty("skipDockerIncompatibleTests") == "true") {
+    exclude dockerIncompatibleTestPatterns
+  }
+
+  // Run every test class in its own process.
+  // Uncomment to unblock build while troubleshooting inexplicable test errors.
+  // This setting makes the build take 35 minutes, without it it takes about 10.
+  // forkEvery 1
+
+  // Sets the maximum number of test executors that may exist at the same time.
+  // Also, Gradle executes tests in 1 thread and some of our test infrastructures
+  // depend on that, e.g. DualDatabaseTestInvocationContextProvider injects
+  // different implementation of TransactionManager into TransactionManagerFactory.
+  maxParallelForks 5
+
+  systemProperty 'test.projectRoot', rootProject.projectRootDir
+  systemProperty 'test.resourcesDir', resourcesDir
+}
+
+test {
+  // Don't run any tests from this task, all testing gets done in the
+  // FilteringTest tasks.
+  exclude "**"
+  // TODO(weiminyu): Remove dependency on sqlIntegrationTest
+}.dependsOn(fragileTest, outcastTest, standardTest, registryToolIntegrationTest, sqlIntegrationTest)
+
 project.build.dependsOn devtool
 project.build.dependsOn buildToolImage
 project.build.dependsOn ':stage'

core/src/main/java/google/registry/persistence/NoopJpaEntityScanner.java

@@ -0,0 +1,45 @@
+// Copyright 2020 The Nomulus Authors. All Rights Reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package google.registry.persistence;
+
+import com.google.common.collect.ImmutableSet;
+import org.hibernate.boot.archive.internal.StandardArchiveDescriptorFactory;
+import org.hibernate.boot.archive.scan.internal.ScanResultImpl;
+import org.hibernate.boot.archive.scan.internal.StandardScanner;
+import org.hibernate.boot.archive.scan.spi.ScanEnvironment;
+import org.hibernate.boot.archive.scan.spi.ScanOptions;
+import org.hibernate.boot.archive.scan.spi.ScanParameters;
+import org.hibernate.boot.archive.scan.spi.ScanResult;
+import org.hibernate.boot.archive.scan.spi.Scanner;
+
+/**
+ * A do-nothing {@link Scanner} for Hibernate that works around bugs in Hibernate's default
+ * implementation. This is required for the Nomulus tool.
+ *
+ * <p>Please refer to <a href="../../../../resources/META-INF/persistence.xml">persistence.xml</a>
+ * for more information.
+ */
+public class NoopJpaEntityScanner extends StandardScanner {
+
+  public NoopJpaEntityScanner() {
+    super(StandardArchiveDescriptorFactory.INSTANCE);
+  }
+
+  @Override
+  public ScanResult scan(
+      ScanEnvironment environment, ScanOptions options, ScanParameters parameters) {
+    return new ScanResultImpl(ImmutableSet.of(), ImmutableSet.of(), ImmutableSet.of());
+  }
+}

core/src/main/java/google/registry/tools/RegistryCli.java

@@ -112,8 +112,6 @@ final class RegistryCli implements AutoCloseable, CommandRunner {
     // Create all command instances. It would be preferrable to do this in the constructor, but
     // JCommander mutates the command instances and doesn't reset them so we have to do it for every
     // run.
-    // TODO(weiminyu): extract this into a standalone static method to simplify
-    //   :core:registryToolIntegrationTest
     try {
       for (Map.Entry<String, ? extends Class<? extends Command>> entry : commands.entrySet()) {
         Command command = entry.getValue().getDeclaredConstructor().newInstance();

core/src/main/resources/META-INF/persistence.xml

@@ -11,14 +11,33 @@
     <provider>org.hibernate.jpa.HibernatePersistenceProvider</provider>
 
     <!--
-      All JPA entities must be enumerated here. JPA does not support auto detection.
+      All JPA entity-mapping files and annotated classes must be enumerated
+      here. Automatic entity detection is not part of the JPA spec. Explicit
+      declaration makes it easier to migrate to another provider.
 
-      Note that Hibernate's auto detection functionality (hibernate.archive.autodection)
+      Although Hibernate provides the auto detection functionality (configured by
-      does not meet our needs. It only scans archives, not the 'classes' folders. So we
+      the hibernate.archive.autodetection property), it relies on a fragile
-      are left with two options:
+      scanner that can be broken by certain classes. For example, in the uber jar
-      *  Move tests to another (sub)project. This is not a big problem, but feels unnatural.
+      for the Nomulus tool, a repackaged Guava class ( {@code
-      *  Use Hibernate's ServiceRegistry for bootstrapping (not JPA-compliant)
+      com.google.appengine.repackaged.com.google.common.html.LinkDetector})
+      from appengine-api-1.0-sdk:1.9.81 can break the scanner in
+      hibernate-core:5.4.17.Final. The large number of third-party classes also
+      makes JPA setup noticeably slower in the tool.
+
+      When auto detection is enabled in Hibernate, we also need a separate
+      persistence.xml for tests. See <a
+      href="https://stackoverflow.com/questions/61127082/hibernate-doesnt-find-entities-in-test">
+      this webpage</a> for an example.
+
+      Because of the reasons above, we disable auto detection in Hibernate.
+
+      When auto detection is disabled, Hibernate still invokes the scanner which always
+      goes over the archive that has this file. We need to override the default scanner
+      with an NOOP one for Nomulus tool.
     -->
+
+    <mapping-file>META-INF/orm.xml</mapping-file>
+
     <class>google.registry.model.billing.BillingEvent$Cancellation</class>
     <class>google.registry.model.billing.BillingEvent$OneTime</class>
     <class>google.registry.model.billing.BillingEvent$Recurring</class>
@@ -82,5 +101,12 @@
 
     <!-- TODO(weiminyu): check out application-layer validation. -->
     <validation-mode>NONE</validation-mode>
+    <properties>
+      <!-- Disables auto detection. -->
+      <property name="hibernate.archive.autodetection" value=""/>
+      <!-- NOOP scanner needed for Nomulus tool. -->
+      <property name="hibernate.archive.scanner"
+          value="google.registry.persistence.NoopJpaEntityScanner"/>
+    </properties>
   </persistence-unit>
 </persistence>