From d35460f14c676304b8ccd766b39dd77528e2cb0b Mon Sep 17 00:00:00 2001 From: gbrodman Date: Tue, 13 Apr 2021 20:50:07 -0400 Subject: [PATCH] Convert TmchCrl and ServerSecret to cleaner tm() impls (#1068) * Convert TmchCrl and ServerSecret to cleaner tm() impls When I implemented this originally I knew a lot less than I know now about how we'll be storing and retrieving these singletons from SQL. The optimal way here is to use the single SINGLETON_ID as the primary key, that way we always know how to create the key that we can use in the tm() retrieval. This allows us to use generic tm() methods and to remove the handcrafted SQL queries. --- .../model/common/CrossTldSingleton.java | 2 +- .../registry/model/server/ServerSecret.java | 31 +- .../google/registry/model/tmch/TmchCrl.java | 56 +--- .../model/server/ServerSecretTest.java | 37 +-- .../registry/model/tmch/TmchCrlTest.java | 34 +- .../sql/er_diagram/brief_er_diagram.html | 118 +++---- .../sql/er_diagram/full_er_diagram.html | 308 +++++++++--------- db/src/main/resources/sql/flyway.txt | 1 + .../resources/sql/flyway/V92__singletons.sql | 21 ++ .../sql/schema/db-schema.sql.generated | 10 +- .../resources/sql/schema/nomulus.golden.sql | 10 +- 11 files changed, 285 insertions(+), 343 deletions(-) create mode 100644 db/src/main/resources/sql/flyway/V92__singletons.sql diff --git a/core/src/main/java/google/registry/model/common/CrossTldSingleton.java b/core/src/main/java/google/registry/model/common/CrossTldSingleton.java index 3214c2025..acc9baa0e 100644 --- a/core/src/main/java/google/registry/model/common/CrossTldSingleton.java +++ b/core/src/main/java/google/registry/model/common/CrossTldSingleton.java @@ -31,7 +31,7 @@ public abstract class CrossTldSingleton extends ImmutableObject { public static final long SINGLETON_ID = 1; // There is always exactly one of these. - @Id @Transient long id = SINGLETON_ID; + @Id @javax.persistence.Id long id = SINGLETON_ID; @Transient @Parent Key parent = getCrossTldKey(); } diff --git a/core/src/main/java/google/registry/model/server/ServerSecret.java b/core/src/main/java/google/registry/model/server/ServerSecret.java index 96de884c1..b88006ab3 100644 --- a/core/src/main/java/google/registry/model/server/ServerSecret.java +++ b/core/src/main/java/google/registry/model/server/ServerSecret.java @@ -15,8 +15,6 @@ package google.registry.model.server; import static google.registry.model.common.EntityGroupRoot.getCrossTldKey; -import static google.registry.persistence.transaction.TransactionManagerFactory.jpaTm; -import static google.registry.persistence.transaction.TransactionManagerFactory.ofyTm; import static google.registry.persistence.transaction.TransactionManagerFactory.tm; import com.google.common.annotations.VisibleForTesting; @@ -35,10 +33,10 @@ import google.registry.model.common.CrossTldSingleton; import google.registry.persistence.VKey; import google.registry.schema.replay.NonReplicatedEntity; import java.nio.ByteBuffer; +import java.util.Optional; import java.util.UUID; import java.util.concurrent.ExecutionException; import javax.persistence.Column; -import javax.persistence.Id; import javax.persistence.PostLoad; import javax.persistence.Transient; @@ -67,22 +65,28 @@ public class ServerSecret extends CrossTldSingleton implements NonReplicatedEnti }); private static ServerSecret retrieveAndSaveSecret() { - VKey key = + VKey vkey = VKey.create( ServerSecret.class, SINGLETON_ID, Key.create(getCrossTldKey(), ServerSecret.class, SINGLETON_ID)); + if (tm().isOfy()) { + // Attempt a quick load if we're in ofy first to short-circuit sans transaction + Optional secretWithoutTransaction = tm().loadByKeyIfPresent(vkey); + if (secretWithoutTransaction.isPresent()) { + return secretWithoutTransaction.get(); + } + } return tm().transact( () -> { - // transactionally create a new ServerSecret (once per app setup) if necessary. - // return the ofy() result during Datastore-primary phase - ServerSecret secret = - ofyTm().loadByKeyIfPresent(key).orElseGet(() -> create(UUID.randomUUID())); - // During a dual-write period, write it to both Datastore and SQL - // even if we didn't have to retrieve it from the DB - ofyTm().transact(() -> ofyTm().putWithoutBackup(secret)); - jpaTm().transact(() -> jpaTm().putWithoutBackup(secret)); - return secret; + // Make sure we're in a transaction and attempt to load any existing secret, then + // create it if it's absent. + Optional secret = tm().loadByKeyIfPresent(vkey); + if (!secret.isPresent()) { + secret = Optional.of(create(UUID.randomUUID())); + tm().insertWithoutBackup(secret.get()); + } + return secret.get(); }); } @@ -102,7 +106,6 @@ public class ServerSecret extends CrossTldSingleton implements NonReplicatedEnti @Transient long leastSignificant; /** The UUID value itself. */ - @Id @Column(columnDefinition = "uuid") @Ignore UUID secret; diff --git a/core/src/main/java/google/registry/model/tmch/TmchCrl.java b/core/src/main/java/google/registry/model/tmch/TmchCrl.java index 11c41733f..81c699f12 100644 --- a/core/src/main/java/google/registry/model/tmch/TmchCrl.java +++ b/core/src/main/java/google/registry/model/tmch/TmchCrl.java @@ -25,15 +25,11 @@ import com.googlecode.objectify.annotation.Entity; import google.registry.model.annotations.NotBackedUp; import google.registry.model.annotations.NotBackedUp.Reason; import google.registry.model.common.CrossTldSingleton; -import google.registry.model.tmch.TmchCrl.TmchCrlId; import google.registry.persistence.VKey; import google.registry.schema.replay.NonReplicatedEntity; -import java.io.Serializable; import java.util.Optional; import javax.annotation.concurrent.Immutable; import javax.persistence.Column; -import javax.persistence.Id; -import javax.persistence.IdClass; import org.joda.time.DateTime; /** Datastore singleton for ICANN's TMCH CA certificate revocation list (CRL). */ @@ -41,22 +37,26 @@ import org.joda.time.DateTime; @javax.persistence.Entity @Immutable @NotBackedUp(reason = Reason.EXTERNALLY_SOURCED) -@IdClass(TmchCrlId.class) public final class TmchCrl extends CrossTldSingleton implements NonReplicatedEntity { - @Id String crl; + @Column(name = "certificateRevocations", nullable = false) + String crl; - @Id DateTime updated; + @Column(name = "updateTimestamp", nullable = false) + DateTime updated; - @Id String url; + @Column(nullable = false) + String url; /** Returns the singleton instance of this entity, without memoization. */ public static Optional get() { - VKey key = - VKey.create( - TmchCrl.class, SINGLETON_ID, Key.create(getCrossTldKey(), TmchCrl.class, SINGLETON_ID)); - // return the ofy() result during Datastore-primary phase - return ofyTm().transact(() -> ofyTm().loadByKeyIfPresent(key)); + return tm().transact( + () -> + tm().loadByKeyIfPresent( + VKey.create( + TmchCrl.class, + SINGLETON_ID, + Key.create(getCrossTldKey(), TmchCrl.class, SINGLETON_ID)))); } /** @@ -75,13 +75,7 @@ public final class TmchCrl extends CrossTldSingleton implements NonReplicatedEnt tmchCrl.crl = checkNotNull(crl, "crl"); tmchCrl.url = checkNotNull(url, "url"); ofyTm().transactNew(() -> ofyTm().putWithoutBackup(tmchCrl)); - jpaTm() - .transactNew( - () -> { - // Delete the old one and insert the new one - jpaTm().query("DELETE FROM TmchCrl").executeUpdate(); - jpaTm().putWithoutBackup(tmchCrl); - }); + jpaTm().transactNew(() -> jpaTm().putWithoutBackup(tmchCrl)); }); } @@ -99,26 +93,4 @@ public final class TmchCrl extends CrossTldSingleton implements NonReplicatedEnt public final DateTime getUpdated() { return updated; } - - static class TmchCrlId implements Serializable { - - @Column(name = "certificateRevocations") - String crl; - - @Column(name = "updateTimestamp") - DateTime updated; - - String url; - - /** Hibernate requires this default constructor. */ - private TmchCrlId() {} - - static TmchCrlId create(String crl, DateTime updated, String url) { - TmchCrlId result = new TmchCrlId(); - result.crl = crl; - result.updated = updated; - result.url = url; - return result; - } - } } diff --git a/core/src/test/java/google/registry/model/server/ServerSecretTest.java b/core/src/test/java/google/registry/model/server/ServerSecretTest.java index a5bf4320e..b7decf038 100644 --- a/core/src/test/java/google/registry/model/server/ServerSecretTest.java +++ b/core/src/test/java/google/registry/model/server/ServerSecretTest.java @@ -15,16 +15,19 @@ package google.registry.model.server; import static com.google.common.truth.Truth.assertThat; -import static google.registry.model.ofy.ObjectifyService.ofy; -import static google.registry.persistence.transaction.TransactionManagerFactory.jpaTm; +import static google.registry.testing.DatabaseHelper.loadByEntity; +import static google.registry.testing.DatabaseHelper.persistResource; import google.registry.model.EntityTestCase; import google.registry.model.ofy.RequestCapturingAsyncDatastoreService; +import google.registry.testing.DualDatabaseTest; +import google.registry.testing.TestOfyAndSql; +import google.registry.testing.TestOfyOnly; import java.util.UUID; import org.junit.jupiter.api.BeforeEach; -import org.junit.jupiter.api.Test; /** Unit tests for {@link ServerSecret}. */ +@DualDatabaseTest public class ServerSecretTest extends EntityTestCase { ServerSecretTest() { @@ -36,24 +39,22 @@ public class ServerSecretTest extends EntityTestCase { ServerSecret.resetCache(); } - @Test + @TestOfyAndSql void testGet_bootstrapping_savesSecretToDatastore() { ServerSecret secret = ServerSecret.get(); assertThat(secret).isNotNull(); - assertThat(ofy().load().entity(new ServerSecret()).now()).isEqualTo(secret); - assertThat(loadFromSql()).isEqualTo(secret); + assertThat(loadByEntity(new ServerSecret())).isEqualTo(secret); } - @Test + @TestOfyAndSql void testGet_existingSecret_returned() { ServerSecret secret = ServerSecret.create(new UUID(123, 456)); - ofy().saveWithoutBackup().entity(secret).now(); + persistResource(secret); assertThat(ServerSecret.get()).isEqualTo(secret); - assertThat(ofy().load().entity(new ServerSecret()).now()).isEqualTo(secret); - assertThat(loadFromSql()).isEqualTo(secret); + assertThat(loadByEntity(new ServerSecret())).isEqualTo(secret); } - @Test + @TestOfyOnly // relies on request-capturing datastore void testGet_cachedSecret() { int numInitialReads = RequestCapturingAsyncDatastoreService.getReads().size(); ServerSecret secret = ServerSecret.get(); @@ -63,21 +64,9 @@ public class ServerSecretTest extends EntityTestCase { assertThat(RequestCapturingAsyncDatastoreService.getReads()).hasSize(numReads); } - @Test + @TestOfyAndSql void testAsBytes() { byte[] bytes = ServerSecret.create(new UUID(123, 0x456)).asBytes(); assertThat(bytes).isEqualTo(new byte[] {0, 0, 0, 0, 0, 0, 0, 123, 0, 0, 0, 0, 0, 0, 0x4, 0x56}); } - - private static ServerSecret loadFromSql() { - return jpaTm() - .transact( - () -> - jpaTm() - .query("FROM ServerSecret", ServerSecret.class) - .setMaxResults(1) - .getResultStream() - .findFirst() - .get()); - } } diff --git a/core/src/test/java/google/registry/model/tmch/TmchCrlTest.java b/core/src/test/java/google/registry/model/tmch/TmchCrlTest.java index 41a7e46e3..f1473d60e 100644 --- a/core/src/test/java/google/registry/model/tmch/TmchCrlTest.java +++ b/core/src/test/java/google/registry/model/tmch/TmchCrlTest.java @@ -15,61 +15,43 @@ package google.registry.model.tmch; import static com.google.common.truth.Truth.assertThat; -import static google.registry.model.ofy.ObjectifyService.ofy; -import static google.registry.persistence.transaction.TransactionManagerFactory.jpaTm; +import static google.registry.testing.DatabaseHelper.loadByEntity; import google.registry.model.EntityTestCase; +import google.registry.testing.DualDatabaseTest; +import google.registry.testing.TestOfyAndSql; import java.util.Optional; -import org.junit.jupiter.api.Test; /** Unit tests for {@link TmchCrl}. */ +@DualDatabaseTest public class TmchCrlTest extends EntityTestCase { TmchCrlTest() { super(JpaEntityCoverageCheck.ENABLED); } - @Test + @TestOfyAndSql void testSuccess() { assertThat(TmchCrl.get()).isEqualTo(Optional.empty()); TmchCrl.set("lolcat", "https://lol.cat"); assertThat(TmchCrl.get().get().getCrl()).isEqualTo("lolcat"); } - @Test + @TestOfyAndSql void testDualWrite() { TmchCrl expected = new TmchCrl(); expected.crl = "lolcat"; expected.url = "https://lol.cat"; expected.updated = fakeClock.nowUtc(); TmchCrl.set("lolcat", "https://lol.cat"); - assertThat(ofy().load().entity(new TmchCrl()).now()).isEqualTo(expected); - assertThat(loadFromSql()).isEqualTo(expected); + assertThat(loadByEntity(new TmchCrl())).isEqualTo(expected); } - @Test + @TestOfyAndSql void testMultipleWrites() { TmchCrl.set("first", "https://first.cat"); assertThat(TmchCrl.get().get().getCrl()).isEqualTo("first"); TmchCrl.set("second", "https://second.cat"); assertThat(TmchCrl.get().get().getCrl()).isEqualTo("second"); - jpaTm() - .transact( - () -> - assertThat( - jpaTm().query("SELECT COUNT(*) FROM TmchCrl", Long.class).getSingleResult()) - .isEqualTo(1L)); - } - - private static TmchCrl loadFromSql() { - return jpaTm() - .transact( - () -> - jpaTm() - .query("FROM TmchCrl", TmchCrl.class) - .setMaxResults(1) - .getResultStream() - .findFirst() - .get()); } } diff --git a/db/src/main/resources/sql/er_diagram/brief_er_diagram.html b/db/src/main/resources/sql/er_diagram/brief_er_diagram.html index d2840093c..bf5be2576 100644 --- a/db/src/main/resources/sql/er_diagram/brief_er_diagram.html +++ b/db/src/main/resources/sql/er_diagram/brief_er_diagram.html @@ -261,19 +261,19 @@ td.section { generated on - 2021-04-08 17:21:58.993542 + 2021-04-12 17:28:08.926599 last flyway file - V91__defer_fkeys.sql + V92__singletons.sql

 

 

- + SchemaCrawler_Diagram - + generated by @@ -284,7 +284,7 @@ td.section { generated on - 2021-04-08 17:21:58.993542 + 2021-04-12 17:28:08.926599 @@ -2849,23 +2849,23 @@ td.section { serversecret_6cc90f09 - - + + public.ServerSecret - - + + [table] - - secret + + id - + - - uuid not null + + int8 not null - + signedmarkrevocationentry_99c39721 @@ -3004,64 +3004,48 @@ td.section { tmchcrl_d282355 - - + + public.TmchCrl - - + + [table] - - certificate_revocations + + id - + - - text not null + + int8 not null - - update_timestamp - - - - - timestamptz not null - - - url - - - - - text not null - - + transaction_d50389d4 - - + + public.Transaction - - + + [table] - + id - + - + bigserial not null - + - + auto-incremented - + @@ -6427,8 +6411,8 @@ td.section {
-
secret - uuid not null + id + int8 not null @@ -6445,7 +6429,7 @@ td.section { - secret + id @@ -6708,18 +6692,8 @@ td.section { - certificate_revocations - text not null - - - - update_timestamp - timestamptz not null - - - - url - text not null + id + int8 not null @@ -6736,17 +6710,7 @@ td.section { - certificate_revocations - - - - - update_timestamp - - - - - url + id diff --git a/db/src/main/resources/sql/er_diagram/full_er_diagram.html b/db/src/main/resources/sql/er_diagram/full_er_diagram.html index 0bc6414ce..42bb8c0ae 100644 --- a/db/src/main/resources/sql/er_diagram/full_er_diagram.html +++ b/db/src/main/resources/sql/er_diagram/full_er_diagram.html @@ -261,19 +261,19 @@ td.section { generated on - 2021-04-08 17:21:56.891855 + 2021-04-12 17:28:07.032036 last flyway file - V91__defer_fkeys.sql + V92__singletons.sql

 

 

- + SchemaCrawler_Diagram - + generated by @@ -284,7 +284,7 @@ td.section { generated on - 2021-04-08 17:21:56.891855 + 2021-04-12 17:28:07.032036 @@ -6233,139 +6233,155 @@ td.section { serversecret_6cc90f09 - - + + public.ServerSecret - - + + [table] - + secret - + - + uuid not null - + + id + + + + + int8 not null + + signedmarkrevocationentry_99c39721 - - + + public.SignedMarkRevocationEntry - - + + [table] - + revision_id + + + + int8 not null + + + revocation_time + - int8 not null + timestamptz not null - - revocation_time + + smd_id - timestamptz not null - - - smd_id - - - - text not null - + signedmarkrevocationlist_c5d968fb - - + + public.SignedMarkRevocationList - - + + [table] - + revision_id + + + + bigserial not null + - bigserial not null + auto-incremented + + + creation_time - auto-incremented - - - creation_time - - - - timestamptz - + signedmarkrevocationentry_99c39721:w->signedmarkrevocationlist_c5d968fb:e - - - - - - - - + + + + + + + + fk5ivlhvs3121yx2li5tqh54u4 spec11threatmatch_a61228a6 - - + + public.Spec11ThreatMatch - - + + [table] - + id + + + + bigserial not null + - bigserial not null + auto-incremented + + + check_date - auto-incremented + date not null - check_date + domain_name - date not null + text not null - domain_name + domain_repo_id @@ -6373,7 +6389,7 @@ td.section { text not null - domain_repo_id + registrar_id @@ -6381,127 +6397,127 @@ td.section { text not null - registrar_id + threat_types - text not null + _text not null - threat_types + tld - _text not null - - - tld - - - - text not null - + sqlreplaycheckpoint_342081b3 - - + + public.SqlReplayCheckpoint - - + + [table] - + revision_id + + + + int8 not null + + + last_replay_time + - int8 not null - - - last_replay_time - - - - timestamptz not null - + tmchcrl_d282355 - - + + public.TmchCrl - - + + [table] - + certificate_revocations - + - + text not null - + update_timestamp - + - + timestamptz not null - + url - + - + text not null - + + id + + + + + int8 not null + + transaction_d50389d4 - - + + public.Transaction - - + + [table] - + id + + + + bigserial not null + + + + + auto-incremented + + + contents + - bigserial not null - - - - - auto-incremented - - - contents - - - - bytea - + @@ -13062,9 +13078,14 @@ td.section {
-
secret + secret uuid not null + + + id + int8 not null + @@ -13080,7 +13101,7 @@ td.section { - secret + id @@ -13098,7 +13119,7 @@ td.section { - secret + id ascending @@ -13707,19 +13728,24 @@ td.section { - certificate_revocations + certificate_revocations text not null - update_timestamp + update_timestamp timestamptz not null - url + url text not null + + + id + int8 not null + @@ -13735,17 +13761,7 @@ td.section { - certificate_revocations - - - - - update_timestamp - - - - - url + id @@ -13763,17 +13779,7 @@ td.section { - certificate_revocations - ascending - - - - update_timestamp - ascending - - - - url + id ascending diff --git a/db/src/main/resources/sql/flyway.txt b/db/src/main/resources/sql/flyway.txt index 205230686..b5d64e597 100644 --- a/db/src/main/resources/sql/flyway.txt +++ b/db/src/main/resources/sql/flyway.txt @@ -89,3 +89,4 @@ V88__transfer_billing_cancellation_history_id.sql V89__host_history_host_deferred.sql V90__update_timestamp.sql V91__defer_fkeys.sql +V92__singletons.sql diff --git a/db/src/main/resources/sql/flyway/V92__singletons.sql b/db/src/main/resources/sql/flyway/V92__singletons.sql new file mode 100644 index 000000000..eba670422 --- /dev/null +++ b/db/src/main/resources/sql/flyway/V92__singletons.sql @@ -0,0 +1,21 @@ +-- Copyright 2021 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. + +ALTER TABLE "ServerSecret" DROP CONSTRAINT "ServerSecret_pkey"; +ALTER TABLE "ServerSecret" ADD COLUMN id bigint NOT NULL; +ALTER TABLE "ServerSecret" ADD PRIMARY KEY(id); + +ALTER TABLE "TmchCrl" DROP CONSTRAINT "TmchCrl_pkey"; +ALTER TABLE "TmchCrl" ADD COLUMN id bigint NOT NULL; +ALTER TABLE "TmchCrl" ADD PRIMARY KEY(id); diff --git a/db/src/main/resources/sql/schema/db-schema.sql.generated b/db/src/main/resources/sql/schema/db-schema.sql.generated index 24e40854a..c6c06137d 100644 --- a/db/src/main/resources/sql/schema/db-schema.sql.generated +++ b/db/src/main/resources/sql/schema/db-schema.sql.generated @@ -665,8 +665,9 @@ ); create table "ServerSecret" ( - secret uuid not null, - primary key (secret) + id int8 not null, + secret uuid, + primary key (id) ); create table "SignedMarkRevocationEntry" ( @@ -742,10 +743,11 @@ ); create table "TmchCrl" ( - certificate_revocations text not null, + id int8 not null, + certificate_revocations text not null, update_timestamp timestamptz not null, url text not null, - primary key (certificate_revocations, update_timestamp, url) + primary key (id) ); create table "Transaction" ( diff --git a/db/src/main/resources/sql/schema/nomulus.golden.sql b/db/src/main/resources/sql/schema/nomulus.golden.sql index a81d39259..e85acbce3 100644 --- a/db/src/main/resources/sql/schema/nomulus.golden.sql +++ b/db/src/main/resources/sql/schema/nomulus.golden.sql @@ -948,7 +948,8 @@ ALTER SEQUENCE public."SafeBrowsingThreat_id_seq" OWNED BY public."Spec11ThreatM -- CREATE TABLE public."ServerSecret" ( - secret uuid NOT NULL + secret uuid NOT NULL, + id bigint NOT NULL ); @@ -1055,7 +1056,8 @@ CREATE TABLE public."Tld" ( CREATE TABLE public."TmchCrl" ( certificate_revocations text NOT NULL, update_timestamp timestamp with time zone NOT NULL, - url text NOT NULL + url text NOT NULL, + id bigint NOT NULL ); @@ -1389,7 +1391,7 @@ ALTER TABLE ONLY public."Spec11ThreatMatch" -- ALTER TABLE ONLY public."ServerSecret" - ADD CONSTRAINT "ServerSecret_pkey" PRIMARY KEY (secret); + ADD CONSTRAINT "ServerSecret_pkey" PRIMARY KEY (id); -- @@ -1429,7 +1431,7 @@ ALTER TABLE ONLY public."Tld" -- ALTER TABLE ONLY public."TmchCrl" - ADD CONSTRAINT "TmchCrl_pkey" PRIMARY KEY (certificate_revocations, update_timestamp, url); + ADD CONSTRAINT "TmchCrl_pkey" PRIMARY KEY (id); --