From 89569be3a6ab333c7e18bccbcfa8d1193efe12d8 Mon Sep 17 00:00:00 2001 From: Sofia Papagiannaki <1632407+papagian@users.noreply.github.com> Date: Thu, 2 Mar 2023 13:01:36 +0200 Subject: [PATCH] SQLStore: Fix wrong usage of xorm's insert functions in tests (#63850) * SQLStore: Fix InsertId * Prefs: Fix Insert return value * Fix tests * Add guidelines --- contribute/backend/style-guide.md | 6 ++++++ pkg/infra/db/sqlbuilder_test.go | 3 ++- pkg/infra/serverlock/serverlock_test.go | 5 +++-- pkg/services/preference/prefimpl/store.go | 1 + pkg/services/preference/prefimpl/xorm_store.go | 3 ++- pkg/services/sqlstore/session.go | 12 ++++++------ pkg/services/sqlstore/user.go | 2 +- 7 files changed, 21 insertions(+), 11 deletions(-) diff --git a/contribute/backend/style-guide.md b/contribute/backend/style-guide.md index 31ede76a9b8..08d1eb8ff99 100644 --- a/contribute/backend/style-guide.md +++ b/contribute/backend/style-guide.md @@ -190,6 +190,12 @@ for context. If a column, or column combination, should be unique, add a corresponding uniqueness constraint through a migration. +### Usage of XORM Session.Insert() and Session.InsertOne() + +The [Session.Insert()](https://pkg.go.dev/github.com/go-xorm/xorm#Session.Insert) and [Session.InsertOne()](https://pkg.go.dev/github.com/go-xorm/xorm#Session.InsertOne) are poorly documented and return the number of affected rows contrary to a common mistake that they return the newly introduced primary key. Therefore, contributors should be extra cautious when using them. + +The same applies for the respective [Engine.Insert()](https://pkg.go.dev/github.com/go-xorm/xorm#Engine.Insert) and [Engine.InsertOne()](https://pkg.go.dev/github.com/go-xorm/xorm#Engine.InsertOne) + ## JSON The simplejson package is used a lot throughout the backend codebase, diff --git a/pkg/infra/db/sqlbuilder_test.go b/pkg/infra/db/sqlbuilder_test.go index 8bc1d506627..ce88c870a56 100644 --- a/pkg/infra/db/sqlbuilder_test.go +++ b/pkg/infra/db/sqlbuilder_test.go @@ -217,7 +217,8 @@ func createDummyUser(t *testing.T, sqlStore DB) *user.User { err := sqlStore.WithDbSession(context.Background(), func(sess *Session) error { sess.UseBool("is_admin") var err error - id, err = sess.Insert(usr) + _, err = sess.Insert(usr) + id = usr.ID return err }) require.NoError(t, err) diff --git a/pkg/infra/serverlock/serverlock_test.go b/pkg/infra/serverlock/serverlock_test.go index 05ba7e60d96..79212a02c57 100644 --- a/pkg/infra/serverlock/serverlock_test.go +++ b/pkg/infra/serverlock/serverlock_test.go @@ -106,9 +106,10 @@ func TestLockAndRelease(t *testing.T) { // inserting a row with lock in the past err := sl.SQLStore.WithTransactionalDbSession(context.Background(), func(sess *db.Session) error { - r, err := sess.Insert(lock) + affectedRows, err := sess.Insert(&lock) require.NoError(t, err) - require.Equal(t, int64(1), r) + require.Equal(t, int64(1), affectedRows) + require.Equal(t, int64(1), lock.Id) return nil }) require.NoError(t, err) diff --git a/pkg/services/preference/prefimpl/store.go b/pkg/services/preference/prefimpl/store.go index c1fbac86577..7c8575a8a07 100644 --- a/pkg/services/preference/prefimpl/store.go +++ b/pkg/services/preference/prefimpl/store.go @@ -9,6 +9,7 @@ import ( type store interface { Get(context.Context, *pref.Preference) (*pref.Preference, error) List(context.Context, *pref.Preference) ([]*pref.Preference, error) + // Insert adds a new preference and returns its sequential ID Insert(context.Context, *pref.Preference) (int64, error) Update(context.Context, *pref.Preference) error DeleteByUser(context.Context, int64) error diff --git a/pkg/services/preference/prefimpl/xorm_store.go b/pkg/services/preference/prefimpl/xorm_store.go index 90d256bbcd6..b431d395f7d 100644 --- a/pkg/services/preference/prefimpl/xorm_store.go +++ b/pkg/services/preference/prefimpl/xorm_store.go @@ -73,7 +73,8 @@ func (s *sqlStore) Insert(ctx context.Context, cmd *pref.Preference) (int64, err var ID int64 var err error err = s.db.WithTransactionalDbSession(ctx, func(sess *db.Session) error { - ID, err = sess.Insert(cmd) + _, err = sess.Insert(cmd) + ID = cmd.ID return err }) return ID, err diff --git a/pkg/services/sqlstore/session.go b/pkg/services/sqlstore/session.go index 062d166963f..b7848f2da62 100644 --- a/pkg/services/sqlstore/session.go +++ b/pkg/services/sqlstore/session.go @@ -126,21 +126,21 @@ func (ss *SQLStore) withDbSession(ctx context.Context, engine *xorm.Engine, call return retryer.Retry(ss.retryOnLocks(ctx, callback, sess, retry), ss.dbCfg.QueryRetries, time.Millisecond*time.Duration(10), time.Second) } -func (sess *DBSession) InsertId(bean interface{}, dialect migrator.Dialect) (int64, error) { +func (sess *DBSession) InsertId(bean interface{}, dialect migrator.Dialect) error { table := sess.DB().Mapper.Obj2Table(getTypeName(bean)) if err := dialect.PreInsertId(table, sess.Session); err != nil { - return 0, err + return err } - id, err := sess.Session.InsertOne(bean) + _, err := sess.Session.InsertOne(bean) if err != nil { - return 0, err + return err } if err := dialect.PostInsertId(table, sess.Session); err != nil { - return 0, err + return err } - return id, nil + return nil } func (sess *DBSession) WithReturningID(driverName string, query string, args []interface{}) (int64, error) { diff --git a/pkg/services/sqlstore/user.go b/pkg/services/sqlstore/user.go index 266c88424fa..2fe7a4c7a96 100644 --- a/pkg/services/sqlstore/user.go +++ b/pkg/services/sqlstore/user.go @@ -172,7 +172,7 @@ func (ss *SQLStore) getOrCreateOrg(sess *DBSession, orgName string) (int64, erro org.Updated = time.Now() if org.ID != 0 { - if _, err := sess.InsertId(&org, ss.Dialect); err != nil { + if err := sess.InsertId(&org, ss.Dialect); err != nil { return 0, err } } else {