Repository: oom-ai/oomstore (go)

Issue:
Modify API `GetFeature`, `ListFeature`: use full_name instead of feature_name

Pull request:
Jinghan/Replace `FeatureName` by `FeatureFullName` for feature APIs
This PR does:
- use `FeatureFullName` instead of `FeatureName` in metadata store
- refactor feature related APIs (`GetFeatureByName`, `ListFeature`, `UpdateFeature`)



close #810 

Patch:
diff --git a/internal/database/metadata/informer/feature.go b/internal/database/metadata/informer/feature.go
index 90efddb6..9cb4a05e 100644
--- a/internal/database/metadata/informer/feature.go
+++ b/internal/database/metadata/informer/feature.go
@@ -34,11 +34,11 @@ func (c *FeatureCache) List(opt metadata.ListFeatureOpt) types.FeatureList {
 	}
 
 	// filter names
-	if opt.FeatureNames != nil {
+	if opt.FeatureFullNames != nil {
 		var tmp types.FeatureList
-		for _, name := range *opt.FeatureNames {
+		for _, fullName := range *opt.FeatureFullNames {
 			if f := features.Find(func(f *types.Feature) bool {
-				return f.Name == name
+				return f.FullName == fullName
 			}); f != nil {
 				tmp = append(tmp, f)
 			}
diff --git a/internal/database/metadata/mock_metadata/store.go b/internal/database/metadata/mock_metadata/store.go
index 276d592c..2ac445dd 100644
--- a/internal/database/metadata/mock_metadata/store.go
+++ b/internal/database/metadata/mock_metadata/store.go
@@ -172,18 +172,18 @@ func (mr *MockStoreMockRecorder) GetFeature(ctx, id interface{}) *gomock.Call {
 }
 
 // GetFeatureByName mocks base method.
-func (m *MockStore) GetFeatureByName(ctx context.Context, name string) (*types.Feature, error) {
+func (m *MockStore) GetFeatureByName(ctx context.Context, fullName string) (*types.Feature, error) {
 	m.ctrl.T.Helper()
-	ret := m.ctrl.Call(m, "GetFeatureByName", ctx, name)
+	ret := m.ctrl.Call(m, "GetFeatureByName", ctx, fullName)
 	ret0, _ := ret[0].(*types.Feature)
 	ret1, _ := ret[1].(error)
 	return ret0, ret1
 }
 
 // GetFeatureByName indicates an expected call of GetFeatureByName.
-func (mr *MockStoreMockRecorder) GetFeatureByName(ctx, name interface{}) *gomock.Call {
+func (mr *MockStoreMockRecorder) GetFeatureByName(ctx, fullName interface{}) *gomock.Call {
 	mr.mock.ctrl.T.Helper()
-	return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetFeatureByName", reflect.TypeOf((*MockStore)(nil).GetFeatureByName), ctx, name)
+	return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetFeatureByName", reflect.TypeOf((*MockStore)(nil).GetFeatureByName), ctx, fullName)
 }
 
 // GetGroup mocks base method.
@@ -534,18 +534,18 @@ func (mr *MockDBStoreMockRecorder) GetFeature(ctx, id interface{}) *gomock.Call
 }
 
 // GetFeatureByName mocks base method.
-func (m *MockDBStore) GetFeatureByName(ctx context.Context, name string) (*types.Feature, error) {
+func (m *MockDBStore) GetFeatureByName(ctx context.Context, fullName string) (*types.Feature, error) {
 	m.ctrl.T.Helper()
-	ret := m.ctrl.Call(m, "GetFeatureByName", ctx, name)
+	ret := m.ctrl.Call(m, "GetFeatureByName", ctx, fullName)
 	ret0, _ := ret[0].(*types.Feature)
 	ret1, _ := ret[1].(error)
 	return ret0, ret1
 }
 
 // GetFeatureByName indicates an expected call of GetFeatureByName.
-func (mr *MockDBStoreMockRecorder) GetFeatureByName(ctx, name interface{}) *gomock.Call {
+func (mr *MockDBStoreMockRecorder) GetFeatureByName(ctx, fullName interface{}) *gomock.Call {
 	mr.mock.ctrl.T.Helper()
-	return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetFeatureByName", reflect.TypeOf((*MockDBStore)(nil).GetFeatureByName), ctx, name)
+	return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetFeatureByName", reflect.TypeOf((*MockDBStore)(nil).GetFeatureByName), ctx, fullName)
 }
 
 // GetGroup mocks base method.
diff --git a/internal/database/metadata/sqlutil/feature.go b/internal/database/metadata/sqlutil/feature.go
index 895e7a2f..7d8a9e22 100644
--- a/internal/database/metadata/sqlutil/feature.go
+++ b/internal/database/metadata/sqlutil/feature.go
@@ -55,17 +55,17 @@ func GetFeature(ctx context.Context, sqlxCtx metadata.SqlxContext, id int) (*typ
 	return &feature, nil
 }
 
-func GetFeatureByName(ctx context.Context, sqlxCtx metadata.SqlxContext, name string) (*types.Feature, error) {
+func GetFeatureByName(ctx context.Context, sqlxCtx metadata.SqlxContext, fullName string) (*types.Feature, error) {
 	var (
 		feature types.Feature
 		group   *types.Group
 		err     error
 	)
 
-	query := `SELECT * FROM feature WHERE name = ?`
-	if err := sqlxCtx.GetContext(ctx, &feature, sqlxCtx.Rebind(query), name); err != nil {
+	query := `SELECT * FROM feature WHERE full_name = ?`
+	if err := sqlxCtx.GetContext(ctx, &feature, sqlxCtx.Rebind(query), fullName); err != nil {
 		if err == sql.ErrNoRows {
-			return nil, errdefs.NotFound(fmt.Errorf("feature %s not found", name))
+			return nil, errdefs.NotFound(fmt.Errorf("feature %s not found", fullName))
 		}
 		return nil, err
 	}
@@ -134,11 +134,11 @@ func buildListFeatureCond(opt metadata.ListFeatureOpt) ([]string, []interface{},
 		in["id"] = *opt.FeatureIDs
 	}
 
-	if opt.FeatureNames != nil {
-		if len(*opt.FeatureNames) == 0 {
+	if opt.FeatureFullNames != nil {
+		if len(*opt.FeatureFullNames) == 0 {
 			return []string{"false"}, nil, nil
 		}
-		in["name"] = *opt.FeatureNames
+		in["full_name"] = *opt.FeatureFullNames
 	}
 	return dbutil.BuildConditions(and, in)
 }
diff --git a/internal/database/metadata/store.go b/internal/database/metadata/store.go
index b456b3df..88747c09 100644
--- a/internal/database/metadata/store.go
+++ b/internal/database/metadata/store.go
@@ -29,7 +29,7 @@ type DBStore interface {
 	CreateFeature(ctx context.Context, opt CreateFeatureOpt) (int, error)
 	UpdateFeature(ctx context.Context, opt UpdateFeatureOpt) error
 	GetFeature(ctx context.Context, id int) (*types.Feature, error)
-	GetFeatureByName(ctx context.Context, name string) (*types.Feature, error)
+	GetFeatureByName(ctx context.Context, fullName string) (*types.Feature, error)
 	ListFeature(ctx context.Context, opt ListFeatureOpt) (types.FeatureList, error)
 
 	// feature group
diff --git a/internal/database/metadata/types.go b/internal/database/metadata/types.go
index a0914439..0a2c863a 100644
--- a/internal/database/metadata/types.go
+++ b/internal/database/metadata/types.go
@@ -60,8 +60,8 @@ type UpdateRevisionOpt struct {
 }
 
 type ListFeatureOpt struct {
-	EntityID     *int
-	GroupID      *int
-	FeatureIDs   *[]int
-	FeatureNames *[]string
+	EntityID         *int
+	GroupID          *int
+	FeatureIDs       *[]int
+	FeatureFullNames *[]string
 }
diff --git a/oomcli/cmd/edit_feature.go b/oomcli/cmd/edit_feature.go
index 4891c77d..154bd7e4 100644
--- a/oomcli/cmd/edit_feature.go
+++ b/oomcli/cmd/edit_feature.go
@@ -29,7 +29,7 @@ var editFeatureCmd = &cobra.Command{
 		if len(args) > 1 {
 			log.Fatalf("argument at most one, got %d", len(args))
 		} else if len(args) == 1 {
-			editFeatureOpt.FeatureNames = &[]string{args[0]}
+			editFeatureOpt.FeatureFullNames = &[]string{args[0]}
 		}
 
 	},
diff --git a/oomcli/cmd/get_meta_feature.go b/oomcli/cmd/get_meta_feature.go
index 2c15d320..2d37c89b 100644
--- a/oomcli/cmd/get_meta_feature.go
+++ b/oomcli/cmd/get_meta_feature.go
@@ -28,7 +28,7 @@ var getMetaFeatureCmd = &cobra.Command{
 			getMetaFeatureOpt.GroupName = nil
 		}
 		if len(args) == 1 {
-			getMetaFeatureOpt.FeatureNames = &[]string{args[0]}
+			getMetaFeatureOpt.FeatureFullNames = &[]string{args[0]}
 		}
 	},
 	Run: func(cmd *cobra.Command, args []string) {
@@ -61,8 +61,8 @@ func queryFeatures(ctx context.Context, oomStore *oomstore.OomStore, opt types.L
 		return nil, fmt.Errorf("failed getting features, error %v\n", err)
 	}
 
-	if opt.FeatureNames != nil && len(features) == 0 {
-		return nil, fmt.Errorf("feature '%s' not found", (*opt.FeatureNames)[0])
+	if opt.FeatureFullNames != nil && len(features) == 0 {
+		return nil, fmt.Errorf("feature '%s' not found", (*opt.FeatureFullNames)[0])
 	}
 
 	return features, nil
diff --git a/oomcli/cmd/update_feature.go b/oomcli/cmd/update_feature.go
index 6b32fc3c..b83d3ade 100644
--- a/oomcli/cmd/update_feature.go
+++ b/oomcli/cmd/update_feature.go
@@ -15,7 +15,7 @@ var updateFeatureCmd = &cobra.Command{
 	Short: "Update a particular feature",
 	Args:  cobra.ExactArgs(1),
 	PreRun: func(cmd *cobra.Command, args []string) {
-		updateFeatureOpt.FeatureName = args[0]
+		updateFeatureOpt.FeatureFullName = args[0]
 		if !cmd.Flags().Changed("description") {
 			updateFeatureOpt.NewDescription = nil
 		}
@@ -26,7 +26,7 @@ var updateFeatureCmd = &cobra.Command{
 		defer oomStore.Close()
 
 		if err := oomStore.UpdateFeature(ctx, updateFeatureOpt); err != nil {
-			log.Fatalf("failed to update feature %s, err %v\n", updateFeatureOpt.FeatureName, err)
+			log.Fatalf("failed to update feature %s, err %v\n", updateFeatureOpt.FeatureFullName, err)
 		}
 	},
 }
diff --git a/pkg/oomstore/export.go b/pkg/oomstore/export.go
index 50f3d47c..3f8807bf 100644
--- a/pkg/oomstore/export.go
+++ b/pkg/oomstore/export.go
@@ -36,8 +36,12 @@ func (s *OomStore) ChannelExport(ctx context.Context, opt types.ChannelExportOpt
 			GroupName: &revision.Group.Name,
 		})
 	} else {
+		fullNames := make([]string, 0, len(opt.FeatureNames))
+		for _, name := range opt.FeatureNames {
+			fullNames = append(fullNames, fmt.Sprintf("%s.%s", revision.Group.Name, name))
+		}
 		features, err = s.ListFeature(ctx, types.ListFeatureOpt{
-			FeatureNames: &opt.FeatureNames,
+			FeatureFullNames: &fullNames,
 		})
 	}
 	if err != nil {
diff --git a/pkg/oomstore/feature.go b/pkg/oomstore/feature.go
index 1a24adfa..bb019b2b 100644
--- a/pkg/oomstore/feature.go
+++ b/pkg/oomstore/feature.go
@@ -13,15 +13,15 @@ func (s *OomStore) GetFeature(ctx context.Context, id int) (*types.Feature, erro
 	return s.metadata.GetFeature(ctx, id)
 }
 
-// Get metadata of a feature by name.
-func (s *OomStore) GetFeatureByName(ctx context.Context, name string) (*types.Feature, error) {
-	return s.metadata.GetFeatureByName(ctx, name)
+// Get metadata of a feature by full name.
+func (s *OomStore) GetFeatureByName(ctx context.Context, fullName string) (*types.Feature, error) {
+	return s.metadata.GetFeatureByName(ctx, fullName)
 }
 
 // List metadata of features meeting particular criteria.
 func (s *OomStore) ListFeature(ctx context.Context, opt types.ListFeatureOpt) (types.FeatureList, error) {
 	metadataOpt := metadata.ListFeatureOpt{
-		FeatureNames: opt.FeatureNames,
+		FeatureFullNames: opt.FeatureFullNames,
 	}
 	if opt.EntityName != nil {
 		entity, err := s.metadata.GetEntityByName(ctx, *opt.EntityName)
@@ -42,7 +42,7 @@ func (s *OomStore) ListFeature(ctx context.Context, opt types.ListFeatureOpt) (t
 
 // Update metadata of a feature.
 func (s *OomStore) UpdateFeature(ctx context.Context, opt types.UpdateFeatureOpt) error {
-	feature, err := s.metadata.GetFeatureByName(ctx, opt.FeatureName)
+	feature, err := s.metadata.GetFeatureByName(ctx, opt.FeatureFullName)
 	if err != nil {
 		return err
 	}
diff --git a/pkg/oomstore/join.go b/pkg/oomstore/join.go
index 997e7369..a578e1ab 100644
--- a/pkg/oomstore/join.go
+++ b/pkg/oomstore/join.go
@@ -20,7 +20,7 @@ import (
 // Currently, this API only supports batch features.
 func (s *OomStore) ChannelJoin(ctx context.Context, opt types.ChannelJoinOpt) (*types.JoinResult, error) {
 	features, err := s.metadata.ListFeature(ctx, metadata.ListFeatureOpt{
-		FeatureNames: &opt.FeatureNames,
+		FeatureFullNames: &opt.FeatureNames,
 	})
 	if err != nil {
 		return nil, err
diff --git a/pkg/oomstore/online_query.go b/pkg/oomstore/online_query.go
index 58ff4a98..8050df4a 100644
--- a/pkg/oomstore/online_query.go
+++ b/pkg/oomstore/online_query.go
@@ -13,7 +13,7 @@ import (
 // Get online features of a particular entity instance.
 func (s *OomStore) OnlineGet(ctx context.Context, opt types.OnlineGetOpt) (*types.FeatureValues, error) {
 	features := s.metadata.CacheListFeature(ctx, metadata.ListFeatureOpt{
-		FeatureNames: &opt.FeatureNames,
+		FeatureFullNames: &opt.FeatureNames,
 	}).Filter(func(f *types.Feature) bool {
 		return f.Group.OnlineRevisionID != nil
 	})
@@ -58,7 +58,7 @@ func (s *OomStore) OnlineGet(ctx context.Context, opt types.OnlineGetOpt) (*type
 // Get online features of multiple entity instances.
 func (s *OomStore) OnlineMultiGet(ctx context.Context, opt types.OnlineMultiGetOpt) (map[string]*types.FeatureValues, error) {
 	features := s.metadata.CacheListFeature(ctx, metadata.ListFeatureOpt{
-		FeatureNames: &opt.FeatureNames,
+		FeatureFullNames: &opt.FeatureNames,
 	})
 
 	features = features.Filter(func(f *types.Feature) bool {
diff --git a/pkg/oomstore/types/feature.go b/pkg/oomstore/types/feature.go
index e22f44b0..cc2f3c34 100644
--- a/pkg/oomstore/types/feature.go
+++ b/pkg/oomstore/types/feature.go
@@ -60,6 +60,13 @@ func (l *FeatureList) Names() (names []string) {
 	return
 }
 
+func (l *FeatureList) FullNames() (fullNames []string) {
+	for _, f := range *l {
+		fullNames = append(fullNames, f.FullName)
+	}
+	return
+}
+
 func (l *FeatureList) IDs() (ids []int) {
 	for _, f := range *l {
 		ids = append(ids, f.ID)
diff --git a/pkg/oomstore/types/options.go b/pkg/oomstore/types/options.go
index 02092cde..c01a8b3a 100644
--- a/pkg/oomstore/types/options.go
+++ b/pkg/oomstore/types/options.go
@@ -8,14 +8,14 @@ type CreateFeatureOpt struct {
 }
 
 type ListFeatureOpt struct {
-	EntityName   *string
-	GroupName    *string
-	FeatureNames *[]string
+	EntityName       *string
+	GroupName        *string
+	FeatureFullNames *[]string
 }
 
 type UpdateFeatureOpt struct {
-	FeatureName    string
-	NewDescription *string
+	FeatureFullName string
+	NewDescription  *string
 }
 
 type CreateEntityOpt struct {


Test patch:
diff --git a/internal/database/metadata/test_impl/feature.go b/internal/database/metadata/test_impl/feature.go
index 11786ea0..2a9b7c08 100644
--- a/internal/database/metadata/test_impl/feature.go
+++ b/internal/database/metadata/test_impl/feature.go
@@ -146,6 +146,7 @@ func TestGetFeatureByName(t *testing.T, prepareStore PrepareStoreFn, destroyStor
 
 	_, err := store.CreateFeature(ctx, metadata.CreateFeatureOpt{
 		FeatureName: "phone",
+		FullName:    "device_info.phone",
 		GroupID:     groupID,
 		Description: "description",
 		ValueType:   types.String,
@@ -157,11 +158,12 @@ func TestGetFeatureByName(t *testing.T, prepareStore PrepareStoreFn, destroyStor
 	assert.EqualError(t, err, "feature p not found")
 
 	// case 2: correct feature name, return feature `phone`
-	feature, err := store.GetFeatureByName(ctx, "phone")
+	feature, err := store.GetFeatureByName(ctx, "device_info.phone")
 	assert.NoError(t, err)
 	expected := &types.Feature{
 		ID:          1,
 		Name:        "phone",
+		FullName:    "device_info.phone",
 		ValueType:   types.String,
 		Description: "description",
 		GroupID:     1,
@@ -183,6 +185,7 @@ func TestCacheListFeature(t *testing.T, prepareStore PrepareStoreFn, destroyStor
 
 	featureID, err := store.CreateFeature(ctx, metadata.CreateFeatureOpt{
 		FeatureName: "phone",
+		FullName:    "device_info.phone",
 		GroupID:     groupID,
 		Description: "description",
 		ValueType:   types.String,
@@ -226,6 +229,13 @@ func TestCacheListFeature(t *testing.T, prepareStore PrepareStoreFn, destroyStor
 		EntityID: &entityID,
 	})
 	assert.Equal(t, 1, len(features))
+
+	// case 8: list features by FeatureFullNames
+	features, err = store.ListFeature(ctx, metadata.ListFeatureOpt{
+		FeatureFullNames: &[]string{"device_info.phone"},
+	})
+	assert.NoError(t, err)
+	assert.Equal(t, 1, len(features))
 }
 
 func TestListFeature(t *testing.T, prepareStore PrepareStoreFn, destroyStore DestroyStoreFn) {
@@ -242,6 +252,7 @@ func TestListFeature(t *testing.T, prepareStore PrepareStoreFn, destroyStore Des
 
 	featureID, err := store.CreateFeature(ctx, metadata.CreateFeatureOpt{
 		FeatureName: "phone",
+		FullName:    "device_info.phone",
 		GroupID:     groupID,
 		Description: "description",
 		ValueType:   types.String,
@@ -290,6 +301,13 @@ func TestListFeature(t *testing.T, prepareStore PrepareStoreFn, destroyStore Des
 	})
 	assert.NoError(t, err)
 	assert.Equal(t, 1, len(features))
+
+	// case 8: list features by FeatureFullNames
+	features, err = store.ListFeature(ctx, metadata.ListFeatureOpt{
+		FeatureFullNames: &[]string{"device_info.phone"},
+	})
+	assert.NoError(t, err)
+	assert.Equal(t, 1, len(features))
 }
 
 func TestUpdateFeature(t *testing.T, prepareStore PrepareStoreFn, destroyStore DestroyStoreFn) {
diff --git a/oomcli/test/test_get_meta_feature.sh b/oomcli/test/test_get_meta_feature.sh
index 4f240ba2..5f215439 100755
--- a/oomcli/test/test_get_meta_feature.sh
+++ b/oomcli/test/test_get_meta_feature.sh
@@ -31,12 +31,12 @@ expected='
 ID,NAME,GROUP,ENTITY,CATEGORY,VALUE-TYPE,DESCRIPTION,ONLINE-REVISION-ID
 2,model,phone,device,batch,string,model,<NULL>
 '
-actual=$(oomcli get meta feature model -o csv --wide)
+actual=$(oomcli get meta feature phone.model -o csv --wide)
 ignore_time() { cut -d ',' -f 1-8 <<<"$1"; }
 assert_eq "$case" "$(sort <<< "$expected")" "$(ignore_time "$actual" | sort)"
 
 
-case='oomcli get meta feature: one feature'
+case='oomcli get meta feature in yaml: one feature'
 expected='
 kind: Feature
 name: model
@@ -45,7 +45,7 @@ value-type: string
 description: model
 '
 
-actual=$(oomcli get meta feature model -o yaml)
+actual=$(oomcli get meta feature phone.model -o yaml)
 assert_eq "$case" "$expected" "$actual"
 
 case='oomcli get meta feature: multiple features'
diff --git a/pkg/oomstore/join_test.go b/pkg/oomstore/join_test.go
index 11ae70a2..d2194f74 100644
--- a/pkg/oomstore/join_test.go
+++ b/pkg/oomstore/join_test.go
@@ -111,7 +111,7 @@ func TestChannelJoin(t *testing.T) {
 	for _, tc := range testCases {
 		t.Run(tc.description, func(t *testing.T) {
 			metadataStore.EXPECT().Refresh().Return(nil).AnyTimes()
-			metadataStore.EXPECT().ListFeature(gomock.Any(), metadata.ListFeatureOpt{FeatureNames: &tc.opt.FeatureNames}).Return(tc.features, nil)
+			metadataStore.EXPECT().ListFeature(gomock.Any(), metadata.ListFeatureOpt{FeatureFullNames: &tc.opt.FeatureNames}).Return(tc.features, nil)
 			if tc.entity != nil {
 				for _, featureList := range tc.featureMap {
 					metadataStore.EXPECT().ListRevision(gomock.Any(), &featureList[0].GroupID).Return(revisions, nil).AnyTimes()
diff --git a/pkg/oomstore/online_query_test.go b/pkg/oomstore/online_query_test.go
index c5439aed..512f1bac 100644
--- a/pkg/oomstore/online_query_test.go
+++ b/pkg/oomstore/online_query_test.go
@@ -78,7 +78,7 @@ func TestOnlineGet(t *testing.T) {
 
 	for _, tc := range testCases {
 		t.Run(tc.description, func(t *testing.T) {
-			metadataStore.EXPECT().CacheListFeature(gomock.Any(), metadata.ListFeatureOpt{FeatureNames: &tc.opt.FeatureNames}).Return(tc.features)
+			metadataStore.EXPECT().CacheListFeature(gomock.Any(), metadata.ListFeatureOpt{FeatureFullNames: &tc.opt.FeatureNames}).Return(tc.features)
 			if tc.entityName != nil {
 				onlineStore.EXPECT().Get(gomock.Any(), gomock.Any()).Return(dbutil.RowMap{
 					"price": int64(100),
@@ -173,7 +173,7 @@ func TestOnlineMultiGet(t *testing.T) {
 
 	for _, tc := range testCases {
 		t.Run(tc.description, func(t *testing.T) {
-			metadataStore.EXPECT().CacheListFeature(gomock.Any(), metadata.ListFeatureOpt{FeatureNames: &tc.opt.FeatureNames}).Return(tc.features)
+			metadataStore.EXPECT().CacheListFeature(gomock.Any(), metadata.ListFeatureOpt{FeatureFullNames: &tc.opt.FeatureNames}).Return(tc.features)
 			if tc.entityName != nil {
 				onlineStore.EXPECT().MultiGet(gomock.Any(), gomock.Any()).Return(map[string]dbutil.RowMap{
 					"1234": {