Fix NpgsqlBox behaves incorrectly with negative coordinates (#5502)

Fixes #5500 (cherry picked from commit 5b998e8)
npgsql · Jan 31, 2024 · fa583c3 · fa583c3
1 parent 6c2f1b7
commit fa583c3
Show file tree

Hide file tree

Showing 2 changed files with 73 additions and 58 deletions.
diff --git a/src/Npgsql/NpgsqlTypes/NpgsqlTypes.cs b/src/Npgsql/NpgsqlTypes/NpgsqlTypes.cs
@@ -128,67 +128,33 @@ public override bool Equals(object? obj)
 /// </remarks>
 public struct NpgsqlBox : IEquatable<NpgsqlBox>
 {
+    NpgsqlPoint _upperRight;
     public NpgsqlPoint UpperRight
     {
         get => _upperRight;
         set
         {
-            if (value.X < _lowerLeft.X)
-            {
-                _upperRight.X = _lowerLeft.X;
-                _lowerLeft.X = value.X;
-            }
-            else
-            {
-                _upperRight.X = value.X;
-            }
-
-            if (value.Y < _lowerLeft.Y)
-            {
-                _upperRight.Y = _lowerLeft.Y;
-                _lowerLeft.Y = value.Y;
-            }
-            else
-            {
-                _upperRight.Y = value.Y;
-            }
+            _upperRight = value;
+            NormalizeBox();
         }
     }
-    private NpgsqlPoint _upperRight;
-
 
+    NpgsqlPoint _lowerLeft;
     public NpgsqlPoint LowerLeft
     {
         get => _lowerLeft;
         set
         {
-            if (value.X > _upperRight.X)
-            {
-                _lowerLeft.X = _upperRight.X;
-                _upperRight.X = value.X;
-            }
-            else
-            {
-                _lowerLeft.X = value.X;
-            }
-
-            if (value.Y > _upperRight.Y)
-            {
-                _lowerLeft.Y = _upperRight.Y;
-                _upperRight.Y = value.Y;
-            }
-            else
-            {
-                _lowerLeft.Y = value.Y;
-            }
+            _lowerLeft = value;
+            NormalizeBox();
         }
     }
-    private NpgsqlPoint _lowerLeft;
 
     public NpgsqlBox(NpgsqlPoint upperRight, NpgsqlPoint lowerLeft) : this()
     {
-        UpperRight = upperRight;
-        LowerLeft = lowerLeft;
+        _upperRight = upperRight;
+        _lowerLeft = lowerLeft;
+        NormalizeBox();
     }
 
     public NpgsqlBox(double top, double right, double bottom, double left)
@@ -216,6 +182,17 @@ public override string ToString()
 
     public override int GetHashCode()
         => HashCode.Combine(Top, Right, Bottom, LowerLeft);
+
+    // Swaps corners for isomorphic boxes, to mirror postgres behavior.
+    // See: https://github.com/postgres/postgres/blob/af2324fabf0020e464b0268be9ef03e8f46ed84b/src/backend/utils/adt/geo_ops.c#L435-L447
+    void NormalizeBox()
+    {
+        if (_upperRight.X < _lowerLeft.X)
+            (_upperRight.X, _lowerLeft.X) = (_lowerLeft.X, _upperRight.X);
+
+        if (_upperRight.Y < _lowerLeft.Y)
+            (_upperRight.Y, _lowerLeft.Y) = (_lowerLeft.Y, _upperRight.Y);
+    }
 }
 
 /// <summary>

diff --git a/test/Npgsql.Tests/Types/GeometricTypeTests.cs b/test/Npgsql.Tests/Types/GeometricTypeTests.cs
@@ -34,36 +34,74 @@ public async Task Box()
             NpgsqlDbType.Box,
             skipArrayCheck: true); // Uses semicolon instead of comma as separator
 
+        await AssertType(
+            new NpgsqlBox(top: -10, right: 0, bottom: -20, left: -10),
+            "(0,-10),(-10,-20)",
+            "box",
+            NpgsqlDbType.Box,
+            skipArrayCheck: true); // Uses semicolon instead of comma as separator
+
         await AssertType(
             new NpgsqlBox(top: 1, right: 2, bottom: 3, left: 4),
             "(4,3),(2,1)",
             "box",
             NpgsqlDbType.Box,
             skipArrayCheck: true); // Uses semicolon instead of comma as separator
+
+        var swapped = new NpgsqlBox(top: -20, right: -10, bottom: -10, left: 0);
+
+        await AssertType(
+            swapped,
+            "(0,-10),(-10,-20)",
+            "box",
+            NpgsqlDbType.Box,
+            skipArrayCheck: true); // Uses semicolon instead of comma as separator
+
+        await AssertType(
+            swapped with { UpperRight = new NpgsqlPoint(-20,-10) },
+            "(-10,-10),(-20,-20)",
+            "box",
+            NpgsqlDbType.Box,
+            skipArrayCheck: true); // Uses semicolon instead of comma as separator
+
+        await AssertType(
+            swapped with { LowerLeft = new NpgsqlPoint(10, 10) },
+            "(10,10),(0,-10)",
+            "box",
+            NpgsqlDbType.Box,
+            skipArrayCheck: true); // Uses semicolon instead of comma as separator
     }
 
     [Test]
     public async Task Box_array()
     {
-        var boxarr = await AssertType(
-            new[]
-            {
-                new NpgsqlBox(top: 3, right: 4, bottom: 1, left: 2),
-                new NpgsqlBox(top: 5, right: 6, bottom: 3, left: 4),
-            },
-            "{(4,3),(2,1);(6,5),(4,3)}",
+        var data = new[]
+        {
+            new NpgsqlBox(top: 3, right: 4, bottom: 1, left: 2),
+            new NpgsqlBox(top: 5, right: 6, bottom: 3, left: 4),
+            new NpgsqlBox(top: -10, right: 0, bottom: -20, left: -10)
+        };
+
+        await AssertType(
+            data,
+            "{(4,3),(2,1);(6,5),(4,3);(0,-10),(-10,-20)}",
             "box[]",
-            NpgsqlDbType.Box | NpgsqlDbType.Array);
+            NpgsqlDbType.Box | NpgsqlDbType.Array
+            );
+
+        var swappedData = new[]
+        {
+            new NpgsqlBox(top: 1, right: 2, bottom: 3, left: 4),
+            new NpgsqlBox(top: 3, right: 4, bottom: 5, left: 6),
+            new NpgsqlBox(top: -20, right: -10, bottom: -10, left: 0)
+        };
 
         await AssertType(
-            new[]
-            {
-                new NpgsqlBox(top: 1, right: 2, bottom: 3, left: 4),
-                new NpgsqlBox(top: 3, right: 4, bottom: 5, left: 6)
-            },
-            "{(4,3),(2,1);(6,5),(4,3)}",
+            swappedData,
+            "{(4,3),(2,1);(6,5),(4,3);(0,-10),(-10,-20)}",
             "box[]",
-            NpgsqlDbType.Box | NpgsqlDbType.Array);
+            NpgsqlDbType.Box | NpgsqlDbType.Array
+            );
     }
 
     [Test]