Create DAO for Spec11ThreatMatch

[leginachen]

To avoid duplicate entries in Spec11ThreatMatch when backfilling, this DAO deletes all entries for a particular date every time a new entry for that date is persisted.

---

This change is 

[gbrodman]

Reviewable status: 0 of 2 files reviewed, 7 unresolved discussions (waiting on @gbrodman, @leginachen, and @sarahcaseybot)

---

core/src/main/java/google/registry/model/reporting/Spec11ThreatMatchDao.java, line 21 at r1 (raw file):

import org.joda.time.LocalDate;

/** Data access object for {@link google.registry.model.reporting.Spec11ThreatMatch} */

nit: period at the end if the comment after the }

---

core/src/main/java/google/registry/model/reporting/Spec11ThreatMatchDao.java, line 23 at r1 (raw file):

/** Data access object for {@link google.registry.model.reporting.Spec11ThreatMatch} */
public class Spec11ThreatMatchDao {

There should be a comment explaining why we pass in the transaction manager, rather than using jpaTm() like we do in most other places

---

core/src/main/java/google/registry/model/reporting/Spec11ThreatMatchDao.java, line 42 at r1 (raw file):

            .getEntityManager()
            .createQuery(
                "SELECT domainName FROM Spec11ThreatMatch WHERE check_date = :date", String.class)

any particular reason not to select the whole object?

---

core/src/test/java/google/registry/model/reporting/Spec11ThreatMatchDaoTest.java, line 35 at r1 (raw file):

