Add analyze API to high-level rest client by romseygeek · Pull Request #31577 · elastic/elasticsearch

romseygeek · 2018-06-26T12:20:34Z

Relates to #27205

elasticmachine · 2018-06-26T12:20:36Z

Pinging @elastic/es-core-infra

nik9000

I think DetailedAnayzeResponse deserves some more testing, as does the request's to and from xcontent stuff.

nik9000 · 2018-06-26T14:18:16Z

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

+
+        Request request = RequestConverters.analyze(indexAnalyzeRequest);
+        assertThat(request.getEndpoint(), equalTo("/test_index/_analyze"));
+        assertThat(request.getEntity(), notNullValue());


I think other folks are using assertToXContentBody(validateQueryRequest, request.getEntity());. Might be worth leaving a comment about why you aren't using it so no one gets confused.

nik9000 · 2018-06-26T14:18:54Z

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

+        {
+            // tag::analyze-index-normalizer-request
+            AnalyzeRequest request = new AnalyzeRequest();
+            request.index("my_index");              // <1>


Make them line up?

nik9000 · 2018-06-26T14:20:44Z

server/src/main/java/org/elasticsearch/action/admin/indices/analyze/AnalyzeRequest.java

    private String normalizer;

-    public static class NameOrDefinition implements Writeable {
+    public static class NameOrDefinition implements Writeable, ToXContentObject {


You want ToXContentFragment, not ToXContentObject, I think.

nik9000 · 2018-06-26T14:24:27Z

server/src/main/java/org/elasticsearch/action/admin/indices/analyze/AnalyzeRequest.java

+        @Override
+        public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
+            if (Strings.isNullOrEmpty(name) == false) {
+                return builder.value(name);


This looks like it is missing the name.

nik9000 · 2018-06-26T14:26:24Z

server/src/main/java/org/elasticsearch/action/admin/indices/analyze/AnalyzeResponse.java

+                    attributes.put(field, parser.text());
+                }
+            }
+            return new AnalyzeToken(term, position, startOffset, endOffset, positionLength, type, attributes);


I think ConstructingObjectParser or ObjectParser would be better here because they make it easier to be sure that the error handling is good.

The tricky part here is that the response can contain arbitrary attributes, which are collected up into a Map and passed to the constructor. Is there a way of handling that within the ConstructingObjectParser?

No. We honestly need one because it keeps coming up....

nik9000 · 2018-06-26T14:27:03Z

server/src/main/java/org/elasticsearch/action/admin/indices/analyze/AnalyzeResponse.java

        return builder;
    }

+    public static AnalyzeResponse fromXContent(XContentParser parser) throws IOException {


I think the same thing is true here too.

nik9000 · 2018-06-26T14:27:37Z

server/src/main/java/org/elasticsearch/action/admin/indices/analyze/DetailAnalyzeResponse.java

+        AnalyzeTokenList tokenizer = null;
+        AnalyzeTokenList[] tokenfilters = null;
+        for (XContentParser.Token token = parser.nextToken(); token != XContentParser.Token.END_OBJECT; token = parser.nextToken()) {
+            if (token == XContentParser.Token.FIELD_NAME) {


Here too probably.

nik9000 · 2018-06-26T14:29:17Z

server/src/test/java/org/elasticsearch/action/admin/indices/analyze/AnalyzeResponseTests.java

+public class AnalyzeResponseTests extends AbstractStreamableXContentTestCase<AnalyzeResponse> {
+
+    @Override
+    protected boolean supportsUnknownFields() {


I general we want our response parsers to support unknown fields unless we can't for some reason. I think getRandomFieldsExcludeFilter is the right thing to do if parts of the response can't handle random fields.

romseygeek · 2018-06-28T22:34:26Z

retest this please

romseygeek · 2018-07-02T14:48:07Z

Thanks for the review @nik9000 , I think I've addressed all of your comments now.

nik9000

The java stuff LGTM. The docs build seems to fail with this change though.

nik9000 · 2018-07-02T17:41:05Z

server/src/main/java/org/elasticsearch/action/admin/indices/analyze/AnalyzeResponse.java

+                else if (Fields.POSITION_LENGTH.equals(field)) {
+                    positionLength = parser.intValue();
+                }
+                else if (Fields.TYPE.equals(field)) {


We usually stick else on the same line as }. We don't really have a standard though so if you have a strong opinion keeping it like this is fine.

nik9000 · 2018-07-02T17:44:03Z

server/src/main/java/org/elasticsearch/action/admin/indices/analyze/DetailAnalyzeResponse.java

+        return result;
+    }
+
    private boolean customAnalyzer = false;


Could you move the members above the ctor so I don't keep losing them when I try to read this class?

javanna · 2018-07-04T08:35:03Z

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

+include-tagged::{doc-tests}/IndicesClientDocumentationIT.java[analyze-field-request]
+---------------------------------------------------
+
+==== Optional arguemnts


argh, well spotted. Am I alright to directly commit a fix, or should I open another PR?

I am fine with pushing this fix directly

javanna · 2018-07-04T08:40:37Z

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

+        String index = request.index();
+        if (index != null) {
+            builder.addPathPart(index);
+        }


question: I see in the REST spec that we have also prefer_local and format. I don't see them supported in the corresponding REST action though. Can you double check? Maybe those params should be removed from the SPEC?

prefer_local was removed by commit cafc707 and I don't think format has ever been supported. I'll open a new PR to change the rest-spec, and include the typo fix in that one too

sounds good thanks!

* 6.x: Fix not waiting for Netty ThreadDeathWatcher in IT (#31758) (#31789) [Docs] Correct default window_size (#31582) S3 fixture should report 404 on unknown bucket (#31782) [ML] Limit ML filter items to 10K (#31731) Fixture for Minio testing (#31688) [ML] Return statistics about forecasts as part of the jobsstats and usage API (#31647) [DOCS] Add missing get mappings docs to HLRC (#31765) [DOCS] Starting Elasticsearch (#31701) Fix coerce validation_method in GeoBoundingBoxQueryBuilder (#31747) Painless: Complete Removal of Painless Type (#31699) Consolidate watcher setting update registration (#31762) [DOCS] Adds empty 6.3.1 release notes page ingest: Introduction of a bytes processor (#31733) [test] don't run bats tests for suse boxes (#31749) Add analyze API to high-level rest client (#31577) Implemented XContent serialisation for GetIndexResponse (#31675) [DOCS] Typos DOC: Add examples to the SQL docs (#31633) Add support for AWS session tokens (#30414) Watcher: Reenable start/stop yaml tests (#31754) JDBC: Fix stackoverflow on getObject and timestamp conversion (#31735) Support multiple system store types (#31650) Add write*Blob option to replace existing blob (#31729) Split CircuitBreaker-related tests (#31659) Painless: Add Context Docs (#31190) Docs: Remove missing reference Migrate scripted metric aggregation scripts to ScriptContext design (#30111) Watcher: Fix chain input toXcontent serialization (#31721) Remove _all example (#31711) rest-high-level: added get cluster settings (#31706) Docs: Match the examples in the description (#31710) [Docs] Correct typos (#31720) Extend allowed characters for grok field names (#21745) (#31653) (#31722) [DOCS] Check for Windows and *nix file paths (#31648) [ML] Validate ML filter_id (#31535) Fix gradle4.8 deprecation warnings (#31654) Update numbers to reflect 4-byte UTF-8-encoded characters (#27083)

* master: [ML] Rate limit established model memory updates (#31768) [Docs] Correct default window_size (#31582) S3 fixture should report 404 on unknown bucket (#31782) Detach Transport from TransportService (#31727) [ML] Limit ML filter items to 10K (#31731) [ML] Return statistics about forecasts as part of the jobsstats and usage API (#31647) Fixture for Minio testing (#31688) [DOCS] Add missing get mappings docs to HLRC (#31765) [DOCS] Starting Elasticsearch (#31701) Painless: Complete Removal of Painless Type (#31699) Fix not waiting for Netty ThreadDeathWatcher in IT (#31758) Consolidate watcher setting update registration (#31762) Build: re-enabled bwc (#31769) ingest: Introduction of a bytes processor (#31733) Fix coerce validation_method in GeoBoundingBoxQueryBuilder (#31747) Add analyze API to high-level rest client (#31577) [DOCS] Typos DOC: Add examples to the SQL docs (#31633) Add support for AWS session tokens (#30414) Watcher: Reenable start/stop yaml tests (#31754) Implemented XContent serialisation for GetIndexResponse (#31675) JDBC: Fix stackoverflow on getObject and timestamp conversion (#31735) resolveHasher defaults to NOOP (#31723) Account for XContent overhead in in-flight breaker Split CircuitBreaker-related tests (#31659) Add write*Blob option to replace existing blob (#31729) Painless: Add Context Docs (#31190) Watcher: Fix chain input toXcontent serialization (#31721) Docs: Match the examples in the description (#31710) rest-high-level: added get cluster settings (#31706) [Docs] Correct typos (#31720) Clean up double semicolon code typos (#31687) [DOCS] Check for Windows and *nix file paths (#31648) [ML] Validate ML filter_id (#31535) Revert long lines Fix TransportChangePasswordActionTests

romseygeek added 5 commits June 20, 2018 13:30

Non-detailed case

e2f73bb

RequestConverters test

2563b4b

Merge branch 'master' into analyze-hlrest

6332155

Add detailed response

3bd541c

docs

722214e

romseygeek added >feature v7.0.0 :Core/Java High Level REST Client v6.4.0 labels Jun 26, 2018

romseygeek self-assigned this Jun 26, 2018

romseygeek requested a review from javanna June 26, 2018 12:20

romseygeek added 2 commits June 26, 2018 13:32

checkstyle

2c4ee45

checkstyle

c8c5008

javanna mentioned this pull request Jun 26, 2018

Java high-level REST client completeness #27205

Closed

80 tasks

nik9000 reviewed Jun 26, 2018

View reviewed changes

romseygeek added 7 commits June 27, 2018 15:24

docs fix

69e1119

Use ConstructingObjectParser

52d2c41

handle unknown fields

7435228

Add randomization to building responses for tests

ceb6601

checkstyle

fc936ee

fixes

ba2cec1

checkstyle

d573dbb

romseygeek added 3 commits June 29, 2018 08:22

Merge remote-tracking branch 'origin/master' into analyze-hlrest

57ec84e

Deal with arbitrary objects as attributes

070c3cb

compilation, apparently

08d2766

nik9000 requested changes Jul 2, 2018

View reviewed changes

feedback

0e7ce01

docs fixes

20fc870

nik9000 approved these changes Jul 3, 2018

View reviewed changes

romseygeek merged commit 1d11407 into elastic:master Jul 3, 2018

romseygeek deleted the analyze-hlrest branch July 3, 2018 14:57

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

Add analyze API to high-level rest client (#31577)

439e67f

javanna removed their request for review July 4, 2018 08:32

javanna reviewed Jul 4, 2018

View reviewed changes

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

Conversation

romseygeek commented Jun 26, 2018 • edited by nik9000 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Jun 26, 2018

Uh oh!

nik9000 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!

romseygeek commented Jun 28, 2018

Uh oh!

romseygeek commented Jul 2, 2018

Uh oh!

nik9000 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

romseygeek commented Jun 26, 2018 •

edited by nik9000

Loading