merge into existing target, new tests and DRYing #69
Conversation
| @@ -411,8 +411,10 @@ def filter(event) | |||
| return if kv.empty? | |||
There was a problem hiding this comment.
If you fancy, removing this line should satisfy #11 too.
There was a problem hiding this comment.
good point. my concern is with BWC but we could make this a major version bump in that respect and have better consistency in the plugin behaviour.
| event.set(@target, kv) | ||
| @logger.debug? && @logger.debug("Merging into existing target field", :target => @target) | ||
| t = event.get(@target) | ||
| t = {} unless t.is_a?(Hash) |
There was a problem hiding this comment.
Consider that if t is a scalar or array, it just goes away, this becomes a "if target is a hash: merge, otherwise overwrite" operation.
I think this is a very uncommon edge case that can be covered by updating the docs to advise the user, that if they want to keep the value at target, they should stash the value at target and add it back to the new target hash at the key of their choosing. If we support #11 then the "add back" will not fail because target is always a hash.
There was a problem hiding this comment.
Yes, that was my reasoning: if t is anything else than a hash then drop it. Thanks for mentioning #11 - this is also a legitimate concern indeed. Also looking at the add_field behaviour, it will turn the value into an array if the field exists, so we do try to avoid loosing values.
|
Have a look at something you did in the csv filter on Dec 3, 2015 - for consistency in writing fields and values to an event. 😉 😆 |
|
@guyboertje good catch about csv. I actually forgot about that. But at the same time it does not really help with our problem here since kv is always about creating a hash into the target. In csv, all field/value are converted into Our problem here is what do we do with
|
This fixes issue #43 which is a regression introduced into the kv filter v2.0.3 and above.
Previously if an existing
targetfield existed in the event, the kv fields were merged but the refactor at 2.0.3 changed that and now thetargetfield is always overwritten.This PR brings back the pre 2.0.3 behaviour and also add a specific test for that. The specs have also been cleaned up a little bit to DRY the plugin instance creation.
Note that this regression has made it through 2 major versions of the plugin but nonetheless I think it makes sense to not loose existing fields in the target if it exists.