Query Refactoring: validate GeoShapeQueryBuilder strategy and relatio…

…n parameter Before the refactoring we didn't check any invalid settings for strategy and relation in the GeoShapeQueryBuilder. However, using SpatialStrategy.TERM and ShapeRelation.INTERSECTS together is invalid and we tried to protect against that in the validate() method. This PR moves these checks to setter for strategy and relation and adds tests for the new behaviour. Relates to elastic#10217
cbuescher · Sep 25, 2015 · 74d6411 · 74d6411
1 parent a50a0da
commit 74d6411
Show file tree

Hide file tree

Showing 2 changed files with 25 additions and 34 deletions.
diff --git a/core/src/main/java/org/elasticsearch/index/query/GeoShapeQueryBuilder.java b/core/src/main/java/org/elasticsearch/index/query/GeoShapeQueryBuilder.java
@@ -182,6 +182,10 @@ public String indexedShapeType() {
      * @return this
      */
     public GeoShapeQueryBuilder strategy(SpatialStrategy strategy) {
+        if (strategy != null && strategy == SpatialStrategy.TERM && relation != ShapeRelation.INTERSECTS) {
+            throw new IllegalArgumentException("strategy [" + strategy.getStrategyName() + "] only supports relation ["
+                    + ShapeRelation.INTERSECTS.getRelationName() + "] found relation [" + relation.getRelationName() + "]");
+        }
         this.strategy = strategy;
         return this;
     }
@@ -240,6 +244,10 @@ public GeoShapeQueryBuilder relation(ShapeRelation relation) {
         if (relation == null) {
             throw new IllegalArgumentException("No Shape Relation defined");
         }
+        if (strategy != null && strategy == SpatialStrategy.TERM && relation != ShapeRelation.INTERSECTS) {
+            throw new IllegalArgumentException("current strategy [" + strategy.getStrategyName() + "] only supports relation ["
+                    + ShapeRelation.INTERSECTS.getRelationName() + "] found relation [" + relation.getRelationName() + "]");
+        }
         this.relation = relation;
         return this;
     }
@@ -251,17 +259,6 @@ public ShapeRelation relation() {
         return relation;
     }
 
-    public QueryValidationException validate() {
-        QueryValidationException errors = null;
-        // TODO did we validate this before the refactoring and can we do this in setters at all?
-        if (strategy != null && strategy == SpatialStrategy.TERM && relation != ShapeRelation.INTERSECTS) {
-            errors = QueryValidationException.addValidationError(NAME,
-                    "strategy [" + strategy.getStrategyName() + "] only supports relation ["
-                    + ShapeRelation.INTERSECTS.getRelationName() + "] found relation [" + relation.getRelationName() + "]", errors);
-        }
-        return errors;
-    }
-
     @Override
     protected Query doToQuery(QueryShardContext context) throws IOException {
         ShapeBuilder shape;

diff --git a/core/src/test/java/org/elasticsearch/index/query/GeoShapeQueryBuilderTests.java b/core/src/test/java/org/elasticsearch/index/query/GeoShapeQueryBuilderTests.java
@@ -166,37 +166,31 @@ public void testNoIndexedShapeType() throws IOException {
         new GeoShapeQueryBuilder(GEO_SHAPE_FIELD_NAME, "id", (String) null);
     }
 
-    @Test
+    @Test(expected=IllegalArgumentException.class)
     public void testNoRelation() throws IOException {
         ShapeBuilder shape = RandomShapeGenerator.createShapeWithin(getRandom(), null);
-        try {
-            GeoShapeQueryBuilder builder = new GeoShapeQueryBuilder(GEO_SHAPE_FIELD_NAME, shape);
-            builder.relation(null);
-            fail("relation cannot be null");
-        } catch (IllegalArgumentException e) {
-            // expected
-        }
+        GeoShapeQueryBuilder builder = new GeoShapeQueryBuilder(GEO_SHAPE_FIELD_NAME, shape);
+        builder.relation(null);
     }
 
     @Test
-    public void testInvalidRelation() {
+    public void testInvalidRelation() throws IOException {
         ShapeBuilder shape = RandomShapeGenerator.createShapeWithin(getRandom(), null);
+        GeoShapeQueryBuilder builder = new GeoShapeQueryBuilder(GEO_SHAPE_FIELD_NAME, shape);
         try {
-            GeoShapeQueryBuilder builder = new GeoShapeQueryBuilder(GEO_SHAPE_FIELD_NAME, shape);
             builder.strategy(SpatialStrategy.TERM);
-            ShapeRelation relation = randomFrom(ShapeRelation.DISJOINT, ShapeRelation.WITHIN);
-            builder.relation(relation);
-            QueryValidationException exception = builder.validate();
-            assertThat(exception, notNullValue());
-            assertThat(exception.validationErrors(), notNullValue());
-            assertThat(exception.validationErrors().size(), equalTo(1));
-            assertThat(
-                    exception.validationErrors().get(0),
-                    equalTo("[" + GeoShapeQueryBuilder.NAME + "] strategy [" + SpatialStrategy.TERM.getStrategyName()
-                            + "] only supports relation [" + ShapeRelation.INTERSECTS.getRelationName() + "] found relation ["
-                            + relation.getRelationName() + "]"));
-        } catch (IOException e) {
-            throw new RuntimeException(e);
+            builder.relation(randomFrom(ShapeRelation.DISJOINT, ShapeRelation.WITHIN));
+            fail("Illegal combination of strategy and relation setting");
+        } catch (IllegalArgumentException e) {
+            // okay
+        }
+
+        try {
+            builder.relation(randomFrom(ShapeRelation.DISJOINT, ShapeRelation.WITHIN));
+            builder.strategy(SpatialStrategy.TERM);
+            fail("Illegal combination of strategy and relation setting");
+        } catch (IllegalArgumentException e) {
+            // okay
         }
     }