Modify and fail-fast GeoSearchParam (#3827)

redis · Jul 9, 2024 · 927c923 · 927c923
1 parent 72073c5
commit 927c923
Show file tree

Hide file tree

Showing 4 changed files with 50 additions and 33 deletions.
diff --git a/src/main/java/redis/clients/jedis/params/GeoSearchParam.java b/src/main/java/redis/clients/jedis/params/GeoSearchParam.java
@@ -115,18 +115,25 @@ public GeoSearchParam any() {
 
   @Override
   public void addParams(CommandArguments args) {
-    if (this.fromMember) {
-      args.add(Keyword.FROMMEMBER).add(this.member);
-    } else if (this.fromLonLat) {
+    if (fromMember && fromLonLat) {
+      throw new IllegalArgumentException("Both FROMMEMBER and FROMLONLAT cannot be used.");
+    } else if (fromMember) {
+      args.add(Keyword.FROMMEMBER).add(member);
+    } else if (fromLonLat) {
       args.add(Keyword.FROMLONLAT).add(coord.getLongitude()).add(coord.getLatitude());
+    } else {
+      throw new IllegalArgumentException("Either FROMMEMBER or FROMLONLAT must be used.");
     }
 
-    if (this.byRadius) {
-      args.add(Keyword.BYRADIUS).add(this.radius);
-    } else if (this.byBox) {
-      args.add(Keyword.BYBOX).add(this.width).add(this.height);
+    if (byRadius && byBox) {
+      throw new IllegalArgumentException("Both BYRADIUS and BYBOX cannot be used.");
+    } else if (byRadius) {
+      args.add(Keyword.BYRADIUS).add(radius).add(unit);
+    } else if (byBox) {
+      args.add(Keyword.BYBOX).add(width).add(height).add(unit);
+    } else {
+      throw new IllegalArgumentException("Either BYRADIUS or BYBOX must be used.");
     }
-    args.add(this.unit);
 
     if (withCoord) {
       args.add(Keyword.WITHCOORD);

diff --git a/src/test/java/redis/clients/jedis/commands/jedis/GeoCommandsTest.java b/src/test/java/redis/clients/jedis/commands/jedis/GeoCommandsTest.java
@@ -532,30 +532,30 @@ public void geosearchNegative() {
     // combine byradius and bybox
     try {
       jedis.geosearch("barcelona", new GeoSearchParam()
-              .byRadius(3000, GeoUnit.M).byBox(300, 300, GeoUnit.M));
+          .byRadius(3000, GeoUnit.M)
+          .byBox(300, 300, GeoUnit.M));
       fail();
-    } catch (Exception ignored) { }
+    } catch (IllegalArgumentException ignored) { }
 
     // without byradius and without bybox
     try {
-      jedis.geosearch("barcelona", new GeoSearchParam()
-              .fromMember("foobar"));
+      jedis.geosearch("barcelona", new GeoSearchParam().fromMember("foobar"));
       fail();
-    } catch (Exception ignored) { }
+    } catch (IllegalArgumentException ignored) { }
 
     // combine frommember and fromlonlat
     try {
       jedis.geosearch("barcelona", new GeoSearchParam()
-              .fromMember("foobar").fromLonLat(10,10));
+          .fromMember("foobar")
+          .fromLonLat(10,10));
       fail();
-    } catch (Exception ignored) { }
+    } catch (IllegalArgumentException ignored) { }
 
     // without frommember and without fromlonlat
     try {
-      jedis.geosearch("barcelona", new GeoSearchParam()
-              .byRadius(10, GeoUnit.MI));
+      jedis.geosearch("barcelona", new GeoSearchParam().byRadius(10, GeoUnit.MI));
       fail();
-    } catch (Exception ignored) { }
+    } catch (IllegalArgumentException ignored) { }
   }
 
   @Test

