Skip to content

Commit

Permalink
Fix handling of negative values in monthly transaction reporting
Browse files Browse the repository at this point in the history
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 #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.
  • Loading branch information
CydeWeys committed Mar 5, 2025
1 parent a3f510d commit 4a69414
Showing 1 changed file with 11 additions and 11 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down

0 comments on commit 4a69414

Please sign in to comment.