Skip to content

Commit

Permalink
small refactor: Move files around for testability
Browse files Browse the repository at this point in the history
This moves the evaluation and ingester engines to their own packages.

This makes these packages more self-contained and easier to test.
  • Loading branch information
JAORMX committed Aug 28, 2023
1 parent 5d93f85 commit 9868bc1
Show file tree
Hide file tree
Showing 10 changed files with 273 additions and 31 deletions.
30 changes: 30 additions & 0 deletions internal/engine/eval/errors/errors.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
// 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 jq

Check warning on line 16 in internal/engine/eval/errors/errors.go

View workflow job for this annotation

GitHub Actions / golangci-lint / Go Lint

package-comments: should have a package comment (revive)

import (
"errors"
"fmt"
)

// ErrEvaluationFailed is an error that occurs during evaluation of a rule.
var ErrEvaluationFailed = errors.New("evaluation error")

// NewErrEvaluationFailed creates a new evaluation error
func NewErrEvaluationFailed(sfmt string, args ...any) error {
msg := fmt.Sprintf(sfmt, args...)
return fmt.Errorf("%w: %s", ErrEvaluationFailed, msg)
}
13 changes: 2 additions & 11 deletions internal/engine/eval/eval.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,9 @@ package eval

import (
"context"
"errors"
"fmt"

"github.com/stacklok/mediator/internal/engine/eval/jq"
pb "github.com/stacklok/mediator/pkg/generated/protobuf/go/mediator/v1"
)

Expand All @@ -30,15 +30,6 @@ type Evaluator interface {
Eval(ctx context.Context, policy map[string]any, obj any) error
}

// ErrEvaluationFailed is an error that occurs during evaluation of a rule.
var ErrEvaluationFailed = errors.New("evaluation error")

// NewErrEvaluationFailed creates a new evaluation error
func NewErrEvaluationFailed(sfmt string, args ...any) error {
msg := fmt.Sprintf(sfmt, args...)
return fmt.Errorf("%w: %s", ErrEvaluationFailed, msg)
}

// NewRuleEvaluator creates a new rule data evaluator
func NewRuleEvaluator(rt *pb.RuleType) (Evaluator, error) {
ing := rt.Def.GetEval()
Expand All @@ -53,7 +44,7 @@ func NewRuleEvaluator(rt *pb.RuleType) (Evaluator, error) {
return nil, fmt.Errorf("rule type engine missing rest configuration")
}

return NewJQEvaluator(ing.GetJq())
return jq.NewJQEvaluator(ing.GetJq())
default:
return nil, fmt.Errorf("unsupported rule type engine: %s", rt.Def.Eval.Type)
}
Expand Down
5 changes: 3 additions & 2 deletions internal/engine/eval/jq.go → internal/engine/eval/jq/jq.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,15 @@
// limitations under the License.
// Package rule provides the CLI subcommand for managing rules

package eval
package jq

Check warning on line 16 in internal/engine/eval/jq/jq.go

View workflow job for this annotation

GitHub Actions / golangci-lint / Go Lint

package-comments: should have a package comment (revive)

import (
"context"
"encoding/json"
"fmt"
"reflect"

evalerrors "github.com/stacklok/mediator/internal/engine/eval/errors"
"github.com/stacklok/mediator/internal/util"
pb "github.com/stacklok/mediator/pkg/generated/protobuf/go/mediator/v1"
)
Expand Down Expand Up @@ -84,7 +85,7 @@ func (jqe *JQEvaluator) Eval(ctx context.Context, pol map[string]any, obj any) e
msg = fmt.Sprintf("%s\nassertion: %s", msg, string(marshalledAssertion))
}

return NewErrEvaluationFailed(msg)
return evalerrors.NewErrEvaluationFailed(msg)
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,15 +13,16 @@
// limitations under the License.
// Package rule provides the CLI subcommand for managing rules

package eval_test
package jq_test

import (
"context"
"testing"

"github.com/stretchr/testify/assert"

"github.com/stacklok/mediator/internal/engine/eval"
evalerrors "github.com/stacklok/mediator/internal/engine/eval/errors"
"github.com/stacklok/mediator/internal/engine/eval/jq"
pb "github.com/stacklok/mediator/pkg/generated/protobuf/go/mediator/v1"
)

