Skip to content

Add geojson support for XYPoint #162

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.

Sign up for GitHub

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

Merged
heemin32 merged 3 commits into from
Oct 19, 2022
Merged

Add geojson support for XYPoint #162

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
80379d6
Add geojson support for XYPoint
heemin32 Oct 11, 2022
29b5c68
Uses a variable to represent a number
heemin32 Oct 12, 2022
6e35391
Resolve review comments
heemin32 Oct 12, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
277 changes: 194 additions & 83 deletions src/main/java/org/opensearch/geospatial/index/mapper/xypoint/XYPointParser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@

import java.io.IOException;
import java.util.Collections;
import java.util.HashMap;
import java.util.Map;
import java.util.Objects;

import org.opensearch.OpenSearchParseException;
Expand All @@ -15,13 +17,18 @@
import org.opensearch.common.xcontent.XContentParser;
import org.opensearch.common.xcontent.XContentSubParser;
import org.opensearch.common.xcontent.support.MapXContentParser;
import org.opensearch.geometry.ShapeType;

/**
* Parse the value and set XYPoint represented as a String, Object, WKT, array.
*/
public class XYPointParser {
private static final String ERR_MSG_INVALID_TOKEN = "token [{}] not allowed";
private static final String ERR_MSG_INVALID_FIELDS = "field must be either [x|y], or [type|coordinates]";
private static final String X_PARAMETER = "x";
private static final String Y_PARAMETER = "y";
public static final String GEOJSON_TYPE = "type";
public static final String GEOJSON_COORDS = "coordinates";
private static final String NULL_VALUE_PARAMETER = "null_value";
private static final Boolean TRUE = true;

Expand All @@ -33,7 +40,7 @@ public class XYPointParser {
* @return {@link XYPoint} after setting the x and y coordinates parsed from the parse
* @throws OpenSearchParseException
*/
public static XYPoint parseXYPoint(Object value, final boolean ignoreZValue) throws OpenSearchParseException {
public static XYPoint parseXYPoint(final Object value, final boolean ignoreZValue) throws OpenSearchParseException {
Objects.requireNonNull(value, "input value which needs to be parsed should not be null");

try (
Expand All @@ -54,13 +61,13 @@ public static XYPoint parseXYPoint(Object value, final boolean ignoreZValue) thr
}

/**
* Parse the values to set the XYPoint which was represented as a String, Object, WKT or an array.
*
* Parse the values to set the XYPoint which was represented as a String, Object, WKT, Array, or GeoJson.
* <ul>
* <li> String: "100.35, -200.54" </li>
* <li> Object: {"x" : 100.35, "y" : -200.54} </li>
* <li> WKT: "POINT (-200.54 100.35)"</li>
* <li> Array: [ -200.54, 100.35 ]</li>
* <li>Object: <pre>{@code {"x": <x>, "y": <y}}</pre></li>
* <li>String: <pre>{@code "<x>,<y>"}</pre></li>
* <li>WKT: <pre>{@code "POINT (<x> <y>)"}</pre></li>
* <li>Array: <pre>{@code [<x>, <y>]}</pre></li>
* <li>GeoJson: <pre>{@code {"type": "Point", "coordinates": [<x>, <y>]}}</pre><li>
* </ul>
*
* @param parser {@link XContentParser} to parse the value from
Expand All @@ -69,103 +76,207 @@ public static XYPoint parseXYPoint(Object value, final boolean ignoreZValue) thr
* @throws IOException
* @throws OpenSearchParseException
*/
public static XYPoint parseXYPoint(XContentParser parser, final boolean ignoreZValue) throws IOException, OpenSearchParseException {
public static XYPoint parseXYPoint(final XContentParser parser, final boolean ignoreZValue) throws IOException,
OpenSearchParseException {
Objects.requireNonNull(parser, "parser should not be null");

XYPoint point = new XYPoint();
double x = Double.NaN;
double y = Double.NaN;

if (parser.currentToken() == XContentParser.Token.START_OBJECT) {
try (XContentSubParser subParser = new XContentSubParser(parser)) {
while (subParser.nextToken() != XContentParser.Token.END_OBJECT) {
if (subParser.currentToken() != XContentParser.Token.FIELD_NAME) {
throw new OpenSearchParseException("token [{}] not allowed", subParser.currentToken());
}
String field = subParser.currentName();
if (!(X_PARAMETER.equals(field) || Y_PARAMETER.equals(field))) {
throw new OpenSearchParseException("field must be either [{}] or [{}]", X_PARAMETER, Y_PARAMETER);
}
if (X_PARAMETER.equals(field)) {
subParser.nextToken();
switch (subParser.currentToken()) {
case VALUE_NUMBER:
case VALUE_STRING:
try {
x = subParser.doubleValue(TRUE);
} catch (NumberFormatException numberFormatException) {
throw new OpenSearchParseException("[x] must be valid double value", numberFormatException);
}
break;
default:
throw new OpenSearchParseException("[x] must be a number");
}
}
if (Y_PARAMETER.equals(field)) {
subParser.nextToken();
switch (subParser.currentToken()) {
case VALUE_NUMBER:
case VALUE_STRING:
try {
y = subParser.doubleValue(TRUE);
} catch (NumberFormatException numberFormatException) {
throw new OpenSearchParseException("[y] must be valid double value", numberFormatException);
}
break;
default:
throw new OpenSearchParseException("[y] must be a number");
}
}
}
switch (parser.currentToken()) {
case START_OBJECT:
parseXYPointObject(parser, point, ignoreZValue);
break;
case START_ARRAY:
parseXYPointArray(parser, point, ignoreZValue);
break;
case VALUE_STRING:
String val = parser.text();
point.resetFromString(val, ignoreZValue);
break;
default:
throw new OpenSearchParseException("expecting xy_point as an array, a string, or an object format");
}
return point;
}

/**
* Parse point in either basic object format or GeoJson format
*
* Parser is expected to be pointing the start of the object.
* ex) Parser is pointing { in {"x": 12.3, "y": 45.6}
*
* @param parser {@link XContentParser} to parse the value from
* @param point {@link XYPoint} to be returned after setting the x and y coordinates parsed from the parse
* @return {@link XYPoint} after setting the x and y coordinates parsed from the parse
* @throws IOException
*/
private static XYPoint parseXYPointObject(final XContentParser parser, final XYPoint point, final boolean ignoreZValue)
throws IOException {
try (XContentSubParser subParser = new XContentSubParser(parser)) {
if (subParser.nextToken() != XContentParser.Token.FIELD_NAME) {
throw new OpenSearchParseException(ERR_MSG_INVALID_TOKEN, subParser.currentToken());
Copy link
Copy Markdown
Member

@martin-gaievski martin-gaievski Oct 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's not safe to return user input as part of the output, learned from the last pentest. I suggest minimize error message to just "token not allowed" and add logger with all required details.

Copy link
Copy Markdown
Collaborator Author

@heemin32 heemin32 Oct 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you share what security risk this might contain?

Copy link
Copy Markdown
Member

@martin-gaievski martin-gaievski Oct 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure, in my report they have call it "User Input Reflected on Error" and marked it as "Medium" severity

Copy link
Copy Markdown
Collaborator Author

@heemin32 heemin32 Oct 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @martin-gaievski . subParser.currentToken() is not random user input. It is enum defined in XContentParser.Token. So, I think there is no security risk returning it.

}
if (Double.isNaN(x)) {
throw new OpenSearchParseException("field [{}] missing", X_PARAMETER);

String field = subParser.currentName();
if (X_PARAMETER.equals(field) || Y_PARAMETER.equals(field)) {
parseXYPointObjectBasicFields(subParser, point);
} else if (GEOJSON_TYPE.equals(field) || GEOJSON_COORDS.equals(field)) {
parseGeoJsonFields(subParser, point, ignoreZValue);
} else {
throw new OpenSearchParseException(ERR_MSG_INVALID_FIELDS);
}
if (Double.isNaN(y)) {
throw new OpenSearchParseException("field [{}] missing", Y_PARAMETER);

if (subParser.nextToken() != XContentParser.Token.END_OBJECT) {
throw new OpenSearchParseException(ERR_MSG_INVALID_FIELDS);
}
return point.reset(x, y);

return point;
}
}

if (parser.currentToken() == XContentParser.Token.START_ARRAY) {
return parseXYPointArray(parser, ignoreZValue, x, y);
/**
* Parse point in basic object format
*
* Parser is expected to be pointing the first field of the object.
* ex) Parser is pointing x in {"x": 12.3, "y": 45.6}
*
* @param parser {@link XContentParser} to parse the value from
* @param point {@link XYPoint} to be returned after setting the x and y coordinates parsed from the parse
* @return {@link XYPoint} after setting the x and y coordinates parsed from the parse
* @throws IOException
*/
private static XYPoint parseXYPointObjectBasicFields(final XContentParser parser, final XYPoint point) throws IOException {
final int numberOfFields = 2;
Map<String, Double> data = new HashMap<>();
for (int i = 0; i < numberOfFields; i++) {
if (i != 0) {
parser.nextToken();
}

if (parser.currentToken() != XContentParser.Token.FIELD_NAME) {
break;
}

String field = parser.currentName();
if (X_PARAMETER.equals(field) == false && Y_PARAMETER.equals(field) == false) {
Copy link
Copy Markdown
Member

@martin-gaievski martin-gaievski Oct 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume syntax with "==" is for better readability, correct?

Copy link
Copy Markdown
Collaborator Author

@heemin32 heemin32 Oct 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. OpenSearch's standard convention is avoiding negation like !X_PARAMETER.equals(field)

throw new OpenSearchParseException(ERR_MSG_INVALID_FIELDS);
}
switch (parser.nextToken()) {
case VALUE_NUMBER:
case VALUE_STRING:
try {
data.put(field, parser.doubleValue(true));
} catch (NumberFormatException e) {
throw new OpenSearchParseException("[{}] and [{}] must be valid double values", e, X_PARAMETER, Y_PARAMETER);
Copy link
Copy Markdown
Member

@martin-gaievski martin-gaievski Oct 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be consistent to create anew constant for error message here?

Copy link
Copy Markdown
Collaborator Author

@heemin32 heemin32 Oct 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The message is used only in this method. I don't think it is worth to make is as a constant?

Copy link
Copy Markdown
Member

@martin-gaievski martin-gaievski Oct 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm leaving it up to you, but I would make it a constant anyway. From my experience it helps to avoid creating multiple identical messages with slightly different wording. If all messages are in one place chance is higher that next person will reuse one from the list.

}
break;
default:
throw new OpenSearchParseException("{} must be a number", field);
}
}

if (parser.currentToken() == XContentParser.Token.VALUE_STRING) {
String val = parser.text();
return point.resetFromString(val, ignoreZValue);
if (data.get(X_PARAMETER) == null) {
throw new OpenSearchParseException("field [{}] missing", X_PARAMETER);
}
if (data.get(Y_PARAMETER) == null) {
throw new OpenSearchParseException("field [{}] missing", Y_PARAMETER);
}
throw new OpenSearchParseException("Expected xy_point. But, the provided mapping is not of type xy_point");

return point.reset(data.get(X_PARAMETER), data.get(Y_PARAMETER));
}

/**
* Parse the values to set the XYPoint which was represented as an array.
* Parse point in GeoJson format
*
* Parser is expected to be pointing the first field of the object.
* ex) Parser is pointing type in {"type": "Point", "coordinates": [12.3, 45.6]}
*
* @param subParser {@link XContentParser} to parse the values from an array
* @param parser {@link XContentParser} to parse the value from
* @param point {@link XYPoint} to be returned after setting the x and y coordinates parsed from the parse
* @param ignoreZValue boolean parameter which decides if third coordinate needs to be ignored or not
* @param x x coordinate that will be set by parsing the value from array
* @param y y coordinate that will be set by parsing the value from array
* @return {@link XYPoint} after setting the x and y coordinates parsed from the parse
* @throws IOException
*/
private static XYPoint parseXYPointArray(XContentParser subParser, final boolean ignoreZValue, double x, double y) throws IOException {
XYPoint point = new XYPoint();
int element = 0;
while (subParser.nextToken() != XContentParser.Token.END_ARRAY) {
if (subParser.currentToken() != XContentParser.Token.VALUE_NUMBER) {
throw new OpenSearchParseException("numeric value expected");
private static XYPoint parseGeoJsonFields(final XContentParser parser, final XYPoint point, final boolean ignoreZValue)
throws IOException {
final int numberOfFields = 2;
boolean hasTypePoint = false;
boolean hasCoordinates = false;
for (int i = 0; i < numberOfFields; i++) {
if (i != 0) {
parser.nextToken();
}

if (parser.currentToken() != XContentParser.Token.FIELD_NAME) {
if (hasTypePoint == false) {
throw new OpenSearchParseException("field [{}] missing", GEOJSON_TYPE);
}
if (hasCoordinates == false) {
throw new OpenSearchParseException("field [{}] missing", GEOJSON_COORDS);
}
}
element++;
if (element == 1) {
x = subParser.doubleValue();
} else if (element == 2) {
y = subParser.doubleValue();
} else if (element == 3) {
XYPoint.assertZValue(ignoreZValue, subParser.doubleValue());

if (GEOJSON_TYPE.equals(parser.currentName())) {
if (parser.nextToken() != XContentParser.Token.VALUE_STRING) {
throw new OpenSearchParseException("{} must be a string", GEOJSON_TYPE);
}

// To be consistent with geo_shape parsing, ignore case here as well.
if (ShapeType.POINT.name().equalsIgnoreCase(parser.text()) == false) {
throw new OpenSearchParseException("{} must be Point", GEOJSON_TYPE);
}
hasTypePoint = true;
} else if (GEOJSON_COORDS.equals(parser.currentName())) {
if (parser.nextToken() != XContentParser.Token.START_ARRAY) {
throw new OpenSearchParseException("{} must be an array", GEOJSON_COORDS);
}
parseXYPointArray(parser, point, ignoreZValue);
hasCoordinates = true;
} else {
throw new OpenSearchParseException("[xy_point] field type does not accept more than 3 dimensions");
throw new OpenSearchParseException(ERR_MSG_INVALID_FIELDS);
}
}
return point.reset(x, y);

return point;
}

/**
* Parse point in an array format
*
* Parser is expected to be pointing the start of the array.
* ex) Parser is pointing [ in [12.3, 45.6]
*
* @param parser {@link XContentParser} to parse the value from
* @param point {@link XYPoint} to be returned after setting the x and y coordinates parsed from the parse
* @param ignoreZValue boolean parameter which decides if third coordinate needs to be ignored or not
* @return {@link XYPoint} after setting the x and y coordinates parsed from the parse
* @throws IOException
*/
private static XYPoint parseXYPointArray(final XContentParser parser, final XYPoint point, final boolean ignoreZValue)
throws IOException {
try (XContentSubParser subParser = new XContentSubParser(parser)) {
double x = Double.NaN;
double y = Double.NaN;

int element = 0;
while (subParser.nextToken() != XContentParser.Token.END_ARRAY) {
if (parser.currentToken() != XContentParser.Token.VALUE_NUMBER) {
throw new OpenSearchParseException("numeric value expected");
}
element++;
if (element == 1) {
x = parser.doubleValue();
} else if (element == 2) {
y = parser.doubleValue();
} else if (element == 3) {
XYPoint.assertZValue(ignoreZValue, parser.doubleValue());
} else {
throw new OpenSearchParseException("[xy_point] field type does not accept more than 3 values");
}
}

if (element < 2) {
throw new OpenSearchParseException("[xy_point] field type should have at least two dimensions");
}
return point.reset(x, y);
}
}
}
26 changes: 26 additions & 0 deletions src/test/java/org/opensearch/geospatial/index/mapper/xypoint/XYPointFieldMapperIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,9 @@
public class XYPointFieldMapperIT extends GeospatialRestTestCase {
private static final String FIELD_X_KEY = "x";
private static final String FIELD_Y_KEY = "y";
private static final String FIELD_GEOJSON_TYPE_KEY = "type";
private static final String FIELD_GEOJSON_TYPE_VALUE = "Point";
private static final String FIELD_GEOJSON_COORDINATES_KEY = "coordinates";

public void testMappingWithXYPointField() throws Exception {
String indexName = GeospatialTestHelper.randomLowerCaseString();
Expand Down Expand Up @@ -85,6 +88,20 @@ public void testIndexWithXYPointFieldAsObjectFormat() throws Exception {
deleteIndex(indexName);
}

public void testIndexWithXYPointFieldAsGeoJsonFormat() throws Exception {
String indexName = GeospatialTestHelper.randomLowerCaseString();
String fieldName = GeospatialTestHelper.randomLowerCaseString();
createIndex(indexName, Settings.EMPTY, Map.of(fieldName, XYPointFieldMapper.CONTENT_TYPE));
final Point point = ShapeObjectBuilder.randomPoint(randomBoolean());
String docID = indexDocument(indexName, getDocumentWithObjectValueForXYPoint(fieldName, point));
assertTrue("failed to index document", getIndexDocumentCount(indexName) > 0);
final Map<String, Object> document = getDocument(docID, indexName);
assertNotNull("failed to get indexed document", document);
String expectedValue = String.format(Locale.ROOT, "{x=%s, y=%s}", point.getX(), point.getY());
assertEquals("failed to index xy_point", expectedValue, document.get(fieldName).toString());
deleteIndex(indexName);
}

private String getDocumentWithWKTValueForXYPoint(String fieldName, Geometry geometry) throws Exception {
return buildContentAsString(build -> build.field(fieldName, geometry.toString()));
}
Expand All @@ -106,4 +123,13 @@ private String getDocumentWithObjectValueForXYPoint(String fieldName, Point poin
});
}

private String getDocumentWithGeoJsonValueForXYPoint(String fieldName, Point point) throws Exception {
return buildContentAsString(build -> {
build.startObject(fieldName);
build.field(FIELD_GEOJSON_TYPE_KEY, FIELD_GEOJSON_TYPE_VALUE);
build.array(FIELD_GEOJSON_COORDINATES_KEY, new double[] { point.getX(), point.getY() });
build.endObject();
});
}

}
Loading