honeymoose
/
OpenSearch
mirror of https://github.com/honeymoose/OpenSearch.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Query Refactoring: validate GeoShapeQueryBuilder strategy and relation 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 #10217
This commit is contained in:
Christoph Büscher 2015-09-22 14:44:41 +02:00
parent a50a0da183
commit 74d641181e
2 changed files with 25 additions and 34 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

19
core/src/main/java/org/elasticsearch/index/query/GeoShapeQueryBuilder.java
View File

@ -182,6 +182,10 @@ public class GeoShapeQueryBuilder extends AbstractQueryBuilder<GeoShapeQueryBuil
* @return this * @return this
*/ */
public GeoShapeQueryBuilder strategy(SpatialStrategy strategy) { 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; this.strategy = strategy;
return this; return this;
} }
@ -240,6 +244,10 @@ public class GeoShapeQueryBuilder extends AbstractQueryBuilder<GeoShapeQueryBuil
if (relation == null) { if (relation == null) {
throw new IllegalArgumentException("No Shape Relation defined"); 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; this.relation = relation;
return this; return this;
} }
@ -251,17 +259,6 @@ public class GeoShapeQueryBuilder extends AbstractQueryBuilder<GeoShapeQueryBuil
return 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 @Override
protected Query doToQuery(QueryShardContext context) throws IOException { protected Query doToQuery(QueryShardContext context) throws IOException {
ShapeBuilder shape; ShapeBuilder shape;

40
core/src/test/java/org/elasticsearch/index/query/GeoShapeQueryBuilderTests.java
View File

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