From 5ca9f3ff8f6c5a424902d031587943482ede34e9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christoph=20B=C3=BCscher?= Date: Tue, 17 Nov 2015 18:56:54 +0100 Subject: [PATCH 1/3] Geo: Make ShapeBuilders implement Writable We recently refactored the queries to make them parsable on the coordinating note and adding serialization and equals/hashCode capability to them. So far ShapeBuilders nested inside queries were still transported as a byte array that needs to be parsed later on the shard receiving the query. To be able to also serialize geo shapes this way, we also need to make all the implementations of ShapeBuilder implement Writable. This PR adds this to PointBuilder and also adds tests for serialization, equality and hashCode. --- .../common/geo/builders/CircleBuilder.java | 6 +- .../common/geo/builders/EnvelopeBuilder.java | 4 +- .../builders/GeometryCollectionBuilder.java | 2 +- .../geo/builders/LineStringBuilder.java | 2 +- .../geo/builders/MultiLineStringBuilder.java | 2 +- .../geo/builders/MultiPointBuilder.java | 2 +- .../geo/builders/MultiPolygonBuilder.java | 2 +- .../common/geo/builders/PointBuilder.java | 37 ++++- .../common/geo/builders/PolygonBuilder.java | 2 +- .../common/geo/builders/ShapeBuilder.java | 26 +++- .../AbstractShapeBuilderTestCase.java | 134 ++++++++++++++++++ .../geo/builders/PointBuilderTests.java | 38 +++++ 12 files changed, 242 insertions(+), 15 deletions(-) create mode 100644 core/src/test/java/org/elasticsearch/common/geo/builders/AbstractShapeBuilderTestCase.java create mode 100644 core/src/test/java/org/elasticsearch/common/geo/builders/PointBuilderTests.java diff --git a/core/src/main/java/org/elasticsearch/common/geo/builders/CircleBuilder.java b/core/src/main/java/org/elasticsearch/common/geo/builders/CircleBuilder.java index f1054e18663..ce1a51e657b 100644 --- a/core/src/main/java/org/elasticsearch/common/geo/builders/CircleBuilder.java +++ b/core/src/main/java/org/elasticsearch/common/geo/builders/CircleBuilder.java @@ -35,10 +35,10 @@ public class CircleBuilder extends ShapeBuilder { private DistanceUnit unit; private double radius; private Coordinate center; - + /** * Set the center of the circle - * + * * @param center coordinate of the circles center * @return this */ @@ -100,7 +100,7 @@ public class CircleBuilder extends ShapeBuilder { @Override public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { builder.startObject(); - builder.field(FIELD_TYPE, TYPE.shapename); + builder.field(FIELD_TYPE, TYPE.shapeName()); builder.field(FIELD_RADIUS, unit.toString(radius)); builder.field(FIELD_COORDINATES); toXContent(builder, center); diff --git a/core/src/main/java/org/elasticsearch/common/geo/builders/EnvelopeBuilder.java b/core/src/main/java/org/elasticsearch/common/geo/builders/EnvelopeBuilder.java index a296b3406ef..9acfdd2580e 100644 --- a/core/src/main/java/org/elasticsearch/common/geo/builders/EnvelopeBuilder.java +++ b/core/src/main/java/org/elasticsearch/common/geo/builders/EnvelopeBuilder.java @@ -27,7 +27,7 @@ import java.io.IOException; public class EnvelopeBuilder extends ShapeBuilder { - public static final GeoShapeType TYPE = GeoShapeType.ENVELOPE; + public static final GeoShapeType TYPE = GeoShapeType.ENVELOPE; protected Coordinate topLeft; protected Coordinate bottomRight; @@ -61,7 +61,7 @@ public class EnvelopeBuilder extends ShapeBuilder { @Override public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { builder.startObject(); - builder.field(FIELD_TYPE, TYPE.shapename); + builder.field(FIELD_TYPE, TYPE.shapeName()); builder.startArray(FIELD_COORDINATES); toXContent(builder, topLeft); toXContent(builder, bottomRight); diff --git a/core/src/main/java/org/elasticsearch/common/geo/builders/GeometryCollectionBuilder.java b/core/src/main/java/org/elasticsearch/common/geo/builders/GeometryCollectionBuilder.java index 57f3fc67b64..45397ed962f 100644 --- a/core/src/main/java/org/elasticsearch/common/geo/builders/GeometryCollectionBuilder.java +++ b/core/src/main/java/org/elasticsearch/common/geo/builders/GeometryCollectionBuilder.java @@ -102,7 +102,7 @@ public class GeometryCollectionBuilder extends ShapeBuilder { @Override public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { builder.startObject(); - builder.field(FIELD_TYPE, TYPE.shapename); + builder.field(FIELD_TYPE, TYPE.shapeName()); builder.startArray(FIELD_GEOMETRIES); for (ShapeBuilder shape : shapes) { shape.toXContent(builder, params); diff --git a/core/src/main/java/org/elasticsearch/common/geo/builders/LineStringBuilder.java b/core/src/main/java/org/elasticsearch/common/geo/builders/LineStringBuilder.java index 265efe11621..4bf84ea8f50 100644 --- a/core/src/main/java/org/elasticsearch/common/geo/builders/LineStringBuilder.java +++ b/core/src/main/java/org/elasticsearch/common/geo/builders/LineStringBuilder.java @@ -39,7 +39,7 @@ public class LineStringBuilder extends PointCollection { @Override public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { builder.startObject(); - builder.field(FIELD_TYPE, TYPE.shapename); + builder.field(FIELD_TYPE, TYPE.shapeName()); builder.field(FIELD_COORDINATES); coordinatesToXcontent(builder, false); builder.endObject(); diff --git a/core/src/main/java/org/elasticsearch/common/geo/builders/MultiLineStringBuilder.java b/core/src/main/java/org/elasticsearch/common/geo/builders/MultiLineStringBuilder.java index 10ad25c89e1..a004b90a2dc 100644 --- a/core/src/main/java/org/elasticsearch/common/geo/builders/MultiLineStringBuilder.java +++ b/core/src/main/java/org/elasticsearch/common/geo/builders/MultiLineStringBuilder.java @@ -57,7 +57,7 @@ public class MultiLineStringBuilder extends ShapeBuilder { @Override public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { builder.startObject(); - builder.field(FIELD_TYPE, TYPE.shapename); + builder.field(FIELD_TYPE, TYPE.shapeName()); builder.field(FIELD_COORDINATES); builder.startArray(); for(LineStringBuilder line : lines) { diff --git a/core/src/main/java/org/elasticsearch/common/geo/builders/MultiPointBuilder.java b/core/src/main/java/org/elasticsearch/common/geo/builders/MultiPointBuilder.java index d12baad70d9..8d5cfabdabb 100644 --- a/core/src/main/java/org/elasticsearch/common/geo/builders/MultiPointBuilder.java +++ b/core/src/main/java/org/elasticsearch/common/geo/builders/MultiPointBuilder.java @@ -37,7 +37,7 @@ public class MultiPointBuilder extends PointCollection { @Override public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { builder.startObject(); - builder.field(FIELD_TYPE, TYPE.shapename); + builder.field(FIELD_TYPE, TYPE.shapeName()); builder.field(FIELD_COORDINATES); super.coordinatesToXcontent(builder, false); builder.endObject(); diff --git a/core/src/main/java/org/elasticsearch/common/geo/builders/MultiPolygonBuilder.java b/core/src/main/java/org/elasticsearch/common/geo/builders/MultiPolygonBuilder.java index 0998cd2944b..7911ddff835 100644 --- a/core/src/main/java/org/elasticsearch/common/geo/builders/MultiPolygonBuilder.java +++ b/core/src/main/java/org/elasticsearch/common/geo/builders/MultiPolygonBuilder.java @@ -51,7 +51,7 @@ public class MultiPolygonBuilder extends ShapeBuilder { @Override public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { builder.startObject(); - builder.field(FIELD_TYPE, TYPE.shapename); + builder.field(FIELD_TYPE, TYPE.shapeName()); builder.startArray(FIELD_COORDINATES); for(PolygonBuilder polygon : polygons) { builder.startArray(); diff --git a/core/src/main/java/org/elasticsearch/common/geo/builders/PointBuilder.java b/core/src/main/java/org/elasticsearch/common/geo/builders/PointBuilder.java index 53c67387e91..39044728efd 100644 --- a/core/src/main/java/org/elasticsearch/common/geo/builders/PointBuilder.java +++ b/core/src/main/java/org/elasticsearch/common/geo/builders/PointBuilder.java @@ -20,7 +20,10 @@ package org.elasticsearch.common.geo.builders; import java.io.IOException; +import java.util.Objects; +import org.elasticsearch.common.io.stream.StreamInput; +import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.common.xcontent.XContentBuilder; import com.spatial4j.core.shape.Point; @@ -30,6 +33,8 @@ public class PointBuilder extends ShapeBuilder { public static final GeoShapeType TYPE = GeoShapeType.POINT; + public static final PointBuilder PROTOTYPE = new PointBuilder(); + private Coordinate coordinate; public PointBuilder coordinate(Coordinate coordinate) { @@ -48,10 +53,10 @@ public class PointBuilder extends ShapeBuilder { @Override public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { builder.startObject(); - builder.field(FIELD_TYPE, TYPE.shapename); + builder.field(FIELD_TYPE, TYPE.shapeName()); builder.field(FIELD_COORDINATES); toXContent(builder, coordinate); - return builder.endObject(); + return builder.endObject(); } @Override @@ -63,4 +68,32 @@ public class PointBuilder extends ShapeBuilder { public GeoShapeType type() { return TYPE; } + + @Override + public int hashCode() { + return Objects.hash(coordinate); + } + + @Override + public boolean equals(Object obj) { + if (this == obj) { + return true; + } + if (obj == null || getClass() != obj.getClass()) { + return false; + } + PointBuilder other = (PointBuilder) obj; + return Objects.equals(coordinate, other.coordinate); + } + + @Override + public void writeTo(StreamOutput out) throws IOException { + out.writeDouble(coordinate.x); + out.writeDouble(coordinate.y); + } + + @Override + public ShapeBuilder readFrom(StreamInput in) throws IOException { + return new PointBuilder().coordinate(new Coordinate(in.readDouble(), in.readDouble())); + } } diff --git a/core/src/main/java/org/elasticsearch/common/geo/builders/PolygonBuilder.java b/core/src/main/java/org/elasticsearch/common/geo/builders/PolygonBuilder.java index 4a406eb22b8..94d8fc049d8 100644 --- a/core/src/main/java/org/elasticsearch/common/geo/builders/PolygonBuilder.java +++ b/core/src/main/java/org/elasticsearch/common/geo/builders/PolygonBuilder.java @@ -172,7 +172,7 @@ public class PolygonBuilder extends ShapeBuilder { @Override public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { builder.startObject(); - builder.field(FIELD_TYPE, TYPE.shapename); + builder.field(FIELD_TYPE, TYPE.shapeName()); builder.startArray(FIELD_COORDINATES); coordinatesArray(builder, params); builder.endArray(); diff --git a/core/src/main/java/org/elasticsearch/common/geo/builders/ShapeBuilder.java b/core/src/main/java/org/elasticsearch/common/geo/builders/ShapeBuilder.java index 13237727173..e944af9166f 100644 --- a/core/src/main/java/org/elasticsearch/common/geo/builders/ShapeBuilder.java +++ b/core/src/main/java/org/elasticsearch/common/geo/builders/ShapeBuilder.java @@ -26,8 +26,12 @@ import com.spatial4j.core.shape.jts.JtsGeometry; import com.vividsolutions.jts.geom.Coordinate; import com.vividsolutions.jts.geom.Geometry; import com.vividsolutions.jts.geom.GeometryFactory; + import org.elasticsearch.ElasticsearchParseException; import org.elasticsearch.action.support.ToXContentToBytes; +import org.elasticsearch.common.io.stream.NamedWriteable; +import org.elasticsearch.common.io.stream.StreamInput; +import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.common.logging.ESLogger; import org.elasticsearch.common.logging.ESLoggerFactory; import org.elasticsearch.common.unit.DistanceUnit.Distance; @@ -43,7 +47,7 @@ import java.util.*; /** * Basic class for building GeoJSON shapes like Polygons, Linestrings, etc */ -public abstract class ShapeBuilder extends ToXContentToBytes { +public abstract class ShapeBuilder extends ToXContentToBytes implements NamedWriteable { protected static final ESLogger LOGGER = ESLoggerFactory.getLogger(ShapeBuilder.class.getName()); @@ -565,12 +569,16 @@ public abstract class ShapeBuilder extends ToXContentToBytes { ENVELOPE("envelope"), CIRCLE("circle"); - protected final String shapename; + private final String shapename; private GeoShapeType(String shapename) { this.shapename = shapename; } + protected String shapeName() { + return shapename; + } + public static GeoShapeType forName(String geoshapename) { String typename = geoshapename.toLowerCase(Locale.ROOT); for (GeoShapeType type : values()) { @@ -823,4 +831,18 @@ public abstract class ShapeBuilder extends ToXContentToBytes { return geometryCollection; } } + + @Override + public String getWriteableName() { + return type().shapename; + } + + @Override + public void writeTo(StreamOutput out) throws IOException { + } + + @Override + public ShapeBuilder readFrom(StreamInput in) throws IOException { + return null; + } } diff --git a/core/src/test/java/org/elasticsearch/common/geo/builders/AbstractShapeBuilderTestCase.java b/core/src/test/java/org/elasticsearch/common/geo/builders/AbstractShapeBuilderTestCase.java new file mode 100644 index 00000000000..0a60a3ccb31 --- /dev/null +++ b/core/src/test/java/org/elasticsearch/common/geo/builders/AbstractShapeBuilderTestCase.java @@ -0,0 +1,134 @@ +/* + * Licensed to Elasticsearch under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.elasticsearch.common.geo.builders; + +import org.elasticsearch.common.io.stream.BytesStreamOutput; +import org.elasticsearch.common.io.stream.NamedWriteableAwareStreamInput; +import org.elasticsearch.common.io.stream.NamedWriteableRegistry; +import org.elasticsearch.common.io.stream.StreamInput; +import org.elasticsearch.common.xcontent.*; +import org.elasticsearch.test.ESTestCase; +import org.junit.BeforeClass; + +import java.io.IOException; + +import static org.hamcrest.Matchers.*; + +public abstract class AbstractShapeBuilderTestCase extends ESTestCase { + + private static final int NUMBER_OF_TESTBUILDERS = 1; + private static final NamedWriteableRegistry namedWriteableRegistry = new NamedWriteableRegistry(); + + /** + * Setup for the whole base test class. + */ + @BeforeClass + public static void init() { + namedWriteableRegistry.registerPrototype(ShapeBuilder.class, PointBuilder.PROTOTYPE); + } + + /** + * Create the shape that under test + */ + protected abstract SB createTestShapeBuilder(); + + /** + * mutate the given query so the returned query is different + */ + protected abstract SB mutate(SB original); + + /** + * Generic test that creates new shape from a random test shape and checks both for equality + */ + public void testFromXContent() throws IOException { + for (int runs = 0; runs < NUMBER_OF_TESTBUILDERS; runs++) { + SB testShape = createTestShapeBuilder(); + XContentBuilder builder = toXContent(testShape, randomFrom(XContentType.values())); + + XContentParser shapeParser = XContentHelper.createParser(builder.bytes()); + shapeParser.nextToken(); + ShapeBuilder parsedShape = ShapeBuilder.parse(shapeParser); + assertNotSame(testShape, parsedShape); + assertEquals(testShape, parsedShape); + assertEquals(testShape.hashCode(), parsedShape.hashCode()); + } + } + + protected static XContentBuilder toXContent(ShapeBuilder shape, XContentType contentType) throws IOException { + XContentBuilder builder = XContentFactory.contentBuilder(contentType); + if (randomBoolean()) { + builder.prettyPrint(); + } + return shape.toXContent(builder, ToXContent.EMPTY_PARAMS); + } + + /** + * Test serialization and deserialization of the test shape. + */ + public void testSerialization() throws IOException { + for (int runs = 0; runs < NUMBER_OF_TESTBUILDERS; runs++) { + SB testShape = createTestShapeBuilder(); + SB deserializedShape = copyShape(testShape); + assertEquals(deserializedShape, testShape); + assertEquals(deserializedShape.hashCode(), testShape.hashCode()); + assertNotSame(deserializedShape, testShape); + } + } + + public void testEqualsAndHashcode() throws IOException { + for (int runs = 0; runs < NUMBER_OF_TESTBUILDERS; runs++) { + SB firstShape = createTestShapeBuilder(); + assertFalse("shape is equal to null", firstShape.equals(null)); + assertFalse("shape is equal to incompatible type", firstShape.equals("")); + assertTrue("shape is not equal to self", firstShape.equals(firstShape)); + assertThat("same shape's hashcode returns different values if called multiple times", firstShape.hashCode(), + equalTo(firstShape.hashCode())); + assertThat("different shapes should not be equal", mutate(firstShape), not(equalTo(firstShape))); + assertThat("different shapes should have different hashcode", mutate(firstShape).hashCode(), not(equalTo(firstShape.hashCode()))); + + SB secondShape = copyShape(firstShape); + assertTrue("shape is not equal to self", secondShape.equals(secondShape)); + assertTrue("shape is not equal to its copy", firstShape.equals(secondShape)); + assertTrue("equals is not symmetric", secondShape.equals(firstShape)); + assertThat("shape copy's hashcode is different from original hashcode", secondShape.hashCode(), equalTo(firstShape.hashCode())); + + SB thirdShape = copyShape(secondShape); + assertTrue("shape is not equal to self", thirdShape.equals(thirdShape)); + assertTrue("shape is not equal to its copy", secondShape.equals(thirdShape)); + assertThat("shape copy's hashcode is different from original hashcode", secondShape.hashCode(), equalTo(thirdShape.hashCode())); + assertTrue("equals is not transitive", firstShape.equals(thirdShape)); + assertThat("shape copy's hashcode is different from original hashcode", firstShape.hashCode(), equalTo(thirdShape.hashCode())); + assertTrue("equals is not symmetric", thirdShape.equals(secondShape)); + assertTrue("equals is not symmetric", thirdShape.equals(firstShape)); + } + } + + private SB copyShape(SB original) throws IOException { + try (BytesStreamOutput output = new BytesStreamOutput()) { + original.writeTo(output); + try (StreamInput in = new NamedWriteableAwareStreamInput(StreamInput.wrap(output.bytes()), namedWriteableRegistry)) { + ShapeBuilder prototype = (ShapeBuilder) namedWriteableRegistry.getPrototype(ShapeBuilder.class, original.getWriteableName()); + @SuppressWarnings("unchecked") + SB copy = (SB) prototype.readFrom(in); + return copy; + } + } + } +} diff --git a/core/src/test/java/org/elasticsearch/common/geo/builders/PointBuilderTests.java b/core/src/test/java/org/elasticsearch/common/geo/builders/PointBuilderTests.java new file mode 100644 index 00000000000..1e94a1bab3a --- /dev/null +++ b/core/src/test/java/org/elasticsearch/common/geo/builders/PointBuilderTests.java @@ -0,0 +1,38 @@ +/* + * Licensed to Elasticsearch under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.elasticsearch.common.geo.builders; + +import com.vividsolutions.jts.geom.Coordinate; + +import org.elasticsearch.test.geo.RandomShapeGenerator; +import org.elasticsearch.test.geo.RandomShapeGenerator.ShapeType; + +public class PointBuilderTests extends AbstractShapeBuilderTestCase { + + @Override + protected PointBuilder createTestShapeBuilder() { + return (PointBuilder) RandomShapeGenerator.createShape(getRandom(), ShapeType.POINT); + } + + @Override + protected PointBuilder mutate(PointBuilder original) { + return new PointBuilder().coordinate(new Coordinate(original.longitude()/2, original.latitude()/2)); + } +} From e25f7042b91a22d75560f999fc60c0be710a9f5c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christoph=20B=C3=BCscher?= Date: Tue, 17 Nov 2015 20:23:03 +0100 Subject: [PATCH 2/3] Making CircleBuilder writable and adding equals/hashcode --- .../common/geo/builders/CircleBuilder.java | 58 +++++++++++++++++++ .../common/geo/builders/PointBuilder.java | 5 +- .../common/geo/builders/ShapeBuilder.java | 9 +++ .../AbstractShapeBuilderTestCase.java | 9 ++- .../geo/builders/CirlceBuilderTests.java | 58 +++++++++++++++++++ .../common/unit/DistanceUnitTests.java | 16 +++++ 6 files changed, 149 insertions(+), 6 deletions(-) create mode 100644 core/src/test/java/org/elasticsearch/common/geo/builders/CirlceBuilderTests.java diff --git a/core/src/main/java/org/elasticsearch/common/geo/builders/CircleBuilder.java b/core/src/main/java/org/elasticsearch/common/geo/builders/CircleBuilder.java index ce1a51e657b..c65b278c3c1 100644 --- a/core/src/main/java/org/elasticsearch/common/geo/builders/CircleBuilder.java +++ b/core/src/main/java/org/elasticsearch/common/geo/builders/CircleBuilder.java @@ -21,17 +21,23 @@ package org.elasticsearch.common.geo.builders; import com.spatial4j.core.shape.Circle; import com.vividsolutions.jts.geom.Coordinate; + +import org.elasticsearch.common.io.stream.StreamInput; +import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.common.unit.DistanceUnit; import org.elasticsearch.common.unit.DistanceUnit.Distance; import org.elasticsearch.common.xcontent.XContentBuilder; import java.io.IOException; +import java.util.Objects; public class CircleBuilder extends ShapeBuilder { public static final String FIELD_RADIUS = "radius"; public static final GeoShapeType TYPE = GeoShapeType.CIRCLE; + public static final CircleBuilder PROTOTYPE = new CircleBuilder(); + private DistanceUnit unit; private double radius; private Coordinate center; @@ -57,6 +63,13 @@ public class CircleBuilder extends ShapeBuilder { return center(new Coordinate(lon, lat)); } + /** + * Get the center of the circle + */ + public Coordinate center() { + return center; + } + /** * Set the radius of the circle. The String value will be parsed by {@link DistanceUnit} * @param radius Value and unit of the circle combined in a string @@ -97,6 +110,20 @@ public class CircleBuilder extends ShapeBuilder { return this; } + /** + * Get the radius of the circle without unit + */ + public double radius() { + return this.radius; + } + + /** + * Get the radius unit of the circle + */ + public DistanceUnit unit() { + return this.unit; + } + @Override public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { builder.startObject(); @@ -116,4 +143,35 @@ public class CircleBuilder extends ShapeBuilder { public GeoShapeType type() { return TYPE; } + + @Override + public int hashCode() { + return Objects.hash(center, radius, unit.ordinal()); + } + + @Override + public boolean equals(Object obj) { + if (this == obj) { + return true; + } + if (obj == null || getClass() != obj.getClass()) { + return false; + } + CircleBuilder other = (CircleBuilder) obj; + return Objects.equals(center, other.center) && + Objects.equals(radius, other.radius) && + Objects.equals(unit.ordinal(), other.unit.ordinal()); + } + + @Override + public void writeTo(StreamOutput out) throws IOException { + writeCoordinateTo(center, out); + out.writeDouble(radius); + DistanceUnit.writeDistanceUnit(out, unit); + } + + @Override + public ShapeBuilder readFrom(StreamInput in) throws IOException { + return new CircleBuilder().center(readCoordinateFrom(in)).radius(in.readDouble(), DistanceUnit.readDistanceUnit(in)); + } } diff --git a/core/src/main/java/org/elasticsearch/common/geo/builders/PointBuilder.java b/core/src/main/java/org/elasticsearch/common/geo/builders/PointBuilder.java index 39044728efd..127124e599c 100644 --- a/core/src/main/java/org/elasticsearch/common/geo/builders/PointBuilder.java +++ b/core/src/main/java/org/elasticsearch/common/geo/builders/PointBuilder.java @@ -88,12 +88,11 @@ public class PointBuilder extends ShapeBuilder { @Override public void writeTo(StreamOutput out) throws IOException { - out.writeDouble(coordinate.x); - out.writeDouble(coordinate.y); + writeCoordinateTo(coordinate, out); } @Override public ShapeBuilder readFrom(StreamInput in) throws IOException { - return new PointBuilder().coordinate(new Coordinate(in.readDouble(), in.readDouble())); + return new PointBuilder().coordinate(readCoordinateFrom(in)); } } diff --git a/core/src/main/java/org/elasticsearch/common/geo/builders/ShapeBuilder.java b/core/src/main/java/org/elasticsearch/common/geo/builders/ShapeBuilder.java index e944af9166f..2c9f5b4f9bc 100644 --- a/core/src/main/java/org/elasticsearch/common/geo/builders/ShapeBuilder.java +++ b/core/src/main/java/org/elasticsearch/common/geo/builders/ShapeBuilder.java @@ -177,6 +177,15 @@ public abstract class ShapeBuilder extends ToXContentToBytes implements NamedWri return builder.startArray().value(coordinate.x).value(coordinate.y).endArray(); } + protected static void writeCoordinateTo(Coordinate coordinate, StreamOutput out) throws IOException { + out.writeDouble(coordinate.x); + out.writeDouble(coordinate.y); + } + + protected Coordinate readCoordinateFrom(StreamInput in) throws IOException { + return new Coordinate(in.readDouble(), in.readDouble()); + } + public static Orientation orientationFromString(String orientation) { orientation = orientation.toLowerCase(Locale.ROOT); switch (orientation) { diff --git a/core/src/test/java/org/elasticsearch/common/geo/builders/AbstractShapeBuilderTestCase.java b/core/src/test/java/org/elasticsearch/common/geo/builders/AbstractShapeBuilderTestCase.java index 0a60a3ccb31..f585215f9bd 100644 --- a/core/src/test/java/org/elasticsearch/common/geo/builders/AbstractShapeBuilderTestCase.java +++ b/core/src/test/java/org/elasticsearch/common/geo/builders/AbstractShapeBuilderTestCase.java @@ -33,7 +33,7 @@ import static org.hamcrest.Matchers.*; public abstract class AbstractShapeBuilderTestCase extends ESTestCase { - private static final int NUMBER_OF_TESTBUILDERS = 1; + private static final int NUMBER_OF_TESTBUILDERS = 20; private static final NamedWriteableRegistry namedWriteableRegistry = new NamedWriteableRegistry(); /** @@ -42,6 +42,7 @@ public abstract class AbstractShapeBuilderTestCase exte @BeforeClass public static void init() { namedWriteableRegistry.registerPrototype(ShapeBuilder.class, PointBuilder.PROTOTYPE); + namedWriteableRegistry.registerPrototype(ShapeBuilder.class, CircleBuilder.PROTOTYPE); } /** @@ -52,7 +53,7 @@ public abstract class AbstractShapeBuilderTestCase exte /** * mutate the given query so the returned query is different */ - protected abstract SB mutate(SB original); + protected abstract SB mutate(SB original) throws IOException; /** * Generic test that creates new shape from a random test shape and checks both for equality @@ -61,8 +62,10 @@ public abstract class AbstractShapeBuilderTestCase exte for (int runs = 0; runs < NUMBER_OF_TESTBUILDERS; runs++) { SB testShape = createTestShapeBuilder(); XContentBuilder builder = toXContent(testShape, randomFrom(XContentType.values())); + builder = toXContent(testShape, randomFrom(XContentType.values())); XContentParser shapeParser = XContentHelper.createParser(builder.bytes()); + XContentHelper.createParser(builder.bytes()); shapeParser.nextToken(); ShapeBuilder parsedShape = ShapeBuilder.parse(shapeParser); assertNotSame(testShape, parsedShape); @@ -120,7 +123,7 @@ public abstract class AbstractShapeBuilderTestCase exte } } - private SB copyShape(SB original) throws IOException { + protected SB copyShape(SB original) throws IOException { try (BytesStreamOutput output = new BytesStreamOutput()) { original.writeTo(output); try (StreamInput in = new NamedWriteableAwareStreamInput(StreamInput.wrap(output.bytes()), namedWriteableRegistry)) { diff --git a/core/src/test/java/org/elasticsearch/common/geo/builders/CirlceBuilderTests.java b/core/src/test/java/org/elasticsearch/common/geo/builders/CirlceBuilderTests.java new file mode 100644 index 00000000000..6b102b87b2c --- /dev/null +++ b/core/src/test/java/org/elasticsearch/common/geo/builders/CirlceBuilderTests.java @@ -0,0 +1,58 @@ +/* + * Licensed to Elasticsearch under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.elasticsearch.common.geo.builders; + +import com.vividsolutions.jts.geom.Coordinate; + +import org.elasticsearch.common.unit.DistanceUnit; + +import java.io.IOException; + +public class CirlceBuilderTests extends AbstractShapeBuilderTestCase { + + @Override + protected CircleBuilder createTestShapeBuilder() { + double centerX = randomDoubleBetween(-180, 180, false); + double centerY = randomDoubleBetween(-90, 90, false); + return new CircleBuilder() + .center(new Coordinate(centerX, centerY)) + .radius(randomDoubleBetween(0.1, 10.0, false), randomFrom(DistanceUnit.values())); + } + + @Override + protected CircleBuilder mutate(CircleBuilder original) throws IOException { + CircleBuilder mutation = copyShape(original); + double radius = original.radius(); + DistanceUnit unit = original.unit(); + + if (randomBoolean()) { + mutation.center(new Coordinate(original.center().x/2, original.center().y/2)); + } else if (randomBoolean()) { + radius = radius/2; + } else { + DistanceUnit newRandom = unit; + while (newRandom == unit) { + newRandom = randomFrom(DistanceUnit.values()); + }; + unit = newRandom; + } + return mutation.radius(radius, unit); + } +} diff --git a/core/src/test/java/org/elasticsearch/common/unit/DistanceUnitTests.java b/core/src/test/java/org/elasticsearch/common/unit/DistanceUnitTests.java index 1010d2a5e8c..25c3a136271 100644 --- a/core/src/test/java/org/elasticsearch/common/unit/DistanceUnitTests.java +++ b/core/src/test/java/org/elasticsearch/common/unit/DistanceUnitTests.java @@ -57,4 +57,20 @@ public class DistanceUnitTests extends ESTestCase { assertThat("Value can be parsed from '" + testValue + unit.toString() + "'", DistanceUnit.Distance.parseDistance(unit.toString(testValue)).value, equalTo(testValue)); } } + + /** + * This test ensures that we are aware of accidental reordering in the distance unit ordinals, + * since equality in e.g. CircleShapeBuilder, hashCode and serialization rely on them + */ + public void testDistanceUnitNames() { + assertEquals(0, DistanceUnit.INCH.ordinal()); + assertEquals(1, DistanceUnit.YARD.ordinal()); + assertEquals(2, DistanceUnit.FEET.ordinal()); + assertEquals(3, DistanceUnit.KILOMETERS.ordinal()); + assertEquals(4, DistanceUnit.NAUTICALMILES.ordinal()); + assertEquals(5, DistanceUnit.MILLIMETERS.ordinal()); + assertEquals(6, DistanceUnit.CENTIMETERS.ordinal()); + assertEquals(7, DistanceUnit.MILES.ordinal()); + assertEquals(8, DistanceUnit.METERS.ordinal()); + } } From 1f94448e922c90205047abfd0f2a98b75c3d1585 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christoph=20B=C3=BCscher?= Date: Tue, 17 Nov 2015 21:42:43 +0100 Subject: [PATCH 3/3] Making EnvelopeBuilder writable and adding equals/hashCode --- .../common/geo/builders/CircleBuilder.java | 6 +- .../common/geo/builders/EnvelopeBuilder.java | 41 ++++++++++++ .../common/geo/builders/PointBuilder.java | 2 +- .../common/geo/builders/ShapeBuilder.java | 4 +- .../AbstractShapeBuilderTestCase.java | 43 +++++++----- .../geo/builders/EnvelopeBuilderTests.java | 66 +++++++++++++++++++ 6 files changed, 140 insertions(+), 22 deletions(-) create mode 100644 core/src/test/java/org/elasticsearch/common/geo/builders/EnvelopeBuilderTests.java diff --git a/core/src/main/java/org/elasticsearch/common/geo/builders/CircleBuilder.java b/core/src/main/java/org/elasticsearch/common/geo/builders/CircleBuilder.java index c65b278c3c1..5f11d12a4bf 100644 --- a/core/src/main/java/org/elasticsearch/common/geo/builders/CircleBuilder.java +++ b/core/src/main/java/org/elasticsearch/common/geo/builders/CircleBuilder.java @@ -171,7 +171,9 @@ public class CircleBuilder extends ShapeBuilder { } @Override - public ShapeBuilder readFrom(StreamInput in) throws IOException { - return new CircleBuilder().center(readCoordinateFrom(in)).radius(in.readDouble(), DistanceUnit.readDistanceUnit(in)); + public CircleBuilder readFrom(StreamInput in) throws IOException { + return new CircleBuilder() + .center(readCoordinateFrom(in)) + .radius(in.readDouble(), DistanceUnit.readDistanceUnit(in)); } } diff --git a/core/src/main/java/org/elasticsearch/common/geo/builders/EnvelopeBuilder.java b/core/src/main/java/org/elasticsearch/common/geo/builders/EnvelopeBuilder.java index 9acfdd2580e..62f29d2bad7 100644 --- a/core/src/main/java/org/elasticsearch/common/geo/builders/EnvelopeBuilder.java +++ b/core/src/main/java/org/elasticsearch/common/geo/builders/EnvelopeBuilder.java @@ -21,13 +21,19 @@ package org.elasticsearch.common.geo.builders; import com.spatial4j.core.shape.Rectangle; import com.vividsolutions.jts.geom.Coordinate; + +import org.elasticsearch.common.io.stream.StreamInput; +import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.common.xcontent.XContentBuilder; import java.io.IOException; +import java.util.Locale; +import java.util.Objects; public class EnvelopeBuilder extends ShapeBuilder { public static final GeoShapeType TYPE = GeoShapeType.ENVELOPE; + public static final EnvelopeBuilder PROTOTYPE = new EnvelopeBuilder(); protected Coordinate topLeft; protected Coordinate bottomRight; @@ -62,6 +68,7 @@ public class EnvelopeBuilder extends ShapeBuilder { public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { builder.startObject(); builder.field(FIELD_TYPE, TYPE.shapeName()); + builder.field(FIELD_ORIENTATION, orientation.name().toLowerCase(Locale.ROOT)); builder.startArray(FIELD_COORDINATES); toXContent(builder, topLeft); toXContent(builder, bottomRight); @@ -78,4 +85,38 @@ public class EnvelopeBuilder extends ShapeBuilder { public GeoShapeType type() { return TYPE; } + + @Override + public int hashCode() { + return Objects.hash(orientation, topLeft, bottomRight); + } + + @Override + public boolean equals(Object obj) { + if (this == obj) { + return true; + } + if (obj == null || getClass() != obj.getClass()) { + return false; + } + EnvelopeBuilder other = (EnvelopeBuilder) obj; + return Objects.equals(orientation, other.orientation) && + Objects.equals(topLeft, other.topLeft) && + Objects.equals(bottomRight, other.bottomRight); + } + + @Override + public void writeTo(StreamOutput out) throws IOException { + out.writeBoolean(orientation == Orientation.RIGHT); + writeCoordinateTo(topLeft, out); + writeCoordinateTo(bottomRight, out); + } + + @Override + public EnvelopeBuilder readFrom(StreamInput in) throws IOException { + Orientation orientation = in.readBoolean() ? Orientation.RIGHT : Orientation.LEFT; + return new EnvelopeBuilder(orientation) + .topLeft(readCoordinateFrom(in)) + .bottomRight(readCoordinateFrom(in)); + } } diff --git a/core/src/main/java/org/elasticsearch/common/geo/builders/PointBuilder.java b/core/src/main/java/org/elasticsearch/common/geo/builders/PointBuilder.java index 127124e599c..d6d62c28b8c 100644 --- a/core/src/main/java/org/elasticsearch/common/geo/builders/PointBuilder.java +++ b/core/src/main/java/org/elasticsearch/common/geo/builders/PointBuilder.java @@ -92,7 +92,7 @@ public class PointBuilder extends ShapeBuilder { } @Override - public ShapeBuilder readFrom(StreamInput in) throws IOException { + public PointBuilder readFrom(StreamInput in) throws IOException { return new PointBuilder().coordinate(readCoordinateFrom(in)); } } diff --git a/core/src/main/java/org/elasticsearch/common/geo/builders/ShapeBuilder.java b/core/src/main/java/org/elasticsearch/common/geo/builders/ShapeBuilder.java index 2c9f5b4f9bc..7f153a9197f 100644 --- a/core/src/main/java/org/elasticsearch/common/geo/builders/ShapeBuilder.java +++ b/core/src/main/java/org/elasticsearch/common/geo/builders/ShapeBuilder.java @@ -843,13 +843,15 @@ public abstract class ShapeBuilder extends ToXContentToBytes implements NamedWri @Override public String getWriteableName() { - return type().shapename; + return type().shapeName(); } + // NORELEASE this should be deleted as soon as all shape builders implement writable @Override public void writeTo(StreamOutput out) throws IOException { } + // NORELEASE this should be deleted as soon as all shape builders implement writable @Override public ShapeBuilder readFrom(StreamInput in) throws IOException { return null; diff --git a/core/src/test/java/org/elasticsearch/common/geo/builders/AbstractShapeBuilderTestCase.java b/core/src/test/java/org/elasticsearch/common/geo/builders/AbstractShapeBuilderTestCase.java index f585215f9bd..d1f24bfb7d9 100644 --- a/core/src/test/java/org/elasticsearch/common/geo/builders/AbstractShapeBuilderTestCase.java +++ b/core/src/test/java/org/elasticsearch/common/geo/builders/AbstractShapeBuilderTestCase.java @@ -25,6 +25,7 @@ import org.elasticsearch.common.io.stream.NamedWriteableRegistry; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.xcontent.*; import org.elasticsearch.test.ESTestCase; +import org.junit.AfterClass; import org.junit.BeforeClass; import java.io.IOException; @@ -34,36 +35,47 @@ import static org.hamcrest.Matchers.*; public abstract class AbstractShapeBuilderTestCase extends ESTestCase { private static final int NUMBER_OF_TESTBUILDERS = 20; - private static final NamedWriteableRegistry namedWriteableRegistry = new NamedWriteableRegistry(); + private static NamedWriteableRegistry namedWriteableRegistry; /** - * Setup for the whole base test class. + * setup for the whole base test class */ @BeforeClass public static void init() { - namedWriteableRegistry.registerPrototype(ShapeBuilder.class, PointBuilder.PROTOTYPE); - namedWriteableRegistry.registerPrototype(ShapeBuilder.class, CircleBuilder.PROTOTYPE); + if (namedWriteableRegistry == null) { + namedWriteableRegistry = new NamedWriteableRegistry(); + namedWriteableRegistry.registerPrototype(ShapeBuilder.class, PointBuilder.PROTOTYPE); + namedWriteableRegistry.registerPrototype(ShapeBuilder.class, CircleBuilder.PROTOTYPE); + namedWriteableRegistry.registerPrototype(ShapeBuilder.class, EnvelopeBuilder.PROTOTYPE); + } + } + + @AfterClass + public static void afterClass() throws Exception { + namedWriteableRegistry = null; } /** - * Create the shape that under test + * create random shape that is put under test */ protected abstract SB createTestShapeBuilder(); /** - * mutate the given query so the returned query is different + * mutate the given shape so the returned shape is different */ protected abstract SB mutate(SB original) throws IOException; /** - * Generic test that creates new shape from a random test shape and checks both for equality + * Test that creates new shape from a random test shape and checks both for equality */ public void testFromXContent() throws IOException { for (int runs = 0; runs < NUMBER_OF_TESTBUILDERS; runs++) { SB testShape = createTestShapeBuilder(); - XContentBuilder builder = toXContent(testShape, randomFrom(XContentType.values())); - builder = toXContent(testShape, randomFrom(XContentType.values())); - + XContentBuilder contentBuilder = XContentFactory.contentBuilder(randomFrom(XContentType.values())); + if (randomBoolean()) { + contentBuilder.prettyPrint(); + } + XContentBuilder builder = testShape.toXContent(contentBuilder, ToXContent.EMPTY_PARAMS); XContentParser shapeParser = XContentHelper.createParser(builder.bytes()); XContentHelper.createParser(builder.bytes()); shapeParser.nextToken(); @@ -74,14 +86,6 @@ public abstract class AbstractShapeBuilderTestCase exte } } - protected static XContentBuilder toXContent(ShapeBuilder shape, XContentType contentType) throws IOException { - XContentBuilder builder = XContentFactory.contentBuilder(contentType); - if (randomBoolean()) { - builder.prettyPrint(); - } - return shape.toXContent(builder, ToXContent.EMPTY_PARAMS); - } - /** * Test serialization and deserialization of the test shape. */ @@ -95,6 +99,9 @@ public abstract class AbstractShapeBuilderTestCase exte } } + /** + * Test equality and hashCode properties + */ public void testEqualsAndHashcode() throws IOException { for (int runs = 0; runs < NUMBER_OF_TESTBUILDERS; runs++) { SB firstShape = createTestShapeBuilder(); diff --git a/core/src/test/java/org/elasticsearch/common/geo/builders/EnvelopeBuilderTests.java b/core/src/test/java/org/elasticsearch/common/geo/builders/EnvelopeBuilderTests.java new file mode 100644 index 00000000000..e6f3db2f8af --- /dev/null +++ b/core/src/test/java/org/elasticsearch/common/geo/builders/EnvelopeBuilderTests.java @@ -0,0 +1,66 @@ +/* + * Licensed to Elasticsearch under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.elasticsearch.common.geo.builders; + +import com.spatial4j.core.shape.Rectangle; +import com.vividsolutions.jts.geom.Coordinate; + +import org.elasticsearch.common.geo.builders.ShapeBuilder.Orientation; +import org.elasticsearch.test.geo.RandomShapeGenerator; + +import java.io.IOException; + +public class EnvelopeBuilderTests extends AbstractShapeBuilderTestCase { + + @Override + protected EnvelopeBuilder createTestShapeBuilder() { + EnvelopeBuilder envelope = new EnvelopeBuilder(randomFrom(Orientation.values())); + Rectangle box = RandomShapeGenerator.xRandomRectangle(getRandom(), RandomShapeGenerator.xRandomPoint(getRandom())); + envelope.topLeft(box.getMinX(), box.getMaxY()) + .bottomRight(box.getMaxX(), box.getMinY()); + return envelope; + } + + @Override + protected EnvelopeBuilder mutate(EnvelopeBuilder original) throws IOException { + EnvelopeBuilder mutation = copyShape(original); + if (randomBoolean()) { + // toggle orientation + mutation.orientation = (original.orientation == Orientation.LEFT ? Orientation.RIGHT : Orientation.LEFT); + } else { + // move one corner to the middle of original + switch (randomIntBetween(0, 3)) { + case 0: + mutation.topLeft(new Coordinate(randomDoubleBetween(-180.0, original.bottomRight.x, true), original.topLeft.y)); + break; + case 1: + mutation.topLeft(new Coordinate(original.topLeft.x, randomDoubleBetween(original.bottomRight.y, 90.0, true))); + break; + case 2: + mutation.bottomRight(new Coordinate(randomDoubleBetween(original.topLeft.x, 180.0, true), original.bottomRight.y)); + break; + case 3: + mutation.bottomRight(new Coordinate(original.bottomRight.x, randomDoubleBetween(-90.0, original.topLeft.y, true))); + break; + } + } + return mutation; + } +}