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

Enhance contains condition to work on fields that are arrays of string #2248

Merged
merged 1 commit into from Aug 12, 2016

Conversation

Projects
None yet
3 participants
@andrewkroh
Copy link
Member

commented Aug 11, 2016

Addresses #2237

@@ -269,26 +269,36 @@ func (c *Condition) checkEquals(event common.MapStr) bool {
}

func (c *Condition) checkContains(event common.MapStr) bool {

outer:

This comment has been minimized.

Copy link
@ruflin

ruflin Aug 12, 2016

Collaborator

argh :-(

This comment has been minimized.

Copy link
@andrewkroh

andrewkroh Aug 12, 2016

Author Member

Why no love for the label? Naming?

This comment has been minimized.

Copy link
@ruflin

ruflin Aug 12, 2016

Collaborator

It's not supported in my brain :-D

case []string:
for _, s := range value.([]string) {
if strings.Contains(s, equalValue) {
continue outer

This comment has been minimized.

Copy link
@ruflin

ruflin Aug 12, 2016

Collaborator

why not just return true here?

This comment has been minimized.

Copy link
@andrewkroh

andrewkroh Aug 12, 2016

Author Member

That would be changing the behavior of the method. This method returns true iff all the contains conditions are satisfied. Returning true would cause the method to not evaluate any remaining conditions.

When I was adding this, I initially did return true only to realize I broken a test.

This comment has been minimized.

Copy link
@ruflin

ruflin Aug 12, 2016

Collaborator

Got it. Other option would be to package the second for loop into a function/method so "standard" continue could be used. I just think it makes it somehow harder to read.

@ruflin

This comment has been minimized.

Copy link
Collaborator

commented Aug 12, 2016

LGTM

@ruflin ruflin merged commit c010bae into elastic:master Aug 12, 2016

4 checks passed

CLA Commit author has signed the CLA
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
default Build finished.
Details
if !strings.Contains(*value.(*string), equalValue) {
return false
}
case []string:

This comment has been minimized.

Copy link
@monicasarbu

monicasarbu Aug 16, 2016

Contributor

@andrewkroh why not ?

found := false
for _, s := range value.([]string) {
  if strings.Contains(s, equalValue) {
   found = true
  }
}
if !found {
  return false
}

This comment has been minimized.

Copy link
@andrewkroh

andrewkroh Aug 16, 2016

Author Member

They are equivalent if you add the required break statement to the snippet above. But the code in the PR is a bit more concise.

@monicasarbu

This comment has been minimized.

Copy link
Contributor

commented Aug 16, 2016

@andrewkroh does it work without updating setContains() function?

@monicasarbu

This comment has been minimized.

Copy link
Contributor

commented Aug 16, 2016

@andrewkroh can you please add documentation for it?

@andrewkroh

This comment has been minimized.

Copy link
Member Author

commented Aug 16, 2016

does it work without updating setContains() function?

Yes, setContains is for reading the configuration as I understand it. This PR does not make any changes to what is accepted as part of the configuration (it accepts a single string). The PR only enhances it to work properly when the field in the event is an array.

@andrewkroh

This comment has been minimized.

Copy link
Member Author

commented Aug 16, 2016

can you please add documentation for it?

I have updated to docs. See #2285

@monicasarbu monicasarbu deleted the andrewkroh:feature/contains-array branch Aug 22, 2016

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.