Skip to content

Raise ConnectionError on most non-200 errors#185

Merged
matthinea merged 8 commits into
masterfrom
no-silent-failures
Jun 8, 2026
Merged

Raise ConnectionError on most non-200 errors#185
matthinea merged 8 commits into
masterfrom
no-silent-failures

Conversation

@matthinea

@matthinea matthinea commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

Problem

Most methods interface with Elasticsearch via json_request. The json_request method only raises on 5xx errors, or when there's an explicity error key in the JSON response. This leaves 3xx and 4xx errors as silent failures.

Solution

  • For post and put methods, raise unless the response code is 2xx.
  • For 'get' and 'delete' methods, additionally allow 404 Not Found, since apparently some methods rely on that.

Comment thread lib/elastic_record/connection.rb Outdated
@matthinea matthinea requested a review from j1wilmot June 4, 2026 22:53

@j1wilmot j1wilmot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Approach looks good, but should this be grouped with the 5xx handling? Now there are multiple code locations that handle status codes.

@matthinea

Copy link
Copy Markdown
Contributor Author

In principal, yes, but core issue is:

  • 404 is expected to be valid for get and delete
  • all responses should be valid for head
  • only 2xx responses should be valid or put or post

Let me take another pass at this and see if it can be simplified without resulting in major changes to the interface.

@matthinea

Copy link
Copy Markdown
Contributor Author

Ok it looks like we can get by with just 404 being valid for head, which simplifies things. Let's me test this branch on QA in a downstream application and make sure everything's working.

@matthinea

matthinea commented Jun 6, 2026

Copy link
Copy Markdown
Contributor Author

Ok, I added some test cases with real-life examples of how & why GET and DELETE should sometimes not error on 404.

I ran some bulk and individual updates on QA documents with this branch and everything seems to be performing good 👍

@matthinea matthinea merged commit 17448c3 into master Jun 8, 2026
1 check passed
@matthinea matthinea deleted the no-silent-failures branch June 8, 2026 19:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants