fix fieldref assignment to not assume mutable object #62
Conversation
| # this assumes implementation specific feature of returning a mutable object | ||
| # from a field ref which should not be assumed and will change in the future. | ||
|
|
||
| tags = event["tags"] || [] |
There was a problem hiding this comment.
can we just replace this with event.tag(tag) and change the default implementation of tag to do this?
I think this cleanup fits the whole refactoring you're doing :)
There was a problem hiding this comment.
oh yes, good catch, I don't know why I did not correctly clean this. is can be replaced with a simple:
event.tag(tag)
|
@jsvd followup review request please! |
| if matched | ||
| filter_matched(event) | ||
| else | ||
| # Tag this event if we can't parse it. We can use this later to | ||
| # reparse+reindex logs if we improve the patterns given. | ||
| @tag_on_failure.each do |tag| | ||
| event["tags"] ||= [] | ||
| event["tags"] << tag unless event["tags"].include?(tag) | ||
| # do not replace the code below with: |
There was a problem hiding this comment.
I think it's safe to remove this warning, it makes sense in the example below where a developer will instinctively try to optimize/dry/refactor code, here I doubt someone would replace event.tag(tag) with 2 lines of more complex code
There was a problem hiding this comment.
good point :P did not really pay attention to the comment.
|
minor comment otherwise LGTM |
|
thanks, will fix comment and merge! |
fix fieldref assignment to not assume mutable object use Event#tag remove useless comment
colinsurprenant
force-pushed
the
fix/fieldref
branch
from
9242887
to
bc0ad81
Compare
|
pushed version 2.0.3 |
this is related to problem described in elastic/logstash#4140 and also relates to elastic/logstash#4191
we cannot assume anymore that field reference getter returns a mutable object.
tracked in elastic/logstash#4264
sorry for the whitespaces diffs :(