From a6450494c3a9c05667ae8a2ebdfdea55b8206e9b Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Sat, 29 Apr 2023 02:14:26 +0800 Subject: [PATCH] Fix unclear `IsRepositoryExist` logic (#24374) There was only one `IsRepositoryExist` function, it did: `has && isDir` However it's not right, and it would cause 500 error when creating a new repository if the dir exists. Then, it was changed to `has || isDir`, it is still incorrect, it affects the "adopt repo" logic. To make the logic clear: * IsRepositoryModelOrDirExist * IsRepositoryModelExist --- models/repo/repo.go | 16 ++++++++++------ models/repo/update.go | 4 ++-- models/repo_transfer.go | 4 ++-- modules/repository/create.go | 2 +- routers/api/v1/admin/adopt.go | 4 ++-- routers/web/admin/repos.go | 2 +- routers/web/user/setting/adopt.go | 2 +- services/repository/adopt.go | 2 +- 8 files changed, 20 insertions(+), 16 deletions(-) diff --git a/models/repo/repo.go b/models/repo/repo.go index f9de6d493d..2e8c28cbb3 100644 --- a/models/repo/repo.go +++ b/models/repo/repo.go @@ -726,12 +726,9 @@ func GetRepositoriesMapByIDs(ids []int64) (map[int64]*Repository, error) { return repos, db.GetEngine(db.DefaultContext).In("id", ids).Find(&repos) } -// IsRepositoryExist returns true if the repository with given name under user has already existed. -func IsRepositoryExist(ctx context.Context, u *user_model.User, repoName string) (bool, error) { - has, err := db.GetEngine(ctx).Get(&Repository{ - OwnerID: u.ID, - LowerName: strings.ToLower(repoName), - }) +// IsRepositoryModelOrDirExist returns true if the repository with given name under user has already existed. +func IsRepositoryModelOrDirExist(ctx context.Context, u *user_model.User, repoName string) (bool, error) { + has, err := IsRepositoryModelExist(ctx, u, repoName) if err != nil { return false, err } @@ -739,6 +736,13 @@ func IsRepositoryExist(ctx context.Context, u *user_model.User, repoName string) return has || isDir, err } +func IsRepositoryModelExist(ctx context.Context, u *user_model.User, repoName string) (bool, error) { + return db.GetEngine(ctx).Get(&Repository{ + OwnerID: u.ID, + LowerName: strings.ToLower(repoName), + }) +} + // GetTemplateRepo populates repo.TemplateRepo for a generated repository and // returns an error on failure (NOTE: no error is returned for // non-generated repositories, and TemplateRepo will be left untouched) diff --git a/models/repo/update.go b/models/repo/update.go index f4cb67bb8f..4894e0a1b9 100644 --- a/models/repo/update.go +++ b/models/repo/update.go @@ -116,7 +116,7 @@ func CheckCreateRepository(doer, u *user_model.User, name string, overwriteOrAdo return err } - has, err := IsRepositoryExist(db.DefaultContext, u, name) + has, err := IsRepositoryModelOrDirExist(db.DefaultContext, u, name) if err != nil { return fmt.Errorf("IsRepositoryExist: %w", err) } else if has { @@ -147,7 +147,7 @@ func ChangeRepositoryName(doer *user_model.User, repo *Repository, newRepoName s return err } - has, err := IsRepositoryExist(db.DefaultContext, repo.Owner, newRepoName) + has, err := IsRepositoryModelOrDirExist(db.DefaultContext, repo.Owner, newRepoName) if err != nil { return fmt.Errorf("IsRepositoryExist: %w", err) } else if has { diff --git a/models/repo_transfer.go b/models/repo_transfer.go index 27a77f9b8c..1c873cec57 100644 --- a/models/repo_transfer.go +++ b/models/repo_transfer.go @@ -172,7 +172,7 @@ func CreatePendingRepositoryTransfer(ctx context.Context, doer, newOwner *user_m } // Check if new owner has repository with same name. - if has, err := repo_model.IsRepositoryExist(ctx, newOwner, repo.Name); err != nil { + if has, err := repo_model.IsRepositoryModelExist(ctx, newOwner, repo.Name); err != nil { return fmt.Errorf("IsRepositoryExist: %w", err) } else if has { return repo_model.ErrRepoAlreadyExist{ @@ -249,7 +249,7 @@ func TransferOwnership(doer *user_model.User, newOwnerName string, repo *repo_mo newOwnerName = newOwner.Name // ensure capitalisation matches // Check if new owner has repository with same name. - if has, err := repo_model.IsRepositoryExist(ctx, newOwner, repo.Name); err != nil { + if has, err := repo_model.IsRepositoryModelOrDirExist(ctx, newOwner, repo.Name); err != nil { return fmt.Errorf("IsRepositoryExist: %w", err) } else if has { return repo_model.ErrRepoAlreadyExist{ diff --git a/modules/repository/create.go b/modules/repository/create.go index c1395242c5..e1f0bdcdf9 100644 --- a/modules/repository/create.go +++ b/modules/repository/create.go @@ -35,7 +35,7 @@ func CreateRepositoryByExample(ctx context.Context, doer, u *user_model.User, re return err } - has, err := repo_model.IsRepositoryExist(ctx, u, repo.Name) + has, err := repo_model.IsRepositoryModelExist(ctx, u, repo.Name) if err != nil { return fmt.Errorf("IsRepositoryExist: %w", err) } else if has { diff --git a/routers/api/v1/admin/adopt.go b/routers/api/v1/admin/adopt.go index 47fd0ef3c3..ccd8be9171 100644 --- a/routers/api/v1/admin/adopt.go +++ b/routers/api/v1/admin/adopt.go @@ -95,7 +95,7 @@ func AdoptRepository(ctx *context.APIContext) { } // check not a repo - has, err := repo_model.IsRepositoryExist(ctx, ctxUser, repoName) + has, err := repo_model.IsRepositoryModelExist(ctx, ctxUser, repoName) if err != nil { ctx.InternalServerError(err) return @@ -157,7 +157,7 @@ func DeleteUnadoptedRepository(ctx *context.APIContext) { } // check not a repo - has, err := repo_model.IsRepositoryExist(ctx, ctxUser, repoName) + has, err := repo_model.IsRepositoryModelExist(ctx, ctxUser, repoName) if err != nil { ctx.InternalServerError(err) return diff --git a/routers/web/admin/repos.go b/routers/web/admin/repos.go index b22546f749..9a0e467b48 100644 --- a/routers/web/admin/repos.go +++ b/routers/web/admin/repos.go @@ -133,7 +133,7 @@ func AdoptOrDeleteRepository(ctx *context.Context) { repoName := dirSplit[1] // check not a repo - has, err := repo_model.IsRepositoryExist(ctx, ctxUser, repoName) + has, err := repo_model.IsRepositoryModelExist(ctx, ctxUser, repoName) if err != nil { ctx.ServerError("IsRepositoryExist", err) return diff --git a/routers/web/user/setting/adopt.go b/routers/web/user/setting/adopt.go index c9995e7278..01668c3954 100644 --- a/routers/web/user/setting/adopt.go +++ b/routers/web/user/setting/adopt.go @@ -31,7 +31,7 @@ func AdoptOrDeleteRepository(ctx *context.Context) { root := user_model.UserPath(ctxUser.LowerName) // check not a repo - has, err := repo_model.IsRepositoryExist(ctx, ctxUser, dir) + has, err := repo_model.IsRepositoryModelExist(ctx, ctxUser, dir) if err != nil { ctx.ServerError("IsRepositoryExist", err) return diff --git a/services/repository/adopt.go b/services/repository/adopt.go index 94b2c3f3d5..55e77a78a7 100644 --- a/services/repository/adopt.go +++ b/services/repository/adopt.go @@ -206,7 +206,7 @@ func DeleteUnadoptedRepository(ctx context.Context, doer, u *user_model.User, re } } - if exist, err := repo_model.IsRepositoryExist(ctx, u, repoName); err != nil { + if exist, err := repo_model.IsRepositoryModelExist(ctx, u, repoName); err != nil { return err } else if exist { return repo_model.ErrRepoAlreadyExist{