Fix put mappings java API documentation #31955

cbuescher · 2018-07-11T10:39:47Z

The current docs of the put-mapping Java API is currently broken. In its current
form, it creates an index and uses the whole mapping definition given as a JSON
string as the type name. Since we didn't check the index created in the
IndicesDocumentationIT, this went unnoticed so far.

This change adds test to catch this error to the documentation test, changes the
documentation so it works correctly now and adds an input validation to
PutMappingRequest#buildFromSimplifiedDef() which was used internally to reject
calls where no mapping definition is given.

Closes #31906

The current docs of the put-mapping Java API is currently broken. It its current form, it creates an index and uses the whole mapping definition given as a JSON string as the type name. Since we didn't check the index created in the IndicesDocumentationIT so far this went unnoticed. This change adds test to catch this error to the documentation test, changes the documentation so it works correctly now and adds an input validation to PutMappingRequest#buildFromSimplifiedDef() which was used internally to reject calls where no mapping definition is given. Closes elastic#31906

elasticmachine · 2018-07-11T10:39:49Z

Pinging @elastic/es-search-aggs

cbuescher · 2018-07-11T10:43:30Z

server/src/test/java/org/elasticsearch/client/documentation/IndicesDocumentationIT.java

-                        "  }\n" +
-                        "}")
+        client.admin().indices().prepareCreate("twitter")    // <1>
+                .addMapping("tweet", "message", "type=text") // <2>


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.

…mplifiedDef

pcsanwald

Few small comments, but I'll defer to your judgement on any of these are worth fixing.

pcsanwald · 2018-07-16T10:58:55Z

docs/java-api/admin/indices/put-mapping.asciidoc

@@ -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:


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

pcsanwald · 2018-07-16T11:19:52Z

server/src/test/java/org/elasticsearch/client/documentation/IndicesDocumentationIT.java

+        getMappingsResponse = client.admin().indices().prepareGetMappings("twitter").get();
+        assertEquals(1, getMappingsResponse.getMappings().size());
+        indexMapping = getMappingsResponse.getMappings().get("twitter");
+        assertThat(indexMapping.get("user"), instanceOf(MappingMetaData.class));


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

pcsanwald · 2018-07-16T11:22:16Z

server/src/test/java/org/elasticsearch/client/documentation/IndicesDocumentationIT.java

+        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}}}"));


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?

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?

I'm good with your reasoning.

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.

The current docs of the put-mapping Java API is currently broken. It its current form, it creates an index and uses the whole mapping definition given as a JSON string as the type name. Since we didn't check the index created in the IndicesDocumentationIT so far this went unnoticed. This change adds test to catch this error to the documentation test, changes the documentation so it works correctly now and adds an input validation to PutMappingRequest#buildFromSimplifiedDef() which was used internally to reject calls where no mapping definition is given. Closes #31906

* 6.x: Fix rollup on date fields that don't support epoch_millis (#31890) Revert "Introduce a Hashing Processor (#31087)" (#32179) [test] use randomized runner in packaging tests (#32109) Painless: Fix caching bug and clean up addPainlessClass. (#32142) Fix BwC Tests looking for UUID Pre 6.4 (#32158) (#32169) Call setReferences() on custom referring tokenfilters in _analyze (#32157) Add more contexts to painless execute api (#30511) Add EC2 credential test for repository-s3 (#31918) Fix CP for namingConventions when gradle home has spaces (#31914) Convert Version to Java - clusterformation part1 (#32009) Fix Java 11 javadoc compile problem Improve docs for search preferences (#32098) Configurable password hashing algorithm/cost(#31234) (#32092) [DOCS] Update TLS on Docker for 6.3 ESIndexLevelReplicationTestCase doesn't support replicated failures but it's good to know what they are Switch distribution to new style Requests (#30595) Build: Skip jar tests if jar disabled Build: Move shadow customizations into common code (#32014) Painless: Add PainlessClassBuilder (#32141) Fix accidental duplication of bwc test for script behavior Handle missing values in painless (#30975) (#31903) Build: Make additional test deps of check (#32015) Painless: Fix Bug with Duplicate PainlessClasses (#32110) Adjust translog after versionType removed in 7.0 (#32020) Disable C2 from using AVX-512 on JDK 10 (#32138) [Rollup] Add new capabilities endpoint for concrete rollup indices (#32111) Mute :qa:mixed-cluster indices.stats/10_index/Index - all’ [ML] Wait for aliases in multi-node tests (#32086) Ensure to release translog snapshot in primary-replica resync (#32045) Docs: Fix missing example script quote (#32010) Add Index UUID to `/_stats` Response (#31871) (#32113) [ML] Move analyzer dependencies out of categorization config (#32123) [ML][DOCS] Add missing 6.3.0 release notes (#32099) Updates the build to gradle 4.9 (#32087) Update monitoring template version to 6040099 (#32088) Fix put mappings java API documentation (#31955) Add exclusion option to `keep_types` token filter (#32012)

