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

Add support for field capabilities to the high-level REST client. #29664

Merged

Conversation

Projects
None yet
5 participants
@jtibshirani
Copy link
Member

commented Apr 23, 2018

Addresses part of #27205.

@jtibshirani jtibshirani force-pushed the jtibshirani:feature/rest-field-capabilities branch 3 times, most recently Apr 23, 2018

@elasticmachine

This comment has been minimized.

Copy link
Collaborator

commented Apr 24, 2018

@javanna
Copy link
Member

left a comment

hi @jtibshirani this looks good! I left some comments but it's is already pretty close. Thanks!

client/rest-high-level/src/test/java/org/elasticsearch/client/FieldCapabilitiesIT.java Outdated
() -> execute(request, highLevelClient()::fieldCaps, highLevelClient()::fieldCapsAsync));
}

public void testNonexistentIndices() {

This comment has been minimized.

Copy link
@javanna

javanna Apr 24, 2018

Member

nit: NonExistent?

This comment has been minimized.

Copy link
@jtibshirani

jtibshirani Apr 24, 2018

Author Member

Oops, I usually spell it 'nonexistent' :) Will fix to be consistent with other tests.

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

expectThrows(IllegalArgumentException.class,
() -> execute(request, highLevelClient()::fieldCaps, highLevelClient()::fieldCapsAsync));
}

This comment has been minimized.

Copy link
@javanna

javanna Apr 24, 2018

Member

here we are testing request validation? shall we move this test to a unit test? for instance FieldCapabilitiesRequestTests?

This comment has been minimized.

Copy link
@jtibshirani

jtibshirani Apr 24, 2018

Author Member

I moved this to the method FieldCapabilitiesRequestTests#testValidation.

This comment has been minimized.

Copy link
@javanna

javanna Apr 25, 2018

Member

thanks

client/rest-high-level/src/test/java/org/elasticsearch/client/RequestTests.java Outdated
assertEquals(4, request.getParameters().size());

// Note that we don't check the field param value explicitly, as field
// names are added to the request in a non-deterministic order.

This comment has been minimized.

Copy link
@javanna

javanna Apr 24, 2018

Member

Couldn't we check that the fields set are the expected ones? Could we do something similar to what we do here: https://github.com/elastic/elasticsearch/blob/master/client/rest-high-level/src/test/java/org/elasticsearch/client/RequestTests.java#L332 ?

This comment has been minimized.

Copy link
@jtibshirani

jtibshirani Apr 24, 2018

Author Member

FieldCapabilitiesRequest#fields differs from GetRequest#sortedFields in that before adding fields to the request, it passes them through a hash set for de-duplication. I tried to clarify the comment a bit, and ended up adding a stronger check.

This comment has been minimized.

Copy link
@javanna

javanna Apr 26, 2018

Member

gotcha! This is quite annoying for testing, also technically if you provide an array, you would expect an array with same ordering when retrieving through the getter. Not a huge deal but I wonder if we should use a LinkedHashSet for deduplication.

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

import static org.hamcrest.Matchers.hasKey;

public class FieldCapabilitiesIT extends ESRestHighLevelClientTestCase {

This comment has been minimized.

Copy link
@javanna

javanna Apr 24, 2018

Member

Would it be possible to add these new tests to SearchIT?

This comment has been minimized.

Copy link
@jtibshirani

jtibshirani Apr 24, 2018

Author Member

Sure, I moved them. I was copying rank eval, which has its own RankEvalIT -- later we may want to combine that too for consistency.

server/src/test/java/org/elasticsearch/action/fieldcaps/FieldCapabilitiesResponseTests.java Outdated
return new FieldCapabilitiesResponse(responses);
}

