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

Fix #7998: ignore event fields that contains a nil interface of type encoding.TextMarshaler #7999

Merged
merged 3 commits into from
Aug 23, 2018

Conversation

cyrilleverrier
Copy link
Contributor

@cyrilleverrier cyrilleverrier commented Aug 17, 2018

Fix #7998: ignore event fields that contains a nil interface of type encoding.TextMarshaler

@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@kvch
Copy link
Contributor

kvch commented Aug 17, 2018

jenkins test this

@@ -158,6 +158,9 @@ func normalizeValue(value interface{}, keys ...string) (interface{}, []error) {

switch value.(type) {
case encoding.TextMarshaler:
if reflect.ValueOf(value).Kind() == reflect.Ptr && reflect.ValueOf(value).IsNil() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it make sense to do this check up on line 135 where we check for plain old nil?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Value may be either a nil interface of an interface storing a nil pointer :)

when running the libbeat TestNormalizeValue unit tests:

  1. the check for value == nil at line 135 protects against the test {nil, nil}
  2. the check for stored nil pointers that I added at line 161 protects against {nilTimePtr, nil}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, but should we do this check immediately upon entering normalizeValue so that we drop all interfaces storing nil pointers?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andrewkroh: I didn't want to add a run-time check for all fields in the event map.

The 2 cases where the interface{} can store a pointer to a nil value are encoding.TextMarshaler and followPointer. And followPointer already checks for nil pointer

If you prefer, I can move the test to the beginning of the function normalizeValue.
It could be safer if a new encoder case is added later.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't want to add a run-time check for all fields in the event map.

I suspected that might be a reason for not doing it at method entry.

@andrewkroh
Copy link
Member

jenkins test this

@andrewkroh andrewkroh merged commit 04d6e55 into elastic:master Aug 23, 2018
@cyrilleverrier cyrilleverrier deleted the issues/7998 branch August 23, 2018 04:03
ycombinator added a commit that referenced this pull request Dec 18, 2019
… type encoding.TextMarshaler (#7999) (#15175)

* Fix #7998: ignore event fields that contains a nil interface of type encoding.TextMarshaler (#7999)

Ignore event fields that contains a nil interface of type encoding.TextMarshaler.

Fixes #7998

* Removing assertion using uuid package
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants