From fdfac8760f2a0db4de7bb83862df8b26f6c2916a Mon Sep 17 00:00:00 2001
From: Vipul A M <vipulnsward@gmail.com>
Date: Fri, 5 Jun 2020 01:33:46 +0530
Subject: [PATCH] Although libraries support both formats of sign before and
 after DIGITS(ex: https://github.com/moment/luxon/pull/683,
 https://github.com/moment/moment/issues/2408), many do not.

For example PG refers to https://www.ietf.org/rfc/rfc3339.txt when converting(Ref: https://www.postgresql.org/docs/current/datatype-datetime.html)

According to the ref there is no explicit mention of allowing sign before the parts, which reads as below:

 Durations:

    dur-second        = 1*DIGIT "S"
    dur-minute        = 1*DIGIT "M" [dur-second]
    dur-hour          = 1*DIGIT "H" [dur-minute]
    dur-time          = "T" (dur-hour / dur-minute / dur-second)
    dur-day           = 1*DIGIT "D"
    dur-week          = 1*DIGIT "W"
    dur-month         = 1*DIGIT "M" [dur-day]
    dur-year          = 1*DIGIT "Y" [dur-month]
    dur-date          = (dur-day / dur-month / dur-year) [dur-time]

    duration          = "P" (dur-date / dur-time / dur-week)

We should not attempt to move sign forward in this case.
---
 activesupport/CHANGELOG.md                    | 21 +++++++++++++++++++
 .../duration/iso8601_serializer.rb            | 12 +++--------
 activesupport/test/core_ext/duration_test.rb  |  5 +++--
 3 files changed, 27 insertions(+), 11 deletions(-)

diff --git a/activesupport/CHANGELOG.md b/activesupport/CHANGELOG.md
index 45aa6958de0..71d62d907ad 100644
--- a/activesupport/CHANGELOG.md
+++ b/activesupport/CHANGELOG.md
@@ -1,3 +1,24 @@
+*   Calling `iso8601` on negative durations retains the negative sign on individual
+    digits instead of prepending it.
+
+    This change is required so we can interoperate with PostgreSQL, which prefers
+    negative signs for each component.
+
+    Compatibility with other iso8601 parsers which support leading negatives as well
+    as negatives per component is still retained.
+
+    Before:
+
+        (-1.year - 1.day).iso8601
+        # => "-P1Y1D"
+
+    After:
+
+        (-1.year - 1.day).iso8601
+        # => "P-1Y-1D"
+
+    *Vipul A M*
+
 *   `require_dependency` has been documented to be _obsolete_ in `:zeitwerk`
     mode. The method is not deprecated as such (yet), but applications are
     encouraged to not use it.
diff --git a/activesupport/lib/active_support/duration/iso8601_serializer.rb b/activesupport/lib/active_support/duration/iso8601_serializer.rb
index 2a28050c13c..96296274df8 100644
--- a/activesupport/lib/active_support/duration/iso8601_serializer.rb
+++ b/activesupport/lib/active_support/duration/iso8601_serializer.rb
@@ -15,7 +15,7 @@ module ActiveSupport
 
       # Builds and returns output string.
       def serialize
-        parts, sign = normalize
+        parts = normalize
         return "PT0S" if parts.empty?
 
         output = +"P"
@@ -30,7 +30,7 @@ module ActiveSupport
           time << "#{sprintf(@precision ? "%0.0#{@precision}f" : '%g', parts[:seconds])}S"
         end
         output << "T#{time}" unless time.empty?
-        "#{sign}#{output}"
+        output
       end
 
       private
@@ -48,13 +48,7 @@ module ActiveSupport
             parts[:days] += parts.delete(:weeks) * SECONDS_PER_WEEK / SECONDS_PER_DAY
           end
 
-          # If all parts are negative - let's make a negative duration
-          sign = ""
-          if parts.values.all? { |v| v < 0 }
-            sign = "-"
-            parts.transform_values!(&:-@)
-          end
-          [parts, sign]
+          parts
         end
 
         def week_mixed_with_date?(parts)
diff --git a/activesupport/test/core_ext/duration_test.rb b/activesupport/test/core_ext/duration_test.rb
index 01072bece79..8b607729510 100644
--- a/activesupport/test/core_ext/duration_test.rb
+++ b/activesupport/test/core_ext/duration_test.rb
@@ -583,12 +583,13 @@ class DurationTest < ActiveSupport::TestCase
       ["P1Y1M21D",      1.year + 1.month + 3.week        ],
       ["P1Y1M",         1.year + 1.month                 ],
       ["P1Y1M1D",       1.year + 1.month + 1.day         ],
-      ["-P1Y1D",        -1.year - 1.day                  ],
+      ["P-1Y-1D",       -1.year - 1.day                  ],
       ["P1Y-1DT-1S",    1.year - 1.day - 1.second        ], # Parts with different signs are exists in PostgreSQL interval datatype.
       ["PT1S",          1.second                         ],
       ["PT1.4S",        (1.4).seconds                    ],
       ["P1Y1M1DT1H",    1.year + 1.month + 1.day + 1.hour],
       ["PT0S",          0.minutes                        ],
+      ["PT-0.2S",       (-0.2).seconds                   ],
     ]
     expectations.each do |expected_output, duration|
       assert_equal expected_output, duration.iso8601, expected_output.inspect
@@ -616,7 +617,7 @@ class DurationTest < ActiveSupport::TestCase
 
   def test_iso8601_output_and_reparsing
     patterns = %w[
-      P1Y P0.5Y P0,5Y P1Y1M P1Y0.5M P1Y0,5M P1Y1M1D P1Y1M0.5D P1Y1M0,5D P1Y1M1DT1H P1Y1M1DT0.5H P1Y1M1DT0,5H P1W +P1Y -P1Y
+      P1Y P0.5Y P0,5Y P1Y1M P1Y0.5M P1Y0,5M P1Y1M1D P1Y1M0.5D P1Y1M0,5D P1Y1M1DT1H P1Y1M1DT0.5H P1Y1M1DT0,5H P1W +P1Y -P1Y P-1Y
       P1Y1M1DT1H1M P1Y1M1DT1H0.5M P1Y1M1DT1H0,5M P1Y1M1DT1H1M1S P1Y1M1DT1H1M1.0S P1Y1M1DT1H1M1,0S P-1Y-2M3DT-4H-5M-6S
     ]
     # That could be weird, but if we parse P1Y1M0.5D and output it to ISO 8601, we'll get P1Y1MT12.0H.