Remove vestiages of geojsonhint (#556)

gadomski · Phil Varner · web-flow · commit 5ca0c0997a0a · 2023-07-14T15:50:29.000Z
* fix: remove vestiages of geojsonhint

* improve error handling

* Update CHANGELOG.md

---------

Co-authored-by: Phil Varner &lt;pvarner@element84.com&gt;
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -5,6 +5,13 @@ All notable changes to this project will be documented in this file.
 The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/)
 and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.html).
 
+## [Unreleased]
+
+### Changed
+
+- Simplify the error handling around geometry errors.
+- When an OpenSearch request returns a 400 status code, use this same status code with a meaningful error message in the stac-server response, instead of always returning a 500 error.
+
 ## [2.2.2] - 2023-07-06
 
 ### Changed
diff --git a/README.md b/README.md
@@ -587,14 +587,16 @@ either be done through the OpenSearch API or Dashboard.
 
 ##### Option 1 - API method
 
-This assumes the master username is `admin` and creats a user with the name `stac_server`.
+This assumes the master username is `admin` and creates a user with the name `stac_server`.
+Environment variables `HOST` and `OPENSEARCH_MASTER_USER_PASSWORD` should be set in the
+shell environment.
 
 Create the Role:
 
 ```shell
 curl -X "PUT" "${HOST}/_plugins/_security/api/roles/stac_server_role" \
      -H 'Content-Type: application/json; charset=utf-8' \
-     -u 'admin:xxxxxxxx' \
+     -u "admin:${OPENSEARCH_MASTER_USER_PASSWORD}" \
      -d $'{
   "cluster_permissions": [
     "cluster_composite_ops",
@@ -629,7 +631,7 @@ Create the User:
 ```shell
 curl -X "PUT" "${HOST}/_plugins/_security/api/internalusers/stac_server" \
      -H 'Content-Type: application/json; charset=utf-8' \
-     -u 'admin:xxxxxxxx' \
+     -u "admin:${OPENSEARCH_MASTER_USER_PASSWORD}" \
      -d $'{ "password": "xxx" }'
 ```
 
@@ -640,7 +642,7 @@ Map the Role to the User:
 ```shell
 curl -X "PUT" "${HOST}/_plugins/_security/api/rolesmapping/stac_server_role" \
      -H 'Content-Type: application/json; charset=utf-8' \
