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); --