  Spec11ThreatMatchDaoTest() {
    super(JpaEntityCoverageCheck.ENABLED);

Does it work without this constructor, or if you use DISABLED? We test the persistence of Spec11ThreatMatch objects in other tests, so the coverage check should already pass

---

core/src/test/java/google/registry/model/reporting/Spec11ThreatMatchDaoTest.java, line 46 at r1 (raw file):

              jpaTm().saveNew(createThreatMatchByDate("today.org", TODAY));
              jpaTm().saveNew(createThreatMatchByDate("yesterday.com", YESTERDAY));
              ImmutableList<Spec11ThreatMatch> domains = jpaTm().loadAll(Spec11ThreatMatch.class);

i don't think this load does anything

---

core/src/test/java/google/registry/model/reporting/Spec11ThreatMatchDaoTest.java, line 52 at r1 (raw file):

  @Test
  void testDeleteEntriesByDate() {
    JpaTransactionManager jpaTm = jpaTm();

no need to do this separately, just use jpaTm() everywhere

---

core/src/test/java/google/registry/model/reporting/Spec11ThreatMatchDaoTest.java, line 68 at r1 (raw file):

          ImmutableList<String> persistedYesterday =
              Spec11ThreatMatchDao.loadEntriesByDate(jpaTm, YESTERDAY);
          assertThat(persistedYesterday).contains("yesterday.com");

"contains" is a bit vague. We should be checking that the result contains exactly what we'd expect -- no more, no less.

[leginachen]

Reviewable status: 0 of 2 files reviewed, 6 unresolved discussions (waiting on @gbrodman and @sarahcaseybot)

---

core/src/main/java/google/registry/model/reporting/Spec11ThreatMatchDao.java, line 23 at r1 (raw file):

Previously, gbrodman wrote…

There should be a comment explaining why we pass in the transaction manager, rather than using jpaTm() like we do in most other places

Done.

---

core/src/main/java/google/registry/model/reporting/Spec11ThreatMatchDao.java, line 42 at r1 (raw file):

Previously, gbrodman wrote…

any particular reason not to select the whole object?

Yeah, I ran into errors selecting the whole object. I don't think it knows how to convert rows into entities. I just tried it again and got an unexpected token: * error at org.hibernate.hql.internal.antlr.HqlBaseParser.selectClause

---

core/src/test/java/google/registry/model/reporting/Spec11ThreatMatchDaoTest.java, line 35 at r1 (raw file):

Previously, gbrodman wrote…

Does it work without this constructor, or if you use DISABLED? We test the persistence of Spec11ThreatMatch objects in other tests, so the coverage check should already pass

Yes

---

core/src/test/java/google/registry/model/reporting/Spec11ThreatMatchDaoTest.java, line 46 at r1 (raw file):

Previously, gbrodman wrote…

i don't think this load does anything

Removed during last commit

---

core/src/test/java/google/registry/model/reporting/Spec11ThreatMatchDaoTest.java, line 52 at r1 (raw file):

Previously, gbrodman wrote…

no need to do this separately, just use jpaTm() everywhere

Done.

---

core/src/test/java/google/registry/model/reporting/Spec11ThreatMatchDaoTest.java, line 68 at r1 (raw file):

Previously, gbrodman wrote…

"contains" is a bit vague. We should be checking that the result contains exactly what we'd expect -- no more, no less.

Used containsExactly instead. thanks!

[gbrodman]

Reviewable status: 0 of 2 files reviewed, 5 unresolved discussions (waiting on @gbrodman, @leginachen, and @sarahcaseybot)

---

core/src/main/java/google/registry/model/reporting/Spec11ThreatMatchDao.java, line 23 at r1 (raw file):

Previously, leginachen (Legina Chen) wrote…

Done.

we should have it as a class-level comment instead of having to repeat it for every method

---

core/src/main/java/google/registry/model/reporting/Spec11ThreatMatchDao.java, line 42 at r1 (raw file):

Previously, leginachen (Legina Chen) wrote…

Yeah, I ran into errors selecting the whole object. I don't think it knows how to convert rows into entities. I just tried it again and got an unexpected token: * error at org.hibernate.hql.internal.antlr.HqlBaseParser.selectClause

Did you try the syntax that, for instance, the RegistryLockDao uses where it's like SELECT match FROM Spec11ThreatMatch match WHERE match.check_date = :date" ?

---

core/src/main/java/google/registry/model/reporting/Spec11ThreatMatchDao.java, line 27 at r2 (raw file):

   * Delete all entries with the specified date from the database. A JpaTransactionManager is passed
   * in because this method is called from a BEAM pipeline and we don't know where it's coming from.
   */

Might not be necessary after the comment update, but java doc entries, if they're multiline, should consist of a single sentence fragment in the first line, followed by line breaks, followed by a

tag with the text

https://engdoc.corp.google.com/eng/doc/devguide/java/style/index.md?cl=head#s7-javadoc

/**
 * An example of javadoc formatting.
 * 
 * <p>This is some more text that can span multiple lines.
 */

---

core/src/test/java/google/registry/model/reporting/Spec11ThreatMatchDaoTest.java, line 29 at r2 (raw file):

/** Unit tests for {@link Spec11ThreatMatchDao}. */
public class Spec11ThreatMatchDaoTest extends EntityTestCase {

nit: should be a blank line between the class declaration and the private static final vars

---

core/src/test/java/google/registry/model/reporting/Spec11ThreatMatchDaoTest.java, line 73 at r2 (raw file):

              ImmutableList<String> persisted =
                  Spec11ThreatMatchDao.loadEntriesByDate(jpaTm(), TODAY);
              assertThat(persisted).contains("today.com");

you can use containsExactly here as well with multiple values

[sarahcaseybot]

Reviewed 1 of 2 files at r2, 1 of 1 files at r3.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @leginachen)

[leginachen]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @gbrodman)

---

core/src/main/java/google/registry/model/reporting/Spec11ThreatMatchDao.java, line 23 at r1 (raw file):

Previously, gbrodman wrote…

we should have it as a class-level comment instead of having to repeat it for every method

Done.

---

core/src/main/java/google/registry/model/reporting/Spec11ThreatMatchDao.java, line 42 at r1 (raw file):

Previously, gbrodman wrote…

Did you try the syntax that, for instance, the RegistryLockDao uses where it's like SELECT match FROM Spec11ThreatMatch match WHERE match.check_date = :date" ?

Done.

---

core/src/main/java/google/registry/model/reporting/Spec11ThreatMatchDao.java, line 27 at r2 (raw file):

Previously, gbrodman wrote…

Might not be necessary after the comment update, but java doc entries, if they're multiline, should consist of a single sentence fragment in the first line, followed by line breaks, followed by a

tag with the text

https://engdoc.corp.google.com/eng/doc/devguide/java/style/index.md?cl=head#s7-javadoc

/**
 * An example of javadoc formatting.
 * 
 * <p>This is some more text that can span multiple lines.
 */

Done.

---

core/src/test/java/google/registry/model/reporting/Spec11ThreatMatchDaoTest.java, line 73 at r2 (raw file):

Previously, gbrodman wrote…

you can use containsExactly here as well with multiple values

Done.

[gbrodman]

overall looks good!

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @leginachen and @sarahcaseybot)

---

core/src/main/java/google/registry/model/reporting/Spec11ThreatMatchDao.java, line 27 at r2 (raw file):

Previously, leginachen (Legina Chen) wrote…

Done.

Same comment for class-level javadoc formatting

nit about having a blank line between the class declaration and the first method's javadoc

[leginachen]

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @gbrodman and @sarahcaseybot)

---

core/src/main/java/google/registry/model/reporting/Spec11ThreatMatchDao.java, line 27 at r2 (raw file):

Previously, gbrodman wrote…

Same comment for class-level javadoc formatting

nit about having a blank line between the class declaration and the first method's javadoc

DOne.

[gbrodman]

Reviewable status: 0 of 2 files reviewed, all discussions resolved (waiting on @sarahcaseybot)