Add delete API to the High Level Rest Client by dadoonet · Pull Request #23187 · elastic/elasticsearch

dadoonet · 2017-02-15T15:03:37Z

No description provided.

dadoonet · 2017-02-15T15:04:54Z

client/rest-high-level/src/test/java/org/elasticsearch/client/CrudIT.java

+            assertEquals("type", deleteResponse.getType());
+            assertEquals("does_not_exist", deleteResponse.getId());
+            assertEquals(DocWriteResponse.Result.NOT_FOUND, deleteResponse.getResult());
+            assertEquals(1, deleteResponse.getVersion());


This is surprising to me. I'd expect it to be -1 similar to the GET request

I think Get request returns an error with NOT FOUND response code in this case? Which make more sense to me.

Anyway a delete increments the version even if the document is not found, and is kept in the versions map as a tombstone.

# Conflicts: # client/rest-high-level/src/main/java/org/elasticsearch/client/Request.java # client/rest-high-level/src/test/java/org/elasticsearch/client/CrudIT.java

tlrx · 2017-02-15T16:14:29Z

client/rest-high-level/src/main/java/org/elasticsearch/client/Request.java

        return new Request(method, endpoint, parameters.getParams(), entity);
    }

+    static Request delete(DeleteRequest deleteRequest) {


Can you please move this up so that methods are somewhat alphabetically sorted?

Ok. I'm also moving ping method. And add a comments for coming developers

tlrx · 2017-02-15T16:15:20Z

client/rest-high-level/src/main/java/org/elasticsearch/client/RestHighLevelClient.java

    }

+    /**
+     * Deletes a document by id using the delete api


"Deletes a document using the Delete API" with a link to the documentation like it is done for other methods

Right. I did not notice the modifications when I merged the master branch into my branch.

tlrx · 2017-02-16T09:42:30Z

client/rest-high-level/src/main/java/org/elasticsearch/client/RestHighLevelClient.java

+    /**
+     * Asynchronously deletes a document by id using the delete api
+     */
+    public void deleteAsync(DeleteRequest deleteRequest, ActionListener<DeleteResponse> listener, Header... headers) {


Same here for javadoc

tlrx · 2017-02-16T09:49:37Z

client/rest-high-level/src/test/java/org/elasticsearch/client/CrudIT.java

+            assertEquals("index", deleteResponse.getIndex());
+            assertEquals("type", deleteResponse.getType());
+            assertEquals("id", deleteResponse.getId());
+            assertEquals(DocWriteResponse.Result.DELETED, deleteResponse.getResult());


I don't see tests for routing, parent, version type etc. That would be nice to check that parameters are correctly propagated.

tlrx · 2017-02-16T09:50:10Z

client/rest-high-level/src/test/java/org/elasticsearch/client/CrudIT.java

+        String document = "{\"field1\":\"value1\",\"field2\":\"value2\"}";
+        StringEntity stringEntity = new StringEntity(document, ContentType.APPLICATION_JSON);
+        Response response = client().performRequest("PUT", "/index/type/id", Collections.singletonMap("refresh", "wait_for"), stringEntity);
+        assertEquals(201, response.getStatusLine().getStatusCode());


I think we could start using our own high level rest client for those? Now we have Index API support...

tlrx · 2017-02-16T10:05:25Z

client/rest-high-level/src/test/java/org/elasticsearch/client/CrudIT.java

        }
    }
