Stop returning "es." internal exception headers as http response headers#22703
Conversation
3817f0d to
2bc7547
Compare
tlrx
left a comment
There was a problem hiding this comment.
Thanks @javanna, I think this is heading in the right direction. I left some minor comments but we still need to figure out what to do with the TODOs :)
I do like the move of internal headers es.* to exception metadata and I share your concerns about serializing back a parsed exception... but that shouldn't happen, neither in core nor in the high level rest client.
For a long term, I think we should also drop the metadataToXcontent() method which can lead to name conflicts in the rendered xcontent and also adds confusion if we add a metadata class attribute.
There was a problem hiding this comment.
Maybe remove "instead" from the exception message?
There was a problem hiding this comment.
ok I will remove the TODO then
There was a problem hiding this comment.
I think it's OK here because the token is a value, not an array. But later in the loop we need to take care of arrays.
I think we can already do it for values and just ignore objects for now
There was a problem hiding this comment.
Are we testing this somewhere?
that's correct, I think that the problem is that those additional fields are printed at the same level as the metadata (previous es. headers). They should rather be in a different object that only subclasses can write, but that is a breaking change. That said I am not sure how many clients are able to parse our exceptions entirely given how difficult we made it for them :) |
|
@s1monw mind having a look too? |
…Exception and stop returning them as response headers Closes elastic#17593
…ptionTests or ExceptionSerializationTests
17ab7eb to
95b9979
Compare
95b9979 to
ea0ee3b
Compare
Move "es." internal headers to separate metadata set in
ElasticsearchExceptionand stop returning them as response headers. These headers are printed out as part of the response body already, they weren't meant to be sent back as response headers too. They were introduced to prevent having to add custom exceptions to our codebase whenever we need to throw an exception that has to hold some additional metadata thatElasticsearchExceptiondoesn't support out of the box. The header notion stays inElasticsearchExceptionbut only for what actually needs to be returned as response header (no "es." prefix).Also removed
ESExceptionTestsand moved its tests underElasticsearchExceptionTestsorExceptionSerializationTestsCloses #17593