Enhance the test for forbidden Schema changes (#815)

* Enhance the test for forbidden Schema changes

Current test is git-based. It is difficult to maintain and does not
catch out-of-order version numbers. It is also more aggressive than
necessary, failing on changes to submitted scripts that have not been
deployed yet.

The new test starts a database, deploys the current schema to it,
then deploys the set of Flyway scripts in this repository to the database.

build.gradle

@@ -260,7 +260,7 @@ subprojects {
       // in the 'configurations' block, the following code must run after
       // project evaluation, when all configurations have been created.
       configurations.each {
-        if (it.name != 'dependencyLicenseReport') {
+        if (it.name != 'dependencyLicenseReport' && it.name != 'integration') {
           it.resolutionStrategy.activateDependencyLocking()
         }
       }

config/nom_build.py

@@ -75,6 +75,7 @@ Pseudo-commands:
 """
 
 # Define all of our special gradle properties here.
+# TODO(b/169318491): use consistent naming style for properties and variables.
 PROPERTIES = [
     Property('mavenUrl',
              'URL to use for the main maven repository (defaults to maven '
@@ -124,6 +125,9 @@ PROPERTIES = [
              'server/schema integration tests. Please refer to <a '
              'href="./integration/README.md">integration project</a> for more '
              'information.'),
+    Property('baseSchemaTag',
+             'The nomulus version tag of the schema for use in the schema'
+             'deployment integration test (:db:schemaIncrementalDeployTest)'),
     Property('schema_version',
              'The nomulus version tag of the schema for use in a database'
              'integration test.'),

db/build.gradle

@@ -59,8 +59,11 @@ ext {
   getJdbcAccessInfo = {
     if (allDbEnv.contains(dbServer)) {
       return getSocketFactoryAccessInfo(dbServer)
-    } else {
+    } else if (!dbServer.isEmpty()) {
       return getAccessInfoByHostPort(dbServer)
+    } else {
+      // Not connecting to a database. Return a dummy object for Flyway config.
+      return [ url: '', user: '', password: '' ]
     }
   }
 
@@ -110,6 +113,7 @@ task compileApiJar(type: Jar) {
 configurations {
   compileApi
   schema
+  integration
 }
 
 artifacts {
@@ -187,3 +191,33 @@ if (ext.isRestricted()) {
     }
   }
 }
+
+if (project.baseSchemaTag != '') {
+  repositories {
+    maven {
+      url project.publish_repo
+    }
+  }
+  dependencies {
+    integration "google.registry:schema:${project.baseSchemaTag}"
+  }
+
+  // Checks if Flyway scripts can be deployed to an existing database with
+  // an older release. Please refer to SchemaTest.java for more information.
+  task schemaIncrementalDeployTest(dependsOn: processResources, type: Test) {
+    useJUnitPlatform()
+    include 'google/registry/sql/flyway/SchemaTest.*'
+    classpath = configurations.testRuntimeClasspath
+        .plus(configurations.integration)
+        .plus(files(sourceSets.test.output.classesDirs))
+        .plus(files(sourceSets.test.output.resourcesDir))
+        .plus(files(sourceSets.main.output.classesDirs))
+
+    // Declare test-runtime dependency on Flyway scripts in the resources dir.
+    // They are not on classpath since they conflict with the base schema.
+    inputs.dir sourceSets.main.output.resourcesDir
+
+    // Specifies which test to run using the following property
+    systemProperty 'deploy_to_existing_db', 'true'
+  }
+}

db/src/test/java/google/registry/sql/flyway/SchemaTest.java

@@ -18,6 +18,7 @@ import static com.google.common.truth.Truth.assertThat;
 import static google.registry.testing.truth.TextDiffSubject.assertThat;
 
 import com.google.common.base.Joiner;
+import com.google.common.flogger.FluentLogger;
 import com.google.common.io.Resources;
 import google.registry.persistence.NomulusPostgreSql;
 import java.io.File;
@@ -26,16 +27,41 @@ import java.nio.charset.StandardCharsets;
 import java.nio.file.Paths;
 import org.flywaydb.core.Flyway;
 import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.condition.DisabledIfSystemProperty;
+import org.junit.jupiter.api.condition.EnabledIfSystemProperty;
 import org.testcontainers.containers.BindMode;
 import org.testcontainers.containers.Container.ExecResult;
 import org.testcontainers.containers.PostgreSQLContainer;
 import org.testcontainers.junit.jupiter.Container;
 import org.testcontainers.junit.jupiter.Testcontainers;
 
-/** Unit tests about Cloud SQL schema. */
+/**
+ * Schema deployment tests using Flyway.
+ *
+ * <p>This class has two test methods:
+ *
+ * <ul>
+ *   <li>{@link #deploySchema_emptyDb()} is invoked only in UNIT tests (:db:test in Gradle). It
+ *       deploys the entire set of Flyway scripts (found on classpath) to an empty database and
+ *       compares the resulting schema with the golden schema.
+ *   <li>{@link #deploySchema_existingDb()} is invoked only in an integration test
+ *       (:db:schemaIncrementalDeployTest in Gradle). It first populates the test database with an
+ *       earlier release of the schema (found on the classpath), then deploys the latest Flyway
+ *       scripts (found on local filesystem under the resources directory) to that database. This
+ *       test detects all forbidden changes to deployed scripts including content change, file
+ *       renaming, and file deletion.
+ *       <p>This test also checks for out-of-order version numbers, i.e., new scripts with lower
+ *       numbers than that of the last deployed script. Out-of-order versions are confusing to
+ *       maintainers, however, Flyway does not provide ways to check before schema deployment. In
+ *       this test, out-of-order scripts are ignored in the incremental-deployment phase (default
+ *       Flyway behavior). The final validate call will fail on them.
+ * </ul>
+ */
 @Testcontainers
 class SchemaTest {
 
+  private static final FluentLogger logger = FluentLogger.forEnclosingClass();
+
   // Resource path that is mapped to the testcontainer instance.
   private static final String MOUNTED_RESOURCE_PATH = "testcontainer/mount";
   // The mount point in the container.
@@ -57,7 +83,8 @@ class SchemaTest {
               MOUNTED_RESOURCE_PATH, CONTAINER_MOUNT_POINT, BindMode.READ_WRITE);
 
   @Test
-  void deploySchema_success() throws Exception {
+  @DisabledIfSystemProperty(named = "deploy_to_existing_db", matches = ".*")
+  void deploySchema_emptyDb() throws Exception {
     Flyway flyway =
         Flyway.configure()
             .locations("sql/flyway")
@@ -86,6 +113,32 @@ class SchemaTest {
         .hasSameContentAs(Resources.getResource("sql/schema/nomulus.golden.sql"));
   }
 
+  @Test
+  @EnabledIfSystemProperty(named = "deploy_to_existing_db", matches = ".*")
+  void deploySchema_existingDb() {
+    // Initialize database with the base schema, which is on the classpath.
+    Flyway flyway =
+        Flyway.configure()
+            .locations("sql/flyway")
+            .dataSource(
+                sqlContainer.getJdbcUrl(), sqlContainer.getUsername(), sqlContainer.getPassword())
+            .load();
+    flyway.migrate();
+    logger.atInfo().log("Base schema version: %s", flyway.info().current().getVersion().toString());
+
+    // Deploy latest scripts from resources directory.
+    flyway =
+        Flyway.configure()
+            .locations("filesystem:build/resources/main/sql/flyway")
+            .dataSource(
+                sqlContainer.getJdbcUrl(), sqlContainer.getUsername(), sqlContainer.getPassword())
+            .load();
+    flyway.migrate();
+    flyway.validate();
+    logger.atInfo().log(
+        "Latest schema version: %s", flyway.info().current().getVersion().toString());
+  }
+
   private static String[] getSchemaDumpCommand(String username, String dbName) {
     return new String[] {
       "pg_dump",

gradle.properties

@@ -24,6 +24,7 @@ dbName=postgres
 dbUser=
 dbPassword=
 publish_repo=
+baseSchemaTag=
 schema_version=
 nomulus_version=
 dot_path=/usr/bin/dot

integration/run_compatibility_tests.sh

@@ -79,6 +79,8 @@ function runTest() {
   echo "Running test with -Pnomulus_version=${nomulus_version}" \
       "-Pschema_version=${schema_version}"
 
+  # The https scheme in the Maven repo URL below is required for Kokoro. See
+  # ./run_schema_check.sh for more information.
   (cd ${SCRIPT_DIR}/..; \
       ./gradlew :integration:sqlIntegrationTest \
           -PdevProject=${dev_project} \

integration/run_schema_check.sh

@@ -25,12 +25,13 @@ $(basename "$0") OPTIONS
 Checks for post-deployment change to Flyway scripts.
 
 With Flyway, once an incremental change script is deployed, it must not be
-changed. Even changes to comments or whitespaces would cause validation
-failures during future deployment. This script checks for changes (including
-removal and renaming which may happen due to incorrect merge conflict
-resolution) to scripts that have already been deployed to Sandbox. The
-assumption is that the schema in Sandbox is always newer than that in
-production.
+edited, renamed, or deleted. This script checks for changes to scripts that have
+already been deployed to Sandbox. The assumption is that the schema in Sandbox
+is always at least as recent as that in production. Please refer to Gradle task
+:db:schemaIncrementalDeployTest for more information.
+
+Note that this test MAY fail to catch forbidden changes during the period when
+a new schema release is created but not yet deployed to Sandbox.
 
 A side-effect of this check is that old branches missing recently deployed
 scripts must update first.
@@ -65,16 +66,14 @@ fi
 
 sandbox_tag=$(fetchVersion sql sandbox ${DEV_PROJECT})
 echo "Checking Flyway scripts against schema in Sandbox (${sandbox_tag})."
-modified_sqls=$(git diff --name-status ${sandbox_tag} \
-    db/src/main/resources/sql/flyway | grep "^M\|^D\|^R" | grep \.sql$ | wc -l)
 
-if [[ ${modified_sqls} = 0 ]]; then
-  echo "No illegal change to deployed schema scripts."
-  exit 0
-else
-  echo "Changes to the following files are not allowed:"
-  echo $(git diff --name-status ${sandbox_tag} \
-      db/src/main/resources/sql/flyway | grep "^M\|^D\|^R" | grep \.sql$)
-  echo "Make sure your branch is up to date with HEAD of master."
-  exit 1
-fi
+# The URL of the Maven repo on GCS for the publish_repo parameter must use the
+# https scheme (https://storage.googleapis.com/{BUCKET}/{PATH}) in order to work
+# with Kokoro. Gradle's alternative gcs scheme does not work on Kokoro: a GCP
+# credential with proper scopes for GCS access is required even for public
+# buckets, however, Kokoro VM instances are not set up with such credentials.
+# Incidentally, gcs can be used on Cloud Build.
+(cd ${SCRIPT_DIR}/..; \
+    ./gradlew :db:schemaIncrementalDeployTest \
+        -PbaseSchemaTag=${sandbox_tag} \
+        -Ppublish_repo=https://storage.googleapis.com/${DEV_PROJECT}-deployed-tags/maven)