Simplify branch protect interface (#6688)

pkg/api/controller.go

@@ -1821,11 +1821,7 @@ func (c *Controller) SetBranchProtectionRules(w http.ResponseWriter, r *http.Req
 			Value: blockedActions,
 		}
 	}
-	eTag := params.IfMatch
-	if swag.StringValue(eTag) == "" {
-		eTag = nil
-	}
-	err := c.Catalog.SetBranchProtectionRules(ctx, repository, rules, eTag)
+	err := c.Catalog.SetBranchProtectionRules(ctx, repository, rules, params.IfMatch)
 	if c.handleAPIError(ctx, w, r, err) {
 		return
 	}
@@ -3092,7 +3088,7 @@ func (c *Controller) InternalDeleteBranchProtectionRule(w http.ResponseWriter, r
 	for p := range rules.BranchPatternToBlockedActions {
 		if p == body.Pattern {
 			delete(rules.BranchPatternToBlockedActions, p)
-			err = c.Catalog.SetBranchProtectionRules(ctx, repository, rules, swag.String(graveler.BranchProtectionSkipValidationChecksum))
+			err = c.Catalog.SetBranchProtectionRules(ctx, repository, rules, nil)
 			if c.handleAPIError(ctx, w, r, err) {
 				return
 			}
@@ -3146,7 +3142,7 @@ func (c *Controller) InternalCreateBranchProtectionRule(w http.ResponseWriter, r
 		Value: []graveler.BranchProtectionBlockedAction{graveler.BranchProtectionBlockedAction_STAGING_WRITE, graveler.BranchProtectionBlockedAction_COMMIT},
 	}
 	rules.BranchPatternToBlockedActions[body.Pattern] = blockedActions
-	err = c.Catalog.SetBranchProtectionRules(ctx, repository, rules, swag.String(graveler.BranchProtectionSkipValidationChecksum))
+	err = c.Catalog.SetBranchProtectionRules(ctx, repository, rules, nil)
 	if c.handleAPIError(ctx, w, r, err) {
 		return
 	}

pkg/catalog/interface.go

@@ -167,9 +167,9 @@ type Interface interface {
 	// The returned checksum represents the current state of the rules, and can be passed to SetBranchProtectionRules for conditional updates.
 	GetBranchProtectionRules(ctx context.Context, repositoryID string) (*graveler.BranchProtectionRules, *string, error)
 	// SetBranchProtectionRules sets the branch protection rules for the given repository.
-	// If lastKnownChecksum doesn't match the current state, the update will fail with ErrPreconditionFailed.
-	// If lastKnownChecksum is nil, the update is performed only if no rules exist.
-	// If lastKnownChecksum is equal to BranchProtectionSkipValidationChecksum, the update is always performed.
+	// If lastKnownChecksum doesn't match the current state, the update fails with ErrPreconditionFailed.
+	// If lastKnownChecksum is the empty string, the update is performed only if no rules exist.
+	// If lastKnownChecksum is nil, the update is performed unconditionally.
 	SetBranchProtectionRules(ctx context.Context, repositoryID string, rules *graveler.BranchProtectionRules, lastKnownChecksum *string) error
 
 	// SetLinkAddress to validate single use limited in time of a given physical address

pkg/graveler/branch/protection_manager.go

@@ -3,15 +3,12 @@ package branch
 import (
 	"context"
 	"errors"
-	"fmt"
 	"time"
 
 	"github.com/gobwas/glob"
 	"github.com/treeverse/lakefs/pkg/cache"
 	"github.com/treeverse/lakefs/pkg/graveler"
 	"github.com/treeverse/lakefs/pkg/graveler/settings"
-	"github.com/treeverse/lakefs/pkg/kv"
-	"google.golang.org/protobuf/proto"
 )
 
 const ProtectionSettingKey = "protected_branches"
@@ -22,10 +19,6 @@ const (
 	matcherCacheJitter = 1 * time.Minute
 )
 
-var (
-	ErrRuleNotExists = fmt.Errorf("branch protection rule does not exist: %w", graveler.ErrNotFound)
-)
-
 type ProtectionManager struct {
 	settingManager *settings.Manager
 	matchers       cache.Cache
@@ -36,39 +29,28 @@ func NewProtectionManager(settingManager *settings.Manager) *ProtectionManager {
 }
 
 func (m *ProtectionManager) GetRules(ctx context.Context, repository *graveler.RepositoryRecord) (*graveler.BranchProtectionRules, *string, error) {
-	rulesMsg, checksum, err := m.settingManager.GetLatest(ctx, repository, ProtectionSettingKey, &graveler.BranchProtectionRules{})
-	if errors.Is(err, graveler.ErrNotFound) {
-		return &graveler.BranchProtectionRules{}, nil, nil
-	}
+	rulesMsg := &graveler.BranchProtectionRules{}
+	checksum, err := m.settingManager.GetLatest(ctx, repository, ProtectionSettingKey, rulesMsg)
 	if err != nil {
 		return nil, nil, err
 	}
-	if proto.Size(rulesMsg) == 0 {
-		return &graveler.BranchProtectionRules{}, nil, nil
-	}
-	return rulesMsg.(*graveler.BranchProtectionRules), checksum, nil
-}
-func (m *ProtectionManager) SetRules(ctx context.Context, repository *graveler.RepositoryRecord, rules *graveler.BranchProtectionRules) error {
-	return m.settingManager.Save(ctx, repository, ProtectionSettingKey, rules)
+	return rulesMsg, checksum, nil
 }
 
-func (m *ProtectionManager) SetRulesIf(ctx context.Context, repository *graveler.RepositoryRecord, rules *graveler.BranchProtectionRules, lastKnownChecksum *string) error {
-	err := m.settingManager.SaveIf(ctx, repository, ProtectionSettingKey, rules, lastKnownChecksum)
-	if errors.Is(err, kv.ErrPredicateFailed) {
-		return graveler.ErrPreconditionFailed
-	}
-	return err
+func (m *ProtectionManager) SetRules(ctx context.Context, repository *graveler.RepositoryRecord, rules *graveler.BranchProtectionRules, lastKnownChecksum *string) error {
+	return m.settingManager.Save(ctx, repository, ProtectionSettingKey, rules, lastKnownChecksum)
 }
 
 func (m *ProtectionManager) IsBlocked(ctx context.Context, repository *graveler.RepositoryRecord, branchID graveler.BranchID, action graveler.BranchProtectionBlockedAction) (bool, error) {
-	rules, err := m.settingManager.Get(ctx, repository, ProtectionSettingKey, &graveler.BranchProtectionRules{})
+	rules := &graveler.BranchProtectionRules{}
+	err := m.settingManager.Get(ctx, repository, ProtectionSettingKey, rules)
 	if errors.Is(err, graveler.ErrNotFound) {
 		return false, nil
 	}
 	if err != nil {
 		return false, err
 	}
-	for pattern, blockedActions := range rules.(*graveler.BranchProtectionRules).BranchPatternToBlockedActions {
+	for pattern, blockedActions := range rules.BranchPatternToBlockedActions {
 		pattern := pattern
 		matcher, err := m.matchers.GetOrSet(pattern, func() (v interface{}, err error) {
 			return glob.Compile(pattern)

pkg/graveler/branch/protection_manager_test.go

@@ -9,6 +9,7 @@ import (
 	"github.com/go-openapi/swag"
 	"github.com/go-test/deep"
 	"github.com/golang/mock/gomock"
+	"github.com/stretchr/testify/require"
 	"github.com/treeverse/lakefs/pkg/graveler"
 	"github.com/treeverse/lakefs/pkg/graveler/branch"
 	"github.com/treeverse/lakefs/pkg/graveler/mock"
@@ -29,11 +30,8 @@ func TestSetAndGet(t *testing.T) {
 	ctx := context.Background()
 	bpm := prepareTest(t, ctx)
 	rules, eTag, err := bpm.GetRules(ctx, repository)
-	testutil.Must(t, err)
-	if len(rules.BranchPatternToBlockedActions) != 0 {
-		t.Fatalf("expected no rules, got %d rules", len(rules.BranchPatternToBlockedActions))
-	}
-	testutil.Must(t, bpm.SetRulesIf(ctx, repository, &graveler.BranchProtectionRules{
+	require.ErrorIs(t, err, graveler.ErrNotFound)
+	testutil.Must(t, bpm.SetRules(ctx, repository, &graveler.BranchProtectionRules{
 		BranchPatternToBlockedActions: map[string]*graveler.BranchProtectionBlockedActions{
 			"main*": {Value: []graveler.BranchProtectionBlockedAction{
 				graveler.BranchProtectionBlockedAction_STAGING_WRITE},
@@ -57,7 +55,7 @@ func TestSetAndGet(t *testing.T) {
 func TestSetWrongETag(t *testing.T) {
 	ctx := context.Background()
 	bpm := prepareTest(t, ctx)
-	err := bpm.SetRulesIf(ctx, repository, &graveler.BranchProtectionRules{
+	err := bpm.SetRules(ctx, repository, &graveler.BranchProtectionRules{
 		BranchPatternToBlockedActions: map[string]*graveler.BranchProtectionBlockedActions{
 			"main*": {Value: []graveler.BranchProtectionBlockedAction{
 				graveler.BranchProtectionBlockedAction_STAGING_WRITE},
@@ -106,7 +104,7 @@ func TestIsBlocked(t *testing.T) {
 			bpm := prepareTest(t, ctx)
 			testutil.Must(t, bpm.SetRules(ctx, repository, &graveler.BranchProtectionRules{
 				BranchPatternToBlockedActions: tst.patternToBlockedActions,
-			}))
+			}, nil))
 
 			for branchID, expectedBlockedActions := range tst.expectedBlockedActions {
 				for _, action := range expectedBlockedActions.Value {

pkg/graveler/graveler.go

@@ -9,7 +9,6 @@ import (
 	"time"
 
 	"github.com/cenkalti/backoff/v4"
-	"github.com/go-openapi/swag"
 	"github.com/google/uuid"
 	"github.com/hashicorp/go-multierror"
 	"github.com/prometheus/client_golang/prometheus"
@@ -46,8 +45,6 @@ const (
 	RepoMetadataUpdateRandomFactor   = 0.5
 )
 
-const BranchProtectionSkipValidationChecksum = "skip-checksum-validation"
-
 // Basic Types
 
 // DiffType represents the type of the change
@@ -618,8 +615,8 @@ type VersionController interface {
 
 	// SetBranchProtectionRules sets the branch protection rules for the repository.
 	// If lastKnownChecksum doesn't match the current state, the update fails with ErrPreconditionFailed.
-	// If lastKnownChecksum is nil, the update is performed only if no rules exist.
-	// If lastKnownChecksum is equal to BranchProtectionSkipValidationChecksum, the update is always performed.
+	// If lastKnownChecksum is the empty string, the update is performed only if no rules exist.
+	// If lastKnownChecksum is nil, the update is performed unconditionally.
 	SetBranchProtectionRules(ctx context.Context, repository *RepositoryRecord, rules *BranchProtectionRules, lastKnownChecksum *string) error
 
 	// SetLinkAddress saves the address for linking under the repository.
@@ -1502,11 +1499,7 @@ func (g *Graveler) GetBranchProtectionRules(ctx context.Context, repository *Rep
 }
 
 func (g *Graveler) SetBranchProtectionRules(ctx context.Context, repository *RepositoryRecord, rules *BranchProtectionRules, lastKnownChecksum *string) error {
-	if swag.StringValue(lastKnownChecksum) == BranchProtectionSkipValidationChecksum {
-		// TODO(johnnyaug): remove this logic and constant once the legacy API is dropped.
-		return g.protectedBranchesManager.SetRules(ctx, repository, rules)
-	}
-	return g.protectedBranchesManager.SetRulesIf(ctx, repository, rules, lastKnownChecksum)
+	return g.protectedBranchesManager.SetRules(ctx, repository, rules, lastKnownChecksum)
 }
 
 // getFromStagingArea returns the most updated value of a given key in a branch staging area.
@@ -3243,11 +3236,11 @@ type ProtectedBranchesManager interface {
 	// GetRules returns all branch protection rules for the repository.
 	// The returned checksum represents the current state of the rules, and can be passed to SetRulesIf for conditional updates.
 	GetRules(ctx context.Context, repository *RepositoryRecord) (*BranchProtectionRules, *string, error)
-	SetRules(ctx context.Context, repository *RepositoryRecord, rules *BranchProtectionRules) error
-	// SetRulesIf sets the branch protection rules for the repository.
+	// SetRules sets the branch protection rules for the repository.
 	// If lastKnownChecksum does not match the current checksum, returns ErrPreconditionFailed.
-	// If lastKnownChecksum is nil, the rules are set only if no rules are currently set.
-	SetRulesIf(ctx context.Context, repository *RepositoryRecord, rules *BranchProtectionRules, lastKnownChecksum *string) error
+	// If lastKnownChecksum is the empty string, the rules are set only if they are not currently set.
+	// If lastKnownChecksum is nil, the rules are set unconditionally.
+	SetRules(ctx context.Context, repository *RepositoryRecord, rules *BranchProtectionRules, lastKnownChecksum *string) error
 	// IsBlocked returns whether the action is blocked by any branch protection rule matching the given branch.
 	IsBlocked(ctx context.Context, repository *RepositoryRecord, branchID BranchID, action BranchProtectionBlockedAction) (bool, error)
 }