From f4580eca837d2c319ca9070389938403530b8378 Mon Sep 17 00:00:00 2001 From: Adnan Maolood Date: Mon, 21 Mar 2022 09:55:04 -0400 Subject: [PATCH] api/graph: Improve error handling --- api/go.mod | 2 +- api/go.sum | 4 +-- api/graph/schema.resolvers.go | 52 ++++++++++++++++------------------- 3 files changed, 27 insertions(+), 31 deletions(-) diff --git a/api/go.mod b/api/go.mod index d7cab97..24c4f69 100644 --- a/api/go.mod +++ b/api/go.mod @@ -3,7 +3,7 @@ module git.sr.ht/~sircmpwn/git.sr.ht/api go 1.14 require ( - git.sr.ht/~sircmpwn/core-go v0.0.0-20220217133755-ebf93be7318f + git.sr.ht/~sircmpwn/core-go v0.0.0-20220321082727-3f80f677f56d git.sr.ht/~sircmpwn/dowork v0.0.0-20210820133136-d3970e97def3 github.com/99designs/gqlgen v0.14.0 github.com/Masterminds/squirrel v1.4.0 diff --git a/api/go.sum b/api/go.sum index ceb2671..22c3774 100644 --- a/api/go.sum +++ b/api/go.sum @@ -31,8 +31,8 @@ cloud.google.com/go/storage v1.6.0/go.mod h1:N7U0C8pVQ/+NIKOBQyamJIeKQKkZ+mxpohl cloud.google.com/go/storage v1.8.0/go.mod h1:Wv1Oy7z6Yz3DshWRJFhqM/UCfaWIRTdp0RXyy7KQOVs= cloud.google.com/go/storage v1.10.0/go.mod h1:FLPqc6j+Ki4BU591ie1oL6qBQGu2Bl/tZ9ullr3+Kg0= dmitri.shuralyov.com/gpu/mtl v0.0.0-20190408044501-666a987793e9/go.mod h1:H6x//7gZCb22OMCxBHrMx7a5I7Hp++hsVxbQ4BYO7hU= -git.sr.ht/~sircmpwn/core-go v0.0.0-20220217133755-ebf93be7318f h1:ofisQDKdPe5C0E8YPRqU4rV3Nj3Qvo9Z3GfBjKuDY9c= -git.sr.ht/~sircmpwn/core-go v0.0.0-20220217133755-ebf93be7318f/go.mod h1:uUqzeO5OLl/nRZfPk0igIAweRZiVwUmu/OGYfjS9fWc= +git.sr.ht/~sircmpwn/core-go v0.0.0-20220321082727-3f80f677f56d h1:0weIXd6ya99UAY0ZCOIKRIuFCarlxm3d8mI9thx4qHI= +git.sr.ht/~sircmpwn/core-go v0.0.0-20220321082727-3f80f677f56d/go.mod h1:uUqzeO5OLl/nRZfPk0igIAweRZiVwUmu/OGYfjS9fWc= git.sr.ht/~sircmpwn/dowork v0.0.0-20210820133136-d3970e97def3 h1:9WCv5cK67s2SiY/R4DWT/OchEsFnfYDz3lbevKxZ4QI= git.sr.ht/~sircmpwn/dowork v0.0.0-20210820133136-d3970e97def3/go.mod h1:8neHEO3503w/rNtttnR0JFpQgM/GFhaafVwvkPsFIDw= git.sr.ht/~sircmpwn/getopt v0.0.0-20191230200459-23622cc906b3 h1:4wDp4BKF7NQqoh73VXpZsB/t1OEhDpz/zEpmdQfbjDk= diff --git a/api/graph/schema.resolvers.go b/api/graph/schema.resolvers.go index 4588ca5..e4aa3ae 100644 --- a/api/graph/schema.resolvers.go +++ b/api/graph/schema.resolvers.go @@ -136,7 +136,7 @@ func (r *mutationResolver) CreateRepository(ctx context.Context, name string, vi &repo.Description, &repo.Visibility, &repo.Path, &repo.OwnerID); err != nil { if strings.Contains(err.Error(), "duplicate key value violates unique constraint") { - return valid.Errorf(ctx, "name", "A repository with this name already exists.") + return valid.Error(ctx, "name", "A repository with this name already exists.") } return err } @@ -183,7 +183,7 @@ func (r *mutationResolver) CreateRepository(ctx context.Context, name string, vi if err != nil { return valid.Errorf(ctx, "cloneUrl", "Invalid clone URL: %s", err) } else if u.Host == "" { - return valid.Errorf(ctx, "cloneUrl", "Cannot use URL without host") + return valid.Error(ctx, "cloneUrl", "Cannot use URL without host") } else if _, ok := allowedCloneSchemes[u.Scheme]; !ok { return valid.Errorf(ctx, "cloneUrl", "Unsupported protocol %q", u.Scheme) } @@ -199,21 +199,21 @@ func (r *mutationResolver) CreateRepository(ctx context.Context, name string, vi u.Path = strings.TrimPrefix(u.Path, "/") split := strings.SplitN(u.Path, "/", 2) if len(split) != 2 { - return valid.Errorf(ctx, "cloneUrl", "Invalid clone URL") + return valid.Error(ctx, "cloneUrl", "Invalid clone URL") } canonicalName, repoName := split[0], split[1] entity := canonicalName if strings.HasPrefix(entity, "~") { entity = entity[1:] } else { - return valid.Errorf(ctx, "cloneUrl", "Invalid username") + return valid.Error(ctx, "cloneUrl", "Invalid username") } repo, err := loaders.ForContext(ctx). RepositoriesByOwnerRepoName.Load(loaders.OwnerRepoName{entity, repoName}) if err != nil { panic(err) } else if repo == nil { - return valid.Errorf(ctx, "cloneUrl", "Repository not found") + return valid.Error(ctx, "cloneUrl", "Repository not found") } cloneURL = &repo.Path } @@ -277,20 +277,20 @@ func (r *mutationResolver) UpdateRepository(ctx context.Context, id int, input m query := sq.Update(repo.Table()). PlaceholderFormat(sq.Dollar) - valid := valid.New(ctx).WithInput(input) + validation := valid.New(ctx).WithInput(input) - valid.OptionalString("name", func(name string) { - valid.Expect(repoNameRE.MatchString(name), + validation.OptionalString("name", func(name string) { + validation.Expect(repoNameRE.MatchString(name), "Invalid repository name '%s' (must match %s)", name, repoNameRE.String()). WithField("name") - valid.Expect(name != "." && name != "..", + validation.Expect(name != "." && name != "..", "Invalid repository name '%s' (must not be . or ..)", name). WithField("name") - valid.Expect(name != ".git" && name != ".hg", + validation.Expect(name != ".git" && name != ".hg", "Invalid repository name '%s' (must not be .git or .hg)", name). WithField("name") - if !valid.Ok() { + if !validation.Ok() { return } @@ -307,12 +307,10 @@ func (r *mutationResolver) UpdateRepository(ctx context.Context, id int, input m `, id, auth.ForContext(ctx).UserID) if err := row.Scan(&origPath); err != nil { if err == sql.ErrNoRows { - // TODO: Ability to return errors from OptionalString callback? - valid.Error("No repository by ID %d found for this user", id). - WithField("id") + validation.Error("No repository by ID %d found for this user", id) return } - valid.Error(err.Error()) + validation.Error("%s", err.Error()) return } @@ -325,10 +323,11 @@ func (r *mutationResolver) UpdateRepository(ctx context.Context, id int, input m repoPath := path.Join(repoStore, "~"+user.Username, name) err := os.Rename(origPath, repoPath) if errors.Is(err, os.ErrExist) { - valid.Error("A repository with this name already exists."). + validation.Error("A repository with this name already exists."). WithField("name") + return } else if err != nil { - valid.Error(err.Error()) + validation.Error("%s", err.Error()) return } moved = true @@ -336,22 +335,19 @@ func (r *mutationResolver) UpdateRepository(ctx context.Context, id int, input m query = query.Set(`path`, repoPath) }) - valid.NullableString("description", func(description *string) { + validation.NullableString("description", func(description *string) { if description == nil { query = query.Set(`description`, nil) } else { - // XXX: Should we allow an empty description? query = query.Set(`description`, *description) } }) - valid.OptionalString("visibility", func(vis string) { - valid.Expect(model.Visibility(vis).IsValid(), - "Invalid visibility '%s'", vis) + validation.OptionalString("visibility", func(vis string) { query = query.Set(`visibility`, vis) }) - valid.NullableString("readme", func(readme *string) { + validation.NullableString("readme", func(readme *string) { if readme == nil { query = query.Set(`readme`, nil) } else { @@ -359,7 +355,7 @@ func (r *mutationResolver) UpdateRepository(ctx context.Context, id int, input m } }) - if !valid.Ok() { + if !validation.Ok() { return errors.New("placeholder") // TODO: Avoid surfacing placeholder error } @@ -382,7 +378,7 @@ func (r *mutationResolver) UpdateRepository(ctx context.Context, id int, input m } // This must be done after the query so that repo.Path is populated - valid.OptionalString("HEAD", func(ref string) { + validation.OptionalString("HEAD", func(ref string) { gitRepo := repo.Repo() gitRepo.Lock() defer gitRepo.Unlock() @@ -390,17 +386,17 @@ func (r *mutationResolver) UpdateRepository(ctx context.Context, id int, input m // Make sure that the branch exists branch, err := gitRepo.Storer.Reference(branchName) if err != nil { - valid.Error("%s", err.Error()).WithField("HEAD") + validation.Error("%s", err.Error()).WithField("HEAD") return } head := plumbing.NewSymbolicReference(plumbing.HEAD, branch.Name()) if err := gitRepo.Storer.SetReference(head); err != nil { - valid.Error("%s", err.Error()).WithField("HEAD") + validation.Error("%s", err.Error()).WithField("HEAD") return } }) - if !valid.Ok() { + if !validation.Ok() { return errors.New("placeholder") // TODO: Avoid surfacing placeholder error } -- 2.38.4