diff --git a/src/test/java/redis/clients/jedis/commands/unified/GeoCommandsTestBase.java b/src/test/java/redis/clients/jedis/commands/unified/GeoCommandsTestBase.java
@@ -532,30 +532,30 @@ public void geosearchNegative() {
     // combine byradius and bybox
     try {
       jedis.geosearch("barcelona", new GeoSearchParam()
-              .byRadius(3000, GeoUnit.M).byBox(300, 300, GeoUnit.M));
+          .byRadius(3000, GeoUnit.M)
+          .byBox(300, 300, GeoUnit.M));
       fail();
-    } catch (redis.clients.jedis.exceptions.JedisDataException ignored) { }
+    } catch (IllegalArgumentException ignored) { }
 
     // without byradius and without bybox
     try {
-      jedis.geosearch("barcelona", new GeoSearchParam()
-              .fromMember("foobar"));
+      jedis.geosearch("barcelona", new GeoSearchParam().fromMember("foobar"));
       fail();
-    } catch (java.lang.IllegalArgumentException ignored) { }
+    } catch (IllegalArgumentException ignored) { }
 
     // combine frommember and fromlonlat
     try {
       jedis.geosearch("barcelona", new GeoSearchParam()
-              .fromMember("foobar").fromLonLat(10,10));
+          .fromMember("foobar")
+          .fromLonLat(10,10));
       fail();
-    } catch (java.lang.IllegalArgumentException ignored) { }
+    } catch (IllegalArgumentException ignored) { }
 
     // without frommember and without fromlonlat
     try {
-      jedis.geosearch("barcelona", new GeoSearchParam()
-              .byRadius(10, GeoUnit.MI));
+      jedis.geosearch("barcelona", new GeoSearchParam().byRadius(10, GeoUnit.MI));
       fail();
-    } catch (redis.clients.jedis.exceptions.JedisDataException ignored) { }
+    } catch (IllegalArgumentException ignored) { }
   }
 
   @Test

diff --git a/src/test/java/redis/clients/jedis/commands/unified/cluster/ClusterGeoCommandsTest.java b/src/test/java/redis/clients/jedis/commands/unified/cluster/ClusterGeoCommandsTest.java
@@ -15,9 +15,9 @@
 //
 //import redis.clients.jedis.GeoCoordinate;
 //import redis.clients.jedis.args.GeoUnit;
+//import redis.clients.jedis.commands.unified.GeoCommandsTestBase;
 //import redis.clients.jedis.params.GeoRadiusParam;
 //import redis.clients.jedis.params.GeoRadiusStoreParam;
-//import redis.clients.jedis.commands.unified.GeoCommandsTestBase;
 //
 //public class ClusterGeoCommandsTest extends GeoCommandsTestBase {
 //
@@ -51,8 +51,8 @@
 //    jedis.geoadd("Sicily {ITA}", coordinateMap);
 //
 //    long size = jedis.georadiusStore("Sicily {ITA}", 15, 37, 200, GeoUnit.KM,
-//      GeoRadiusParam.geoRadiusParam(),
-//      GeoRadiusStoreParam.geoRadiusStoreParam().store("{ITA} SicilyStore"));
+//        GeoRadiusParam.geoRadiusParam(),
+//        GeoRadiusStoreParam.geoRadiusStoreParam().store("{ITA} SicilyStore"));
 //    assertEquals(2, size);
 //    List<String> expected = new ArrayList<>();
 //    expected.add("Palermo");
@@ -73,8 +73,8 @@
 //    jedis.geoadd("Sicily {ITA}", 15.087269, 37.502669, "Catania");
 //
 //    long size = jedis.georadiusByMemberStore("Sicily {ITA}", "Agrigento", 100, GeoUnit.KM,
-//      GeoRadiusParam.geoRadiusParam(),
-//      GeoRadiusStoreParam.geoRadiusStoreParam().store("{ITA} SicilyStore"));
+//        GeoRadiusParam.geoRadiusParam(),
+//        GeoRadiusStoreParam.geoRadiusStoreParam().store("{ITA} SicilyStore"));
 //    assertEquals(2, size);
 //    List<String> expected = new ArrayList<>();
 //    expected.add("Agrigento");
@@ -86,4 +86,14 @@
 //  @Override
 //  public void georadiusByMemberStoreBinary() {
 //  }
+//
+//  @Ignore
+//  @Override
+//  public void geosearchstore() {
+//  }
+//
+//  @Ignore
+//  @Override
+//  public void geosearchstoreWithdist() {
+//  }
 //}