From 3ea31d024e79b62087492ee020566c554cd1b5cc Mon Sep 17 00:00:00 2001 From: Lai Jiang Date: Wed, 12 Jul 2023 12:56:09 -0400 Subject: [PATCH] Add a floor of zero to transaction report counts (#2074) See b/290228682, there are edge cases in which the net_renew would be negative when a domain is cancelled by superusers during renew grace period. The correct thing to do is attribute the cancellation to the owning registrar, but that would require changing the owing registrar of the the corresponding cancellation DomainHistory, which has cascading effects that we don't want to deal with. As such we simply floor the number here to zero to prevent any negative value from appearing, which should have negligible impact as the edge cage happens very rarely, more specifically when a cancellation happens during grace period by a registrar other than the the owning one. All the numbers here should be positive to pass ICANN validation. --- .../icann/sql/cloud_sql_transaction_counts.sql | 11 ++++++++++- .../icann/ActivityReportingQueryBuilderTest.java | 2 +- .../icann/transaction_counts_test_cloud_sql.sql | 11 ++++++++++- 3 files changed, 21 insertions(+), 3 deletions(-) diff --git a/core/src/main/resources/google/registry/reporting/icann/sql/cloud_sql_transaction_counts.sql b/core/src/main/resources/google/registry/reporting/icann/sql/cloud_sql_transaction_counts.sql index 1e64be742..006398a77 100644 --- a/core/src/main/resources/google/registry/reporting/icann/sql/cloud_sql_transaction_counts.sql +++ b/core/src/main/resources/google/registry/reporting/icann/sql/cloud_sql_transaction_counts.sql @@ -47,7 +47,16 @@ FROM ( END AS clientId, tld, report_field AS field, - report_amount AS amount, + -- See b/290228682, there are edge cases in which the net_renew would be negative when + -- a domain is cancelled by superusers during renew grace period. The correct thing + -- to do is attribute the cancellation to the owning registrar, but that would require + -- changing the owing registrar of the the corresponding cancellation DomainHistory, + -- which has cascading effects that we don't want to deal with. As such we simply + -- floor the number here to zero to prevent any negative value from appearing, which + -- should have negligible impact as the edge cage happens very rarely, more specifically + -- when a cancellation happens during grace period by a registrar other than the the + -- owning one. All the numbers here should be positive to pass ICANN validation. + MAX(report_amount, 0) AS amount, reporting_time AS reportingTime FROM EXTERNAL_QUERY("projects/%PROJECT_ID%/locations/us/connections/%PROJECT_ID%-sql", ''' SELECT history_type, history_other_registrar_id, history_registrar_id, domain_repo_id, history_revision_id FROM "DomainHistory";''') AS dh diff --git a/core/src/test/java/google/registry/reporting/icann/ActivityReportingQueryBuilderTest.java b/core/src/test/java/google/registry/reporting/icann/ActivityReportingQueryBuilderTest.java index 015f8bdfa..386c3e495 100644 --- a/core/src/test/java/google/registry/reporting/icann/ActivityReportingQueryBuilderTest.java +++ b/core/src/test/java/google/registry/reporting/icann/ActivityReportingQueryBuilderTest.java @@ -45,7 +45,7 @@ class ActivityReportingQueryBuilderTest { } @Test - void testIntermediaryQueryMatch_cloud_sql() { + void testIntermediaryQueryMatch_cloudSql() { ImmutableList expectedQueryNames = ImmutableList.of( ActivityReportingQueryBuilder.REGISTRAR_OPERATING_STATUS, diff --git a/core/src/test/resources/google/registry/reporting/icann/transaction_counts_test_cloud_sql.sql b/core/src/test/resources/google/registry/reporting/icann/transaction_counts_test_cloud_sql.sql index 78460eeaf..cf0db80e4 100644 --- a/core/src/test/resources/google/registry/reporting/icann/transaction_counts_test_cloud_sql.sql +++ b/core/src/test/resources/google/registry/reporting/icann/transaction_counts_test_cloud_sql.sql @@ -47,7 +47,16 @@ FROM ( END AS clientId, tld, report_field AS field, - report_amount AS amount, + -- See b/290228682, there are edge cases in which the net_renew would be negative when + -- a domain is cancelled by superusers during renew grace period. The correct thing + -- to do is attribute the cancellation to the owning registrar, but that would require + -- changing the owing registrar of the the corresponding cancellation DomainHistory, + -- which has cascading effects that we don't want to deal with. As such we simply + -- floor the number here to zero to prevent any negative value from appearing, which + -- should have negligible impact as the edge cage happens very rarely, more specifically + -- when a cancellation happens during grace period by a registrar other than the the + -- owning one. All the numbers here should be positive to pass ICANN validation. + MAX(report_amount, 0) AS amount, reporting_time AS reportingTime FROM EXTERNAL_QUERY("projects/domain-registry-alpha/locations/us/connections/domain-registry-alpha-sql", ''' SELECT history_type, history_other_registrar_id, history_registrar_id, domain_repo_id, history_revision_id FROM "DomainHistory";''') AS dh