From 04b30f5c04710a0b323e6de6da9ef95af22a6f26 Mon Sep 17 00:00:00 2001 From: Ben McIlwain Date: Thu, 20 Mar 2025 17:13:08 -0400 Subject: [PATCH] Fix handling of negative values in monthly transaction reporting (#2704) The SQL statement was incorrectly flooring to zero one layer too deep, which was negating all negative transaction report rows (which occur most frequently when a domain in the autorenew grace period is deleted). I've changed it so that it now only floors to zero at the report level, which still solves the issue reported in http://b/290228682 but whose original fix caused the issue http://b/344645788 This bug was introduced in https://github.com/google/nomulus/pull/2074 I tested this by running the new query against the DB for 2024 Q4 using the registrar that was having issues and confirmed that the total renewal numbers for .app now match with the sum total of what we invoiced for the last three months of 2024. --- .../icann/sql/transaction_counts.sql | 22 +++++++++---------- ...TransactionsReportingQueryBuilderTest.java | 4 +++- .../icann/transaction_counts_test.sql | 22 +++++++++---------- 3 files changed, 25 insertions(+), 23 deletions(-) diff --git a/core/src/main/resources/google/registry/reporting/icann/sql/transaction_counts.sql b/core/src/main/resources/google/registry/reporting/icann/sql/transaction_counts.sql index 763958f7b..88467b9b1 100644 --- a/core/src/main/resources/google/registry/reporting/icann/sql/transaction_counts.sql +++ b/core/src/main/resources/google/registry/reporting/icann/sql/transaction_counts.sql @@ -32,7 +32,16 @@ FROM ( WHEN field = 'TRANSFER_NACKED' THEN 'TRANSFER_GAINING_NACKED' ELSE field END AS metricName, - SUM(amount) AS metricValue + -- 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 non-negative to pass ICANN validation. + GREATEST(SUM(amount), 0) AS metricValue FROM ( SELECT CASE @@ -47,16 +56,7 @@ FROM ( END AS clientId, tld, report_field AS field, - -- 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. - GREATEST(report_amount, 0) AS amount, + report_amount 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/TransactionsReportingQueryBuilderTest.java b/core/src/test/java/google/registry/reporting/icann/TransactionsReportingQueryBuilderTest.java index 5357b290b..b7afdfa01 100644 --- a/core/src/test/java/google/registry/reporting/icann/TransactionsReportingQueryBuilderTest.java +++ b/core/src/test/java/google/registry/reporting/icann/TransactionsReportingQueryBuilderTest.java @@ -15,6 +15,7 @@ package google.registry.reporting.icann; import static com.google.common.truth.Truth.assertThat; +import static com.google.common.truth.Truth.assertWithMessage; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; @@ -60,7 +61,8 @@ class TransactionsReportingQueryBuilderTest { for (String queryName : expectedQueryNames) { String actualTableName = String.format("%s_201709", queryName); String testFilename = String.format("%s_test.sql", queryName); - assertThat(actualQueries.get(actualTableName)) + assertWithMessage("Query expected in test data file %s differs", testFilename) + .that(actualQueries.get(actualTableName)) .isEqualTo(ReportingTestData.loadFile(testFilename)); } } diff --git a/core/src/test/resources/google/registry/reporting/icann/transaction_counts_test.sql b/core/src/test/resources/google/registry/reporting/icann/transaction_counts_test.sql index 2e52d80d3..4b1ee8c96 100644 --- a/core/src/test/resources/google/registry/reporting/icann/transaction_counts_test.sql +++ b/core/src/test/resources/google/registry/reporting/icann/transaction_counts_test.sql @@ -32,7 +32,16 @@ FROM ( WHEN field = 'TRANSFER_NACKED' THEN 'TRANSFER_GAINING_NACKED' ELSE field END AS metricName, - SUM(amount) AS metricValue + -- 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 non-negative to pass ICANN validation. + GREATEST(SUM(amount), 0) AS metricValue FROM ( SELECT CASE @@ -47,16 +56,7 @@ FROM ( END AS clientId, tld, report_field AS field, - -- 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. - GREATEST(report_amount, 0) AS amount, + report_amount 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