From 927e8bbd73b64163c95d3ed002e71db79e2c9a56 Mon Sep 17 00:00:00 2001 From: guyben Date: Tue, 5 Feb 2019 12:55:34 -0800 Subject: [PATCH] Move LocalDate injection to the Actions themselves We want to make it clear what query (or POST) inputs the user needs to / can give for each Action. That means moving all the @Injects of these parameters to the Actions themselves instead of injecting them in "hidden" indirect dependencies. This has the extra benefit of allowing these indirect dependencies to work for JSON Actions as well, since the "regular" way we @Inject parameters can corrupt the POST JSON data. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=232540758 --- java/google/registry/reporting/ReportingModule.java | 10 ++++++---- .../spec11/GenerateSpec11ReportAction.java | 4 +++- .../reporting/spec11/PublishSpec11ReportAction.java | 6 ++++-- .../registry/reporting/spec11/Spec11EmailUtils.java | 13 +++++++------ .../registry/reporting/ReportingModuleTest.java | 8 ++++---- .../spec11/PublishSpec11ReportActionTest.java | 2 ++ .../reporting/spec11/Spec11EmailUtilsTest.java | 7 ++++--- 7 files changed, 30 insertions(+), 20 deletions(-) diff --git a/java/google/registry/reporting/ReportingModule.java b/java/google/registry/reporting/ReportingModule.java index 47890971b..2ffe40bd9 100644 --- a/java/google/registry/reporting/ReportingModule.java +++ b/java/google/registry/reporting/ReportingModule.java @@ -83,7 +83,8 @@ public class ReportingModule { */ @Provides static YearMonth provideYearMonth( - @Parameter(PARAM_YEAR_MONTH) Optional yearMonthOptional, LocalDate date) { + @Parameter(PARAM_YEAR_MONTH) Optional yearMonthOptional, + @Parameter(PARAM_DATE) LocalDate date) { return yearMonthOptional.orElseGet(() -> new YearMonth(date.minusMonths(1))); } @@ -108,9 +109,10 @@ public class ReportingModule { * current date. */ @Provides - static LocalDate provideDate( - @Parameter(PARAM_DATE) Optional dateOptional, Clock clock) { - return dateOptional.orElseGet(() -> new LocalDate(clock.nowUtc(), DateTimeZone.UTC)); + @Parameter(PARAM_DATE) + static LocalDate provideDate(HttpServletRequest req, Clock clock) { + return provideDateOptional(req) + .orElseGet(() -> new LocalDate(clock.nowUtc(), DateTimeZone.UTC)); } /** Constructs a {@link Dataflow} API client with default settings. */ diff --git a/java/google/registry/reporting/spec11/GenerateSpec11ReportAction.java b/java/google/registry/reporting/spec11/GenerateSpec11ReportAction.java index 798582c57..7eb250a0c 100644 --- a/java/google/registry/reporting/spec11/GenerateSpec11ReportAction.java +++ b/java/google/registry/reporting/spec11/GenerateSpec11ReportAction.java @@ -14,6 +14,7 @@ package google.registry.reporting.spec11; +import static google.registry.reporting.ReportingModule.PARAM_DATE; import static google.registry.reporting.ReportingUtils.enqueueBeamReportingTask; import static google.registry.request.Action.Method.POST; import static javax.servlet.http.HttpServletResponse.SC_INTERNAL_SERVER_ERROR; @@ -30,6 +31,7 @@ import google.registry.config.RegistryConfig.Config; import google.registry.keyring.api.KeyModule.Key; import google.registry.reporting.ReportingModule; import google.registry.request.Action; +import google.registry.request.Parameter; import google.registry.request.Response; import google.registry.request.auth.Auth; import java.io.IOException; @@ -70,7 +72,7 @@ public class GenerateSpec11ReportAction implements Runnable { @Config("spec11TemplateUrl") String spec11TemplateUrl, @Config("defaultJobZone") String jobZone, @Key("safeBrowsingAPIKey") String apiKey, - LocalDate date, + @Parameter(PARAM_DATE) LocalDate date, Response response, Dataflow dataflow) { this.projectId = projectId; diff --git a/java/google/registry/reporting/spec11/PublishSpec11ReportAction.java b/java/google/registry/reporting/spec11/PublishSpec11ReportAction.java index a8d9df6ed..be1a8f11e 100644 --- a/java/google/registry/reporting/spec11/PublishSpec11ReportAction.java +++ b/java/google/registry/reporting/spec11/PublishSpec11ReportAction.java @@ -15,6 +15,7 @@ package google.registry.reporting.spec11; import static com.google.common.collect.ImmutableSet.toImmutableSet; +import static google.registry.reporting.ReportingModule.PARAM_DATE; import static google.registry.request.Action.Method.POST; import static javax.servlet.http.HttpServletResponse.SC_INTERNAL_SERVER_ERROR; import static javax.servlet.http.HttpServletResponse.SC_NOT_MODIFIED; @@ -83,7 +84,7 @@ public class PublishSpec11ReportAction implements Runnable { Spec11RegistrarThreatMatchesParser spec11RegistrarThreatMatchesParser, Dataflow dataflow, Response response, - LocalDate date) { + @Parameter(PARAM_DATE) LocalDate date) { this.projectId = projectId; this.registryName = registryName; this.jobId = jobId; @@ -148,7 +149,7 @@ public class PublishSpec11ReportAction implements Runnable { spec11RegistrarThreatMatchesParser.getRegistrarThreatMatches(date); String subject = String.format("%s Monthly Threat Detector [%s]", registryName, date); emailUtils.emailSpec11Reports( - Spec11EmailSoyInfo.MONTHLY_SPEC_11_EMAIL, subject, monthlyMatchesSet); + date, Spec11EmailSoyInfo.MONTHLY_SPEC_11_EMAIL, subject, monthlyMatchesSet); } private void processDailyDiff(LocalDate previousDate) throws IOException, JSONException { @@ -158,6 +159,7 @@ public class PublishSpec11ReportAction implements Runnable { spec11RegistrarThreatMatchesParser.getRegistrarThreatMatches(date); String dailySubject = String.format("%s Daily Threat Detector [%s]", registryName, date); emailUtils.emailSpec11Reports( + date, Spec11EmailSoyInfo.DAILY_SPEC_11_EMAIL, dailySubject, getNewMatches(previousMatches, currentMatches)); diff --git a/java/google/registry/reporting/spec11/Spec11EmailUtils.java b/java/google/registry/reporting/spec11/Spec11EmailUtils.java index 4cfddcc92..82c91fb12 100644 --- a/java/google/registry/reporting/spec11/Spec11EmailUtils.java +++ b/java/google/registry/reporting/spec11/Spec11EmailUtils.java @@ -52,7 +52,6 @@ public class Spec11EmailUtils { .compileToTofu(); private final SendEmailService emailService; - private final LocalDate date; private final String outgoingEmailAddress; private final String alertRecipientAddress; private final String spec11ReplyToAddress; @@ -63,7 +62,6 @@ public class Spec11EmailUtils { @Inject Spec11EmailUtils( SendEmailService emailService, - LocalDate date, @Config("gSuiteOutgoingEmailAddress") String outgoingEmailAddress, @Config("alertRecipientEmailAddress") String alertRecipientAddress, @Config("spec11ReplyToEmailAddress") String spec11ReplyToAddress, @@ -71,7 +69,6 @@ public class Spec11EmailUtils { @Config("registryName") String registryName, Retrier retrier) { this.emailService = emailService; - this.date = date; this.outgoingEmailAddress = outgoingEmailAddress; this.alertRecipientAddress = alertRecipientAddress; this.spec11ReplyToAddress = spec11ReplyToAddress; @@ -85,6 +82,7 @@ public class Spec11EmailUtils { * appropriate address. */ void emailSpec11Reports( + LocalDate date, SoyTemplateInfo soyTemplateInfo, String subject, Set registrarThreatMatchesSet) { @@ -92,7 +90,7 @@ public class Spec11EmailUtils { retrier.callWithRetry( () -> { for (RegistrarThreatMatches registrarThreatMatches : registrarThreatMatchesSet) { - emailRegistrar(soyTemplateInfo, subject, registrarThreatMatches); + emailRegistrar(date, soyTemplateInfo, subject, registrarThreatMatches); } }, IOException.class, @@ -110,13 +108,14 @@ public class Spec11EmailUtils { } private void emailRegistrar( + LocalDate date, SoyTemplateInfo soyTemplateInfo, String subject, RegistrarThreatMatches registrarThreatMatches) throws MessagingException { Message msg = emailService.createMessage(); msg.setSubject(subject); - String content = getContent(soyTemplateInfo, registrarThreatMatches); + String content = getContent(date, soyTemplateInfo, registrarThreatMatches); msg.setContent(content, "text/html"); msg.setHeader("Content-Type", "text/html"); msg.setFrom(new InternetAddress(outgoingEmailAddress)); @@ -127,7 +126,9 @@ public class Spec11EmailUtils { } private String getContent( - SoyTemplateInfo soyTemplateInfo, RegistrarThreatMatches registrarThreatMatches) { + LocalDate date, + SoyTemplateInfo soyTemplateInfo, + RegistrarThreatMatches registrarThreatMatches) { Renderer renderer = SOY_SAUCE.newRenderer(soyTemplateInfo); // Soy templates require that data be in raw map/list form. List> threatMatchMap = diff --git a/javatests/google/registry/reporting/ReportingModuleTest.java b/javatests/google/registry/reporting/ReportingModuleTest.java index 922d54a45..39acfca07 100644 --- a/javatests/google/registry/reporting/ReportingModuleTest.java +++ b/javatests/google/registry/reporting/ReportingModuleTest.java @@ -108,13 +108,13 @@ public class ReportingModuleTest { @Test public void testEmptyDate_returnsToday() { - assertThat(ReportingModule.provideDate(Optional.empty(), clock)) - .isEqualTo(new LocalDate(2017, 7, 1)); + when(req.getParameter("date")).thenReturn(null); + assertThat(ReportingModule.provideDate(req, clock)).isEqualTo(new LocalDate(2017, 7, 1)); } @Test public void testGivenDate_returnsThatDate() { - assertThat(ReportingModule.provideDate(Optional.of(new LocalDate(2017, 7, 2)), clock)) - .isEqualTo(new LocalDate(2017, 7, 2)); + when(req.getParameter("date")).thenReturn("2017-07-02"); + assertThat(ReportingModule.provideDate(req, clock)).isEqualTo(new LocalDate(2017, 7, 2)); } } diff --git a/javatests/google/registry/reporting/spec11/PublishSpec11ReportActionTest.java b/javatests/google/registry/reporting/spec11/PublishSpec11ReportActionTest.java index 65b1d7ed4..282460a7b 100644 --- a/javatests/google/registry/reporting/spec11/PublishSpec11ReportActionTest.java +++ b/javatests/google/registry/reporting/spec11/PublishSpec11ReportActionTest.java @@ -105,6 +105,7 @@ public class PublishSpec11ReportActionTest { assertThat(response.getStatus()).isEqualTo(SC_OK); verify(emailUtils) .emailSpec11Reports( + secondOfMonth, Spec11EmailSoyInfo.MONTHLY_SPEC_11_EMAIL, "Super Cool Registry Monthly Threat Detector [2018-06-02]", sampleThreatMatches()); @@ -154,6 +155,7 @@ public class PublishSpec11ReportActionTest { assertThat(response.getStatus()).isEqualTo(SC_OK); verify(emailUtils) .emailSpec11Reports( + date, Spec11EmailSoyInfo.DAILY_SPEC_11_EMAIL, "Super Cool Registry Daily Threat Detector [2018-06-05]", sampleThreatMatches()); diff --git a/javatests/google/registry/reporting/spec11/Spec11EmailUtilsTest.java b/javatests/google/registry/reporting/spec11/Spec11EmailUtilsTest.java index caa86b75b..55800fd05 100644 --- a/javatests/google/registry/reporting/spec11/Spec11EmailUtilsTest.java +++ b/javatests/google/registry/reporting/spec11/Spec11EmailUtilsTest.java @@ -61,6 +61,7 @@ public class Spec11EmailUtilsTest { private Spec11EmailUtils emailUtils; private Spec11RegistrarThreatMatchesParser parser; private ArgumentCaptor gotMessage; + private final LocalDate date = new LocalDate(2018, 7, 15); @Before public void setUp() throws Exception { @@ -69,7 +70,6 @@ public class Spec11EmailUtilsTest { .thenAnswer((args) -> new MimeMessage(Session.getInstance(new Properties(), null))); parser = mock(Spec11RegistrarThreatMatchesParser.class); - LocalDate date = new LocalDate(2018, 7, 15); when(parser.getRegistrarThreatMatches(date)).thenReturn(sampleThreatMatches()); gotMessage = ArgumentCaptor.forClass(Message.class); @@ -77,7 +77,6 @@ public class Spec11EmailUtilsTest { emailUtils = new Spec11EmailUtils( emailService, - date, "my-sender@test.com", "my-receiver@test.com", "my-reply-to@test.com", @@ -89,6 +88,7 @@ public class Spec11EmailUtilsTest { @Test public void testSuccess_emailMonthlySpec11Reports() throws Exception { emailUtils.emailSpec11Reports( + date, Spec11EmailSoyInfo.MONTHLY_SPEC_11_EMAIL, "Super Cool Registry Monthly Threat Detector [2018-07-15]", sampleThreatMatches()); @@ -143,6 +143,7 @@ public class Spec11EmailUtilsTest { @Test public void testSuccess_emailDailySpec11Reports() throws Exception { emailUtils.emailSpec11Reports( + date, Spec11EmailSoyInfo.DAILY_SPEC_11_EMAIL, "Super Cool Registry Daily Threat Detector [2018-07-15]", sampleThreatMatches()); @@ -224,7 +225,7 @@ public class Spec11EmailUtilsTest { RuntimeException.class, () -> emailUtils.emailSpec11Reports( - Spec11EmailSoyInfo.MONTHLY_SPEC_11_EMAIL, "bar", sampleThreatMatches())); + date, Spec11EmailSoyInfo.MONTHLY_SPEC_11_EMAIL, "bar", sampleThreatMatches())); assertThat(thrown).hasMessageThat().isEqualTo("Emailing spec11 report failed"); assertThat(thrown) .hasCauseThat()