Skip to content
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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add SetValue method to libbeat common.MapStr #4838

Merged
merged 2 commits into from Aug 8, 2017

Conversation

Projects
None yet
3 participants
@simitt
Copy link
Contributor

commented Aug 7, 2017

SetValue dereferences given values for *int, *string and *boolean,
otherwise it just sets the value for the given key as long as the value
is not nil or empty.

Add SetValue method to libbeat common.MapStr
SetValue dereferences given values for *int, *string and *boolean,
otherwise it just sets the value for the given key as long as the value
is not nil or empty.

@simitt simitt requested a review from ruflin Aug 7, 2017

@simitt simitt added the review label Aug 7, 2017

@ruflin ruflin added the libbeat label Aug 7, 2017

@@ -138,6 +138,15 @@ func (m MapStr) Put(key string, value interface{}) (interface{}, error) {
return walkMap(key, m, mapStrOperation{putOperation{value}, true})
}

// Dereferences a value if it is a pointer. Sets the dereferenced value if not nil.

This comment has been minimized.

Copy link
@ruflin

ruflin Aug 7, 2017

Collaborator

Comments in Golang must always start with the method name.

Value interface{}
}

func (op setOperation) Do(key string, data MapStr) (interface{}, error) {

This comment has been minimized.

Copy link
@ruflin

ruflin Aug 7, 2017

Collaborator

Should we make this private?

This comment has been minimized.

Copy link
@simitt

simitt Aug 7, 2017

Author Contributor

Then it would break with the mapStrOperator interface which is used by all other methods. I just aligned using this interface to make it consistent.

@ruflin

This comment has been minimized.

Copy link
Collaborator

commented Aug 8, 2017

jenkins, retest it

@ruflin

ruflin approved these changes Aug 8, 2017

@ruflin ruflin merged commit fa77316 into elastic:master Aug 8, 2017

5 of 6 checks passed

beats-ci Build started sha1 is merged.
Details
CLA Commit author is a member of Elasticsearch
Details
codecov/patch 100% of diff hit (target 61.99%)
Details
codecov/project 62.38% (+0.39%) compared to 23d9fe6
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ruflin

This comment has been minimized.

Copy link
Collaborator

commented Aug 8, 2017

Jenkins is somehow hanging with auditbeat, but this is unrelated to the PR. Merging.

@@ -138,6 +138,15 @@ func (m MapStr) Put(key string, value interface{}) (interface{}, error) {
return walkMap(key, m, mapStrOperation{putOperation{value}, true})
}

// SetValue dereferences a value if it is a pointer. Sets the dereferenced value if not nil.

This comment has been minimized.

Copy link
@andrewkroh

andrewkroh Aug 8, 2017

Member

This operation seems like something that should be done during event normalization. In my mind this doesn't fit well with the generic nature of MapStr, and the API becomes confusing because now we have both Put and SetValue.

During event normalization some of these things are done automatically. nil values are removed. pointers are dereferenced (for all types). I don't believe empty lists are removed (but we could add that).

@simitt @ruflin Could we remove this method and improve ConvertToGenericEvent to handle this normalization?

This comment has been minimized.

Copy link
@ruflin

ruflin Aug 8, 2017

Collaborator

@andrewkroh I like the suggestion a lot. TBH I was not really aware / forgot that we have this. This removes the hassle of having to think about which method to use when creating the event.

What happens in the case that Put("a.b.c", nil) is used and the event is then normalised. Will the full tree be removed?

@simitt In case you agree with the change, do you want to work on this?

This comment has been minimized.

Copy link
@andrewkroh

andrewkroh Aug 8, 2017

Member

What happens in the case that Put("a.b.c", nil) is used

func TestEmptyMap(t *testing.T) {
	m := MapStr{}
	m.Put("a.b.c", nil)
	t.Log(ConvertToGenericEvent(m).StringToPrint())
}
{
  "a": {
    "b": {}
  }
}

So we need some additional checks in the normalization code to make the whole tree disappear.

This comment has been minimized.

Copy link
@simitt

simitt Aug 9, 2017

Author Contributor

Of course, I was not aware of having this normalization method in an event. Thank you for pointing this out, it will save ourselves some trouble.
I will have a closer look and create a PR to remove empty lists then.
@andrewkroh , @ruflin could one of you revert the commit in the beats project, I don't want to mess with your master.

This comment has been minimized.

Copy link
@ruflin

ruflin Aug 9, 2017

Collaborator

@simitt No worries about reverting the commit. Just remove the code again in the follow up commit. I think so far we never reverted a commit, we only move forward :-D

This comment has been minimized.

Copy link
@simitt

simitt Aug 9, 2017

Author Contributor

just thought it would be cleaner to revert, but fine with me this way.


switch op.Value.(type) {
case *bool:
if newVal := op.Value.(*bool); newVal != nil {

This comment has been minimized.

Copy link
@andrewkroh

andrewkroh Aug 8, 2017

Member

These secondary type assertions are not necessary if the switch statement is modified to be switch v := op.Value.(type) (see TypeSwitchGuard). Within each case statement v takes on the type declared in the case.

This comment has been minimized.

Copy link
@simitt

simitt Aug 9, 2017

Author Contributor

Nice to know, will have a closer look then..

if newVal := op.Value.(*string); newVal != nil {
data[key] = *newVal
}
case MapStr:

This comment has been minimized.

Copy link
@andrewkroh

andrewkroh Aug 8, 2017

Member

These empty checks can be added to the appropriate normalizeX function in libbeat/common/event.go.

simitt added a commit to simitt/beats that referenced this pull request Aug 10, 2017

andrewkroh added a commit that referenced this pull request Aug 10, 2017

ramon-garcia added a commit to ramon-garcia/beats that referenced this pull request Dec 5, 2017

Add SetValue method to libbeat common.MapStr (elastic#4838)
SetValue dereferences given values for *int, *string and *boolean,
otherwise it just sets the value for the given key as long as the value
is not nil or empty.

ramon-garcia added a commit to ramon-garcia/beats that referenced this pull request Dec 5, 2017

athom added a commit to athom/beats that referenced this pull request Jan 25, 2018

Add SetValue method to libbeat common.MapStr (elastic#4838)
SetValue dereferences given values for *int, *string and *boolean,
otherwise it just sets the value for the given key as long as the value
is not nil or empty.

athom added a commit to athom/beats that referenced this pull request Jan 25, 2018

@simitt simitt deleted the simitt:feature/commonMapstrSet branch Apr 12, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.