fix(db): only update fields when necessary for repo hot path writes

api/webhook/post.go

@@ -8,8 +8,9 @@ import (
 	"errors"
 	"fmt"
 	"io"
+	"maps"
 	"net/http"
-	"reflect"
+	"slices"
 	"strings"
 	"time"
 
@@ -258,16 +259,38 @@ func PostWebhook(c *gin.Context) {
 	}
 
 	// update custom props, topics, default branch on all events (some repos may have these fields set prior to installing Vela)
-	repo.SetBranch(r.GetBranch())
-	repo.SetCustomProps(r.GetCustomProps())
-	repo.SetTopics(r.GetTopics())
+	repoMetaUpdates := &types.Repo{ID: repo.ID}
+	changed := false
 
-	repo, err = db.UpdateRepo(ctx, repo)
-	if err != nil {
-		retErr := fmt.Errorf("%s: failed to update repo %s: %w", baseErr, r.GetFullName(), err)
-		util.HandleError(c, http.StatusInternalServerError, retErr)
+	if !maps.Equal(r.GetCustomProps(), repo.GetCustomProps()) {
+		repo.SetCustomProps(r.GetCustomProps())
+		repoMetaUpdates.SetCustomProps(r.GetCustomProps())
 
-		return
+		changed = true
+	}
+
+	if !slices.Equal(r.GetTopics(), repo.GetTopics()) {
+		repo.SetTopics(r.GetTopics())
+		repoMetaUpdates.SetTopics(r.GetTopics())
+
+		changed = true
+	}
+
+	if r.GetBranch() != repo.GetBranch() {
+		repo.SetBranch(r.GetBranch())
+		repoMetaUpdates.SetBranch(r.GetBranch())
+
+		changed = true
+	}
+
+	if changed {
+		err = db.PartialUpdateRepo(ctx, repoMetaUpdates)
+		if err != nil {
+			retErr := fmt.Errorf("%s: failed to update repo %s: %w", baseErr, r.GetFullName(), err)
+			util.HandleError(c, http.StatusInternalServerError, retErr)
+
+			return
+		}
 	}
 
 	l.Debugf(`build author: %s,
@@ -338,7 +361,8 @@ func PostWebhook(c *gin.Context) {
 			err = fmt.Errorf("unable to create webhook %s/%d: %w", r.GetFullName(), h.GetNumber(), err)
 
 			// check if the retry limit has been exceeded
-			if i < retryLimit {
+			if i < retryLimit-1 {
+				logrus.Infof("retrying hook creation after attempt %d error: %v", i+1, err)
 				// continue to the next iteration of the loop
 				continue
 			}
@@ -713,25 +737,31 @@ func handleRepositoryEvent(ctx context.Context, l *logrus.Entry, db database.Int
 
 		h.SetRepo(dbRepo)
 
+		repoMetaUpdates := &types.Repo{ID: dbRepo.ID}
+
 		// the only edits to a repo that impact Vela are to these three fields
-		if !strings.EqualFold(dbRepo.GetBranch(), r.GetBranch()) {
+		if dbRepo.GetBranch() != r.GetBranch() {
 			dbRepo.SetBranch(r.GetBranch())
+			repoMetaUpdates.SetBranch(r.GetBranch())
 		}
 
 		if dbRepo.GetActive() != r.GetActive() {
 			dbRepo.SetActive(r.GetActive())
+			repoMetaUpdates.SetActive(r.GetActive())
 		}
 
-		if !reflect.DeepEqual(dbRepo.GetTopics(), r.GetTopics()) {
+		if !slices.Equal(dbRepo.GetTopics(), r.GetTopics()) {
 			dbRepo.SetTopics(r.GetTopics())
+			repoMetaUpdates.SetTopics(r.GetTopics())
 		}
 
-		if !reflect.DeepEqual(dbRepo.GetCustomProps(), r.GetCustomProps()) {
+		if !maps.Equal(dbRepo.GetCustomProps(), r.GetCustomProps()) {
 			dbRepo.SetCustomProps(r.GetCustomProps())
+			repoMetaUpdates.SetCustomProps(r.GetCustomProps())
 		}
 
 		// update repo object in the database after applying edits
-		dbRepo, err := db.UpdateRepo(ctx, dbRepo)
+		err := db.PartialUpdateRepo(ctx, repoMetaUpdates)
 		if err != nil {
 			retErr := fmt.Errorf("%s: failed to update repo %s: %w", baseErr, r.GetFullName(), err)
 

database/integration_test.go

@@ -1651,6 +1651,35 @@ func testRepos(t *testing.T, db Interface, resources *Resources) {
 	methods["UpdateRepo"] = true
 	methods["GetRepo"] = true
 
+	updateRepo := new(api.Repo)
+	updateRepo.SetID(resources.Repos[0].GetID())
+	updateRepo.SetBranch("dev")
+
+	err = db.PartialUpdateRepo(context.TODO(), updateRepo)
+	if err != nil {
+		t.Errorf("unable to partially update repo: %v", err)
+	}
+
+	got, err := db.GetRepo(context.TODO(), resources.Repos[0].GetID())
+	if err != nil {
+		t.Errorf("unable to get repo %d by ID: %v", resources.Repos[0].GetID(), err)
+	}
+
+	if got.GetBranch() != "dev" {
+		t.Errorf("PartialUpdateRepo() branch is %v, want %v", got.GetBranch(), "dev")
+	}
+
+	resetUpdateRepo := new(api.Repo)
+	resetUpdateRepo.SetID(resources.Repos[0].GetID())
+	resetUpdateRepo.SetBranch("main")
+
+	err = db.PartialUpdateRepo(context.TODO(), resetUpdateRepo)
+	if err != nil {
+		t.Errorf("unable to partially update repo: %v", err)
+	}
+
+	methods["PartialUpdateRepo"] = true
+
 	for _, build := range resources.Builds {
 		err = db.DeleteBuild(context.TODO(), build)
 		if err != nil {

database/repo/create.go

@@ -4,6 +4,7 @@ package repo
 
 import (
 	"context"
+	"database/sql"
 	"fmt"
 
 	"github.com/sirupsen/logrus"
@@ -29,6 +30,14 @@ func (e *Engine) CreateRepo(ctx context.Context, r *api.Repo) (*api.Repo, error)
 		return nil, err
 	}
 
+	if !repo.Counter.Valid {
+		repo.Counter = sql.NullInt64{Int64: 0, Valid: true}
+	}
+
+	if !repo.HookCounter.Valid {
+		repo.HookCounter = sql.NullInt64{Int64: 0, Valid: true}
+	}
+
 	// encrypt the fields for the repo
 	err = repo.Encrypt(e.config.EncryptionKey)
 	if err != nil {

database/repo/create_test.go

@@ -42,7 +42,7 @@ func TestRepo_Engine_CreateRepo(t *testing.T) {
 	_mock.ExpectQuery(`INSERT INTO "repos"
 ("user_id","hash","org","name","full_name","link","clone","branch","topics","build_limit","timeout","counter","hook_counter","visibility","private","trusted","active","allow_events","merge_queue_events","pipeline_type","previous_name","approve_build","approval_timeout","install_id","custom_props","id")
 VALUES ($1,$2,$3,$4,$5,$6,$7,$8,$9,$10,$11,$12,$13,$14,$15,$16,$17,$18,$19,$20,$21,$22,$23,$24,$25,$26) RETURNING "id"`).
-		WithArgs(1, AnyArgument{}, "foo", "bar", "foo/bar", nil, nil, nil, AnyArgument{}, AnyArgument{}, AnyArgument{}, AnyArgument{}, AnyArgument{}, "public", false, false, false, nil, nil, "yaml", "oldName", nil, nil, 0, `{"foo":"bar"}`, 1).
+		WithArgs(1, AnyArgument{}, "foo", "bar", "foo/bar", "", "", "", AnyArgument{}, AnyArgument{}, AnyArgument{}, AnyArgument{}, AnyArgument{}, "public", false, false, false, 0, nil, "yaml", "oldName", "", 0, 0, `{"foo":"bar"}`, 1).
 		WillReturnRows(_rows)
 
 	_sqlite := testSqlite(t)

database/repo/interface.go

@@ -50,4 +50,6 @@ type RepoInterface interface {
 	ListReposForUser(context.Context, *api.User, string, map[string]any, int, int) ([]*api.Repo, error)
 	// UpdateRepo defines a function that updates an existing repo.
 	UpdateRepo(context.Context, *api.Repo) (*api.Repo, error)
+	// PartialUpdateRepo defines a function that updates an existing repo with a map of fields to update.
+	PartialUpdateRepo(context.Context, *api.Repo) error
 }

database/repo/update_partial.go

@@ -0,0 +1,33 @@
+// SPDX-License-Identifier: Apache-2.0
+
+package repo
+
+import (
+	"context"
+	"fmt"
+
+	"github.com/sirupsen/logrus"
+
+	api "github.com/go-vela/server/api/types"
+	"github.com/go-vela/server/constants"
+	"github.com/go-vela/server/database/types"
+)
+
+// UpdateRepo updates an existing repo in the database.
+func (e *Engine) PartialUpdateRepo(ctx context.Context, r *api.Repo) error {
+	e.logger.WithFields(logrus.Fields{
+		"id": r.GetID(),
+	}).Tracef("updating repo %d", r.GetID())
+
+	if r.GetID() == 0 {
+		return fmt.Errorf("repo ID must be set")
+	}
+
+	repo := types.RepoFromAPI(r)
+
+	// send query to the database
+	return e.client.
+		WithContext(ctx).
+		Table(constants.TableRepo).
+		Updates(repo).Error
+}

database/repo/update_partial_test.go

@@ -0,0 +1,93 @@
+// SPDX-License-Identifier: Apache-2.0
+
+package repo
+
+import (
+	"context"
+	"testing"
+
+	"github.com/DATA-DOG/go-sqlmock"
+
+	api "github.com/go-vela/server/api/types"
+	"github.com/go-vela/server/constants"
+	"github.com/go-vela/server/database/testutils"
+)
+
+func TestRepo_Engine_PartialUpdateRepo(t *testing.T) {
+	// setup types
+	_repo := testutils.APIRepo()
+	_repo.SetID(1)
+	_repo.GetOwner().SetID(1)
+	_repo.SetHash("baz")
+	_repo.SetOrg("foo")
+	_repo.SetName("bar")
+	_repo.SetFullName("foo/bar")
+	_repo.SetVisibility("public")
+	_repo.SetPipelineType("yaml")
+	_repo.SetPreviousName("oldName")
+	_repo.SetApproveBuild(constants.ApproveForkAlways)
+	_repo.SetTopics([]string{})
+	_repo.SetAllowEvents(api.NewEventsFromMask(1))
+	_repo.SetApprovalTimeout(5)
+	_repo.SetCustomProps(map[string]any{"foo": "bar"})
+
+	updates := new(api.Repo)
+	updates.SetID(1)
+	updates.SetTopics([]string{"topic1", "topic2"})
+	updates.SetVisibility("private")
+
+	_postgres, _mock := testPostgres(t)
+
+	defer func() { _sql, _ := _postgres.client.DB(); _sql.Close() }()
+
+	// ensure the mock expects the query
+	_mock.ExpectExec(`UPDATE "repos" SET "topics"=$1,"visibility"=$2 WHERE "id" = $3`).
+		WithArgs(`{"topic1","topic2"}`, "private", 1).
+		WillReturnResult(sqlmock.NewResult(1, 1))
+
+	_sqlite := testSqlite(t)
+
+	defer func() { _sql, _ := _sqlite.client.DB(); _sql.Close() }()
+
+	_, err := _sqlite.CreateRepo(context.TODO(), _repo)
+	if err != nil {
+		t.Errorf("unable to create test repo for sqlite: %v", err)
+	}
+
+	// setup tests
+	tests := []struct {
+		failure  bool
+		name     string
+		database *Engine
+	}{
+		{
+			failure:  false,
+			name:     "postgres",
+			database: _postgres,
+		},
+		{
+			failure:  false,
+			name:     "sqlite3",
+			database: _sqlite,
+		},
+	}
+
+	// run tests
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			err := test.database.PartialUpdateRepo(context.TODO(), updates)
+
+			if test.failure {
+				if err == nil {
+					t.Errorf("PartialUpdateRepo for %s should have returned err", test.name)
+				}
+
+				return
+			}
+
+			if err != nil {
+				t.Errorf("PartialUpdateRepo for %s returned err: %v", test.name, err)
+			}
+		})
+	}
+}

database/repo/update_test.go

@@ -40,7 +40,7 @@ func TestRepo_Engine_UpdateRepo(t *testing.T) {
 	_mock.ExpectExec(`UPDATE "repos"
 SET "user_id"=$1,"hash"=$2,"org"=$3,"name"=$4,"full_name"=$5,"link"=$6,"clone"=$7,"branch"=$8,"topics"=$9,"build_limit"=$10,"timeout"=$11,"counter"=$12,"hook_counter"=$13,"visibility"=$14,"private"=$15,"trusted"=$16,"active"=$17,"allow_events"=$18,"merge_queue_events"=$19,"pipeline_type"=$20,"previous_name"=$21,"approve_build"=$22,"approval_timeout"=$23,"install_id"=$24,"custom_props"=$25
 WHERE "id" = $26`).
-		WithArgs(1, AnyArgument{}, "foo", "bar", "foo/bar", nil, nil, nil, AnyArgument{}, AnyArgument{}, AnyArgument{}, AnyArgument{}, AnyArgument{}, "public", false, false, false, 1, nil, "yaml", "oldName", constants.ApproveForkAlways, 5, 0, `{"foo":"bar"}`, 1).
+		WithArgs(1, AnyArgument{}, "foo", "bar", "foo/bar", "", "", "", AnyArgument{}, AnyArgument{}, AnyArgument{}, AnyArgument{}, AnyArgument{}, "public", false, false, false, 1, nil, "yaml", "oldName", constants.ApproveForkAlways, 5, 0, `{"foo":"bar"}`, 1).
 		WillReturnResult(sqlmock.NewResult(1, 1))
 
 	_sqlite := testSqlite(t)