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

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

Merged
merged 2 commits into from Aug 11, 2017

Conversation

joschi
Copy link
Contributor

@joschi 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
Copy link
Contributor Author

joschi commented Aug 9, 2017

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

@bernd bernd self-assigned this Aug 11, 2017
Copy link
Member

@bernd bernd left a comment

Choose a reason for hiding this comment

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

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;

Copy link
Member

@bernd bernd left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM 👍

@bernd
Copy link
Member

bernd commented Aug 11, 2017

Waiting for the tests to turn green.

@bernd bernd merged commit 1aa1ba2 into master Aug 11, 2017
@bernd bernd deleted the issue-4068 branch August 11, 2017 12:52
joschi added a commit that referenced this pull request Aug 11, 2017
bernd pushed 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants