HLRC: Add delete user action by iverase · Pull Request #35294 · elastic/elasticsearch

iverase · 2018-11-06T12:02:15Z

It adds delete user action to the high level rest client.

Relates #29827

It adds delete user action to the high level rest client. Relates elastic#29827

elasticmachine · 2018-11-06T12:03:11Z

Pinging @elastic/es-core-infra

iverase · 2018-11-06T13:53:06Z

@elasticmachine test this please

iverase · 2018-11-06T14:50:28Z

@elasticmachine test this please

hub-cap

Great work. The biggest thing here is the comment about the Optional stuff going on. I think the deciding factor is whether it throws a 404 or not for a bad user. Rest of the stuff is small.

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

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

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

hub-cap · 2018-11-06T19:37:54Z

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

+/**
+ * Response for a role being deleted from the native realm
+ */
+public final class DeleteUserResponse {


this could probably extend AcknowledgedResponse. See StartRollupJobResponse for what I mean. It is just a generic {"name": boolean} class that allows you to specify name.

I would prefer not to extend that class as it seems weird to have a dependency in this class to rollup package. In addition the response is not an acknowledge but a boolean defining if the user was found or not.

If you still thing it is better to use AcknowledgedResponse as a base class I will change it.

its being moved to core in another PR thats currently in flight ;)

I also think its worth potentially renaming it because this is really just {"SomethingNamed": boolean}, and it just started as acknowledged response.

Touche :) I will implement it that way then

I reimplement it by extending AcknowledgedResponse.

iverase · 2018-11-07T14:30:13Z

@elasticmachine test this please

hub-cap · 2018-11-12T19:37:21Z

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

+/**
+ * Response for a user being deleted from the native realm
+ */
+public final class DeleteUserResponse extends AcknowledgedResponse {


I think we can gut this altogether since optional has a "isPresent()" field. See #35470 and leave your thoughts there or here.

Im still on the fence for whether we even need a found true/false in the code since we return an optional. WDYT?

hub-cap · 2018-11-14T22:16:08Z

ok, so going back to this i think its worthwhile to 1) throw a 404 if it does not exist, 2) remove found=true if it does exist, 3) remove the optional. Sorry for the rework, this was being fleshed out while we discussed :/

iverase · 2018-11-19T10:17:23Z

@elasticmachine test this please

iverase · 2018-11-19T13:36:15Z

@elasticmachine test this please

hub-cap · 2018-11-21T04:29:14Z

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

+     */
+    public DeleteUserResponse deleteUser(DeleteUserRequest request, RequestOptions options) throws IOException {
+        return restHighLevelClient.performRequestAndParseEntity(request, SecurityRequestConverters::deleteUser, options,
+            DeleteUserResponse::fromXContent, singleton(404));


Hey I saw some updates on this PR and I just wanted to throw a reminder out that we are not going to do a singleton(404) here, because we want a delete that is not found to throw an exception. Also, last time we spoke you were going to change this to AcknowledgedResponse. <3

I have tried those approaches but they seem not to work. If I use AcknowledgedResponse, then you get the following error:

> Throwable #1: java.io.IOException: Unable to parse response body for Response{requestLine=DELETE /_xpack/security/user/testUser?refresh=true HTTP/1.1, host=http://[::1]:58075, response=HTTP/1.1 200 OK} > at __randomizedtesting.SeedInfo.seed([311ED8EAD4721921:2905907290C29FEC]:0) > at org.elasticsearch.client.RestHighLevelClient.internalPerformRequest(RestHighLevelClient.java:1417) > at org.elasticsearch.client.RestHighLevelClient.performRequest(RestHighLevelClient.java:1383) > at org.elasticsearch.client.RestHighLevelClient.performRequestAndParseEntity(RestHighLevelClient.java:1350) > at org.elasticsearch.client.SecurityClient.deleteUser(SecurityClient.java:114) > at org.elasticsearch.client.documentation.SecurityDocumentationIT.testDeleteUser(SecurityDocumentationIT.java:158) > at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method) > at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) > at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) > at java.base/java.lang.reflect.Method.invoke(Method.java:566) > at java.base/java.lang.Thread.run(Thread.java:834) > Caused by: java.lang.IllegalArgumentException: Required [acknowledged

I think I need to override the class to be able to parse the response. Then I tried not to catch the exception and then I got the following error:

> Throwable #1: ElasticsearchStatusException[Unable to parse response body]; nested: ResponseException[method [DELETE], host [http://[::1]:57316], URI [/_xpack/security/user/testUser?refresh=true], status line [HTTP/1.1 404 Not Found] > {"found":false}]; nested: ResponseException[method [DELETE], host [http://[::1]:57316], URI [/_xpack/security/user/testUser?refresh=true], status line [HTTP/1.1 404 Not Found] > {"found":false}]; > at __randomizedtesting.SeedInfo.seed([1E08456835E2EAC7:6130DF071526C0A]:0) > at org.elasticsearch.client.RestHighLevelClient.parseResponseException(RestHighLevelClient.java:1650) > at org.elasticsearch.client.RestHighLevelClient.internalPerformRequest(RestHighLevelClient.java:1411) > at org.elasticsearch.client.RestHighLevelClient.performRequest(RestHighLevelClient.java:1383) > at org.elasticsearch.client.RestHighLevelClient.performRequestAndParseEntity(RestHighLevelClient.java:1350) > at org.elasticsearch.client.SecurityClient.deleteUser(SecurityClient.java:113) > at org.elasticsearch.client.documentation.SecurityDocumentationIT.testDeleteUser(SecurityDocumentationIT.java:166) > at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method) > at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) > at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) > at java.base/java.lang.reflect.Method.invoke(Method.java:566) > at java.base/java.lang.Thread.run(Thread.java:834) > Suppressed: ParsingException[Failed to parse object: expecting field with name [error] but found [found]]

Not sure if I can implement it in any other way without changing the underlaying implementation.

It might also depend on which implementation of AcknowledgedResponse you are using, since we have two, yay duplication!!! You should have a look at StartRollupJobResponse, which is an example of changing the word from acknowledged to started. this should get you on the right track.

DeleteUserResponse is a subclass of AcknowledgedResponse. I am using the same pattern.

heh, duh... Sorry, ive been on vacation and full of turkey since last week.

so the only thing here now is to remove singleton(404), right?

No problem, happy you enjoyed your time off!

Yes, the issue we have is the 404 and as I showed before, the problem is that if I do not capture the error there is a parsing error because the framework is expecting a different type of message:

Suppressed: ParsingException[Failed to parse object: expecting field with name [error] but found [found]]

Not sure if we can avoid without changing the current implementation.

Apologies, I misread the comment before. Since server side is not throwing exceptions, lets keep the singleton(404), so we dont have to mod the parsing code. This is something we should definitely fix server side.

* HLRC: Add delete user action It adds delete user action to the high level rest client. Relates #29827

HLRC: Add delete user action

396e65c

It adds delete user action to the high level rest client. Relates elastic#29827

iverase requested a review from hub-cap November 6, 2018 12:02

iverase self-assigned this Nov 6, 2018

iverase added >enhancement :Core/Java High Level REST Client labels Nov 6, 2018

iverase added v7.0.0 v6.6.0 labels Nov 6, 2018

elasticmachine mentioned this pull request Nov 6, 2018

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

Closed

98 tasks

iverase added 2 commits November 6, 2018 13:42

Fix precommit

876f4a0

Fix precommit

ae9059f

Merge remote-tracking branch 'origin/master' into deleteUser

2d4aa98

hub-cap suggested changes Nov 6, 2018

View reviewed changes

iverase added 2 commits November 7, 2018 09:44

address some of the review comments

321935a

fist bug serializing response

e4a0904

iverase added 7 commits November 8, 2018 13:11

Implement DeleteUserResponse by extending AcknowledgedResponse

058f6c4

Merge remote-tracking branch 'origin/master' into deleteUser

86786e2

Acknowledge response is in core now

ff6f055

javadocs

041aeb6

simplify docs

ee6d4c6

sync with head and implement response as Optional

a2f2d52

fix checkStyle

7465462

hub-cap reviewed Nov 12, 2018

View reviewed changes

iverase added 3 commits November 19, 2018 07:22

remove otioonal and use AcknowledgeResponse

b83312d

Merge remote-tracking branch 'origin/master' into deleteUser

e89ea8b

Implement delete response

7e90b83

iverase added 5 commits November 19, 2018 07:55

remove unused imports

f3e76a5

add license

0351a26

formatting

5f10520

add license

1f7fb31

fix checkStyle

c697fc0

Merge remote-tracking branch 'origin/master' into deleteUser

a55981a

hub-cap reviewed Nov 21, 2018

View reviewed changes

Merge remote-tracking branch 'origin/master' into deleteUser

4368f2e

hub-cap approved these changes Nov 28, 2018

View reviewed changes

Merge remote-tracking branch 'origin/master' into deleteUser

d4d66af

iverase merged commit 93ed8b7 into elastic:master Nov 29, 2018

iverase added a commit that referenced this pull request Nov 29, 2018

HLRC: Add delete user action (#35294)

f57e429

* HLRC: Add delete user action It adds delete user action to the high level rest client. Relates #29827

iverase deleted the deleteUser branch November 29, 2018 06:55

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

Conversation

iverase commented Nov 6, 2018

Uh oh!

elasticmachine commented Nov 6, 2018

Uh oh!

iverase commented Nov 6, 2018

Uh oh!

iverase commented Nov 6, 2018

Uh oh!

hub-cap left a comment

Choose a reason for hiding this comment

Uh oh!

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!

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!

iverase commented Nov 7, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hub-cap commented Nov 14, 2018

Uh oh!

iverase commented Nov 19, 2018

Uh oh!

iverase commented Nov 19, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iverase Nov 21, 2018 • 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

iverase Nov 21, 2018 •

edited

Loading