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

Support "IN" operator in govaluate statements #922

Closed
palourde opened this issue Jan 24, 2018 · 7 comments
Closed

Support "IN" operator in govaluate statements #922

palourde opened this issue Jan 24, 2018 · 7 comments
Assignees

Comments

@palourde
Copy link
Contributor

At this moment, the IN operator can't be used in govaluate.

For example, if I use the statement "foo" in entity.Subscriptions, govaluate will return:

failed to evaluate statement: "foo" in entity.Subscriptions error="Value '[bar foo]' cannot be used with the comparator 'in', it is not a number"

I believe it's because the value would need to be in the form of ("foo", "bar") instead of [foo bar] so we will probably need to create a custom function that users can call to support that format.

@palourde palourde added the bug label Jan 24, 2018
@grepory
Copy link
Contributor

grepory commented Jan 24, 2018

That seems... Strange. This feels more like a govaluate bug. Can we make sure that govaluate just doesn’t support arrays?

@palourde
Copy link
Contributor Author

That's definitively a bug in govaluate IMO (see Knetic/govaluate#79) but considering that we forked that package and that Eric's PR is still not merged, I would be tempted to say we need to either fix that with a custom function or by modifying govaluate itself to support that.

@grepory
Copy link
Contributor

grepory commented Jan 24, 2018

We could just keep maintaining our fork and merge upstream changes into it over time.

@palourde
Copy link
Contributor Author

palourde commented Mar 6, 2018

Additionally, I just realized we can't use a comma in the filter statements, in the current version of sensuctl, since we use them to separate statements, e.g.:

? Statements (comma separated list): weekday(timestamp) in (1, 2, 3, 4, 5)
Error: invalid statement 'weekday(timestamp) in (1': Unbalanced parenthesis

@grepory
Copy link
Contributor

grepory commented Mar 18, 2018

Can we change the way we input filters to not use commas to separate statements?

@jamesdphillips
Copy link
Contributor

Didn't find this issue when I previously searched and created a similar ticket. 😬

Copying over my investigation:

When using the IN operator the right side (RHS) of the expression must be an instance of []interface. This, for instance, makes it frustrating when working the Subscriptions field ([]string) on the Check and Entity types.

Expected Behavior

I would expect to be able write the expression 'unix' IN entity.Subscriptions, however since the RHS of the expression is a slice of strings, this will always evaluate to false.

Possible Solution

A few possible avenues.

a) Expose a function

  • We could expose a function (eg. sliceContains) that allows the user to check if an identifier has a specified value.
  • In this case, to avoid any confusion. it may make sense to simply remove IN.
// Example expression
entity.LastSeen > 12345678 && sliceContains(entity.Subscriptions, 'unix')

Drawbacks

  • Lots of type assertions, I guess
  • Functions require extra documentation
  • Without any sort of module/packages/namespaces, we run the risk of identifiers colliding

b) Add to types package

I believe, we potentially could add a type with some helpful methods familiar to those who write Python and Ruby.

type MyStringSlice []string

func (MyStringSlice) Count() int {
  // ...
}

func (MyStringSlice) Contains(interface{}) {
  // ...
}

type Entity struct {
  // ...
  Subscriptions MyStringSlice
  // ...
}
// example expression
entity.LastSeen > 12345678 && entity.Subscriptions.Contains('unix')

Drawbacks

  • Is protobuf going to make this needlessly painful?
  • Accessors feature makes heavy use of reflection
  • Methods require extra documentation

c) Expand govaluate

Update govaluate so that IN operator can handle greater range of expressions.

I figure this could partially be optimized at the planning stage;

  • if the RHS of the expression is an array literal we know the RHS should be cast to []interface
  • if the LHS of the expression is a string, integer, float literal, etc. we know that the RHS should be cast to []string, []int, []float, etc.
  • becomes more tricky when the type of the LHS isn't as easy to infer; eg. (1 + 3 / 6.0) IN myIdentifier
  • (in the planner) becomes nearly impossible when the LHS is an identifier. myIdentifier IN otherIdentifer
// the follow example expression becomes viable
entity.LastSeen > 12345678 && 'unix' IN entity.Subscriptions

Drawbacks

  • Planning and in some cases evaluation likely becomes a bit slower.
  • Potentially tricky edge cases

Context

  • Initially ran into this while trying to filter entities by subscriptions for the web UI.
  • For what it's worth this behavior is documented in govaluate's MANUAL.
  • Blocks [Web] Entities page: filter #839

... I don't have a particularly strong opinion with regards to what is the best solution and open to any thoughts.

@echlebek
Copy link
Contributor

I made a release of github.com/sensu/govaluate that fixes this issue.

echlebek added a commit that referenced this issue May 31, 2018
Closes #922

Signed-off-by: Eric Chlebek <eric@sensu.io>
echlebek added a commit that referenced this issue May 31, 2018
Closes #922

Signed-off-by: Eric Chlebek <eric@sensu.io>
echlebek added a commit that referenced this issue May 31, 2018
Closes #922

Signed-off-by: Eric Chlebek <eric@sensu.io>
nikkictl pushed a commit that referenced this issue Jun 4, 2018
commit 9747e04
Merge: f6fc96d 7194168
Author: Nikki Attea <nikki@sensu.io>
Date:   Mon Jun 4 07:33:37 2018 -0700

    Merge branch 'testing/metric-extraction' of https://github.com/sensu/sensu-go into testing/metric-extraction

    Signed-off-by: Nikki Attea <nikki@sensu.io>

commit f6fc96d
Author: Nikki Attea <nikki@sensu.io>
Date:   Fri Jun 1 10:50:46 2018 -0500

    Remove metric extraction e2e test in favor of a more detailed integration test

    Signed-off-by: Nikki Attea <nikki@sensu.io>

commit d6d1286
Author: Terrance Kennedy <terrancerkennedy@gmail.com>
Date:   Fri Jun 1 14:46:27 2018 -0600

    Travis: unshallow git history to detect version

    Signed-off-by: Terrance Kennedy <terrancerkennedy@gmail.com>

commit ca7a4b8
Author: Neal Granger <neal@nealg.com>
Date:   Fri Jun 1 12:54:58 2018 -0700

    Display a modal when authentication fails (#1627)

    In the event that automatic session refresh fails, show the user a message instead of redirecting directly to the sign in page.

    Signed-off-by: Neal Granger <neal@nealg.com>

commit ca88abe
Author: Terrance Kennedy <terrancerkennedy@gmail.com>
Date:   Fri Jun 1 13:52:42 2018 -0600

    Remove tag info from deploy

    Signed-off-by: Terrance Kennedy <terrancerkennedy@gmail.com>

commit 04be123
Author: Terrance Kennedy <terrance@sensu.io>
Date:   Fri Jun 1 15:21:26 2018 -0400

    Pass SENSU_BUILD_ITERATION to packaging container (#1641)

    Signed-off-by: Terrance Kennedy <terrancerkennedy@gmail.com>

    ## What is this change?

    Passes the `SENSU_BUILD_ITERATION` environment variable into the sensu-go-build docker container when deploying packages.

    ## Why is this change necessary?

    Sensu nightly builds were broken on Travis because the version command could not determine the build iteration.

    ## Does your change need a Changelog entry?

    Not this time.

    ## Do you need clarification on anything?

    I'm good.

    ## Were there any complications while making this change?

    I noticed that the package_cloud gem was being installed on the host, which is unnecessary because it's only installed/used inside the sensu-go-build docker container. Builds should be a few seconds faster now too.

commit be0a32d
Author: Eric Chlebek <echlebek@gmail.com>
Date:   Fri Jun 1 11:36:26 2018 -0700

    Comment out failing tests. (#1639)

    These tests are failing on some environments, and we don't know
    why yet.

    Signed-off-by: Eric Chlebek <eric@sensu.io>

commit a59a476
Author: Nikki Attea <nikki@sensu.io>
Date:   Fri Jun 1 10:56:16 2018 -0500

    Remove omitempty from boolean fields (#1632)

    Signed-off-by: Nikki Attea <nikki@sensu.io>

commit 7194168
Author: Nikki Attea <nikki@sensu.io>
Date:   Fri Jun 1 10:50:46 2018 -0500

    Remove metric extraction e2e test in favor of a more detailed integration test

    Signed-off-by: Nikki Attea <nikki@sensu.io>

commit 318d489
Author: Eric Chlebek <echlebek@gmail.com>
Date:   Thu May 31 11:19:40 2018 -0700

    Upgrade govaluate to release 3.1.0. (#1629)

    Closes #922

    Signed-off-by: Eric Chlebek <eric@sensu.io>

Signed-off-by: Nikki Attea <nikki@sensu.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants