From 54f1357d832cf9c07d14fb23802082c39bb96593 Mon Sep 17 00:00:00 2001
From: Weimin Yu <weiminyu@google.com>
Date: Thu, 21 May 2020 12:20:56 -0400
Subject: [PATCH] Fix show-sql which stopped working (#596)

* Fix show-sql which stopped working

Made show-sql property configurable in JpaUnitTestRules.

Added a few comments on foreign key constraint behavior.
---
 .../registry/model/domain/DomainBaseSqlTest.java |  6 ++++--
 .../persistence/transaction/JpaTestRules.java    | 12 ++++++++++++
 .../transaction/JpaTransactionManagerRule.java   | 16 ++++++++++------
 3 files changed, 26 insertions(+), 8 deletions(-)

diff --git a/core/src/test/java/google/registry/model/domain/DomainBaseSqlTest.java b/core/src/test/java/google/registry/model/domain/DomainBaseSqlTest.java
index 60d8503a0..b8f93eebf 100644
--- a/core/src/test/java/google/registry/model/domain/DomainBaseSqlTest.java
+++ b/core/src/test/java/google/registry/model/domain/DomainBaseSqlTest.java
@@ -119,7 +119,8 @@ public class DomainBaseSqlTest {
         .transact(
             () -> {
               // Persist the contacts.  Note that these need to be persisted before the domain
-              // otherwise we get a foreign key constraint error.
+              // otherwise we get a foreign key constraint error.  If we ever decide to defer the
+              // relevant foreign key checks to commit time, then the order would not matter.
               jpaTm().saveNew(contact);
               jpaTm().saveNew(contact2);
 
@@ -127,7 +128,8 @@ public class DomainBaseSqlTest {
               jpaTm().saveNew(domain);
 
               // Persist the host.  This does _not_ need to be persisted before the domain,
-              // presumably because its relationship is stored in a join table.
+              // because only the row in the join table (DomainHost) is subject to foreign key
+              // constraints, and Hibernate knows to insert it after domain and host.
               jpaTm().saveNew(host);
             });
 
diff --git a/core/src/test/java/google/registry/persistence/transaction/JpaTestRules.java b/core/src/test/java/google/registry/persistence/transaction/JpaTestRules.java
index 69c4507cf..4f5f7b300 100644
--- a/core/src/test/java/google/registry/persistence/transaction/JpaTestRules.java
+++ b/core/src/test/java/google/registry/persistence/transaction/JpaTestRules.java
@@ -28,6 +28,7 @@ import java.util.List;
 import java.util.Map;
 import java.util.Objects;
 import java.util.Optional;
+import org.hibernate.cfg.Environment;
 import org.joda.time.DateTime;
 import org.junit.jupiter.api.extension.AfterEachCallback;
 import org.junit.jupiter.api.extension.BeforeEachCallback;
@@ -180,6 +181,17 @@ public class JpaTestRules {
       return this;
     }
 
+    /**
+     * Enables logging of SQL statements.
+     *
+     * <p>SQL logging is very noisy and disabled by default. This method maybe useful when
+     * troubleshooting a specific test.
+     */
+    public Builder withSqlLogging() {
+      withProperty(Environment.SHOW_SQL, "true");
+      return this;
+    }
+
     /** Builds a {@link JpaIntegrationTestRule} instance. */
     public JpaIntegrationTestRule buildIntegrationTestRule() {
       return new JpaIntegrationTestRule(
diff --git a/core/src/test/java/google/registry/persistence/transaction/JpaTransactionManagerRule.java b/core/src/test/java/google/registry/persistence/transaction/JpaTransactionManagerRule.java
index 8b575e124..f9e0799ea 100644
--- a/core/src/test/java/google/registry/persistence/transaction/JpaTransactionManagerRule.java
+++ b/core/src/test/java/google/registry/persistence/transaction/JpaTransactionManagerRule.java
@@ -41,6 +41,8 @@ import java.sql.ResultSet;
 import java.sql.SQLException;
 import java.sql.Statement;
 import java.util.HashMap;
+import java.util.Map;
+import java.util.Objects;
 import java.util.Optional;
 import java.util.Properties;
 import java.util.stream.Collectors;
@@ -146,13 +148,15 @@ abstract class JpaTransactionManagerRule extends ExternalResource {
     ImmutableMap properties = PersistenceModule.providesDefaultDatabaseConfigs();
     if (!userProperties.isEmpty()) {
       // If there are user properties, create a new properties object with these added.
-      ImmutableMap.Builder builder = properties.builder();
-      builder.putAll(userProperties);
-      // Forbid Hibernate push to stay consistent with flyway-based schema management.
-      builder.put(Environment.HBM2DDL_AUTO, "none");
-      builder.put(Environment.SHOW_SQL, "true");
-      properties = builder.build();
+      Map<String, String> mergedProperties = Maps.newHashMap();
+      mergedProperties.putAll(properties);
+      mergedProperties.putAll(userProperties);
+      properties = ImmutableMap.copyOf(mergedProperties);
     }
+    // Forbid Hibernate push to stay consistent with flyway-based schema management.
+    checkState(
+        Objects.equals(properties.get(Environment.HBM2DDL_AUTO), "none"),
+        "The HBM2DDL_AUTO property must be 'none'.");
     assertReasonableNumDbConnections();
     emf =
         createEntityManagerFactory(