Deprecation warnings break the high-level client in strict mode #37090
Comments
|
Pinging @elastic/es-core-features |
javanna
changed the title
|
I did some investigation here and came up with the following. if we just use a new exception, you end up seeing sync throwing that exception and async returning the following I do not think this is optimal as we have the entity there as well. we can parse the entity if it comes back valid, and I would like to give that back to the user. We cannot create an exception with the generic Response So we could 1) do what I have done above w/ just a new exception w/o any parsing of the response entity, 2) make all of our responses extend something that we can pull out warnings and return that instead of throwing an exception (this is the opposite of strict), or 3) we can throw an exception that just has an or hopefully 4) something i have not yet seen that makes this easier. |
|
After discussing with the team, a new option was presented. the new option, which would require some rework in the LLRC, is to have the HLRC be in charge of warnings handling if you are using the HLRC. So if someone who is using the HLRC wants to set strict, they would set it in the HLRC and not in the LLRC. This would mean requiring the final value set in the LLRC to be mutable, or the HLRC constructor that takes in a LLRC will fail in this case. The advantages are that LLRC would not throw exceptions and that HLRC can handle them without throwing this Disadvantage is that you can still get the LLRC from I will do a bit of coding on this to see if its worth investigating further because im still not 100% sold on it, and it does not really make the "throw exception but still hold the state of the response" portion of the original issue any easier. |
|
after looking things over and more discussion, this still does not solve the issue of "should we be parsing the response and throwing the exception." If we want to call |
The LLRC's exception handling for strict mode was previously throwing an exception the HLRC assumed was an error response. This is not the case if the result is valid in strict mode, as it will return the proper response wrapped in an exception with warnings. This commit fixes the HLRC such that it no longer spews if it encounters a strict LLRC response. Closes elastic#37090
|
ok, lots of chatting with myself and I end up just throwing the original ElasticsearchStatusException wrapped with the new exception such that it does not try to parse it and blow up. This is the best that I could do given the constraints. |
The LLRC's exception handling for strict mode was previously throwing an exception the HLRC assumed was an error response. This is not the case if the result is valid in strict mode, as it will return the proper response wrapped in an exception with warnings. This commit fixes the HLRC such that it no longer spews if it encounters a strict LLRC response. Closes #37090
The LLRC's exception handling for strict mode was previously throwing an exception the HLRC assumed was an error response. This is not the case if the result is valid in strict mode, as it will return the proper response wrapped in an exception with warnings. This commit fixes the HLRC such that it no longer spews if it encounters a strict LLRC response. Closes elastic#37090 Relates elastic#37247
The LLRC's exception handling for strict mode was previously throwing an exception the HLRC assumed was an error response. This is not the case if the result is valid in strict mode, as it will return the proper response wrapped in an exception with warnings. This commit fixes the HLRC such that it no longer spews if it encounters a strict LLRC response. Closes #37090 Relates #37247
When the high-level client uses a low-level client that fails on deprecation warnings, that breaks the high-level client quite badly. The low-level client throws a
ResponseExceptionwhich is caught by the high-level client which tries to parse the response body as if it was an error, although it most likely was not. Instead of signaling the deprecation warnings clearly, the high-level client ends up breaking with:The cause of the failure does contain the deprecation warnings, but it is not so obvious that they did cause the exception to be thrown. The problem here is that a ResponseException gets thrown based on a response that returned OK status code, and no additional exception message is provided to signal what the problem was. The response itself does not necessarily tell the reason of the failure. That same response would not cause any exception with strict deprecation disabled. I believe we should use a different exception for deprecation warnings.
The text was updated successfully, but these errors were encountered: