[HLRC] Add support for Delete role mapping API by bizybot · Pull Request #34531 · elastic/elasticsearch

bizybot · 2018-10-16T15:27:21Z

Building on expression dsl and create role mapping API, this
commit adds support for delete role mapping API to high level client.

Building on expression dsl and create role mapping API, this commit adds support for delete role mapping API to high level client.

elasticmachine · 2018-10-16T15:27:24Z

Pinging @elastic/es-core-infra

elasticmachine · 2018-10-16T15:27:25Z

Pinging @elastic/es-security

bizybot · 2018-10-16T20:39:54Z

@elasticmachine test this please

jkakavas · 2018-10-17T11:10:57Z

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

+     * Delete a role mapping.
+     * See <a href="https://www.elastic.co/guide/en/elasticsearch/reference/current/security-api-delete-role-mapping.html">
+     * the docs</a> for more.
+     * @param request the request with the role mapping information to be deleted.


s/role mapping information/role mapping name/

Done, Thank you.

jkakavas · 2018-10-17T11:12:06Z

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

+     * Asynchronously delete a role mapping.
+     * See <a href="https://www.elastic.co/guide/en/elasticsearch/reference/current/security-api-delete-role-mapping.html">
+     * the docs</a> for more.
+     * @param request the request with the role mapping information to be deleted.


s/role mapping information/role mapping name/

Done, Thank you.

...est-high-level/src/main/java/org/elasticsearch/client/security/DeleteRoleMappingRequest.java

jkakavas · 2018-10-17T11:17:58Z

...est-high-level/src/main/java/org/elasticsearch/client/security/DeleteRoleMappingRequest.java

+    private final RefreshPolicy refreshPolicy;
+
+    public DeleteRoleMappingRequest(final String name, @Nullable final RefreshPolicy refreshPolicy) {
+        this.name = Objects.requireNonNull(name, "role-mapping name is missing");


I'd change the message to role mapping name is required for consistency with other mandatory fields in API requests

Done, Thank you.

...est-high-level/src/main/java/org/elasticsearch/client/security/DeleteRoleMappingRequest.java

...st-high-level/src/main/java/org/elasticsearch/client/security/DeleteRoleMappingResponse.java

jkakavas · 2018-10-17T11:49:18Z

...est-high-level/src/main/java/org/elasticsearch/client/security/DeleteRoleMappingRequest.java

+    private final RefreshPolicy refreshPolicy;
+
+    public DeleteRoleMappingRequest(final String name, @Nullable final RefreshPolicy refreshPolicy) {
+        this.name = Objects.requireNonNull(name, "role-mapping name is missing");


This doesn't handle empty Strings. We could implement validate() and use StringUtils.hasText()

I'd validate in the constructor. You can use Strings.hasText to check and then throw an exception

Done, Thank you.

...igh-level/src/test/java/org/elasticsearch/client/security/DeleteRoleMappingRequestTests.java

jkakavas · 2018-10-17T11:55:22Z

docs/java-rest/high-level/security/delete-role-mapping.asciidoc

+[[java-rest-high-security-delete-role-mapping-response]]
+==== Response
+The returned `DeleteRoleMappingResponse` contains a single field, `found`. If the mapping
+is successfully found and deleted, the request returns true. Otherwise, found is set to false.


If the mapping is successfully found and deleted, the request returns true. Otherwise, found is set to false.

Maybe change to

If the mapping is successfully found and deleted, found is set to true. Otherwise, it is set to false.

Corrected it, Thanks.

jkakavas · 2018-10-17T11:57:01Z

docs/java-rest/high-level/security/delete-role-mapping.asciidoc

+--------------------------------------------------
+include-tagged::{doc-tests}/SecurityDocumentationIT.java[delete-role-mapping-execute-async]
+--------------------------------------------------
+<1> The `DeleteRoleMappingResponse` to execute and the `ActionListener` to use when


s/DeleteRoleMappingResponse/DeleteRoleMappingRequest

Also corrected in the put-role-mapping.asciidoc. Thanks.

- Changes in java doc - Correct asciidoc - Correct the check for role mapping name in the constructor

jkakavas

LGTM

jaymode

I left two really minor comments. Otherwise LGTM

jaymode · 2018-10-18T15:11:44Z

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

+        assertNull(request.getEntity());
+    }
+
+


nit: extra line

jaymode · 2018-10-18T15:13:13Z

docs/java-rest/high-level/security/delete-role-mapping.asciidoc

+[[java-rest-high-security-delete-role-mapping-response]]
+==== Response
+The returned `DeleteRoleMappingResponse` contains a single field, `found`. If the mapping
+is successfully found and deleted, the found is set to true. Otherwise, found is set to false.


s/the found/found

Building on expression dsl and create role mapping API, this commit adds support for delete role mapping API to high level client.

[HLRC] Add support for Delete role mapping API

f1b9d11

Building on expression dsl and create role mapping API, this commit adds support for delete role mapping API to high level client.

bizybot added v7.0.0 :Core/Java High Level REST Client :Security/Security Security issues without another label v6.5.0 labels Oct 16, 2018

bizybot requested review from jaymode and jkakavas October 16, 2018 15:27

jkakavas requested changes Oct 17, 2018

View reviewed changes

Yogesh Gaikwad added 3 commits October 18, 2018 06:14

Address review comments

964d6b1

- Changes in java doc - Correct asciidoc - Correct the check for role mapping name in the constructor

Add test for DeleteRoleMappingResponse.

f16ae64

Merge branch 'master' into delete-role-mapping-hl-api

cd1c393

jkakavas approved these changes Oct 18, 2018

View reviewed changes

jaymode approved these changes Oct 18, 2018

View reviewed changes

Yogesh Gaikwad added 2 commits October 19, 2018 08:55

Address review comments

54c09c8

Merge branch 'master' into delete-role-mapping-hl-api

e8f9a77

bizybot merged commit 39a6163 into elastic:master Oct 19, 2018

bizybot added a commit that referenced this pull request Oct 19, 2018

[HLRC] Add support for Delete role mapping API (#34531)

ddd143b

Building on expression dsl and create role mapping API, this commit adds support for delete role mapping API to high level client.

elasticmachine mentioned this pull request Oct 19, 2018

Java high-level REST client completeness for xpack APIs #29827

Closed

98 tasks

colings86 added >enhancement and removed :Security/Security Security issues without another label labels Oct 25, 2018

kcm pushed a commit that referenced this pull request Oct 30, 2018

[HLRC] Add support for Delete role mapping API (#34531)

ec2e6c8

Building on expression dsl and create role mapping API, this commit adds support for delete role mapping API to high level client.

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Conversation

bizybot commented Oct 16, 2018

Uh oh!

elasticmachine commented Oct 16, 2018

Uh oh!

elasticmachine commented Oct 16, 2018

Uh oh!

bizybot commented Oct 16, 2018

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

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!

jkakavas left a comment

Choose a reason for hiding this comment

Uh oh!

jaymode 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants