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

Matching inverted exact/regex stream rules when field is not present #2172

Merged
merged 2 commits into from May 3, 2016

Conversation

Projects
None yet
2 participants
@dennisoelkers
Member

dennisoelkers commented May 2, 2016

Before this change, inverted exact/regex stream rules did not match when the field was not present. This was counter intuitive and perceived as erroneus, this PR Is changing this behavior.

Fixes #2160

dennisoelkers added some commits May 2, 2016

Inverted regex/exact rules should match when field is not present.
Before this change, inverted exact/regex rules did not match when the
field is not present or null. This is changed and reflected in the
tests.

Fixes #2160.

@dennisoelkers dennisoelkers added the bug label May 2, 2016

@dennisoelkers dennisoelkers added this to the 2.0.1 milestone May 2, 2016

return rule.getInverted() ^ value.toString().trim().equals(rule.getValue());
final String value = msg.getField(rule.getField()).toString();
return rule.getInverted() ^ value.trim().equals(rule.getValue());

This comment has been minimized.

@joschi

joschi May 2, 2016

Contributor

Minor nitpick: While this works, I'd prefer using the logical OR operator here.

This comment has been minimized.

@dennisoelkers

dennisoelkers May 3, 2016

Member

This is not part of the scope of this PR.

This comment has been minimized.

@joschi

joschi May 3, 2016

Contributor

I think we could do some opportunistic refactoring in this case (and actually fix an error that is accidentally working).

This comment has been minimized.

@dennisoelkers

dennisoelkers May 3, 2016

Member

What is the error here?

This comment has been minimized.

@joschi

joschi May 3, 2016

Contributor

Using the bitwise OR operator (^) instead of the logical OR operator (||) as it's a logical expression to be evaluated and not some bit-shifting.

This comment has been minimized.

@dennisoelkers

dennisoelkers May 3, 2016

Member

This is perfectly fine for booleans, as specified in the JLS: http://docs.oracle.com/javase/specs/jls/se8/html/jls-15.html#jls-15.22.2

This comment has been minimized.

@joschi

joschi May 3, 2016

Contributor

Hm, I think I'm getting confused now. The ^ bitwise operator and the || logical operator also have different characteristics:

For ^, the result value is true if the operand values are different; otherwise, the result is false.

Is this the intended behavior? The || operator would return true if at least one of the operands is true while the ^ operator would return true only if the operands are different (e. g. both are false). So ^ is like a logical XOR.

This comment has been minimized.

@dennisoelkers

dennisoelkers May 3, 2016

Member

Yeah, this is intended. The critical difference is that if the rule is inverted, then it should not match if the message and the rule fields are equal. (field "foo" should not be "bar")

This comment has been minimized.

@joschi

joschi May 3, 2016

Contributor

I interpreted the intention of that expression completely wrong. Everything's fine.

Everything's fine

@joschi joschi self-assigned this May 2, 2016

@joschi

This comment has been minimized.

Contributor

joschi commented May 3, 2016

LGTM. 👍

@joschi joschi merged commit 391697a into master May 3, 2016

4 checks passed

ci-server-integration Jenkins build graylog2-server-integration-pr 877 has succeeded
Details
ci-web-linter Jenkins build graylog-pr-linter-check 365 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

@joschi joschi deleted the issue-2160 branch May 3, 2016

joschi added a commit that referenced this pull request May 3, 2016

Matching inverted exact/regex stream rules when field is not present (#…
…2172)

Inverted regex/exact rules should match when field is not present.

Before this change, inverted exact/regex rules did not match when the field is not present or null. This is changed and reflected in the tests.

Fixes #2160.
(cherry picked from commit 391697a)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment