Add get mappings support to high-level rest client by dakrone · Pull Request #30889 · elastic/elasticsearch

dakrone · 2018-05-25T22:46:16Z

This adds support for the get mappings API to the high level rest client.

Relates to #27205

This adds support for the get mappings API to the high level rest client. Relates to elastic#27205

elasticmachine · 2018-05-25T22:46:17Z

Pinging @elastic/es-core-infra

javanna

I left a few minor comments, LGTM otherwise

javanna · 2018-05-28T13:19:12Z

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

+            "_mapping", getMappingsRequest.types()));
+
+        Params parameters = new Params(request);
+        parameters.withMasterTimeout(getMappingsRequest.masterNodeTimeout());


I don't see this being set in the corresponding REST action, would you mind opening another PR to fix that? It needs to be added to the SPEC too.

javanna · 2018-05-28T13:19:28Z

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

+
+        Params parameters = new Params(request);
+        parameters.withMasterTimeout(getMappingsRequest.masterNodeTimeout());
+        parameters.withIndicesOptions(getMappingsRequest.indicesOptions());


I think that we need to set the local flag also

javanna · 2018-05-28T13:23:02Z

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


+    static Request getMappings(GetMappingsRequest getMappingsRequest) throws IOException {
+        Request request = new Request(HttpGet.METHOD_NAME, endpoint(getMappingsRequest.indices(),
+            "_mapping", getMappingsRequest.types()));


I think that you need to protect both indices and types from NPEs. Unfortunately you can set null from their setter methods. We already do this for other API.

javanna · 2018-05-28T13:23:22Z

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

+        getMappingRequest.indices(indices);
+
+        String type = randomAlphaOfLengthBetween(3, 10);
+        getMappingRequest.types(type);


test setting indices and/or types null also

can we also test with no types provided?

javanna · 2018-05-28T13:23:41Z

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

+        Map<String, String> expectedParams = new HashMap<>();
+
+        setRandomIndicesOptions(getMappingRequest::indicesOptions, getMappingRequest::indicesOptions, expectedParams);
+        setRandomMasterTimeout(getMappingRequest, expectedParams);


add a test around the local flag?

Yep, was added

javanna · 2018-05-28T13:26:11Z

...er/src/main/java/org/elasticsearch/action/admin/indices/mapping/get/GetMappingsResponse.java

+        return this.mappings.equals(other.mappings);
+    }
+
+    private static final class Fields {


can we get rid of this inner class and declare the field at the top-level?

javanna · 2018-05-28T13:28:11Z

server/src/main/java/org/elasticsearch/rest/action/admin/indices/RestGetMappingAction.java

        final GetMappingsRequest getMappingsRequest = new GetMappingsRequest();
        getMappingsRequest.indices(indices).types(types);
        getMappingsRequest.indicesOptions(IndicesOptions.fromRequest(request, getMappingsRequest.indicesOptions()));
+        getMappingsRequest.masterNodeTimeout(request.paramAsTime("master_timeout", getMappingsRequest.masterNodeTimeout()));


oh thanks I hadn't noticed that you already added this missing line ++ can you double check the spec too?

I added it to the spec

javanna · 2018-05-28T13:30:03Z

server/src/main/java/org/elasticsearch/rest/action/admin/indices/RestGetMappingAction.java

-                            message = String.format(Locale.ROOT, "types [%s] missing", toNamesString(difference.toArray(new String[0])));
-                        }
+                        final String message = String.format(Locale.ROOT, "type" + (difference.size() == 1 ? "" : "s") +
+                            " [%s] missing", Strings.collectionToCommaDelimitedString(difference));


remove the toNamesString method if we no longer use it?

javanna · 2018-05-28T13:32:52Z

server/src/main/java/org/elasticsearch/rest/action/admin/indices/RestGetMappingAction.java

