Refactor push mirror find and add check for updating push mirror

Author: lunny

models/repo/pushmirror.go

@@ -9,6 +9,7 @@ import (
 
 	"code.gitea.io/gitea/models/db"
 	"code.gitea.io/gitea/modules/log"
+	"code.gitea.io/gitea/modules/optional"
 	"code.gitea.io/gitea/modules/timeutil"
 	"code.gitea.io/gitea/modules/util"
 
@@ -96,26 +97,47 @@ func DeletePushMirrors(ctx context.Context, opts PushMirrorOptions) error {
 	return util.NewInvalidArgumentErrorf("repoID required and must be set")
 }
 
+type findPushMirrorOptions struct {
+	db.ListOptions
+	RepoID       int64
+	SyncOnCommit optional.Option[bool]
+}
+
+func (opts findPushMirrorOptions) ToConds() builder.Cond {
+	cond := builder.NewCond()
+	if opts.RepoID > 0 {
+		cond = cond.And(builder.Eq{"repo_id": opts.RepoID})
+	}
+	if opts.SyncOnCommit.Has() {
+		cond = cond.And(builder.Eq{"sync_on_commit": opts.SyncOnCommit.Value()})
+	}
+	return cond
+}
+
 // GetPushMirrorsByRepoID returns push-mirror information of a repository.
 func GetPushMirrorsByRepoID(ctx context.Context, repoID int64, listOptions db.ListOptions) ([]*PushMirror, int64, error) {
-	sess := db.GetEngine(ctx).Where("repo_id = ?", repoID)
-	if listOptions.Page != 0 {
-		sess = db.SetSessionPagination(sess, &listOptions)
-		mirrors := make([]*PushMirror, 0, listOptions.PageSize)
-		count, err := sess.FindAndCount(&mirrors)
-		return mirrors, count, err
+	return db.FindAndCount[PushMirror](ctx, findPushMirrorOptions{
+		ListOptions: listOptions,
+		RepoID:      repoID,
+	})
+}
+
+func GetPushMirrorByID(ctx context.Context, id int64) (*PushMirror, error) {
+	mirror, has, err := db.GetByID[PushMirror](ctx, id)
+	if err != nil {
+		return nil, err
+	} else if !has {
+		return nil, ErrPushMirrorNotExist
 	}
-	mirrors := make([]*PushMirror, 0, 10)
-	count, err := sess.FindAndCount(&mirrors)
-	return mirrors, count, err
+	return mirror, nil
 }
 
 // GetPushMirrorsSyncedOnCommit returns push-mirrors for this repo that should be updated by new commits
 func GetPushMirrorsSyncedOnCommit(ctx context.Context, repoID int64) ([]*PushMirror, error) {
-	mirrors := make([]*PushMirror, 0, 10)
-	return mirrors, db.GetEngine(ctx).
-		Where("repo_id = ? AND sync_on_commit = ?", repoID, true).
-		Find(&mirrors)
+	return db.Find[PushMirror](ctx, findPushMirrorOptions{
+		RepoID:       repoID,
+		SyncOnCommit: optional.Some(true),
+	})
 }
 
 // PushMirrorsIterate iterates all push-mirror repositories.

routers/web/repo/setting/setting.go

@@ -317,15 +317,13 @@ func SettingsPost(ctx *context.Context) {
 			return
 		}
 
-		id, err := strconv.ParseInt(form.PushMirrorID, 10, 64)
+		m, err := selectPushMirrorByForm(ctx, form, repo)
 		if err != nil {
-			ctx.ServerError("UpdatePushMirrorIntervalPushMirrorID", err)
+			ctx.NotFound("", nil)
 			return
 		}
-		m := &repo_model.PushMirror{
-			ID:       id,
-			Interval: interval,
-		}
+
+		m.Interval = interval
 		if err := repo_model.UpdatePushMirrorInterval(ctx, m); err != nil {
 			ctx.ServerError("UpdatePushMirrorInterval", err)
 			return
@@ -1002,17 +1000,14 @@ func selectPushMirrorByForm(ctx *context.Context, form *forms.RepoSettingForm, r
 		return nil, err
 	}
 
-	pushMirrors, _, err := repo_model.GetPushMirrorsByRepoID(ctx, repo.ID, db.ListOptions{})
+	pushMirror, err := repo_model.GetPushMirrorByID(ctx, id)
 	if err != nil {
 		return nil, err
 	}
 
-	for _, m := range pushMirrors {
-		if m.ID == id {
-			m.Repo = repo
-			return m, nil
-		}
+	if pushMirror.RepoID != repo.ID {
+		return nil, fmt.Errorf("PushMirror[%v] not associated to repository %v", id, repo)
 	}
-
-	return nil, fmt.Errorf("PushMirror[%v] not associated to repository %v", id, repo)
+	pushMirror.Repo = repo
+	return pushMirror, nil
 }

tests/integration/mirror_push_test.go

@@ -10,6 +10,7 @@ import (
 	"net/url"
 	"strconv"
 	"testing"
+	"time"
 
 	"code.gitea.io/gitea/models/db"
 	repo_model "code.gitea.io/gitea/models/repo"
@@ -18,6 +19,7 @@ import (
 	"code.gitea.io/gitea/modules/git"
 	"code.gitea.io/gitea/modules/gitrepo"
 	"code.gitea.io/gitea/modules/setting"
+	"code.gitea.io/gitea/modules/test"
 	gitea_context "code.gitea.io/gitea/services/context"
 	"code.gitea.io/gitea/services/migrations"
 	mirror_service "code.gitea.io/gitea/services/mirror"
@@ -119,3 +121,88 @@ func doRemovePushMirror(ctx APITestContext, address, username, password string,
 		assert.Contains(t, flashCookie.Value, "success")
 	}
 }
+
+func TestRepoSettingPushMirror(t *testing.T) {
+	defer tests.PrepareTestEnv(t)()
+
+	session := loginUser(t, "user2")
+
+	repoPrefix := "/user2/repo2"
+	repo2 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 2})
+
+	defer test.MockVariableValue(&setting.Migrations.AllowedDomains, "127.0.0.1")()
+	assert.NoError(t, migrations.Init())
+	defer func() {
+		migrations.Init()
+	}()
+
+	// visit repository setting page
+	req := NewRequest(t, "GET", repoPrefix+"/settings")
+	resp := session.MakeRequest(t, req, http.StatusOK)
+	htmlDoc := NewHTMLParser(t, resp.Body)
+
+	defer func() {
+		// avoid dirty mirror data once test failure
+		repo_model.DeletePushMirrors(db.DefaultContext, repo_model.PushMirrorOptions{
+			RepoID: repo2.ID,
+		})
+	}()
+
+	onGiteaRun(t, func(t *testing.T, u *url.URL) {
+		t.Run("Push Mirror Add", func(t *testing.T) {
+			req = NewRequestWithValues(t, "POST", repoPrefix+"/settings", map[string]string{
+				"_csrf":                htmlDoc.GetCSRF(),
+				"action":               "push-mirror-add",
+				"push_mirror_address":  u.String() + "/user1/repo1.git",
+				"push_mirror_interval": "0",
+			})
+			session.MakeRequest(t, req, http.StatusSeeOther)
+
+			flashCookie := session.GetCookie(gitea_context.CookieNameFlash)
+			assert.NotNil(t, flashCookie)
+			assert.Contains(t, flashCookie.Value, "success")
+
+			mirrors, cnt, err := repo_model.GetPushMirrorsByRepoID(db.DefaultContext, repo2.ID, db.ListOptions{})
+			assert.NoError(t, err)
+			assert.Len(t, mirrors, 1)
+			assert.EqualValues(t, 1, cnt)
+			assert.EqualValues(t, 0, mirrors[0].Interval)
+		})
+
+		mirrors, _, _ := repo_model.GetPushMirrorsByRepoID(db.DefaultContext, repo2.ID, db.ListOptions{})
+
+		t.Run("Push Mirror Update", func(t *testing.T) {
+			req := NewRequestWithValues(t, "POST", repoPrefix+"/settings", map[string]string{
+				"_csrf":                htmlDoc.GetCSRF(),
+				"action":               "push-mirror-update",
+				"push_mirror_id":       strconv.FormatInt(mirrors[0].ID, 10),
+				"push_mirror_interval": "10m0s",
+			})
+			session.MakeRequest(t, req, http.StatusSeeOther)
+
+			mirror, err := repo_model.GetPushMirrorByID(db.DefaultContext, mirrors[0].ID)
+			assert.NoError(t, err)
+			assert.EqualValues(t, 10*time.Minute, mirror.Interval)
+
+			req = NewRequestWithValues(t, "POST", repoPrefix+"/settings", map[string]string{
+				"_csrf":                htmlDoc.GetCSRF(),
+				"action":               "push-mirror-update",
+				"push_mirror_id":       strconv.FormatInt(9999, 10), // 1 is an mirror ID which is not exist
+				"push_mirror_interval": "10m0s",
+			})
+			session.MakeRequest(t, req, http.StatusNotFound)
+		})
+
+		t.Run("Push Mirror Remove", func(t *testing.T) {
+			req := NewRequestWithValues(t, "POST", repoPrefix+"/settings", map[string]string{
+				"_csrf":          htmlDoc.GetCSRF(),
+				"action":         "push-mirror-remove",
+				"push_mirror_id": strconv.FormatInt(mirrors[0].ID, 10),
+			})
+			session.MakeRequest(t, req, http.StatusSeeOther)
+
+			_, err := repo_model.GetPushMirrorByID(db.DefaultContext, mirrors[0].ID)
+			assert.Error(t, err)
+		})
+	})
+}