Add get field mappings to High Level REST API Client #31423

vladimirdolzhenko · 2018-06-19T09:37:49Z

Add get field mappings to High Level REST API Client
Relates to #27205

Relates to elastic#27205

elasticmachine · 2018-06-19T09:37:50Z

Pinging @elastic/es-core-infra

javanna · 2018-06-19T10:01:58Z

@nik9000 @hub-cap @cbuescher does either of you have time to review this one?

nik9000 · 2018-06-19T13:24:54Z

Will look.

nik9000 · 2018-06-19T15:02:53Z

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

@@ -908,6 +923,13 @@ static String endpoint(String[] indices, String endpoint, String[] suffixes) {
                .addCommaSeparatedPathParts(suffixes).build();
    }

+    static String endpoint(String[] indices, String endpoint, String[] suffixes, String extraEndpoint, String[] extraSuffixes) {


I think I'd make just call the EndpointBuilder directly rather than making a method. Unless you know that you are going to add a few more uses of the method.

makes sense. thanks

nik9000 · 2018-06-19T15:05:41Z

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

+            getFieldMappingsRequest.types((String[]) null);
+        }
+
+        String[] fields = new String[randomIntBetween(1, 5)];


You support leaving these null to, right? Maybe add a randomization option for that.

nik9000 · 2018-06-19T15:07:42Z

libs/x-content/src/main/java/org/elasticsearch/common/xcontent/XContentBuilder.java

@@ -253,6 +255,10 @@ public XContentBuilder startObject() throws IOException {
        return this;
    }

