builtin: Return evalerrors.ErrEvaluationSkipSilently in case the builtin evaluator doesn't match the entity

internal/engine/ingester/builtin/builtin.go

@@ -21,13 +21,13 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
-	"log"
 	"reflect"
 	"strings"
 
 	"github.com/go-git/go-billy/v5"
 	"google.golang.org/protobuf/reflect/protoreflect"
 
+	evalerrors "github.com/stacklok/mediator/internal/engine/errors"
 	engif "github.com/stacklok/mediator/internal/engine/interfaces"
 	"github.com/stacklok/mediator/internal/util"
 	pb "github.com/stacklok/mediator/pkg/generated/protobuf/go/mediator/v1"
@@ -42,6 +42,7 @@ const (
 // BuiltinRuleDataIngest is the engine for a rule type that uses builtin methods
 type BuiltinRuleDataIngest struct {
 	builtinCfg  *pb.BuiltinType
+	ruleMethods rule_methods.Methods
 	method      string
 	accessToken string
 }
@@ -55,6 +56,7 @@ func NewBuiltinRuleDataIngest(
 		builtinCfg:  builtinCfg,
 		accessToken: access_token,
 		method:      builtinCfg.GetMethod(),
+		ruleMethods: &rule_methods.RuleMethods{},
 	}, nil
 }
 
@@ -66,10 +68,10 @@ func (*BuiltinRuleDataIngest) FileContext() billy.Filesystem {
 
 // Ingest calls the builtin method and populates the data to be returned
 func (idi *BuiltinRuleDataIngest) Ingest(ctx context.Context, ent protoreflect.ProtoMessage, params map[string]any) (*engif.Result, error) {
-	// call internal method stored in pkg and method
-	rm := rule_methods.RuleMethods{}
-	value := reflect.ValueOf(rm)
-	method := value.MethodByName(idi.method)
+	method, err := idi.ruleMethods.GetMethod(idi.method)
+	if err != nil {
+		return nil, fmt.Errorf("cannot get method: %w", err)
+	}
 
 	// Check if the method exists
 	if !method.IsValid() {
@@ -79,12 +81,8 @@ func (idi *BuiltinRuleDataIngest) Ingest(ctx context.Context, ent protoreflect.P
 	matches, err := entityMatchesParams(ctx, ent, params)
 	if err != nil {
 		return nil, fmt.Errorf("cannot check if entity matches params: %w", err)
-	}
-
-	// TODO: this should be a warning
-	if !matches {
-		log.Printf("entity not matching parameters, skipping")
-		return nil, nil
+	} else if !matches {
+		return nil, evalerrors.ErrEvaluationSkipSilently
 	}
 
 	// call method

internal/engine/ingester/builtin/builtin_test.go

@@ -0,0 +1,122 @@
+// Copyright 2023 Stacklok, Inc.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//	http://www.apache.role/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+// Package rule provides the CLI subcommand for managing rules
+
+// Package builtin provides the builtin ingestion engine
+// this test is directly in the builtin package because it is testing the internals of the ingestor and setting
+// the rule methods to a fake
+package builtin
+
+import (
+	"context"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"google.golang.org/protobuf/reflect/protoreflect"
+
+	evalerrors "github.com/stacklok/mediator/internal/engine/errors"
+	pb "github.com/stacklok/mediator/pkg/generated/protobuf/go/mediator/v1"
+)
+
+func TestBuiltInWorks(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		name       string
+		methodName string
+		ent        protoreflect.ProtoMessage
+		params     map[string]any
+		ingested   map[string]any
+	}{
+		{
+			name:       "passthrough works",
+			methodName: "Passthrough",
+			ent: &pb.Artifact{
+				Name: "test",
+			},
+			ingested: map[string]any{
+				"name": "test",
+			},
+			params: map[string]any{
+				"name": "test",
+			},
+		},
+	}
+
+	for _, tt := range tests {
+		tt := tt
+
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+
+			bi, err := NewBuiltinRuleDataIngest(nil, "")
+			assert.NoError(t, err)
+			bi.method = tt.methodName
+
+			res, err := bi.Ingest(context.Background(), tt.ent, tt.params)
+			assert.NoError(t, err)
+			assert.Equal(t, tt.ingested, res.Object)
+		})
+	}
+}
+
+func TestBuiltinErrorCases(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		name       string
+		methodName string
+		expErr     error
+		ent        protoreflect.ProtoMessage
+		params     map[string]any
+	}{
+		{
+			name:       "entity doesn't match",
+			methodName: "Passthrough",
+			expErr:     evalerrors.ErrEvaluationSkipSilently,
+			ent:        &pb.Artifact{},
+			params: map[string]any{
+				"foo": "bar",
+			},
+		},
+		{
+			name:       "method doesn't match",
+			methodName: "nosuchmethod",
+			expErr:     nil, // there's no specific error for this
+			ent:        &pb.Artifact{},
+			params: map[string]any{
+				"foo": "bar",
+			},
+		},
+	}
+
+	for _, tt := range tests {
+		tt := tt
+
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+
+			bi, err := NewBuiltinRuleDataIngest(nil, "")
+			assert.NoError(t, err)
+			bi.method = tt.methodName
+
+			res, err := bi.Ingest(context.Background(), tt.ent, tt.params)
+			assert.Error(t, err, "expected error")
+			assert.Nil(t, res)
+			if tt.expErr != nil {
+				assert.Equal(t, tt.expErr, err)
+			}
+		})
+	}
+}

pkg/rule_methods/rule_methods.go

@@ -15,5 +15,38 @@
 // Package rule_methods provides the methods that are used by the rules
 package rule_methods
 
+import (
+	"context"
+	"encoding/json"
+	"fmt"
+	"reflect"
+
+	"google.golang.org/protobuf/encoding/protojson"
+	"google.golang.org/protobuf/reflect/protoreflect"
+)
+
+// Methods is the interface that is used to get the method by name
+type Methods interface {
+	GetMethod(string) (reflect.Value, error)
+}
+
 // RuleMethods is the struct that contains the methods that are used by the rules
 type RuleMethods struct{}
+
+// GetMethod gets the method by name from the RuleMethods struct
+func (r *RuleMethods) GetMethod(mName string) (reflect.Value, error) {
+	value := reflect.ValueOf(r)
+	method := value.MethodByName(mName)
+
+	// Check if the method exists
+	if !method.IsValid() {
+		return reflect.Value{}, fmt.Errorf("rule method not found")
+	}
+
+	return method, nil
+}
+
+// Passthrough is a method that passes the entity through, just marshalling it
+func (_ *RuleMethods) Passthrough(_ context.Context, _ string, ent protoreflect.ProtoMessage) (json.RawMessage, error) {
+	return protojson.Marshal(ent)
+}