add option to skip missing map keys #36
Conversation
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
requested review from
a team,
shore and
alvin-huang
and removed request for
a team
| // For comparison results with missing keys: missing means not equal. | ||
| if evaluateSkipMissing(ptr, datum) { | ||
| switch expression.Operator { | ||
| case grammar.MatchEqual: |
There was a problem hiding this comment.
I think we should probably handle the other operators too:
- MatchIn - false
- MatchNotIn - true
- MatchEmpty - true (sort of like how len(slice) == 0 works for a nil slice and an allocated slice that is currently empty)
- MatchNotEmpty - false
- MatchMatches - false
- MatchNotMatches - true
I would also recommend adding a func to grammar/ast.go (possibly called NotPresentMatchDisposition) to do the big switch statement over all operators and return the bool, error that you are returning in these cases. That way the big switch is colocated with other functions with similar needs and next to the const definitions so that when we add more operators in the future we don't accidentally miss adding them to this block of code.
There was a problem hiding this comment.
Any opinion on using an Option vs hardcoding the behavior? I think all HashiCorp products should have matching behavior, so I have a preference for hardcoding (or dropping this entirely and documenting the "x" in m approach if we can't get consensus).
|
I don't love the idea of hardcoding this change; the library coalesces to strings and generally treats unset as an empty string so the current behavior is IMHO correct. I'd be more into the idea of a way to check existence of a key so that you can be explicit -- if X is not set, or X is set but != Y, then... |
|
I think I may have gotten turned around a bit by the description. I thought given the option name that you were suggesting that the hardcoded behavior should be that it does not treat missing keys as unequal. I think you're suggesting that the behavior should be to treat missing keys as unequal instead of erroring...I'm into that. (Naming is hard, but something like "WithSkipErrorOnMissingKeys" might be a bit more descriptive.) I took a look at the linked issue and it says that "The major downside of [the hardcoded] approach is bexpr users" and stops there. Don't leave us hanging :-) Also I think that the comparisons to WithUnknownValue are misleading. Although in some cases you could use them interchangeably one is used to set a default value, which is different from how comparisons are treated, and I think the commenting on the new option that talks about how the two differ creates some confusion instead of resolves it. But if this becomes hardcoded then it won't be an issue anyways. You'd just have to ensure that using the unknown value doesn't stop working for those that want a default. |
|
I went for a clean start to implement the suggestions: #38 |
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"andMap["x"] != "something else"will both match whenMap["x"] = "y". IfMap["x"]is unset then the!=operation will match since a missing key does not match any values.I'll try to get reviews from someone on hashicorp/nomad, hashicorp/consul, and hashicorp/boundary to make sure we're all aligned.
As noted in the parent issue my preference is for hardcoding this change and dropping the Option. This PR implements the Option path to demonstrate the most complicated option. Hardcoding it means removing code and collapsing tests.