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.

Sign up for GitHub

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

Jump to bottom

Fix serialization of empty field capabilities response #33263

Merged
merged 4 commits into from
Aug 30, 2018
Merged

Fix serialization of empty field capabilities response #33263

merged 4 commits into from
Aug 30, 2018

Conversation

jimczi
Copy link
Contributor

@jimczi jimczi commented Aug 30, 2018

When no response are required (no indices match the requested patterns) the
empty response throws an NPE in the transport serialization (writeTo).

@jimczi
Fix serialization of empty field capabilities response
6a55390 
When no response are required (no indices match the requested patterns) the
empty response throws an NPE in the transport serialization (writeTo).
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

javanna
javanna reviewed Aug 30, 2018
View reviewed changes
server/src/main/java/org/elasticsearch/action/fieldcaps/FieldCapabilitiesResponse.java Outdated
@@ -65,6 +65,7 @@ private FieldCapabilitiesResponse(Map<String, Map<String, FieldCapabilities>> re
*/
FieldCapabilitiesResponse() {
this.responseMap = Collections.emptyMap();
this.indexResponses = Collections.emptyList();
Copy link
Member

Choose a reason for hiding this comment

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

super minor nit: we say in the javadocs that this is used for serialization, but it's also used when the response is empty (totalNumRequest == 0 in TransportFieldCapabilitiesAction). Maybe we could have the transport action use one of the other constructors and provide an empty map? And maybe we should validate that what is provided is never null?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

++, I pushed a605160

javanna
javanna reviewed Aug 30, 2018
View reviewed changes
server/src/test/java/org/elasticsearch/search/fieldcaps/FieldCapabilitiesIT.java Outdated
FieldCapabilitiesResponse response = client().prepareFieldCaps("empty_index_pattern*")
.setFields("*")
.execute().actionGet();
assertEquals(0, response.get().size());
Copy link
Member

Choose a reason for hiding this comment

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

This could be done from a unit test as well right? Create the response using the default constructor and call writeTo on it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I pushed a605160

javanna
javanna approved these changes Aug 30, 2018
View reviewed changes
Copy link
Member

@javanna javanna left a comment

Choose a reason for hiding this comment

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

left two nits, LGTM otherwise, good catch!

javanna
javanna reviewed Aug 30, 2018
View reviewed changes
server/src/main/java/org/elasticsearch/action/fieldcaps/FieldCapabilitiesAction.java Outdated
@@ -32,6 +34,6 @@ private FieldCapabilitiesAction() {

@Override
public FieldCapabilitiesResponse newResponse() {
return new FieldCapabilitiesResponse();
return new FieldCapabilitiesResponse(Collections.emptyMap());
Copy link
Member

Choose a reason for hiding this comment

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

Wasn't this one for serialization? I would have changed the other call from the TransportAction, this is ok as-is I think, unless we want to completely get rid of the default constructor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oups, I fixed, thanks !

javanna
javanna approved these changes Aug 30, 2018
View reviewed changes
Copy link
Member

@javanna javanna left a comment

Choose a reason for hiding this comment

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

thanks @jimczi

@jimczi jimczi merged commit d063009 into elastic:master Aug 30, 2018
@jimczi jimczi deleted the field_caps_empty_serialization branch August 30, 2018 16:08
jimczi added a commit that referenced this pull request Aug 30, 2018
@jimczi
Fix serialization of empty field capabilities response (#33263)
34f13f8 
Fix serialization of empty field capabilities response

When no response are required (no indices match the requested patterns) the
empty response throws an NPE in the transport serialization (writeTo).
jimczi added a commit that referenced this pull request Aug 30, 2018
@jimczi
Fix serialization of empty field capabilities response (#33263)
4a548b0 
Fix serialization of empty field capabilities response

When no response are required (no indices match the requested patterns) the
empty response throws an NPE in the transport serialization (writeTo).
dnhatn added a commit that referenced this pull request Sep 1, 2018
@dnhatn
Merge branch '6.x' into ccr-6.x
a6312a5 
* 6.x:
  Mute test watcher usage stats output
  [Rollup] Fix FullClusterRestart test
  TEST: Disable soft-deletes in ParentChildTestCase
  TEST: Disable randomized soft-deletes settings
  Integrates soft-deletes into Elasticsearch (#33222)
  drop `index.shard.check_on_startup: fix` (#32279)
  Fix AwaitsFix issue number
  Mute SmokeTestWatcherWithSecurityIT testsi
  [DOCS] Moves ml folder from x-pack/docs to docs (#33248)
  TEST: mute more SmokeTestWatcherWithSecurityIT tests
  [DOCS] Move rollup APIs to docs (#31450)
  [DOCS] Rename X-Pack Commands section (#33005)
  Fixes SecurityIntegTestCase so it always adds at least one alias (#33296)
  TESTS: Fix Random Fail in MockTcpTransportTests (#33061) (#33307)
  MINOR: Remove Dead Code from PathTrie (#33280) (#33306)
  Fix pom for build-tools (#33300)
  Lazy evaluate java9home (#33301)
  SQL: test coverage for JdbcResultSet (#32813)
  Work around to be able to generate eclipse projects (#33295)
  Different handling for security specific errors in the CLI. Fix for #33230 (#33255)
  [ML] Refactor delimited file structure detection (#33233)
  SQL: Support multi-index format as table identifier (#33278)
  Enable forbiddenapis server java9 (#33245)
  [MUTE] SmokeTestWatcherWithSecurityIT flaky tests
  Add region ISO code to GeoIP Ingest plugin (#31669) (#33276)
  Don't be strict for 6.x
  Update serialization versions for custom IndexMetaData backport
  Replace IndexMetaData.Custom with Map-based custom metadata (#32749)
  Painless: Fix Bindings Bug (#33274)
  SQL: prevent duplicate generation for repeated aggs (#33252)
  TEST: Mute testMonitorClusterHealth
  Fix serialization of empty field capabilities response (#33263)
  Fix nested _source retrieval with includes/excludes (#33180)
  [DOCS] TLS file resources are reloadable (#33258)
  Watcher: Ensure TriggerEngine start replaces existing watches (#33157)
  Ignore module-info in jar hell checks (#33011)
  Fix docs build after #33241
  [DOC] Repository GCS ADC not supported (#33238)
  Upgrade to latest Gradle 4.10  (#32801)
  Fix/30904 cluster formation part2 (#32877)
  Move file-based discovery to core (#33241)
  HLRC: add client side RefreshPolicy (#33209)
  [Kerberos] Add unsupported languages for tests (#33253)
  Watcher: Reload properly on remote shard change (#33167)
  Fix classpath security checks for external tests. (#33066)
  [Rollup] Only allow aggregating on multiples of configured interval (#32052)
  Added deprecation warning for rescore in scroll queries (#33070)
  Apply settings filter to get cluster settings API (#33247)
  [Rollup] Re-factor Rollup Indexer into a generic indexer for re-usability   (#32743)
  HLRC: create base timed request class (#33216)
  HLRC: Use Optional in validation logic (#33104)
  Painless: Add Bindings (#33042)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@javanna javanna javanna approved these changes

Assignees
No one assigned
Labels
>bug :Core/Infra/Transport API Transport client API v6.4.1 v6.5.0 v7.0.0-beta1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jimczi @elasticmachine @javanna @colings86