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