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

filter out null values when iterating over select_jsonpath results #233

Merged
merged 2 commits into from Dec 7, 2017

Conversation

@kroepke
Copy link
Member

@kroepke kroepke commented Dec 7, 2017

fix #232

@kroepke kroepke added this to the 2.4.0 milestone Dec 7, 2017
@ghost ghost assigned kroepke Dec 7, 2017
));
.map(entry -> Maps.immutableEntry(entry.getKey(), unwrapJsonNode(entry.getValue().read(json, configuration))))
.filter(entry -> entry.getValue() != null)
.collect(toMap(Map.Entry::getKey, Map.Entry::getValue));

This comment has been minimized.

@joschi

joschi Dec 7, 2017
Contributor

Call me old-fashioned, but couldn't this be solved shorter and faster with the following change?

diff --git a/plugin/src/main/java/org/graylog/plugins/pipelineprocessor/functions/json/SelectJsonPath.java b/plugin/src/main/java/org/graylog/plugins/pipelineprocessor/functions/json/SelectJsonPath.java
index 2ea6222..b18f249 100644
--- a/plugin/src/main/java/org/graylog/plugins/pipelineprocessor/functions/json/SelectJsonPath.java
+++ b/plugin/src/main/java/org/graylog/plugins/pipelineprocessor/functions/json/SelectJsonPath.java
@@ -19,7 +19,6 @@ package org.graylog.plugins.pipelineprocessor.functions.json;
 import com.fasterxml.jackson.databind.JsonNode;
 import com.fasterxml.jackson.databind.ObjectMapper;
 import com.google.common.collect.ImmutableList;
-import com.google.common.collect.Maps;
 import com.google.inject.TypeLiteral;
 import com.jayway.jsonpath.Configuration;
 import com.jayway.jsonpath.JsonPath;
@@ -31,9 +30,11 @@ import org.graylog.plugins.pipelineprocessor.ast.functions.FunctionArgs;
 import org.graylog.plugins.pipelineprocessor.ast.functions.FunctionDescriptor;
 import org.graylog.plugins.pipelineprocessor.ast.functions.ParameterDescriptor;

+import javax.annotation.Nullable;
 import javax.inject.Inject;
 import java.io.IOException;
 import java.util.Collections;
+import java.util.HashMap;
 import java.util.Map;

 import static com.google.common.collect.ImmutableList.of;
@@ -74,16 +75,14 @@ public class SelectJsonPath extends AbstractFunction<Map<String, Object>> {
         if (json == null || paths == null) {
             return Collections.emptyMap();
         }
-        // a plain .collect(toMap(...)) will fail on null values, because of the HashMap#merge method in its implementation
-        // since json nodes at certain paths might be missing, the value could be null.
-        // filter null values out first
-        return paths
-                .entrySet().stream()
-                .map(entry -> Maps.immutableEntry(entry.getKey(), unwrapJsonNode(entry.getValue().read(json, configuration))))
-                .filter(entry -> entry.getValue() != null)
-                .collect(toMap(Map.Entry::getKey, Map.Entry::getValue));
+        final Map<String, Object> map = new HashMap<>();
+        for (Map.Entry<String, JsonPath> entry : paths.entrySet()) {
+            map.put(entry.getKey(), unwrapJsonNode(entry.getValue().read(json, configuration)));
+        }
+        return map;
     }

+    @Nullable
     private Object unwrapJsonNode(Object value) {
         if (!(value instanceof JsonNode)) {
             return value;

At least FunctionsSnippetsTest is green with that.

This comment has been minimized.

@kroepke

kroepke Dec 7, 2017
Author Member

Sure, I don't care which way, I just added to what was there previously.

@joschi
joschi approved these changes Dec 7, 2017
@joschi joschi merged commit ecc33b8 into master Dec 7, 2017
2 of 4 checks passed
2 of 4 checks passed
continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/travis-ci/push The Travis CI build could not complete due to an error
Details
@garybot2
graylog-project/pr Jenkins build graylog-project-pr-snapshot 794 has succeeded
Details
license/cla Contributor License Agreement is signed.
Details
@ghost ghost removed the ready-for-review label Dec 7, 2017
@joschi joschi deleted the issue-232 branch Dec 7, 2017
joschi pushed a commit that referenced this pull request Dec 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants