Skip to content
This repository has been archived by the owner on Jun 20, 2023. It is now read-only.

Deserialization process should not swallow exceptions #185

Merged
merged 3 commits into from
Mar 7, 2015

Conversation

igor-kupczynski
Copy link
Contributor

Hi,

We've found an interesting issue in one of our clusters today.

  1. We have a proxy in front of elasticsearch client nodes. Among other things this proxy does a http authentication.
  2. A java app uses jest client to do a request to the proxy
  3. We had a configuration error and proxy was responding with 401 unathorized, but we missed it since no exception was thrown. We only noticed later when we were investigatin an issue this lead to,

In short - I think we should not swallow exceptions when parsing response body. I'm not sure if my way (removing the try-catch) is the best, but it was good enough for our usecase.

Please suggest if this makes sense or do you prefer some other solution.

kramer pushed a commit that referenced this pull request Mar 6, 2015
…on to the failed method of provided handler. (partially related to #185)
@kramer
Copy link
Member

kramer commented Mar 6, 2015

Technically that exception is not swallowed but just not propagated (since it's logged in error level) 😛 but you have a point. This was done because back then ES sent non-json responses for 4xx responses to some actions (and we didn't want to throw an exception for those because "not found" is a valid response for say IndicesExists) - see #46 for details. Since ES now seems to return proper json bodies within not found responses, we can revert this behaviour.

Please fix the failing tests and consider using @Test(expected = ) instead of @Rule before I can merge this.

Thank you for your contribution!

@igor-kupczynski
Copy link
Contributor Author

Yes, it makes sense now, I was not aware about the history.

Four tests were failing:

  • SearchIntegrationTest#searchWithQueryBuilder had a bug in the test itself -- ES returned a message "no handler for XYZ", it got unnoticed because of this non-propagation ;-)
  • NodesHotThreads were harder to fix, as a response to this action is not json and this is intended. I've ended up with making AbstractAction#convertJsonStringToMapObject instance and not static method and overrided it NodeHotThreads

Please let me know what do you think.

Thanks,
Igor

@kramer kramer changed the title AbstractAction#convertJsonStringToMapObject swallows exceptions Deserialization process should not swallow exceptions Mar 7, 2015
kramer pushed a commit that referenced this pull request Mar 7, 2015
Deserialization process should not swallow exceptions
@kramer kramer merged commit df7a222 into searchbox-io:master Mar 7, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants