Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add delete API to the High Level Rest Client #23187

Merged
merged 12 commits into from
Feb 24, 2017
Merged

Add delete API to the High Level Rest Client #23187

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
7df1df2
Add delete API to the High Level Rest Client
dadoonet Feb 15, 2017
0bbcd94
Merge branch 'master' into hlclient/add-delete-method
dadoonet Feb 15, 2017
14f0490
Merge with master
dadoonet Feb 15, 2017
5aa2ab3
Fix after 1st review
dadoonet Feb 17, 2017
b2ec4c1
Remove a non needed comment
dadoonet Feb 20, 2017
efa28e0
Check that we correctly propagate delete request parameters
dadoonet Feb 20, 2017
0ee74f9
Remove createSampleIndexRequest method and provide ids
dadoonet Feb 20, 2017
bf8241e
Remove createSampleDocument method and use the sync'ed index method
dadoonet Feb 20, 2017
9bde68e
delete and index tests can share some part of the code
dadoonet Feb 20, 2017
b680e62
Merge branch 'master' into hlclient/add-delete-method
dadoonet Feb 24, 2017
4ebc6dd
Fix after last merge with master and apply last comments
dadoonet Feb 24, 2017
3e4b917
Merge branch 'master' into hlclient/add-delete-method
dadoonet Feb 24, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 19 additions & 2 deletions client/rest-high-level/src/main/java/org/elasticsearch/client/Request.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
package org.elasticsearch.client;

import org.apache.http.HttpEntity;
import org.apache.http.client.methods.HttpDelete;
import org.apache.http.client.methods.HttpGet;
import org.apache.http.client.methods.HttpHead;
import org.apache.http.client.methods.HttpPost;
Expand All @@ -28,6 +29,7 @@
import org.apache.http.entity.ContentType;
import org.apache.lucene.util.BytesRef;
import org.elasticsearch.action.DocWriteRequest;
import org.elasticsearch.action.delete.DeleteRequest;
import org.elasticsearch.action.get.GetRequest;
import org.elasticsearch.action.index.IndexRequest;
import org.elasticsearch.action.support.ActiveShardCount;
Expand Down Expand Up @@ -69,8 +71,19 @@ public String toString() {
'}';
}

static Request ping() {
return new Request("HEAD", "/", Collections.emptyMap(), null);
static Request delete(DeleteRequest deleteRequest) {
String endpoint = endpoint(deleteRequest.index(), deleteRequest.type(), deleteRequest.id());

Params parameters = Params.builder();
parameters.withRouting(deleteRequest.routing());
parameters.withParent(deleteRequest.parent());
parameters.withTimeout(deleteRequest.timeout());
parameters.withVersion(deleteRequest.version());
parameters.withVersionType(deleteRequest.versionType());
parameters.withRefreshPolicy(deleteRequest.getRefreshPolicy());
parameters.withWaitForActiveShards(deleteRequest.waitForActiveShards());

return new Request(HttpDelete.METHOD_NAME, endpoint, parameters.getParams(), null);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should test this in RequestTests and have unit tests to make sure that all supported parameters are propagated.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you want to test the Params builder actually?

Something like:

    public void testWithParams() {
        Request.Params params = Request.Params.builder();
        params.withFetchSourceContext(FetchSourceContext.FETCH_SOURCE);
        params.withParent("parent");
        params.withPipeline("pipeline");
        params.withPreference("preference");
        params.withRealtime(true);
        if (randomBoolean()) {
            params.withRefresh(true);
        } else {
            params.withRefreshPolicy(WriteRequest.RefreshPolicy.WAIT_UNTIL);
        }
        params.withRouting("routing");
        params.withStoredFields(new String[]{"field1","field2"});
        params.withTimeout(TimeValue.timeValueMillis(100));
        params.withVersion(2);
        params.withVersionType(VersionType.EXTERNAL);
        params.withWaitForActiveShards(ActiveShardCount.ALL);
        Map<String, String> requestParams = params.getParams();
        assertEquals(10, requestParams.size());
    }

I can add it in the PR or later.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the above is that useful, I would rather like to make sure that the new method to generate a request calls all of the with* methods that need to be called and the params from map to request are all propagated.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I understand. So test something like the following?

    public void testDelete() {
        DeleteRequest deleteRequest = new DeleteRequest("index", "type", "id")
            .routing("routing");

        Request request = Request.delete(deleteRequest);
        assertEquals(true, request.params.containsKey("routing"));
    }

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes something along those lines. we have existing tests for this in RequestTests, see e.g. testIndex

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the pointer. I did that in RequestTests.

}

static Request exists(GetRequest getRequest) {
Expand Down Expand Up @@ -118,6 +131,10 @@ static Request index(IndexRequest indexRequest) {
return new Request(method, endpoint, parameters.getParams(), entity);
}

static Request ping() {
return new Request("HEAD", "/", Collections.emptyMap(), null);
}

/**
* Utility method to build request's endpoint.
*/
Expand Down
22 changes: 22 additions & 0 deletions client/rest-high-level/src/main/java/org/elasticsearch/client/RestHighLevelClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@
import org.elasticsearch.action.ActionListener;
import org.elasticsearch.action.ActionRequest;
import org.elasticsearch.action.ActionRequestValidationException;
import org.elasticsearch.action.delete.DeleteRequest;
import org.elasticsearch.action.delete.DeleteResponse;
import org.elasticsearch.action.get.GetRequest;
import org.elasticsearch.action.get.GetResponse;
import org.elasticsearch.action.index.IndexRequest;
Expand Down Expand Up @@ -121,6 +123,26 @@ public void indexAsync(IndexRequest indexRequest, ActionListener<IndexResponse>
performRequestAsyncAndParseEntity(indexRequest, Request::index, IndexResponse::fromXContent, listener, emptySet(), headers);
}

/**
* Deletes a document by id using the Delete api
*
* See <a href="https://www.elastic.co/guide/en/elasticsearch/reference/current/docs-delete.html">Delete API on elastic.co</a>
*/
public DeleteResponse delete(DeleteRequest deleteRequest, Header... headers) throws IOException {
return performRequestAndParseEntity(deleteRequest, Request::delete, DeleteResponse::fromXContent, Collections.singleton(404),
headers);
}

/**
* Asynchronously deletes a document by id using the Delete api
*
* See <a href="https://www.elastic.co/guide/en/elasticsearch/reference/current/docs-delete.html">Delete API on elastic.co</a>
*/
public void deleteAsync(DeleteRequest deleteRequest, ActionListener<DeleteResponse> listener, Header... headers) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here for javadoc

performRequestAsyncAndParseEntity(deleteRequest, Request::delete, DeleteResponse::fromXContent, listener,
Collections.singleton(404), headers);
}

private <Req extends ActionRequest, Resp> Resp performRequestAndParseEntity(Req request, Function<Req, Request> requestConverter,
CheckedFunction<XContentParser, Resp, IOException> entityParser, Set<Integer> ignores, Header... headers) throws IOException {
return performRequest(request, requestConverter, (response) -> parseEntity(response.getEntity(), entityParser), ignores, headers);
Expand Down
81 changes: 80 additions & 1 deletion client/rest-high-level/src/test/java/org/elasticsearch/client/CrudIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,11 @@
import org.apache.http.entity.ContentType;
import org.apache.http.entity.StringEntity;
import org.elasticsearch.ElasticsearchException;
import org.elasticsearch.ElasticsearchStatusException;
import org.elasticsearch.action.DocWriteRequest;
import org.elasticsearch.action.DocWriteResponse;
import org.elasticsearch.action.delete.DeleteRequest;
import org.elasticsearch.action.delete.DeleteResponse;
import org.elasticsearch.ElasticsearchStatusException;
import org.elasticsearch.action.get.GetRequest;
import org.elasticsearch.action.get.GetResponse;
import org.elasticsearch.action.index.IndexRequest;
Expand All @@ -44,6 +46,83 @@

public class CrudIT extends ESRestHighLevelClientTestCase {

public void testDelete() throws IOException {
{
// Testing non existing document
String docId = "does_not_exist";
DeleteRequest deleteRequest = new DeleteRequest("index", "type", docId);
DeleteResponse deleteResponse = execute(deleteRequest, highLevelClient()::delete, highLevelClient()::deleteAsync);
assertEquals("index", deleteResponse.getIndex());
assertEquals("type", deleteResponse.getType());
assertEquals(docId, deleteResponse.getId());
assertEquals(DocWriteResponse.Result.NOT_FOUND, deleteResponse.getResult());
}
{
// Testing deletion
String docId = "id";
highLevelClient().index(new IndexRequest("index", "type", docId).source(Collections.singletonMap("foo", "bar")));
DeleteRequest deleteRequest = new DeleteRequest("index", "type", docId);
if (randomBoolean()) {
deleteRequest.version(1L);
}
DeleteResponse deleteResponse = execute(deleteRequest, highLevelClient()::delete, highLevelClient()::deleteAsync);
assertEquals("index", deleteResponse.getIndex());
assertEquals("type", deleteResponse.getType());
assertEquals(docId, deleteResponse.getId());
assertEquals(DocWriteResponse.Result.DELETED, deleteResponse.getResult());
}
{
// Testing version conflict
String docId = "version_conflict";
highLevelClient().index(new IndexRequest("index", "type", docId).source(Collections.singletonMap("foo", "bar")));
DeleteRequest deleteRequest = new DeleteRequest("index", "type", docId).version(2);
ElasticsearchException exception = expectThrows(ElasticsearchException.class,
() -> execute(deleteRequest, highLevelClient()::delete, highLevelClient()::deleteAsync));
assertEquals(RestStatus.CONFLICT, exception.status());
assertEquals("Elasticsearch exception [type=version_conflict_engine_exception, " +
"reason=[type][" + docId + "]: " +
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The format of the line seems a bit weird

"version conflict, current version [1] is different than the one provided [2]]", exception.getMessage());
assertEquals("index", exception.getMetadata("es.index").get(0));
}
{
// Testing version type
String docId = "version_type";
highLevelClient().index(new IndexRequest("index", "type", docId).source(Collections.singletonMap("foo", "bar"))
.versionType(VersionType.EXTERNAL).version(12));
DeleteRequest deleteRequest = new DeleteRequest("index", "type", docId).versionType(VersionType.EXTERNAL).version(13);
DeleteResponse deleteResponse = execute(deleteRequest, highLevelClient()::delete, highLevelClient()::deleteAsync);
assertEquals("index", deleteResponse.getIndex());
assertEquals("type", deleteResponse.getType());
assertEquals(docId, deleteResponse.getId());
assertEquals(DocWriteResponse.Result.DELETED, deleteResponse.getResult());
}
{
// Testing version type with a wrong version
String docId = "wrong_version";
highLevelClient().index(new IndexRequest("index", "type", docId).source(Collections.singletonMap("foo", "bar"))
.versionType(VersionType.EXTERNAL).version(12));
ElasticsearchStatusException exception = expectThrows(ElasticsearchStatusException.class, () -> {
DeleteRequest deleteRequest = new DeleteRequest("index", "type", docId).versionType(VersionType.EXTERNAL).version(10);
execute(deleteRequest, highLevelClient()::delete, highLevelClient()::deleteAsync);
});
assertEquals(RestStatus.CONFLICT, exception.status());
assertEquals("Elasticsearch exception [type=version_conflict_engine_exception, reason=[type][" +
docId + "]: version conflict, current version [12] is higher or equal to the one provided [10]]", exception.getMessage());
assertEquals("index", exception.getMetadata("es.index").get(0));
}
{
// Testing routing
String docId = "routing";
highLevelClient().index(new IndexRequest("index", "type", docId).source(Collections.singletonMap("foo", "bar")).routing("foo"));
DeleteRequest deleteRequest = new DeleteRequest("index", "type", docId).routing("foo");
DeleteResponse deleteResponse = execute(deleteRequest, highLevelClient()::delete, highLevelClient()::deleteAsync);
assertEquals("index", deleteResponse.getIndex());
assertEquals("type", deleteResponse.getType());
assertEquals(docId, deleteResponse.getId());
assertEquals(DocWriteResponse.Result.DELETED, deleteResponse.getResult());
}
}

public void testExists() throws IOException {
{
GetRequest getRequest = new GetRequest("index", "type", "id");
Expand Down
113 changes: 76 additions & 37 deletions client/rest-high-level/src/test/java/org/elasticsearch/client/RequestTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,11 @@
import org.apache.http.HttpEntity;
import org.apache.http.entity.ByteArrayEntity;
import org.elasticsearch.action.DocWriteRequest;
import org.elasticsearch.action.delete.DeleteRequest;
import org.elasticsearch.action.get.GetRequest;
import org.elasticsearch.action.index.IndexRequest;
import org.elasticsearch.action.support.WriteRequest;
import org.elasticsearch.action.support.replication.ReplicatedWriteRequest;
import org.elasticsearch.action.support.replication.ReplicationRequest;
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.lucene.uid.Versions;
Expand Down Expand Up @@ -55,6 +57,36 @@ public void testGet() {
getAndExistsTest(Request::get, "GET");
}

public void testDelete() throws IOException {
String index = randomAsciiOfLengthBetween(3, 10);
String type = randomAsciiOfLengthBetween(3, 10);
String id = randomAsciiOfLengthBetween(3, 10);
DeleteRequest deleteRequest = new DeleteRequest(index, type, id);

Map<String, String> expectedParams = new HashMap<>();

enrichDocWriteRequest(deleteRequest, expectedParams);
enrichReplicationRequest(deleteRequest, expectedParams);

if (frequently()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

out of curiosity, why frequently and not e.g. half of the times?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy and paste from index test. I can change it in both places or only here. As you prefer.

if (randomBoolean()) {
String routing = randomAsciiOfLengthBetween(3, 10);
deleteRequest.routing(routing);
expectedParams.put("routing", routing);
}
if (randomBoolean()) {
String parent = randomAsciiOfLengthBetween(3, 10);
deleteRequest.parent(parent);
expectedParams.put("parent", parent);
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we can share some common parts between index and delete?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haha! Funny. I wanted to propose that but was unsure. I totally agree.


Request request = Request.delete(deleteRequest);
assertEquals("/" + index + "/" + type + "/" + id, request.endpoint);
assertEquals(expectedParams, request.params);
assertEquals("DELETE", request.method);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you check that the request entity is null as well?

}

public void testExists() {
getAndExistsTest(Request::exists, "HEAD");
}
Expand Down Expand Up @@ -185,34 +217,8 @@ public void testIndex() throws IOException {
}
}

// There is some logic around _create endpoint and version/version type
if (indexRequest.opType() == DocWriteRequest.OpType.CREATE) {
indexRequest.version(randomFrom(Versions.MATCH_ANY, Versions.MATCH_DELETED));
expectedParams.put("version", Long.toString(Versions.MATCH_DELETED));
} else {
if (randomBoolean()) {
long version = randomFrom(Versions.MATCH_ANY, Versions.MATCH_DELETED, Versions.NOT_FOUND, randomNonNegativeLong());
indexRequest.version(version);
if (version != Versions.MATCH_ANY) {
expectedParams.put("version", Long.toString(version));
}
}
if (randomBoolean()) {
VersionType versionType = randomFrom(VersionType.values());
indexRequest.versionType(versionType);
if (versionType != VersionType.INTERNAL) {
expectedParams.put("version_type", versionType.name().toLowerCase(Locale.ROOT));
}
}
}

if (randomBoolean()) {
String timeout = randomTimeValue();
indexRequest.timeout(timeout);
expectedParams.put("timeout", timeout);
} else {
expectedParams.put("timeout", ReplicationRequest.DEFAULT_TIMEOUT.getStringRep());
}
enrichDocWriteRequest(indexRequest, expectedParams);
enrichReplicationRequest(indexRequest, expectedParams);

if (frequently()) {
if (randomBoolean()) {
Expand All @@ -230,14 +236,6 @@ public void testIndex() throws IOException {
indexRequest.setPipeline(pipeline);
expectedParams.put("pipeline", pipeline);
}

if (randomBoolean()) {
WriteRequest.RefreshPolicy refreshPolicy = randomFrom(WriteRequest.RefreshPolicy.values());
indexRequest.setRefreshPolicy(refreshPolicy);
if (refreshPolicy != WriteRequest.RefreshPolicy.NONE) {
expectedParams.put("refresh", refreshPolicy.getValue());
}
}
}

XContentType xContentType = randomFrom(XContentType.values());
Expand Down Expand Up @@ -307,4 +305,45 @@ public void testEndpoint() {
assertEquals("/a/b/_create", Request.endpoint("a", "b", "_create"));
assertEquals("/a/b/c/_create", Request.endpoint("a", "b", "c", "_create"));
}
}

private void enrichReplicationRequest(ReplicatedWriteRequest request, Map<String, String> expectedParams) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd have different static methods setRandomTimeout(..) and setRandomRefreshPolicy(...)

if (randomBoolean()) {
String timeout = randomTimeValue();
request.timeout(timeout);
expectedParams.put("timeout", timeout);
} else {
expectedParams.put("timeout", ReplicationRequest.DEFAULT_TIMEOUT.getStringRep());
}

if (randomBoolean()) {
WriteRequest.RefreshPolicy refreshPolicy = randomFrom(WriteRequest.RefreshPolicy.values());
request.setRefreshPolicy(refreshPolicy);
if (refreshPolicy != WriteRequest.RefreshPolicy.NONE) {
expectedParams.put("refresh", refreshPolicy.getValue());
}
}
}

private void enrichDocWriteRequest(DocWriteRequest request, Map<String, String> expectedParams) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer to have two static methods setRandomVersion() and setRandomVersionType

// There is some logic around _create endpoint and version/version type
if (request.opType() == DocWriteRequest.OpType.CREATE) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This part should be in the testIndex() method because it is specific to Index requests, it could be:

public void testIndex()  {
    ...
    if (request.opType() == DocWriteRequest.OpType.CREATE) {
        ...
    } else {
        setRandomVersion(indexRequest, expectedParams);
        setRandomVersionType(indexRequest, expectedParams);
        ...
   }

request.version(randomFrom(Versions.MATCH_ANY, Versions.MATCH_DELETED));
expectedParams.put("version", Long.toString(Versions.MATCH_DELETED));
} else {
if (randomBoolean()) {
long version = randomFrom(Versions.MATCH_ANY, Versions.MATCH_DELETED, Versions.NOT_FOUND, randomNonNegativeLong());
request.version(version);
if (version != Versions.MATCH_ANY) {
expectedParams.put("version", Long.toString(version));
}
}
if (randomBoolean()) {
VersionType versionType = randomFrom(VersionType.values());
request.versionType(versionType);
if (versionType != VersionType.INTERNAL) {
expectedParams.put("version_type", versionType.name().toLowerCase(Locale.ROOT));
}
}
}
}
}