Fix validation of close_pit request #88702
Conversation
dnhatn
added
>bug
:Search/Search
|
Pinging @elastic/es-search (Team:Search) |
|
Hi @dnhatn, I've created a changelog YAML for you. |
|
Hi Nhat. Just wondering if we can also safeguard the calling code (TransportAction) from similar mistakes in the future? Can be a separate PR |
|
@ywelsch I added a safeguard, then backed it out after checking other |
| final ActionRequestValidationException validationException; | ||
| try { | ||
| validationException = request.validate(); | ||
| } catch (Exception e) { |
There was a problem hiding this comment.
I am trying to understand this part of code. If assertion is always false, it means we will always throw AssertionError, right? So the code after assert false will never be executed?
Or are we checking assertions only during testing mode? And in runtime mode we don't execute assertions?
There was a problem hiding this comment.
@mayya-sharipova Here we fail with AssertionError in tests, but log a warning message and fail the request with an exception in production.
There was a problem hiding this comment.
@dnhatn Thanks for clarifying, I was also thinking the same.
There was a problem hiding this comment.
@dnhatn Thanks for the fix, was also educational for me to study your changes.
|
@ywelsch @mayya-sharipova Thanks for review. |
ClosePointInTimeRequest#validate should return a validation error for an invalid pit_id, but it throws an IAE instead. This mistake prevents close_point_in_time tasks of requests with empty pit_id from being removed.
ClosePointInTimeRequest#validate should return a validation error for an invalid pit_id, but it throws an IAE instead. This mistake prevents close_point_in_time tasks of requests with empty pit_id from being removed.
ClosePointInTimeRequest#validateshould return a validation error for an invalid pit_id, but it throws an IAE instead. This mistake prevents close_pit tasks of requests with empty pit_id from being removed.I also looked at other
ActionRequest#validateimplementations. They do not throw exceptions.