Remove GetRepositoryByRef and add GetRepositoryByOwnerAndName (#3043)

* remove GetRepositoryByRef and add GetRepositoryByOwnerAndName

* fix tests

* fix tests bug

* some improvements

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{

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.

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
 	}

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
 }

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(`

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
 		}

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
 	}