Fix reporting module parameters and yearMonth usage

This is a final refactor to address Nick's comments in [] where YearMonth really should be injected as a Joda type instead of a raw string, and the HTTP parameters should be separate from the default-provided dependencies.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=173539965
This commit is contained in:
larryruili 2017-10-26 08:17:08 -07:00 committed by jianglai
parent d22986a0a3
commit 4a9b8b918a
14 changed files with 109 additions and 87 deletions

View file

@ -20,13 +20,13 @@ import static google.registry.reporting.IcannReportingModule.ICANN_REPORTING_DAT
import com.google.common.collect.ImmutableMap;
import com.google.common.io.Resources;
import google.registry.config.RegistryConfig.Config;
import google.registry.request.Parameter;
import google.registry.util.ResourceUtils;
import google.registry.util.SqlTemplate;
import java.io.IOException;
import java.net.URL;
import javax.inject.Inject;
import org.joda.time.LocalDate;
import org.joda.time.YearMonth;
import org.joda.time.format.DateTimeFormat;
import org.joda.time.format.DateTimeFormatter;
@ -44,7 +44,9 @@ public final class ActivityReportingQueryBuilder implements QueryBuilder {
static final String ACTIVITY_REPORT_AGGREGATION = "activity_report_aggregation";
@Inject @Config("projectId") String projectId;
@Inject @Parameter(IcannReportingModule.PARAM_YEAR_MONTH) String yearMonth;
@Inject YearMonth yearMonth;
@Inject ActivityReportingQueryBuilder() {}
/** Returns the aggregate query which generates the activity report from the saved view. */
@ -60,11 +62,9 @@ public final class ActivityReportingQueryBuilder implements QueryBuilder {
/** Sets the month we're doing activity reporting for, and returns the view query map. */
@Override
public ImmutableMap<String, String> getViewQueryMap() throws IOException {
LocalDate reportDate =
DateTimeFormat.forPattern("yyyy-MM").parseLocalDate(yearMonth).withDayOfMonth(1);
LocalDate firstDayOfMonth = reportDate;
LocalDate firstDayOfMonth = yearMonth.toLocalDate(1);
// The pattern-matching is inclusive, so we subtract 1 day to only report that month's data.
LocalDate lastDayOfMonth = reportDate.plusMonths(1).minusDays(1);
LocalDate lastDayOfMonth = yearMonth.toLocalDate(1).plusMonths(1).minusDays(1);
return createQueryMap(firstDayOfMonth, lastDayOfMonth);
}
@ -135,9 +135,9 @@ public final class ActivityReportingQueryBuilder implements QueryBuilder {
}
/** Returns the table name of the query, suffixed with the yearMonth in _YYYYMM format. */
/** Returns the table name of the query, suffixed with the yearMonth in _yyyyMM format. */
private String getTableName(String queryName) {
return String.format("%s_%s", queryName, yearMonth.replace("-", ""));
return String.format("%s_%s", queryName, DateTimeFormat.forPattern("yyyyMM").print(yearMonth));
}
/** Returns {@link String} for file in {@code reporting/sql/} directory. */

View file

@ -16,6 +16,7 @@ package google.registry.reporting;
import static google.registry.request.RequestParameters.extractOptionalEnumParameter;
import static google.registry.request.RequestParameters.extractOptionalParameter;
import static java.lang.annotation.RetentionPolicy.RUNTIME;
import com.google.api.client.googleapis.auth.oauth2.GoogleCredential;
import com.google.api.client.http.HttpTransport;
@ -29,10 +30,15 @@ import google.registry.request.HttpException.BadRequestException;
import google.registry.request.Parameter;
import google.registry.util.Clock;
import google.registry.util.SendEmailService;
import java.lang.annotation.Documented;
import java.lang.annotation.Retention;
import java.util.Optional;
import javax.inject.Qualifier;
import javax.servlet.http.HttpServletRequest;
import org.joda.time.Duration;
import org.joda.time.YearMonth;
import org.joda.time.format.DateTimeFormat;
import org.joda.time.format.DateTimeFormatter;
/** Module for dependencies required by ICANN monthly transactions/activity reporting. */
@Module
@ -44,9 +50,7 @@ public final class IcannReportingModule {
ACTIVITY
}
static final String PARAM_OPTIONAL_YEAR_MONTH = "yearMonthOptional";
static final String PARAM_YEAR_MONTH = "yearMonth";
static final String PARAM_OPTIONAL_SUBDIR = "subdirOptional";
static final String PARAM_SUBDIR = "subdir";
static final String PARAM_REPORT_TYPE = "reportType";
static final String ICANN_REPORTING_DATA_SET = "icann_reporting";
@ -55,42 +59,45 @@ public final class IcannReportingModule {
private static final String DEFAULT_SUBDIR = "icann/monthly";
private static final String BIGQUERY_SCOPE = "https://www.googleapis.com/auth/cloud-platform";
/** Extracts an optional yearMonth in yyyy-MM format from the request. */
/** Extracts an optional YearMonth in yyyy-MM format from the request. */
@Provides
@Parameter(PARAM_OPTIONAL_YEAR_MONTH)
static Optional<String> provideYearMonthOptional(HttpServletRequest req) {
return extractOptionalParameter(req, PARAM_YEAR_MONTH);
@Parameter(PARAM_YEAR_MONTH)
static Optional<YearMonth> provideYearMonthOptional(HttpServletRequest req) {
DateTimeFormatter formatter = DateTimeFormat.forPattern("yyyy-MM");
Optional<String> optionalYearMonthStr = extractOptionalParameter(req, PARAM_YEAR_MONTH);
try {
return optionalYearMonthStr.map(s -> YearMonth.parse(s, formatter));
} catch (IllegalArgumentException e) {
throw new BadRequestException(
String.format(
"yearMonth must be in yyyy-MM format, got %s instead",
optionalYearMonthStr.orElse("UNSPECIFIED YEARMONTH")));
}
}
/** Provides the yearMonth in yyyy-MM format, defaults to one month prior to run time. */
@Provides
@Parameter(PARAM_YEAR_MONTH)
static String provideYearMonth(
@Parameter(PARAM_OPTIONAL_YEAR_MONTH) Optional<String> yearMonthOptional, Clock clock) {
String yearMonth =
yearMonthOptional.orElse(
DateTimeFormat.forPattern("yyyy-MM").print(clock.nowUtc().minusMonths(1)));
if (!yearMonth.matches("[0-9]{4}-[0-9]{2}")) {
throw new BadRequestException(
String.format("yearMonth must be in yyyy-MM format, got %s instead", yearMonth));
}
return yearMonth;
static YearMonth provideYearMonth(
@Parameter(PARAM_YEAR_MONTH) Optional<YearMonth> yearMonthOptional, Clock clock) {
return yearMonthOptional.orElseGet(() -> new YearMonth(clock.nowUtc().minusMonths(1)));
}
/** Provides an optional subdirectory to store/upload reports to, extracted from the request. */
@Provides
@Parameter(PARAM_OPTIONAL_SUBDIR)
@Parameter(PARAM_SUBDIR)
static Optional<String> provideSubdirOptional(HttpServletRequest req) {
return extractOptionalParameter(req, PARAM_SUBDIR);
}
/** Provides the subdirectory to store/upload reports to, defaults to icann/monthly/yearMonth. */
@Provides
@Parameter(PARAM_SUBDIR)
@ReportingSubdir
static String provideSubdir(
@Parameter(PARAM_OPTIONAL_SUBDIR) Optional<String> subdirOptional,
@Parameter(PARAM_YEAR_MONTH) String yearMonth) {
String subdir = subdirOptional.orElse(String.format("%s/%s", DEFAULT_SUBDIR, yearMonth));
@Parameter(PARAM_SUBDIR) Optional<String> subdirOptional, YearMonth yearMonth) {
String subdir =
subdirOptional.orElse(
String.format(
"%s/%s", DEFAULT_SUBDIR, DateTimeFormat.forPattern("yyyy-MM").print(yearMonth)));
if (subdir.startsWith("/") || subdir.endsWith("/")) {
throw new BadRequestException(
String.format("subdir must not start or end with a \"/\", got %s instead.", subdir));
@ -100,14 +107,15 @@ public final class IcannReportingModule {
/** Provides an optional reportType to store/upload reports to, extracted from the request. */
@Provides
@Parameter(PARAM_REPORT_TYPE)
static Optional<ReportType> provideReportTypeOptional(HttpServletRequest req) {
return extractOptionalEnumParameter(req, ReportType.class, PARAM_REPORT_TYPE);
}
/** Provides a list of reportTypes specified. If absent, we default to both report types. */
@Provides
@Parameter(PARAM_REPORT_TYPE)
static ImmutableList<ReportType> provideReportTypes(Optional<ReportType> reportTypeOptional) {
static ImmutableList<ReportType> provideReportTypes(
@Parameter(PARAM_REPORT_TYPE) Optional<ReportType> reportTypeOptional) {
return reportTypeOptional.map(ImmutableList::of)
.orElseGet(() -> ImmutableList.of(ReportType.ACTIVITY, ReportType.TRANSACTIONS));
}
@ -144,5 +152,11 @@ public final class IcannReportingModule {
static SendEmailService provideSendEmailService() {
return new SendEmailService();
}
/** Dagger qualifier for the subdirectory we stage to/upload from. */
@Qualifier
@Documented
@Retention(RUNTIME)
public @interface ReportingSubdir {}
}

View file

@ -34,7 +34,7 @@ import google.registry.bigquery.BigqueryUtils.TableType;
import google.registry.config.RegistryConfig.Config;
import google.registry.gcs.GcsUtils;
import google.registry.reporting.IcannReportingModule.ReportType;
import google.registry.request.Parameter;
import google.registry.reporting.IcannReportingModule.ReportingSubdir;
import google.registry.util.FormattingLogger;
import java.io.IOException;
import java.util.ArrayList;
@ -47,6 +47,8 @@ import java.util.Map.Entry;
import java.util.concurrent.ExecutionException;
import java.util.stream.Collectors;
import javax.inject.Inject;
import org.joda.time.YearMonth;
import org.joda.time.format.DateTimeFormat;
/**
* Class containing methods for staging ICANN monthly reports on GCS.
@ -58,10 +60,9 @@ public class IcannReportingStager {
private static final FormattingLogger logger = FormattingLogger.getLoggerForCallerClass();
@Inject @Config("icannReportingBucket") String reportingBucket;
@Inject @Parameter(IcannReportingModule.PARAM_YEAR_MONTH) String yearMonth;
@Inject
@Parameter(IcannReportingModule.PARAM_SUBDIR)
@Inject YearMonth yearMonth;
@Inject @ReportingSubdir
String subdir;
@Inject ActivityReportingQueryBuilder activityQueryBuilder;
@ -242,7 +243,9 @@ public class IcannReportingStager {
String reportFilename =
String.format(
"%s-%s-%s.csv",
tld, Ascii.toLowerCase(reportType.toString()), yearMonth.replace("-", ""));
tld,
Ascii.toLowerCase(reportType.toString()),
DateTimeFormat.forPattern("yyyyMM").print(yearMonth));
String reportBucketname = String.format("%s/%s", reportingBucket, subdir);
final GcsFilename gcsFilename = new GcsFilename(reportBucketname, reportFilename);
gcsUtils.createFromBytes(gcsFilename, reportBytes);

View file

@ -24,7 +24,6 @@ import com.google.common.net.MediaType;
import google.registry.bigquery.BigqueryJobFailureException;
import google.registry.reporting.IcannReportingModule.ReportType;
import google.registry.request.Action;
import google.registry.request.Parameter;
import google.registry.request.Response;
import google.registry.request.auth.Auth;
import google.registry.util.FormattingLogger;
@ -56,10 +55,7 @@ public final class IcannReportingStagingAction implements Runnable {
private static final FormattingLogger logger = FormattingLogger.getLoggerForCallerClass();
@Inject
@Parameter(IcannReportingModule.PARAM_REPORT_TYPE)
ImmutableList<ReportType> reportTypes;
@Inject ImmutableList<ReportType> reportTypes;
@Inject IcannReportingStager stager;
@Inject Retrier retrier;
@Inject Response response;

View file

@ -27,8 +27,8 @@ import com.google.common.collect.ImmutableMap;
import com.google.common.io.ByteStreams;
import google.registry.config.RegistryConfig.Config;
import google.registry.gcs.GcsUtils;
import google.registry.reporting.IcannReportingModule.ReportingSubdir;
import google.registry.request.Action;
import google.registry.request.Parameter;
import google.registry.request.Response;
import google.registry.request.auth.Auth;
import google.registry.util.FormattingLogger;
@ -62,8 +62,7 @@ public final class IcannReportingUploadAction implements Runnable {
@Config("icannReportingBucket")
String reportingBucket;
@Inject
@Parameter(IcannReportingModule.PARAM_SUBDIR)
@Inject @ReportingSubdir
String subdir;
@Inject GcsUtils gcsUtils;

View file

@ -20,13 +20,14 @@ import static google.registry.reporting.IcannReportingModule.ICANN_REPORTING_DAT
import com.google.common.collect.ImmutableMap;
import com.google.common.io.Resources;
import google.registry.config.RegistryConfig.Config;
import google.registry.request.Parameter;
import google.registry.util.ResourceUtils;
import google.registry.util.SqlTemplate;
import java.io.IOException;
import java.net.URL;
import javax.inject.Inject;
import org.joda.time.DateTime;
import org.joda.time.LocalTime;
import org.joda.time.YearMonth;
import org.joda.time.format.DateTimeFormat;
import org.joda.time.format.DateTimeFormatter;
@ -36,7 +37,9 @@ import org.joda.time.format.DateTimeFormatter;
public final class TransactionsReportingQueryBuilder implements QueryBuilder {
@Inject @Config("projectId") String projectId;
@Inject @Parameter(IcannReportingModule.PARAM_YEAR_MONTH) String yearMonth;
@Inject YearMonth yearMonth;
@Inject TransactionsReportingQueryBuilder() {}
static final String TRANSACTIONS_REPORT_AGGREGATION = "transactions_report_aggregation";
@ -61,15 +64,9 @@ public final class TransactionsReportingQueryBuilder implements QueryBuilder {
@Override
public ImmutableMap<String, String> getViewQueryMap() throws IOException {
// Set the earliest date to to yearMonth on day 1 at 00:00:00
DateTime earliestReportTime =
DateTimeFormat.forPattern("yyyy-MM")
.parseDateTime(yearMonth)
.withDayOfMonth(1)
.withHourOfDay(0)
.withMinuteOfHour(0)
.withSecondOfMinute(0);
// Set the latest date to yearMonth on the last day at 23:59:59
DateTime latestReportTime = earliestReportTime.plusMonths(1).minusSeconds(1);
DateTime earliestReportTime = yearMonth.toLocalDate(1).toDateTime(new LocalTime(0, 0, 0));
// Set the latest date to yearMonth on the last day at 23:59:59.999
DateTime latestReportTime = earliestReportTime.plusMonths(1).minusMillis(1);
return createQueryMap(earliestReportTime, latestReportTime);
}
@ -95,7 +92,7 @@ public final class TransactionsReportingQueryBuilder implements QueryBuilder {
.build();
queriesBuilder.put(getTableName(TOTAL_DOMAINS), totalDomainsQuery);
DateTimeFormatter timestampFormatter = DateTimeFormat.forPattern("yyyy-MM-dd HH:mm:ss");
DateTimeFormatter timestampFormatter = DateTimeFormat.forPattern("yyyy-MM-dd HH:mm:ss.SSS");
String totalNameserversQuery =
SqlTemplate.create(getQueryFromFile("total_nameservers.sql"))
.put("PROJECT_ID", projectId)
@ -173,9 +170,9 @@ public final class TransactionsReportingQueryBuilder implements QueryBuilder {
return queriesBuilder.build();
}
/** Returns the table name of the query, suffixed with the yearMonth in _YYYYMM format. */
/** Returns the table name of the query, suffixed with the yearMonth in _yyyyMM format. */
private String getTableName(String queryName) {
return String.format("%s_%s", queryName, yearMonth.replace("-", ""));
return String.format("%s_%s", queryName, DateTimeFormat.forPattern("yyyyMM").print(yearMonth));
}
/** Returns {@link String} for file in {@code reporting/sql/} directory. */

View file

@ -19,6 +19,7 @@ import static com.google.common.truth.Truth.assertThat;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import java.io.IOException;
import org.joda.time.YearMonth;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;
@ -29,7 +30,7 @@ public class ActivityReportingQueryBuilderTest {
private ActivityReportingQueryBuilder getQueryBuilder() {
ActivityReportingQueryBuilder queryBuilder = new ActivityReportingQueryBuilder();
queryBuilder.yearMonth = "2017-09";
queryBuilder.yearMonth = new YearMonth(2017, 9);
queryBuilder.projectId = "domain-registry-alpha";
return queryBuilder;
}

View file

@ -16,6 +16,7 @@ package google.registry.reporting;
import static com.google.common.truth.Truth.assertThat;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;
import google.registry.reporting.IcannReportingModule.ReportType;
import google.registry.request.HttpException.BadRequestException;
@ -25,6 +26,7 @@ import google.registry.util.Clock;
import java.util.Optional;
import javax.servlet.http.HttpServletRequest;
import org.joda.time.DateTime;
import org.joda.time.YearMonth;
import org.junit.Before;
import org.junit.Rule;
import org.junit.Test;
@ -46,32 +48,41 @@ public class IcannReportingModuleTest {
}
@Test
public void testEmptyYearMonth_returnsCurrentDate() {
assertThat(IcannReportingModule.provideYearMonth(Optional.empty(), clock)).isEqualTo("2017-06");
public void testEmptyYearMonthParameter_returnsEmptyYearMonthOptional() {
when(req.getParameter("yearMonth")).thenReturn("");
assertThat(IcannReportingModule.provideYearMonthOptional(req)).isEqualTo(Optional.empty());
}
@Test
public void testInvalidYearMonthParameter_throwsException() {
when(req.getParameter("yearMonth")).thenReturn("201705");
thrown.expect(
BadRequestException.class, "yearMonth must be in yyyy-MM format, got 201705 instead");
IcannReportingModule.provideYearMonthOptional(req);
}
@Test
public void testEmptyYearMonth_returnsLastMonth() {
assertThat(IcannReportingModule.provideYearMonth(Optional.empty(), clock))
.isEqualTo(new YearMonth(2017, 6));
}
@Test
public void testGivenYearMonth_returnsThatMonth() {
assertThat(IcannReportingModule.provideYearMonth(Optional.of("2017-05"), clock))
.isEqualTo("2017-05");
}
@Test
public void testInvalidYearMonth_throwsException() {
thrown.expect(
BadRequestException.class, "yearMonth must be in yyyy-MM format, got 201705 instead");
IcannReportingModule.provideYearMonth(Optional.of("201705"), clock);
assertThat(IcannReportingModule.provideYearMonth(Optional.of(new YearMonth(2017, 5)), clock))
.isEqualTo(new YearMonth(2017, 5));
}
@Test
public void testEmptySubDir_returnsDefaultSubdir() {
assertThat(IcannReportingModule.provideSubdir(Optional.empty(), "2017-06"))
assertThat(IcannReportingModule.provideSubdir(Optional.empty(), new YearMonth(2017, 6)))
.isEqualTo("icann/monthly/2017-06");
}
@Test
public void testGivenSubdir_returnsManualSubdir() {
assertThat(IcannReportingModule.provideSubdir(Optional.of("manual/dir"), "2017-06"))
assertThat(
IcannReportingModule.provideSubdir(Optional.of("manual/dir"), new YearMonth(2017, 6)))
.isEqualTo("manual/dir");
}
@ -80,7 +91,7 @@ public class IcannReportingModuleTest {
thrown.expect(
BadRequestException.class,
"subdir must not start or end with a \"/\", got /whoops instead.");
IcannReportingModule.provideSubdir(Optional.of("/whoops"), "2017-06");
IcannReportingModule.provideSubdir(Optional.of("/whoops"), new YearMonth(2017, 6));
}
@Test

View file

@ -39,6 +39,7 @@ import java.util.concurrent.ExecutionException;
import java.util.concurrent.Executor;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.TimeoutException;
import org.joda.time.YearMonth;
import org.junit.Rule;
import org.junit.Test;
import org.junit.runner.RunWith;
@ -62,14 +63,14 @@ public class IcannReportingStagerTest {
IcannReportingStager action = new IcannReportingStager();
ActivityReportingQueryBuilder activityBuilder = new ActivityReportingQueryBuilder();
activityBuilder.projectId = "test-project";
activityBuilder.yearMonth = "2017-06";
activityBuilder.yearMonth = new YearMonth(2017, 6);
action.activityQueryBuilder = activityBuilder;
TransactionsReportingQueryBuilder transactionsBuilder = new TransactionsReportingQueryBuilder();
transactionsBuilder.projectId = "test-project";
transactionsBuilder.yearMonth = "2017-06";
transactionsBuilder.yearMonth = new YearMonth(2017, 6);
action.transactionsQueryBuilder = transactionsBuilder;
action.reportingBucket = "test-bucket";
action.yearMonth = "2017-06";
action.yearMonth = new YearMonth(2017, 6);
action.subdir = "icann/monthly/2017-06";
action.bigquery = bigquery;
action.gcsUtils = new GcsUtils(gcsService, 1024);

View file

@ -131,4 +131,3 @@ public class IcannReportingStagingActionTest {
+ " check logs for more details.");
}
}

View file

@ -19,6 +19,7 @@ import static com.google.common.truth.Truth.assertThat;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import java.io.IOException;
import org.joda.time.YearMonth;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;
@ -29,7 +30,7 @@ public class TransactionsReportingQueryBuilderTest {
private TransactionsReportingQueryBuilder getQueryBuilder() {
TransactionsReportingQueryBuilder queryBuilder = new TransactionsReportingQueryBuilder();
queryBuilder.yearMonth = "2017-09";
queryBuilder.yearMonth = new YearMonth(2017, 9);
queryBuilder.projectId = "domain-registry-alpha";
return queryBuilder;
}

View file

@ -45,12 +45,12 @@ JOIN (
`domain-registry-alpha.latest_datastore_export.DomainBase`,
UNNEST(nsHosts) AS hosts
WHERE _d = 'DomainResource'
AND creationTime <= TIMESTAMP("2017-09-30 23:59:59")
AND deletionTime > TIMESTAMP("2017-09-30 23:59:59") ) AS domain_table
AND creationTime <= TIMESTAMP("2017-09-30 23:59:59.999")
AND deletionTime > TIMESTAMP("2017-09-30 23:59:59.999") ) AS domain_table
ON
host_table.__key__.name = domain_table.referencedHostName
WHERE creationTime <= TIMESTAMP("2017-09-30 23:59:59")
AND deletionTime > TIMESTAMP("2017-09-30 23:59:59")
WHERE creationTime <= TIMESTAMP("2017-09-30 23:59:59.999")
AND deletionTime > TIMESTAMP("2017-09-30 23:59:59.999")
GROUP BY tld, registrarName
ORDER BY tld, registrarName

View file

@ -63,8 +63,8 @@ FROM (
WHERE entries.domainTransactionRecords IS NOT NULL )
-- Only look at this month's data
WHERE reportingTime
BETWEEN TIMESTAMP('2017-09-01 00:00:00')
AND TIMESTAMP('2017-09-30 23:59:59')
BETWEEN TIMESTAMP('2017-09-01 00:00:00.000')
AND TIMESTAMP('2017-09-30 23:59:59.999')
GROUP BY
tld,
clientId,

View file

@ -63,8 +63,8 @@ FROM (
WHERE entries.domainTransactionRecords IS NOT NULL )
-- Only look at this month's data
WHERE reportingTime
BETWEEN TIMESTAMP('2017-09-01 00:00:00')
AND TIMESTAMP('2017-09-30 23:59:59')
BETWEEN TIMESTAMP('2017-09-01 00:00:00.000')
AND TIMESTAMP('2017-09-30 23:59:59.999')
GROUP BY
tld,
clientId,