-
Notifications
You must be signed in to change notification settings - Fork 885
CASSJAVA-108 Update org.json (and very likely ESRI) dependency #2052
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: 4.x
Are you sure you want to change the base?
Changes from all commits
9869638
33ac076
657acc1
1e652b3
8977df0
5421b18
c84a4a3
a22b6f8
6deade2
06fefa4
1662665
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,6 +23,9 @@ | |
import com.datastax.dse.driver.api.core.data.geometry.LineString; | ||
import com.datastax.dse.driver.api.core.data.geometry.Point; | ||
import com.esri.core.geometry.ogc.OGCLineString; | ||
import com.fasterxml.jackson.databind.JsonNode; | ||
import com.fasterxml.jackson.databind.ObjectMapper; | ||
import com.fasterxml.jackson.databind.node.ArrayNode; | ||
import java.nio.ByteBuffer; | ||
import java.nio.ByteOrder; | ||
import org.junit.Test; | ||
|
@@ -101,8 +104,26 @@ public void should_parse_valid_geo_json() { | |
} | ||
|
||
@Test | ||
public void should_convert_to_geo_json() { | ||
assertThat(lineString.asGeoJson()).isEqualTo(json); | ||
public void should_convert_to_geo_json() throws Exception { | ||
|
||
ObjectMapper mapper = new ObjectMapper(); | ||
JsonNode root = mapper.readTree(lineString.asGeoJson()); | ||
assertThat(root.get("type").toString()).isEqualTo("\"LineString\""); | ||
|
||
double expected[][] = {{30.0, 10.0}, {10.0, 30.0}, {40.0, 40.0}}; | ||
JsonNode coordinatesNode = root.get("coordinates"); | ||
assertThat(coordinatesNode.isArray()).isTrue(); | ||
ArrayNode coordinatesArray = (ArrayNode) coordinatesNode; | ||
assertThat(coordinatesArray.size()).isEqualTo(3); | ||
for (int i = 0; i < expected.length; ++i) { | ||
|
||
JsonNode elemNode = coordinatesArray.get(i); | ||
assertThat(elemNode.isArray()).isTrue(); | ||
ArrayNode elemArray = (ArrayNode) elemNode; | ||
assertThat(elemArray.size()).isEqualTo(2); | ||
assertThat(elemArray.get(0).asDouble()).isEqualTo(expected[i][0]); | ||
assertThat(elemArray.get(1).asDouble()).isEqualTo(expected[i][1]); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps the most contentious change in the whole PR. With this code we no longer concern ourselves with the format of the GeoJSON returned by ESRI... we only seek to confirm that we can find the fields we expect at the places we expect them. Any additional content or formatting isn't our concern. My rationale is that this is really the heart of the matter. We don't claim to follow a specific GeoJSON spec or that any responses will contain only some properties but not any others. With that in mind our objective should be to make sure we don't see any regressions on the props customers actually expect, specifically the "type" and "coordinates" fields. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note: without this change you get failures like the following:
|
||
} | ||
|
||
@Test | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -24,6 +24,9 @@ | |
import com.datastax.dse.driver.api.core.data.geometry.Point; | ||
import com.datastax.dse.driver.api.core.data.geometry.Polygon; | ||
import com.esri.core.geometry.ogc.OGCPolygon; | ||
import com.fasterxml.jackson.databind.JsonNode; | ||
import com.fasterxml.jackson.databind.ObjectMapper; | ||
import com.fasterxml.jackson.databind.node.ArrayNode; | ||
import java.nio.ByteBuffer; | ||
import java.nio.ByteOrder; | ||
import org.junit.Test; | ||
|
@@ -109,8 +112,44 @@ public void should_parse_valid_geo_json() { | |
} | ||
|
||
@Test | ||
public void should_convert_to_geo_json() { | ||
assertThat(polygon.asGeoJson()).isEqualTo(json); | ||
public void should_convert_to_geo_json() throws Exception { | ||
|
||
ObjectMapper mapper = new ObjectMapper(); | ||
JsonNode root = mapper.readTree(polygon.asGeoJson()); | ||
assertThat(root.get("type").toString()).isEqualTo("\"Polygon\""); | ||
|
||
/* | ||
Note that the order of values in expected differs from the order of insertion when creating | ||
the Polygon. OGC expects the "exterior" ring of the polygon to be listed in counter-clockwise | ||
order... and that's what this sequence represents (draw it out on a graph if you don't believe me). | ||
|
||
Weirdly this is the opposite of the order used for this test when we were using ESRI 1.2.1. | ||
That fact (combined with the fact that only ESRI classes are used for serialization here) makes me | ||
think that the earlier version was just doing it wrong... or at least doing it in a way that | ||
didn't agree with the spec. Either way it is clearly correct that we should go counter-clockwise... | ||
so that's what we're doing. | ||
*/ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Part of the explanation here might be that GeoJSON appears to be somewhat looser in ordering the points in a path. There's actually an old ticket for DSE asking for GeoJSON encoding to preserve the OGC ordering even though it isn't required to do so. That ticket was never implemented... which presumably explains why this test looked the way it did (although I'm speculating a bit there). Either way it is the case that this test (a) uses only ESRI classes locally and (b) is serializing and deserializing data internally (i.e. no interaction with anything outside of ESRI is happening here). It appears that the GeoJSON impl in the new version of esri-geometry-api is more strict about ordering of points... which presumably explains why we've started seeing failures since the upgrade. |
||
double expected[][] = {{30.0, 10.0}, {40.0, 40.0}, {20.0, 40.0}, {10.0, 20.0}, {30.0, 10.0}}; | ||
JsonNode coordinatesNode = root.get("coordinates"); | ||
assertThat(coordinatesNode.isArray()).isTrue(); | ||
ArrayNode coordinatesArray = (ArrayNode) coordinatesNode; | ||
|
||
// There's an extra layer here, presumably indicating the bounds of the polygon | ||
assertThat(coordinatesArray.size()).isEqualTo(1); | ||
JsonNode polygonNode = coordinatesArray.get(0); | ||
assertThat(polygonNode.isArray()).isTrue(); | ||
ArrayNode polygonArray = (ArrayNode) polygonNode; | ||
|
||
assertThat(polygonArray.size()).isEqualTo(5); | ||
for (int i = 0; i < expected.length; ++i) { | ||
|
||
JsonNode elemNode = polygonArray.get(i); | ||
assertThat(elemNode.isArray()).isTrue(); | ||
ArrayNode elemArray = (ArrayNode) elemNode; | ||
assertThat(elemArray.size()).isEqualTo(2); | ||
assertThat(elemArray.get(0).asDouble()).isEqualTo(expected[i][0]); | ||
assertThat(elemArray.get(1).asDouble()).isEqualTo(expected[i][1]); | ||
} | ||
} | ||
|
||
@Test | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ESRI 1.2.1 used to test equality by a literal comparison across fields of OGCGeometry types. As of 2.2.4 this was changed to two distinct comparisons: equals(Object) (the standard Java equality op) and equals(OGCGeometry). The later was subsequently deprecated and moved to an Equals(OGCGeometry) method. The code above was calling the (now deprecated) method.
More info can be found on a thread starting from the relevant esri-geometry-api PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: without this change you get failures like the following: