Always trim message field values on Message class #2510
Conversation
|
Please add the upgrade notes to |
| @@ -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()) { | |||
There was a problem hiding this comment.
Why would we handle "required" fields different from "normal" fields in this case? Wouldn't this introduce another inconsistency?
There was a problem hiding this comment.
Also, why should we trim empty "required" fields?
There was a problem hiding this comment.
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?
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
force-pushed
the
issue-1936
branch
from
caec59a
to
7d54c54
Compare
| 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)) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
Use the trimmed key consistently when adding a new field.
|
LGTM. 👍 |
Before this change, one of the public constructors of
Messagewas usingaddField()to add fields in theMessageobject, while the other was adding the fields by hand to the internalMapused to store message fields.That created an inconsistency, as the constructor using
addField()trims allStringfield 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 theMessageFilterChainProcessor.This commit ensures both constructors trim
Stringvalues, while still ensuringMessage(final String message, final String source, final DateTime timestamp)will create aMessageobject 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-reviewtag, but feel free to add any concerns or comments 😄Fixes #1936.