HLRC: add support for get license basic/trial status API #33176

javanna · 2018-08-27T16:04:11Z

Relates to #29827

elasticmachine · 2018-08-27T16:04:13Z

Pinging @elastic/es-core-infra

javanna · 2018-08-27T16:05:25Z

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

+    public GetBasicStatusResponse getBasicStatus(RequestOptions options) throws IOException {
+        return restHighLevelClient.performRequestAndParseEntity(Validatable.EMPTY,
+            request -> RequestConverters.getLicenseBasicStatus(), options, GetBasicStatusResponse::fromXContent, emptySet());
+    }


Here I went for exposing only sync versions of these two methods, I don't see how they would be used in an async fashion. Also, I went for not adding an empty request and mimic what we already did with ping and info, which are somehow an exception when you look at the method argument.

javanna · 2018-08-27T16:14:12Z

...t/rest-high-level/src/main/java/org/elasticsearch/client/license/GetBasicStatusResponse.java

+    }
+
+    @Override
+    public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {


this is only here for testing, and it's duplicated in the original transport client class. The problem is that AbstractXContentTestCase requires a type that extends ToXContent, and we need to test the parsing code from the client response. Do you have something in mind to be able to test the parsing code using the rendering code from Elasticsearch?

Here is the first incarnation of what I wanted to do, and I feel like we can make this more generic but I wanted to just get it done in an idea before going crazy on a generic impl for these tests.

https://github.com/hub-cap/elasticsearch/blob/f06095cb29c72ade77dd51d62d75096071cd7619/x-pack/protocol/src/test/java/org/elasticsearch/protocol/xpack/watcher/DeleteWatchResponseTests.java

that looks good, I am tweaking AbstractXContentTestCase to be able to support this scenario (especially to make sure that we don't lose testing support for unknown fields etc), but my problem is that I don't have the org.elasticsearch.license.GetTrialStatusResponse in the test classpath. Should I move it somewhere else? Or add the plugin to the test classpath? how did you do that in your branch?

javanna · 2018-08-29T09:22:19Z

@hub-cap I pushed my changes where I add streamlined support for testing when the server class is ToXContent and the client class only does the parsing. I cannot use it yet entirely as I cannot add xpack core as a dependency (it causes jar hell) but it gives you an idea.

hub-cap · 2018-08-29T17:43:05Z

Thanks @javanna. ill have a look at what we can do here, testing wise. I discussed it with @jasontedor and we came to the conclusion it would be best to test against a live server instead of unit testing. It will be much easier for a few reasons.

Eventually the code should not depend on any of the server side bits
It would be a lot easier to test the client against different versions of the server for BWC.

The second one is the bigger one, of course, because just the unit tests would not cover any changes in BWC.

javanna · 2018-08-29T18:04:35Z

ok @hub-cap I agree on proper live testing. One reason for unit tests may be forward compatibility (the supportsUnknownFields bit) of the client, that may not be extensively tested, as we can only test up to some point in the (known) future. Let me know what I need to do with this PR once you know more.

javanna · 2018-09-18T14:41:53Z

@hub-cap I have merged master in. I have left the additional testing (with still the same issues as before) in the PR for you to look at, although you mentioned we may want to remove that part from this PR and look at testing later. Let me know.

javanna · 2018-09-19T09:48:55Z

retest this please

hub-cap

Left some comments. Good general direction. thank you immensely for pointing out the request converter tests and IT tests, somehow these had previously slipped thru the cracks.

hub-cap · 2018-09-19T16:49:45Z

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

+
+import java.io.IOException;
+
+public class LicenseIT extends ESRestHighLevelClientTestCase {


ty for adding this here. Ive also pinged a few other license pull requests to do the same. Somehow it got missed by me in other PRs

hub-cap · 2018-09-19T16:51:19Z

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

+
+public class LicenseRequestConvertersTests extends ESTestCase{
+
+    //TODO other existing methods don't have unit tests?


nope. Also noted on other reviews.

cool I will remove the TODO then

hub-cap · 2018-09-19T17:01:24Z

x-pack/plugin/core/src/main/java/org/elasticsearch/license/RestGetTrialStatus.java

-                    }
-                });
+    protected RestChannelConsumer doPrepareRequest(RestRequest request, XPackClient client) {
+        return channel -> client.licensing().prepareGetStartTrial().execute(new RestToXContentListener<>(channel));


ty for doing this on these responses. Does it need to be part of this PR any more now that we are not using the server libs for testing the client?

it is not necessary as part of this PR, just a nice to have while I was at it.

this is now necessary in order to use AbstractHlrcStreamableXContentTestCase

javanna · 2018-09-20T09:24:33Z

...t-high-level/src/test/java/org/elasticsearch/client/license/GetBasicStatusResponseTests.java

+        assertEquals(expectedInstance.eligibleToStartBasic, newInstance.isEligibleToStartBasic());
+    }
+
+    static class TestResponse implements ToXContentObject {


if we want to keep on unit testing the responses using the existing test infra, there has got to be somewhere a ToXContent. I take it that we don't want to (and we can't at the moment) depend on core and reuse the toXContent from core, then this is the easiest way I could find. Obviously it duplicates code. Let me know what you think.

@nik9000 also did soemthing like this in #33921 if you want to have a look. I think this makes more sense than putting them on the response, just because they are never used. And like we said previously, the client ToXContent -> client FromXContent is much less useful then server ToXContent -> client fromXContent, so I dont consider it a bad thing to move them here, its just making life easier than manually writing a parser.

In #33921 I did the refactoring to not need a ToXContent and instead pass in a method that does the writing. I figured that that would be a thing we could build off of later if we wanted to add tests on the server side that assert that the server side responses generate xcontent that is parsable by the clients.

javanna · 2018-09-20T09:26:21Z

test/framework/src/main/java/org/elasticsearch/test/AbstractToFromXContentTestCase.java

+import java.util.function.Predicate;
+import java.util.function.Supplier;
+
+public abstract class AbstractToFromXContentTestCase<T extends ToXContent, P> extends ESTestCase {


this new test class is like the previous AbstractXContentTestCase but it supports two types, one for outputting to Xcontent, and one for parsing the output. AbstractXContentTestCase can become a specialization of this that uses the same class for both.

javanna · 2018-09-20T09:27:15Z

@hub-cap I pushed an update, let me know what you want me to do with the response unit tests.

hub-cap · 2018-09-28T16:55:51Z

Generally speaking I like the approach here. I want @nik9000 to have a look as well, as hes done similar stuff, but I think youve gone one step further to ensure we can use a common base class for these tests. So big +1 on that. If @nik9000 is good, i will finish the review.

Relates to elastic#29827

javanna · 2018-11-08T13:15:56Z

...in/core/src/test/java/org/elasticsearch/protocol/AbstractHlrcStreamableXContentTestCase.java

+    @Override
+    protected T doParseInstance(XContentParser parser) throws IOException {
+        return convertHlrcToInternal(doHlrcParseInstance(parser));
+    }


@vladimirdolzhenko thoughts?

in this case you hide xpack-parsing-to-xpack-instance functionality that I don't want to drop.

but isn't it there only for testing? And isn't it a duplicate of the parsing code that is already under client?

I see your point.

Usually we have both toXContent and fromXContent in xpack request/response and fromXContent only on HLRC side. I'd like to keep fromXContent on a server side (maybe another clean up iteration is required)

anyway - I would not go with this change on a level of AbstractHlrcStreamableXContentTestCase

I think this is a mistake. To me, the main point of having such a test class under xpack is to avoid code duplication. Otherwise it would be easy to have tests under client that rely on e.g. toXContent copy-pasted from xpack that's not needed in client. Given where tests currently are (under x-pack), code duplication would not be required, but we end up duplicating parsing code under x-pack and client. I don't think we should maintain this moving forward. Why keep functionality that is not needed?

This change in my PR does not affect existing tests that override this method, as the test method is not yet made final. I think though that this is a better default behaviour than asking people to duplicate fromXContent: less code to maintain, and less hassle in writing tests.

I'd like to keep fromXContent on a server side (maybe another clean up iteration is required)

How can we do that? Response parsing is needed in the client only, which should not depend on server (at least ideally).

Agree - it's a big step forward. But we have to clean up many other leftovers in this case. @hub-cap @nik9000 your objections ?

Maybe we're extending the wrong test class here. That is why I built that fluent testing thingy - I figured we'd want to be fairly surgical in what we tested. I don't think we should maintain response parsing in the server. If we can get away with dropping it. I don't like keeping it just for testing.

maybe the problem here, and where the confusion comes from, is that requests need parsing in server, and toXContent in client, while responses need parsing in client and toXContent in server. I guess we can't have a single test class that works well for both requests and responses? And my change here is only addressing responses although the test is meant to be more generic though it is used at the moment only for responses.

javanna · 2018-11-08T13:16:39Z

I have brought this PR up-to-date. Much easier to test things with the recent changes. @vladimirdolzhenko maybe you want to have a look?

javanna · 2018-11-09T21:03:27Z

retest this please

javanna · 2018-11-09T21:14:13Z

retest this please1

javanna · 2018-11-10T14:12:17Z

@hub-cap tests are green, can you review this please?

If Nik is ok with this then I am ok with this :)

nik9000 · 2018-11-13T14:12:13Z

Lgtm!

…

On Sat, Nov 10, 2018, 09:12 Luca Cavanna ***@***.*** wrote: @hub-cap <https://github.com/hub-cap> tests are green, can you review this please? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#33176 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AANLouYzX0sGe2wQVSWJF83Tcjpdc3f9ks5utt7FgaJpZM4WODpq> .

javanna · 2018-11-13T15:01:41Z

backport blocked by #35202

Relates to elastic#29827

Relates to #29827

javanna added >enhancement v7.0.0 :Core/Java High Level REST Client v6.5.0 labels Aug 27, 2018

javanna requested a review from hub-cap August 27, 2018 16:04

javanna commented Aug 27, 2018

View reviewed changes

elasticmachine mentioned this pull request Aug 27, 2018

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

Closed

98 tasks

hub-cap previously requested changes Sep 19, 2018

View reviewed changes

javanna commented Sep 20, 2018

View reviewed changes

javanna mentioned this pull request Sep 25, 2018

Make the high-level REST client skip types for document deletion. #34041

Closed

colings86 added v6.6.0 and removed v6.5.0 labels Oct 25, 2018

HLRC: add support for get license basic/trial status API

e047f63

Relates to elastic#29827

javanna force-pushed the enhancement/hlrc_get_license_status branch from eafecfd to e047f63 Compare November 8, 2018 13:09

remove unused constant

3ae1de0

javanna commented Nov 8, 2018

View reviewed changes

javanna added 3 commits November 8, 2018 15:49

unused imports

337a7ea

unused imports

14f6797

Merge branch 'master' into enhancement/hlrc_get_license_status

e127c9c

javanna merged commit 0b7d18d into elastic:master Nov 13, 2018

javanna added the backport pending label Nov 13, 2018

javanna added a commit to javanna/elasticsearch that referenced this pull request Nov 14, 2018

HLRC: add support for get license basic/trial status API (elastic#33176)

fe070e9

Relates to elastic#29827

javanna mentioned this pull request Nov 14, 2018

HLRC: add support for get license basic/trial status API #35529

Merged

javanna added a commit that referenced this pull request Nov 14, 2018

HLRC: add support for get license basic/trial status API (#33176)

b01a58b

Relates to #29827

javanna removed the backport pending label Nov 14, 2018

javanna mentioned this pull request Nov 14, 2018

Clean up XPackInfoResponse class and related tests #35547

Merged

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


		import java.io.IOException;

		public class LicenseIT extends ESRestHighLevelClientTestCase {


		public class LicenseRequestConvertersTests extends ESTestCase{

		//TODO other existing methods don't have unit tests?

HLRC: add support for get license basic/trial status API #33176

HLRC: add support for get license basic/trial status API #33176

Conversation

javanna commented Aug 27, 2018

elasticmachine commented Aug 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javanna commented Aug 29, 2018

hub-cap commented Aug 29, 2018

javanna commented Aug 29, 2018

javanna commented Sep 18, 2018

javanna commented Sep 19, 2018

hub-cap left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javanna commented Sep 20, 2018

hub-cap commented Sep 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javanna Nov 8, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javanna commented Nov 8, 2018

javanna commented Nov 9, 2018

javanna commented Nov 9, 2018

javanna commented Nov 10, 2018

nik9000 commented Nov 13, 2018 via email

javanna commented Nov 13, 2018

javanna Nov 8, 2018 •

edited