Modify and fail-fast GeoSearchParam (#3827)

redis · Apr 29, 2024 · dfb1640 · dfb1640
1 parent 8049b7b
commit dfb1640
Show file tree

Hide file tree

Showing 5 changed files with 48 additions and 49 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
@@ -540,30 +540,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
@@ -537,30 +537,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
@@ -84,13 +84,13 @@ public void georadiusByMemberStore() {
   public void georadiusByMemberStoreBinary() {
   }
 
-  @Test
   @Ignore
+  @Override
   public void geosearchstore() {
   }
 
-  @Test
   @Ignore
+  @Override
   public void geosearchstoreWithdist() {
   }
 }
diff --git a/src/test/java/redis/clients/jedis/commands/unified/pipeline/GeoPipelineCommandsTest.java b/src/test/java/redis/clients/jedis/commands/unified/pipeline/GeoPipelineCommandsTest.java
@@ -798,32 +798,24 @@ public void geosearch() {
         contains(0L));
   }
 
-  @Test
-  public void geosearchNegative() {
-    // combine byradius and bybox
-    pipe.geosearch("barcelona",
-        new GeoSearchParam().byRadius(3000, GeoUnit.M).byBox(300, 300, GeoUnit.M));
-
-    // without frommember and without fromlonlat
-    pipe.geosearch("barcelona",
-        new GeoSearchParam().byRadius(10, GeoUnit.MI));
+  @Test(expected = IllegalArgumentException.class)
+  public void geosearchSearchParamCombineFromMemberAndFromLonLat() {
+    pipe.geosearch("barcelona", new GeoSearchParam().fromMember("foobar").fromLonLat(10, 10));
+  }
 
-    assertThat(pipe.syncAndReturnAll(), contains(
-        instanceOf(JedisDataException.class),
-        instanceOf(JedisDataException.class)
-    ));
+  @Test(expected = IllegalArgumentException.class)
+  public void geosearchSearchParamWithoutFromMemberAndFromLonLat() {
+    pipe.geosearch("barcelona", new GeoSearchParam().byRadius(10, GeoUnit.MI));
   }
 
   @Test(expected = IllegalArgumentException.class)
-  public void geosearchSearchParamWithoutRadiousAndWithoutBox() {
-    pipe.geosearch("barcelona",
-        new GeoSearchParam().fromMember("foobar"));
+  public void geosearchSearchParamCombineByRadiousAndByBox() {
+    pipe.geosearch("barcelona", new GeoSearchParam().byRadius(3000, GeoUnit.M).byBox(300, 300, GeoUnit.M));
   }
 
   @Test(expected = IllegalArgumentException.class)
-  public void geosearchSearchParamCombineMemberAndLonLat() {
-    pipe.geosearch("barcelona",
-        new GeoSearchParam().fromMember("foobar").fromLonLat(10, 10));
+  public void geosearchSearchParamWithoutByRadiousAndByBox() {
+    pipe.geosearch("barcelona", new GeoSearchParam().fromMember("foobar"));
   }
 
   @Test