Allow multiple threat types in the Spec11ThreatMatch table (#650)

* Update to generic Spec11ThreatMatch table

* Fix SQL syntax

* Make changes to the schema and add a test for null and empty threatTypes

* Fix a small typo

* Change the exception thrown with illegal arguments

Change the import for isNullOrEmpty

* Fix import for checkArgument

* Added a threat to test multiple threat types

core/src/main/java/google/registry/model/reporting/Spec11ThreatMatch.java

@@ -14,18 +14,20 @@
 
 package google.registry.model.reporting;
 
+import static com.google.common.base.Preconditions.checkArgument;
+import static google.registry.util.CollectionUtils.isNullOrEmpty;
 import static google.registry.util.PreconditionsUtils.checkArgumentNotNull;
 
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
 import google.registry.model.Buildable;
 import google.registry.model.ImmutableObject;
 import google.registry.schema.replay.DatastoreEntity;
 import google.registry.schema.replay.SqlEntity;
 import google.registry.util.DomainNameUtils;
+import java.util.Set;
 import javax.persistence.Column;
 import javax.persistence.Entity;
-import javax.persistence.EnumType;
-import javax.persistence.Enumerated;
 import javax.persistence.GeneratedValue;
 import javax.persistence.GenerationType;
 import javax.persistence.Id;
@@ -36,11 +38,11 @@ import org.joda.time.LocalDate;
 @Entity
 @Table(
     indexes = {
-      @Index(name = "safebrowsing_threat_registrar_id_idx", columnList = "registrarId"),
-      @Index(name = "safebrowsing_threat_tld_idx", columnList = "tld"),
-      @Index(name = "safebrowsing_threat_check_date_idx", columnList = "checkDate")
+      @Index(name = "spec11threatmatch_registrar_id_idx", columnList = "registrarId"),
+      @Index(name = "spec11threatmatch_tld_idx", columnList = "tld"),
+      @Index(name = "spec11threatmatch_check_date_idx", columnList = "checkDate")
     })
-public class SafeBrowsingThreat extends ImmutableObject implements Buildable, SqlEntity {
+public class Spec11ThreatMatch extends ImmutableObject implements Buildable, SqlEntity {
 
   /** The type of threat detected. */
   public enum ThreatType {
@@ -60,10 +62,9 @@ public class SafeBrowsingThreat extends ImmutableObject implements Buildable, Sq
   @Column(nullable = false)
   String domainName;
 
-  /** The type of threat detected. */
+  /** The types of threat detected. */
   @Column(nullable = false)
-  @Enumerated(EnumType.STRING)
-  ThreatType threatType;
+  Set<ThreatType> threatTypes;
 
   /** Primary key of the domain table and unique identifier for all EPP resources. */
   @Column(nullable = false)
@@ -89,8 +90,8 @@ public class SafeBrowsingThreat extends ImmutableObject implements Buildable, Sq
     return domainName;
   }
 
-  public ThreatType getThreatType() {
-    return threatType;
+  public ImmutableSet<ThreatType> getThreatTypes() {
+    return ImmutableSet.copyOf(threatTypes);
   }
 
   public String getDomainRepoId() {
@@ -119,18 +120,18 @@ public class SafeBrowsingThreat extends ImmutableObject implements Buildable, Sq
     return new Builder(clone(this));
   }
 
-  /** A builder for constructing {@link SafeBrowsingThreat}, since it is immutable. */
-  public static class Builder extends Buildable.Builder<SafeBrowsingThreat> {
+  /** A builder for constructing {@link Spec11ThreatMatch}, since it is immutable. */
+  public static class Builder extends Buildable.Builder<Spec11ThreatMatch> {
     public Builder() {}
 
-    private Builder(SafeBrowsingThreat instance) {
+    private Builder(Spec11ThreatMatch instance) {
       super(instance);
     }
 
     @Override
-    public SafeBrowsingThreat build() {
+    public Spec11ThreatMatch build() {
       checkArgumentNotNull(getInstance().domainName, "Domain name cannot be null");
-      checkArgumentNotNull(getInstance().threatType, "Threat type cannot be null");
+      checkArgumentNotNull(getInstance().threatTypes, "Threat types cannot be null");
       checkArgumentNotNull(getInstance().domainRepoId, "Repo ID cannot be null");
       checkArgumentNotNull(getInstance().registrarId, "Registrar ID cannot be null");
       checkArgumentNotNull(getInstance().checkDate, "Check date cannot be null");
@@ -145,8 +146,10 @@ public class SafeBrowsingThreat extends ImmutableObject implements Buildable, Sq
       return this;
     }
 
-    public Builder setThreatType(ThreatType threatType) {
-      getInstance().threatType = threatType;
+    public Builder setThreatTypes(ImmutableSet<ThreatType> threatTypes) {
+      checkArgument(!isNullOrEmpty(threatTypes), "threatTypes cannot be null or empty.");
+
+      getInstance().threatTypes = threatTypes;
       return this;
     }
 

core/src/main/java/google/registry/persistence/converter/Spec11ThreatMatchThreatTypeSetConverter.java

@@ -0,0 +1,34 @@
+// Copyright 2020 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.
+
+package google.registry.persistence.converter;
+
+import google.registry.model.reporting.Spec11ThreatMatch.ThreatType;
+import javax.persistence.AttributeConverter;
+import javax.persistence.Converter;
+
+/** JPA {@link AttributeConverter} for storing/retrieving {@code Set}. */
+@Converter(autoApply = true)
+public class Spec11ThreatMatchThreatTypeSetConverter extends StringSetConverterBase<ThreatType> {
+
+  @Override
+  String toString(ThreatType element) {
+    return element.name();
+  }
+
+  @Override
+  ThreatType fromString(String value) {
+    return ThreatType.valueOf(value);
+  }
+}

core/src/main/resources/META-INF/persistence.xml

@@ -28,7 +28,7 @@
     <class>google.registry.model.host.HostResource</class>
     <class>google.registry.model.registrar.Registrar</class>
     <class>google.registry.model.registrar.RegistrarContact</class>
-    <class>google.registry.model.reporting.SafeBrowsingThreat</class>
+    <class>google.registry.model.reporting.Spec11ThreatMatch</class>
     <class>google.registry.schema.domain.RegistryLock</class>
     <class>google.registry.schema.tmch.ClaimsList</class>
     <class>google.registry.schema.cursor.Cursor</class>
@@ -56,6 +56,7 @@
     <class>google.registry.persistence.converter.LocalDateConverter</class>
     <class>google.registry.persistence.converter.PostalInfoChoiceListConverter</class>
     <class>google.registry.persistence.converter.RegistrarPocSetConverter</class>
+    <class>google.registry.persistence.converter.Spec11ThreatMatchThreatTypeSetConverter</class>
     <class>google.registry.persistence.converter.StatusValueSetConverter</class>
     <class>google.registry.persistence.converter.StringListConverter</class>
     <class>google.registry.persistence.converter.StringSetConverter</class>

core/src/test/java/google/registry/model/reporting/Spec11ThreatMatchTest.java

@@ -15,7 +15,8 @@
 package google.registry.model.reporting;
 
 import static com.google.common.truth.Truth.assertThat;
-import static google.registry.model.reporting.SafeBrowsingThreat.ThreatType.MALWARE;
+import static google.registry.model.reporting.Spec11ThreatMatch.ThreatType.MALWARE;
+import static google.registry.model.reporting.Spec11ThreatMatch.ThreatType.UNWANTED_SOFTWARE;
 import static google.registry.persistence.transaction.TransactionManagerFactory.jpaTm;
 import static google.registry.testing.DatastoreHelper.createTld;
 import static google.registry.testing.SqlHelper.assertThrowForeignKeyViolation;
@@ -34,18 +35,18 @@ import org.joda.time.format.ISODateTimeFormat;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 
-/** Unit tests for {@link SafeBrowsingThreat}. */
-public class SafeBrowsingThreatTest extends EntityTestCase {
+/** Unit tests for {@link Spec11ThreatMatch}. */
+public class Spec11ThreatMatchTest extends EntityTestCase {
 
   private static final String REGISTRAR_ID = "registrar";
   private static final LocalDate DATE = LocalDate.parse("2020-06-10", ISODateTimeFormat.date());
 
-  private SafeBrowsingThreat threat;
+  private Spec11ThreatMatch threat;
   private DomainBase domain;
   private HostResource host;
   private ContactResource registrantContact;
 
-  public SafeBrowsingThreatTest() {
+  public Spec11ThreatMatchTest() {
     super(true);
   }
 
@@ -89,8 +90,8 @@ public class SafeBrowsingThreatTest extends EntityTestCase {
             .build();
 
     threat =
-        new SafeBrowsingThreat.Builder()
-            .setThreatType(MALWARE)
+        new Spec11ThreatMatch.Builder()
+            .setThreatTypes(ImmutableSet.of(MALWARE, UNWANTED_SOFTWARE))
             .setCheckDate(DATE)
             .setDomainName("foo.tld")
             .setDomainRepoId(domainRepoId)
@@ -111,8 +112,8 @@ public class SafeBrowsingThreatTest extends EntityTestCase {
               jpaTm().saveNew(threat);
             });
 
-    VKey<SafeBrowsingThreat> threatVKey = VKey.createSql(SafeBrowsingThreat.class, threat.getId());
-    SafeBrowsingThreat persistedThreat = jpaTm().transact(() -> jpaTm().load(threatVKey));
+    VKey<Spec11ThreatMatch> threatVKey = VKey.createSql(Spec11ThreatMatch.class, threat.getId());
+    Spec11ThreatMatch persistedThreat = jpaTm().transact(() -> jpaTm().load(threatVKey));
     threat.id = persistedThreat.id;
     assertThat(threat).isEqualTo(persistedThreat);
   }
@@ -148,7 +149,7 @@ public class SafeBrowsingThreatTest extends EntityTestCase {
   }
 
   @Test
-  public void testFailure_threatsWithNullFields() {
+  public void testFailure_threatsWithInvalidFields() {
     assertThrows(
         IllegalArgumentException.class, () -> threat.asBuilder().setRegistrarId(null).build());
 
@@ -159,9 +160,12 @@ public class SafeBrowsingThreatTest extends EntityTestCase {
         IllegalArgumentException.class, () -> threat.asBuilder().setCheckDate(null).build());
 
     assertThrows(
-        IllegalArgumentException.class, () -> threat.asBuilder().setThreatType(null).build());
+        IllegalArgumentException.class, () -> threat.asBuilder().setDomainRepoId(null).build());
 
     assertThrows(
-        IllegalArgumentException.class, () -> threat.asBuilder().setDomainRepoId(null).build());
+        IllegalArgumentException.class, () -> threat.asBuilder().setThreatTypes(ImmutableSet.of()));
+
+    assertThrows(
+        IllegalArgumentException.class, () -> threat.asBuilder().setThreatTypes(null).build());
   }
 }

core/src/test/java/google/registry/schema/integration/SqlIntegrationTestSuite.java

@@ -22,7 +22,7 @@ import google.registry.model.domain.DomainBaseSqlTest;
 import google.registry.model.history.HostHistoryTest;
 import google.registry.model.poll.PollMessageTest;
 import google.registry.model.registry.RegistryLockDaoTest;
-import google.registry.model.reporting.SafeBrowsingThreatTest;
+import google.registry.model.reporting.Spec11ThreatMatchTest;
 import google.registry.persistence.transaction.JpaEntityCoverage;
 import google.registry.schema.cursor.CursorDaoTest;
 import google.registry.schema.integration.SqlIntegrationTestSuite.AfterSuiteTest;
@@ -84,7 +84,7 @@ import org.junit.runner.RunWith;
   RegistrarDaoTest.class,
   RegistryLockDaoTest.class,
   ReservedListDaoTest.class,
-  SafeBrowsingThreatTest.class,
+  Spec11ThreatMatchTest.class,
   // AfterSuiteTest must be the last entry. See class javadoc for details.
   AfterSuiteTest.class
 })

db/src/main/resources/sql/flyway/V37__update_spec11threatmatch.sql

@@ -0,0 +1,31 @@
+-- Copyright 2020 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 if exists "SafeBrowsingThreat"
+  rename to "Spec11ThreatMatch";
+
+alter table if exists "Spec11ThreatMatch"
+  alter column "threat_type" type text[] using threat_type::text[];
+
+alter table if exists "Spec11ThreatMatch"
+  rename column "threat_type" to "threat_types";
+
+alter index if exists "safebrowsing_threat_registrar_id_idx"
+  rename to "spec11threatmatch_registrar_id_idx";
+
+alter index if exists "safebrowsing_threat_tld_idx"
+  rename to "spec11threatmatch_tld_idx";
+
+alter index if exists "safebrowsing_threat_check_date_idx"
+  rename to "spec11threatmatch_check_date_idx";

db/src/main/resources/sql/schema/db-schema.sql.generated

@@ -408,13 +408,13 @@ create sequence history_id_sequence start 1 increment 1;
         primary key (revision_id)
     );
 
-    create table "SafeBrowsingThreat" (
+    create table "Spec11ThreatMatch" (
        id  bigserial not null,
         check_date text not null,
         domain_name text not null,
         domain_repo_id text not null,
         registrar_id text not null,
-        threat_type text not null,
+        threat_types text[] not null,
         tld text not null,
         primary key (id)
     );
@@ -459,9 +459,9 @@ create index idx_registry_lock_registrar_id on "RegistryLock" (registrar_id);
     alter table if exists "RegistryLock" 
        add constraint idx_registry_lock_repo_id_revision_id unique (repo_id, revision_id);
 create index reservedlist_name_idx on "ReservedList" (name);
-create index safebrowsing_threat_registrar_id_idx on "SafeBrowsingThreat" (registrar_id);
-create index safebrowsing_threat_tld_idx on "SafeBrowsingThreat" (tld);
-create index safebrowsing_threat_check_date_idx on "SafeBrowsingThreat" (check_date);
+create index spec11threatmatch_registrar_id_idx on "Spec11ThreatMatch" (registrar_id);
+create index spec11threatmatch_tld_idx on "Spec11ThreatMatch" (tld);
+create index spec11threatmatch_check_date_idx on "Spec11ThreatMatch" (check_date);
 
     alter table if exists "ClaimsEntry" 
        add constraint FK6sc6at5hedffc0nhdcab6ivuq 

db/src/main/resources/sql/schema/nomulus.golden.sql

@@ -657,16 +657,16 @@ ALTER SEQUENCE public."ReservedList_revision_id_seq" OWNED BY public."ReservedLi
 
 
 --
--- Name: SafeBrowsingThreat; Type: TABLE; Schema: public; Owner: -
+-- Name: Spec11ThreatMatch; Type: TABLE; Schema: public; Owner: -
 --
 
-CREATE TABLE public."SafeBrowsingThreat" (
+CREATE TABLE public."Spec11ThreatMatch" (
     id bigint NOT NULL,
     check_date text NOT NULL,
     domain_name text NOT NULL,
     domain_repo_id text NOT NULL,
     registrar_id text NOT NULL,
-    threat_type text NOT NULL,
+    threat_types text[] NOT NULL,
     tld text NOT NULL
 );
 
@@ -687,7 +687,7 @@ CREATE SEQUENCE public."SafeBrowsingThreat_id_seq"
 -- Name: SafeBrowsingThreat_id_seq; Type: SEQUENCE OWNED BY; Schema: public; Owner: -
 --
 
-ALTER SEQUENCE public."SafeBrowsingThreat_id_seq" OWNED BY public."SafeBrowsingThreat".id;
+ALTER SEQUENCE public."SafeBrowsingThreat_id_seq" OWNED BY public."Spec11ThreatMatch".id;
 
 
 --
@@ -747,10 +747,10 @@ ALTER TABLE ONLY public."ReservedList" ALTER COLUMN revision_id SET DEFAULT next
 
 
 --
--- Name: SafeBrowsingThreat id; Type: DEFAULT; Schema: public; Owner: -
+-- Name: Spec11ThreatMatch id; Type: DEFAULT; Schema: public; Owner: -
 --
 
-ALTER TABLE ONLY public."SafeBrowsingThreat" ALTER COLUMN id SET DEFAULT nextval('public."SafeBrowsingThreat_id_seq"'::regclass);
+ALTER TABLE ONLY public."Spec11ThreatMatch" ALTER COLUMN id SET DEFAULT nextval('public."SafeBrowsingThreat_id_seq"'::regclass);
 
 
 --
@@ -906,10 +906,10 @@ ALTER TABLE ONLY public."ReservedList"
 
 
 --
--- Name: SafeBrowsingThreat SafeBrowsingThreat_pkey; Type: CONSTRAINT; Schema: public; Owner: -
+-- Name: Spec11ThreatMatch SafeBrowsingThreat_pkey; Type: CONSTRAINT; Schema: public; Owner: -
 --
 
-ALTER TABLE ONLY public."SafeBrowsingThreat"
+ALTER TABLE ONLY public."Spec11ThreatMatch"
     ADD CONSTRAINT "SafeBrowsingThreat_pkey" PRIMARY KEY (id);
 
 
@@ -1175,24 +1175,24 @@ CREATE INDEX reservedlist_name_idx ON public."ReservedList" USING btree (name);
 
 
 --
--- Name: safebrowsing_threat_check_date_idx; Type: INDEX; Schema: public; Owner: -
+-- Name: spec11threatmatch_check_date_idx; Type: INDEX; Schema: public; Owner: -
 --
 
-CREATE INDEX safebrowsing_threat_check_date_idx ON public."SafeBrowsingThreat" USING btree (check_date);
+CREATE INDEX spec11threatmatch_check_date_idx ON public."Spec11ThreatMatch" USING btree (check_date);
 
 
 --
--- Name: safebrowsing_threat_registrar_id_idx; Type: INDEX; Schema: public; Owner: -
+-- Name: spec11threatmatch_registrar_id_idx; Type: INDEX; Schema: public; Owner: -
 --
 
-CREATE INDEX safebrowsing_threat_registrar_id_idx ON public."SafeBrowsingThreat" USING btree (registrar_id);
+CREATE INDEX spec11threatmatch_registrar_id_idx ON public."Spec11ThreatMatch" USING btree (registrar_id);
 
 
 --
--- Name: safebrowsing_threat_tld_idx; Type: INDEX; Schema: public; Owner: -
+-- Name: spec11threatmatch_tld_idx; Type: INDEX; Schema: public; Owner: -
 --
 
-CREATE INDEX safebrowsing_threat_tld_idx ON public."SafeBrowsingThreat" USING btree (tld);
+CREATE INDEX spec11threatmatch_tld_idx ON public."Spec11ThreatMatch" USING btree (tld);
 
 
 --
@@ -1460,18 +1460,18 @@ ALTER TABLE ONLY public."PollMessage"
 
 
 --
--- Name: SafeBrowsingThreat fk_safebrowsing_threat_domain_repo_id; Type: FK CONSTRAINT; Schema: public; Owner: -
+-- Name: Spec11ThreatMatch fk_safebrowsing_threat_domain_repo_id; Type: FK CONSTRAINT; Schema: public; Owner: -
 --
 
-ALTER TABLE ONLY public."SafeBrowsingThreat"
+ALTER TABLE ONLY public."Spec11ThreatMatch"
     ADD CONSTRAINT fk_safebrowsing_threat_domain_repo_id FOREIGN KEY (domain_repo_id) REFERENCES public."Domain"(repo_id);
 
 
 --
--- Name: SafeBrowsingThreat fk_safebrowsing_threat_registrar_id; Type: FK CONSTRAINT; Schema: public; Owner: -
+-- Name: Spec11ThreatMatch fk_safebrowsing_threat_registrar_id; Type: FK CONSTRAINT; Schema: public; Owner: -
 --
 
-ALTER TABLE ONLY public."SafeBrowsingThreat"
+ALTER TABLE ONLY public."Spec11ThreatMatch"
     ADD CONSTRAINT fk_safebrowsing_threat_registrar_id FOREIGN KEY (registrar_id) REFERENCES public."Registrar"(registrar_id);