Regression: Fix assigning to nil map in DeepMerge #18143
Conversation
If a Value in a MapStr itself has the type MapStr but is nil, we will have a panic in DeepUpdate. This change fixes the Update functionality to handle typed nil values.
urso
added
bug
review
libbeat
needs_backport
|
Pinging @elastic/integrations-services (Team:Services) |
botelastic
bot
added
the
needs_team
| @@ -102,9 +102,17 @@ func deepUpdateValue(old interface{}, val MapStr, overwrite bool) interface{} { | |||
|
|
|||
| switch sub := old.(type) { | |||
| case MapStr: | |||
| if sub == nil { | |||
| return val | |||
| } | |||
There was a problem hiding this comment.
Surprised that this isn't caught already by the if old == nil check at the start of this function. 🤔
There was a problem hiding this comment.
Just found https://groups.google.com/forum/#!topic/golang-nuts/wnH302gBa4I/discussion.
Should we remove the code on lines 99-101?
There was a problem hiding this comment.
There is nil, and then there is nil :)
An interface value is a tuple (type, value). An interface is nil, if and only if both type and value are nil. This is why the initial check fails here. We have a type (type MapStr, nil). The switch statement only looks for type, unpacks it, and we finally have the nil value in sub. This is when the call panics.
Removing line 99 should be safe. E.g.: https://play.golang.org/p/uajkDxQW9qH
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
Steps errorsExpand to view the steps failures
|
andresrc
added
[zube]: Inbox
[zube]: In Review
and removed
needs_team
💔 Build Failed
Expand to view the summary
Build stats
Test stats 🧪
Steps errorsExpand to view the steps failures
Log outputExpand to view the last 100 lines of log output
|
|
Failures are unrelated (generators fail + codecov upload failed after CI run did succeed). |
(cherry picked from commit 68fc3c4)
What does this PR do?
If a Value in a MapStr itself has the type MapStr but is nil, we will
have a panic in DeepUpdate. This change fixes the Update functionality
to handle typed nil values.
Why is it important?
Do not panic the Beat if we have an event with
nilvalues.Checklist
- [ ] I have commented my code, particularly in hard-to-understand areas- [ ] I have made corresponding changes to the documentation- [ ] I have made corresponding change to the default configuration filesCHANGELOG.next.asciidocorCHANGELOG-developer.next.asciidoc.Related issues