Add geojson support for XYPoint #162

heemin32 · 2022-10-11T01:57:40Z

See #152

Signed-off-by: Heemin Kim heemin@amazon.com

Description

Supports GeoJson Point type in XYPoint field

Issues Resolved

#152

Check List

Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

VijayanB · 2022-10-11T19:21:42Z

Description has to be changed to xy_point not geo_point :)

heemin32 · 2022-10-11T20:34:13Z

Description has to be changed to xy_point not geo_point :)

Copy&Paste error. Thanks!

martin-gaievski · 2022-10-12T18:41:01Z

src/main/java/org/opensearch/geospatial/index/mapper/xypoint/XYPointParser.java

+        throws IOException {
+        try (XContentSubParser subParser = new XContentSubParser(parser)) {
+            if (subParser.nextToken() != XContentParser.Token.FIELD_NAME) {
+                throw new OpenSearchParseException(ERR_MSG_INVALID_TOKEN, subParser.currentToken());


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.

Could you share what security risk this might contain?

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

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.

martin-gaievski · 2022-10-12T18:45:05Z

src/main/java/org/opensearch/geospatial/index/mapper/xypoint/XYPointParser.java

+
+    private static XYPoint parseGeoPointObjectBasicFields(final XContentParser parser, final XYPoint point) throws IOException {
+        HashMap<String, Double> data = new HashMap<>();
+        for (int i = 0; i < 2; i++) {


better introduce/use constant IMO

This number is specific to this method. Let me use variable instead to make its meaning to be explicit.

martin-gaievski · 2022-10-12T18:50:19Z

src/main/java/org/opensearch/geospatial/index/mapper/xypoint/XYPointParser.java

-        if (parser.currentToken() == XContentParser.Token.START_ARRAY) {
-            return parseXYPointArray(parser, ignoreZValue, x, y);
+            String field = parser.currentName();
+            if (X_PARAMETER.equals(field) == false && Y_PARAMETER.equals(field) == false) {


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

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

martin-gaievski · 2022-10-12T18:53:08Z

src/main/java/org/opensearch/geospatial/index/mapper/xypoint/XYPointParser.java

+                    try {
+                        data.put(field, parser.doubleValue(true));
+                    } catch (NumberFormatException e) {
+                        throw new OpenSearchParseException("[{}] and [{}] must be valid double values", e, X_PARAMETER, Y_PARAMETER);


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

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

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.

src/main/java/org/opensearch/geospatial/index/mapper/xypoint/XYPointParser.java

Signed-off-by: Heemin Kim <heemin@amazon.com>

navneet1v

Minor comment.

src/main/java/org/opensearch/geospatial/index/mapper/xypoint/XYPointParser.java

Signed-off-by: Heemin Kim <heemin@amazon.com>

opensearch-trigger-bot · 2022-10-19T21:51:46Z

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-2.x 2.x
# Navigate to the new working tree
cd .worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-162-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 63c97b083f137a035563710c67c57fc23b9493f4
# Push it to GitHub
git push --set-upstream origin backport/backport-162-to-2.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-162-to-2.x.

* Add geojson support for XYPoint Signed-off-by: Heemin Kim <heemin@amazon.com> (cherry picked from commit 63c97b0)

heemin32 requested review from a team, navneet1v and VijayanB October 11, 2022 01:57

heemin32 force-pushed the geojson branch from 97a20e1 to 5c825cd Compare October 11, 2022 02:01

martin-gaievski reviewed Oct 12, 2022

View reviewed changes

heemin32 force-pushed the geojson branch from d41294e to d457eb8 Compare October 12, 2022 19:49