Added Put Mapping API to high-level Rest client (#27205) by catalin-ursachi · Pull Request #27869 · elastic/elasticsearch

catalin-ursachi · 2017-12-18T17:20:13Z

Adds the Put Mapping API (part of #27205).

Still have to write the docs.

elasticmachine · 2017-12-18T17:20:14Z

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?

catalin-ursachi · 2017-12-18T17:22:19Z

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

    }

+    static Request putMapping(PutMappingRequest putMappingRequest) throws IOException {
+        String endpoint = endpoint(putMappingRequest.indices(), "_mapping", putMappingRequest.type());


Do we want to support putMappingRequest.getConcreteIndex() here? If so, how/when should it be used? Does it make sense to support in client-specific code?

no that's an internal concept leaked to our external API. Maybe check in RestHighLevelClient that concreteIndex is not set, as it will be ignored.

olcbean · 2017-12-18T19:28:33Z

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

    }

+    /**
+     * Creates an index using the Put Mapping API


put mapping adds a type or fields to an existing index?

dadoonet · 2017-12-19T10:48:13Z

IMO this is one of the API we should not add to the REST Client. See discussion at #15613 (comment).

If we merge it, we should may be merge it as @Deprecated... ?

But I'd prefer that others comment as well.

catalin-ursachi · 2018-01-03T11:53:52Z

Fwiw, would it not be simplest to merge this as is, and modify it together with the corresponding endpoint (be it PUT /_settings or something else) if and when necessary?

elasticmachine · 2018-01-03T11:53:53Z

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

this looks pretty good @catalin-ursachi thanks a lot! Besides the few comments that I left, I think this only needs docs. If you could work on that, I would be happy to merge this in. Sorry it took me a while to look at it.

javanna · 2018-01-16T16:15:20Z

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

    }

+    static Request putMapping(PutMappingRequest putMappingRequest) throws IOException {
+        String endpoint = endpoint(putMappingRequest.indices(), "_mapping", putMappingRequest.type());


no that's an internal concept leaked to our external API. Maybe check in RestHighLevelClient that concreteIndex is not set, as it will be ignored.

javanna · 2018-01-16T16:18:30Z

core/src/main/java/org/elasticsearch/action/admin/indices/mapping/put/PutMappingRequest.java

+        if (source != null) {
+            builder.rawValue(new BytesArray(source), XContentType.JSON);
+        } else {
+            builder.startObject().endObject();


is it ok here to only end the object? should we not start it too? and above, does rawValue print out a valid complete object?

I don't understand the first question; we are both starting and ending it, aren't we?

And yes, as source has to be a complete object, rawValue applied by itself to the builder also produces one.

javanna · 2018-01-16T16:22:15Z

...src/test/java/org/elasticsearch/action/admin/indices/mapping/put/PutMappingRequestTests.java

+        mapping.endObject();
+        mapping.endObject();
+        mapping.endObject();
+        request.source(mapping);


do we need to test the case where source is not set?

catalin-ursachi · 2018-01-21T18:28:32Z

...src/test/java/org/elasticsearch/action/admin/indices/mapping/put/PutMappingRequestTests.java

+        return request;
+    }
+
+    private static XContentBuilder randomMapping() throws IOException {


@javanna - These two methods are duplicated in CreateIndexRequestTests; what would be a sensible place to commonise them?

(Although fwiw randomMapping does differ slightly between the two classes)

you can make one of them public and call it from both tests, I don't mind

javanna

thanks for the update @catalin-ursachi I added a few more comments, nothing major though, and thanks a lot for adding the docs. Once you address my latest comments this should be ready to be merged.

javanna · 2018-01-22T15:38:26Z

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


+    static Request putMapping(PutMappingRequest putMappingRequest) throws IOException {
+        // The concreteIndex is an internal concept, not applicable to requests made over the REST API.
+        assert (putMappingRequest.getConcreteIndex() == null);


given that a request is provided by users, this should rather be an exception that gets thrown. Also this assertion may make the user's application crash. Throw IllegalArgumentException instead?

javanna · 2018-01-22T15:40:08Z

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

+        Params parameters = Params.builder();
+        parameters.withTimeout(putMappingRequest.timeout());
+        parameters.withMasterTimeout(putMappingRequest.masterNodeTimeout());
+        parameters.withUpdateAllTypes(putMappingRequest.updateAllTypes());


this parameter has been removed from master, I think that you may see that once you merge master in. I need to remember to add it back though when backporting to 6.x.

javanna · 2018-01-22T15:40:36Z

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

+            PutMappingRequest putMappingRequest = new PutMappingRequest(indexName);
+
+            putMappingRequest.type("type_name");
+


nit: can you remove this empty line and the one above please?

javanna · 2018-01-22T15:40:50Z

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

+            assertTrue(putMappingResponse.isAcknowledged());
+
+            Map<String, Object> indexMetaData = getIndexMetadata(indexName);
+


nit: can you remove this empty line?

javanna · 2018-01-22T15:41:44Z

...level/src/test/java/org/elasticsearch/client/documentation/IndicesClientDocumentationIT.java

+                    "      }\n" +
+                    "    }\n" +
+                    "  }\n" +
+                    "}", // <2>


was this required? Maybe you can revert this small part?

The IDE indented the lines; will revert.

Would you like me to add the two extra spaces per line within the string back in? They seemed superfluous.

javanna · 2018-01-22T15:45:05Z

...src/test/java/org/elasticsearch/action/admin/indices/mapping/put/PutMappingRequestTests.java

+
+        String expectedRequestBody = "{}";
+
+        assertEquals(expectedRequestBody, actualRequestBody);


here too can you remove some of these empty lines please?

javanna · 2018-01-22T15:45:55Z

...src/test/java/org/elasticsearch/action/admin/indices/mapping/put/PutMappingRequestTests.java

+        return request;
+    }
+
+    private static XContentBuilder randomMapping() throws IOException {


you can make one of them public and call it from both tests, I don't mind

javanna · 2018-01-22T15:48:16Z

...src/test/java/org/elasticsearch/action/admin/indices/mapping/put/PutMappingRequestTests.java

+        mapping.endObject();
+        mapping.endObject();
+        mapping.endObject();
+        request.source(mapping);


I had left a comment on this, do we need to test also the case where the source is not set hence it's null? We do cover it in PutMappingRequest#toXContent

Oh, that's the new testToXContentWithEmptySource below.

(Sorry, all comments got hidden because the merge with master moved all these files to server/...)

nice, sorry I missed it.

javanna · 2018-01-22T15:53:03Z

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

+--------------------------------------------------
+include-tagged::{doc-tests}/IndicesClientDocumentationIT.java[put-mapping-request-source]
+--------------------------------------------------
+<1> The mapping source


should we cover the different way to provide the source like we did in other docs pages (e.g. search API)? I see that we didn't do it for the create index API, but maybe we should have. Shall we have a common page on how to provide a source and link to it? What do you think? Maybe we could also do it as a followup so this PR doesn't get too big.

That does make sense; but it does rather seem like there could be some more user-friendly mapping-building tools, abstracting out the boilerplate. E.g. the ones in Elastic4s:

createIndex("artists").mappings( mapping("modern").fields( textField("name") ) )

(https://github.com/sksamuel/elastic4s)

But anyway; as things are now, I think a separate doc makes sense, so as not to clutter the Create Index and Put Mapping pages too much?

I agree that we should do better, but for now we are only porting what the transport client supports to the new client. I think that having a separate page would be good so we don't repeat the same content in different pages, I assume that the content would be the same, it can probably already be extracted from the search docs page.

Cool; I am tempted to suggest doing it as a follow-up PR, then, since I'm not sure I've got the time to do it just now. If you think it's a blocker for this one, though, then that's fair enough.

agreed, fine as a follow-up PR

@catalin-ursachi let me know if you want to take this, otherwise I can take care of it.

By all means feel free to take it; otherwise I'll pick it up when I next set aside some time for OS work, but I can't promise when that is. 🙂

javanna · 2018-01-22T15:54:14Z

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

+<2> The type to create (or update)
+
+==== Mapping source
+A description of the fields to create on the mapping; if not defined, the mapping will default to empty.


should we list this under the optional arguments? After all it is optional?

Hmm. Technically it's optional; but a PutMapping request without a source just adds a new type; which, in the ES 6.x+ world, is basically meaningless.

catalin-ursachi · 2018-01-22T18:09:30Z

Made all the requested changes, save for the two outstanding questions.

javanna · 2018-01-23T10:10:44Z

thanks @catalin-ursachi !

Relates to #27205

…t mapping API Relates to #27869

* es/master: Remove redundant argument for buildConfiguration of s3 plugin (#28281) Completely remove Painless Type from AnalyzerCaster in favor of Java Class. (#28329) Fix spelling error Reindex: Wait for deletion in test Reindex: log more on rare test failure Ensure we protect Collections obtained from scripts from self-referencing (#28335) [Docs] Fix asciidoc style in composite agg docs Adds the ability to specify a format on composite date_histogram source (#28310) Provide a better error message for the case when all shards failed (#28333) [Test] Re-Add integer_range and date_range field types for query builder tests (#28171) Added Put Mapping API to high-level Rest client (#27869) Revert change that does not return all indices if a specific alias is requested via get alias api. (#28294) Painless: Replace Painless Type with Java Class during Casts (#27847) Notify affixMap settings when any under the registered prefix matches (#28317)

* es/6.x: Remove redundant argument for buildConfiguration of s3 plugin (#28281) Completely remove Painless Type from AnalyzerCaster in favor of Java Class. (#28329) Fix spelling error Reindex: Wait for deletion in test Reindex: log more on rare test failure Ensure we protect Collections obtained from scripts from self-referencing (#28335) Provide a better error message for the case when all shards failed (#28333) REST high-level client: add support for update_all_types option to put mapping API Added Put Mapping API to high-level Rest client (#27869) [Test] Re-Add integer_range and date_range field types for query builder tests (#28171) Revert change that does not return all indices if a specific alias is requested via get alias api. (#28294) Painless: Replace Painless Type with Java Class during Casts (#27847) Notify affixMap settings when any under the registered prefix matches (#28317)

* master: (94 commits) Completely remove Painless Type from AnalyzerCaster in favor of Java Class. (elastic#28329) Fix spelling error Reindex: Wait for deletion in test Reindex: log more on rare test failure Ensure we protect Collections obtained from scripts from self-referencing (elastic#28335) [Docs] Fix asciidoc style in composite agg docs Adds the ability to specify a format on composite date_histogram source (elastic#28310) Provide a better error message for the case when all shards failed (elastic#28333) [Test] Re-Add integer_range and date_range field types for query builder tests (elastic#28171) Added Put Mapping API to high-level Rest client (elastic#27869) Revert change that does not return all indices if a specific alias is requested via get alias api. (elastic#28294) Painless: Replace Painless Type with Java Class during Casts (elastic#27847) Notify affixMap settings when any under the registered prefix matches (elastic#28317) Trim down usages of `ShardOperationFailedException` interface (elastic#28312) Do not return all indices if a specific alias is requested via get aliases api. [Test] Lower bwc version for rank-eval rest tests CountedBitSet doesn't need to extend BitSet. (elastic#28239) Calculate sum in Kahan summation algorithm in aggregations (elastic#27807) (elastic#27848) Remove the `update_all_types` option. (elastic#28288) Add information when master node left to DiscoveryNodes' shortSummary() (elastic#28197) ...

KnowledgeGarden · 2018-02-02T00:16:40Z

The preliminary doc https://www.elastic.co/guide/en/elasticsearch/client/java-rest/6.x/java-rest-high-put-mapping.html shows how to use putMapping. When attempted on 6.1.2, the api is missing; documentation to set mappings seems incomplete.

KnowledgeGarden · 2018-02-02T02:29:45Z

Documentation on CreateIndexRequest -- a class in the codebase for 6.1.3 says to fire that request to make a CreateIndexResponse with client.indices().create(...) and, of course, that API is missing. There must be a way to create index/mappings pairs well documented somewhere online.

javanna · 2018-02-02T09:08:41Z

@KnowledgeGarden to know when something was/will be released, you can check the labels of the PR. This one is marked 6.3.0, meaning that it will be released with 6.3.0. It is true that the request is already there, as it is part of the transport client, but the high-level REST client doesn't support it yet in version 6.1.3.

KnowledgeGarden · 2018-02-02T18:30:37Z

@javanna Thanks! Timeline for release of 6.3.0?

catalin-ursachi · 2018-02-04T21:20:49Z

Hi @KnowledgeGarden; you can of course use the low-level rest client for creating an index and adding a mapping:

// Create index
client().performRequest("PUT", "my_new_index");

// Put mapping
HttpEntity mappingSource = new NStringEntity(
    "{\n" +
        "  \"properties\": {\n" +
        "    \"name\": {\n" +
        "      \"type\": \"text\"\n" +
        "    }\n" +
        "  }\n" +
        "}\n",
    ContentType.APPLICATION_JSON);
client().performRequest("PUT", "my_new_index/_mapping/my_new_type", Collections.emptyMap(), mappingSource);

Added Put Mapping API to high-level Rest client (elastic#27205)

232b219

catalin-ursachi commented Dec 18, 2017

View reviewed changes

dadoonet mentioned this pull request Dec 18, 2017

Remove support for types? #15613

Closed

olcbean reviewed Dec 18, 2017

View reviewed changes

Fixed comments

c0385c1

dadoonet added :Java High Level REST Client discuss labels Dec 19, 2017

javanna mentioned this pull request Jan 16, 2018

Java high-level REST client completeness #27205

Closed

80 tasks

javanna self-requested a review January 16, 2018 16:08

javanna added >enhancement and removed discuss labels Jan 16, 2018

javanna self-assigned this Jan 16, 2018

javanna suggested changes Jan 16, 2018

View reviewed changes

catalin-ursachi added 5 commits January 20, 2018 23:25

Review mark-ups

658eb7a

Merge remote-tracking branch 'origin/master' into put-mapping

7cd96c6

Moved PutMappingResponseTests

6a00310

Added Put Mapping docs

b7a110e

Merge remote-tracking branch 'Catalin/put-mapping' into put-mapping

574d2fa

catalin-ursachi commented Jan 21, 2018

View reviewed changes

javanna approved these changes Jan 22, 2018

View reviewed changes

catalin-ursachi added 3 commits January 22, 2018 17:50

Merge remote-tracking branch 'origin/master' into put-mapping

9a7c924

Review mark-ups

ca458e5

Fixed import order

b9ec26b

javanna added v7.0.0 v6.3.0 labels Jan 23, 2018

javanna merged commit cf61d79 into elastic:master Jan 23, 2018

javanna added the backport pending label Jan 23, 2018

javanna pushed a commit that referenced this pull request Jan 23, 2018

Added Put Mapping API to high-level Rest client (#27869)

a6f2869

Relates to #27205

javanna added a commit that referenced this pull request Jan 23, 2018

REST high-level client: add support for update_all_types option to pu…

9d7ab44

…t mapping API Relates to #27869

javanna removed the backport pending label Jan 23, 2018

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

		PutMappingRequest putMappingRequest = new PutMappingRequest(indexName);

		putMappingRequest.type("type_name");

		assertTrue(putMappingResponse.isAcknowledged());

		Map<String, Object> indexMetaData = getIndexMetadata(indexName);


		String expectedRequestBody = "{}";

		assertEquals(expectedRequestBody, actualRequestBody);

Conversation

catalin-ursachi commented Dec 18, 2017

Uh oh!

elasticmachine commented Dec 18, 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!

dadoonet commented Dec 19, 2017

Uh oh!

catalin-ursachi commented Jan 3, 2018

Uh oh!

elasticmachine commented Jan 3, 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!

javanna left a comment • 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!

catalin-ursachi Jan 22, 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!

catalin-ursachi Jan 22, 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!

javanna left a comment •

edited

Loading

catalin-ursachi Jan 22, 2018 •

edited

Loading

catalin-ursachi Jan 22, 2018 •

edited

Loading

catalin-ursachi Jan 22, 2018 •

edited

Loading