Deprecate types in termvector and mtermvector requests. #36182
Conversation
jtibshirani
added
:Search Foundations/Mapping
|
Pinging @elastic/es-search |
jtibshirani
force-pushed
the
deprecate-types-in-termvectors
branch
from
a6f9c4b
to
9516344
Compare
| "ids" : ["testing_document"] | ||
|
|
||
| - match: {docs.0.term_vectors.text.terms.brown.term_freq: 2} | ||
| - match: {docs.0.term_vectors.text.terms.brown.ttf: 2} |
There was a problem hiding this comment.
What is the expected behaviour when docs are stored with a custom type but queried via a typeless api?
Not sure there's a test for that here? (Ignore me - I've just seen that overlaps with #35790 )
There was a problem hiding this comment.
👍 the tests in that PR should cover this case.
| @@ -60,6 +63,8 @@ | |||
| * required. | |||
| */ | |||
| public class TermVectorsRequest extends SingleShardRequest<TermVectorsRequest> implements RealtimeRequest { | |||
| private static final DeprecationLogger deprecationLogger = new DeprecationLogger( | |||
| LogManager.getLogger(MultiTermVectorsRequest.class)); | |||
|
|
|||
There was a problem hiding this comment.
Cut-and-paste code has wrong class?
There was a problem hiding this comment.
Oops, thanks for catching this!
| @@ -621,6 +626,7 @@ public static void parseRequest(TermVectorsRequest termVectorsRequest, XContentP | |||
| termVectorsRequest.index = parser.text(); | |||
| } else if (TYPE.match(currentFieldName, parser.getDeprecationHandler())) { | |||
| termVectorsRequest.type = parser.text(); | |||
| deprecationLogger.deprecated(RestMultiTermVectorsAction.TYPES_DEPRECATION_MESSAGE); | |||
| } else if (ID.match(currentFieldName, parser.getDeprecationHandler())) { | |||
There was a problem hiding this comment.
cut-and-paste code with wrong message?
There was a problem hiding this comment.
I'll update this too, as I think it's clearer to use RestTermVectorsAction here. I initially used the multi termvectors message because this method is also used in parsing multi termvector requests.
|
LGTM aside from logging comment and I see tests fail due to TermVectorsUnitTests missing a couple of |
|
@elasticmachine run default distro tests |
|
@elasticmachine run gradle build tests 2 |
jtibshirani
changed the title
The following updates were made:
Rest*TermVectorsAction, plus tests inRest*TermVectorsActionTests.I assumed here that we will be going with the endpoint format
{index}/_termvectors/{id}, but we should hold off on merging this until we've reached a final consensus.