@return

* master: Painless: Simplify Naming in Lookup Package (#32177) Handle missing values in painless (#32207) add support for write index resolution when creating/updating documents (#31520) ECS Task IAM profile credentials ignored in repository-s3 plugin (#31864) Remove indication of future multi-homing support (#32187) Rest test - allow for snapshots to take 0 milliseconds Make x-pack-core generate a pom file Rest HL client: Add put watch action (#32026) Build: Remove pom generation for plugin zip files (#32180) Fix comments causing errors with Java 11 Fix rollup on date fields that don't support epoch_millis (#31890) Detect and prevent configuration that triggers a Gradle bug (#31912) [test] port linux package packaging tests (#31943) Revert "Introduce a Hashing Processor (#31087)" (#32178) Remove empty @return from JavaDoc Adjust SSLDriver behavior for JDK11 changes (#32145) [test] use randomized runner in packaging tests (#32109) Add support for field aliases. (#32172) Painless: Fix caching bug and clean up addPainlessClass. (#32142) Call setReferences() on custom referring tokenfilters in _analyze (#32157) Fix BwC Tests looking for UUID Pre 6.4 (#32158) Improve docs for search preferences (#32159) use before instead of onOrBefore Add more contexts to painless execute api (#30511) Add EC2 credential test for repository-s3 (#31918) A replica can be promoted and started in one cluster state update (#32042) Fix Java 11 javadoc compile problem Fix CP for namingConventions when gradle home has spaces (#31914) Fix `range` queries on `_type` field for singe type indices (#31756) [DOCS] Update TLS on Docker for 6.3 (#32114) ESIndexLevelReplicationTestCase doesn't support replicated failures but it's good to know what they are Remove versionType from translog (#31945) Switch distribution to new style Requests (#30595) Build: Skip jar tests if jar disabled Painless: Add PainlessClassBuilder (#32141) Build: Make additional test deps of check (#32015) Disable C2 from using AVX-512 on JDK 10 (#32138) Build: Move shadow customizations into common code (#32014) Painless: Fix Bug with Duplicate PainlessClasses (#32110) Remove empty @param from Javadoc Re-disable packaging tests on suse boxes Docs: Fix missing example script quote (#32010) [ML] Wait for aliases in multi-node tests (#32086) [ML] Move analyzer dependencies out of categorization config (#32123) Ensure to release translog snapshot in primary-replica resync (#32045) Handle TokenizerFactory TODOs (#32063) Relax TermVectors API to work with textual fields other than TextFieldType (#31915) Updates the build to gradle 4.9 (#32087) Mute :qa:mixed-cluster indices.stats/10_index/Index - all’ Check that client methods match API defined in the REST spec (#31825) Enable testing in FIPS140 JVM (#31666) Fix put mappings java API documentation (#31955) Add exclusion option to `keep_types` token filter (#32012) [Test] Modify assert statement for ssl handshake (#32072)

cbuescher added review :Search Foundations/Mapping Index mappings, including merging and defining field types v7.0.0 v6.4.0 labels Jul 11, 2018

cbuescher commented Jul 11, 2018

View reviewed changes

Christoph Büscher added 2 commits July 11, 2018 14:56

Remove restriction on non-empty args in PutMappingRequest.buildFromSi…

48d993a

…mplifiedDef

iter

a1662d7

pcsanwald approved these changes Jul 16, 2018

View reviewed changes

Christoph Büscher added 3 commits July 16, 2018 13:39

iter

34514b2

iter

a2a7f0c

Merge branch 'master' into fix-31906

34566a0

cbuescher merged commit 3616526 into elastic:master Jul 17, 2018

cbuescher added v6.2.5 v6.3.2 labels Jul 17, 2018

cbuescher added v6.0.3 v6.1.5 labels Jul 17, 2018

pcsanwald mentioned this pull request Jul 24, 2018

[DOCS] Added RNs for 6.3.2 #32328

Merged

cbuescher added the >docs General docs changes label Jul 24, 2018

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix put mappings java API documentation #31955

Fix put mappings java API documentation #31955

cbuescher commented Jul 11, 2018 •

edited

Loading

elasticmachine commented Jul 11, 2018

cbuescher Jul 11, 2018

pcsanwald left a comment

pcsanwald Jul 16, 2018

pcsanwald Jul 16, 2018

cbuescher Jul 16, 2018

pcsanwald Jul 16, 2018

cbuescher Jul 16, 2018

pcsanwald Jul 16, 2018

cbuescher Jul 16, 2018

Fix put mappings java API documentation #31955

Fix put mappings java API documentation #31955

Conversation

cbuescher commented Jul 11, 2018 • edited Loading

elasticmachine commented Jul 11, 2018

Choose a reason for hiding this comment

pcsanwald 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

cbuescher commented Jul 11, 2018 •

edited

Loading