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

Re-enable "_source" fetching in search requests with selected fields #4069

Merged
merged 2 commits into from Aug 11, 2017

Conversation

Projects
None yet
2 participants
@joschi
Contributor

joschi commented Aug 9, 2017

The inclusion of the _source field has to be explicitly re-enabled if a search request specified the fields field to only return specific fields.

By default operations return the contents of the _source field unless you have used the fields parameter or if the _source field is disabled.

https://www.elastic.co/guide/en/elasticsearch/reference/2.4/search-request-source-filtering.html

Fixes #4068

@joschi joschi added this to the 2.4.0 milestone Aug 9, 2017

@joschi

This comment has been minimized.

Contributor

joschi commented Aug 9, 2017

This has to be back-ported into the 2.3 branch.

@bernd bernd self-assigned this Aug 11, 2017

@bernd

While this fixes the NPE when using the fields parameter, it also returns all fields in the result and thus makes the fields parameter basically a no-op.
The problem is that Jest cannot handle responses without a _source field yet.

See:

I think we should use source filtering instead of just including the source document. (as the TODO entry suggests 😉) The following diff works for me. There is no NPE and the result only contains the selected fields in addition to the timestamp and message id fields.

diff --git graylog2-server/src/main/java/org/graylog2/indexer/searches/Searches.java graylog2-server/src/main/java/org/graylog2/indexer/searches/Searches.java
index 259840051..12e590565 100644
--- graylog2-server/src/main/java/org/graylog2/indexer/searches/Searches.java
+++ graylog2-server/src/main/java/org/graylog2/indexer/searches/Searches.java
@@ -614,12 +613,11 @@ public class Searches {
         }
 
         if (config.fields() != null) {
-            // http://www.elasticsearch.org/guide/en/elasticsearch/reference/current/search-request-fields.html#search-request-fields
-            // "For backwards compatibility, if the fields parameter specifies fields which are not stored , it will
-            // load the _source and extract it from it. This functionality has been replaced by the source filtering
-            // parameter."
-            // TODO: Look at the source filtering parameter once we switched to ES 1.x.
-            request.fields(config.fields());
+            // Use source filtering instead of SearchSourceBuilder#fields() here because Jest cannot handle responses
+            // without a "_source" field yet. See:
+            // https://github.com/searchbox-io/Jest/issues/157
+            // https://github.com/searchbox-io/Jest/issues/339
+            request.fetchSource(config.fields().toArray(new String[config.fields().size()]), new String[0]);
         }
 
         return request;
@bernd

bernd approved these changes Aug 11, 2017

Thanks, LGTM 👍

@bernd

This comment has been minimized.

Member

bernd commented Aug 11, 2017

Waiting for the tests to turn green.

@bernd bernd merged commit 1aa1ba2 into master Aug 11, 2017

3 of 4 checks passed

graylog-project/pr Jenkins build graylog-project-pr-snapshot 387 has failed
Details
ci-web-linter Jenkins build graylog-pr-linter-check 1858 has succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@bernd bernd deleted the issue-4068 branch Aug 11, 2017

joschi added a commit that referenced this pull request Aug 11, 2017

bernd added a commit that referenced this pull request Aug 11, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment