mirrors/nomulus
1
0
Fork 0
mirror of https://github.com/google/nomulus synced 2026-01-03 19:54:18 +00:00
Code Issues Releases Wiki Activity

Fix handling of negative values in monthly transaction reporting (#2704)

Browse Source 
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.
This commit is contained in:
Ben McIlwain
2025-03-20 17:13:08 -04:00
committed by GitHub
parent 11702bc940
commit 04b30f5c04
3 changed files with 25 additions and 23 deletions
Download Patch File Download Diff File Expand all files Collapse all files

22
core/src/main/resources/google/registry/reporting/icann/sql/transaction_counts.sql
View File

@@ -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

4
core/src/test/java/google/registry/reporting/icann/TransactionsReportingQueryBuilderTest.java
View File

@@ -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));
}
}

22
core/src/test/resources/google/registry/reporting/icann/transaction_counts_test.sql
View File

@@ -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