-
Notifications
You must be signed in to change notification settings - Fork 495
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
Added whitelistNodes and blacklistNodes #796
Conversation
…iltering for path expanders.
The failed checks don't look related to my changes. |
@@ -325,96 +325,4 @@ public void testCompoundLabelWorksInBlacklist() { | |||
assertEquals("Gene Hackman", node.getProperty("name")); | |||
}); | |||
} | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we have equivalent tests for the new behavior?
return whitelistAllowedEvaluation; | ||
if (!whitelistNodes.isEmpty()) { | ||
// ensure endNodes and terminatorNodes are whitelisted | ||
whitelistNodes.addAll(endNodes); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this a Set?
for (LabelMatcherGroup group : sequenceMatchers) { | ||
group.setEndNodesOnly(endNodesOnly); | ||
} | ||
Evaluator endAndTerminatorNodeEvaluator = NodeEvaluators.endAndTerminatorNodeEvaluator(endNodes, terminatorNodes); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can add multiple, individual evaluators, but then you are bound by the order and prioritization of the traversal-framework.
Looks good, I'll merge. But if possible please add a few tests if the new functionality is not yet covered enough. |
…iltering for path expanders. (#796)
…iltering for path expanders. (#796)
Just noting that the commits/functionality here didn't make it into any releases yet as of August 1 2018 |
Does this functionality actually work? I don't see unit tests in this diff. I also suspect this somehow broke teminatorNodes. |
@InverseFalcon can we see to add this to the 3.4 / 3.5 branches too, sorry that I missed it in cherry picking back then? |
whitelistNodes and blacklistNodes are present and working in 3.5.0.4, so they did get it, I'll check for what other versions contain the additions. As for tests, there's a new test file I added with this PR for the node filters here: If you have an example case that demonstrates a breakage with terminatorNodes, can you create a new issue for that for us to investigate? |
I believe the release notes are accurate, I can see proper behavior for whitelistNodes and blacklistNodes in 3.3.0.4 and 3.4.0.2, when they were first introduced. @marccelani If you were using a version after these, please create a new issue and let us know the version of Neo4j and APOC you were using, and a sample for recreating the behavior you noted. |
This finishes up node-specific filtering for path expanders.