From 3681e61621661260dac9845c4972a17ddc3a98a8 Mon Sep 17 00:00:00 2001 From: oanatmaria Date: Thu, 19 Oct 2023 17:33:06 +0300 Subject: [PATCH 1/7] Add validation for Diff struct --- Depfile | 5 +- cache/cache.go | 8 +++ go.mod | 1 + go.sum | 1 + magefiles/magefile.go | 5 ++ model/diff/diff.go | 65 ++++++++++++++++++++++++ model/diff/diff_test.go | 69 ++++++++++++++++++++++++++ model/diff/mock_directory_validator.go | 64 ++++++++++++++++++++++++ model/model.go | 41 ++++++--------- model/model_test.go | 20 ++++---- 10 files changed, 243 insertions(+), 36 deletions(-) create mode 100644 model/diff/diff.go create mode 100644 model/diff/diff_test.go create mode 100644 model/diff/mock_directory_validator.go diff --git a/Depfile b/Depfile index 538a80f..c0f1078 100644 --- a/Depfile +++ b/Depfile @@ -8,4 +8,7 @@ go: version: "v1.10.1" golangci-lint: importPath: "github.com/golangci/golangci-lint/cmd/golangci-lint" - version: "v1.53.3" + version: "v1.54.1" + mockgen: + importPath: github.com/golang/mock/mockgen + version: "v1.6.0" diff --git a/cache/cache.go b/cache/cache.go index c3393ff..ade00dd 100644 --- a/cache/cache.go +++ b/cache/cache.go @@ -27,6 +27,14 @@ func (c *Cache) UpdateModel(m *model.Model) error { return nil } +// Returns a copy of the current model. +func (c *Cache) GetModel() model.Model { + c.mtx.Lock() + defer c.mtx.Unlock() + m := *c.model + return m +} + // ObjectExists, checks if given object type name exists in the model cache. func (c *Cache) ObjectExists(on model.ObjectName) bool { c.mtx.RLock() diff --git a/go.mod b/go.mod index 36f7d5f..b694f6f 100644 --- a/go.mod +++ b/go.mod @@ -7,6 +7,7 @@ require ( github.com/aserto-dev/errors v0.0.6 github.com/aserto-dev/go-aserto v0.20.4 github.com/aserto-dev/go-directory v0.30.0 + github.com/golang/mock v1.1.1 github.com/magefile/mage v1.15.0 github.com/nsf/jsondiff v0.0.0-20230430225905-43f6cf3098c1 github.com/pkg/errors v0.9.1 diff --git a/go.sum b/go.sum index e5cdd25..4af4e88 100644 --- a/go.sum +++ b/go.sum @@ -28,6 +28,7 @@ github.com/ghodss/yaml v1.0.0/go.mod h1:4dBDuWmgqj2HViK6kFavaiC9ZROes6MMH2rRYeME github.com/godbus/dbus/v5 v5.0.4/go.mod h1:xhWf0FNVPg57R7Z0UbKHbJfkEywrmjJnf7w5xrFpKfA= github.com/golang/glog v0.0.0-20160126235308-23def4e6c14b/go.mod h1:SBH7ygxi8pfUlaOkMMuAQtPIUF8ecWP5IEl/CR7VP2Q= github.com/golang/glog v1.1.2 h1:DVjP2PbBOzHyzA+dn3WhHIq4NdVu3Q+pvivFICf/7fo= +github.com/golang/mock v1.1.1 h1:G5FRp8JnTd7RQH5kemVNlMeyXQAztQ3mOWV95KxsXH8= github.com/golang/mock v1.1.1/go.mod h1:oTYuIxOrZwtPieC+H1uAHpcLFnEyAGVDL/k47Jfbm0A= github.com/golang/protobuf v1.2.0/go.mod h1:6lQm79b+lXiMfvg/cZm0SGofjICqVBUtrP5yJMmIC1U= github.com/golang/protobuf v1.3.2/go.mod h1:6lQm79b+lXiMfvg/cZm0SGofjICqVBUtrP5yJMmIC1U= diff --git a/magefiles/magefile.go b/magefiles/magefile.go index dd2a4f0..cb2c819 100644 --- a/magefiles/magefile.go +++ b/magefiles/magefile.go @@ -39,3 +39,8 @@ func Test() error { func Deps() { deps.GetAllDeps() } + +// Generate generates all code. +func Generate() error { + return common.Generate() +} diff --git a/model/diff/diff.go b/model/diff/diff.go new file mode 100644 index 0000000..108d15b --- /dev/null +++ b/model/diff/diff.go @@ -0,0 +1,65 @@ +package diff + +import ( + "context" + + "github.com/aserto-dev/go-directory/pkg/derr" + "github.com/pkg/errors" +) + +//go:generate go run github.com/golang/mock/mockgen -destination=mock_directory_validator.go -package=diff github.com/aserto-dev/azm/model/diff DirectoryValidator + +type Diff struct { + Added Changes + Removed Changes +} + +type Changes struct { + Objects []string + Relations map[string][]string +} + +type DirectoryValidator interface { + HasObjectInstances(ctx context.Context, objectType string) (bool, error) + HasRelationInstances(ctx context.Context, objectType, relationName string) (bool, error) +} + +func (d *Diff) Validate(ctx context.Context, dv DirectoryValidator) error { + if err := d.validateObjectTypes(ctx, dv); err != nil { + return err + } + + if err := d.validateRelationsTypes(ctx, dv); err != nil { + return err + } + + return nil +} + +func (d *Diff) validateObjectTypes(ctx context.Context, dv DirectoryValidator) error { + for _, objType := range d.Removed.Objects { + hasInstance, err := dv.HasObjectInstances(ctx, objType) + if err != nil { + return err + } + if hasInstance { + return errors.Wrapf(derr.ErrObjectTypeInUse, "object type: %s", objType) + } + } + return nil +} + +func (d *Diff) validateRelationsTypes(ctx context.Context, dv DirectoryValidator) error { + for objType, rels := range d.Removed.Relations { + for _, rel := range rels { + hasInstance, err := dv.HasRelationInstances(ctx, objType, rel) + if err != nil { + return err + } + if hasInstance { + return errors.Wrapf(derr.ErrRelationTypeInUse, "relation type: %s; object type: %s", rel, objType) + } + } + } + return nil +} diff --git a/model/diff/diff_test.go b/model/diff/diff_test.go new file mode 100644 index 0000000..35f1d1d --- /dev/null +++ b/model/diff/diff_test.go @@ -0,0 +1,69 @@ +package diff_test + +import ( + "context" + "testing" + + "github.com/aserto-dev/azm/model/diff" + "github.com/aserto-dev/go-directory/pkg/derr" + gomock "github.com/golang/mock/gomock" + "github.com/stretchr/testify/require" +) + +func TestValidateDiffNoDeletion(t *testing.T) { + ctrl := gomock.NewController(t) + mockDirectoryValidator := diff.NewMockDirectoryValidator(ctrl) + + dif := diff.Diff{Removed: diff.Changes{}, Added: diff.Changes{}} + err := dif.Validate(context.Background(), mockDirectoryValidator) + + require.NoError(t, err) +} + +func TestValidateDiffWithObjectTypeDeletion(t *testing.T) { + ctrl := gomock.NewController(t) + mockDirectoryValidator := diff.NewMockDirectoryValidator(ctrl) + objType := "user" + bCtx := context.Background() + + dif := diff.Diff{Removed: diff.Changes{Objects: []string{objType}}, Added: diff.Changes{}} + + mockDirectoryValidator.EXPECT().HasObjectInstances(bCtx, objType).Return(false, nil) + err := dif.Validate(bCtx, mockDirectoryValidator) + + require.NoError(t, err) +} + +func TestValidateDiffWith2ObjectTypeDeletion(t *testing.T) { + ctrl := gomock.NewController(t) + mockDirectoryValidator := diff.NewMockDirectoryValidator(ctrl) + objTypes := []string{"user", "member"} + bCtx := context.Background() + + dif := diff.Diff{Removed: diff.Changes{Objects: objTypes}, Added: diff.Changes{}} + + mockDirectoryValidator.EXPECT().HasObjectInstances(bCtx, objTypes[0]).Return(false, nil) + mockDirectoryValidator.EXPECT().HasObjectInstances(bCtx, objTypes[1]).Return(true, nil) + err := dif.Validate(bCtx, mockDirectoryValidator) + + require.Error(t, err) + require.Contains(t, err.Error(), derr.ErrObjectTypeInUse.Message) +} + +func TestValidateDiffWithRelationTypeDeletion(t *testing.T) { + ctrl := gomock.NewController(t) + mockDirectoryValidator := diff.NewMockDirectoryValidator(ctrl) + objTypes := []string{"user", "member"} + relationTypes := map[string][]string{"folder": {"parent_folder"}} + bCtx := context.Background() + + dif := diff.Diff{Removed: diff.Changes{Objects: objTypes, Relations: relationTypes}, Added: diff.Changes{}} + + mockDirectoryValidator.EXPECT().HasObjectInstances(bCtx, objTypes[0]).Return(false, nil) + mockDirectoryValidator.EXPECT().HasObjectInstances(bCtx, objTypes[1]).Return(false, nil) + mockDirectoryValidator.EXPECT().HasRelationInstances(bCtx, "folder", relationTypes["folder"][0]).Return(true, nil) + err := dif.Validate(bCtx, mockDirectoryValidator) + + require.Error(t, err) + require.Contains(t, err.Error(), derr.ErrRelationTypeInUse.Message) +} diff --git a/model/diff/mock_directory_validator.go b/model/diff/mock_directory_validator.go new file mode 100644 index 0000000..256c44d --- /dev/null +++ b/model/diff/mock_directory_validator.go @@ -0,0 +1,64 @@ +// Code generated by MockGen. DO NOT EDIT. +// Source: github.com/aserto-dev/azm/model/diff (interfaces: DirectoryValidator) + +// Package diff is a generated GoMock package. +package diff + +import ( + context "context" + gomock "github.com/golang/mock/gomock" + reflect "reflect" +) + +// MockDirectoryValidator is a mock of DirectoryValidator interface +type MockDirectoryValidator struct { + ctrl *gomock.Controller + recorder *MockDirectoryValidatorMockRecorder +} + +// MockDirectoryValidatorMockRecorder is the mock recorder for MockDirectoryValidator +type MockDirectoryValidatorMockRecorder struct { + mock *MockDirectoryValidator +} + +// NewMockDirectoryValidator creates a new mock instance +func NewMockDirectoryValidator(ctrl *gomock.Controller) *MockDirectoryValidator { + mock := &MockDirectoryValidator{ctrl: ctrl} + mock.recorder = &MockDirectoryValidatorMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use +func (m *MockDirectoryValidator) EXPECT() *MockDirectoryValidatorMockRecorder { + return m.recorder +} + +// HasObjectInstances mocks base method +func (m *MockDirectoryValidator) HasObjectInstances(arg0 context.Context, arg1 string) (bool, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "HasObjectInstances", arg0, arg1) + ret0, _ := ret[0].(bool) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// HasObjectInstances indicates an expected call of HasObjectInstances +func (mr *MockDirectoryValidatorMockRecorder) HasObjectInstances(arg0, arg1 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "HasObjectInstances", reflect.TypeOf((*MockDirectoryValidator)(nil).HasObjectInstances), arg0, arg1) +} + +// HasRelationInstances mocks base method +func (m *MockDirectoryValidator) HasRelationInstances(arg0 context.Context, arg1, arg2 string) (bool, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "HasRelationInstances", arg0, arg1, arg2) + ret0, _ := ret[0].(bool) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// HasRelationInstances indicates an expected call of HasRelationInstances +func (mr *MockDirectoryValidatorMockRecorder) HasRelationInstances(arg0, arg1, arg2 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "HasRelationInstances", reflect.TypeOf((*MockDirectoryValidator)(nil).HasRelationInstances), arg0, arg1, arg2) +} diff --git a/model/model.go b/model/model.go index 9aef76d..c1d612d 100644 --- a/model/model.go +++ b/model/model.go @@ -7,6 +7,7 @@ import ( "time" "github.com/aserto-dev/azm/graph" + "github.com/aserto-dev/azm/model/diff" ) const ModelVersion int = 1 @@ -59,16 +60,6 @@ type Metadata struct { ETag string `json:"etag"` } -type Diff struct { - Added Changes - Removed Changes -} - -type Changes struct { - Objects []ObjectName - Relations map[ObjectName][]RelationName -} - func New(r io.Reader) (*Model, error) { m := Model{} dec := json.NewDecoder(r) @@ -113,43 +104,43 @@ func (m *Model) Write(w io.Writer) error { return enc.Encode(m) } -func (m *Model) Diff(newModel *Model) *Diff { - // newModel - m => additions +func (m *Model) Diff(newModel *Model) *diff.Diff { + // newmodel - m => additions added := newModel.subtract(m) // m - newModel => deletions deleted := m.subtract(newModel) - return &Diff{Added: *added, Removed: *deleted} + return &diff.Diff{Added: *added, Removed: *deleted} } -func (m *Model) subtract(newModel *Model) *Changes { - changes := &Changes{ - Objects: make([]ObjectName, 0), - Relations: make(map[ObjectName][]RelationName), +func (m *Model) subtract(newModel *Model) *diff.Changes { + chgs := &diff.Changes{ + Objects: make([]string, 0), + Relations: make(map[string][]string), } if m == nil { - return changes + return chgs } if newModel == nil { for objName := range m.Objects { - changes.Objects = append(changes.Objects, objName) + chgs.Objects = append(chgs.Objects, string(objName)) } - return changes + return chgs } for objName, obj := range m.Objects { if newModel.Objects[objName] == nil { - changes.Objects = append(changes.Objects, objName) + chgs.Objects = append(chgs.Objects, string(objName)) } else { - for relName := range obj.Relations { - if newModel.Objects[objName].Relations[relName] == nil { - changes.Relations[objName] = append(changes.Relations[objName], relName) + for relname := range obj.Relations { + if newModel.Objects[objName].Relations[relname] == nil { + chgs.Relations[string(objName)] = append(chgs.Relations[string(objName)], string(relname)) } } } } - return changes + return chgs } diff --git a/model/model_test.go b/model/model_test.go index 45ae065..8b63d2d 100644 --- a/model/model_test.go +++ b/model/model_test.go @@ -215,16 +215,16 @@ func TestDiff(t *testing.T) { stretch.Equal(t, len(diffNilWithNil.Added.Relations), 0) stretch.Equal(t, len(diffNilWithNil.Removed.Relations), 0) - diffM1WithM3 := m1.Diff(&m3) - stretch.Equal(t, len(diffM1WithM3.Added.Objects), 1) - stretch.Equal(t, diffM1WithM3.Added.Objects[0], model.ObjectName("new_user")) - stretch.Equal(t, len(diffM1WithM3.Removed.Objects), 1) - stretch.Equal(t, diffM1WithM3.Removed.Objects[0], model.ObjectName("user")) - - stretch.Equal(t, len(diffM1WithM3.Added.Relations), 1) - stretch.Equal(t, diffM1WithM3.Added.Relations["folder"], []model.RelationName{"viewer"}) - stretch.Equal(t, len(diffM1WithM3.Removed.Relations), 1) - stretch.Equal(t, diffM1WithM3.Removed.Relations["document"], []model.RelationName{"parent_folder"}) + diffm1m3 := m1.Diff(&m3) + stretch.Equal(t, len(diffm1m3.Added.Objects), 1) + stretch.Equal(t, diffm1m3.Added.Objects[0], "new_user") + stretch.Equal(t, len(diffm1m3.Removed.Objects), 1) + stretch.Equal(t, diffm1m3.Removed.Objects[0], "user") + + stretch.Equal(t, len(diffm1m3.Added.Relations), 1) + stretch.Equal(t, diffm1m3.Added.Relations["folder"], []string{"viewer"}) + stretch.Equal(t, len(diffm1m3.Removed.Relations), 1) + stretch.Equal(t, diffm1m3.Removed.Relations["document"], []string{"parent_folder"}) } func TestGraph(t *testing.T) { From 221755404322dd9c5c4a45f273c7aa36cd1e5e1f Mon Sep 17 00:00:00 2001 From: oanatmaria Date: Mon, 23 Oct 2023 13:28:44 +0300 Subject: [PATCH 2/7] Resolve comments --- cache/cache.go | 8 ++-- go.mod | 2 + go.sum | 4 ++ model/diff/diff.go | 42 +++++++++-------- model/diff/diff_test.go | 41 +++++++++++++---- model/diff/mock_directory_validator.go | 64 -------------------------- model/diff/mock_instances.go | 64 ++++++++++++++++++++++++++ 7 files changed, 129 insertions(+), 96 deletions(-) delete mode 100644 model/diff/mock_directory_validator.go create mode 100644 model/diff/mock_instances.go diff --git a/cache/cache.go b/cache/cache.go index ade00dd..0f81788 100644 --- a/cache/cache.go +++ b/cache/cache.go @@ -4,6 +4,7 @@ import ( "sync" "github.com/aserto-dev/azm/model" + "github.com/aserto-dev/azm/model/diff" ) type Cache struct { @@ -27,12 +28,11 @@ func (c *Cache) UpdateModel(m *model.Model) error { return nil } -// Returns a copy of the current model. -func (c *Cache) GetModel() model.Model { +// Returns a diff struct resulted between the old and the new model. +func (c *Cache) Diff(other *model.Model) *diff.Diff { c.mtx.Lock() defer c.mtx.Unlock() - m := *c.model - return m + return c.model.Diff(other) } // ObjectExists, checks if given object type name exists in the model cache. diff --git a/go.mod b/go.mod index b694f6f..0152531 100644 --- a/go.mod +++ b/go.mod @@ -8,6 +8,7 @@ require ( github.com/aserto-dev/go-aserto v0.20.4 github.com/aserto-dev/go-directory v0.30.0 github.com/golang/mock v1.1.1 + github.com/hashicorp/go-multierror v1.1.1 github.com/magefile/mage v1.15.0 github.com/nsf/jsondiff v0.0.0-20230430225905-43f6cf3098c1 github.com/pkg/errors v0.9.1 @@ -27,6 +28,7 @@ require ( github.com/google/uuid v1.3.1 // indirect github.com/grpc-ecosystem/grpc-gateway v1.16.0 // indirect github.com/grpc-ecosystem/grpc-gateway/v2 v2.18.0 // indirect + github.com/hashicorp/errwrap v1.0.0 // indirect github.com/mattn/go-colorable v0.1.13 // indirect github.com/mattn/go-isatty v0.0.19 // indirect github.com/pmezard/go-difflib v1.0.0 // indirect diff --git a/go.sum b/go.sum index 4af4e88..3af91f9 100644 --- a/go.sum +++ b/go.sum @@ -47,6 +47,10 @@ github.com/grpc-ecosystem/grpc-gateway v1.16.0 h1:gmcG1KaJ57LophUzW0Hy8NmPhnMZb4 github.com/grpc-ecosystem/grpc-gateway v1.16.0/go.mod h1:BDjrQk3hbvj6Nolgz8mAMFbcEtjT1g+wF4CSlocrBnw= github.com/grpc-ecosystem/grpc-gateway/v2 v2.18.0 h1:RtRsiaGvWxcwd8y3BiRZxsylPT8hLWZ5SPcfI+3IDNk= github.com/grpc-ecosystem/grpc-gateway/v2 v2.18.0/go.mod h1:TzP6duP4Py2pHLVPPQp42aoYI92+PCrVotyR5e8Vqlk= +github.com/hashicorp/errwrap v1.0.0 h1:hLrqtEDnRye3+sgx6z4qVLNuviH3MR5aQ0ykNJa/UYA= +github.com/hashicorp/errwrap v1.0.0/go.mod h1:YH+1FKiLXxHSkmPseP+kNlulaMuP3n2brvKWEqk/Jc4= +github.com/hashicorp/go-multierror v1.1.1 h1:H5DkEtf6CXdFp0N0Em5UCwQpXMWke8IA0+lD48awMYo= +github.com/hashicorp/go-multierror v1.1.1/go.mod h1:iw975J/qwKPdAO1clOe2L8331t/9/fmwbPZ6JB6eMoM= github.com/hexops/gotextdiff v1.0.3 h1:gitA9+qJrrTCsiCl7+kh75nPqQt1cx4ZkudSTLoUqJM= github.com/kr/pretty v0.3.1 h1:flRD4NNwYAUpkphVc1HcthR4KEIFJ65n8Mw5qdRn3LE= github.com/kr/text v0.2.0 h1:5Nx0Ya0ZqY2ygV366QzturHI13Jq95ApcVaJBhpS+AY= diff --git a/model/diff/diff.go b/model/diff/diff.go index 108d15b..0b5445f 100644 --- a/model/diff/diff.go +++ b/model/diff/diff.go @@ -4,10 +4,11 @@ import ( "context" "github.com/aserto-dev/go-directory/pkg/derr" + "github.com/hashicorp/go-multierror" "github.com/pkg/errors" ) -//go:generate go run github.com/golang/mock/mockgen -destination=mock_directory_validator.go -package=diff github.com/aserto-dev/azm/model/diff DirectoryValidator +//go:generate go run github.com/golang/mock/mockgen -destination=mock_instances.go -package=diff github.com/aserto-dev/azm/model/diff Instances type Diff struct { Added Changes @@ -19,47 +20,52 @@ type Changes struct { Relations map[string][]string } -type DirectoryValidator interface { - HasObjectInstances(ctx context.Context, objectType string) (bool, error) - HasRelationInstances(ctx context.Context, objectType, relationName string) (bool, error) +type Instances interface { + ObjectsExist(ctx context.Context, objectType string) (bool, error) + RelationsExist(ctx context.Context, objectType, relationName string) (bool, error) } -func (d *Diff) Validate(ctx context.Context, dv DirectoryValidator) error { +func (d *Diff) Validate(ctx context.Context, dv Instances) error { + var errs error if err := d.validateObjectTypes(ctx, dv); err != nil { - return err + errs = multierror.Append(errs, err) } if err := d.validateRelationsTypes(ctx, dv); err != nil { - return err + errs = multierror.Append(errs, err) } - return nil + return errs } -func (d *Diff) validateObjectTypes(ctx context.Context, dv DirectoryValidator) error { +func (d *Diff) validateObjectTypes(ctx context.Context, dv Instances) error { + var errs error for _, objType := range d.Removed.Objects { - hasInstance, err := dv.HasObjectInstances(ctx, objType) + hasInstance, err := dv.ObjectsExist(ctx, objType) if err != nil { - return err + errs = multierror.Append(errs, err) + continue } if hasInstance { - return errors.Wrapf(derr.ErrObjectTypeInUse, "object type: %s", objType) + errs = multierror.Append(errs, errors.Wrapf(derr.ErrObjectTypeInUse, "object type: %s", objType)) } } - return nil + return errs } -func (d *Diff) validateRelationsTypes(ctx context.Context, dv DirectoryValidator) error { +func (d *Diff) validateRelationsTypes(ctx context.Context, dv Instances) error { + var errs error for objType, rels := range d.Removed.Relations { for _, rel := range rels { - hasInstance, err := dv.HasRelationInstances(ctx, objType, rel) + hasInstance, err := dv.RelationsExist(ctx, objType, rel) if err != nil { - return err + errs = multierror.Append(errs, err) + continue } if hasInstance { - return errors.Wrapf(derr.ErrRelationTypeInUse, "relation type: %s; object type: %s", rel, objType) + errs = multierror.Append(errs, errors.Wrapf(derr.ErrRelationTypeInUse, "relation type: %s; object type: %s", rel, objType)) } } } - return nil + return errs } diff --git a/model/diff/diff_test.go b/model/diff/diff_test.go index 35f1d1d..849164a 100644 --- a/model/diff/diff_test.go +++ b/model/diff/diff_test.go @@ -2,6 +2,7 @@ package diff_test import ( "context" + "errors" "testing" "github.com/aserto-dev/azm/model/diff" @@ -12,7 +13,7 @@ import ( func TestValidateDiffNoDeletion(t *testing.T) { ctrl := gomock.NewController(t) - mockDirectoryValidator := diff.NewMockDirectoryValidator(ctrl) + mockDirectoryValidator := diff.NewMockInstances(ctrl) dif := diff.Diff{Removed: diff.Changes{}, Added: diff.Changes{}} err := dif.Validate(context.Background(), mockDirectoryValidator) @@ -22,13 +23,13 @@ func TestValidateDiffNoDeletion(t *testing.T) { func TestValidateDiffWithObjectTypeDeletion(t *testing.T) { ctrl := gomock.NewController(t) - mockDirectoryValidator := diff.NewMockDirectoryValidator(ctrl) + mockDirectoryValidator := diff.NewMockInstances(ctrl) objType := "user" bCtx := context.Background() dif := diff.Diff{Removed: diff.Changes{Objects: []string{objType}}, Added: diff.Changes{}} - mockDirectoryValidator.EXPECT().HasObjectInstances(bCtx, objType).Return(false, nil) + mockDirectoryValidator.EXPECT().ObjectsExist(bCtx, objType).Return(false, nil) err := dif.Validate(bCtx, mockDirectoryValidator) require.NoError(t, err) @@ -36,14 +37,14 @@ func TestValidateDiffWithObjectTypeDeletion(t *testing.T) { func TestValidateDiffWith2ObjectTypeDeletion(t *testing.T) { ctrl := gomock.NewController(t) - mockDirectoryValidator := diff.NewMockDirectoryValidator(ctrl) + mockDirectoryValidator := diff.NewMockInstances(ctrl) objTypes := []string{"user", "member"} bCtx := context.Background() dif := diff.Diff{Removed: diff.Changes{Objects: objTypes}, Added: diff.Changes{}} - mockDirectoryValidator.EXPECT().HasObjectInstances(bCtx, objTypes[0]).Return(false, nil) - mockDirectoryValidator.EXPECT().HasObjectInstances(bCtx, objTypes[1]).Return(true, nil) + mockDirectoryValidator.EXPECT().ObjectsExist(bCtx, objTypes[0]).Return(false, nil) + mockDirectoryValidator.EXPECT().ObjectsExist(bCtx, objTypes[1]).Return(true, nil) err := dif.Validate(bCtx, mockDirectoryValidator) require.Error(t, err) @@ -52,18 +53,38 @@ func TestValidateDiffWith2ObjectTypeDeletion(t *testing.T) { func TestValidateDiffWithRelationTypeDeletion(t *testing.T) { ctrl := gomock.NewController(t) - mockDirectoryValidator := diff.NewMockDirectoryValidator(ctrl) + mockDirectoryValidator := diff.NewMockInstances(ctrl) objTypes := []string{"user", "member"} relationTypes := map[string][]string{"folder": {"parent_folder"}} bCtx := context.Background() dif := diff.Diff{Removed: diff.Changes{Objects: objTypes, Relations: relationTypes}, Added: diff.Changes{}} - mockDirectoryValidator.EXPECT().HasObjectInstances(bCtx, objTypes[0]).Return(false, nil) - mockDirectoryValidator.EXPECT().HasObjectInstances(bCtx, objTypes[1]).Return(false, nil) - mockDirectoryValidator.EXPECT().HasRelationInstances(bCtx, "folder", relationTypes["folder"][0]).Return(true, nil) + mockDirectoryValidator.EXPECT().ObjectsExist(bCtx, objTypes[0]).Return(false, nil) + mockDirectoryValidator.EXPECT().ObjectsExist(bCtx, objTypes[1]).Return(false, nil) + mockDirectoryValidator.EXPECT().RelationsExist(bCtx, "folder", relationTypes["folder"][0]).Return(true, nil) err := dif.Validate(bCtx, mockDirectoryValidator) require.Error(t, err) require.Contains(t, err.Error(), derr.ErrRelationTypeInUse.Message) } + +func TestValidateDiffWithObjectInstances(t *testing.T) { + ctrl := gomock.NewController(t) + mockDirectoryValidator := diff.NewMockInstances(ctrl) + objTypes := []string{"user", "member"} + relationTypes := map[string][]string{"folder": {"parent_folder"}} + bCtx := context.Background() + + dif := diff.Diff{Removed: diff.Changes{Objects: objTypes, Relations: relationTypes}, Added: diff.Changes{}} + + mockDirectoryValidator.EXPECT().ObjectsExist(bCtx, objTypes[0]).Return(false, errors.New("Boom!")) + mockDirectoryValidator.EXPECT().ObjectsExist(bCtx, objTypes[1]).Return(true, nil) + mockDirectoryValidator.EXPECT().RelationsExist(bCtx, "folder", relationTypes["folder"][0]).Return(true, nil) + err := dif.Validate(bCtx, mockDirectoryValidator) + + require.Error(t, err) + require.Contains(t, err.Error(), derr.ErrRelationTypeInUse.Message) + require.Contains(t, err.Error(), derr.ErrObjectTypeInUse.Message) + require.Contains(t, err.Error(), "Boom!") +} diff --git a/model/diff/mock_directory_validator.go b/model/diff/mock_directory_validator.go deleted file mode 100644 index 256c44d..0000000 --- a/model/diff/mock_directory_validator.go +++ /dev/null @@ -1,64 +0,0 @@ -// Code generated by MockGen. DO NOT EDIT. -// Source: github.com/aserto-dev/azm/model/diff (interfaces: DirectoryValidator) - -// Package diff is a generated GoMock package. -package diff - -import ( - context "context" - gomock "github.com/golang/mock/gomock" - reflect "reflect" -) - -// MockDirectoryValidator is a mock of DirectoryValidator interface -type MockDirectoryValidator struct { - ctrl *gomock.Controller - recorder *MockDirectoryValidatorMockRecorder -} - -// MockDirectoryValidatorMockRecorder is the mock recorder for MockDirectoryValidator -type MockDirectoryValidatorMockRecorder struct { - mock *MockDirectoryValidator -} - -// NewMockDirectoryValidator creates a new mock instance -func NewMockDirectoryValidator(ctrl *gomock.Controller) *MockDirectoryValidator { - mock := &MockDirectoryValidator{ctrl: ctrl} - mock.recorder = &MockDirectoryValidatorMockRecorder{mock} - return mock -} - -// EXPECT returns an object that allows the caller to indicate expected use -func (m *MockDirectoryValidator) EXPECT() *MockDirectoryValidatorMockRecorder { - return m.recorder -} - -// HasObjectInstances mocks base method -func (m *MockDirectoryValidator) HasObjectInstances(arg0 context.Context, arg1 string) (bool, error) { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "HasObjectInstances", arg0, arg1) - ret0, _ := ret[0].(bool) - ret1, _ := ret[1].(error) - return ret0, ret1 -} - -// HasObjectInstances indicates an expected call of HasObjectInstances -func (mr *MockDirectoryValidatorMockRecorder) HasObjectInstances(arg0, arg1 interface{}) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "HasObjectInstances", reflect.TypeOf((*MockDirectoryValidator)(nil).HasObjectInstances), arg0, arg1) -} - -// HasRelationInstances mocks base method -func (m *MockDirectoryValidator) HasRelationInstances(arg0 context.Context, arg1, arg2 string) (bool, error) { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "HasRelationInstances", arg0, arg1, arg2) - ret0, _ := ret[0].(bool) - ret1, _ := ret[1].(error) - return ret0, ret1 -} - -// HasRelationInstances indicates an expected call of HasRelationInstances -func (mr *MockDirectoryValidatorMockRecorder) HasRelationInstances(arg0, arg1, arg2 interface{}) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "HasRelationInstances", reflect.TypeOf((*MockDirectoryValidator)(nil).HasRelationInstances), arg0, arg1, arg2) -} diff --git a/model/diff/mock_instances.go b/model/diff/mock_instances.go new file mode 100644 index 0000000..29c4abf --- /dev/null +++ b/model/diff/mock_instances.go @@ -0,0 +1,64 @@ +// Code generated by MockGen. DO NOT EDIT. +// Source: github.com/aserto-dev/azm/model/diff (interfaces: Instances) + +// Package diff is a generated GoMock package. +package diff + +import ( + context "context" + gomock "github.com/golang/mock/gomock" + reflect "reflect" +) + +// MockInstances is a mock of Instances interface +type MockInstances struct { + ctrl *gomock.Controller + recorder *MockInstancesMockRecorder +} + +// MockInstancesMockRecorder is the mock recorder for MockInstances +type MockInstancesMockRecorder struct { + mock *MockInstances +} + +// NewMockInstances creates a new mock instance +func NewMockInstances(ctrl *gomock.Controller) *MockInstances { + mock := &MockInstances{ctrl: ctrl} + mock.recorder = &MockInstancesMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use +func (m *MockInstances) EXPECT() *MockInstancesMockRecorder { + return m.recorder +} + +// ObjectsExist mocks base method +func (m *MockInstances) ObjectsExist(arg0 context.Context, arg1 string) (bool, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "ObjectsExist", arg0, arg1) + ret0, _ := ret[0].(bool) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// ObjectsExist indicates an expected call of ObjectsExist +func (mr *MockInstancesMockRecorder) ObjectsExist(arg0, arg1 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ObjectsExist", reflect.TypeOf((*MockInstances)(nil).ObjectsExist), arg0, arg1) +} + +// RelationsExist mocks base method +func (m *MockInstances) RelationsExist(arg0 context.Context, arg1, arg2 string) (bool, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "RelationsExist", arg0, arg1, arg2) + ret0, _ := ret[0].(bool) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// RelationsExist indicates an expected call of RelationsExist +func (mr *MockInstancesMockRecorder) RelationsExist(arg0, arg1, arg2 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "RelationsExist", reflect.TypeOf((*MockInstances)(nil).RelationsExist), arg0, arg1, arg2) +} From 2022c73d7c46aafbcd6774f8169a4b304cc7a5d0 Mon Sep 17 00:00:00 2001 From: oanatmaria Date: Mon, 23 Oct 2023 13:43:40 +0300 Subject: [PATCH 3/7] Remove context as parameter --- model/diff/diff.go | 20 +++++++-------- model/diff/diff_test.go | 47 +++++++++++++++++------------------- model/diff/mock_instances.go | 17 ++++++------- 3 files changed, 39 insertions(+), 45 deletions(-) diff --git a/model/diff/diff.go b/model/diff/diff.go index 0b5445f..82d0b89 100644 --- a/model/diff/diff.go +++ b/model/diff/diff.go @@ -1,8 +1,6 @@ package diff import ( - "context" - "github.com/aserto-dev/go-directory/pkg/derr" "github.com/hashicorp/go-multierror" "github.com/pkg/errors" @@ -21,27 +19,27 @@ type Changes struct { } type Instances interface { - ObjectsExist(ctx context.Context, objectType string) (bool, error) - RelationsExist(ctx context.Context, objectType, relationName string) (bool, error) + ObjectsExist(objectType string) (bool, error) + RelationsExist(objectType, relationName string) (bool, error) } -func (d *Diff) Validate(ctx context.Context, dv Instances) error { +func (d *Diff) Validate(dv Instances) error { var errs error - if err := d.validateObjectTypes(ctx, dv); err != nil { + if err := d.validateObjectTypes(dv); err != nil { errs = multierror.Append(errs, err) } - if err := d.validateRelationsTypes(ctx, dv); err != nil { + if err := d.validateRelationsTypes(dv); err != nil { errs = multierror.Append(errs, err) } return errs } -func (d *Diff) validateObjectTypes(ctx context.Context, dv Instances) error { +func (d *Diff) validateObjectTypes(dv Instances) error { var errs error for _, objType := range d.Removed.Objects { - hasInstance, err := dv.ObjectsExist(ctx, objType) + hasInstance, err := dv.ObjectsExist(objType) if err != nil { errs = multierror.Append(errs, err) continue @@ -53,11 +51,11 @@ func (d *Diff) validateObjectTypes(ctx context.Context, dv Instances) error { return errs } -func (d *Diff) validateRelationsTypes(ctx context.Context, dv Instances) error { +func (d *Diff) validateRelationsTypes(dv Instances) error { var errs error for objType, rels := range d.Removed.Relations { for _, rel := range rels { - hasInstance, err := dv.RelationsExist(ctx, objType, rel) + hasInstance, err := dv.RelationsExist(objType, rel) if err != nil { errs = multierror.Append(errs, err) continue diff --git a/model/diff/diff_test.go b/model/diff/diff_test.go index 849164a..d75d61b 100644 --- a/model/diff/diff_test.go +++ b/model/diff/diff_test.go @@ -1,7 +1,6 @@ package diff_test import ( - "context" "errors" "testing" @@ -11,41 +10,41 @@ import ( "github.com/stretchr/testify/require" ) +var ErrBoom = errors.New("Boom") + func TestValidateDiffNoDeletion(t *testing.T) { ctrl := gomock.NewController(t) - mockDirectoryValidator := diff.NewMockInstances(ctrl) + mockInstances := diff.NewMockInstances(ctrl) dif := diff.Diff{Removed: diff.Changes{}, Added: diff.Changes{}} - err := dif.Validate(context.Background(), mockDirectoryValidator) + err := dif.Validate(mockInstances) require.NoError(t, err) } func TestValidateDiffWithObjectTypeDeletion(t *testing.T) { ctrl := gomock.NewController(t) - mockDirectoryValidator := diff.NewMockInstances(ctrl) + mockInstances := diff.NewMockInstances(ctrl) objType := "user" - bCtx := context.Background() dif := diff.Diff{Removed: diff.Changes{Objects: []string{objType}}, Added: diff.Changes{}} - mockDirectoryValidator.EXPECT().ObjectsExist(bCtx, objType).Return(false, nil) - err := dif.Validate(bCtx, mockDirectoryValidator) + mockInstances.EXPECT().ObjectsExist(objType).Return(false, nil) + err := dif.Validate(mockInstances) require.NoError(t, err) } func TestValidateDiffWith2ObjectTypeDeletion(t *testing.T) { ctrl := gomock.NewController(t) - mockDirectoryValidator := diff.NewMockInstances(ctrl) + mockInstances := diff.NewMockInstances(ctrl) objTypes := []string{"user", "member"} - bCtx := context.Background() dif := diff.Diff{Removed: diff.Changes{Objects: objTypes}, Added: diff.Changes{}} - mockDirectoryValidator.EXPECT().ObjectsExist(bCtx, objTypes[0]).Return(false, nil) - mockDirectoryValidator.EXPECT().ObjectsExist(bCtx, objTypes[1]).Return(true, nil) - err := dif.Validate(bCtx, mockDirectoryValidator) + mockInstances.EXPECT().ObjectsExist(objTypes[0]).Return(false, nil) + mockInstances.EXPECT().ObjectsExist(objTypes[1]).Return(true, nil) + err := dif.Validate(mockInstances) require.Error(t, err) require.Contains(t, err.Error(), derr.ErrObjectTypeInUse.Message) @@ -53,17 +52,16 @@ func TestValidateDiffWith2ObjectTypeDeletion(t *testing.T) { func TestValidateDiffWithRelationTypeDeletion(t *testing.T) { ctrl := gomock.NewController(t) - mockDirectoryValidator := diff.NewMockInstances(ctrl) + mockInstances := diff.NewMockInstances(ctrl) objTypes := []string{"user", "member"} relationTypes := map[string][]string{"folder": {"parent_folder"}} - bCtx := context.Background() dif := diff.Diff{Removed: diff.Changes{Objects: objTypes, Relations: relationTypes}, Added: diff.Changes{}} - mockDirectoryValidator.EXPECT().ObjectsExist(bCtx, objTypes[0]).Return(false, nil) - mockDirectoryValidator.EXPECT().ObjectsExist(bCtx, objTypes[1]).Return(false, nil) - mockDirectoryValidator.EXPECT().RelationsExist(bCtx, "folder", relationTypes["folder"][0]).Return(true, nil) - err := dif.Validate(bCtx, mockDirectoryValidator) + mockInstances.EXPECT().ObjectsExist(objTypes[0]).Return(false, nil) + mockInstances.EXPECT().ObjectsExist(objTypes[1]).Return(false, nil) + mockInstances.EXPECT().RelationsExist("folder", relationTypes["folder"][0]).Return(true, nil) + err := dif.Validate(mockInstances) require.Error(t, err) require.Contains(t, err.Error(), derr.ErrRelationTypeInUse.Message) @@ -71,20 +69,19 @@ func TestValidateDiffWithRelationTypeDeletion(t *testing.T) { func TestValidateDiffWithObjectInstances(t *testing.T) { ctrl := gomock.NewController(t) - mockDirectoryValidator := diff.NewMockInstances(ctrl) + mockInstances := diff.NewMockInstances(ctrl) objTypes := []string{"user", "member"} relationTypes := map[string][]string{"folder": {"parent_folder"}} - bCtx := context.Background() dif := diff.Diff{Removed: diff.Changes{Objects: objTypes, Relations: relationTypes}, Added: diff.Changes{}} - mockDirectoryValidator.EXPECT().ObjectsExist(bCtx, objTypes[0]).Return(false, errors.New("Boom!")) - mockDirectoryValidator.EXPECT().ObjectsExist(bCtx, objTypes[1]).Return(true, nil) - mockDirectoryValidator.EXPECT().RelationsExist(bCtx, "folder", relationTypes["folder"][0]).Return(true, nil) - err := dif.Validate(bCtx, mockDirectoryValidator) + mockInstances.EXPECT().ObjectsExist(objTypes[0]).Return(false, ErrBoom) + mockInstances.EXPECT().ObjectsExist(objTypes[1]).Return(true, nil) + mockInstances.EXPECT().RelationsExist("folder", relationTypes["folder"][0]).Return(true, nil) + err := dif.Validate(mockInstances) require.Error(t, err) require.Contains(t, err.Error(), derr.ErrRelationTypeInUse.Message) require.Contains(t, err.Error(), derr.ErrObjectTypeInUse.Message) - require.Contains(t, err.Error(), "Boom!") + require.Contains(t, err.Error(), ErrBoom.Error()) } diff --git a/model/diff/mock_instances.go b/model/diff/mock_instances.go index 29c4abf..ffedf25 100644 --- a/model/diff/mock_instances.go +++ b/model/diff/mock_instances.go @@ -5,7 +5,6 @@ package diff import ( - context "context" gomock "github.com/golang/mock/gomock" reflect "reflect" ) @@ -34,31 +33,31 @@ func (m *MockInstances) EXPECT() *MockInstancesMockRecorder { } // ObjectsExist mocks base method -func (m *MockInstances) ObjectsExist(arg0 context.Context, arg1 string) (bool, error) { +func (m *MockInstances) ObjectsExist(arg0 string) (bool, error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "ObjectsExist", arg0, arg1) + ret := m.ctrl.Call(m, "ObjectsExist", arg0) ret0, _ := ret[0].(bool) ret1, _ := ret[1].(error) return ret0, ret1 } // ObjectsExist indicates an expected call of ObjectsExist -func (mr *MockInstancesMockRecorder) ObjectsExist(arg0, arg1 interface{}) *gomock.Call { +func (mr *MockInstancesMockRecorder) ObjectsExist(arg0 interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ObjectsExist", reflect.TypeOf((*MockInstances)(nil).ObjectsExist), arg0, arg1) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ObjectsExist", reflect.TypeOf((*MockInstances)(nil).ObjectsExist), arg0) } // RelationsExist mocks base method -func (m *MockInstances) RelationsExist(arg0 context.Context, arg1, arg2 string) (bool, error) { +func (m *MockInstances) RelationsExist(arg0, arg1 string) (bool, error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "RelationsExist", arg0, arg1, arg2) + ret := m.ctrl.Call(m, "RelationsExist", arg0, arg1) ret0, _ := ret[0].(bool) ret1, _ := ret[1].(error) return ret0, ret1 } // RelationsExist indicates an expected call of RelationsExist -func (mr *MockInstancesMockRecorder) RelationsExist(arg0, arg1, arg2 interface{}) *gomock.Call { +func (mr *MockInstancesMockRecorder) RelationsExist(arg0, arg1 interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "RelationsExist", reflect.TypeOf((*MockInstances)(nil).RelationsExist), arg0, arg1, arg2) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "RelationsExist", reflect.TypeOf((*MockInstances)(nil).RelationsExist), arg0, arg1) } From b3460b9bd9b9699fd4fb083f33fc831b0529ac00 Mon Sep 17 00:00:00 2001 From: oanatmaria Date: Wed, 25 Oct 2023 15:39:53 +0300 Subject: [PATCH 4/7] Rework Instances interface --- model/diff/diff.go | 70 ++++++++++++++++++++++++++---------- model/diff/diff_test.go | 18 ++++------ model/diff/mock_instances.go | 28 +++++++-------- 3 files changed, 72 insertions(+), 44 deletions(-) diff --git a/model/diff/diff.go b/model/diff/diff.go index 82d0b89..55fec83 100644 --- a/model/diff/diff.go +++ b/model/diff/diff.go @@ -4,6 +4,7 @@ import ( "github.com/aserto-dev/go-directory/pkg/derr" "github.com/hashicorp/go-multierror" "github.com/pkg/errors" + "github.com/samber/lo" ) //go:generate go run github.com/golang/mock/mockgen -destination=mock_instances.go -package=diff github.com/aserto-dev/azm/model/diff Instances @@ -18,50 +19,81 @@ type Changes struct { Relations map[string][]string } +// Only the types of the relation instances are needed. +type RelationKind struct { + Object string + Relation string + Subject string + SubjectRelation string +} + type Instances interface { - ObjectsExist(objectType string) (bool, error) - RelationsExist(objectType, relationName string) (bool, error) + ObjectTypes() ([]string, error) + RelationTypes() ([]*RelationKind, error) } func (d *Diff) Validate(dv Instances) error { var errs error - if err := d.validateObjectTypes(dv); err != nil { + var rels []*RelationKind + if len(d.Removed.Objects) > 0 { + objs, err := dv.ObjectTypes() + if err != nil { + return err + } + + rels, err = dv.RelationTypes() + if err != nil { + return err + } + + err = d.validateObjectTypes(objs, rels) errs = multierror.Append(errs, err) } - if err := d.validateRelationsTypes(dv); err != nil { + if len(d.Removed.Relations) > 0 { + var err error + if len(rels) == 0 { + rels, err = dv.RelationTypes() + if err != nil { + return err + } + + } + err = d.validateRelationsTypes(rels) errs = multierror.Append(errs, err) } - return errs + if merr, ok := errs.(*multierror.Error); ok && len(merr.Errors) > 0 { + return errs + } + + return nil } -func (d *Diff) validateObjectTypes(dv Instances) error { +func (d *Diff) validateObjectTypes(objs []string, rels []*RelationKind) error { var errs error for _, objType := range d.Removed.Objects { - hasInstance, err := dv.ObjectsExist(objType) - if err != nil { - errs = multierror.Append(errs, err) - continue + _, found := lo.Find(objs, func(obj string) bool { return obj == objType }) + if found { + errs = multierror.Append(errs, errors.Wrapf(derr.ErrObjectTypeInUse, "object type: %s", objType)) } - if hasInstance { + _, found = lo.Find(rels, func(rel *RelationKind) bool { return rel.Object == objType || rel.Subject == objType }) + if found { errs = multierror.Append(errs, errors.Wrapf(derr.ErrObjectTypeInUse, "object type: %s", objType)) } } return errs } -func (d *Diff) validateRelationsTypes(dv Instances) error { +func (d *Diff) validateRelationsTypes(relations []*RelationKind) error { var errs error for objType, rels := range d.Removed.Relations { for _, rel := range rels { - hasInstance, err := dv.RelationsExist(objType, rel) - if err != nil { - errs = multierror.Append(errs, err) - continue - } - if hasInstance { - errs = multierror.Append(errs, errors.Wrapf(derr.ErrRelationTypeInUse, "relation type: %s; object type: %s", rel, objType)) + _, found := lo.Find(relations, func(rl *RelationKind) bool { + return rl.Object == objType && rl.Relation == rel + }) + if found { + errs = multierror.Append(errs, errors.Wrapf(derr.ErrRelationTypeInUse, "object type: %s", objType)) } } } diff --git a/model/diff/diff_test.go b/model/diff/diff_test.go index d75d61b..07cda49 100644 --- a/model/diff/diff_test.go +++ b/model/diff/diff_test.go @@ -29,7 +29,8 @@ func TestValidateDiffWithObjectTypeDeletion(t *testing.T) { dif := diff.Diff{Removed: diff.Changes{Objects: []string{objType}}, Added: diff.Changes{}} - mockInstances.EXPECT().ObjectsExist(objType).Return(false, nil) + mockInstances.EXPECT().ObjectTypes().Return([]string{}, nil) + mockInstances.EXPECT().RelationTypes().Return([]*diff.RelationKind{}, nil) err := dif.Validate(mockInstances) require.NoError(t, err) @@ -42,8 +43,8 @@ func TestValidateDiffWith2ObjectTypeDeletion(t *testing.T) { dif := diff.Diff{Removed: diff.Changes{Objects: objTypes}, Added: diff.Changes{}} - mockInstances.EXPECT().ObjectsExist(objTypes[0]).Return(false, nil) - mockInstances.EXPECT().ObjectsExist(objTypes[1]).Return(true, nil) + mockInstances.EXPECT().ObjectTypes().Return([]string{"user"}, nil) + mockInstances.EXPECT().RelationTypes().Return([]*diff.RelationKind{}, nil) err := dif.Validate(mockInstances) require.Error(t, err) @@ -58,9 +59,8 @@ func TestValidateDiffWithRelationTypeDeletion(t *testing.T) { dif := diff.Diff{Removed: diff.Changes{Objects: objTypes, Relations: relationTypes}, Added: diff.Changes{}} - mockInstances.EXPECT().ObjectsExist(objTypes[0]).Return(false, nil) - mockInstances.EXPECT().ObjectsExist(objTypes[1]).Return(false, nil) - mockInstances.EXPECT().RelationsExist("folder", relationTypes["folder"][0]).Return(true, nil) + mockInstances.EXPECT().ObjectTypes().Return([]string{}, nil) + mockInstances.EXPECT().RelationTypes().Return([]*diff.RelationKind{{Object: "folder", Relation: "parent_folder"}}, nil) err := dif.Validate(mockInstances) require.Error(t, err) @@ -75,13 +75,9 @@ func TestValidateDiffWithObjectInstances(t *testing.T) { dif := diff.Diff{Removed: diff.Changes{Objects: objTypes, Relations: relationTypes}, Added: diff.Changes{}} - mockInstances.EXPECT().ObjectsExist(objTypes[0]).Return(false, ErrBoom) - mockInstances.EXPECT().ObjectsExist(objTypes[1]).Return(true, nil) - mockInstances.EXPECT().RelationsExist("folder", relationTypes["folder"][0]).Return(true, nil) + mockInstances.EXPECT().ObjectTypes().Return([]string{}, ErrBoom) err := dif.Validate(mockInstances) require.Error(t, err) - require.Contains(t, err.Error(), derr.ErrRelationTypeInUse.Message) - require.Contains(t, err.Error(), derr.ErrObjectTypeInUse.Message) require.Contains(t, err.Error(), ErrBoom.Error()) } diff --git a/model/diff/mock_instances.go b/model/diff/mock_instances.go index ffedf25..3b50bfa 100644 --- a/model/diff/mock_instances.go +++ b/model/diff/mock_instances.go @@ -32,32 +32,32 @@ func (m *MockInstances) EXPECT() *MockInstancesMockRecorder { return m.recorder } -// ObjectsExist mocks base method -func (m *MockInstances) ObjectsExist(arg0 string) (bool, error) { +// ObjectTypes mocks base method +func (m *MockInstances) ObjectTypes() ([]string, error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "ObjectsExist", arg0) - ret0, _ := ret[0].(bool) + ret := m.ctrl.Call(m, "ObjectTypes") + ret0, _ := ret[0].([]string) ret1, _ := ret[1].(error) return ret0, ret1 } -// ObjectsExist indicates an expected call of ObjectsExist -func (mr *MockInstancesMockRecorder) ObjectsExist(arg0 interface{}) *gomock.Call { +// ObjectTypes indicates an expected call of ObjectTypes +func (mr *MockInstancesMockRecorder) ObjectTypes() *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ObjectsExist", reflect.TypeOf((*MockInstances)(nil).ObjectsExist), arg0) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ObjectTypes", reflect.TypeOf((*MockInstances)(nil).ObjectTypes)) } -// RelationsExist mocks base method -func (m *MockInstances) RelationsExist(arg0, arg1 string) (bool, error) { +// RelationTypes mocks base method +func (m *MockInstances) RelationTypes() ([]*RelationKind, error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "RelationsExist", arg0, arg1) - ret0, _ := ret[0].(bool) + ret := m.ctrl.Call(m, "RelationTypes") + ret0, _ := ret[0].([]*RelationKind) ret1, _ := ret[1].(error) return ret0, ret1 } -// RelationsExist indicates an expected call of RelationsExist -func (mr *MockInstancesMockRecorder) RelationsExist(arg0, arg1 interface{}) *gomock.Call { +// RelationTypes indicates an expected call of RelationTypes +func (mr *MockInstancesMockRecorder) RelationTypes() *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "RelationsExist", reflect.TypeOf((*MockInstances)(nil).RelationsExist), arg0, arg1) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "RelationTypes", reflect.TypeOf((*MockInstances)(nil).RelationTypes)) } From 6b3ea8eb20eb4a2dcaae656c28a3d6bad5eba285 Mon Sep 17 00:00:00 2001 From: oanatmaria Date: Thu, 26 Oct 2023 12:17:59 +0300 Subject: [PATCH 5/7] Improve error message --- model/diff/diff.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/model/diff/diff.go b/model/diff/diff.go index 55fec83..c13d48a 100644 --- a/model/diff/diff.go +++ b/model/diff/diff.go @@ -75,11 +75,11 @@ func (d *Diff) validateObjectTypes(objs []string, rels []*RelationKind) error { for _, objType := range d.Removed.Objects { _, found := lo.Find(objs, func(obj string) bool { return obj == objType }) if found { - errs = multierror.Append(errs, errors.Wrapf(derr.ErrObjectTypeInUse, "object type: %s", objType)) + errs = multierror.Append(errs, errors.Wrapf(derr.ErrObjectTypeInUse, "object type [%s]", objType)) } - _, found = lo.Find(rels, func(rel *RelationKind) bool { return rel.Object == objType || rel.Subject == objType }) + rel, found := lo.Find(rels, func(rel *RelationKind) bool { return rel.Object == objType || rel.Subject == objType }) if found { - errs = multierror.Append(errs, errors.Wrapf(derr.ErrObjectTypeInUse, "object type: %s", objType)) + errs = multierror.Append(errs, errors.Wrapf(derr.ErrRelationTypeInUse, "object type [%s], relation type [%s]", objType, rel.Relation)) } } return errs @@ -93,7 +93,7 @@ func (d *Diff) validateRelationsTypes(relations []*RelationKind) error { return rl.Object == objType && rl.Relation == rel }) if found { - errs = multierror.Append(errs, errors.Wrapf(derr.ErrRelationTypeInUse, "object type: %s", objType)) + errs = multierror.Append(errs, errors.Wrapf(derr.ErrRelationTypeInUse, "object type [%s], relation type [%s]", objType, rel)) } } } From 9881bcef537947b184b7b6f0eb71acadd277da8e Mon Sep 17 00:00:00 2001 From: oanatmaria Date: Thu, 26 Oct 2023 15:54:43 +0300 Subject: [PATCH 6/7] Include SubjectRlation check --- model/diff/diff.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/model/diff/diff.go b/model/diff/diff.go index c13d48a..371b956 100644 --- a/model/diff/diff.go +++ b/model/diff/diff.go @@ -90,7 +90,7 @@ func (d *Diff) validateRelationsTypes(relations []*RelationKind) error { for objType, rels := range d.Removed.Relations { for _, rel := range rels { _, found := lo.Find(relations, func(rl *RelationKind) bool { - return rl.Object == objType && rl.Relation == rel + return (rl.Object == objType && rl.Relation == rel) || (rl.Subject == objType && rl.SubjectRelation == rel) }) if found { errs = multierror.Append(errs, errors.Wrapf(derr.ErrRelationTypeInUse, "object type [%s], relation type [%s]", objType, rel)) From 316525f089f0a8fbbff54a0281adbe551214655a Mon Sep 17 00:00:00 2001 From: oanatmaria Date: Thu, 2 Nov 2023 11:25:43 +0200 Subject: [PATCH 7/7] Fix comments --- model/diff/diff.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/model/diff/diff.go b/model/diff/diff.go index 371b956..b8dcf8e 100644 --- a/model/diff/diff.go +++ b/model/diff/diff.go @@ -73,8 +73,7 @@ func (d *Diff) Validate(dv Instances) error { func (d *Diff) validateObjectTypes(objs []string, rels []*RelationKind) error { var errs error for _, objType := range d.Removed.Objects { - _, found := lo.Find(objs, func(obj string) bool { return obj == objType }) - if found { + if lo.Contains(objs, objType) { errs = multierror.Append(errs, errors.Wrapf(derr.ErrObjectTypeInUse, "object type [%s]", objType)) } rel, found := lo.Find(rels, func(rel *RelationKind) bool { return rel.Object == objType || rel.Subject == objType })