From 047b6a7429e3d98070a1d475ba66e068fcd1bce2 Mon Sep 17 00:00:00 2001 From: Ayman Bagabas Date: Mon, 5 Feb 2024 12:43:16 -0500 Subject: [PATCH] fix: invalid error on empty repo collabs (#466) 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 --- 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(-) diff --git a/pkg/backend/collab.go b/pkg/backend/collab.go index c9635ae37..ade58b239 100644 --- a/pkg/backend/collab.go +++ b/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 } diff --git a/pkg/proto/errors.go b/pkg/proto/errors.go index cc48b6f78..fa4bc6126 100644 --- a/pkg/proto/errors.go +++ b/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") ) diff --git a/pkg/webhook/branch_tag.go b/pkg/webhook/branch_tag.go index 89771e9fe..5e545879e 100644 --- a/pkg/webhook/branch_tag.go +++ b/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 } diff --git a/pkg/webhook/collaborator.go b/pkg/webhook/collaborator.go index e7b737b16..67a42aed9 100644 --- a/pkg/webhook/collaborator.go +++ b/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 } diff --git a/pkg/webhook/push.go b/pkg/webhook/push.go index 6ef6062cd..a25d79379 100644 --- a/pkg/webhook/push.go +++ b/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 } diff --git a/pkg/webhook/repository.go b/pkg/webhook/repository.go index 3cad5c39f..99fad6eec 100644 --- a/pkg/webhook/repository.go +++ b/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 } diff --git a/pkg/webhook/webhook.go b/pkg/webhook/webhook.go index e5c4d2bb2..8bd28e9c1 100644 --- a/pkg/webhook/webhook.go +++ b/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 +} diff --git a/testscript/testdata/repo-collab.txtar b/testscript/testdata/repo-collab.txtar index d692831bb..d2960693a 100644 --- a/testscript/testdata/repo-collab.txtar +++ b/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 . diff --git a/testscript/testdata/soft-browse.txtar b/testscript/testdata/soft-browse.txtar index 2ab46f333..7514f770d 100644 --- a/testscript/testdata/soft-browse.txtar +++ b/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