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

Comparisons against missing map keys return an error unless preceded by an "in" check #35

Closed
schmichael opened this issue Apr 17, 2023 · 0 comments · Fixed by #38
Closed

Comments

@schmichael
Copy link
Member

schmichael commented Apr 17, 2023

hashicorp/nomad#16758 pointed out the following surprising bexpr behavior which can be reproduced with: https://go.dev/play/p/iHY_PWXljwW

To provide an inline example:

type Node struct {
  // ...other fields exist but are unrelated...

  Meta map[string]string
}

The filter "foo" in Meta and Meta["foo"] == "bar" works as expected: Node instances with a foo=bar entry are returned. Node instances without a foo key or a foo key equal to other values are dropped.

Surprisingly the seemingly equivalent filter Meta["foo"] == "bar" behaves differently: Node instances without a foo key will return an error. This breaks Filter.Execute(...) uses, and it's not really possible for the caller of Evaluator.Evaluate(...) to tell if the returned error is for a map key or a struct field.

Proposal

I think field lookups on maps should be given special treatment to avoid requiring "k" in m and ... prefixes anytime m varies in shape between objects.

I can think of 2 options if we want to fix this:

Option 1: Skip unknown map keys

When pointerstructure returns ErrNotFound, peek at the reflect.Kind of the parent path and if it exists and is a map: return false instead of an error.

The major downside of this approach is bexpr users who don't use dynamically shaped maps. If all your maps have the same keys, the existing behavior is fine. This seems like an unusual use case though.

Option 2: Option to skip unknown map keys

Same behavior as Option 1 but put behind an Option.

The major downside to this approach is that variation between implementations (eg Nomad and Consul) seem unnecessarily awkward. I can't imagine why anyone wouldn't want this behavior.

Non-option: UnknownValue

The UnknownValue("") option doesn't work for 2 reasons:

  1. UnknownValue ignores invalid struct fields in addition to the desirable skip-missing-map-keys behavior. These errors are desirable so users can tell the difference between a typo and no-matching-objects.
  2. UnknownValue lacks the ability to ensure a missing map key is skipped (returns false from evaluate(...)). Since UnknownValue always coalesces missing values to a valid value, it's impossible to different missing fields from a match. There are hacks around this like setting the unknown value to a UUID or a hash of the input expression, but you try getting that past coworkers in a review. 😅

I actually don't know when UnknownValue would be useful due to the above caveats. 🤷

schmichael added a commit that referenced this issue Apr 17, 2023
Fixes #35 (option 2)

This adds a SkipMissingMapKey option which treats comparisons against
missing maps keys as unequal.

For example with this option the expressions `Map["x"] == "y"` and
`Map["x"] != "something else"` will both match when `Map["x"] =
"y"`. If `Map["x"]` is unset then the `!=` operation will match since
a missing key does not match any values.
schmichael added a commit that referenced this issue Apr 21, 2023
Implements MatchOperator.NotPresentDisposition to determine an
operator's behavior when a map key in the selector is not found.

Fixes #35
Replaces #36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
1 participant