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

@@ -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,

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)

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)

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

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

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) {

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 {