feat: avoid updating all columns (#9572)

This patch contains two fixes/enhancements to two functions that were updating all columns of the `access_token` and `repository` table when they were only updating a select few columns. Within Codeberg we saw these two queries quite often when something problematic with the database was going on, likely because of this all columns update pattern. `UpdateAccessToken` is removed and a new function `UpdateLastUsed` was added, for `updateRepoRunsNumbers` we can simply add which columns we want to have updated in that query.

It's likely there are more of such queries, but these were the ones being executed often.

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/9572
Reviewed-by: Michael Kriese <michael.kriese@gmx.de>
Reviewed-by: Otto <otto@codeberg.org>
Co-authored-by: Gusted <postmaster@gusted.xyz>
Co-committed-by: Gusted <postmaster@gusted.xyz>

models/actions/run.go

@@ -218,6 +218,7 @@ func updateRepoRunsNumbers(ctx context.Context, repo *repo_model.Repository) err
 				),
 				),
 		).
+		Cols("num_action_runs", "num_closed_action_runs").
 		Update(repo)
 	return err
 }

models/actions/run_test.go

@@ -6,7 +6,11 @@ package actions
 import (
 	"testing"
 
+	repo_model "forgejo.org/models/repo"
+	"forgejo.org/models/unittest"
+
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 func TestGetRunBefore(t *testing.T) {
@@ -36,3 +40,49 @@ func TestSetDefaultConcurrencyGroup(t *testing.T) {
 	run.SetDefaultConcurrencyGroup()
 	assert.Equal(t, "refs/heads/main_testing_pull_request__auto", run.ConcurrencyGroup)
 }
+
+func TestUpdateRepoRunsNumbers(t *testing.T) {
+	require.NoError(t, unittest.PrepareTestDatabase())
+
+	t.Run("Normal", func(t *testing.T) {
+		t.Run("Repo 1", func(t *testing.T) {
+			repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1})
+
+			require.NoError(t, updateRepoRunsNumbers(t.Context(), repo))
+
+			repo = unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1})
+			assert.Equal(t, 1, repo.NumActionRuns)
+			assert.Equal(t, 1, repo.NumClosedActionRuns)
+		})
+
+		t.Run("Repo 4", func(t *testing.T) {
+			repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 4})
+
+			require.NoError(t, updateRepoRunsNumbers(t.Context(), repo))
+
+			repo = unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 4})
+			assert.Equal(t, 4, repo.NumActionRuns)
+			assert.Equal(t, 4, repo.NumClosedActionRuns)
+		})
+
+		t.Run("Repo 63", func(t *testing.T) {
+			repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 63})
+
+			require.NoError(t, updateRepoRunsNumbers(t.Context(), repo))
+
+			repo = unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 63})
+			assert.Equal(t, 3, repo.NumActionRuns)
+			assert.Equal(t, 2, repo.NumClosedActionRuns)
+		})
+	})
+
+	t.Run("Columns specifc", func(t *testing.T) {
+		repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1})
+		repo.Name = "ishouldnotbeupdated"
+
+		require.NoError(t, updateRepoRunsNumbers(t.Context(), repo))
+
+		repo = unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1})
+		assert.Equal(t, "repo1", repo.Name)
+	})
+}

models/auth/access_token.go

@@ -127,6 +127,13 @@ func (t *AccessToken) DisplayPublicOnly() bool {
 	return publicOnly
 }
 
+// UpdateLastUsed updates the time this token was last used to now.
+func (t *AccessToken) UpdateLastUsed(ctx context.Context) error {
+	t.UpdatedUnix = timeutil.TimeStampNow()
+	_, err := db.GetEngine(ctx).ID(t.ID).Cols("updated_unix").NoAutoTime().Update(t)
+	return err
+}
+
 func getAccessTokenIDFromCache(token string) int64 {
 	if successfulAccessTokenCache == nil {
 		return 0
@@ -220,12 +227,6 @@ func (opts ListAccessTokensOptions) ToOrders() string {
 	return "created_unix DESC"
 }
 
-// UpdateAccessToken updates information of access token.
-func UpdateAccessToken(ctx context.Context, t *AccessToken) error {
-	_, err := db.GetEngine(ctx).ID(t.ID).AllCols().Update(t)
-	return err
-}
-
 // DeleteAccessTokenByID deletes access token by given ID.
 func DeleteAccessTokenByID(ctx context.Context, id, userID int64) error {
 	cnt, err := db.GetEngine(ctx).ID(id).Delete(&AccessToken{
@@ -258,5 +259,6 @@ func RegenerateAccessTokenByID(ctx context.Context, id, userID int64) (*AccessTo
 	// Reset the creation time, token is unused
 	t.UpdatedUnix = timeutil.TimeStampNow()
 
-	return t, UpdateAccessToken(ctx, t)
+	_, err = db.GetEngine(ctx).ID(t.ID).Cols("token_salt", "token", "token_hash", "token_last_eight", "updated_unix").NoAutoTime().Update(t)
+	return t, err
 }

models/auth/access_token_test.go

@@ -5,10 +5,12 @@ package auth_test
 
 import (
 	"testing"
+	"time"
 
 	auth_model "forgejo.org/models/auth"
 	"forgejo.org/models/db"
 	"forgejo.org/models/unittest"
+	"forgejo.org/modules/timeutil"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
@@ -107,14 +109,17 @@ func TestListAccessTokens(t *testing.T) {
 	assert.Empty(t, tokens)
 }
 
-func TestUpdateAccessToken(t *testing.T) {
-	require.NoError(t, unittest.PrepareTestDatabase())
-	token, err := auth_model.GetAccessTokenBySHA(db.DefaultContext, "4c6f36e6cf498e2a448662f915d932c09c5a146c")
-	require.NoError(t, err)
-	token.Name = "Token Z"
+func TestUpdateLastUsed(t *testing.T) {
+	timeutil.MockSet(time.Date(2021, 1, 1, 0, 0, 0, 0, time.UTC))
+	defer timeutil.MockUnset()
 
-	require.NoError(t, auth_model.UpdateAccessToken(db.DefaultContext, token))
-	unittest.AssertExistsAndLoadBean(t, token)
+	require.NoError(t, unittest.PrepareTestDatabase())
+	token := unittest.AssertExistsAndLoadBean(t, &auth_model.AccessToken{ID: 2})
+
+	require.NoError(t, token.UpdateLastUsed(t.Context()))
+
+	token = unittest.AssertExistsAndLoadBean(t, &auth_model.AccessToken{ID: 2})
+	assert.Equal(t, timeutil.TimeStampNow(), token.UpdatedUnix)
 }
 
 func TestDeleteAccessTokenByID(t *testing.T) {