Deprecate types in update requests. #36181
Conversation
jtibshirani
added
:Search Foundations/Mapping
|
Pinging @elastic/es-search |
jtibshirani
force-pushed
the
deprecate-types-in-update
branch
3 times, most recently
from
71ce02f to
faed06f
Compare
mayya-sharipova
left a comment
mayya-sharipova
left a comment
There was a problem hiding this comment.
Thanks Julie! Just a couple of small comments
There was a problem hiding this comment.
Should it be here RestUpdateAction instead of RestSearchAction?
There was a problem hiding this comment.
Yes, thanks! I have been doing a lot of copy-paste.
There was a problem hiding this comment.
Do we want to use deprecatedAndMaybeLog instead of deprecated not to log same deprecation warnings many times?
There was a problem hiding this comment.
Thanks a lot, I hadn't seen this method. I think we should use it more generally for types deprecation -- I will go over and update some of our other logging statements as well.
There was a problem hiding this comment.
<2> Document id instead of <3> Document id>?
There was a problem hiding this comment.
Is it worth keeping a deprecated example usage in this test?
I think there's some work to be done to make ESRestTestCase support assertWarnings checks first (currently have to use getStrictDeprecationMode=false to avoid test failures when making deprecated calls)
There was a problem hiding this comment.
Will do, this shouldn't be hard now that we are able to check warnings in REST integration tests.
There was a problem hiding this comment.
Also remove String type = updateResponse.getType(); example from line 377?
There was a problem hiding this comment.
^^ methods testCreate, testIndexWithId and testIndexWithoutId above still going with "test" doc type. Is this intentional?
There was a problem hiding this comment.
It wasn't intentional. I switched them to _doc when deprecating get requests, so this should be fixed.
There was a problem hiding this comment.
I found I had to do this too and didn't like it. EsRestTestCase has a "fail or nothing" approach to testing responses.
Either the whole test fails (strictDeprecationMode is too strict) or in the alternative lax mode we blindly accept any warnings as ok - we can't check we got the expected warnings because ESRestTestCase does not work with the assertWarnings feature declared in base class ESTestCase. I'm going to look into adding that support to ESRestTestCase. Opened #36251
There was a problem hiding this comment.
👍 thanks for this work, I'll avoid using getStrictDeprecationMode.
There was a problem hiding this comment.
Test the undeprecated form too?
|
Thanks @mayya-sharipova and @markharwood for the detailed reviews, and sorry I took so long to respond. I was juggling quite a few of these at once and decided it was best to focus on one at a time. I'm working on bringing the PR up to date and will try to address your comments. |
jtibshirani
force-pushed
the
deprecate-types-in-update
branch
from
faed06f to
23aeeff
Compare
|
This should now be ready for another look. |
|
@jtibshirani thanks Julie, I had another look. Everything looks good to me. |
5 participants
The following updates were made:
RestUpdateAction, plus a test inRestUpdateActionTests.Because of an earlier PR, the REST yml tests were already updated (one version without types, and another legacy version that retains types).