From 35cc5b0402d46d672e02bbe1ad15d1460077e8f4 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sat, 2 Dec 2017 15:34:39 +0800 Subject: [PATCH] Remove GetRepositoryByRef and add GetRepositoryByOwnerAndName (#3043) * remove GetRepositoryByRef and add GetRepositoryByOwnerAndName * fix tests * fix tests bug * some improvements --- cmd/serv.go | 14 +++----------- models/error.go | 10 ++++++---- models/issue.go | 9 +++++++-- models/repo.go | 31 +++++++++++++++---------------- modules/context/context.go | 9 +++------ modules/lfs/server.go | 17 ++++++----------- routers/repo/http.go | 18 ++---------------- 7 files changed, 42 insertions(+), 66 deletions(-) diff --git a/cmd/serv.go b/cmd/serv.go index 5dc378585..1ff296d00 100644 --- a/cmd/serv.go +++ b/cmd/serv.go @@ -158,18 +158,10 @@ func runServ(c *cli.Context) error { } os.Setenv(models.EnvRepoName, reponame) - repoUser, err := models.GetUserByName(username) - if err != nil { - if models.IsErrUserNotExist(err) { - fail("Repository owner does not exist", "Unregistered owner: %s", username) - } - fail("Internal error", "Failed to get repository owner (%s): %v", username, err) - } - - repo, err := models.GetRepositoryByName(repoUser.ID, reponame) + repo, err := models.GetRepositoryByOwnerAndName(username, reponame) if err != nil { if models.IsErrRepoNotExist(err) { - fail(accessDenied, "Repository does not exist: %s/%s", repoUser.Name, reponame) + fail(accessDenied, "Repository does not exist: %s/%s", username, reponame) } fail("Internal error", "Failed to get repository: %v", err) } @@ -263,7 +255,7 @@ func runServ(c *cli.Context) error { //LFS token authentication if verb == lfsAuthenticateVerb { - url := fmt.Sprintf("%s%s/%s.git/info/lfs", setting.AppURL, repoUser.Name, repo.Name) + url := fmt.Sprintf("%s%s/%s.git/info/lfs", setting.AppURL, username, repo.Name) now := time.Now() token := jwt.NewWithClaims(jwt.SigningMethodHS256, jwt.MapClaims{ diff --git a/models/error.go b/models/error.go index 50d9cba17..fceee21fd 100644 --- a/models/error.go +++ b/models/error.go @@ -572,9 +572,10 @@ func (err ErrLFSLockAlreadyExist) Error() string { // ErrRepoNotExist represents a "RepoNotExist" kind of error. type ErrRepoNotExist struct { - ID int64 - UID int64 - Name string + ID int64 + UID int64 + OwnerName string + Name string } // IsErrRepoNotExist checks if an error is a ErrRepoNotExist. @@ -584,7 +585,8 @@ func IsErrRepoNotExist(err error) bool { } func (err ErrRepoNotExist) Error() string { - return fmt.Sprintf("repository does not exist [id: %d, uid: %d, name: %s]", err.ID, err.UID, err.Name) + return fmt.Sprintf("repository does not exist [id: %d, uid: %d, owner_name: %s, name: %s]", + err.ID, err.UID, err.OwnerName, err.Name) } // ErrRepoAlreadyExist represents a "RepoAlreadyExist" kind of error. diff --git a/models/issue.go b/models/issue.go index 17aef1055..00e0bf802 100644 --- a/models/issue.go +++ b/models/issue.go @@ -964,7 +964,7 @@ func NewIssue(repo *Repository, issue *Issue, labelIDs []int64, uuids []string) // GetIssueByRef returns an Issue specified by a GFM reference. // See https://help.github.com/articles/writing-on-github#references for more information on the syntax. func GetIssueByRef(ref string) (*Issue, error) { - n := strings.IndexByte(ref, byte('#')) + n := strings.IndexByte(ref, '#') if n == -1 { return nil, errMissingIssueNumber } @@ -974,7 +974,12 @@ func GetIssueByRef(ref string) (*Issue, error) { return nil, errInvalidIssueNumber } - repo, err := GetRepositoryByRef(ref[:n]) + i := strings.IndexByte(ref[:n], '/') + if i < 2 { + return nil, ErrInvalidReference + } + + repo, err := GetRepositoryByOwnerAndName(ref[:i], ref[i+1:n]) if err != nil { return nil, err } diff --git a/models/repo.go b/models/repo.go index 8d57ae51a..a9f0d9303 100644 --- a/models/repo.go +++ b/models/repo.go @@ -1740,13 +1740,13 @@ func DeleteRepository(doer *User, uid, repoID int64) error { if err != nil { return err } else if !has { - return ErrRepoNotExist{repoID, uid, ""} + return ErrRepoNotExist{repoID, uid, "", ""} } if cnt, err := sess.ID(repoID).Delete(&Repository{}); err != nil { return err } else if cnt != 1 { - return ErrRepoNotExist{repoID, uid, ""} + return ErrRepoNotExist{repoID, uid, "", ""} } if org.IsOrganization() { @@ -1891,21 +1891,20 @@ func DeleteRepository(doer *User, uid, repoID int64) error { return nil } -// GetRepositoryByRef returns a Repository specified by a GFM reference. -// See https://help.github.com/articles/writing-on-github#references for more information on the syntax. -func GetRepositoryByRef(ref string) (*Repository, error) { - n := strings.IndexByte(ref, byte('/')) - if n < 2 { - return nil, ErrInvalidReference - } - - userName, repoName := ref[:n], ref[n+1:] - user, err := GetUserByName(userName) +// GetRepositoryByOwnerAndName returns the repository by given ownername and reponame. +func GetRepositoryByOwnerAndName(ownerName, repoName string) (*Repository, error) { + var repo Repository + has, err := x.Select("repository.*"). + Join("INNER", "user", "`user`.id = repository.owner_id"). + Where("repository.lower_name = ?", strings.ToLower(repoName)). + And("`user`.lower_name = ?", strings.ToLower(ownerName)). + Get(&repo) if err != nil { return nil, err + } else if !has { + return nil, ErrRepoNotExist{0, 0, ownerName, repoName} } - - return GetRepositoryByName(user.ID, repoName) + return &repo, nil } // GetRepositoryByName returns the repository by given name under user if exists. @@ -1918,7 +1917,7 @@ func GetRepositoryByName(ownerID int64, name string) (*Repository, error) { if err != nil { return nil, err } else if !has { - return nil, ErrRepoNotExist{0, ownerID, name} + return nil, ErrRepoNotExist{0, ownerID, "", name} } return repo, err } @@ -1929,7 +1928,7 @@ func getRepositoryByID(e Engine, id int64) (*Repository, error) { if err != nil { return nil, err } else if !has { - return nil, ErrRepoNotExist{id, 0, ""} + return nil, ErrRepoNotExist{id, 0, "", ""} } return repo, nil } diff --git a/modules/context/context.go b/modules/context/context.go index 6fb0a2cde..10a84cd9b 100644 --- a/modules/context/context.go +++ b/modules/context/context.go @@ -176,12 +176,9 @@ func Contexter() macaron.Handler { repoName := c.Params(":reponame") branchName := "master" - owner, err := models.GetUserByName(ownerName) - if err == nil { - repo, err := models.GetRepositoryByName(owner.ID, repoName) - if err == nil && len(repo.DefaultBranch) > 0 { - branchName = repo.DefaultBranch - } + repo, err := models.GetRepositoryByOwnerAndName(ownerName, repoName) + if err == nil && len(repo.DefaultBranch) > 0 { + branchName = repo.DefaultBranch } prefix := setting.AppURL + path.Join(ownerName, repoName, "src", "branch", branchName) c.PlainText(http.StatusOK, []byte(com.Expand(` diff --git a/modules/lfs/server.go b/modules/lfs/server.go index 68f2af151..474a3f56c 100644 --- a/modules/lfs/server.go +++ b/modules/lfs/server.go @@ -108,10 +108,9 @@ func ObjectOidHandler(ctx *context.Context) { } func getAuthenticatedRepoAndMeta(ctx *context.Context, rv *RequestVars, requireWrite bool) (*models.LFSMetaObject, *models.Repository) { - repositoryString := rv.User + "/" + rv.Repo - repository, err := models.GetRepositoryByRef(repositoryString) + repository, err := models.GetRepositoryByOwnerAndName(rv.User, rv.Repo) if err != nil { - log.Debug("Could not find repository: %s - %s", repositoryString, err) + log.Debug("Could not find repository: %s/%s - %s", rv.User, rv.Repo, err) writeStatus(ctx, 404) return nil, nil } @@ -197,7 +196,6 @@ func getMetaHandler(ctx *context.Context) { // PostHandler instructs the client how to upload data func PostHandler(ctx *context.Context) { - if !setting.LFS.StartServer { writeStatus(ctx, 404) return @@ -210,10 +208,9 @@ func PostHandler(ctx *context.Context) { rv := unpack(ctx) - repositoryString := rv.User + "/" + rv.Repo - repository, err := models.GetRepositoryByRef(repositoryString) + repository, err := models.GetRepositoryByOwnerAndName(rv.User, rv.Repo) if err != nil { - log.Debug("Could not find repository: %s - %s", repositoryString, err) + log.Debug("Could not find repository: %s/%s - %s", rv.User, rv.Repo, err) writeStatus(ctx, 404) return } @@ -261,12 +258,10 @@ func BatchHandler(ctx *context.Context) { // Create a response object for _, object := range bv.Objects { - - repositoryString := object.User + "/" + object.Repo - repository, err := models.GetRepositoryByRef(repositoryString) + repository, err := models.GetRepositoryByOwnerAndName(object.User, object.Repo) if err != nil { - log.Debug("Could not find repository: %s - %s", repositoryString, err) + log.Debug("Could not find repository: %s/%s - %s", object.User, object.Repo, err) writeStatus(ctx, 404) return } diff --git a/routers/repo/http.go b/routers/repo/http.go index cac1ec335..c5b45f9cc 100644 --- a/routers/repo/http.go +++ b/routers/repo/http.go @@ -64,23 +64,9 @@ func HTTP(ctx *context.Context) { reponame = reponame[:len(reponame)-5] } - repoUser, err := models.GetUserByName(username) + repo, err := models.GetRepositoryByOwnerAndName(username, reponame) if err != nil { - if models.IsErrUserNotExist(err) { - ctx.Handle(http.StatusNotFound, "GetUserByName", nil) - } else { - ctx.Handle(http.StatusInternalServerError, "GetUserByName", err) - } - return - } - - repo, err := models.GetRepositoryByName(repoUser.ID, reponame) - if err != nil { - if models.IsErrRepoNotExist(err) { - ctx.Handle(http.StatusNotFound, "GetRepositoryByName", nil) - } else { - ctx.Handle(http.StatusInternalServerError, "GetRepositoryByName", err) - } + ctx.NotFoundOrServerError("GetRepositoryByOwnerAndName", models.IsErrRepoNotExist, err) return }