Address review comments

Change-Id: I99f87a46cd96d96710aac4532a61361dcc7e2a27
googleapis · Mar 26, 2024 · 36b5da3 · 36b5da3
1 parent 0fb3604
commit 36b5da3
Show file tree

Hide file tree

Showing 4 changed files with 136 additions and 161 deletions.
diff --git a/bigtable/admin.go b/bigtable/admin.go
@@ -2190,26 +2190,25 @@ type AuthorizedViewConf struct {
 	TableID          string
 	AuthorizedViewID string
 
-	AuthorizedViewTypeConf AuthorizedViewTypeConf
-	DeletionProtection     DeletionProtection
+	// Types that are valid to be assigned to AuthorizedView:
+	//  *SubsetViewConf
+	AuthorizedView     isAuthorizedView
+	DeletionProtection DeletionProtection
 }
 
-// AuthorizedViewTypeConf contains information about the type of an authorized view.
-type AuthorizedViewTypeConf struct {
-	AuthorizedViewType AuthorizedViewType
-
-	SubsetView *SubsetViewConf
+// A private interface that currently only implemented by SubsetViewConf, ensuring that only SubsetViewConf instances are accepted as an AuthorizedView.
+// In the future if a new type of AuthorizedView is introduced, it should also implements this interface.
+type isAuthorizedView interface {
+	isAuthorizedView()
 }
 
-// AuthorizedViewType represents the type of an authorized view.
-type AuthorizedViewType int
+func (*SubsetViewConf) isAuthorizedView() {}
 
-const (
-	// AuthorizedViewTypeUnspecified represents an unspecified authorized view type.
-	AuthorizedViewTypeUnspecified AuthorizedViewType = iota
-	// AuthorizedViewTypeSubsetView represents subset view type of an authorized view.
-	AuthorizedViewTypeSubsetView
-)
+// SubsetViewConf contains configuration specific to an authorized view of subset view type.
+type SubsetViewConf struct {
+	RowPrefixes   [][]byte
+	FamilySubsets map[string]FamilySubset
+}
 
 func (av AuthorizedViewConf) proto() *btapb.AuthorizedView {
 	var avp btapb.AuthorizedView
@@ -2223,30 +2222,23 @@ func (av AuthorizedViewConf) proto() *btapb.AuthorizedView {
 		break
 	}
 
-	switch avt := av.AuthorizedViewTypeConf.AuthorizedViewType; avt {
-	case AuthorizedViewTypeSubsetView:
+	switch avt := av.AuthorizedView.(type) {
+	case *SubsetViewConf:
 		avp.AuthorizedView = &btapb.AuthorizedView_SubsetView_{
-			SubsetView: av.AuthorizedViewTypeConf.SubsetView.proto(),
+			SubsetView: avt.proto(),
 		}
 	default:
 		break
 	}
 	return &avp
 }
 
-// SetSubsetView sets the AuthorizedViewType to AuthorizedViewTypeSubsetView and the subsetView pointer accordingly
-func (av *AuthorizedViewTypeConf) SetSubsetView(s SubsetViewConf) error {
-	av.AuthorizedViewType = AuthorizedViewTypeSubsetView
-	av.SubsetView = &s
-	return nil
-}
-
-// GetSubsetView returns an error if the type is not AuthorizedViewTypeSubsetView.
-func (av *AuthorizedViewTypeConf) GetSubsetView() (*SubsetViewConf, error) {
-	if av.AuthorizedViewType != AuthorizedViewTypeSubsetView {
-		return nil, fmt.Errorf("not a subset view: :%v", av.AuthorizedViewType)
+// GetSubsetView returns nil if the type of the AuthorizedView is not SubsetView.
+func (av *AuthorizedViewConf) GetSubsetView() *SubsetViewConf {
+	if sv, ok := av.AuthorizedView.(*SubsetViewConf); ok {
+		return sv
 	}
-	return av.SubsetView, nil
+	return nil
 }
 
 // FamilySubset represents a subset of a column family.
@@ -2255,12 +2247,6 @@ type FamilySubset struct {
 	QualifierPrefixes [][]byte
 }
 
-// SubsetViewConf contains configuration specific to an authorized view of subset view type.
-type SubsetViewConf struct {
-	RowPrefixes   [][]byte
-	FamilySubsets map[string]FamilySubset
-}
-
 // AddRowPrefix adds a new row prefix to the subset view.
 func (s *SubsetViewConf) AddRowPrefix(prefix []byte) {
 	s.RowPrefixes = append(s.RowPrefixes, prefix)
@@ -2324,9 +2310,6 @@ func (ac *AdminClient) CreateAuthorizedView(ctx context.Context, conf *Authorize
 	if conf.TableID == "" || conf.AuthorizedViewID == "" {
 		return errors.New("both AuthorizedViewID and TableID are required")
 	}
-	if conf.AuthorizedViewTypeConf.AuthorizedViewType == AuthorizedViewTypeUnspecified {
-		return errors.New("AuthorizedViewType must be specified")
-	}
 
 	ctx = mergeOutgoingMetadata(ctx, ac.md)
 	req := &btapb.CreateAuthorizedViewRequest{
@@ -2345,7 +2328,6 @@ func (ac *AdminClient) GetAuthorizedView(ctx context.Context, tableID, authorize
 		Name: fmt.Sprintf("%s/tables/%s/authorizedViews/%s", ac.instancePrefix(), tableID, authorizedViewID),
 	}
 	var res *btapb.AuthorizedView
-	av := &AuthorizedViewConf{TableID: tableID, AuthorizedViewID: authorizedViewID}
 
 	err := gax.Invoke(ctx, func(ctx context.Context, _ gax.CallSettings) error {
 		var err error
@@ -2357,6 +2339,7 @@ func (ac *AdminClient) GetAuthorizedView(ctx context.Context, tableID, authorize
 		return nil, err
 	}
 
+	av := &AuthorizedViewConf{TableID: tableID, AuthorizedViewID: authorizedViewID}
 	if res.DeletionProtection {
 		av.DeletionProtection = Protected
 	} else {
@@ -2365,18 +2348,18 @@ func (ac *AdminClient) GetAuthorizedView(ctx context.Context, tableID, authorize
 	if res.GetSubsetView() != nil {
 		s := SubsetViewConf{}
 		s.fillConf(res.GetSubsetView())
-		av.AuthorizedViewTypeConf.SetSubsetView(s)
+		av.AuthorizedView = &s
 	}
 	return av, nil
 }
 
-// AuthorizedViews returns a list of the authorized views in the table. The names returned are fully
-// qualified as projects/<project>/instances/<instance>/tables/<table>/authorizedViews/<authorizedView>
+// AuthorizedViews returns a list of the authorized views in the table.
 func (ac *AdminClient) AuthorizedViews(ctx context.Context, tableID string) ([]string, error) {
 	names := []string{}
+	prefix := fmt.Sprintf("%s/tables/%s", ac.instancePrefix(), tableID)
 
 	req := &btapb.ListAuthorizedViewsRequest{
-		Parent: fmt.Sprintf("%s/tables/%s", ac.instancePrefix(), tableID),
+		Parent: prefix,
 		View:   btapb.AuthorizedView_NAME_ONLY,
 	}
 	var res *btapb.ListAuthorizedViewsResponse
@@ -2390,15 +2373,14 @@ func (ac *AdminClient) AuthorizedViews(ctx context.Context, tableID string) ([]s
 	}
 
 	for _, av := range res.AuthorizedViews {
-		names = append(names, av.Name)
+		names = append(names, strings.TrimPrefix(av.Name, prefix+"/authorizedViews/"))
 	}
 	return names, nil
 }
 
 // UpdateAuthorizedViewConf contains all the information necessary to update or partial update an authorized view.
 type UpdateAuthorizedViewConf struct {
 	AuthorizedViewConf AuthorizedViewConf
-	UpdateMask         []string
 	IgnoreWarnings     bool
 }
 
@@ -2410,9 +2392,19 @@ func (ac *AdminClient) UpdateAuthorizedView(ctx context.Context, conf UpdateAuth
 	}
 	av := conf.AuthorizedViewConf.proto()
 	av.Name = ac.authorizedViewPath(conf.AuthorizedViewConf.TableID, conf.AuthorizedViewConf.AuthorizedViewID)
+
+	updateMask := &field_mask.FieldMask{
+		Paths: []string{},
+	}
+	if conf.AuthorizedViewConf.DeletionProtection != None {
+		updateMask.Paths = append(updateMask.Paths, "deletion_protection")
+	}
+	if sv := conf.AuthorizedViewConf.GetSubsetView(); sv != nil {
+		updateMask.Paths = append(updateMask.Paths, "subset_view")
+	}
 	req := &btapb.UpdateAuthorizedViewRequest{
 		AuthorizedView: av,
-		UpdateMask:     &field_mask.FieldMask{Paths: conf.UpdateMask},
+		UpdateMask:     updateMask,
 		IgnoreWarnings: conf.IgnoreWarnings,
 	}
 	lro, err := ac.tClient.UpdateAuthorizedView(ctx, req)

diff --git a/bigtable/admin_test.go b/bigtable/admin_test.go
@@ -317,12 +317,9 @@ func TestTableAdmin_CreateAuthorizedView_DeletionProtection_Protected(t *testing
 
 	deletionProtection := Protected
 	err = c.CreateAuthorizedView(context.Background(), &AuthorizedViewConf{
-		TableID:          "my-cool-table",
-		AuthorizedViewID: "my-cool-authorized-view",
-		AuthorizedViewTypeConf: AuthorizedViewTypeConf{
-			AuthorizedViewType: AuthorizedViewTypeSubsetView,
-			SubsetView:         &SubsetViewConf{},
-		},
+		TableID:            "my-cool-table",
+		AuthorizedViewID:   "my-cool-authorized-view",
+		AuthorizedView:     &SubsetViewConf{},
 		DeletionProtection: deletionProtection,
 	})
 	if err != nil {
@@ -346,12 +343,9 @@ func TestTableAdmin_CreateAuthorizedView_DeletionProtection_Unprotected(t *testi
 
 	deletionProtection := Unprotected
 	err := c.CreateAuthorizedView(context.Background(), &AuthorizedViewConf{
-		TableID:          "my-cool-table",
-		AuthorizedViewID: "my-cool-authorized-view",
-		AuthorizedViewTypeConf: AuthorizedViewTypeConf{
-			AuthorizedViewType: AuthorizedViewTypeSubsetView,
-			SubsetView:         &SubsetViewConf{},
-		},
+		TableID:            "my-cool-table",
+		AuthorizedViewID:   "my-cool-authorized-view",
+		AuthorizedView:     &SubsetViewConf{},
 		DeletionProtection: deletionProtection,
 	})
 	if err != nil {
@@ -381,7 +375,6 @@ func TestTableAdmin_UpdateAuthorizedViewWithDeletionProtection(t *testing.T) {
 			AuthorizedViewID:   "my-cool-authorized-view",
 			DeletionProtection: deletionProtection,
 		},
-		UpdateMask: []string{"deletion_protection"},
 	})
 	if err != nil {
 		t.Fatalf("UpdateAuthorizedView failed: %v", err)
@@ -401,6 +394,32 @@ func TestTableAdmin_UpdateAuthorizedViewWithDeletionProtection(t *testing.T) {
 	}
 }
 
+func TestTableAdmin_UpdateAuthorizedViewWithSubsetView(t *testing.T) {
+	mock := &mockTableAdminClock{}
+	c := setupTableClient(t, mock)
+
+	err := c.UpdateAuthorizedView(context.Background(), UpdateAuthorizedViewConf{
+		AuthorizedViewConf: AuthorizedViewConf{
+			TableID:          "my-cool-table",
+			AuthorizedViewID: "my-cool-authorized-view",
+			AuthorizedView:   &SubsetViewConf{},
+		},
+	})
+	if err != nil {
+		t.Fatalf("UpdateAuthorizedView failed: %v", err)
+	}
+	updateAuthorizedViewReq := mock.updateAuthorizedViewReq
+	if !cmp.Equal(updateAuthorizedViewReq.AuthorizedView.Name, "projects/my-cool-project/instances/my-cool-instance/tables/my-cool-table/authorizedViews/my-cool-authorized-view") {
+		t.Errorf("UpdateAuthorizedViewRequest does not match: AuthorizedViewName: %v, expected %v", updateAuthorizedViewReq.AuthorizedView.Name, "projects/my-cool-project/instances/my-cool-instance/tables/my-cool-table/authorizedViews/my-cool-authorized-view")
+	}
+	if !cmp.Equal(len(updateAuthorizedViewReq.UpdateMask.Paths), 1) {
+		t.Errorf("UpdateAuthorizedViewRequest does not match: UpdateMask has length of %d, expected %v", len(updateAuthorizedViewReq.UpdateMask.Paths), 1)
+	}
+	if !cmp.Equal(updateAuthorizedViewReq.UpdateMask.Paths[0], "subset_view") {
+		t.Errorf("UpdateAuthorizedViewRequest does not match: updateAuthorizedViewReq.UpdateMask.Paths[0]: %v, expected: %v", updateAuthorizedViewReq.UpdateMask.Paths[0], "subset_view")
+	}
+}
+
 type mockAdminClock struct {
 	btapb.BigtableInstanceAdminClient