Only inject @Parameter-created variables in the Action itself

Icann reports have 3 parameter-provided injections:

- yearMonth
- subdir
- reportType

We move all of them away from the "inner classes" and only @Inject them in the Actions themselves.

This has 2 benefits:
- it's much clearer what all the parameter inputs of the Actions are
- the "inner injected classes" don't assume anything about the Action that uses them - they will work just as well for JSON actions as for "regular" actions.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=233625765

javatests/google/registry/reporting/icann/ActivityReportingQueryBuilderTest.java

@@ -27,21 +27,21 @@ import org.junit.runners.JUnit4;
 @RunWith(JUnit4.class)
 public class ActivityReportingQueryBuilderTest {
 
+  private final YearMonth yearMonth = new YearMonth(2017, 9);
+
   private ActivityReportingQueryBuilder getQueryBuilder() {
     ActivityReportingQueryBuilder queryBuilder = new ActivityReportingQueryBuilder();
-    queryBuilder.yearMonth = new YearMonth(2017, 9);
     queryBuilder.projectId = "domain-registry-alpha";
     queryBuilder.dnsCountQueryCoordinator =
         new BasicDnsCountQueryCoordinator(
-            new BasicDnsCountQueryCoordinator.Params(null, queryBuilder.yearMonth,
-                                                     queryBuilder.projectId));
+            new BasicDnsCountQueryCoordinator.Params(null, queryBuilder.projectId));
     return queryBuilder;
   }
 
   @Test
   public void testAggregateQueryMatch() {
     ActivityReportingQueryBuilder queryBuilder = getQueryBuilder();
-    assertThat(queryBuilder.getReportQuery())
+    assertThat(queryBuilder.getReportQuery(yearMonth))
         .isEqualTo(
             "#standardSQL\nSELECT * FROM "
                 + "`domain-registry-alpha.icann_reporting.activity_report_aggregation_201709`");
@@ -59,7 +59,7 @@ public class ActivityReportingQueryBuilderTest {
             ActivityReportingQueryBuilder.ACTIVITY_REPORT_AGGREGATION);
 
     ActivityReportingQueryBuilder queryBuilder = getQueryBuilder();
-    ImmutableMap<String, String> actualQueries = queryBuilder.getViewQueryMap();
+    ImmutableMap<String, String> actualQueries = queryBuilder.getViewQueryMap(yearMonth);
     for (String queryName : expectedQueryNames) {
       String actualTableName = String.format("%s_201709", queryName);
       String testFilename = String.format("%s_test.sql", queryName);

javatests/google/registry/reporting/icann/BUILD

@@ -26,6 +26,7 @@ java_library(
         "@com_google_http_client",
         "@com_google_truth",
         "@com_google_truth_extensions_truth_java8_extension",
+        "@javax_servlet_api",
         "@joda_time",
         "@junit",
         "@org_mockito_all",

javatests/google/registry/reporting/icann/IcannReportingModuleTest.java

@@ -15,12 +15,10 @@
 package google.registry.reporting.icann;
 
 import static com.google.common.truth.Truth.assertThat;
-import static google.registry.testing.JUnitBackports.assertThrows;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
 
-import google.registry.reporting.icann.IcannReportingModule.ReportType;
-import google.registry.request.HttpException.BadRequestException;
-import java.util.Optional;
-import org.joda.time.YearMonth;
+import javax.servlet.http.HttpServletRequest;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
@@ -30,39 +28,30 @@ import org.junit.runners.JUnit4;
 public class IcannReportingModuleTest {
 
   @Test
-  public void testEmptySubDir_returnsDefaultSubdir() {
-    assertThat(IcannReportingModule.provideSubdir(Optional.empty(), new YearMonth(2017, 6)))
-        .isEqualTo("icann/monthly/2017-06");
-  }
+  public void testProvideReportTypes() {
+    HttpServletRequest req = mock(HttpServletRequest.class);
 
-  @Test
-  public void testGivenSubdir_returnsManualSubdir() {
-    assertThat(
-            IcannReportingModule.provideSubdir(Optional.of("manual/dir"), new YearMonth(2017, 6)))
-        .isEqualTo("manual/dir");
-  }
+    when(req.getParameter("reportTypes")).thenReturn(null);
+    assertThat(IcannReportingModule.provideReportTypes(req))
+        .containsExactly(
+            IcannReportingModule.ReportType.ACTIVITY, IcannReportingModule.ReportType.TRANSACTIONS);
 
-  @Test
-  public void testInvalidSubdir_throwsException() {
-    BadRequestException thrown =
-        assertThrows(
-            BadRequestException.class,
-            () ->
-                IcannReportingModule.provideSubdir(Optional.of("/whoops"), new YearMonth(2017, 6)));
-    assertThat(thrown)
-        .hasMessageThat()
-        .contains("subdir must not start or end with a \"/\", got /whoops instead.");
-  }
+    when(req.getParameter("reportTypes")).thenReturn("");
+    assertThat(IcannReportingModule.provideReportTypes(req))
+        .containsExactly(
+            IcannReportingModule.ReportType.ACTIVITY, IcannReportingModule.ReportType.TRANSACTIONS);
 
-  @Test
-  public void testGivenReportType_returnsReportType() {
-    assertThat(IcannReportingModule.provideReportTypes(Optional.of(ReportType.ACTIVITY)))
-        .containsExactly(ReportType.ACTIVITY);
-  }
+    when(req.getParameter("reportTypes")).thenReturn("activity");
+    assertThat(IcannReportingModule.provideReportTypes(req))
+        .containsExactly(IcannReportingModule.ReportType.ACTIVITY);
 
-  @Test
-  public void testNoReportType_returnsBothReportTypes() {
-    assertThat(IcannReportingModule.provideReportTypes(Optional.empty()))
-        .containsExactly(ReportType.ACTIVITY, ReportType.TRANSACTIONS);
+    when(req.getParameter("reportTypes")).thenReturn("transactions");
+    assertThat(IcannReportingModule.provideReportTypes(req))
+        .containsExactly(IcannReportingModule.ReportType.TRANSACTIONS);
+
+    when(req.getParameter("reportTypes")).thenReturn("activity,transactions");
+    assertThat(IcannReportingModule.provideReportTypes(req))
+        .containsExactly(
+            IcannReportingModule.ReportType.ACTIVITY, IcannReportingModule.ReportType.TRANSACTIONS);
   }
 }

javatests/google/registry/reporting/icann/IcannReportingStagerTest.java

@@ -50,6 +50,8 @@ public class IcannReportingStagerTest {
   BigqueryConnection bigquery = mock(BigqueryConnection.class);
   FakeResponse response = new FakeResponse();
   GcsService gcsService = GcsServiceFactory.createGcsService();
+  YearMonth yearMonth = new YearMonth(2017, 6);
+  String subdir = "icann/monthly/2017-06";
 
   @Rule
   public final AppEngineRule appEngine = AppEngineRule.builder()
@@ -61,16 +63,12 @@ public class IcannReportingStagerTest {
     IcannReportingStager action = new IcannReportingStager();
     ActivityReportingQueryBuilder activityBuilder = new ActivityReportingQueryBuilder();
     activityBuilder.projectId = "test-project";
-    activityBuilder.yearMonth = new YearMonth(2017, 6);
     activityBuilder.dnsCountQueryCoordinator = new BasicDnsCountQueryCoordinator(null);
     action.activityQueryBuilder = activityBuilder;
     TransactionsReportingQueryBuilder transactionsBuilder = new TransactionsReportingQueryBuilder();
     transactionsBuilder.projectId = "test-project";
-    transactionsBuilder.yearMonth = new YearMonth(2017, 6);
     action.transactionsQueryBuilder = transactionsBuilder;
     action.reportingBucket = "test-bucket";
-    action.yearMonth = new YearMonth(2017, 6);
-    action.subdir = "icann/monthly/2017-06";
     action.bigquery = bigquery;
     action.gcsUtils = new GcsUtils(gcsService, 1024);
     return action;
@@ -100,7 +98,7 @@ public class IcannReportingStagerTest {
             .build();
     when(bigquery.queryToLocalTableSync(any(String.class))).thenReturn(activityReportTable);
     IcannReportingStager stager = createStager();
-    stager.stageReports(ReportType.ACTIVITY);
+    stager.stageReports(yearMonth, subdir, ReportType.ACTIVITY);
 
     String expectedReport1 = "fooField,barField\r\n12,34";
     String expectedReport2 = "fooField,barField\r\n56,78";
@@ -143,7 +141,7 @@ public class IcannReportingStagerTest {
             .build();
     when(bigquery.queryToLocalTableSync(any(String.class))).thenReturn(transactionReportTable);
     IcannReportingStager stager = createStager();
-    stager.stageReports(ReportType.TRANSACTIONS);
+    stager.stageReports(yearMonth, subdir, ReportType.TRANSACTIONS);
 
     String expectedReport1 =
         "registrar,iana,field\r\n\"reg1\",123,10\r\n\"reg2\",456,20\r\nTotals,,30";
@@ -165,7 +163,7 @@ public class IcannReportingStagerTest {
     IcannReportingStager stager = createStager();
     ImmutableList<String> filenames =
         ImmutableList.of("fooTld-transactions-201706.csv", "barTld-activity-201706.csv");
-    stager.createAndUploadManifest(filenames);
+    stager.createAndUploadManifest(subdir, filenames);
 
     String expectedManifest = "fooTld-transactions-201706.csv\nbarTld-activity-201706.csv\n";
     byte[] generatedManifest =

javatests/google/registry/reporting/icann/IcannReportingStagingActionTest.java

@@ -24,14 +24,17 @@ import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
 
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
 import google.registry.bigquery.BigqueryJobFailureException;
 import google.registry.reporting.icann.IcannReportingModule.ReportType;
+import google.registry.request.HttpException.BadRequestException;
 import google.registry.testing.AppEngineRule;
 import google.registry.testing.FakeClock;
 import google.registry.testing.FakeResponse;
 import google.registry.testing.FakeSleeper;
 import google.registry.testing.TaskQueueHelper.TaskMatcher;
 import google.registry.util.Retrier;
+import java.util.Optional;
 import org.joda.time.YearMonth;
 import org.junit.Before;
 import org.junit.Rule;
@@ -46,6 +49,9 @@ public class IcannReportingStagingActionTest {
   FakeResponse response = new FakeResponse();
   IcannReportingStager stager = mock(IcannReportingStager.class);
   ReportingEmailUtils emailUtils = mock(ReportingEmailUtils.class);
+  YearMonth yearMonth = new YearMonth(2017, 6);
+  String subdir = "default/dir";
+  IcannReportingStagingAction action;
 
   @Rule
   public final AppEngineRule appEngine = AppEngineRule.builder()
@@ -56,37 +62,36 @@ public class IcannReportingStagingActionTest {
 
   @Before
   public void setUp() throws Exception {
-    when(stager.stageReports(ReportType.ACTIVITY)).thenReturn(ImmutableList.of("a", "b"));
-    when(stager.stageReports(ReportType.TRANSACTIONS)).thenReturn(ImmutableList.of("c", "d"));
-  }
-
-  private static void assertUploadTaskEnqueued(String subDir) {
-    TaskMatcher matcher =
-        new TaskMatcher()
-            .url("/_dr/task/icannReportingUpload")
-            .method("POST")
-            .param("subdir", subDir);
-    assertTasksEnqueued("retryable-cron-tasks", matcher);
-  }
-
-  private IcannReportingStagingAction createAction(ImmutableList<ReportType> reportingMode) {
-    IcannReportingStagingAction action = new IcannReportingStagingAction();
-    action.yearMonth = new YearMonth(2017, 6);
-    action.subdir = "default/dir";
-    action.reportTypes = reportingMode;
+    action = new IcannReportingStagingAction();
+    action.yearMonth = yearMonth;
+    action.overrideSubdir = Optional.of(subdir);
+    action.reportTypes = ImmutableSet.of(ReportType.ACTIVITY, ReportType.TRANSACTIONS);
     action.response = response;
     action.stager = stager;
     action.retrier = new Retrier(new FakeSleeper(new FakeClock()), 3);
     action.emailUtils = emailUtils;
-    return action;
+
+    when(stager.stageReports(yearMonth, subdir, ReportType.ACTIVITY))
+        .thenReturn(ImmutableList.of("a", "b"));
+    when(stager.stageReports(yearMonth, subdir, ReportType.TRANSACTIONS))
+        .thenReturn(ImmutableList.of("c", "d"));
+  }
+
+  private static void assertUploadTaskEnqueued(String subdir) {
+    TaskMatcher matcher =
+        new TaskMatcher()
+            .url("/_dr/task/icannReportingUpload")
+            .method("POST")
+            .param("subdir", subdir);
+    assertTasksEnqueued("retryable-cron-tasks", matcher);
   }
 
   @Test
   public void testActivityReportingMode_onlyStagesActivityReports() throws Exception {
-    IcannReportingStagingAction action = createAction(ImmutableList.of(ReportType.ACTIVITY));
+    action.reportTypes = ImmutableSet.of(ReportType.ACTIVITY);
     action.run();
-    verify(stager).stageReports(ReportType.ACTIVITY);
-    verify(stager).createAndUploadManifest(ImmutableList.of("a", "b"));
+    verify(stager).stageReports(yearMonth, subdir, ReportType.ACTIVITY);
+    verify(stager).createAndUploadManifest(subdir, ImmutableList.of("a", "b"));
     verify(emailUtils)
         .emailResults(
             "ICANN Monthly report staging summary [SUCCESS]",
@@ -96,12 +101,10 @@ public class IcannReportingStagingActionTest {
 
   @Test
   public void testAbsentReportingMode_stagesBothReports() throws Exception {
-    IcannReportingStagingAction action =
-        createAction(ImmutableList.of(ReportType.ACTIVITY, ReportType.TRANSACTIONS));
     action.run();
-    verify(stager).stageReports(ReportType.ACTIVITY);
-    verify(stager).stageReports(ReportType.TRANSACTIONS);
-    verify(stager).createAndUploadManifest(ImmutableList.of("a", "b", "c", "d"));
+    verify(stager).stageReports(yearMonth, subdir, ReportType.ACTIVITY);
+    verify(stager).stageReports(yearMonth, subdir, ReportType.TRANSACTIONS);
+    verify(stager).createAndUploadManifest(subdir, ImmutableList.of("a", "b", "c", "d"));
     verify(emailUtils)
         .emailResults(
             "ICANN Monthly report staging summary [SUCCESS]",
@@ -111,15 +114,13 @@ public class IcannReportingStagingActionTest {
 
   @Test
   public void testRetryOnBigqueryException() throws Exception {
-    IcannReportingStagingAction action =
-        createAction(ImmutableList.of(ReportType.ACTIVITY, ReportType.TRANSACTIONS));
-    when(stager.stageReports(ReportType.TRANSACTIONS))
+    when(stager.stageReports(yearMonth, subdir, ReportType.TRANSACTIONS))
         .thenThrow(new BigqueryJobFailureException("Expected failure", null, null, null))
         .thenReturn(ImmutableList.of("c", "d"));
     action.run();
-    verify(stager, times(2)).stageReports(ReportType.ACTIVITY);
-    verify(stager, times(2)).stageReports(ReportType.TRANSACTIONS);
-    verify(stager).createAndUploadManifest(ImmutableList.of("a", "b", "c", "d"));
+    verify(stager, times(2)).stageReports(yearMonth, subdir, ReportType.ACTIVITY);
+    verify(stager, times(2)).stageReports(yearMonth, subdir, ReportType.TRANSACTIONS);
+    verify(stager).createAndUploadManifest(subdir, ImmutableList.of("a", "b", "c", "d"));
     verify(emailUtils)
         .emailResults(
             "ICANN Monthly report staging summary [SUCCESS]",
@@ -129,15 +130,14 @@ public class IcannReportingStagingActionTest {
 
   @Test
   public void testEmailEng_onMoreThanRetriableFailure() throws Exception {
-    IcannReportingStagingAction action =
-        createAction(ImmutableList.of(ReportType.ACTIVITY));
-    when(stager.stageReports(ReportType.ACTIVITY))
+    action.reportTypes = ImmutableSet.of(ReportType.ACTIVITY);
+    when(stager.stageReports(yearMonth, subdir, ReportType.ACTIVITY))
         .thenThrow(new BigqueryJobFailureException("Expected failure", null, null, null));
     RuntimeException thrown = assertThrows(RuntimeException.class, action::run);
     assertThat(thrown).hasCauseThat().isInstanceOf(BigqueryJobFailureException.class);
     assertThat(thrown).hasMessageThat().isEqualTo("Staging action failed.");
     assertThat(thrown).hasCauseThat().hasMessageThat().isEqualTo("Expected failure");
-    verify(stager, times(3)).stageReports(ReportType.ACTIVITY);
+    verify(stager, times(3)).stageReports(yearMonth, subdir, ReportType.ACTIVITY);
     verify(emailUtils)
         .emailResults(
             "ICANN Monthly report staging summary [FAILURE]",
@@ -146,4 +146,29 @@ public class IcannReportingStagingActionTest {
     // Assert no upload task enqueued
     assertNoTasksEnqueued("retryable-cron-tasks");
   }
+
+  @Test
+  public void testEmptySubDir_returnsDefaultSubdir() {
+    action.overrideSubdir = Optional.empty();
+    assertThat(action.getSubdir(new YearMonth(2017, 6))).isEqualTo("icann/monthly/2017-06");
+  }
+
+  @Test
+  public void testGivenSubdir_returnsManualSubdir() {
+    action.overrideSubdir = Optional.of("manual/dir");
+    assertThat(action.getSubdir(new YearMonth(2017, 6))).isEqualTo("manual/dir");
+  }
+
+  @Test
+  public void testInvalidSubdir_throwsException() {
+    action.overrideSubdir = Optional.of("/whoops");
+    BadRequestException thrown =
+        assertThrows(
+            BadRequestException.class,
+            () ->
+                action.getSubdir(new YearMonth(2017, 6)));
+    assertThat(thrown)
+        .hasMessageThat()
+        .contains("subdir must not start or end with a \"/\", got /whoops instead.");
+  }
 }

javatests/google/registry/reporting/icann/TransactionsReportingQueryBuilderTest.java

@@ -27,9 +27,10 @@ import org.junit.runners.JUnit4;
 @RunWith(JUnit4.class)
 public class TransactionsReportingQueryBuilderTest {
 
+  private final YearMonth yearMonth = new YearMonth(2017, 9);
+
   private TransactionsReportingQueryBuilder getQueryBuilder() {
     TransactionsReportingQueryBuilder queryBuilder = new TransactionsReportingQueryBuilder();
-    queryBuilder.yearMonth = new YearMonth(2017, 9);
     queryBuilder.projectId = "domain-registry-alpha";
     return queryBuilder;
   }
@@ -37,7 +38,7 @@ public class TransactionsReportingQueryBuilderTest {
   @Test
   public void testAggregateQueryMatch() {
     TransactionsReportingQueryBuilder queryBuilder = getQueryBuilder();
-    assertThat(queryBuilder.getReportQuery())
+    assertThat(queryBuilder.getReportQuery(yearMonth))
         .isEqualTo(
             "#standardSQL\nSELECT * FROM "
                 + "`domain-registry-alpha.icann_reporting.transactions_report_aggregation_201709`");
@@ -56,7 +57,7 @@ public class TransactionsReportingQueryBuilderTest {
             TransactionsReportingQueryBuilder.ATTEMPTED_ADDS);
 
     TransactionsReportingQueryBuilder queryBuilder = getQueryBuilder();
-    ImmutableMap<String, String> actualQueries = queryBuilder.getViewQueryMap();
+    ImmutableMap<String, String> actualQueries = queryBuilder.getViewQueryMap(yearMonth);
     for (String queryName : expectedQueryNames) {
       String actualTableName = String.format("%s_201709", queryName);
       String testFilename = String.format("%s_test.sql", queryName);

javatests/google/registry/request/RequestParametersTest.java

@@ -23,6 +23,7 @@ import static google.registry.request.RequestParameters.extractOptionalEnumParam
 import static google.registry.request.RequestParameters.extractOptionalParameter;
 import static google.registry.request.RequestParameters.extractRequiredDatetimeParameter;
 import static google.registry.request.RequestParameters.extractRequiredParameter;
+import static google.registry.request.RequestParameters.extractSetOfEnumParameters;
 import static google.registry.request.RequestParameters.extractSetOfParameters;
 import static google.registry.testing.JUnitBackports.assertThrows;
 import static org.mockito.Mockito.mock;
@@ -86,25 +87,25 @@ public class RequestParametersTest {
 
   @Test
   public void testExtractSetOfParameters_empty_returnsEmpty() {
-    when(req.getParameterValues("spin")).thenReturn(new String[]{""});
+    when(req.getParameter("spin")).thenReturn("");
     assertThat(extractSetOfParameters(req, "spin")).isEmpty();
   }
 
   @Test
   public void testExtractSetOfParameters_oneValue_returnsValue() {
-    when(req.getParameterValues("spin")).thenReturn(new String[]{"bog"});
+    when(req.getParameter("spin")).thenReturn("bog");
     assertThat(extractSetOfParameters(req, "spin")).containsExactly("bog");
   }
 
   @Test
   public void testExtractSetOfParameters_multipleValues_returnsAll() {
-    when(req.getParameterValues("spin")).thenReturn(new String[]{"bog,gob"});
+    when(req.getParameter("spin")).thenReturn("bog,gob");
     assertThat(extractSetOfParameters(req, "spin")).containsExactly("bog", "gob");
   }
 
   @Test
   public void testExtractSetOfParameters_multipleValuesWithEmpty_removesEmpty() {
-    when(req.getParameterValues("spin")).thenReturn(new String[]{",bog,,gob,"});
+    when(req.getParameter("spin")).thenReturn(",bog,,gob,");
     assertThat(extractSetOfParameters(req, "spin")).containsExactly("bog", "gob");
   }
 
@@ -116,6 +117,53 @@ public class RequestParametersTest {
     assertThat(thrown).hasMessageThat().contains("spin");
   }
 
+  @Test
+  public void testExtractSetOfEnumParameters_notPresent_returnsEmpty() {
+    assertThat(extractSetOfEnumParameters(req, Club.class, "spin")).isEmpty();
+  }
+
+  @Test
+  public void testExtractSetOfEnumParameters_empty_returnsEmpty() {
+    when(req.getParameter("spin")).thenReturn("");
+    assertThat(extractSetOfEnumParameters(req, Club.class, "spin")).isEmpty();
+  }
+
+  @Test
+  public void testExtractSetOfEnumParameters_oneValue_returnsValue() {
+    when(req.getParameter("spin")).thenReturn("DANCE");
+    assertThat(extractSetOfEnumParameters(req, Club.class, "spin")).containsExactly(Club.DANCE);
+  }
+
+  @Test
+  public void testExtractSetOfEnumParameters_multipleValues_returnsAll() {
+    when(req.getParameter("spin")).thenReturn("DANCE,FLOOR");
+    assertThat(extractSetOfEnumParameters(req, Club.class, "spin"))
+        .containsExactly(Club.DANCE, Club.FLOOR);
+  }
+
+  @Test
+  public void testExtractSetOfEnumParameters_multipleValuesWithEmpty_removesEmpty() {
+    when(req.getParameter("spin")).thenReturn(",DANCE,,FLOOR,");
+    assertThat(extractSetOfEnumParameters(req, Club.class, "spin"))
+        .containsExactly(Club.DANCE, Club.FLOOR);
+  }
+
+  @Test
+  public void testExtractSetOfEnumParameters_multipleValues_caseInsensitive() {
+    when(req.getParameter("spin")).thenReturn("danCE,FlooR");
+    assertThat(extractSetOfEnumParameters(req, Club.class, "spin"))
+        .containsExactly(Club.DANCE, Club.FLOOR);
+  }
+
+  @Test
+  public void testExtractSetOfEnumParameters_multipleParameters_error() {
+    when(req.getParameterValues("spin")).thenReturn(new String[]{"DANCE", "FLOOR"});
+    BadRequestException thrown =
+        assertThrows(
+            BadRequestException.class, () -> extractSetOfEnumParameters(req, Club.class, "spin"));
+    assertThat(thrown).hasMessageThat().contains("spin");
+  }
+
   @Test
   public void testExtractBooleanParameter_notPresent_returnsFalse() {
     assertThat(extractBooleanParameter(req, "love")).isFalse();