-
Notifications
You must be signed in to change notification settings - Fork 37
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
Add constant comparison operator to jq evaluation #4512
Add constant comparison operator to jq evaluation #4512
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes a lot of sense, and it's sort of amazing that it was missing for so long. Thanks for adding the test coverage, too!
internal/engine/eval/jq/jq.go
Outdated
if a.Profile == nil { | ||
return nil, fmt.Errorf("missing profile accessor") | ||
if a.Profile == nil && a.Constant == nil { | ||
return nil, fmt.Errorf("missing profile or constant accessor") | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you want Profile
and Constant
to be XOR (at least one set, but not both)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, I've updated it
internal/engine/eval/jq/jq.go
Outdated
@@ -88,9 +99,9 @@ func (jqe *Evaluator) Eval(ctx context.Context, pol map[string]any, res *engif.R | |||
} | |||
|
|||
// Deep compare | |||
if !reflect.DeepEqual(profileVal, dataVal) { | |||
if !reflect.DeepEqual(numberToFloat64(expectedVal), numberToFloat64(dataVal)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does this work for booleans (like your example)? I believe you that it does, but I was surprised to see that it worked.
Oh, I see! numberToFloat64
actually returns any
, and passes through other values. I'm trying to figure out if there's a better name -- maybe standardizeNumbers
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the name
internal/engine/eval/jq/jq_test.go
Outdated
Constant: &structpb.Value{ | ||
Kind: &structpb.Value_BoolValue{ | ||
BoolValue: false, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can write this as:
Constant: &structpb.Value{ | |
Kind: &structpb.Value_BoolValue{ | |
BoolValue: false, | |
}, | |
Constant: &structpb.NewBoolValue(false), |
internal/engine/eval/jq/jq_test.go
Outdated
Constant: &structpb.Value{ | ||
Kind: &structpb.Value_ListValue{ | ||
ListValue: &structpb.ListValue{ | ||
Values: []*structpb.Value{ | ||
{ | ||
Kind: &structpb.Value_StringValue{ | ||
StringValue: "a", | ||
}, | ||
}, | ||
{ | ||
Kind: &structpb.Value_StringValue{ | ||
StringValue: "b", | ||
}, | ||
}, | ||
{ | ||
Kind: &structpb.Value_StringValue{ | ||
StringValue: "c", | ||
}, | ||
}, | ||
}, | ||
}, | ||
}, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I love that we're testing list input. 🎉
Again, I think the helpers in structpb
will make this less painful to read and write:
Constant: &structpb.Value{ | |
Kind: &structpb.Value_ListValue{ | |
ListValue: &structpb.ListValue{ | |
Values: []*structpb.Value{ | |
{ | |
Kind: &structpb.Value_StringValue{ | |
StringValue: "a", | |
}, | |
}, | |
{ | |
Kind: &structpb.Value_StringValue{ | |
StringValue: "b", | |
}, | |
}, | |
{ | |
Kind: &structpb.Value_StringValue{ | |
StringValue: "c", | |
}, | |
}, | |
}, | |
}, | |
}, | |
}, | |
Constant: &structpb.NewListValue(structpb.ListValue{Values: { | |
structpb.NewStringValue("a"), | |
structpb.NewStringValue("b"), | |
structpb.NewStringValue("c"), | |
}, | |
}}), |
Unfortunately, NewList
returns a *ListValue, error
, so we can't just feed it into NewListValue
directly. (We could also do it outside the function, or write an errorToPanic[T](T, err) T
helper method.)
Summary
This adds a
constant
field to thejq
evaluation engine.This allows us to write ruletypes like the one below, which compares the evaluation result to a constant value, rather than requiring the profile to supply to value.
The example below compares the field
delete_branch_on_merge
in the evaluation result to the constanttrue
.This also changes the
jq
equality check to not differentiate between number type (int
,float64
etc).The alternative would be to let the user specify the type of their number constant, but I'm making the assumption that the likeliness of someone needing that is small, and not worth the additional overhead of setting a type every time.
Ref mindersec/minder-rules-and-profiles#156
I will update the docs once this is approved.
Change Type
Mark the type of change your PR introduces:
Testing
Outline how the changes were tested, including steps to reproduce and any relevant configurations.
Attach screenshots if helpful.
Review Checklist: