Skip to content
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.

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

Display query errors from ElasticSearch #16940

Draft
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

PBMikeW
Copy link
Contributor

@PBMikeW PBMikeW commented Nov 1, 2024

Currently Elastic Query run does not return errors. (Cost me a chunk of time when converting from lucene to elastic)

#16938

For discussion. Honestly not sure the best way to handle this.

Options:

  1. Do we want to bother with this or leave it to users to try and dig into logging? (Error is logged in ES module).
  2. We could just let the error hit the front end. (however one query error breaks whole page)
  3. We swallow any errors on API and helpers and keep the exception bubbling up to the admin ui.
  4. We use some sort of notification instead.

ElasticQueryService rethrows to bubble error up to admin UI and to ApiExceptionHandlingFilter.
image

New ApiExceptionHandlingFilter to return nice problem details on API exception.
image

Swallow error on razor helper. Might need to do liquid filter too. Optionally could not swallow error and razor would return:
image

@PBMikeW
Copy link
Contributor Author

PBMikeW commented Nov 1, 2024

@Skrypt @sebastienros

{
Type = "https://tools.ietf.org/html/rfc7231#section-6.6.1",
Title = "An error occurred while processing your request.",
Status = StatusCodes.Status500InternalServerError,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I would use a 400 for this one.

Copy link
Contributor

Choose a reason for hiding this comment

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

We could also pass an error code along with the error string that we could document. Just to make a distinction between 500 and 400.

Copy link
Contributor

This pull request has merge conflicts. Please resolve those before requesting a review.

@MikeAlhayek
Copy link
Member

The class ElasticQueryService is now named ElasticsearchQueryService. Also please test out your changes with the latest changes done to Elasticsearch module as it has been changed to use a different library.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants