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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Always trim message field values on Message class #2510

Merged
merged 4 commits into from Jul 28, 2016

Conversation

Projects
None yet
4 participants
@edmundoa
Member

edmundoa commented Jul 20, 2016

Before this change, one of the public constructors of Message was using addField() to add fields in the Message object, while the other was adding the fields by hand to the internal Map used to store message fields.

That created an inconsistency, as the constructor using addField() trims all String field values by default, unlike the other constructor. It lead to the issue mentioned in: #1936, and it also affected to extractors and drool rules. Basically, every filter used in the MessageFilterChainProcessor.

This commit ensures both constructors trim String values, while still ensuring Message(final String message, final String source, final DateTime timestamp) will create a Message object containing the required fields given as arguments.

Making this change will break stream rules, extractors, and drool rules checking for one or more leading or trailing whitespaces in message fields.

I am still working on some documentation around this PR, that's why it is missing the ready-to-review tag, but feel free to add any concerns or comments 馃槃

Fixes #1936.

@edmundoa edmundoa added this to the 2.1.0 milestone Jul 20, 2016

@bernd

This comment has been minimized.

Member

bernd commented Jul 20, 2016

Please add the upgrade notes to UPGRADING.rst once you have them. 馃槂

@dennisoelkers dennisoelkers self-assigned this Jul 21, 2016

@@ -309,7 +317,7 @@ public void addField(final String key, final Object value) {
} else if(value instanceof String) {
final String str = ((String) value).trim();
if(!str.isEmpty()) {
if (isRequiredField || !str.isEmpty()) {

This comment has been minimized.

@joschi

joschi Jul 22, 2016

Contributor

Why would we handle "required" fields different from "normal" fields in this case? Wouldn't this introduce another inconsistency?

This comment has been minimized.

@joschi

joschi Jul 22, 2016

Contributor

Also, why should we trim empty "required" fields?

This comment has been minimized.

@edmundoa

edmundoa Jul 22, 2016

Member

Yes, you are right, it adds another consistency to keep the old behaviour, that is, messages created with the Message(final String message, final String source, final DateTime timestamp) constructor will contain at least the message, and source fields, even if their values are empty (or spaces).

I added the special case to fix some tests, but maybe it doesn't make sense. How do you think we should handle the case when a message is created with empty message or source fields?

edmundoa and others added some commits Jul 20, 2016

Always trim message field values on Message class
Before this change, one of the public constructors of `Message` was using
`addField()` to add fields in the `Message` object, while the other was
adding the fields by hand to the internal `Map` used to store message
fields.

That created an inconsistency, as the constructor using `addField()`
trims all `String` field values by default, unlike the other constructor.
You can read more about it in: #1936.

This commit ensures both constructors trim `String` values, while still
ensuring `Message(final String message, final String source, final
DateTime timestamp)` will create a `Message` object containing the
required fields given as arguments.

Making this change will break stream rules, extractors, and drool rules
checking for one or more leading or trailing whitespaces in message fields.

Fixes #1936

@dennisoelkers dennisoelkers force-pushed the issue-1936 branch from caec59a to 7d54c54 Jul 26, 2016

addField(key, value, true);
}
private void addField(final String key, final Object value, final boolean isRequiredField) {
// Don't accept protected keys. (some are allowed though lol)
if (RESERVED_FIELDS.contains(key) && !RESERVED_SETTABLE_FIELDS.contains(key) || !validKey(key)) {

This comment has been minimized.

@dennisoelkers

dennisoelkers Jul 26, 2016

Member

While we are at it: why is the trimmed key used further down, while it is used non-trimmed here? Shouldn't we fix that to make it consistent?

This comment has been minimized.

@edmundoa

edmundoa Jul 26, 2016

Member

That's a good question, and I don't have the answer for it. Honestly, I didn't want to touch more than I had to, to fix this issue. I'll take a look.

@@ -131,9 +131,9 @@
public Message(final String message, final String source, final DateTime timestamp) {
// Adding the fields directly because they would not be accepted as a reserved fields.

This comment has been minimized.

@dennisoelkers

dennisoelkers Jul 26, 2016

Member

This comment is now out of sync with the following code.

Use trimmed key on addField
Use the trimmed key consistently when adding a new field.

@joschi joschi self-assigned this Jul 28, 2016

@joschi

This comment has been minimized.

Contributor

joschi commented Jul 28, 2016

LGTM. 馃憤

@joschi joschi merged commit 31787c6 into master Jul 28, 2016

4 checks passed

ci-server-integration Jenkins build graylog2-server-integration-pr 1167 has succeeded
Details
ci-web-linter Jenkins build graylog-pr-linter-check 650 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-1936 branch Jul 28, 2016

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