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.

Sign up for GitHub

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

Jump to bottom

Add constant comparison operator to jq evaluation #4512

Merged
merged 3 commits into from
Sep 18, 2024
Merged

Add constant comparison operator to jq evaluation #4512

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
be38dd9
Add constant comparison operator to jq evaluation
eleftherias Sep 17, 2024
d4d7312
Address review feedback
eleftherias Sep 18, 2024
1c77734
Merge branch 'main' into jq-rule-with-constant
eleftherias Sep 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions docs/docs/ref/proto.md
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

51 changes: 40 additions & 11 deletions internal/engine/eval/jq/jq.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,11 +42,11 @@ func NewJQEvaluator(assertions []*pb.RuleType_Definition_Eval_JQComparison) (*Ev

for idx := range assertions {
a := assertions[idx]
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")
}
Copy link
Member

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)?

Copy link
Contributor Author

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


if a.Profile.Def == "" {
if a.Profile != nil && a.Profile.Def == "" {
return nil, fmt.Errorf("missing profile accessor definition")
}

Expand All @@ -72,14 +72,25 @@ func (jqe *Evaluator) Eval(ctx context.Context, pol map[string]any, res *engif.R
obj := res.Object

for idx := range jqe.assertions {
var profileVal, dataVal any
var expectedVal, dataVal any
var err error

a := jqe.assertions[idx]
profileVal, err := util.JQReadFrom[any](ctx, a.Profile.Def, pol)
// we ignore util.ErrNoValueFound because we want to allow the JQ accessor to return the default value
// which is fine for DeepEqual
if err != nil && !errors.Is(err, util.ErrNoValueFound) {
return fmt.Errorf("cannot get values from profile accessor: %w", err)

// If there is no profile accessor, get the expected value from the constant accessor
if a.Profile == nil {
expectedVal, err = util.JQReadConstant[any](a.Constant.AsInterface())
if err != nil {
return fmt.Errorf("cannot get values from profile accessor: %w", err)
}
} else {
// Get the expected value from the profile accessor
expectedVal, err = util.JQReadFrom[any](ctx, a.Profile.Def, pol)
// we ignore util.ErrNoValueFound because we want to allow the JQ accessor to return the default value
// which is fine for DeepEqual
if err != nil && !errors.Is(err, util.ErrNoValueFound) {
return fmt.Errorf("cannot get values from profile accessor: %w", err)
}
}

dataVal, err = util.JQReadFrom[any](ctx, a.Ingested.Def, obj)
Expand All @@ -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)) {
Copy link
Member

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the name

msg := fmt.Sprintf("data does not match profile: for assertion %d, got %v, want %v",
idx, dataVal, profileVal)
idx, dataVal, expectedVal)

marshalledAssertion, err := json.MarshalIndent(a, "", " ")
if err == nil {
Expand All @@ -103,3 +114,21 @@ func (jqe *Evaluator) Eval(ctx context.Context, pol map[string]any, res *engif.R

return nil
}

// Convert numeric types to float64
func numberToFloat64(v any) any {
switch v := v.(type) {
case int:
return float64(v)
case int32:
return float64(v)
case int64:
return float64(v)
case float32:
return float64(v)
case float64:
return v
default:
return v
}
}
185 changes: 181 additions & 4 deletions internal/engine/eval/jq/jq_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"testing"

"github.com/stretchr/testify/assert"
"google.golang.org/protobuf/types/known/structpb"

evalerrors "github.com/stacklok/minder/internal/engine/errors"
"github.com/stacklok/minder/internal/engine/eval/jq"
Expand Down Expand Up @@ -65,8 +66,10 @@ func TestNewJQEvaluatorValid(t *testing.T) {
},
},
{
Profile: &pb.RuleType_Definition_Eval_JQComparison_Operator{
Def: ".b",
Constant: &structpb.Value{
Kind: &structpb.Value_StringValue{
StringValue: "b",
},
},
Ingested: &pb.RuleType_Definition_Eval_JQComparison_Operator{
Def: ".b",
Expand Down Expand Up @@ -111,11 +114,12 @@ func TestNewJQEvaluatorInvalid(t *testing.T) {
},
},
{
name: "invalid nil profile accessor",
name: "invalid nil profile and constant accessor",
args: args{
assertions: []*pb.RuleType_Definition_Eval_JQComparison{
{
Profile: nil,
Profile: nil,
Constant: nil,
Ingested: &pb.RuleType_Definition_Eval_JQComparison_Operator{
Def: ".",
},
Expand Down Expand Up @@ -229,6 +233,27 @@ func TestValidJQEvals(t *testing.T) {
},
},
},
{
name: "valid single rule evaluates constant string",
assertions: []*pb.RuleType_Definition_Eval_JQComparison{
{
Constant: &structpb.Value{
Kind: &structpb.Value_StringValue{
StringValue: "simple",
},
},
Ingested: &pb.RuleType_Definition_Eval_JQComparison_Operator{
Def: ".simple",
},
},
},
args: args{
pol: map[string]any{},
obj: map[string]any{
"simple": "simple",
},
},
},
{
name: "valid single rule evaluates int",
assertions: []*pb.RuleType_Definition_Eval_JQComparison{
Expand All @@ -250,6 +275,27 @@ func TestValidJQEvals(t *testing.T) {
},
},
},
{
name: "valid single rule evaluates constant int",
assertions: []*pb.RuleType_Definition_Eval_JQComparison{
{
Constant: &structpb.Value{
Kind: &structpb.Value_NumberValue{
NumberValue: 1,
},
},
Ingested: &pb.RuleType_Definition_Eval_JQComparison_Operator{
Def: ".simple",
},
},
},
args: args{
pol: map[string]any{},
obj: map[string]any{
"simple": 1,
},
},
},
{
name: "valid single rule evaluates bool",
assertions: []*pb.RuleType_Definition_Eval_JQComparison{
Expand All @@ -271,6 +317,27 @@ func TestValidJQEvals(t *testing.T) {
},
},
},
{
name: "valid single rule evaluates constant bool",
assertions: []*pb.RuleType_Definition_Eval_JQComparison{
{
Constant: &structpb.Value{
Kind: &structpb.Value_BoolValue{
BoolValue: false,
},
Copy link
Member

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:

Suggested change
Constant: &structpb.Value{
Kind: &structpb.Value_BoolValue{
BoolValue: false,
},
Constant: &structpb.NewBoolValue(false),

},
Ingested: &pb.RuleType_Definition_Eval_JQComparison_Operator{
Def: ".simple",
},
},
},
args: args{
pol: map[string]any{},
obj: map[string]any{
"simple": false,
},
},
},
{
name: "valid single rule evaluates array",
assertions: []*pb.RuleType_Definition_Eval_JQComparison{
Expand All @@ -292,6 +359,45 @@ func TestValidJQEvals(t *testing.T) {
},
},
},
{
name: "valid single rule evaluates constant array",
assertions: []*pb.RuleType_Definition_Eval_JQComparison{
{
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",
},
},
},
},
},
},
Copy link
Member

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:

Suggested change
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.)

Ingested: &pb.RuleType_Definition_Eval_JQComparison_Operator{
Def: ".simple",
},
},
},
args: args{
pol: map[string]any{},
obj: map[string]any{
"simple": []any{"a", "b", "c"},
},
},
},
}
for _, tt := range tests {
tt := tt
Expand Down Expand Up @@ -447,6 +553,77 @@ func TestValidJQEvalsFailed(t *testing.T) {
},
},
},
{
name: "constant value doesn't match",
assertions: []*pb.RuleType_Definition_Eval_JQComparison{
{
Constant: &structpb.Value{
Kind: &structpb.Value_BoolValue{
BoolValue: true,
},
},
Ingested: &pb.RuleType_Definition_Eval_JQComparison_Operator{
Def: ".simple",
},
},
},
args: args{
pol: map[string]any{},
obj: map[string]any{
"simple": false,
},
},
},
{
name: "constant type doesn't match",
assertions: []*pb.RuleType_Definition_Eval_JQComparison{
{
Constant: &structpb.Value{
Kind: &structpb.Value_BoolValue{
BoolValue: false,
},
},
Ingested: &pb.RuleType_Definition_Eval_JQComparison_Operator{
Def: ".simple",
},
},
},
args: args{
pol: map[string]any{},
obj: map[string]any{
"simple": 123,
},
},
},
{
name: "constant array doesn't match",
assertions: []*pb.RuleType_Definition_Eval_JQComparison{
{
Constant: &structpb.Value{
Kind: &structpb.Value_ListValue{
ListValue: &structpb.ListValue{
Values: []*structpb.Value{
{
Kind: &structpb.Value_StringValue{
StringValue: "a",
},
},
},
},
},
},
Ingested: &pb.RuleType_Definition_Eval_JQComparison_Operator{
Def: ".simple",
},
},
},
args: args{
pol: map[string]any{},
obj: map[string]any{
"simple": []any{"a", "b", "d"},
},
},
},
}
for _, tt := range tests {
tt := tt
Expand Down
10 changes: 10 additions & 0 deletions internal/util/jq.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,3 +90,13 @@ func JQReadFrom[T any](ctx context.Context, path string, obj any) (T, error) {

return out, nil
}

// JQReadConstant gets the typed value from the given constant. Returns an error when the type assertion fails.
func JQReadConstant[T any](constant any) (T, error) {
out, ok := constant.(T)
if !ok {
return out, fmt.Errorf("could not type assert %v to %v", constant, reflect.TypeOf(out))
}

return out, nil
}
3 changes: 3 additions & 0 deletions pkg/api/openapi/minder/v1/minder.swagger.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading