Deprecate types in update_by_query and delete_by_query #36365
Conversation
|
Pinging @elastic/es-search |
There was a problem hiding this comment.
Thanks @mayya-sharipova, this looks good to me overall! I left some small comments, and also noted that it would be good to use the changes in #36328.
| // checking here for a deprecation from its internal search request | ||
| assertWarnings(RestSearchAction.TYPES_DEPRECATION_MESSAGE); | ||
| } | ||
|
|
||
| public void testParseEmpty() throws IOException { | ||
| RestDeleteByQueryAction action = new RestDeleteByQueryAction(Settings.EMPTY, mock(RestController.class)); |
There was a problem hiding this comment.
Small comment, but maybe we could refactor to use the same RestDeleteByQueryAction that we've created in setUp.
| @@ -89,7 +89,9 @@ public DeleteByQueryRequest setQuery(QueryBuilder query) { | |||
|
|
|||
| /** | |||
| * Set the document types for the delete | |||
| * @deprecated Types are in the process of being removed. | |||
There was a problem hiding this comment.
Super small comment, but maybe we can mention 'Instead of using a type, prefer to filter on a field on the document' (as in https://github.com/elastic/elasticsearch/blob/master/server/src/main/java/org/elasticsearch/action/search/SearchRequest.java#L221)?
| @@ -512,8 +512,9 @@ private static Request prepareReindexRequest(ReindexRequest reindexRequest, bool | |||
| } | |||
|
|
|||
| static Request updateByQuery(UpdateByQueryRequest updateByQueryRequest) throws IOException { | |||
| String endpoint = | |||
| endpoint(updateByQueryRequest.indices(), updateByQueryRequest.getDocTypes(), "_update_by_query"); | |||
| String endpoint = updateByQueryRequest.isNoTypeRequest() | |||
There was a problem hiding this comment.
I don't think we need a special check for no types here, because if the types array is empty, the logic in endpoint will just skip that path component. This is nice, because it lets us avoid introducing the additional isNoTypeRequest methods.
|
|
||
| import java.io.IOException; | ||
| import java.util.Collections; | ||
|
|
||
| import static java.util.Collections.emptyList; | ||
| import static org.mockito.Mockito.mock; | ||
|
|
||
| public class RestDeleteByQueryActionTests extends ESTestCase { |
There was a problem hiding this comment.
I just noticed we don't have any REST test coverage for using delete- and update-by-query with types. It might be nice to add a test case here and in RestUpdateByQueryActionTests that checks the type in the URL is propagated correctly to the request object? That way we have some minimal coverage to be sure this doesn't break.
Also, I just merged #36328, which should help cut down on boilerplate in these tests.
There was a problem hiding this comment.
@jtibshirani Thanks Julie. What "request object" do you have in mind here? Is it RestRequest? Does this line introduced in my last commit is enough for testing:
assertEquals("some_type", request.param("type"));
There was a problem hiding this comment.
I was thinking we could have a separate test for request building where we call action.buildRequest(...) and check that the return value (like DeleteByQueryRequest) contains the right document types.
There was a problem hiding this comment.
@jtibshirani got it, thanks Julie. I have addressed this in my last commit.
|
@elasticmachine test this please |
|
@jtibshirani Thanks for the review. I have tried to address your comments. Can you please continue the review when you have time |
Relates to #35190