Repository: oom-ai/oomstore
Pull request #1151 (2022-02-09 03:26:45)

Jinghan/add `ListGroupOpt` to metadata method `ListGroup`
This PR modifies the metadata method `ListGroup`: add argument `ListGroupOpt`, so that `ListGroup` could be used in more situations.


Patch:
diff --git a/internal/database/metadata/mock_metadata/store.go b/internal/database/metadata/mock_metadata/store.go
index ef0778ee..a8922044 100644
--- a/internal/database/metadata/mock_metadata/store.go
+++ b/internal/database/metadata/mock_metadata/store.go
@@ -291,18 +291,18 @@ func (mr *MockStoreMockRecorder) ListFeature(ctx, opt interface{}) *gomock.Call
 }
 
 // ListGroup mocks base method.
-func (m *MockStore) ListGroup(ctx context.Context, entityID *int, groupIDs *[]int) (types.GroupList, error) {
+func (m *MockStore) ListGroup(ctx context.Context, opt metadata.ListGroupOpt) (types.GroupList, error) {
 	m.ctrl.T.Helper()
-	ret := m.ctrl.Call(m, "ListGroup", ctx, entityID, groupIDs)
+	ret := m.ctrl.Call(m, "ListGroup", ctx, opt)
 	ret0, _ := ret[0].(types.GroupList)
 	ret1, _ := ret[1].(error)
 	return ret0, ret1
 }
 
 // ListGroup indicates an expected call of ListGroup.
-func (mr *MockStoreMockRecorder) ListGroup(ctx, entityID, groupIDs interface{}) *gomock.Call {
+func (mr *MockStoreMockRecorder) ListGroup(ctx, opt interface{}) *gomock.Call {
 	mr.mock.ctrl.T.Helper()
-	return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ListGroup", reflect.TypeOf((*MockStore)(nil).ListGroup), ctx, entityID, groupIDs)
+	return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ListGroup", reflect.TypeOf((*MockStore)(nil).ListGroup), ctx, opt)
 }
 
 // ListRevision mocks base method.
@@ -652,18 +652,18 @@ func (mr *MockDBStoreMockRecorder) ListFeature(ctx, opt interface{}) *gomock.Cal
 }
 
 // ListGroup mocks base method.
-func (m *MockDBStore) ListGroup(ctx context.Context, entityID *int, groupIDs *[]int) (types.GroupList, error) {
+func (m *MockDBStore) ListGroup(ctx context.Context, opt metadata.ListGroupOpt) (types.GroupList, error) {
 	m.ctrl.T.Helper()
-	ret := m.ctrl.Call(m, "ListGroup", ctx, entityID, groupIDs)
+	ret := m.ctrl.Call(m, "ListGroup", ctx, opt)
 	ret0, _ := ret[0].(types.GroupList)
 	ret1, _ := ret[1].(error)
 	return ret0, ret1
 }
 
 // ListGroup indicates an expected call of ListGroup.
-func (mr *MockDBStoreMockRecorder) ListGroup(ctx, entityID, groupIDs interface{}) *gomock.Call {
+func (mr *MockDBStoreMockRecorder) ListGroup(ctx, opt interface{}) *gomock.Call {
 	mr.mock.ctrl.T.Helper()
-	return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ListGroup", reflect.TypeOf((*MockDBStore)(nil).ListGroup), ctx, entityID, groupIDs)
+	return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ListGroup", reflect.TypeOf((*MockDBStore)(nil).ListGroup), ctx, opt)
 }
 
 // ListRevision mocks base method.
diff --git a/internal/database/metadata/mysql/db.go b/internal/database/metadata/mysql/db.go
index 596d3489..16e3c70c 100644
--- a/internal/database/metadata/mysql/db.go
+++ b/internal/database/metadata/mysql/db.go
@@ -100,8 +100,8 @@ func (db *DB) GetGroupByName(ctx context.Context, name string) (*types.Group, er
 	return sqlutil.GetGroupByName(ctx, db, name)
 }
 
-func (db *DB) ListGroup(ctx context.Context, entityID *int, groupIDs *[]int) (types.GroupList, error) {
-	return sqlutil.ListGroup(ctx, db, entityID, groupIDs)
+func (db *DB) ListGroup(ctx context.Context, opt metadata.ListGroupOpt) (types.GroupList, error) {
+	return sqlutil.ListGroup(ctx, db, opt)
 }
 
 func (db *DB) CreateRevision(ctx context.Context, opt metadata.CreateRevisionOpt) (int, error) {
diff --git a/internal/database/metadata/mysql/tx.go b/internal/database/metadata/mysql/tx.go
index dcd0a012..3d2a73e3 100644
--- a/internal/database/metadata/mysql/tx.go
+++ b/internal/database/metadata/mysql/tx.go
@@ -48,8 +48,8 @@ func (tx *Tx) GetGroupByName(ctx context.Context, name string) (*types.Group, er
 	return sqlutil.GetGroupByName(ctx, tx, name)
 }
 
-func (tx *Tx) ListGroup(ctx context.Context, entityID *int, groupIDs *[]int) (types.GroupList, error) {
-	return sqlutil.ListGroup(ctx, tx, entityID, groupIDs)
+func (tx *Tx) ListGroup(ctx context.Context, opt metadata.ListGroupOpt) (types.GroupList, error) {
+	return sqlutil.ListGroup(ctx, tx, opt)
 }
 
 func (tx *Tx) CreateFeature(ctx context.Context, opt metadata.CreateFeatureOpt) (int, error) {
diff --git a/internal/database/metadata/postgres/db.go b/internal/database/metadata/postgres/db.go
index e350e047..35c3020f 100644
--- a/internal/database/metadata/postgres/db.go
+++ b/internal/database/metadata/postgres/db.go
@@ -69,8 +69,8 @@ func (db *DB) GetGroupByName(ctx context.Context, name string) (*types.Group, er
 	return sqlutil.GetGroupByName(ctx, db, name)
 }
 
-func (db *DB) ListGroup(ctx context.Context, entityID *int, groupIDs *[]int) (types.GroupList, error) {
-	return sqlutil.ListGroup(ctx, db, entityID, groupIDs)
+func (db *DB) ListGroup(ctx context.Context, opt metadata.ListGroupOpt) (types.GroupList, error) {
+	return sqlutil.ListGroup(ctx, db, opt)
 }
 
 func (db *DB) CreateFeature(ctx context.Context, opt metadata.CreateFeatureOpt) (int, error) {
diff --git a/internal/database/metadata/postgres/tx.go b/internal/database/metadata/postgres/tx.go
index 2ab96e4f..ccecbf50 100644
--- a/internal/database/metadata/postgres/tx.go
+++ b/internal/database/metadata/postgres/tx.go
@@ -48,8 +48,8 @@ func (tx *Tx) GetGroupByName(ctx context.Context, name string) (*types.Group, er
 	return sqlutil.GetGroupByName(ctx, tx, name)
 }
 
-func (tx *Tx) ListGroup(ctx context.Context, entityID *int, groupIDs *[]int) (types.GroupList, error) {
-	return sqlutil.ListGroup(ctx, tx, entityID, groupIDs)
+func (tx *Tx) ListGroup(ctx context.Context, opt metadata.ListGroupOpt) (types.GroupList, error) {
+	return sqlutil.ListGroup(ctx, tx, opt)
 }
 
 func (tx *Tx) CreateFeature(ctx context.Context, opt metadata.CreateFeatureOpt) (int, error) {
diff --git a/internal/database/metadata/sqlite/db.go b/internal/database/metadata/sqlite/db.go
index e5cd99b6..80b81560 100644
--- a/internal/database/metadata/sqlite/db.go
+++ b/internal/database/metadata/sqlite/db.go
@@ -90,8 +90,8 @@ func (db *DB) GetGroupByName(ctx context.Context, name string) (*types.Group, er
 	return sqlutil.GetGroupByName(ctx, db, name)
 }
 
-func (db *DB) ListGroup(ctx context.Context, entityID *int, groupIDs *[]int) (types.GroupList, error) {
-	return sqlutil.ListGroup(ctx, db, entityID, groupIDs)
+func (db *DB) ListGroup(ctx context.Context, opt metadata.ListGroupOpt) (types.GroupList, error) {
+	return sqlutil.ListGroup(ctx, db, opt)
 }
 
 func (db *DB) CreateRevision(ctx context.Context, opt metadata.CreateRevisionOpt) (int, error) {
diff --git a/internal/database/metadata/sqlite/tx.go b/internal/database/metadata/sqlite/tx.go
index bf30b934..4d777198 100644
--- a/internal/database/metadata/sqlite/tx.go
+++ b/internal/database/metadata/sqlite/tx.go
@@ -48,8 +48,8 @@ func (tx *Tx) GetGroupByName(ctx context.Context, name string) (*types.Group, er
 	return sqlutil.GetGroupByName(ctx, tx, name)
 }
 
-func (tx *Tx) ListGroup(ctx context.Context, entityID *int, groupIDs *[]int) (types.GroupList, error) {
-	return sqlutil.ListGroup(ctx, tx, entityID, groupIDs)
+func (tx *Tx) ListGroup(ctx context.Context, opt metadata.ListGroupOpt) (types.GroupList, error) {
+	return sqlutil.ListGroup(ctx, tx, opt)
 }
 
 func (tx *Tx) CreateFeature(ctx context.Context, opt metadata.CreateFeatureOpt) (int, error) {
diff --git a/internal/database/metadata/sqlutil/feature.go b/internal/database/metadata/sqlutil/feature.go
index da9e9a1a..aa7c5381 100644
--- a/internal/database/metadata/sqlutil/feature.go
+++ b/internal/database/metadata/sqlutil/feature.go
@@ -99,7 +99,9 @@ func ListFeature(ctx context.Context, sqlxCtx metadata.SqlxContext, opt metadata
 
 	// enrich group
 	groupIDs := features.GroupIDs()
-	groups, err := ListGroup(ctx, sqlxCtx, nil, &groupIDs)
+	groups, err := ListGroup(ctx, sqlxCtx, metadata.ListGroupOpt{
+		GroupIDs: &groupIDs,
+	})
 	if err != nil {
 		return nil, err
 	}
diff --git a/internal/database/metadata/sqlutil/group.go b/internal/database/metadata/sqlutil/group.go
index c1c03ad5..734895be 100644
--- a/internal/database/metadata/sqlutil/group.go
+++ b/internal/database/metadata/sqlutil/group.go
@@ -85,8 +85,8 @@ func GetGroupByName(ctx context.Context, sqlxCtx metadata.SqlxContext, name stri
 	return &group, nil
 }
 
-func ListGroup(ctx context.Context, sqlxCtx metadata.SqlxContext, entityID *int, groupIDs *[]int) (types.GroupList, error) {
-	cond, args, err := buildListGroupCond(entityID, groupIDs)
+func ListGroup(ctx context.Context, sqlxCtx metadata.SqlxContext, opt metadata.ListGroupOpt) (types.GroupList, error) {
+	cond, args, err := buildListGroupCond(opt)
 	if err != nil {
 		return nil, errdefs.WithStack(err)
 	}
@@ -107,19 +107,26 @@ func ListGroup(ctx context.Context, sqlxCtx metadata.SqlxContext, entityID *int,
 	return groups, nil
 }
 
-func buildListGroupCond(entityID *int, groupIDs *[]int) (string, []interface{}, error) {
+func buildListGroupCond(opt metadata.ListGroupOpt) (string, []interface{}, error) {
 	args := make([]interface{}, 0)
 	cond := make([]string, 0)
 
-	if entityID != nil {
-		cond = append(cond, "entity_id = ?")
-		args = append(args, *entityID)
+	if opt.EntityIDs != nil {
+		if len(*opt.EntityIDs) == 0 {
+			return "false", args, nil
+		}
+		s, inArgs, err := sqlx.In("entity_id IN (?)", *opt.EntityIDs)
+		if err != nil {
+			return "", nil, errdefs.WithStack(err)
+		}
+		cond = append(cond, s)
+		args = append(args, inArgs...)
 	}
-	if groupIDs != nil {
-		if len(*groupIDs) == 0 {
+	if opt.GroupIDs != nil {
+		if len(*opt.GroupIDs) == 0 {
 			return "false", args, nil
 		}
-		s, inArgs, err := sqlx.In("id IN (?)", *groupIDs)
+		s, inArgs, err := sqlx.In("id IN (?)", *opt.GroupIDs)
 		if err != nil {
 			return "", nil, errdefs.WithStack(err)
 		}
diff --git a/internal/database/metadata/sqlutil/revision.go b/internal/database/metadata/sqlutil/revision.go
index 95760dee..260aa7aa 100644
--- a/internal/database/metadata/sqlutil/revision.go
+++ b/internal/database/metadata/sqlutil/revision.go
@@ -109,7 +109,9 @@ func ListRevision(ctx context.Context, sqlxCtx metadata.SqlxContext, groupID *in
 
 func enrichRevisions(ctx context.Context, sqlxCtx metadata.SqlxContext, revisions types.RevisionList) error {
 	groupIDs := revisions.GroupIDs()
-	groups, err := ListGroup(ctx, sqlxCtx, nil, &groupIDs)
+	groups, err := ListGroup(ctx, sqlxCtx, metadata.ListGroupOpt{
+		GroupIDs: &groupIDs,
+	})
 	if err != nil {
 		return err
 	}
diff --git a/internal/database/metadata/store.go b/internal/database/metadata/store.go
index 3b7dbd3a..01dc4e85 100644
--- a/internal/database/metadata/store.go
+++ b/internal/database/metadata/store.go
@@ -37,7 +37,7 @@ type DBStore interface {
 	UpdateGroup(ctx context.Context, opt UpdateGroupOpt) error
 	GetGroup(ctx context.Context, id int) (*types.Group, error)
 	GetGroupByName(ctx context.Context, name string) (*types.Group, error)
-	ListGroup(ctx context.Context, entityID *int, groupIDs *[]int) (types.GroupList, error)
+	ListGroup(ctx context.Context, opt ListGroupOpt) (types.GroupList, error)
 
 	// revision
 	CreateRevision(ctx context.Context, opt CreateRevisionOpt) (int, error)
diff --git a/internal/database/metadata/types.go b/internal/database/metadata/types.go
index c9ab48e7..5a51cd0c 100644
--- a/internal/database/metadata/types.go
+++ b/internal/database/metadata/types.go
@@ -66,6 +66,11 @@ type ListFeatureOpt struct {
 	FeatureIDs *[]int
 }
 
+type ListGroupOpt struct {
+	EntityIDs *[]int
+	GroupIDs  *[]int
+}
+
 type ListCachedFeatureOpt struct {
 	FullNames *[]string
 	GroupName *string
diff --git a/pkg/oomstore/group.go b/pkg/oomstore/group.go
index 13f04467..769b791f 100644
--- a/pkg/oomstore/group.go
+++ b/pkg/oomstore/group.go
@@ -40,7 +40,11 @@ func (s *OomStore) GetGroupByName(ctx context.Context, name string) (*types.Grou
 
 // ListGroup lists metadata of feature groups under the same entity.
 func (s *OomStore) ListGroup(ctx context.Context, entityID *int) (types.GroupList, error) {
-	return s.metadata.ListGroup(ctx, entityID, nil)
+	opt := metadata.ListGroupOpt{}
+	if entityID != nil {
+		opt.EntityIDs = &[]int{*entityID}
+	}
+	return s.metadata.ListGroup(ctx, opt)
 }
 
 // UpdateGroup updates metadata of a feature group.
diff --git a/pkg/oomstore/push.go b/pkg/oomstore/push.go
index 8b5cec70..5c0abed3 100644
--- a/pkg/oomstore/push.go
+++ b/pkg/oomstore/push.go
@@ -17,17 +17,13 @@ func (s *OomStore) Push(ctx context.Context, opt types.PushOpt) error {
 	features := s.metadata.ListCachedFeature(ctx, metadata.ListCachedFeatureOpt{
 		GroupName: &opt.GroupName,
 	})
-	if len(features) == 0 {
-		return errdefs.Errorf("Feature group is empty or does not exist")
-	}
-	entity := features[0].Entity()
-	group := features[0].Group
-
 	var featureNames []string
 	for name := range opt.FeatureValues {
 		featureNames = append(featureNames, name)
 	}
 
+	entity := features[0].Entity()
+	group := features[0].Group
 	if group.Category != types.CategoryStream {
 		return errdefs.Errorf("Push API is for streaming features only")
 	}


Test patch:
diff --git a/internal/database/metadata/test_impl/group.go b/internal/database/metadata/test_impl/group.go
index a2e3b3f1..2dff760c 100644
--- a/internal/database/metadata/test_impl/group.go
+++ b/internal/database/metadata/test_impl/group.go
@@ -95,34 +95,62 @@ func TestListGroup(t *testing.T, prepareStore PrepareStoreFn, destroyStore Destr
 	_, err = store.CreateGroup(ctx, userBehaviorOpt)
 	require.NoError(t, err)
 
-	groups, err := store.ListGroup(ctx, nil, &[]int{})
+	groups, err := store.ListGroup(ctx, metadata.ListGroupOpt{
+		EntityIDs: nil,
+		GroupIDs:  &[]int{},
+	})
 	assert.NoError(t, err)
 	assert.Equal(t, 0, len(groups))
 
-	groups, err = store.ListGroup(ctx, &deviceGroupID, nil)
+	groups, err = store.ListGroup(ctx, metadata.ListGroupOpt{
+		EntityIDs: &[]int{deviceGroupID},
+		GroupIDs:  nil,
+	})
 	assert.NoError(t, err)
 	assert.Equal(t, 1, len(groups))
 
-	groups, err = store.ListGroup(ctx, &userGroupID, nil)
+	groups, err = store.ListGroup(ctx, metadata.ListGroupOpt{
+		EntityIDs: &[]int{userGroupID},
+		GroupIDs:  nil,
+	})
 	assert.NoError(t, err)
 	assert.Equal(t, 2, len(groups))
 
-	groups, err = store.ListGroup(ctx, nil, nil)
+	groups, err = store.ListGroup(ctx, metadata.ListGroupOpt{
+		EntityIDs: nil,
+		GroupIDs:  nil,
+	})
 	assert.NoError(t, err)
 	assert.Equal(t, 3, len(groups))
 
-	groups, err = store.ListGroup(ctx, intPtr(0), nil)
+	groups, err = store.ListGroup(ctx, metadata.ListGroupOpt{
+		EntityIDs: &[]int{0},
+		GroupIDs:  nil,
+	})
 	assert.NoError(t, err)
 	assert.Equal(t, 0, len(groups))
 
 	ids := []int{deviceGroupID, userGroupID}
-	groups, err = store.ListGroup(ctx, nil, &ids)
+	groups, err = store.ListGroup(ctx, metadata.ListGroupOpt{
+		EntityIDs: nil,
+		GroupIDs:  &ids,
+	})
 	assert.NoError(t, err)
 	assert.Equal(t, 2, len(groups))
 
-	groups, err = store.ListGroup(ctx, &userEntityID, &ids)
+	groups, err = store.ListGroup(ctx, metadata.ListGroupOpt{
+		EntityIDs: &[]int{userEntityID},
+		GroupIDs:  &ids,
+	})
 	assert.NoError(t, err)
 	assert.Equal(t, 1, len(groups))
+
+	groups, err = store.ListGroup(ctx, metadata.ListGroupOpt{
+		EntityIDs: &[]int{userEntityID, deviceEntityID},
+		GroupIDs:  nil,
+	})
+	assert.NoError(t, err)
+	assert.Equal(t, 3, len(groups))
 }
 
 func TestCreateGroup(t *testing.T, prepareStore PrepareStoreFn, destroyStore DestroyStoreFn) {
diff --git a/oomcli/test/test_push.sh b/oomcli/test/test_push.sh
index 4d30e519..6e486f70 100755
--- a/oomcli/test/test_push.sh
+++ b/oomcli/test/test_push.sh
@@ -23,7 +23,3 @@ user,user-click.last_5_click_posts,user-click.number_of_user_starred_posts
 oomcli push --entity-key 1 --group user-click --feature last_5_click_posts=2,3,4,5,6 --feature number_of_user_starred_posts=11
 actual=$(oomcli get online --feature user-click.last_5_click_posts,user-click.number_of_user_starred_posts -k 1 -o csv)
 assert_eq "$case" "$expected" "$actual"
-
-case="push stream feature with non-existent group name"
-actual=$(oomcli push --entity-key 1 --group non-existent-group --feature f1=foo --feature f2=bar 2>&1 || true)
-assert_contains "$case" "group is empty or does not exist" "$actual"
diff --git a/oomcli/test/util.sh b/oomcli/test/util.sh
index 2e9ba982..daacfe47 100644
--- a/oomcli/test/util.sh
+++ b/oomcli/test/util.sh
@@ -50,27 +50,6 @@ assert_eq() {
   fi
 }
 
-assert_contains() {
-  local case expected actual
-  case="case - $1"
-  expected="$(trim "$2")"
-  actual="$(trim "$3")"
-
-  if [[ "$actual" == *"$expected"* ]]; then
-      info "${BLD}${GRN}Passed $case${RST}"
-      return 0
-  else
-      erro "${BLD}${GRN}Failed $case${RST}"
-      echo "${BLD}${YLW}=> expected contains:${RST}"
-      echo "$expected"
-      echo "${BLD}${YLW}=> actual:${RST}"
-      echo "$actual"
-      echo "${BLD}${YLW}=> diff:${RST}"
-      diff --color=auto <(echo "$expected" ) <(echo "$actual")
-      return 1
-  fi
-}
-
 # register features for the sample data
 register_features() {
     oomcli register entity device --description "device"