From 959c7f789936bc32f4d6e07f811d53855ca54f45 Mon Sep 17 00:00:00 2001 From: Weimin Yu Date: Thu, 24 Sep 2020 12:31:08 -0400 Subject: [PATCH] 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 | 2 +- config/nom_build.py | 4 ++ db/build.gradle | 36 +++++++++++- .../registry/sql/flyway/SchemaTest.java | 57 ++++++++++++++++++- gradle.properties | 1 + integration/run_compatibility_tests.sh | 2 + integration/run_schema_check.sh | 35 ++++++------ 7 files changed, 115 insertions(+), 22 deletions(-) diff --git a/build.gradle b/build.gradle index 96aecb6ce..6063a7ed8 100644 --- a/build.gradle +++ b/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() } } diff --git a/config/nom_build.py b/config/nom_build.py index 76c4cf909..c3112aa97 100644 --- a/config/nom_build.py +++ b/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 integration project 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.'), diff --git a/db/build.gradle b/db/build.gradle index 04705c801..888e0eaa7 100644 --- a/db/build.gradle +++ b/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' + } +} diff --git a/db/src/test/java/google/registry/sql/flyway/SchemaTest.java b/db/src/test/java/google/registry/sql/flyway/SchemaTest.java index 00580dbb8..d0b82b8f1 100644 --- a/db/src/test/java/google/registry/sql/flyway/SchemaTest.java +++ b/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. + * + *

This class has two test methods: + * + *

+ */ @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", diff --git a/gradle.properties b/gradle.properties index 7873f1e50..cfa1488c7 100644 --- a/gradle.properties +++ b/gradle.properties @@ -24,6 +24,7 @@ dbName=postgres dbUser= dbPassword= publish_repo= +baseSchemaTag= schema_version= nomulus_version= dot_path=/usr/bin/dot diff --git a/integration/run_compatibility_tests.sh b/integration/run_compatibility_tests.sh index c9404386d..ada9fac29 100755 --- a/integration/run_compatibility_tests.sh +++ b/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} \ diff --git a/integration/run_schema_check.sh b/integration/run_schema_check.sh index 5b9ba4584..a41c7edaf 100755 --- a/integration/run_schema_check.sh +++ b/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)