@@ -129,47 +131,15 @@ public RestResponse buildResponse(final GetMappingsResponse response, final XCon
                        status = RestStatus.OK;
                    } else {
                        status = RestStatus.NOT_FOUND;


what do think about moving all this logic to the corresponding transport action (as a follow-up)? I didn't look too close but I think that this causes different behaviour between REST and transport at the moment, it would be nice to have a simpler REST action maybe.

Yeah, I don't love this logic but this isn't the place to remove it, I think it'd be fine as a follow-up. The difficulty is that we are not consistent here (see: #30768 (comment) ) I think we need to sort that out before removing this.

I am not talking about removing this (I agree it's not nice and we should talk about that), but rather moving this logic to the transport action. That would maintain the same behaviour at REST and most likely change it for transport client users. Anyway, doesn't belong here.

javanna · 2018-05-28T13:33:24Z

server/src/main/java/org/elasticsearch/rest/action/admin/indices/RestGetMappingAction.java

@@ -83,6 +84,7 @@ public RestChannelConsumer prepareRequest(final RestRequest request, final NodeC
        final GetMappingsRequest getMappingsRequest = new GetMappingsRequest();


we may also want to look at removing RestGetAllMappingsAction if possible. As a follow-up.

Yes, definitely +1 on removing that if possible.

…gs-api

dakrone · 2018-05-29T17:41:45Z

Thanks for taking a look @javanna, I think I've addressed all of your comments

javanna · 2018-05-30T10:46:15Z

docs/java-rest/high-level/indices/get_mappings.asciidoc

+--------------------------------------------------
+<1> Returning all indices' mappings
+<2> Retrieving the mappings for a particular index and type
+<3> Getting the mappings for the "tweet" as a Java Map


I get these errors when building the docs:

asciidoc: WARNING: get_mappings.asciidoc: line 14: list item index: expected 2 got 1 asciidoc: WARNING: get_mappings.asciidoc: line 15: list item index: expected 3 got 2

I have no idea why these occur, I checked and there are 3 annotations in IndicesClientDocumentationIT for this tag, and 3 here, not sure why they are off-by-one

Found it :)

javanna

LGTM besides the docs issue, feel free to merge once those are addressed. thanks @dakrone !

…gs-api

…atement

dakrone · 2018-06-04T20:32:25Z

Thanks for taking a look @javanna

* master: Add get mappings support to high-level rest client (#30889) Fix index prefixes to work with span_multi (#31066) [DOCS] Removes redundant authorization pages [DOCS] Re-adds custom realm Change ObjectParser exception (#31030) Upgrade to Lucene-7.4.0-snapshot-0a7c3f462f (#31073)

javanna · 2018-06-05T08:17:14Z

@dakrone do you plan on backporting this as well?

This adds support for the get mappings API to the high level rest client. Relates to #27205

* elastic/6.x: [Rollup] Disallow index patterns that match the rollup index (#30491) Revert "Fixing MixedClusterClientYamlTestSuiteIT" Fixing MixedClusterClientYamlTestSuiteIT Add get mappings support to high-level rest client (#30889) [DOCS] Fixes security example (#31082) Allow terms query in _rollup_search (#30973)

javanna · 2018-06-19T07:52:53Z

docs/java-rest/high-level/supported-apis.asciidoc

 include::indices/force_merge.asciidoc[]
 include::indices/rollover.asciidoc[]
 include::indices/put_mapping.asciidoc[]
+include::indices/get_mappings.asciidoc[]


heya @dakrone this is not enough for the page to be linked, in fact this API is currently missing in our docs, would you mind fixing that please?

This commit adds the high-level rest client docs for the get mappings API that was added in elastic#30889

This commit adds the high-level rest client docs for the get mappings API that was added in #30889

Add get mappings support to high-level rest client

95a8a67

This adds support for the get mappings API to the high level rest client. Relates to elastic#27205

dakrone added >enhancement v7.0.0 :Core/Java High Level REST Client labels May 25, 2018

javanna mentioned this pull request May 28, 2018

Java high-level REST client completeness #27205

Closed

80 tasks

javanna suggested changes May 28, 2018

View reviewed changes

dakrone added 8 commits May 29, 2018 11:03

Merge remote-tracking branch 'origin/master' into hlrc-add-get-mappin…

31ac916

…gs-api

Add support for "local" parameter

d1921d8

protect against null for indices and types

7414c06

Add tests for empty/null indices and types

dd9feb1

Remove debugging in toString method

1b6fd2f

move MAPPINGS parsefield to top level

9108459

Add master_timeout to rest spec for get mappings

7ffcdf7

Remove unused toNamesString method

3aa0094

dakrone added 2 commits May 29, 2018 13:56

Fix null casting

ad03b58

Fix test

da13f19

javanna reviewed May 30, 2018

View reviewed changes

javanna approved these changes May 30, 2018

View reviewed changes

dakrone added 3 commits June 4, 2018 09:02

Merge remote-tracking branch 'origin/master' into hlrc-add-get-mappin…

7a137e1

…gs-api

grumble grumble fix test after master merge that removed an import st…

1fc8121

…atement

Fix documentation typo

19f9386

dakrone merged commit b22a055 into elastic:master Jun 4, 2018

dakrone added a commit that referenced this pull request Jun 5, 2018

Add get mappings support to high-level rest client (#30889)

0fcacfe

This adds support for the get mappings API to the high level rest client. Relates to #27205

dakrone added the v6.4.0 label Jun 5, 2018

javanna reviewed Jun 19, 2018

View reviewed changes

dakrone added a commit to dakrone/elasticsearch that referenced this pull request Jul 3, 2018

[DOCS] Add missing get mappings docs to HLRC

f07f672

This commit adds the high-level rest client docs for the get mappings API that was added in elastic#30889

dakrone mentioned this pull request Jul 3, 2018

[DOCS] Add missing get mappings docs to HLRC #31765

Merged

dakrone added a commit that referenced this pull request Jul 3, 2018

[DOCS] Add missing get mappings docs to HLRC (#31765)

42ff8aa

This commit adds the high-level rest client docs for the get mappings API that was added in #30889

dakrone added a commit that referenced this pull request Jul 3, 2018

[DOCS] Add missing get mappings docs to HLRC (#31765)

1ff7066

This commit adds the high-level rest client docs for the get mappings API that was added in #30889

dakrone deleted the hlrc-add-get-mappings-api branch February 4, 2019 14:47

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

		@@ -83,6 +84,7 @@ public RestChannelConsumer prepareRequest(final RestRequest request, final NodeC
		final GetMappingsRequest getMappingsRequest = new GetMappingsRequest();

Conversation

dakrone commented May 25, 2018

Uh oh!

elasticmachine commented May 25, 2018

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!

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!

dakrone commented May 29, 2018

Uh oh!

javanna May 30, 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!

javanna left a comment

Choose a reason for hiding this comment

Uh oh!

dakrone commented Jun 4, 2018

Uh oh!

javanna commented Jun 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

javanna May 30, 2018 •

edited

Loading