+
+    public void testDelete() throws IOException {


Can you move this up so that it is alphabetically sorted?

tlrx · 2017-02-16T10:22:16Z

client/rest-high-level/src/test/java/org/elasticsearch/client/CrudIT.java

+            assertEquals("type", deleteResponse.getType());
+            assertEquals("does_not_exist", deleteResponse.getId());
+            assertEquals(DocWriteResponse.Result.NOT_FOUND, deleteResponse.getResult());
+            assertEquals(1, deleteResponse.getVersion());


I think Get request returns an error with NOT FOUND response code in this case? Which make more sense to me.

Anyway a delete increments the version even if the document is not found, and is kept in the versions map as a tombstone.

* Sort alphabetically method names * Add javadoc link to the ref guide * Add more tests about routing, version and version type

dadoonet · 2017-02-17T10:46:54Z

Thanks @tlrx for the review!

I pushed a new commit to address your comments. LMK.

javanna

left a few comments LGTM otherwise

javanna · 2017-02-17T17:08:44Z

client/rest-high-level/src/main/java/org/elasticsearch/client/Request.java


-    static Request ping() {
-        return new Request("HEAD", "/", Collections.emptyMap(), null);
+    // For developers please add your new methods in the alphabetical order


I think this comment won't be enough to enforce this convention... why does order matter anyways @tlrx ?

Agreed. But at least as I was not aware of this convention and did not find it in code comments I added it.
Any suggestion? Remove the comment?

@javanna It doesn't matter, that's not a convention and it doesn't make sense to add a comment for that. I just find it easier to find and read class methods when they are sorted, specially when it's a suite of operations like this. But if I'm the only one to care about this, let's add new method wherever you want.

ok let's remove the comment.

javanna · 2017-02-17T17:14:42Z

client/rest-high-level/src/test/java/org/elasticsearch/client/CrudIT.java

+     * @return A sample index request
+     * @throws IOException If problem when generating the content
+     */
+    private IndexRequest createSampleIndexRequest() throws IOException {


do we need a method for this? don't we always do createSampleDocument(createSampleIndexRequest))?

I also wonder if it's ok to always rely on auto-generated ids. maybe we should provide our ids instead.

don't we always do createSampleDocument(createSampleIndexRequest))?

No. We also do:

createSampleDocument(createSampleIndexRequest().routing("foo"));

See

elasticsearch/client/rest-high-level/src/test/java/org/elasticsearch/client/CrudIT.java

Line 112 in 5aa2ab3

String docId = createSampleDocument(createSampleIndexRequest().routing("foo"));

I also wonder if it's ok to always rely on auto-generated ids. maybe we should provide our ids instead.

Why not?

note that sometimes I'm doing:

createSampleIndexRequest().id(randomAsciiOfLength(15));

Where I basically force the id. So I'd say that we have both covered.

that's ok. anyways we are testing delete here.

So I'm now using my own ids for every test.

javanna · 2017-02-17T17:18:39Z

client/rest-high-level/src/main/java/org/elasticsearch/client/Request.java

+        parameters.withRefreshPolicy(deleteRequest.getRefreshPolicy());
+        parameters.withWaitForActiveShards(deleteRequest.waitForActiveShards());
+
+        return new Request(HttpDelete.METHOD_NAME, endpoint, parameters.getParams(), null);


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

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.

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.

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")); }

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

Thanks for the pointer. I did that in RequestTests.

javanna · 2017-02-20T08:45:42Z

client/rest-high-level/src/test/java/org/elasticsearch/client/CrudIT.java

+    private IndexRequest createSampleIndexRequest() throws IOException {
+        final XContentType xContentType = randomFrom(XContentType.values());
+        IndexRequest indexRequest = new IndexRequest("index", "type");
+        indexRequest.source(XContentBuilder.builder(xContentType.xContent()).startObject().field("test", "test").endObject());


if this is only used for testing deletions, I don't think we need to randomize the content type when indexing. Let's use json? we can also provide it as a string.

Agreed. I simplified all that.

dadoonet · 2017-02-20T09:17:43Z

@javanna @tlrx I think I addressed your comments. Wanna review one more time?

javanna · 2017-02-20T09:21:21Z

client/rest-high-level/src/test/java/org/elasticsearch/client/CrudIT.java

+     * @throws IOException if something goes wrong while executing the index request
+     */
+    private void createSampleDocument(IndexRequest request) throws IOException {
+        IndexResponse indexResponse = execute(request, highLevelClient()::index, highLevelClient()::indexAsync);


one more thing, given that this is used only to test deletions, let's just use the sync version of the method rather than randomizing. I do wonder whether this additional method is needed.

If we do only the sync method, then we can definitely remove that method IMO. I'll do that.

javanna · 2017-02-20T09:23:18Z

client/rest-high-level/src/test/java/org/elasticsearch/client/RequestTests.java

+            expectedParams.put("timeout", ReplicationRequest.DEFAULT_TIMEOUT.getStringRep());
+        }
+
+        if (frequently()) {


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

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

javanna · 2017-02-20T09:24:23Z

client/rest-high-level/src/test/java/org/elasticsearch/client/RequestTests.java

+                    expectedParams.put("refresh", refreshPolicy.getValue());
+                }
+            }
+        }


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

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

dadoonet · 2017-02-20T22:38:23Z

@javanna @tlrx Any other thoughts?

tlrx

I left some more comments I'd like to see merged in. Otherwise it's ok, thanks @dadoonet

tlrx · 2017-02-21T09:06:43Z

client/rest-high-level/src/test/java/org/elasticsearch/client/CrudIT.java

+                () -> execute(deleteRequest, highLevelClient()::delete, highLevelClient()::deleteAsync));
+            assertEquals(RestStatus.CONFLICT, exception.status());
+            assertEquals("Elasticsearch exception [type=version_conflict_engine_exception, " +
+                "reason=[type][" + docId + "]: " +


The format of the line seems a bit weird

tlrx · 2017-02-21T09:09:16Z

client/rest-high-level/src/test/java/org/elasticsearch/client/RequestTests.java

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


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

tlrx · 2017-02-21T09:13:13Z

client/rest-high-level/src/test/java/org/elasticsearch/client/RequestTests.java

    }
-}
+
+    private void enrichReplicationRequest(ReplicatedWriteRequest request, Map<String, String> expectedParams) {


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

tlrx · 2017-02-21T09:13:57Z

client/rest-high-level/src/test/java/org/elasticsearch/client/RequestTests.java

+        }
+    }
+
+    private void enrichDocWriteRequest(DocWriteRequest request, Map<String, String> expectedParams) {


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

tlrx · 2017-02-21T09:16:13Z

client/rest-high-level/src/test/java/org/elasticsearch/client/RequestTests.java

+
+    private void enrichDocWriteRequest(DocWriteRequest request, Map<String, String> expectedParams) {
+        // There is some logic around _create endpoint and version/version type
+        if (request.opType() == DocWriteRequest.OpType.CREATE) {


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); ... }

elasticmachine · 2017-02-23T18:12:40Z

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

javanna · 2017-02-23T18:37:24Z

@dadoonet can you merge this please? it's ready no?

dadoonet · 2017-02-23T19:12:30Z

I hope doing it tomorrow morning CET.

# Conflicts: # client/rest-high-level/src/main/java/org/elasticsearch/client/Request.java # client/rest-high-level/src/main/java/org/elasticsearch/client/RestHighLevelClient.java # client/rest-high-level/src/test/java/org/elasticsearch/client/CrudIT.java # client/rest-high-level/src/test/java/org/elasticsearch/client/RequestTests.java

dadoonet · 2017-02-24T08:21:03Z

Thanks Tanguy. That makes the code much more readable. I like it!

I think I should now have one more step which is to try to use all the randomXXX() methods I have to the other tests.
WDYT?

tlrx · 2017-02-24T10:42:11Z

@dadoonet I think this can go in. Feel free to create a follow up pull request to apply randomVersion() etc to other tests.

Add delete API to the High Level Rest Client

7df1df2

dadoonet added :Java High Level REST Client v6.0.0-alpha1 labels Feb 15, 2017

dadoonet self-assigned this Feb 15, 2017

dadoonet commented Feb 15, 2017

View reviewed changes

dadoonet added 2 commits February 15, 2017 16:08

Merge branch 'master' into hlclient/add-delete-method

0bbcd94

# Conflicts: # client/rest-high-level/src/main/java/org/elasticsearch/client/Request.java # client/rest-high-level/src/test/java/org/elasticsearch/client/CrudIT.java

Merge with master

14f0490

dadoonet requested a review from tlrx February 16, 2017 08:59

tlrx requested changes Feb 16, 2017

View reviewed changes

clintongormley added the >enhancement label Feb 16, 2017

Fix after 1st review

5aa2ab3

* Sort alphabetically method names * Add javadoc link to the ref guide * Add more tests about routing, version and version type

javanna approved these changes Feb 17, 2017

View reviewed changes

Remove a non needed comment

b2ec4c1

javanna reviewed Feb 20, 2017

View reviewed changes

dadoonet added 2 commits February 20, 2017 09:56

Check that we correctly propagate delete request parameters

efa28e0

Remove createSampleIndexRequest method and provide ids

0ee74f9

javanna reviewed Feb 20, 2017

View reviewed changes

dadoonet added 2 commits February 20, 2017 10:33

Remove createSampleDocument method and use the sync'ed index method

bf8241e

delete and index tests can share some part of the code

9bde68e

tlrx approved these changes Feb 21, 2017

View reviewed changes

dadoonet added 2 commits February 24, 2017 08:58

Fix after last merge with master and apply last comments

4ebc6dd

Merge branch 'master' into hlclient/add-delete-method

3e4b917

dadoonet merged commit 3e4b917 into elastic:master Feb 24, 2017

dadoonet deleted the hlclient/add-delete-method branch February 24, 2017 10:57

javanna mentioned this pull request Jun 6, 2017

Backport high level REST client #25086

Merged

javanna mentioned this pull request Nov 1, 2017

Java high-level REST client completeness #27205

Closed

80 tasks

Conversation

dadoonet commented Feb 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dadoonet commented Feb 17, 2017

Uh oh!

javanna left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dadoonet Feb 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dadoonet commented Feb 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dadoonet Feb 20, 2017 •

edited

Loading