-     -u 'admin:xxxxxxxx' \
+     -u "admin:${OPENSEARCH_MASTER_USER_PASSWORD}"  \
      -d $'{
   "users": [
     "stac_server"
diff --git a/package.json b/package.json
@@ -63,7 +63,6 @@
   "dependencies": {
     "@aws-sdk/client-secrets-manager": "^3.359.0",
     "@mapbox/extent": "^0.4.0",
-    "@mapbox/geojsonhint": "^3.2.0",
     "@opensearch-project/opensearch": "^2.2.1",
     "cors": "^2.8.5",
     "express": "^4.18.2",
diff --git a/src/lib/api.js b/src/lib/api.js
@@ -618,19 +618,6 @@ const searchItems = async function (collectionId, queryParameters, backend, endp
 
   logger.info('Search parameters (processed): %j', searchParams)
 
-  // try { // Attempt to catch invalid geometry before querying Search
-  //   const hints = geometry ? geojsonhint.hint(geometry, {}) : []
-
-  //   if (hints.length > 0) {
-  //     return {
-  //       statusCode: 400,
-  //       body: hints.map(({ message }) => ({ message }))
-  //     }
-  //   }
-  // } catch (e) {
-  //   logger.error(e)
-  // }
-
   let esResponse
   try {
     esResponse = await backend.search(searchParams, page, limit)
@@ -644,29 +631,24 @@ const searchItems = async function (collectionId, queryParameters, backend, endp
         },
         results: []
       }
-    } else {
-      try {
-        // @ts-ignore
-        const e = error['meta']['body']['error']
-
-        let errorMessage
-        if ('caused_by' in e) { // Parse certain types of geometry errors from Search
-          errorMessage = e['caused_by'].map(({ message }) => ({ message }))
-        } else if ('root_cause' in e) { // Parse other types of geometry errors from Search
-          errorMessage = e['root_cause'].map(({ reason }) => ({ reason }))
-        } else if (JSON.stringify(error).includes('failed to create query')) {
-          errorMessage = 'Query failed. Please verify a valid query payload.'
-        } else {
-          throw error
-        }
-
-        return {
-          statusCode: 400,
-          body: errorMessage
-        }
-      } catch (_) {
-        throw error
+    // @ts-ignore
+    } else if (error?.meta?.statusCode === 400) {
+      // @ts-ignore
+      const e = error?.meta?.body?.error
+
+      // only serialize part of the error message,
+      // as error.meta.meta.connection will leak the OpenSearch URL
+      let errorMessage
+      if ('caused_by' in e) {
+        errorMessage = JSON.stringify(e?.caused_by?.reason)
+      } else if (JSON.stringify(e).includes('failed to create query')) {
+        errorMessage = `Query failed with invalid parameters: ${JSON.stringify(e)}`
+      } else {
+        errorMessage = `Unknown error: ${JSON.stringify(e)}`
       }
+      throw new ValidationError(errorMessage)
+    } else {
+      throw error
     }
   }
 
diff --git a/tests/fixtures/geometry/badGeoFourPointsMultiPolygon.json b/tests/fixtures/geometry/badGeoFourPointsMultiPolygon.json
@@ -0,0 +1,69 @@
+{
+  "type": "MultiPolygon",
+  "coordinates": [
+    [
+      [
+        [
+          100.0,
+          0.0
+        ],
+        [
+          101.0,
+          0.0
+        ],
+        [
+          101.0,
+          1.0
+        ],
+        [
+          100.0,
+          1.0
+        ],
+        [
+          100.0,
+          2.0
+        ],
+        [
+          100.0,
+          1.0
+        ],
+        [
+          100.0,
+          0.0
+        ]
+      ]
+    ],
+    [
+      [
+        [
+          100.0,
+          0.0
+        ],
+        [
+          101.0,
+          0.0
+        ],
+        [
+          101.0,
+          1.0
+        ],
+        [
+          100.0,
+          1.0
+        ],
+        [
+          100.0,
+          2.0
+        ],
+        [
+          100.0,
+          1.0
+        ],
+        [
+          100.0,
+          0.0
+        ]
+      ]
+    ]
+  ]
+}
diff --git a/tests/fixtures/geometry/polygonWoundCCW.json b/tests/fixtures/geometry/polygonWoundCCW.json
@@ -0,0 +1,12 @@
+{
+  "type": "Polygon",
+  "coordinates": [
+      [
+          [100.0, 0.0],
+          [100.0, 1.0],
+          [101.0, 1.0],
+          [101.0, 0.0],
+          [100.0, 0.0]
+      ]
+  ]
+}
diff --git a/tests/system/test-api-search-post.js b/tests/system/test-api-search-post.js
@@ -14,7 +14,7 @@ const __filename = fileURLToPath(import.meta.url)
 const __dirname = path.dirname(__filename) // eslint-disable-line no-unused-vars
 const intersectsGeometry = fs.readFileSync(path.resolve(__dirname, '../fixtures/stac/intersectsGeometry.json'), 'utf8')
 
-// const fixture = (filepath) => fs.readFileSync(path.resolve(__dirname, filepath), 'utf8')
+const fixture = (filepath) => fs.readFileSync(path.resolve(__dirname, filepath), 'utf8')
 
 const ingestEntities = async (fixtures) => {
   await ingestItems(
@@ -420,68 +420,92 @@ test('/search preserve intersects geometry in next link', async (t) => {
   t.deepEqual(nextLink.body.intersects, intersectsGeometry)
 })
 
-// test('POST /search using bad geometry, expecting useful error messages', async (t) => {
-//   let response = null
-
-//   response = await t.context.api.client.post('search', {
-//     json: {
-//       intersects: fixture('../fixtures/geometry/badGeoUnclosed.json')
-//     }
-//   })
-//   t.is(response.statusCode, 400)
-//   t.deepEqual(response.body,
-//  [{ message: 'the first and last positions in a LinearRing of coordinates must be the same' }])
-
-//   response = await t.context.api.client.post('search', {
-//     json: {
-//       intersects: fixture('../fixtures/geometry/badGeoRightHandRule.json')
-//     }
-//   })
-//   t.is(response.statusCode, 400)
-//   t.deepEqual(response.body,
-// [{ message: 'Polygons and MultiPolygons should follow the right-hand rule' }])
-
-//   response = await t.context.api.client.post('search', {
-//     json: {
-//       intersects: fixture('../fixtures/geometry/badGeoFourPoints.json')
-//     }
-//   })
-//   t.is(response.statusCode, 400)
-//   t.deepEqual(response.body, [
-//     {
-//       reason: 'failed to create query: at least 4 polygon points required'
-//     },
-//     {
-//       reason: 'failed to create query: at least 4 polygon points required'
-//     }
-//   ])
-
-//   response = await t.context.api.client.post('search', {
-//     json: {
-//       intersects: fixture('../fixtures/geometry/badGeoDuplicateConsecutive.json')
-//     }
-//   })
-//   t.is(response.statusCode, 400)
-//   t.deepEqual(response.body, [
-//     {
-//       reason: 'failed to create query:
-// Provided shape has duplicate consecutive coordinates at: (POINT (100.0 1.0))'
-//     },
-//     {
-//       reason: 'failed to create query:
-// Provided shape has duplicate consecutive coordinates at: (POINT (100.0 1.0))'
-//     }
-//   ])
-
-//   response = await t.context.api.client.post('search', {
-//     json: {
-//       intersects: fixture('../fixtures/geometry/badGeoRightHandRule2.json')
-//     }
-//   })
-//   t.is(response.statusCode, 400)
-//   t.deepEqual(response.body,
-// [{ message: 'Polygons and MultiPolygons should follow the right-hand rule' }])
-// })
+test('POST /search - polygon wound incorrectly, but should succeeed', async (t) => {
+  const response = await t.context.api.client.post('search', {
+    json: {
+      intersects: fixture('../fixtures/geometry/polygonWoundCCW.json')
+    }
+  })
+  t.is(response.features.length, 0)
+})
+
+test('POST /search - failure when polygon is unclosed', async (t) => {
+  const error = await t.throwsAsync(async () => t.context.api.client.post('search', {
+    json: {
+      intersects: fixture('../fixtures/geometry/badGeoUnclosed.json')
+    }
+  }))
+  t.is(error.response.statusCode, 400)
+  t.is(error.response.body.code, 'BadRequest')
+  t.regex(error.response.body.description,
+    /.*invalid LinearRing found \(coordinates are not closed\).*/)
+})
+
+test('POST /search - failure when ambigous winding', async (t) => {
+  // The right-hand rule part is ok (see:
+  // https://github.com/stac-utils/stac-server/issues/549) but there's
+  // coinciding points.
+  const error = await t.throwsAsync(async () => t.context.api.client.post('search', {
+    json: {
+      intersects: fixture('../fixtures/geometry/badGeoRightHandRule.json')
+    }
+  }))
+  t.is(error.response.statusCode, 400)
+  t.is(error.response.body.code, 'BadRequest')
+  t.regex(error.response.body.description,
+    /.*failed to create query: Cannot determine orientation: edges adjacent to.*/)
+})
+
+test('POST /search - failure when ambigous winding 2', async (t) => {
+  // The right-hand rule part is ok (see:
+  // https://github.com/stac-utils/stac-server/issues/549) but there's
+  // coinciding points.
+  const error = await t.throwsAsync(async () => t.context.api.client.post('search', {
+    json: {
+      intersects: fixture('../fixtures/geometry/badGeoRightHandRule2.json')
+    }
+  }))
+  t.is(error.response.statusCode, 400)
+  t.is(error.response.body.code, 'BadRequest')
+  t.regex(error.response.body.description,
+    /.*failed to create query: Cannot determine orientation: edges adjacent to.*/)
+})
+
+test('POST /search - failure when Polygon only has 4 points ', async (t) => {
+  const error = await t.throwsAsync(async () => t.context.api.client.post('search', {
+    json: {
+      intersects: fixture('../fixtures/geometry/badGeoFourPoints.json')
+    }
+  }))
+  t.is(error.response.statusCode, 400)
+  t.is(error.response.body.code, 'BadRequest')
+  t.regex(error.response.body.description,
+    /.*failed to create query: at least 4 polygon points required.*/)
+})
+
+test('POST /search - failure when shape has duplicate consecutive coordinates', async (t) => {
+  const error = await t.throwsAsync(async () => t.context.api.client.post('search', {
+    json: {
+      intersects: fixture('../fixtures/geometry/badGeoDuplicateConsecutive.json')
+    }
+  }))
+  t.is(error.response.statusCode, 400)
+  t.is(error.response.body.code, 'BadRequest')
+  t.regex(error.response.body.description,
+    /.*Provided shape has duplicate consecutive coordinates at.*/)
+})
+
+test('POST /search - failure when MultiPolygon has only 4 points', async (t) => {
+  const error = await t.throwsAsync(async () => t.context.api.client.post('search', {
+    json: {
+      intersects: fixture('../fixtures/geometry/badGeoFourPointsMultiPolygon.json')
+    }
+  }))
+  t.is(error.response.statusCode, 400)
+  t.is(error.response.body.code, 'BadRequest')
+  t.regex(error.response.body.description,
+    /.*failed to create query: at least 4 polygon points required.*/)
+})
 
 test('/search preserve bbox in prev and next links', async (t) => {
   const bbox = [-180, -90, 180, 90]

-Original file line number
+Diff line change
@@ @@ -0,0 +1,69 @@ @@
 +{
 +  "type": "MultiPolygon",
 +  "coordinates": [
 +    [
 +      [
 +        [
 +          100.0,
 +          0.0
 +        ],
 +        [
 +          101.0,
 +          0.0
 +        ],
 +        [
 +          101.0,
 +          1.0
 +        ],
 +        [
 +          100.0,
 +          1.0
 +        ],
 +        [
 +          100.0,
 +          2.0
 +        ],
 +        [
 +          100.0,
 +          1.0
 +        ],
 +        [
 +          100.0,
 +          0.0
 +        ]
 +      ]
 +    ],
 +    [
 +      [
 +        [
 +          100.0,
 +          0.0
 +        ],
 +        [
 +          101.0,
 +          0.0
 +        ],
 +        [
 +          101.0,
 +          1.0
 +        ],
 +        [
 +          100.0,
 +          1.0
 +        ],
 +        [
 +          100.0,
 +          2.0
 +        ],
 +        [
 +          100.0,
 +          1.0
 +        ],
 +        [
 +          100.0,
 +          0.0
 +        ]
 +      ]
 +    ]
 +  ]
 +}