Add support for custom range key and refactor addRange in a trait (#2227

) IpRange and GeoDistance aggregations were missing the ability to specify a custom key for a range. As it was already done for the Range aggregation, I moved that to a trait that is now used everywhere we need `addRange`. For the IpRange aggregation, I also added the key option to the `addMaskRange` method. The only drawback with this refactoring is that the arguments of the trait have to allow string|int|float|null to be usable everywhere and we loose some phpdoc details on the arguments. In my opinion, people using this should already know what format is expected and can always check the doc so I personally prefer to have the code deduplicated, but if you disagree I can change it back to add the key everywhere without using a common trait.
ruflin · Oct 21, 2024 · 223a25f · 223a25f
1 parent e2cb89b
commit 223a25f
Show file tree

Hide file tree

Showing 7 changed files with 119 additions and 97 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -11,6 +11,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ### Added
 * Added support for the multi-match query type `bool_prefix` [#2220](https://github.com/ruflin/Elastica/pull/2220)
 * Supported PHP 8.4 [#2221](https://github.com/ruflin/Elastica/pull/2221)
+* Added support for custom key to IpRange and GeoDistance `addRange` using a common trait [#2227](https://github.com/ruflin/Elastica/pull/2227)
 
 ### Changed
 

diff --git a/src/Aggregation/GeoDistance.php b/src/Aggregation/GeoDistance.php
@@ -4,8 +4,6 @@
 
 namespace Elastica\Aggregation;
 
-use Elastica\Exception\InvalidException;
-
 /**
  * Class GeoDistance.
  *
@@ -14,6 +12,7 @@
 class GeoDistance extends AbstractAggregation
 {
     use Traits\KeyedTrait;
+    use Traits\RangeTrait;
 
     public const DISTANCE_TYPE_ARC = 'arc';
     public const DISTANCE_TYPE_PLANE = 'plane';
@@ -56,35 +55,6 @@ public function setOrigin($origin): self
         return $this->setParam('origin', $origin);
     }
 
-    /**
-     * Add a distance range to this aggregation.
-     *
-     * @param int|null $fromValue a distance
-     * @param int|null $toValue   a distance
-     *
-     * @throws InvalidException
-     *
-     * @return $this
-     */
-    public function addRange(?int $fromValue = null, ?int $toValue = null): self
-    {
-        if (null === $fromValue && null === $toValue) {
-            throw new InvalidException('Either fromValue or toValue must be set. Both cannot be null.');
-        }
-
-        $range = [];
-
-        if (null !== $fromValue) {
-            $range['from'] = $fromValue;
-        }
-
-        if (null !== $toValue) {
-            $range['to'] = $toValue;
-        }
-
-        return $this->addParam('ranges', $range);
-    }
-
     /**
      * Set the unit of distance measure for this aggregation.
      *

diff --git a/src/Aggregation/IpRange.php b/src/Aggregation/IpRange.php
@@ -4,8 +4,6 @@
 
 namespace Elastica\Aggregation;
 
-use Elastica\Exception\InvalidException;
-
 /**
  * Class IpRange.
  *
@@ -14,6 +12,7 @@
 class IpRange extends AbstractAggregation
 {
     use Traits\KeyedTrait;
+    use Traits\RangeTrait;
 
     /**
      * @param string $name  the name of this aggregation
@@ -38,42 +37,21 @@ public function setField(string $field): self
     }
 
     /**
-     * Add an ip range to this aggregation.
-     *
-     * @param string|null $fromValue a valid ipv4 address. Low end of this range, exclusive (greater than)
-     * @param string|null $toValue   a valid ipv4 address. High end of this range, exclusive (less than)
+     * Add an ip range in the form of a CIDR mask.
      *
-     * @throws InvalidException
+     * @param string      $mask a valid CIDR mask
+     * @param string|null $key  customized key value
      *
      * @return $this
      */
-    public function addRange(?string $fromValue = null, ?string $toValue = null): self
+    public function addMaskRange(string $mask, ?string $key = null): self
     {
-        if (null === $fromValue && null === $toValue) {
-            throw new InvalidException('Either fromValue or toValue must be set. Both cannot be null.');
-        }
+        $range = ['mask' => $mask];
 
-        $range = [];
-        if (null !== $fromValue) {
-            $range['from'] = $fromValue;
-        }
-
-        if (null !== $toValue) {
-            $range['to'] = $toValue;
+        if (null !== $key) {
+            $range['key'] = $key;
         }
 
         return $this->addParam('ranges', $range);
     }
-
-    /**
-     * Add an ip range in the form of a CIDR mask.
-     *
-     * @param string $mask a valid CIDR mask
-     *
-     * @return $this
-     */
-    public function addMaskRange(string $mask): self
-    {
-        return $this->addParam('ranges', ['mask' => $mask]);
-    }
 }
diff --git a/src/Aggregation/Range.php b/src/Aggregation/Range.php
@@ -4,8 +4,6 @@
 
 namespace Elastica\Aggregation;
 
-use Elastica\Exception\InvalidException;
-
 /**
  * Class Range.
  *
@@ -14,38 +12,5 @@
 class Range extends AbstractSimpleAggregation
 {
     use Traits\KeyedTrait;
-
-    /**
-     * Add a range to this aggregation.
-     *
-     * @param float|int|string|null $fromValue low end of this range, exclusive (greater than or equal to)
-     * @param float|int|string|null $toValue   high end of this range, exclusive (less than)
-     * @param string|null           $key       customized key value
-     *
-     * @throws InvalidException
-     *
-     * @return $this
-     */
-    public function addRange($fromValue = null, $toValue = null, ?string $key = null): self
-    {
-        if (null === $fromValue && null === $toValue) {
-            throw new InvalidException('Either fromValue or toValue must be set. Both cannot be null.');
-        }
-
-        $range = [];
-
-        if (null !== $fromValue) {
-            $range['from'] = $fromValue;
-        }
-
-        if (null !== $toValue) {
-            $range['to'] = $toValue;
-        }
-
-        if (null !== $key) {
-            $range['key'] = $key;
-        }
-
-        return $this->addParam('ranges', $range);
-    }
+    use Traits\RangeTrait;
 }
diff --git a/src/Aggregation/Traits/RangeTrait.php b/src/Aggregation/Traits/RangeTrait.php
@@ -0,0 +1,44 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Elastica\Aggregation\Traits;
+
+use Elastica\Exception\InvalidException;
+
+trait RangeTrait
+{
+    /**
+     * Add a range to this aggregation.
+     *
+     * @param float|int|string|null $fromValue low end of this range, exclusive (greater than or equal to)
+     * @param float|int|string|null $toValue   high end of this range, exclusive (less than)
+     * @param string|null           $key       customized key value
+     *
+     * @throws InvalidException
+     *
+     * @return $this
+     */
+    public function addRange($fromValue = null, $toValue = null, ?string $key = null): self
+    {
+        if (null === $fromValue && null === $toValue) {
+            throw new InvalidException('Either fromValue or toValue must be set. Both cannot be null.');
+        }
+
+        $range = [];
+
+        if (null !== $fromValue) {
+            $range['from'] = $fromValue;
+        }
+
+        if (null !== $toValue) {
+            $range['to'] = $toValue;
+        }
+
+        if (null !== $key) {
+            $range['key'] = $key;
+        }
+
+        return $this->addParam('ranges', $range);
+    }
+}
diff --git a/tests/Aggregation/GeoDistanceTest.php b/tests/Aggregation/GeoDistanceTest.php
@@ -51,6 +51,37 @@ public function testGeoDistanceKeyedAggregation(): void
         $this->assertSame($expected, \array_keys($results['buckets']));
     }
 
+    /**
+     * @group unit
+     */
+    public function testGeoDistanceAggregationWithKey(): void
+    {
+        $agg = new GeoDistance('geo', 'location', ['lat' => 32.804654, 'lon' => -117.242594]);
+        $agg->addRange(null, 10, 'first');
+        $agg->addRange(10, null, 'second');
+        $agg->setUnit('mi');
+
+        $expected = [
+            'geo_distance' => [
+                'field' => 'location',
+                'origin' => ['lat' => 32.804654, 'lon' => -117.242594],
+                'unit' => 'mi',
+                'ranges' => [
+                    [
+                        'to' => 10,
+                        'key' => 'first',
+                    ],
+                    [
+                        'from' => 10,
+                        'key' => 'second',
+                    ],
+                ],
+            ],
+        ];
+
+        $this->assertEquals($expected, $agg->toArray());
+    }
+
     protected function _getIndexForTest(): Index
     {
         $index = $this->_createIndex();

diff --git a/tests/Aggregation/IpRangeTest.php b/tests/Aggregation/IpRangeTest.php
@@ -67,6 +67,39 @@ public function testIpRangeKeyedAggregation(): void
         $this->assertSame($expected, \array_keys($results['buckets']));
     }
 
+    /**
+     * @group unit
+     */
+    public function testIpRangeAggregationWithKey(): void
+    {
+        $agg = new IpRange('ip', 'address');
+        $agg->addRange('192.168.1.101', null, 'first');
+        $agg->addRange(null, '192.168.1.200', 'second');
+        $agg->addMaskRange('192.168.1.0/24', 'mask');
+
+        $expected = [
+            'ip_range' => [
+                'field' => 'address',
+                'ranges' => [
+                    [
+                        'from' => '192.168.1.101',
+                        'key' => 'first',
+                    ],
+                    [
+                        'to' => '192.168.1.200',
+                        'key' => 'second',
+                    ],
+                    [
+                        'mask' => '192.168.1.0/24',
+                        'key' => 'mask',
+                    ],
+                ],
+            ],
+        ];
+
+        $this->assertEquals($expected, $agg->toArray());
+    }
+
     protected function _getIndexForTest(): Index
     {
         $index = $this->_createIndex();