fix: 🐛 clean catch elastic search errors [v2]

[HannesOberreiter]

Errors from elastic search have a different structure. I try to catch them in the error middleware to give a cleaner response.

Additionally, added a very simple test case, although the test will fail if the elastic search window size is manually increases or a future version of elastic search will increase the default value. Therefore it is not a stable test.

This is my "solution" for the issue I posted a while ago:
#379

Alternatively, we could write for this special case an even nicer error message, eg. "please use sliding window (lastId) if you want to search deeper". But I would say no, as I'm no big fan special hardcoded exceptions.

✅ tests on --fgrep search all good

Cheers
Hannes

[pleary]

Thanks for the pull request @HannesOberreiter . I think I would actually prefer the "special case" option for this scenario. A couple thoughts: this changes the HTTP status of the request from 500 to 400, since 400 is the Elasticsearch query errorCode, not the 500 that we're specifying in our code. This also adds additional structure to the error response that wasn't previously there, which would require that clients update their error handling to get the actual message. The error response normally is:

{
  "status": "500",
  "errors": [
    {
      "errorCode": "500",
      "message": "search_phase_execution_exception\n\tCaused by:\n\t\tillegal_argument_exception: Result window is too large, from + size must be less than or equal to: [10000] but was [10500]. See the scroll api for a more efficient way to request large data sets. This limit can be set by changing the [index.max_result_window] index level setting.\n\tRoot causes:\n\t\tillegal_argument_exception: Result window is too large, from + size must be less than or equal to: [10000] but was [10500]. See the scroll api for a more efficient way to request large data sets. This limit can be set by changing the [index.max_result_window] index level setting."
    }
  ]
}

but this PR makes it:

{
  "status": "400",
  "errors": [
    {
      "errorCode": "400",
      "message": {
        "type": "search_phase_execution_exception",
        "root_cause": [
          {
            "type": "illegal_argument_exception",
            "reason": "Result window is too large, from + size must be less than or equal to: [10000] but was [10500]. See the scroll api for a more efficient way to request large data sets. This limit can be set by changing the [index.max_result_window] index level setting"
          }
        ]
      }
    }
  ]
}

Another concern that isn't new to this PR, but I think it's something we can improve upon while we're making changes - this error response from Elasticsearch isn't aimed at the iNaturalist API user, its aimed at the developer of the API. The message says to "see the scroll api" for example, but API users need not be concerned with that. It may be confusing to API users to see our API return a message telling them to see the scroll API when iNaturalist doesn't have a scroll API. One final concern is that this is returning the Elasticsearch error reason directly, and I can't be 100% sure that message will never contain sensitive information.

So for the above reasons, I would prefer if this pull request didn't modify the response HTTP status, didn't add additional structure to the error response and rather populated errors[0].message with the error message, only caught this specific Elasticsearch message, and used a custom error message aimed at the iNaturalist API user. Result window is too large is descriptive enough, but the relevant parameters are page and per_page (not from and size), and the rest of the Elasticsearch message isn't relevant to an API user. Would you mind modifying this pull request accordingly?

Lastly some syntax here doesn't conform to our .eslintrc linting rules. I just added automated linting so Github actions should alert developers to those kinds of issues going forward, and modified our README to requests developer run eslint before submitting pull requests. I also recommend getting real-time linting working in your text editor if it supports it.

Thanks again for this submission! I think catching these errors with a better error message will be great for API users helping to understand why requests with very large page and per_page parameters are failing.

[HannesOberreiter]

Sure, yeah was anyway unsure how to handle it. Seems all reasonable will adapt my code.

Lastly some syntax here doesn't conform to our .eslintrc linting rules.

Yeah had to disable eslint because it was complaining too much. Especially in main frontend app, but thats another story. Will merge again from the the main branch as I saw you have run the linting.

Cheers
Hannes

[HannesOberreiter]

New default error if elastic search error without exceptions:

{
  "status": "500",
  "errors": [
    {
      "errorCode": "500",
      "message": "Elasticsearch error, if this persists please contact the iNaturalist development team."
    }
  ]
}

New error on too big search window with adapted custom message:

{
  "status": "500",
  "errors": [
    {
      "errorCode": "500",
      "message": "Result window is too large, page + size must be less than or equal to [10000]. Please narrow your search, or use a sliding window approach with id_above or id_below params."
    }
  ]
}

Cheers
Hannes

[pleary]

Thanks for making these changes. I think this is generally a better way to go. One suggestion on the code as it stands now - I'd rather this didn't include the line status = 500;. The default status for errors in v2 is 500, and by hard-coding in the value here it prevents a developer from setting a different value, like 422 or something else. I can't think of a real-world example offhand, but I think its best to honor status if already set, or rely on the API default which is 500.

A second suggestion is that since this change only applies to API v2, it might be good to have API v1 return the same messages. API v1 will eventually be deprecated once API v2 has an official 1.0 release, but for now it is current and it would be great for it to render the same kind of error. Do you want to add something similar for API v1? The place to do that would be https://github.com/inaturalist/iNaturalistAPI/blob/main/lib/util.js#L30, probably replacing the conditional block matching Result window is too large. In that case for API v1 we seem to be setting the status code to 403 (I think 422 would be better, but maybe not worth changing now), so for v1 I think we should preserve that status code. So you could check for the same existence of err.meta, set the same messages, and in the case of a window error set set status code to 403, otherwise leave status alone. Thanks!

[HannesOberreiter]

• added the elastic search error to v1 and added short test case
• renamed page + size to page x size which kinda makes more sense
• removed hardcoded 500

and by hard-coding in the value here it prevents a developer from setting a different value, like 422 or something else

I would argue if you chose to catch the elastic search error upstream you will also return a specific error and not the error object from elastic search. The middleware error handler should catch not handled errors by elastic search and prevent information leaking to the end user, therefore a hardcoded 500 makes more sense. No status here is more like "elastic search error has by chance no error.status field" therefore we return a 500 and if you read the code in one year you don't know what status to expect, when we run into this expection.

Therefore I'm more of a fan for a hardcoded 500 if a non handled elastic search error comes and maybe a specific status code like the 422 for the limit one.

Cheers
Hannes

[pleary]

Thanks! We can always tweak this again in the future, and this is a solid improvement, so I merged this as-is. These errors will end up as 500 unless a developer has chosen to override the status, and if that happens it's on them to provide a reason for it. This also ensures we preserve all existing response codes.

[HannesOberreiter]

Sure no problem, thanks for the clean review process. Will look for more opportunities to help out where I can.

To have the last word: Its still an unhandled direct error from elastic search so there is no developer who could change the status code, only if he/she try catches it (at least thats the feeling I get, would need to dig deeper in the query etc.).