+    public XContentBuilder startObject(ParseField field) throws IOException {


I'm not sure we should add this one. Maybe. But I think it is worth its own little PR if we do.

… - nulls are welcome

nik9000

LGTM

javanna · 2018-06-19T16:43:22Z

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

+     * @return the response
+     * @throws IOException in case there is a problem sending the request or parsing back the response
+     */
+    public GetFieldMappingsResponse getFieldMappings(GetFieldMappingsRequest getFieldMappingsRequest,


it should be singular getFieldMapping. that's what we have in the SPEC

@javanna there is inconsistency with a class name GetFieldMappingsResponse

there are plenty of these, for the method names we follow the SPEC regardless of class names. If we could we would change the class names but we are trying to make it easy for people to migrate off of the transport client so we don't do that.

i see the difficulties - sounds ok to me

javanna · 2018-06-19T16:44:51Z

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

+--------------------------------------------------
+<1> Options for expanding indices names
+
+[[java-rest-high-get-field-mappings-sync]]


aren't we missing a couple of supported parameters here?

good spotted - I found only one - local, pls let me know if anything else you have in mind

javanna · 2018-06-19T16:45:57Z

...t/java/org/elasticsearch/action/admin/indices/mapping/get/GetFieldMappingsResponseTests.java

+
+    @Override
+    protected Predicate<String> getRandomFieldsExcludeFilter() {
+        return s -> true;


what is the value of supporting unknown fields if the exclude filter always returns true?

aren't there parts of the response where we could inject random fields?

in fact not so much - but there are couple, fixed

…API SPEC

…m fields at some level in test

(cherry picked from commit 38b7bd4)

vladimirdolzhenko · 2018-06-20T09:15:30Z

@javanna could you pls have another look into this ?

javanna

left a couple of comments, LGTM otherwise

javanna · 2018-06-21T09:50:29Z

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

+
+        setRandomIndicesOptions(getFieldMappingsRequest::indicesOptions, getFieldMappingsRequest::indicesOptions, expectedParams);
+
+        if (randomBoolean()) {


you can use setRandomLocal

nope, you can't - setRandomLocal requires MasterNodeReadRequest while GetFieldMappingsRequest does not extend it

oh right we could make the comment accept a consumer instead then, we did it in other places, up to you on whether it's worthwhile or not.

javanna · 2018-06-21T09:52:53Z

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

+                final Map<String, Object> source = metaData1.sourceAsMap();
+
+                assertThat(fullName, equalTo("message"));
+                assertThat(source, equalTo(Collections.singletonMap("message", Collections.singletonMap("type", "text"))));


do we need these assertions here? given that this test is added to test the docs snippets, I would avoid testing the functionality itself.

this is still here?

javanna · 2018-06-21T09:58:45Z

...c/main/java/org/elasticsearch/action/admin/indices/mapping/get/GetFieldMappingsResponse.java

+                final Object o = ((Map) value).get(MAPPINGS.getPreferredName());
+                if (!(o instanceof Map)) {
+                    throw new ParsingException(parser.getTokenLocation(), "Nested " + MAPPINGS.getPreferredName() + " is not found");
+                }


is it necessary to parse everything into a map in the first place? At some point later on we do need a map, but I wonder if we can postpone calling parser.map till then.

it's quite good point, will try to do that

javanna · 2018-06-21T09:59:56Z

...t/java/org/elasticsearch/action/admin/indices/mapping/get/GetFieldMappingsResponseTests.java

+        // otherwise random field could be evaluated as index name or type name
+        return s -> {
+            final int c = s.length() > 0 ? charCount(s, '.') : -1;
+            return c != 0 && c != 3;};


maybe a regex would be more readable here :)

sure https://xkcd.com/208/

vladimirdolzhenko · 2018-06-21T14:42:24Z

test this please

…appings_hlrestapi_

vladimirdolzhenko · 2018-06-21T17:19:04Z

@javanna last glance pls

javanna · 2018-06-22T12:32:08Z

...c/main/java/org/elasticsearch/action/admin/indices/mapping/get/GetFieldMappingsResponse.java

+                    final String fullName = (String) map3.get(FieldMappingMetaData.FULL_NAME.getPreferredName());
+                    final XContentBuilder jsonBuilder = jsonBuilder();
+                    final Map<String, ?> values = (Map<String, ?>) map3.get(FieldMappingMetaData.MAPPING.getPreferredName());
+                    jsonBuilder.map(values);


here you may be able to use copyCurrentStructure

unfortunately - we already call parser.map() - therefore copyCurrentStructure is not feasible here

makes sense

So, why call map in the first place? It feels like maybe we should use the pull parser a bit more, build the map as we go.

javanna · 2018-06-22T12:33:58Z

...c/main/java/org/elasticsearch/action/admin/indices/mapping/get/GetFieldMappingsResponse.java

@@ -93,9 +143,34 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
        return builder;
    }

+    private static class Builder {


do we need this class given that it has a single member? Can't we use directly the map member where needed?

it's doable! thanks

nik9000 · 2018-06-22T14:11:33Z

...c/main/java/org/elasticsearch/action/admin/indices/mapping/get/GetFieldMappingsResponse.java

+        ensureExpectedToken(XContentParser.Token.START_OBJECT, parser.nextToken(), parser::getTokenLocation);
+
+        final Map<String, Map<String, Map<String, FieldMappingMetaData>>> mappings = new HashMap<>();
+        final ObjectParser objectParser = createParser(mappings);


It is much more normal to create the objectParser statically. I see that you did it this way so that you could collect the results into something, but that isn't really what object parser is for. I mean, you can do it by passing the mappings map as the context to the parse method and throwing out the return value, but that is weird. Why not have the ObjectParser return the typeMappings?

even better - agree

nik9000 · 2018-06-22T14:14:41Z

...c/main/java/org/elasticsearch/action/admin/indices/mapping/get/GetFieldMappingsResponse.java

+                    final String fullName = (String) map3.get(FieldMappingMetaData.FULL_NAME.getPreferredName());
+                    final XContentBuilder jsonBuilder = jsonBuilder();
+                    final Map<String, ?> values = (Map<String, ?>) map3.get(FieldMappingMetaData.MAPPING.getPreferredName());
+                    jsonBuilder.map(values);


So, why call map in the first place? It feels like maybe we should use the pull parser a bit more, build the map as we go.

vladimirdolzhenko · 2018-06-22T19:05:49Z

thanks @nik9000 for suggestion - I did another try - indeed it looks way better

nik9000 · 2018-06-22T19:13:29Z

...c/main/java/org/elasticsearch/action/admin/indices/mapping/get/GetFieldMappingsResponse.java

+                final Map<String, FieldMappingMetaData> typeMapping = new HashMap<>();
+                typeMappings.put(typeName, typeMapping);
+
+                if (p.nextToken() == XContentParser.Token.START_OBJECT) {


So, if nextToken is START_ARRAY I think this falls over. I think the outer while loop terminates in a weird spot. I think we should skip children in that case.

nik9000

LGTM

vladimirdolzhenko · 2018-06-23T07:39:06Z

thanks @nik9000 and @javanna for the review 👍

* master: Add get field mappings to High Level REST API Client (#31423) [DOCS] Updates Watcher examples for code testing (#31152) TEST: Add bwc recovery tests with synced-flush index [DOCS] Move sql to docs (#31474) [DOCS] Move monitoring to docs folder (#31477) Core: Combine doExecute methods in TransportAction (#31517) IndexShard should not return null stats (#31528) fix repository update with the same settings but different type (#31458) Fix Mockito trying to mock IOException that isn't thrown by method (#31433) (#31527) Node selector per client rather than per request (#31471) Core: Combine messageRecieved methods in TransportRequestHandler (#31519) Upgrade to Lucene 7.4.0. (#31529) [ML] Add ML filter update API (#31437) Allow multiple unicast host providers (#31509) Avoid deprecation warning when running the ML datafeed extractor. (#31463) REST high-level client: add simulate pipeline API (#31158) Get Mapping API to honour allow_no_indices and ignore_unavailable (#31507) [PkiRealm] Invalidate cache on role mappings change (#31510) [Security] Check auth scheme case insensitively (#31490) In NumberFieldType equals and hashCode, make sure that NumberType is taken into account. (#31514) [DOCS] Fix REST tests in SQL docs [DOCS] Add code snippet testing in more ML APIs (#31339) Core: Remove ThreadPool from base TransportAction (#31492) [DOCS] Remove fixed file from build.gradle Rename createNewTranslog to fileBasedRecovery (#31508) Test: Skip assertion on windows [DOCS] Creates field and document level security overview (#30937) [DOCS] Significantly improve SQL docs [DOCS] Move migration APIs to docs (#31473) Core: Convert TransportAction.execute uses to client calls (#31487) Return transport addresses from UnicastHostsProvider (#31426) Ensure local addresses aren't null (#31440) Remove unused generic type for client execute method (#31444) Introduce http and tcp server channels (#31446)

Add get field mappings to High Level REST API Client Relates to #27205

* elastic/master: (92 commits) Reduce number of raw types warnings (elastic#31523) Migrate scripted metric aggregation scripts to ScriptContext design (elastic#30111) turn GetFieldMappingsResponse to ToXContentObject (elastic#31544) Close xcontent parsers (partial) (elastic#31513) Ingest Attachment: Upgrade Tika to 1.18 (elastic#31252) TEST: Correct the assertion arguments order (elastic#31540) Add get field mappings to High Level REST API Client (elastic#31423) [DOCS] Updates Watcher examples for code testing (elastic#31152) TEST: Add bwc recovery tests with synced-flush index [DOCS] Move sql to docs (elastic#31474) [DOCS] Move monitoring to docs folder (elastic#31477) Core: Combine doExecute methods in TransportAction (elastic#31517) IndexShard should not return null stats (elastic#31528) fix repository update with the same settings but different type (elastic#31458) Fix Mockito trying to mock IOException that isn't thrown by method (elastic#31433) (elastic#31527) Node selector per client rather than per request (elastic#31471) Core: Combine messageRecieved methods in TransportRequestHandler (elastic#31519) Upgrade to Lucene 7.4.0. (elastic#31529) [ML] Add ML filter update API (elastic#31437) Allow multiple unicast host providers (elastic#31509) ...

Add get field mappings to High Level REST API Client

1730875

Relates to elastic#27205

vladimirdolzhenko added >enhancement :Core/Java High Level REST Client labels Jun 19, 2018

vladimirdolzhenko requested a review from javanna June 19, 2018 09:37

javanna mentioned this pull request Jun 19, 2018

Java high-level REST client completeness #27205

Closed

80 tasks

javanna removed their request for review June 19, 2018 09:59

vladimirdolzhenko added v7.0.0 v6.4.0 labels Jun 19, 2018

nik9000 requested changes Jun 19, 2018

View reviewed changes

Vladimir Dolzhenko added 2 commits June 19, 2018 17:30

don't introduce ParseField in XContentBuilder so far

b350c33

don't introduce complex endpoint method; improve fields randomization…

4cb66ae

… - nulls are welcome

nik9000 approved these changes Jun 19, 2018

View reviewed changes

javanna reviewed Jun 19, 2018

View reviewed changes

Vladimir Dolzhenko added 3 commits June 20, 2018 09:02

rename getFieldMappings to getFieldMapping for consistency with REST …

207a8d5

…API SPEC

simplification of GetFieldMappingsResponse.fromXContent + allow rando…

f095c57

…m fields at some level in test

add missed doc section for get-field-mapping-request-local

ce482f1

(cherry picked from commit 38b7bd4)

vladimirdolzhenko requested a review from javanna June 20, 2018 09:15

javanna approved these changes Jun 21, 2018

View reviewed changes

Vladimir Dolzhenko added 4 commits June 21, 2018 13:06

clean up

2205664

use regexp in getRandomFieldsExcludeFilter

88192e8

add and use setRandomLocal(Consumer)

e38c145

rebuilt fromXContent

a7403ed

Merge remote-tracking branch 'remotes/origin/master' into get_field_m…

b6265f7

…appings_hlrestapi_

polish a bit randomFieldsExcludeFilter regexp

a12c32e

javanna reviewed Jun 22, 2018

View reviewed changes

Vladimir Dolzhenko added 2 commits June 22, 2018 14:36

drop unnecessary assertThat-s

3424790

drop intermediate builder from fromXContent process

03b0396

nik9000 reviewed Jun 22, 2018

View reviewed changes

Vladimir Dolzhenko added 2 commits June 22, 2018 17:22

unused import...

c991dbc

another round of fromXContent simplification

fa77332

nik9000 reviewed Jun 22, 2018

View reviewed changes

improve junk protection on parsing

7d61d23

nik9000 approved these changes Jun 22, 2018

View reviewed changes

vladimirdolzhenko merged commit b7ef75f into elastic:master Jun 23, 2018

vladimirdolzhenko deleted the get_field_mappings_hlrestapi branch June 23, 2018 07:39

colings86 pushed a commit that referenced this pull request Jun 25, 2018

Add get field mappings to High Level REST API Client (#31423)

27bd688

Add get field mappings to High Level REST API Client Relates to #27205

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


		setRandomIndicesOptions(getFieldMappingsRequest::indicesOptions, getFieldMappingsRequest::indicesOptions, expectedParams);

		if (randomBoolean()) {

Add get field mappings to High Level REST API Client #31423

Add get field mappings to High Level REST API Client #31423

Conversation

vladimirdolzhenko commented Jun 19, 2018

elasticmachine commented Jun 19, 2018

javanna commented Jun 19, 2018

nik9000 commented Jun 19, 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

Choose a reason for hiding this comment

nik9000 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vladimirdolzhenko Jun 19, 2018 • edited Loading

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

vladimirdolzhenko commented Jun 20, 2018

javanna 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

vladimirdolzhenko commented Jun 21, 2018

vladimirdolzhenko commented Jun 21, 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

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

vladimirdolzhenko commented Jun 22, 2018

Choose a reason for hiding this comment

nik9000 left a comment

Choose a reason for hiding this comment

vladimirdolzhenko commented Jun 23, 2018

vladimirdolzhenko Jun 19, 2018 •

edited

Loading