Add proper longitude validation in geo_polygon_query (#30497)

Fixes longitude validation in geo_polygon_query builder. The queries with wrong longitude currently fail but only later during polygon with quite complicated error message. Fixes #30488
elastic · May 10, 2018 · 2a79d92 · 2a79d92
1 parent df17f85
commit 2a79d92
Show file tree

Hide file tree

Showing 7 changed files with 69 additions and 71 deletions.
diff --git a/server/src/main/java/org/elasticsearch/index/query/GeoPolygonQueryBuilder.java b/server/src/main/java/org/elasticsearch/index/query/GeoPolygonQueryBuilder.java
@@ -177,7 +177,7 @@ protected Query doToQuery(QueryShardContext context) throws IOException {
                     throw new QueryShardException(context, "illegal latitude value [{}] for [{}]", point.lat(),
                             GeoPolygonQueryBuilder.NAME);
                 }
-                if (!GeoUtils.isValidLongitude(point.lat())) {
+                if (!GeoUtils.isValidLongitude(point.lon())) {
                     throw new QueryShardException(context, "illegal longitude value [{}] for [{}]", point.lon(),
                             GeoPolygonQueryBuilder.NAME);
                 }

diff --git a/server/src/test/java/org/elasticsearch/index/query/GeoPolygonQueryBuilderTests.java b/server/src/test/java/org/elasticsearch/index/query/GeoPolygonQueryBuilderTests.java
@@ -254,4 +254,38 @@ public void testIgnoreUnmapped() throws IOException {
         QueryShardException e = expectThrows(QueryShardException.class, () -> failingQueryBuilder.toQuery(createShardContext()));
         assertThat(e.getMessage(), containsString("failed to find geo_point field [unmapped]"));
     }
+
+    public void testPointValidation() throws IOException {
+        assumeTrue("test runs only when at least a type is registered", getCurrentTypes().length > 0);
+        QueryShardContext context = createShardContext();
+        String queryInvalidLat = "{\n" +
+            "    \"geo_polygon\":{\n" +
+            "        \"" + GEO_POINT_FIELD_NAME + "\":{\n" +
+            "            \"points\":[\n" +
+            "                [-70, 140],\n" +
+            "                [-80, 30],\n" +
+            "                [-90, 20]\n" +
+            "            ]\n" +
+            "        }\n" +
+            "    }\n" +
+            "}\n";
+
+        QueryShardException e1 = expectThrows(QueryShardException.class, () -> parseQuery(queryInvalidLat).toQuery(context));
+        assertThat(e1.getMessage(), containsString("illegal latitude value [140.0] for [geo_polygon]"));
+
+        String queryInvalidLon = "{\n" +
+            "    \"geo_polygon\":{\n" +
+            "        \"" + GEO_POINT_FIELD_NAME + "\":{\n" +
+            "            \"points\":[\n" +
+            "                [-70, 40],\n" +
+            "                [-80, 30],\n" +
+            "                [-190, 20]\n" +
+            "            ]\n" +
+            "        }\n" +
+            "    }\n" +
+            "}\n";
+
+        QueryShardException e2 = expectThrows(QueryShardException.class, () -> parseQuery(queryInvalidLon).toQuery(context));
+        assertThat(e2.getMessage(), containsString("illegal longitude value [-190.0] for [geo_polygon]"));
+    }
 }
diff --git a/server/src/test/resources/org/elasticsearch/index/query/geo_polygon_exception_1.json b/server/src/test/resources/org/elasticsearch/index/query/geo_polygon_exception_1.json
@@ -1,19 +1,12 @@
 {
-    "filtered": {
-        "query": {
-            "match_all": {}
-        },
-        "filter": {
-            "geo_polygon": {
-                "location": {
-                    "points": {
-                        "points": [
-                            [-70, 40],
-                            [-80, 30],
-                            [-90, 20]
-                        ]
-                    }
-                }
+    "geo_polygon": {
+        "location": {
+            "points": {
+                "points": [
+                    [-70, 40],
+                    [-80, 30],
+                    [-90, 20]
+                ]
             }
         }
     }

diff --git a/server/src/test/resources/org/elasticsearch/index/query/geo_polygon_exception_2.json b/server/src/test/resources/org/elasticsearch/index/query/geo_polygon_exception_2.json
@@ -1,21 +1,13 @@
 {
-    "filtered": {
-        "query": {
-            "match_all": {}
-        },
-        "filter": {
-            "geo_polygon": {
-                "location": {
-                    "points": [
-                        [-70, 40],
-                        [-80, 30],
-                        [-90, 20]
-                    ],
-                    "something_else": {
+    "geo_polygon": {
+        "location": {
+            "points": [
+                [-70, 40],
+                [-80, 30],
+                [-90, 20]
+            ],
+            "something_else": {
 
-                    }
-
-                }
             }
         }
     }

diff --git a/server/src/test/resources/org/elasticsearch/index/query/geo_polygon_exception_3.json b/server/src/test/resources/org/elasticsearch/index/query/geo_polygon_exception_3.json
@@ -1,12 +1,5 @@
 {
-    "filtered": {
-        "query": {
-            "match_all": {}
-        },
-        "filter": {
-            "geo_polygon": {
-                "location": ["WRONG"]
-            }
-        }
+    "geo_polygon": {
+        "location": ["WRONG"]
     }
 }
diff --git a/server/src/test/resources/org/elasticsearch/index/query/geo_polygon_exception_4.json b/server/src/test/resources/org/elasticsearch/index/query/geo_polygon_exception_4.json
@@ -1,19 +1,12 @@
 {
-    "filtered": {
-        "query": {
-            "match_all": {}
+    "geo_polygon": {
+        "location": {
+            "points": [
+                [-70, 40],
+                [-80, 30],
+                [-90, 20]
+            ]
         },
-        "filter": {
-            "geo_polygon": {
-                "location": {
-                    "points": [
-                        [-70, 40],
-                        [-80, 30],
-                        [-90, 20]
-                    ]
-                },
-                "bla": true
-            }
-        }
+        "bla": true
     }
 }
diff --git a/server/src/test/resources/org/elasticsearch/index/query/geo_polygon_exception_5.json b/server/src/test/resources/org/elasticsearch/index/query/geo_polygon_exception_5.json
@@ -1,19 +1,12 @@
 {
-    "filtered": {
-        "query": {
-            "match_all": {}
+    "geo_polygon": {
+        "location": {
+            "points": [
+                [-70, 40],
+                [-80, 30],
+                [-90, 20]
+            ]
         },
-        "filter": {
-            "geo_polygon": {
-                "location": {
-                    "points": [
-                        [-70, 40],
-                        [-80, 30],
-                        [-90, 20]
-                    ]
-                },
-                "bla": ["array"]
-            }
-        }
+        "bla": ["array"]
     }
 }