feat: [CODE-2339]: Send sha in delete branch API and fix dry run rules behaviour (#2672)

* go lint issues and added return in the Deletebranch method
* removed the comment
* fixed go lint issues
* fixed query param in swagger
* Merge branch 'main' of https://git0.harness.io/l7B_kbSEQD2wjrM7PShm5w/PROD/Harness_Commons/gitness into ks/CODE-2339
* feat: [CODE-2339]: Send sha in delete branch API and fix dry run rules behaviour

app/api/controller/pullreq/comment_apply_suggestions.go

@@ -84,9 +84,7 @@ func (i *CommentApplySuggestionsInput) sanitize() error {
 
 type CommentApplySuggestionsOutput struct {
 	CommitID string `json:"commit_id"`
-
-	DryRunRules    bool                   `json:"dry_run_rules,omitempty"`
-	RuleViolations []types.RuleViolations `json:"rule_violations,omitempty"`
+	types.DryRunRulesOutput
 }
 
 // CommentApplySuggestions applies suggestions for code comments.
@@ -138,8 +136,10 @@ func (c *Controller) CommentApplySuggestions(
 
 	if in.DryRunRules {
 		return CommentApplySuggestionsOutput{
-			DryRunRules:    true,
-			RuleViolations: violations,
+			DryRunRulesOutput: types.DryRunRulesOutput{
+				DryRunRules:    true,
+				RuleViolations: violations,
+			},
 		}, nil, nil
 	}
 
@@ -386,7 +386,9 @@ func (c *Controller) CommentApplySuggestions(
 	}
 
 	return CommentApplySuggestionsOutput{
-		CommitID:       commitOut.CommitID.String(),
-		RuleViolations: violations,
+		CommitID: commitOut.CommitID.String(),
+		DryRunRulesOutput: types.DryRunRulesOutput{
+			RuleViolations: violations,
+		},
 	}, nil, nil
 }

app/api/controller/repo/commit.go

@@ -97,8 +97,10 @@ func (c *Controller) CommitFiles(ctx context.Context,
 
 	if in.DryRunRules {
 		return types.CommitFilesResponse{
-			DryRunRules:    true,
-			RuleViolations: violations,
+			DryRunRulesOutput: types.DryRunRulesOutput{
+				DryRunRules:    true,
+				RuleViolations: violations,
+			},
 		}, nil, nil
 	}
 
@@ -154,7 +156,9 @@ func (c *Controller) CommitFiles(ctx context.Context,
 	}
 
 	return types.CommitFilesResponse{
-		CommitID:       commit.CommitID.String(),
-		RuleViolations: violations,
+		CommitID: commit.CommitID.String(),
+		DryRunRulesOutput: types.DryRunRulesOutput{
+			RuleViolations: violations,
+		},
 	}, nil, nil
 }

app/api/controller/repo/create_branch.go

@@ -45,7 +45,7 @@ func (c *Controller) CreateBranch(ctx context.Context,
 	session *auth.Session,
 	repoRef string,
 	in *CreateBranchInput,
-) (*Branch, []types.RuleViolations, error) {
+) (*types.Branch, []types.RuleViolations, error) {
 	repo, err := c.getRepoCheckAccess(ctx, session, repoRef, enum.PermissionRepoPush)
 	if err != nil {
 		return nil, nil, err

app/api/controller/repo/delete_branch.go

@@ -34,10 +34,11 @@ func (c *Controller) DeleteBranch(ctx context.Context,
 	branchName string,
 	bypassRules bool,
 	dryRunRules bool,
-) ([]types.RuleViolations, error) {
+	commitSha string,
+) (types.DeleteBranchOutput, []types.RuleViolations, error) {
 	repo, err := c.getRepoCheckAccess(ctx, session, repoRef, enum.PermissionRepoPush)
 	if err != nil {
-		return nil, err
+		return types.DeleteBranchOutput{}, nil, err
 	}
 
 	// make sure user isn't deleting the default branch
@@ -45,12 +46,12 @@ func (c *Controller) DeleteBranch(ctx context.Context,
 	// and 'refs/heads/branch1' would fail if 'branch1' exists.
 	// TODO: Add functional test to ensure the scenario is covered!
 	if branchName == repo.DefaultBranch {
-		return nil, usererror.ErrDefaultBranchCantBeDeleted
+		return types.DeleteBranchOutput{}, nil, usererror.ErrDefaultBranchCantBeDeleted
 	}
 
 	rules, isRepoOwner, err := c.fetchRules(ctx, session, repo)
 	if err != nil {
-		return nil, err
+		return types.DeleteBranchOutput{}, nil, err
 	}
 
 	violations, err := rules.RefChangeVerify(ctx, protection.RefChangeVerifyInput{
@@ -63,28 +64,38 @@ func (c *Controller) DeleteBranch(ctx context.Context,
 		RefNames:    []string{branchName},
 	})
 	if err != nil {
-		return nil, fmt.Errorf("failed to verify protection rules: %w", err)
-	}
-	if protection.IsCritical(violations) {
-		return violations, nil
+		return types.DeleteBranchOutput{}, nil, fmt.Errorf("failed to verify protection rules: %w", err)
 	}
 
 	if dryRunRules {
-		return []types.RuleViolations{}, nil
+		return types.DeleteBranchOutput{
+			DryRunRulesOutput: types.DryRunRulesOutput{
+				DryRunRules:    true,
+				RuleViolations: violations,
+			},
+		}, nil, nil
+	}
+
+	if protection.IsCritical(violations) {
+		return types.DeleteBranchOutput{}, violations, nil
 	}
 
 	writeParams, err := controller.CreateRPCInternalWriteParams(ctx, c.urlProvider, session, repo)
 	if err != nil {
-		return nil, fmt.Errorf("failed to create RPC write params: %w", err)
+		return types.DeleteBranchOutput{}, nil, fmt.Errorf("failed to create RPC write params: %w", err)
 	}
 
 	err = c.git.DeleteBranch(ctx, &git.DeleteBranchParams{
 		WriteParams: writeParams,
 		BranchName:  branchName,
+		SHA:         commitSha,
 	})
 	if err != nil {
-		return nil, err
+		return types.DeleteBranchOutput{}, nil, err
 	}
 
-	return nil, nil
+	return types.DeleteBranchOutput{
+		DryRunRulesOutput: types.DryRunRulesOutput{
+			RuleViolations: violations,
+		}}, nil, nil
 }

app/api/controller/repo/get_branch.go

@@ -20,6 +20,7 @@ import (
 
 	"github.com/harness/gitness/app/auth"
 	"github.com/harness/gitness/git"
+	"github.com/harness/gitness/types"
 	"github.com/harness/gitness/types/enum"
 )
 
@@ -28,7 +29,7 @@ func (c *Controller) GetBranch(ctx context.Context,
 	session *auth.Session,
 	repoRef string,
 	branchName string,
-) (*Branch, error) {
+) (*types.Branch, error) {
 	repo, err := c.getRepoCheckAccess(ctx, session, repoRef, enum.PermissionRepoView)
 	if err != nil {
 		return nil, err

app/api/controller/repo/list_branches.go

@@ -25,19 +25,13 @@ import (
 	"github.com/harness/gitness/types/enum"
 )
 
-type Branch struct {
-	Name   string        `json:"name"`
-	SHA    string        `json:"sha"`
-	Commit *types.Commit `json:"commit,omitempty"`
-}
-
 // ListBranches lists the branches of a repo.
 func (c *Controller) ListBranches(ctx context.Context,
 	session *auth.Session,
 	repoRef string,
 	includeCommit bool,
 	filter *types.BranchFilter,
-) ([]Branch, error) {
+) ([]types.Branch, error) {
 	repo, err := c.getRepoCheckAccess(ctx, session, repoRef, enum.PermissionRepoView)
 	if err != nil {
 		return nil, err
@@ -56,7 +50,7 @@ func (c *Controller) ListBranches(ctx context.Context,
 		return nil, err
 	}
 
-	branches := make([]Branch, len(rpcOut.Branches))
+	branches := make([]types.Branch, len(rpcOut.Branches))
 	for i := range rpcOut.Branches {
 		branches[i], err = mapBranch(rpcOut.Branches[i])
 		if err != nil {
@@ -95,16 +89,16 @@ func mapToRPCSortOrder(o enum.Order) git.SortOrder {
 	}
 }
 
-func mapBranch(b git.Branch) (Branch, error) {
+func mapBranch(b git.Branch) (types.Branch, error) {
 	var commit *types.Commit
 	if b.Commit != nil {
 		var err error
 		commit, err = controller.MapCommit(b.Commit)
 		if err != nil {
-			return Branch{}, err
+			return types.Branch{}, err
 		}
 	}
-	return Branch{
+	return types.Branch{
 		Name:   b.Name,
 		SHA:    b.SHA.String(),
 		Commit: commit,

app/api/handler/repo/delete_branch.go

@@ -52,15 +52,18 @@ func HandleDeleteBranch(repoCtrl *repo.Controller) http.HandlerFunc {
 			return
 		}
 
-		violations, err := repoCtrl.DeleteBranch(ctx, session, repoRef, branchName, bypassRules, dryRunRules)
+		commitSha := request.GetCommitSHAFromQueryOrDefault(r)
+
+		out, violations, err := repoCtrl.DeleteBranch(ctx, session, repoRef, branchName, bypassRules, dryRunRules, commitSha)
 		if err != nil {
 			render.TranslatedUserError(ctx, w, err)
+			return
 		}
 		if violations != nil {
 			render.Violations(w, violations)
 			return
 		}
 
-		render.DeleteSuccessful(w)
+		render.JSON(w, http.StatusOK, out)
 	}
 }

app/api/openapi/repo.go

@@ -542,6 +542,20 @@ var queryParameterDryRunRules = openapi3.ParameterOrRef{
 	},
 }
 
+var queryParameterCommitSHA = openapi3.ParameterOrRef{
+	Parameter: &openapi3.Parameter{
+		Name:        request.QueryParamCommitSHA,
+		In:          openapi3.ParameterInQuery,
+		Description: ptr.String("Commit SHA the branch is at"),
+		Required:    ptr.Bool(false),
+		Schema: &openapi3.SchemaOrRef{
+			Schema: &openapi3.Schema{
+				Type: ptrSchemaType(openapi3.SchemaTypeString),
+			},
+		},
+	},
+}
+
 var queryParameterDeletedAt = openapi3.ParameterOrRef{
 	Parameter: &openapi3.Parameter{
 		Name:        request.QueryParamDeletedAt,
@@ -900,7 +914,7 @@ func repoOperations(reflector *openapi3.Reflector) {
 	opCreateBranch.WithTags("repository")
 	opCreateBranch.WithMapOfAnything(map[string]interface{}{"operationId": "createBranch"})
 	_ = reflector.SetRequest(&opCreateBranch, new(createBranchRequest), http.MethodPost)
-	_ = reflector.SetJSONResponse(&opCreateBranch, new(repo.Branch), http.StatusCreated)
+	_ = reflector.SetJSONResponse(&opCreateBranch, new(types.Branch), http.StatusCreated)
 	_ = reflector.SetJSONResponse(&opCreateBranch, new(usererror.Error), http.StatusBadRequest)
 	_ = reflector.SetJSONResponse(&opCreateBranch, new(usererror.Error), http.StatusInternalServerError)
 	_ = reflector.SetJSONResponse(&opCreateBranch, new(usererror.Error), http.StatusUnauthorized)
@@ -912,7 +926,7 @@ func repoOperations(reflector *openapi3.Reflector) {
 	opGetBranch.WithTags("repository")
 	opGetBranch.WithMapOfAnything(map[string]interface{}{"operationId": "getBranch"})
 	_ = reflector.SetRequest(&opGetBranch, new(getBranchRequest), http.MethodGet)
-	_ = reflector.SetJSONResponse(&opGetBranch, new(repo.Branch), http.StatusOK)
+	_ = reflector.SetJSONResponse(&opGetBranch, new(types.Branch), http.StatusOK)
 	_ = reflector.SetJSONResponse(&opGetBranch, new(usererror.Error), http.StatusInternalServerError)
 	_ = reflector.SetJSONResponse(&opGetBranch, new(usererror.Error), http.StatusUnauthorized)
 	_ = reflector.SetJSONResponse(&opGetBranch, new(usererror.Error), http.StatusForbidden)
@@ -922,9 +936,9 @@ func repoOperations(reflector *openapi3.Reflector) {
 	opDeleteBranch := openapi3.Operation{}
 	opDeleteBranch.WithTags("repository")
 	opDeleteBranch.WithMapOfAnything(map[string]interface{}{"operationId": "deleteBranch"})
-	opDeleteBranch.WithParameters(queryParameterBypassRules, queryParameterDryRunRules)
+	opDeleteBranch.WithParameters(queryParameterBypassRules, queryParameterDryRunRules, queryParameterCommitSHA)
 	_ = reflector.SetRequest(&opDeleteBranch, new(deleteBranchRequest), http.MethodDelete)
-	_ = reflector.SetJSONResponse(&opDeleteBranch, nil, http.StatusNoContent)
+	_ = reflector.SetJSONResponse(&opDeleteBranch, new(types.DeleteBranchOutput), http.StatusOK)
 	_ = reflector.SetJSONResponse(&opDeleteBranch, new(usererror.Error), http.StatusInternalServerError)
 	_ = reflector.SetJSONResponse(&opDeleteBranch, new(usererror.Error), http.StatusUnauthorized)
 	_ = reflector.SetJSONResponse(&opDeleteBranch, new(usererror.Error), http.StatusForbidden)
@@ -939,7 +953,7 @@ func repoOperations(reflector *openapi3.Reflector) {
 		queryParameterQueryBranches, queryParameterOrder, queryParameterSortBranch,
 		QueryParameterPage, QueryParameterLimit)
 	_ = reflector.SetRequest(&opListBranches, new(listBranchesRequest), http.MethodGet)
-	_ = reflector.SetJSONResponse(&opListBranches, []repo.Branch{}, http.StatusOK)
+	_ = reflector.SetJSONResponse(&opListBranches, []types.Branch{}, http.StatusOK)
 	_ = reflector.SetJSONResponse(&opListBranches, new(usererror.Error), http.StatusInternalServerError)
 	_ = reflector.SetJSONResponse(&opListBranches, new(usererror.Error), http.StatusUnauthorized)
 	_ = reflector.SetJSONResponse(&opListBranches, new(usererror.Error), http.StatusForbidden)

app/api/request/git.go

@@ -27,10 +27,13 @@ import (
 )
 
 const (
+	HeaderParamGitProtocol = "Git-Protocol"
+
+	PathParamCommitSHA = "commit_sha"
+
 	QueryParamGitRef             = "git_ref"
 	QueryParamIncludeCommit      = "include_commit"
 	QueryParamIncludeDirectories = "include_directories"
-	PathParamCommitSHA           = "commit_sha"
 	QueryParamLineFrom           = "line_from"
 	QueryParamLineTo             = "line_to"
 	QueryParamPath               = "path"
@@ -40,7 +43,7 @@ const (
 	QueryParamIncludeStats       = "include_stats"
 	QueryParamInternal           = "internal"
 	QueryParamService            = "service"
-	HeaderParamGitProtocol       = "Git-Protocol"
+	QueryParamCommitSHA          = "commit_sha"
 )
 
 func GetGitRefFromQueryOrDefault(r *http.Request, deflt string) string {
@@ -172,3 +175,7 @@ func GetFileDiffFromQuery(r *http.Request) (files gittypes.FileDiffRequests) {
 	}
 	return
 }
+
+func GetCommitSHAFromQueryOrDefault(r *http.Request) string {
+	return QueryParamOrDefault(r, QueryParamCommitSHA, "")
+}

git/branch.go

@@ -73,6 +73,7 @@ type DeleteBranchParams struct {
 	WriteParams
 	// BranchName is the name of the branch
 	BranchName string
+	SHA        string
 }
 
 type ListBranchesParams struct {
@@ -162,13 +163,14 @@ func (s *Service) DeleteBranch(ctx context.Context, params *DeleteBranchParams)
 
 	repoPath := getFullPathForRepo(s.reposRoot, params.RepoUID)
 	branchRef := api.GetReferenceFromBranchName(params.BranchName)
+	commitSha, _ := sha.NewOrEmpty(params.SHA)
 
 	refUpdater, err := hook.CreateRefUpdater(s.hookClientFactory, params.EnvVars, repoPath, branchRef)
 	if err != nil {
 		return fmt.Errorf("failed to create ref updater to create the branch: %w", err)
 	}
 
-	err = refUpdater.Do(ctx, sha.None, sha.Nil) // delete whatever is there
+	err = refUpdater.Do(ctx, commitSha, sha.Nil)
 	if errors.IsNotFound(err) {
 		return errors.NotFound("branch %q does not exist", params.BranchName)
 	}

types/branch.go

@@ -0,0 +1,25 @@
+// Copyright 2023 Harness, Inc.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package types
+
+type Branch struct {
+	Name   string  `json:"name"`
+	SHA    string  `json:"sha"`
+	Commit *Commit `json:"commit,omitempty"`
+}
+
+type DeleteBranchOutput struct {
+	DryRunRulesOutput
+}

types/commit.go

@@ -16,7 +16,6 @@ package types
 
 // CommitFilesResponse holds commit id.
 type CommitFilesResponse struct {
-	DryRunRules    bool             `json:"dry_run_rules,omitempty"`
-	CommitID       string           `json:"commit_id"`
-	RuleViolations []RuleViolations `json:"rule_violations,omitempty"`
+	CommitID string `json:"commit_id"`
+	DryRunRulesOutput
 }

types/rule.go

@@ -205,3 +205,8 @@ type RulesViolations struct {
 	Message    string           `json:"message"`
 	Violations []RuleViolations `json:"violations"`
 }
+
+type DryRunRulesOutput struct {
+	DryRunRules    bool             `json:"dry_run_rules,omitempty"`
+	RuleViolations []RuleViolations `json:"rule_violations,omitempty"`
+}