Expand Down Expand Up @@ -79,7 +80,7 @@ func TestNewJQEvaluatorValid(t *testing.T) {
t.Run(tt.name, func(t *testing.T) {
t.Parallel()

got, err := eval.NewJQEvaluator(tt.args.assertions)
got, err := jq.NewJQEvaluator(tt.args.assertions)
assert.NoError(t, err, "Got unexpected error")
assert.NotNil(t, got, "Got unexpected nil")
})
Expand Down Expand Up @@ -187,7 +188,7 @@ func TestNewJQEvaluatorInvalid(t *testing.T) {
t.Run(tt.name, func(t *testing.T) {
t.Parallel()

got, err := eval.NewJQEvaluator(tt.args.assertions)
got, err := jq.NewJQEvaluator(tt.args.assertions)
assert.Error(t, err, "expected error")
assert.Nil(t, got, "expected nil")
})
Expand Down Expand Up @@ -296,7 +297,7 @@ func TestValidJQEvals(t *testing.T) {
t.Run(tt.name, func(t *testing.T) {
t.Parallel()

jqe, err := eval.NewJQEvaluator(tt.assertions)
jqe, err := jq.NewJQEvaluator(tt.assertions)
assert.NoError(t, err, "Got unexpected error")
assert.NotNil(t, jqe, "Got unexpected nil")

Expand Down Expand Up @@ -451,12 +452,12 @@ func TestValidJQEvalsFailed(t *testing.T) {
t.Run(tt.name, func(t *testing.T) {
t.Parallel()

jqe, err := eval.NewJQEvaluator(tt.assertions)
jqe, err := jq.NewJQEvaluator(tt.assertions)
assert.NoError(t, err, "Got unexpected error")
assert.NotNil(t, jqe, "Got unexpected nil")

err = jqe.Eval(context.Background(), tt.args.pol, tt.args.obj)
assert.ErrorIs(t, err, eval.ErrEvaluationFailed, "Got unexpected error")
assert.ErrorIs(t, err, evalerrors.ErrEvaluationFailed, "Got unexpected error")
})
}
}
Expand Down Expand Up @@ -523,13 +524,13 @@ func TestInvalidJQEvals(t *testing.T) {
t.Run(tt.name, func(t *testing.T) {
t.Parallel()

jqe, err := eval.NewJQEvaluator(tt.assertions)
jqe, err := jq.NewJQEvaluator(tt.assertions)
assert.NoError(t, err, "Got unexpected error")
assert.NotNil(t, jqe, "Got unexpected nil")

err = jqe.Eval(context.Background(), tt.args.pol, tt.args.obj)
assert.Error(t, err, "Got unexpected error")
assert.NotErrorIs(t, err, eval.ErrEvaluationFailed, "Got unexpected error")
assert.NotErrorIs(t, err, evalerrors.ErrEvaluationFailed, "Got unexpected error")
})
}
}
4 changes: 2 additions & 2 deletions internal/engine/executor.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ import (
"github.com/go-playground/validator/v10"
"google.golang.org/protobuf/types/known/timestamppb"

"github.com/stacklok/mediator/internal/engine/eval"
evalerrors "github.com/stacklok/mediator/internal/engine/eval/errors"
"github.com/stacklok/mediator/internal/events"
"github.com/stacklok/mediator/internal/util"
"github.com/stacklok/mediator/pkg/crypto"
Expand Down Expand Up @@ -608,7 +608,7 @@ func parseRepoID(repoID any) (int32, error) {
}

func errorAsEvalStatus(err error) db.EvalStatusTypes {
if errors.Is(err, eval.ErrEvaluationFailed) {
if errors.Is(err, evalerrors.ErrEvaluationFailed) {
return db.EvalStatusTypesFailure
} else if err != nil {
return db.EvalStatusTypesError
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
// limitations under the License.
// Package rule provides the CLI subcommand for managing rules

package ingester
package builtin

import (
"context"
Expand All @@ -30,6 +30,11 @@ import (
"github.com/stacklok/mediator/pkg/rule_methods"
)

const (
// BuiltinRuleDataIngestType is the type of the builtin rule data ingest engine
BuiltinRuleDataIngestType = "builtin"
)

// BuiltinRuleDataIngest is the engine for a rule type that uses builtin methods
type BuiltinRuleDataIngest struct {
builtinCfg *pb.BuiltinType
Expand Down
11 changes: 6 additions & 5 deletions internal/engine/ingester/ingester.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ import (

"google.golang.org/protobuf/reflect/protoreflect"

"github.com/stacklok/mediator/internal/engine/ingester/builtin"
"github.com/stacklok/mediator/internal/engine/ingester/rest"
pb "github.com/stacklok/mediator/pkg/generated/protobuf/go/mediator/v1"
ghclient "github.com/stacklok/mediator/pkg/providers/github"
)
Expand All @@ -36,20 +38,19 @@ type Ingester interface {
// type definition.
func NewRuleDataIngest(rt *pb.RuleType, cli ghclient.RestAPI, access_token string) (Ingester, error) {
ing := rt.Def.GetIngest()
// TODO: make this more generic and/or use constants
switch rt.Def.Ingest.Type {
case "rest":
case rest.RestRuleDataIngestType:
if rt.Def.Ingest.GetRest() == nil {
return nil, fmt.Errorf("rule type engine missing rest configuration")
}

return NewRestRuleDataIngest(ing.GetRest(), cli)
return rest.NewRestRuleDataIngest(ing.GetRest(), cli)

case "builtin":
case builtin.BuiltinRuleDataIngestType:
if rt.Def.Ingest.GetBuiltin() == nil {
return nil, fmt.Errorf("rule type engine missing internal configuration")
}
return NewBuiltinRuleDataIngest(ing.GetBuiltin(), access_token)
return builtin.NewBuiltinRuleDataIngest(ing.GetBuiltin(), access_token)
default:
return nil, fmt.Errorf("unsupported rule type engine: %s", rt.Def.Ingest.Type)
}
Expand Down
125 changes: 125 additions & 0 deletions internal/engine/ingester/ingester_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,125 @@
// 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 ingester provides necessary interfaces and implementations for ingesting
// data for rules.
package ingester

import (
"testing"

Check failure on line 22 in internal/engine/ingester/ingester_test.go

View workflow job for this annotation

GitHub Actions / golangci-lint / Go Lint

File is not `gci`-ed with --skip-generated -s standard -s default -s prefix(github.com/stacklok/mediator) (gci)
"github.com/stacklok/mediator/internal/engine/ingester/builtin"
"github.com/stacklok/mediator/internal/engine/ingester/rest"
pb "github.com/stacklok/mediator/pkg/generated/protobuf/go/mediator/v1"
"github.com/stretchr/testify/require"

Check failure on line 26 in internal/engine/ingester/ingester_test.go

View workflow job for this annotation

GitHub Actions / golangci-lint / Go Lint

File is not `gci`-ed with --skip-generated -s standard -s default -s prefix(github.com/stacklok/mediator) (gci)
)

func TestNewRuleDataIngest(t *testing.T) {
t.Parallel()

type args struct {
rt *pb.RuleType
access_token string
}
tests := []struct {
name string
args args
wantErr bool
}{
{
name: "rest",
args: args{
rt: &pb.RuleType{
Def: &pb.RuleType_Definition{
Ingest: &pb.RuleType_Definition_Ingest{
Type: rest.RestRuleDataIngestType,
Rest: &pb.RestType{
Endpoint: "https://api.github.com/repos/Foo/Bar",
},
},
},
},
},
},
{
name: "rest missing",
args: args{
rt: &pb.RuleType{
Def: &pb.RuleType_Definition{
Ingest: &pb.RuleType_Definition_Ingest{
Type: rest.RestRuleDataIngestType,
},
},
},
},
wantErr: true,
},
{
name: "builtin",
args: args{
rt: &pb.RuleType{
Def: &pb.RuleType_Definition{
Ingest: &pb.RuleType_Definition_Ingest{
Type: builtin.BuiltinRuleDataIngestType,
Builtin: &pb.BuiltinType{},
},
},
},
},
},
{
name: "builtin missing",
args: args{
rt: &pb.RuleType{
Def: &pb.RuleType_Definition{
Ingest: &pb.RuleType_Definition_Ingest{
Type: builtin.BuiltinRuleDataIngestType,
},
},
},
},
wantErr: true,
},
{
name: "unsupported",
args: args{
rt: &pb.RuleType{
Def: &pb.RuleType_Definition{
Ingest: &pb.RuleType_Definition_Ingest{
Type: "unsupported",
},
},
},
},
wantErr: true,
},
}
for _, tt := range tests {
tt := tt
t.Run(tt.name, func(t *testing.T) {
t.Parallel()

got, err := NewRuleDataIngest(tt.args.rt, nil, tt.args.access_token)
if tt.wantErr {
require.Error(t, err, "Expected error")
require.Nil(t, got, "Expected nil")
return
}

require.NoError(t, err, "Unexpected error")
require.NotNil(t, got, "Expected non-nil")
})
}
}
Loading

0 comments on commit 9868bc1

Please sign in to comment.