public void testFromXContent() throws IOException {

This comment has been minimized.

Copy link
@javanna

javanna Apr 24, 2018

Member

I think that we should make this test class extend AbstractStreamableXContentTestCase, then this test would come almost for free, as well as equals/hashcode tests and the existing serialization test.

This comment has been minimized.

Copy link
@jtibshirani

jtibshirani Apr 25, 2018

Author Member

Updated, thanks!

XContentParserUtils.ensureExpectedToken(XContentParser.Token.START_OBJECT, parser.nextToken(), parser::getTokenLocation);
XContentParser.Token token;
while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) {
XContentParserUtils.ensureExpectedToken(XContentParser.Token.FIELD_NAME, token, parser::getTokenLocation);

This comment has been minimized.

Copy link
@javanna

javanna Apr 24, 2018

Member

I wonder if these two ensureExpectedToken don't make parsing too strict, if so the insertion of random fields should make tests fail if configured properly.

This comment has been minimized.

Copy link
@jtibshirani

jtibshirani Apr 24, 2018

Author Member

In tests, I set the 'excludes filter' to prevent adding random fields under certain paths. I assumed it wasn't okay to have malformed entries in that map or the one below it, as each key represents a field or type name: https://www.elastic.co/guide/en/elasticsearch/reference/current/search-field-caps.html#_response_format

I'm not generally clear though on when we aim to be lenient in terms of parsing -- any suggestions/ guidance is much appreciated!

This comment has been minimized.

Copy link
@javanna

javanna Apr 26, 2018

Member

I had another look at how you test this, and I think that this is all fine. The reason why we are lenient with responses is that we want to be able to parse responses from future versions of Elasticsearch, which may or may not change the format of the response in backwards compatible ways. If we are strict even backwards compatible changes end up breaking our parsing code. In this case though, I think that if these two ensureExpectedToken were to throw exception, it would be due to a breaking change, which we can't do much about.

--------------------------------------------------
include-tagged::{doc-tests}/SearchDocumentationIT.java[field-caps-request]
--------------------------------------------------

This comment has been minimized.

Copy link
@javanna

javanna Apr 24, 2018

Member

shall we add the usual "Optional arguments" section here that describes the indices options arguments?

This comment has been minimized.

Copy link
@jtibshirani

jtibshirani Apr 25, 2018

Author Member

Added a section.

PARSER.declareStringArray(ConstructingObjectParser.optionalConstructorArg(), INDICES_FIELD);
PARSER.declareStringArray(ConstructingObjectParser.optionalConstructorArg(), NON_SEARCHABLE_INDICES_FIELD);
PARSER.declareStringArray(ConstructingObjectParser.optionalConstructorArg(), NON_AGGREGATABLE_INDICES_FIELD);
}

This comment has been minimized.

Copy link
@javanna

javanna Apr 24, 2018

Member

maybe we could move FieldCapabilitiesTests to extend AbstractSerializingTestCase given that we can now parse it back.

This comment has been minimized.

Copy link
@jtibshirani

jtibshirani Apr 25, 2018

Author Member

Updated too.

import java.util.List;
import java.util.Map;
import java.util.stream.Collectors;

/**
* Response for {@link FieldCapabilitiesRequest} requests.
*/
public class FieldCapabilitiesResponse extends ActionResponse implements ToXContentFragment {

This comment has been minimized.

Copy link
@javanna

javanna Apr 24, 2018

Member

Would it be possible to make this a ToXContentObject? then we could also adapt the corresponding REST action and use RestToXContentListener in there.

This comment has been minimized.

Copy link
@jtibshirani

jtibshirani Apr 24, 2018

Author Member

Would it be okay if I did this in a follow-up PR? It would be nice to keep this one tightly-scoped to the REST client addition.

This comment has been minimized.

Copy link
@javanna

javanna Apr 25, 2018

Member

yes sure we usually make this change as part these PRs mostly because in many cases we also go from ToXContentFragment to ToXContentObject. I am fine with a follow-up PR.

This comment has been minimized.

Copy link
@jtibshirani

jtibshirani Apr 26, 2018

Author Member

Addressed in #30182.

* If set to <code>true</code> the response will contain only a merged view of the per index field capabilities.
* Otherwise only unmerged per index field capabilities are returned.
*
* Note that when using the high-level REST client, results are always merged (this flag is always considered 'true').

This comment has been minimized.

Copy link
@javanna

javanna Apr 24, 2018

Member

unrelated, but I am wondering why we allow to set fields either from the request body or from url parameters. I thought that one way should be enough. That also forces us to make a choice between the two in the high-level REST client. We are going for parameters now, that also means that the request object parses fields but doesn't print it out, which generally leads to no unit test coverage for the parsing code :) @jimczi do you recall why this choice was made?

This comment has been minimized.

Copy link
@jimczi

jimczi Apr 24, 2018

Member

Good catch, this is the legacy of _field_stats but this is not very useful here since fields is the only possible field in the body. Let's deprecate requests with body and remove in 7 ?

This comment has been minimized.

Copy link
@javanna

javanna Apr 25, 2018

Member

@jtibshirani would you like to address this too as a follow-up?

This comment has been minimized.

Copy link
@jtibshirani

jtibshirani Apr 25, 2018

Author Member

Sure, would be happy to.

This comment has been minimized.

Copy link
@jtibshirani

jtibshirani Apr 26, 2018

Author Member

Addressed in #30157 and #30185.

@jtibshirani jtibshirani force-pushed the jtibshirani:feature/rest-field-capabilities branch 3 times, most recently Apr 24, 2018

@jtibshirani

This comment has been minimized.

Copy link
Member Author

commented Apr 24, 2018

Thanks @javanna for the review -- I tried to address all your comments + pushed new changes.

@javanna javanna referenced this pull request Apr 24, 2018

Open

Java high-level REST client completeness #27205

75 of 80 tasks complete

@jtibshirani jtibshirani force-pushed the jtibshirani:feature/rest-field-capabilities branch 2 times, most recently Apr 25, 2018

@javanna
Copy link
Member

left a comment

left a few more comments but this LGTM! thanks al lot @jtibshirani !

@@ -93,4 +95,11 @@ public void testFieldCapsRequestSerialization() throws IOException {
assertEquals(deserialized.hashCode(), request.hashCode());
}

This comment has been minimized.

Copy link
@javanna

javanna Apr 26, 2018

Member

maybe while at it we could make this test extend AbstractWireSerializingTestCase, it would save us some code and make things more consistent. Could also be a followup though.

This comment has been minimized.

Copy link
@jtibshirani

jtibshirani Apr 26, 2018

Author Member

Makes sense, will also do in the follow-up PR.

This comment has been minimized.

Copy link
@jtibshirani

jtibshirani Apr 26, 2018

Author Member

Addressed in #30181 (converting the test actually caught a bug).

FieldCapabilitiesRequest request = new FieldCapabilitiesRequest()
.indices("index2");
ActionRequestValidationException exception = request.validate();
assertNotNull(exception);

This comment has been minimized.

Copy link
@javanna

javanna Apr 26, 2018

Member

shall we also add a little test that validation fails when no indices are provided. I know you haven't changed the validation, but given that we are adding a test for it... :)

This comment has been minimized.

Copy link
@jtibshirani

jtibshirani Apr 26, 2018

Author Member

It's actually okay to provide no indices -- if none are provided, then it defaults to all indices.

This comment has been minimized.

Copy link
@javanna

javanna Apr 30, 2018

Member

right, I got confused, thanks for clarifying

XContentParserUtils.ensureExpectedToken(XContentParser.Token.START_OBJECT, parser.nextToken(), parser::getTokenLocation);
XContentParser.Token token;
while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) {
XContentParserUtils.ensureExpectedToken(XContentParser.Token.FIELD_NAME, token, parser::getTokenLocation);

This comment has been minimized.

Copy link
@javanna

javanna Apr 26, 2018

Member

I had another look at how you test this, and I think that this is all fine. The reason why we are lenient with responses is that we want to be able to parse responses from future versions of Elasticsearch, which may or may not change the format of the response in backwards compatible ways. If we are strict even backwards compatible changes end up breaking our parsing code. In this case though, I think that if these two ensureExpectedToken were to throw exception, it would be due to a breaking change, which we can't do much about.

@jtibshirani jtibshirani merged commit d40116d into elastic:master Apr 26, 2018

3 checks passed

CLA Commit author has signed the CLA
Details
elasticsearch-ci Build finished.
Details
elasticsearch-ci/packaging-sample Build finished.
Details

@jtibshirani jtibshirani deleted the jtibshirani:feature/rest-field-capabilities branch Apr 26, 2018

dnhatn added a commit that referenced this pull request Apr 27, 2018

Merge branch 'master' into ccr
* master: (24 commits)
  Watcher: Ensure mail message ids are unique per watch action (#30112)
  REST: Remove GET support for clear cache indices (#29525)
  SQL: Correct error message (#30138)
  Require acknowledgement to start_trial license (#30135)
  Fix a bug in FieldCapabilitiesRequest#equals and hashCode. (#30181)
  SQL: Add BinaryMathProcessor to named writeables list (#30127)
  Tests: Use buildDir as base for generated-resources (#30191)
  Fix SliceBuilderTests#testRandom failures
  Build: Fix deb version to use tilde with prerelease versions (#29000)
  Fix edge cases in CompositeKeyExtractorTests (#30175)
  Document time unit limitations for date histograms (#30177)
  Add support for field capabilities to the high-level REST client. (#29664)
  Remove licenses missed by the migration (#30128)
  [DOCS] Updates docker installation package details (#30110)
  Fix TermsSetQueryBuilder.doEquals() method (#29629)
  [Monitoring] Remove unhelpful Monitoring tests (#30144)
  [Test] Fix RenameProcessorTests.testRenameExistingFieldNullValue() (#29655)
  add copyright/scope configuration for intellij to Contributing Guide (#29688)
  [test] include oss tar in packaging tests (#30155)
  TEST: Update settings should go through cluster state (#29682)
  ...

@jtibshirani jtibshirani added the v7.0.0 label Apr 27, 2018

@javanna

This comment has been minimized.

Copy link
Member

commented May 22, 2018

hi @jtibshirani was this backported? If not could you please take care of backporting to 6.x?

@jtibshirani

This comment has been minimized.

Copy link
Member Author

commented May 22, 2018

No, this wasn't backported either -- will do that now!

@jtibshirani jtibshirani added the v6.4.0 label May 22, 2018

jtibshirani added a commit to jtibshirani/elasticsearch that referenced this pull request May 22, 2018

dnhatn added a commit that referenced this pull request May 24, 2018

Merge branch '6.x' into ccr-6.x
* 6.x:
  [DOCS] Fixes typos in security settings
  Add support for indexed shape routing in geo_shape query (#30760)
  [DOCS] Splits auditing.asciidoc into smaller files
  Painless: Types Section Clean Up (#30283)
  [test] java tests for archive packaging (#30734)
  Deprecate http.pipelining setting (#30786)
  [DOCS] Fix more edit URLs in Stack Overview (#30704)
  Use correct cluster state version for node fault detection (#30810)
  [DOCS] Fixes broken link for native realm
  [DOCS] Clarified audit.index.client.hosts (#30797)
  Change serialization version of doc-value fields.
  Add a `format` option to `docvalue_fields`. (#29639)
  [TEST] Don't expect acks when isolating nodes
  Fixes UpdateSettingsRequestStreamableTests mutate bug
  Revert "Add more yaml tests for get alias API (#29513)"
  Revert "Mutes MachineLearningTests.testNoAttributes_givenSameAndMlEnabled"
  Only allow x-pack metadata if all nodes are ready (#30743)
  Mutes MachineLearningTests.testNoAttributes_givenSameAndMlEnabled
  Use original settings on full-cluster restart (#30780)
  Only ack cluster state updates successfully applied on all nodes (#30672)
  Replace Request#setHeaders with addHeader (#30588)
  [TEST] remove endless wait in RestClientTests (#30776)
  QA: Add xpack tests to rolling upgrade (#30795)
  Add support for search templates to the high-level REST client. (#30473)
  Reduce CLI scripts to one-liners on Windows (#30772)
  Fold RestGetAllSettingsAction in RestGetSettingsAction (#30561)
  Add more yaml tests for get alias API (#29513)
  [Docs] Fix script-fields snippet execution (#30693)
  Convert FieldCapabilitiesResponse to a ToXContentObject. (#30182)
  Remove assert statements from field caps documentation. (#30601)
  Fix a bug in FieldCapabilitiesRequest#equals and hashCode. (#30181)
  Add support for field capabilities to the high-level REST client. (#29664)
  [DOCS] Add SAML configuration information (#30548)
  [DOCS] Remove X-Pack references from SQL CLI (#30694)
  [Docs] Fix typo in circuit breaker docs (#29659)
  [Feature] Adding a char_group tokenizer (#24186)
  Increase the maximum number of filters that may be in the cache. (#30655)
  [Docs] Fix broken cross link in documentation
  Test: wait for netty threads in a JUnit ClassRule (#30763)
  [Security] Include an empty json object in an json array when FLS filters out all fields (#30709)
  [DOCS] fixed incorrect default
  [TEST] Wait for CS to be fully applied in testDeleteCreateInOneBulk
  Enable installing plugins from snapshots.elastic.co (#30765)
  Ignore empty completion input (#30713)
  Fix docs failure on language analyzers (#30722)
  [Docs] Fix inconsistencies in snapshot/restore doc (#30480)
  Add Delete Repository High Level REST API (#30666)
  Reduce CLI scripts to one-liners (#30759)

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.