Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix put mappings java API documentation #31955

Merged
merged 6 commits into from
Jul 17, 2018
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions docs/java-api/admin/indices/put-mapping.asciidoc
Original file line number Diff line number Diff line change
Expand Up @@ -2,17 +2,22 @@

==== Put Mapping

The PUT mapping API allows you to add a new type while creating an index:
You can add mappings for a new type already while creating an index:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the word "already" reads strangely here. what about "You can add mappings for a new type at index creation time".


["source","java",subs="attributes,callouts,macros"]
--------------------------------------------------
include-tagged::{client-tests}/IndicesDocumentationIT.java[index-with-mapping]
--------------------------------------------------
<1> <<java-admin-indices-create-index,Creates an index>> called `twitter`
<2> It also adds a `tweet` mapping type.
<2> Add a `tweet` type with a field called `message` that has the datatype `text`.

There are several variants of the above `addMapping` method, some taking an
`XContentBuilder` or a `Map` with the mapping definition as arguments. Make sure
to check the javadocs to pick the simplest one for your use case.

The PUT mapping API also allows to add a new type to an existing index:
The PUT mapping API also allows to specify the mapping of a type after index
creation. In this case you can provide the mapping as a String similar to the
Rest API syntax:

["source","java",subs="attributes,callouts,macros"]
--------------------------------------------------
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -184,10 +184,13 @@ public PutMappingRequest source(Object... source) {
}

/**
* @param type the mapping type
* @param source consisting of field/properties pairs (e.g. "field1",
* "type=string,store=true"). If the number of arguments is not
* divisible by two an {@link IllegalArgumentException} is thrown
* @param type
* the mapping type
* @param source
* consisting of field/properties pairs (e.g. "field1",
* "type=string,store=true")
* @throws IllegalArgumentException
* if the number of the source arguments is not divisible by two
* @return the mappings definition
*/
public static XContentBuilder buildFromSimplifiedDef(String type, Object... source) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -76,8 +76,12 @@ public void testValidation() {
" concrete index: [[foo/bar]] and indices: [myindex];");
}

/**
* Test that {@link PutMappingRequest#buildFromSimplifiedDef(String, Object...)}
* rejects inputs where the {@code Object...} varargs of field name and properties are not
* paired correctly
*/
public void testBuildFromSimplifiedDef() {
// test that method rejects input where input varargs fieldname/properites are not paired correctly
IllegalArgumentException e = expectThrows(IllegalArgumentException.class,
() -> PutMappingRequest.buildFromSimplifiedDef("type", "only_field"));
assertEquals("mapping source must be pairs of fieldnames and properties definition.", e.getMessage());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,16 @@

package org.elasticsearch.client.documentation;

import org.elasticsearch.action.admin.indices.mapping.get.GetMappingsResponse;
import org.elasticsearch.client.Client;
import org.elasticsearch.cluster.metadata.MappingMetaData;
import org.elasticsearch.common.collect.ImmutableOpenMap;
import org.elasticsearch.common.xcontent.XContentType;
import org.elasticsearch.test.ESIntegTestCase;

import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.instanceOf;

/**
* This class is used to generate the Java indices administration documentation.
* You need to wrap your code between two tags like:
Expand All @@ -48,16 +54,14 @@ public void testPutMappingDocumentation() throws Exception {
Client client = client();

// tag::index-with-mapping
client.admin().indices().prepareCreate("twitter") // <1>
.addMapping("\"tweet\": {\n" + // <2>
" \"properties\": {\n" +
" \"message\": {\n" +
" \"type\": \"text\"\n" +
" }\n" +
" }\n" +
"}")
client.admin().indices().prepareCreate("twitter") // <1>
.addMapping("tweet", "message", "type=text") // <2>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't sure which of the three "addMapping" variants to use in the client API docs, but went with this one since I think its the simplest. Both alternatives addMapping(String type, Map<String, Object> source) and addMapping(String type, XContentBuilder source) would require to setup a builder or a map with the mapping first, which would make this example quiet bloated IMHO.

.get();
// end::index-with-mapping
GetMappingsResponse getMappingsResponse = client.admin().indices().prepareGetMappings("twitter").get();
assertEquals(1, getMappingsResponse.getMappings().size());
ImmutableOpenMap<String, MappingMetaData> indexMapping = getMappingsResponse.getMappings().get("twitter");
assertThat(indexMapping.get("tweet"), instanceOf(MappingMetaData.class));

// we need to delete in order to create a fresh new index with another type
client.admin().indices().prepareDelete("twitter").get();
Expand Down Expand Up @@ -89,6 +93,11 @@ public void testPutMappingDocumentation() throws Exception {
"}", XContentType.JSON)
.get();
// end::putMapping-request-source
getMappingsResponse = client.admin().indices().prepareGetMappings("twitter").get();
assertEquals(1, getMappingsResponse.getMappings().size());
indexMapping = getMappingsResponse.getMappings().get("twitter");
assertThat(indexMapping.get("user"), instanceOf(MappingMetaData.class));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we really need this assertion, given the one immediately following? this feels like it could be an implementation detail.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure

assertThat(indexMapping.get("user").getSourceAsMap().toString(), equalTo("{properties={name={type=text}}}"));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

small thing, but the toString comparison feels a bit brittle here: whitespace and other kind of cosmetic changes can break this prettty easily. could we instead assert on the Map using containsKey or similar?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I was thinking about this but wanted to avoid few extra lines of code here, since I found they would also make the test harder to read. I generally agree with "toString" being brittle. In this case, since most maps use AbstractMap#toString(), it should be quite stable though. So maybe okay to trade for succinctness and readability here, wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm good with your reasoning.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I found a way that avoids "toString()" and is quite short as well, please take a look at the next commit to see if you like this better.


// tag::putMapping-request-source-append
client.admin().indices().preparePutMapping("twitter") // <1>
Expand All @@ -102,6 +111,11 @@ public void testPutMappingDocumentation() throws Exception {
"}", XContentType.JSON)
.get();
// end::putMapping-request-source-append
getMappingsResponse = client.admin().indices().prepareGetMappings("twitter").get();
assertEquals(1, getMappingsResponse.getMappings().size());
indexMapping = getMappingsResponse.getMappings().get("twitter");
assertThat(indexMapping.get("user"), instanceOf(MappingMetaData.class));
assertThat(indexMapping.get("user").getSourceAsMap().toString(), equalTo("{properties={name={type=text}, user_name={type=text}}}"));
}

}