fix: invalid error on empty repo collabs (#466)

Ayman Bagabas created 2 years ago

Propagate a more informative error when adding an existing collaborator.
Ignore webhooks default branch git reference not found errors because
the repo won't have a default branch when it's empty.

Fixes: https://github.com/charmbracelet/soft-serve/issues/464

Change summary

pkg/backend/collab.go                 |  4 ++++
pkg/proto/errors.go                   |  2 ++
pkg/webhook/branch_tag.go             |  2 +-
pkg/webhook/collaborator.go           |  2 +-
pkg/webhook/push.go                   |  9 ++-------
pkg/webhook/repository.go             |  5 ++++-
pkg/webhook/webhook.go                | 16 ++++++++++++++++
testscript/testdata/repo-collab.txtar | 12 ++++++++++++
testscript/testdata/soft-browse.txtar |  2 +-
9 files changed, 43 insertions(+), 11 deletions(-)

Detailed changes

pkg/backend/collab.go

@@ -33,6 +33,10 @@ func (d *Backend) AddCollaborator(ctx context.Context, repo string, username str
 			return d.store.AddCollabByUsernameAndRepo(ctx, tx, username, repo, level)
 		}),
 	); err != nil {
+		if errors.Is(err, db.ErrDuplicateKey) {
+			return proto.ErrCollaboratorExist
+		}
+
 		return err
 	}
 

pkg/proto/errors.go

@@ -21,4 +21,6 @@ var (
 	ErrTokenExpired = errors.New("token expired")
 	// ErrCollaboratorNotFound is returned when a collaborator is not found.
 	ErrCollaboratorNotFound = errors.New("collaborator not found")
+	// ErrCollaboratorExist is returned when a collaborator already exists.
+	ErrCollaboratorExist = errors.New("collaborator already exists")
 )

pkg/webhook/branch_tag.go

@@ -77,7 +77,7 @@ func NewBranchTagEvent(ctx context.Context, user proto.User, repo proto.Reposito
 
 	payload.Repository.Owner.ID = owner.ID
 	payload.Repository.Owner.Username = owner.Username
-	payload.Repository.DefaultBranch, err = proto.RepositoryDefaultBranch(repo)
+	payload.Repository.DefaultBranch, err = getDefaultBranch(repo)
 	if err != nil {
 		return BranchTagEvent{}, err
 	}

pkg/webhook/collaborator.go

@@ -65,7 +65,7 @@ func NewCollaboratorEvent(ctx context.Context, user proto.User, repo proto.Repos
 
 	payload.Repository.Owner.ID = owner.ID
 	payload.Repository.Owner.Username = owner.Username
-	payload.Repository.DefaultBranch, err = proto.RepositoryDefaultBranch(repo)
+	payload.Repository.DefaultBranch, err = getDefaultBranch(repo)
 	if err != nil {
 		return CollaboratorEvent{}, err
 	}

pkg/webhook/push.go

@@ -2,7 +2,6 @@ package webhook
 
 import (
 	"context"
-	"errors"
 	"fmt"
 
 	gitm "github.com/aymanbagabas/git-module"
@@ -75,12 +74,8 @@ func NewPushEvent(ctx context.Context, user proto.User, repo proto.Repository, r
 		return PushEvent{}, err
 	}
 
-	payload.Repository.DefaultBranch, err = proto.RepositoryDefaultBranch(repo)
-	// XXX: we check for ErrReferenceNotExist here because we don't want to
-	// return an error if the repo is an empty repo.
-	// This means that the repo doesn't have a default branch yet and this is
-	// the first push to it.
-	if err != nil && !errors.Is(err, git.ErrReferenceNotExist) {
+	payload.Repository.DefaultBranch, err = getDefaultBranch(repo)
+	if err != nil {
 		return PushEvent{}, err
 	}
 

pkg/webhook/repository.go

@@ -76,7 +76,10 @@ func NewRepositoryEvent(ctx context.Context, user proto.User, repo proto.Reposit
 
 	payload.Repository.Owner.ID = owner.ID
 	payload.Repository.Owner.Username = owner.Username
-	payload.Repository.DefaultBranch, _ = proto.RepositoryDefaultBranch(repo)
+	payload.Repository.DefaultBranch, err = getDefaultBranch(repo)
+	if err != nil {
+		return RepositoryEvent{}, err
+	}
 
 	return payload, nil
 }

pkg/webhook/webhook.go

@@ -7,12 +7,15 @@ import (
 	"crypto/sha256"
 	"encoding/hex"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"io"
 	"net/http"
 
+	"github.com/charmbracelet/soft-serve/git"
 	"github.com/charmbracelet/soft-serve/pkg/db"
 	"github.com/charmbracelet/soft-serve/pkg/db/models"
+	"github.com/charmbracelet/soft-serve/pkg/proto"
 	"github.com/charmbracelet/soft-serve/pkg/store"
 	"github.com/charmbracelet/soft-serve/pkg/utils"
 	"github.com/charmbracelet/soft-serve/pkg/version"
@@ -142,3 +145,16 @@ func SendEvent(ctx context.Context, payload EventPayload) error {
 func repoURL(publicURL string, repo string) string {
 	return fmt.Sprintf("%s/%s.git", publicURL, utils.SanitizeRepo(repo))
 }
+
+func getDefaultBranch(repo proto.Repository) (string, error) {
+	branch, err := proto.RepositoryDefaultBranch(repo)
+	// XXX: we check for ErrReferenceNotExist here because we don't want to
+	// return an error if the repo is an empty repo.
+	// This means that the repo doesn't have a default branch yet and this is
+	// the first push to it.
+	if err != nil && !errors.Is(err, git.ErrReferenceNotExist) {
+		return "", err
+	}
+
+	return branch, nil
+}

testscript/testdata/repo-collab.txtar

@@ -23,6 +23,18 @@ soft repo collab remove test foo
 soft repo collab list test
 ! stdout .
 
+# create empty repo
+soft repo create empty '-d "empty repo"'
+
+# add collab
+soft repo collab add empty foo
+# add collab again
+# test issue #464 https://github.com/charmbracelet/soft-serve/issues/464
+! soft repo collab add empty foo
+stderr '.*already exists.*'
+# a placeholder to reset stderr
+soft help
+
 # stop the server
 [windows] stopserver
 [windows] ! stderr .

testscript/testdata/soft-browse.txtar

@@ -3,7 +3,7 @@
 [windows] skip
 
 # clone repo
-git clone https://github.com/charmbracelet/catwalk.git catwalk
+#git clone https://github.com/charmbracelet/catwalk.git catwalk
 
 # run soft browse
 # disable this temporarily