Deprecate uses of _type as a field name in queries #36503
Conversation
mayya-sharipova
added
v7.0.0
>deprecation
:Search Foundations/Mapping
|
Pinging @elastic/es-search |
mayya-sharipova
force-pushed
the
deprecate_reference_to_types_in_match_query
branch
from
d9b7837 to
0947bad
Compare
There was a problem hiding this comment.
Thanks @mayya-sharipova, the overall approach seems good to me! I left a few small comments.
| @@ -823,10 +823,9 @@ public void testReindex() throws Exception { | |||
| // tag::reindex-request-conflicts | |||
| request.setConflicts("proceed"); // <1> | |||
| // end::reindex-request-conflicts | |||
| // tag::reindex-request-typeOrQuery | |||
There was a problem hiding this comment.
I wonder if we should remove these reindex-related changes, and tackle them in a dedicated PR? I think there may be more updates to track down around the reindex API.
There was a problem hiding this comment.
@jtibshirani Thanks Julie. The only reason I added these changes into this PR is because request.setSourceDocTypes("_doc"); underneath uses a query with a type that we are deprecating in this PR, and if I don't remove this line, CRUDDocumentationIT. :: testReindex fails.
There was a problem hiding this comment.
Thanks, I missed that!
| @@ -75,12 +75,18 @@ public void testFromJson() throws IOException { | |||
| @Override | |||
| public void testToQuery() throws IOException { | |||
| super.testToQuery(); | |||
| assertWarnings("The [type] query is deprecated, filter on a field instead."); | |||
| assertWarnings( | |||
| "The [type] query is deprecated, filter on a field instead.", | |||
There was a problem hiding this comment.
Maybe we could avoid these duplicate warning messages? One idea:
- We generalize the message in
TypeFieldType.TYPES_DEPRECATION_MESSAGEto say "[types removal] Referring to types within search queries is deprecated, filter on a field instead." - We make sure to use
deprecatedAndMaybeLoginTypeQueryBuilderto avoid producing the warning twice.
| @@ -124,6 +125,7 @@ public boolean isSearchable() { | |||
|
|
|||
| @Override | |||
| public Query existsQuery(QueryShardContext context) { | |||
There was a problem hiding this comment.
To be really comprehensive, we could also override the other query types (fuzzy, prefix, wildcard and regex) and issue deprecation warnings.
There was a problem hiding this comment.
Thanks Julie. Turns out that fuzzy, prefix and regex query all fail on the _type field, as the field is not indexed. So these kind of requests are not allowed. wildcard query is calling termQuery underneath, so we should get an expected deprecation message.
| public void testRangeQuery() { | ||
| QueryShardContext context = Mockito.mock(QueryShardContext.class); | ||
| MapperService mapperService = Mockito.mock(MapperService.class); | ||
| Mockito.when(mapperService.documentMapper()).thenReturn(null); |
There was a problem hiding this comment.
I think we can remove this line, since it's superseded by Mockito.when(mapperService.documentMapper()).thenReturn(mapper); below.
|
@jtibshirani Thanks for the feedback, Julie. I have tried to address your feedback, please continue to review when you have available time. |
mayya-sharipova
changed the title
|
@elasticmachine test this please |
|
From your most recent change, I saw that we'll now be emitting two deprecation warnings for some APIs. For example with 'explain', we'll have one for types in the explain URL, and one for the use of a type filter in search. I originally liked your approach because it colocates many deprecations into the same class It might be good to get @markharwood or @cbuescher's thoughts here as well. |
|
@jtibshirani Julie, sorry I have noticed your comment after the merging. Let's discuss it tomorrow. |
|
@jtibshirani I tried to investigate why an explain request on all docs needs to set up a filter on type. This happens here: I wonder if we can get rid of setting filter on type now that we have only a single type on all indexes. Also, because we call this terms query on type internally, we don't go through |
|
I don't think we can remove type filters altogether in 7.0, since users may be searching across multiple indices (containing multiple distinct types). However, we should be able to remove them in 8.0, once types are no longer supported. I had a simple idea for an alternative -- I'll upload a PR so that we can discuss it. An update: I opened #36802 with the alternate approach. |
Relates to #35190