From 7f4e851f06d33dda1bfe0b1f7d93f4cf6ba4d617 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Sun, 4 Sep 2022 21:45:44 +0100 Subject: [PATCH 01/29] Make TryInsert functions within the packages module try to insert first The TryInsert* functions within the packages models make incorrect assumptions about transactional isolation within most databases. It is perfectly possible for a SELECT to return nothing but an INSERT fail with a duplicate in most DBs as it is only INSERT that the locking occurs. This PR changes the code to simply try to insert first and if there is an error then attempt to SELECT from the table. If the SELECT works then the INSERT error is assumed to have been related to the unique constraint failure. This technique avoids us having to parse the error returned from the DBs as these are varied and different. If the SELECT fails then the INSERT error is returned to the user. Fix #19586 Signed-off-by: Andrew Thornton --- models/packages/package.go | 13 +++++-------- models/packages/package_file.go | 12 ++++-------- models/packages/package_version.go | 12 ++++-------- 3 files changed, 13 insertions(+), 24 deletions(-) diff --git a/models/packages/package.go b/models/packages/package.go index e39a7c4e411d4..f9bd6c7657653 100644 --- a/models/packages/package.go +++ b/models/packages/package.go @@ -128,14 +128,11 @@ func TryInsertPackage(ctx context.Context, p *Package) (*Package, error) { LowerName: p.LowerName, } - has, err := e.Get(key) - if err != nil { - return nil, err - } - if has { - return key, ErrDuplicatePackage - } - if _, err = e.Insert(p); err != nil { + if _, err := e.Insert(p); err != nil { + // Try to get the key again + if has, _ := e.Get(key); has { + return key, ErrDuplicatePackage + } return nil, err } return p, nil diff --git a/models/packages/package_file.go b/models/packages/package_file.go index 8f304ce8ac42a..a2e007c7dc160 100644 --- a/models/packages/package_file.go +++ b/models/packages/package_file.go @@ -53,14 +53,10 @@ func TryInsertFile(ctx context.Context, pf *PackageFile) (*PackageFile, error) { CompositeKey: pf.CompositeKey, } - has, err := e.Get(key) - if err != nil { - return nil, err - } - if has { - return pf, ErrDuplicatePackageFile - } - if _, err = e.Insert(pf); err != nil { + if _, err := e.Insert(pf); err != nil { + if has, _ := e.Get(key); has { + return pf, ErrDuplicatePackageFile + } return nil, err } return pf, nil diff --git a/models/packages/package_version.go b/models/packages/package_version.go index 5479bae1c29fc..8bb72f6e1f685 100644 --- a/models/packages/package_version.go +++ b/models/packages/package_version.go @@ -46,14 +46,10 @@ func GetOrInsertVersion(ctx context.Context, pv *PackageVersion) (*PackageVersio LowerVersion: pv.LowerVersion, } - has, err := e.Get(key) - if err != nil { - return nil, err - } - if has { - return key, ErrDuplicatePackageVersion - } - if _, err = e.Insert(pv); err != nil { + if _, err := e.Insert(pv); err != nil { + if has, _ := e.Get(key); has { + return key, ErrDuplicatePackageVersion + } return nil, err } return pv, nil From b29ab42b617f953e91864e844807d2d54493dea0 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Tue, 6 Sep 2022 21:25:31 +0100 Subject: [PATCH 02/29] partial still broken Signed-off-by: Andrew Thornton --- models/db/common.go | 150 +++++++++++++++++++++++++++++ models/packages/package.go | 22 +++-- models/packages/package_file.go | 23 +++-- models/packages/package_version.go | 23 +++-- 4 files changed, 197 insertions(+), 21 deletions(-) diff --git a/models/db/common.go b/models/db/common.go index 1a59a8b5c697f..a31c066ef11db 100644 --- a/models/db/common.go +++ b/models/db/common.go @@ -5,12 +5,16 @@ package db import ( + "context" + "fmt" + "reflect" "strings" "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/util" "xorm.io/builder" + "xorm.io/xorm/schemas" ) // BuildCaseInsensitiveLike returns a condition to check if the given value is like the given key case-insensitively. @@ -21,3 +25,149 @@ func BuildCaseInsensitiveLike(key, value string) builder.Cond { } return builder.Like{"UPPER(" + key + ")", strings.ToUpper(value)} } + +func InsertOnConflictDoNothing(ctx context.Context, bean interface{}) (int64, error) { + e := GetEngine(ctx) + + tableName := x.TableName(bean, true) + table, err := x.TableInfo(bean) + if err != nil { + return 0, err + } + + autoIncrCol := table.AutoIncrColumn() + if autoIncrCol == nil { + return 0, fmt.Errorf("this function requires an autoincrement column") + } + + cols := table.Columns() + colNames := make([]string, 0, len(cols)) + args := make([]interface{}, 1, len(cols)) + + val := reflect.ValueOf(bean) + elem := val.Elem() + for _, col := range cols { + if fieldIdx := col.FieldIndex; fieldIdx != nil { + fieldVal := elem.FieldByIndex(fieldIdx) + if fieldVal.IsZero() { + continue + } + colNames = append(colNames, col.Name) + args = append(args, fieldVal.Interface()) + } + } + + if len(colNames) == 0 { + return 0, fmt.Errorf("empty bean") + } + + uniqueCols := make([]string, 0, len(cols)) + uniqueArgs := make([]interface{}, 1, len(cols)) + for _, index := range table.Indexes { + if index.Type != schemas.UniqueType { + continue + } + indexCol: + for _, iCol := range index.Cols { + for _, uCol := range uniqueCols { + if uCol == iCol { + continue indexCol + } + } + for i, col := range colNames { + if col == iCol { + uniqueCols = append(uniqueCols, col) + uniqueArgs = append(uniqueArgs, args[i+1]) + continue indexCol + } + } + } + } + + if len(uniqueCols) == 0 { + return 0, fmt.Errorf("empty bean") + } + + sb := &strings.Builder{} + switch { + case setting.Database.UseSQLite3 || setting.Database.UsePostgreSQL || setting.Database.UseMySQL: + _, _ = sb.WriteString("INSERT INTO ") + _, _ = sb.WriteString(x.Dialect().Quoter().Quote(tableName)) + _, _ = sb.WriteString(" (") + _, _ = sb.WriteString(colNames[0]) + for _, colName := range colNames[1:] { + _, _ = sb.WriteString(",") + _, _ = sb.WriteString(colName) + } + _, _ = sb.WriteString(") VALUES (") + _, _ = sb.WriteString("?") + for range colNames[1:] { + _, _ = sb.WriteString(",?") + } + switch { + case setting.Database.UseSQLite3 || setting.Database.UsePostgreSQL: + _, _ = sb.WriteString(") ON CONFLICT DO NOTHING") + case setting.Database.UseMySQL: + _, _ = sb.WriteString(") ON CONFLICT DO DUPLICATE KEY ") + _, _ = sb.WriteString(autoIncrCol.Name) + _, _ = sb.WriteString(" = ") + _, _ = sb.WriteString(autoIncrCol.Name) + } + case setting.Database.UseMSSQL: + _, _ = sb.WriteString("MERGE ") + _, _ = sb.WriteString(x.Dialect().Quoter().Quote(tableName)) + _, _ = sb.WriteString(" WITH (HOLDLOCK) AS target USING (SELECT ") + + _, _ = sb.WriteString("? AS ") + _, _ = sb.WriteString(uniqueCols[0]) + for _, uniqueCol := range uniqueCols[1:] { + _, _ = sb.WriteString(", ? AS ") + _, _ = sb.WriteString(uniqueCol) + } + _, _ = sb.WriteString(") AS src ON src.") + _, _ = sb.WriteString(uniqueCols[0]) + _, _ = sb.WriteString("= target.") + _, _ = sb.WriteString(uniqueCols[0]) + for _, uniqueCol := range uniqueCols[1:] { + _, _ = sb.WriteString(" AND src.") + _, _ = sb.WriteString(uniqueCol) + _, _ = sb.WriteString("= target.") + _, _ = sb.WriteString(uniqueCols[0]) + } + _, _ = sb.WriteString(" WHEN NOT MATCHED THEN INSERT (") + _, _ = sb.WriteString(colNames[0]) + for _, colName := range colNames[1:] { + _, _ = sb.WriteString(",") + _, _ = sb.WriteString(colName) + } + _, _ = sb.WriteString(") VALUES (") + _, _ = sb.WriteString("?") + for range colNames[1:] { + _, _ = sb.WriteString(",?") + } + _, _ = sb.WriteString(")") + args = append(uniqueArgs, args[1:]...) + default: + return 0, fmt.Errorf("database type not supported") + } + args[0] = sb.String() + res, err := e.Exec(args...) + if err != nil { + return 0, err + } + + n, err := res.RowsAffected() + if err != nil { + return n, err + } + + if n != 0 { + id, err := res.LastInsertId() + if err != nil { + return n, err + } + elem.FieldByName(autoIncrCol.FieldName).SetInt(id) + } + + return res.RowsAffected() +} diff --git a/models/packages/package.go b/models/packages/package.go index f9bd6c7657653..8c8b6621c5d94 100644 --- a/models/packages/package.go +++ b/models/packages/package.go @@ -120,7 +120,13 @@ type Package struct { // TryInsertPackage inserts a package. If a package exists already, ErrDuplicatePackage is returned func TryInsertPackage(ctx context.Context, p *Package) (*Package, error) { - e := db.GetEngine(ctx) + n, err := db.InsertOnConflictDoNothing(ctx, p) + if err != nil { + return nil, err + } + if n != 0 { + return p, nil + } key := &Package{ OwnerID: p.OwnerID, @@ -128,14 +134,16 @@ func TryInsertPackage(ctx context.Context, p *Package) (*Package, error) { LowerName: p.LowerName, } - if _, err := e.Insert(p); err != nil { - // Try to get the key again - if has, _ := e.Get(key); has { - return key, ErrDuplicatePackage + has, err := db.GetEngine(ctx).Get(key) + if has { + if n == 0 { + err = ErrDuplicatePackage } - return nil, err + return key, err + } else if err == nil { + return TryInsertPackage(ctx, p) } - return p, nil + return nil, err } // DeletePackageByID deletes a package by id diff --git a/models/packages/package_file.go b/models/packages/package_file.go index a2e007c7dc160..162df3d2396c1 100644 --- a/models/packages/package_file.go +++ b/models/packages/package_file.go @@ -45,21 +45,30 @@ type PackageFile struct { // TryInsertFile inserts a file. If the file exists already ErrDuplicatePackageFile is returned func TryInsertFile(ctx context.Context, pf *PackageFile) (*PackageFile, error) { - e := db.GetEngine(ctx) + n, err := db.InsertOnConflictDoNothing(ctx, pf) + if err != nil { + return nil, err + } + if n != 0 { + return pf, nil + } key := &PackageFile{ VersionID: pf.VersionID, LowerName: pf.LowerName, CompositeKey: pf.CompositeKey, } - - if _, err := e.Insert(pf); err != nil { - if has, _ := e.Get(key); has { - return pf, ErrDuplicatePackageFile + has, err := db.GetEngine(ctx).Get(key) + if has { + if n == 0 { + err = ErrDuplicatePackageFile } - return nil, err + return key, err + } else if err == nil { + return TryInsertFile(ctx, pf) } - return pf, nil + + return key, err } // GetFilesByVersionID gets all files of a version diff --git a/models/packages/package_version.go b/models/packages/package_version.go index 8bb72f6e1f685..a0ef1da185d18 100644 --- a/models/packages/package_version.go +++ b/models/packages/package_version.go @@ -39,20 +39,29 @@ type PackageVersion struct { // GetOrInsertVersion inserts a version. If the same version exist already ErrDuplicatePackageVersion is returned func GetOrInsertVersion(ctx context.Context, pv *PackageVersion) (*PackageVersion, error) { - e := db.GetEngine(ctx) - key := &PackageVersion{ PackageID: pv.PackageID, LowerVersion: pv.LowerVersion, } - if _, err := e.Insert(pv); err != nil { - if has, _ := e.Get(key); has { - return key, ErrDuplicatePackageVersion - } + n, err := db.InsertOnConflictDoNothing(ctx, pv) + if err != nil { return nil, err } - return pv, nil + if n != 0 { + return pv, nil + } + + has, err := db.GetEngine(ctx).Get(key) + if has { + if n == 0 { + err = ErrDuplicatePackageVersion + } + return key, err + } else if err == nil { + return GetOrInsertVersion(ctx, pv) + } + return nil, err } // UpdateVersion updates a version From 470064cacfe6d99fee29dd37b7a8777caa39d7bf Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Tue, 6 Sep 2022 21:26:59 +0100 Subject: [PATCH 03/29] more Signed-off-by: Andrew Thornton --- models/packages/package_version.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/models/packages/package_version.go b/models/packages/package_version.go index a0ef1da185d18..900bdd09bd5e1 100644 --- a/models/packages/package_version.go +++ b/models/packages/package_version.go @@ -39,11 +39,6 @@ type PackageVersion struct { // GetOrInsertVersion inserts a version. If the same version exist already ErrDuplicatePackageVersion is returned func GetOrInsertVersion(ctx context.Context, pv *PackageVersion) (*PackageVersion, error) { - key := &PackageVersion{ - PackageID: pv.PackageID, - LowerVersion: pv.LowerVersion, - } - n, err := db.InsertOnConflictDoNothing(ctx, pv) if err != nil { return nil, err @@ -52,6 +47,11 @@ func GetOrInsertVersion(ctx context.Context, pv *PackageVersion) (*PackageVersio return pv, nil } + key := &PackageVersion{ + PackageID: pv.PackageID, + LowerVersion: pv.LowerVersion, + } + has, err := db.GetEngine(ctx).Get(key) if has { if n == 0 { From 8d3864a5454b157cc49b23f510e034ea13e5f599 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Wed, 7 Sep 2022 21:04:30 +0100 Subject: [PATCH 04/29] ensure that timestamps are also set Signed-off-by: Andrew Thornton --- models/db/common.go | 38 +++++++++++++++++++++++++++++++------- 1 file changed, 31 insertions(+), 7 deletions(-) diff --git a/models/db/common.go b/models/db/common.go index a31c066ef11db..022c8b585daed 100644 --- a/models/db/common.go +++ b/models/db/common.go @@ -9,11 +9,13 @@ import ( "fmt" "reflect" "strings" + "time" "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/util" "xorm.io/builder" + "xorm.io/xorm/dialects" "xorm.io/xorm/schemas" ) @@ -36,9 +38,6 @@ func InsertOnConflictDoNothing(ctx context.Context, bean interface{}) (int64, er } autoIncrCol := table.AutoIncrColumn() - if autoIncrCol == nil { - return 0, fmt.Errorf("this function requires an autoincrement column") - } cols := table.Columns() colNames := make([]string, 0, len(cols)) @@ -49,6 +48,27 @@ func InsertOnConflictDoNothing(ctx context.Context, bean interface{}) (int64, er for _, col := range cols { if fieldIdx := col.FieldIndex; fieldIdx != nil { fieldVal := elem.FieldByIndex(fieldIdx) + if col.IsCreated || col.IsUpdated { + t := time.Now() + result, err := dialects.FormatColumnTime(x.Dialect(), x.DatabaseTZ, col, t) + if err != nil { + return 0, err + } + + switch fieldVal.Type().Kind() { + case reflect.Struct: + fieldVal.Set(reflect.ValueOf(t).Convert(fieldVal.Type())) + case reflect.Int, reflect.Int64, reflect.Int32: + fieldVal.SetInt(t.Unix()) + case reflect.Uint, reflect.Uint64, reflect.Uint32: + fieldVal.SetUint(uint64(t.Unix())) + } + + colNames = append(colNames, col.Name) + args = append(args, result) + continue + } + if fieldVal.IsZero() { continue } @@ -109,9 +129,13 @@ func InsertOnConflictDoNothing(ctx context.Context, bean interface{}) (int64, er _, _ = sb.WriteString(") ON CONFLICT DO NOTHING") case setting.Database.UseMySQL: _, _ = sb.WriteString(") ON CONFLICT DO DUPLICATE KEY ") - _, _ = sb.WriteString(autoIncrCol.Name) - _, _ = sb.WriteString(" = ") - _, _ = sb.WriteString(autoIncrCol.Name) + if autoIncrCol == nil { + _, _ = sb.WriteString(autoIncrCol.Name) + _, _ = sb.WriteString(" = ") + _, _ = sb.WriteString(autoIncrCol.Name) + } else { + _, _ = sb.WriteString(" IGNORE") + } } case setting.Database.UseMSSQL: _, _ = sb.WriteString("MERGE ") @@ -161,7 +185,7 @@ func InsertOnConflictDoNothing(ctx context.Context, bean interface{}) (int64, er return n, err } - if n != 0 { + if n != 0 && autoIncrCol != nil { id, err := res.LastInsertId() if err != nil { return n, err From 71522ea0cc0dbae028623fd349890deba6449274 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Wed, 7 Sep 2022 21:48:00 +0100 Subject: [PATCH 05/29] oops lets try to get the mysql syntax right Signed-off-by: Andrew Thornton --- models/db/common.go | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/models/db/common.go b/models/db/common.go index 022c8b585daed..9449d8729b31c 100644 --- a/models/db/common.go +++ b/models/db/common.go @@ -111,7 +111,11 @@ func InsertOnConflictDoNothing(ctx context.Context, bean interface{}) (int64, er sb := &strings.Builder{} switch { case setting.Database.UseSQLite3 || setting.Database.UsePostgreSQL || setting.Database.UseMySQL: - _, _ = sb.WriteString("INSERT INTO ") + _, _ = sb.WriteString("INSERT ") + if setting.Database.UseMySQL && autoIncrCol == nil { + _, _ = sb.WriteString("IGNORE ") + } + _, _ = sb.WriteString("INTO ") _, _ = sb.WriteString(x.Dialect().Quoter().Quote(tableName)) _, _ = sb.WriteString(" (") _, _ = sb.WriteString(colNames[0]) @@ -128,13 +132,11 @@ func InsertOnConflictDoNothing(ctx context.Context, bean interface{}) (int64, er case setting.Database.UseSQLite3 || setting.Database.UsePostgreSQL: _, _ = sb.WriteString(") ON CONFLICT DO NOTHING") case setting.Database.UseMySQL: - _, _ = sb.WriteString(") ON CONFLICT DO DUPLICATE KEY ") - if autoIncrCol == nil { + if autoIncrCol != nil { + _, _ = sb.WriteString(") ON CONFLICT DO DUPLICATE KEY ") _, _ = sb.WriteString(autoIncrCol.Name) _, _ = sb.WriteString(" = ") _, _ = sb.WriteString(autoIncrCol.Name) - } else { - _, _ = sb.WriteString(" IGNORE") } } case setting.Database.UseMSSQL: From abcf334cf01f6c2062a6ce733b70ef2a6bfa8f50 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Thu, 2 Feb 2023 19:00:49 +0000 Subject: [PATCH 06/29] attempt to fix mysql/mssql Signed-off-by: Andrew Thornton --- models/db/common.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/models/db/common.go b/models/db/common.go index 6f9f61d2caccd..b0d74bdb0d6de 100644 --- a/models/db/common.go +++ b/models/db/common.go @@ -132,7 +132,7 @@ func InsertOnConflictDoNothing(ctx context.Context, bean interface{}) (int64, er _, _ = sb.WriteString(") ON CONFLICT DO NOTHING") case setting.Database.UseMySQL: if autoIncrCol != nil { - _, _ = sb.WriteString(") ON CONFLICT DO DUPLICATE KEY ") + _, _ = sb.WriteString(") ON DUPLICATE KEY UPDATE ") _, _ = sb.WriteString(autoIncrCol.Name) _, _ = sb.WriteString(" = ") _, _ = sb.WriteString(autoIncrCol.Name) @@ -170,7 +170,7 @@ func InsertOnConflictDoNothing(ctx context.Context, bean interface{}) (int64, er for range colNames[1:] { _, _ = sb.WriteString(",?") } - _, _ = sb.WriteString(")") + _, _ = sb.WriteString(");") args = append(uniqueArgs, args[1:]...) default: return 0, fmt.Errorf("database type not supported") From 5dff21ac4fa66f37742b0424619e08489bd56dd8 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Tue, 7 Feb 2023 07:14:02 +0000 Subject: [PATCH 07/29] fix insert on conflict for sqlite Signed-off-by: Andrew Thornton --- models/db/common.go | 24 +++++++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-) diff --git a/models/db/common.go b/models/db/common.go index b0d74bdb0d6de..6b42fa1a094ca 100644 --- a/models/db/common.go +++ b/models/db/common.go @@ -40,7 +40,9 @@ func InsertOnConflictDoNothing(ctx context.Context, bean interface{}) (int64, er cols := table.Columns() colNames := make([]string, 0, len(cols)) - args := make([]interface{}, 1, len(cols)) + args := make([]any, 1, len(cols)) + emptyColNames := make([]string, 0, len(cols)) + emptyArgs := make([]any, 0, len(cols)) val := reflect.ValueOf(bean) elem := val.Elem() @@ -69,6 +71,17 @@ func InsertOnConflictDoNothing(ctx context.Context, bean interface{}) (int64, er } if fieldVal.IsZero() { + emptyColNames = append(emptyColNames, col.Name) + switch fieldVal.Type().Kind() { + case reflect.Int, reflect.Int8, reflect.Uint8, reflect.Int16, reflect.Uint16, reflect.Int32, reflect.Uint32, reflect.Int64, reflect.Uint64, reflect.Float32, reflect.Float64, reflect.Complex64, reflect.Complex128: + emptyArgs = append(emptyArgs, 0) + case reflect.String: + emptyArgs = append(emptyArgs, "") + case reflect.Bool: + emptyArgs = append(emptyArgs, false) + default: + emptyArgs = append(emptyArgs, nil) + } continue } colNames = append(colNames, col.Name) @@ -100,6 +113,15 @@ func InsertOnConflictDoNothing(ctx context.Context, bean interface{}) (int64, er continue indexCol } } + for i, col := range emptyColNames { + if col == iCol { + colNames = append(colNames, col) + args = append(args, emptyArgs[i]) + uniqueCols = append(uniqueCols, col) + uniqueArgs = append(uniqueArgs, emptyArgs[i]) + continue indexCol + } + } } } From f934a9891846cb68b7edcd453f446bee90c89816 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Tue, 7 Feb 2023 07:29:41 +0000 Subject: [PATCH 08/29] fix unit-test Signed-off-by: Andrew Thornton --- models/packages/package.go | 5 +---- models/packages/package_file.go | 7 ++----- models/packages/package_test.go | 4 +++- models/packages/package_version.go | 7 ++----- 4 files changed, 8 insertions(+), 15 deletions(-) diff --git a/models/packages/package.go b/models/packages/package.go index 03feebf506cac..720f93de0b513 100644 --- a/models/packages/package.go +++ b/models/packages/package.go @@ -168,10 +168,7 @@ func TryInsertPackage(ctx context.Context, p *Package) (*Package, error) { has, err := db.GetEngine(ctx).Get(key) if has { - if n == 0 { - err = ErrDuplicatePackage - } - return key, err + return key, ErrDuplicatePackage } else if err == nil { return TryInsertPackage(ctx, p) } diff --git a/models/packages/package_file.go b/models/packages/package_file.go index f0a3e04022f64..42b3ad2e9a1bf 100644 --- a/models/packages/package_file.go +++ b/models/packages/package_file.go @@ -48,7 +48,7 @@ func TryInsertFile(ctx context.Context, pf *PackageFile) (*PackageFile, error) { if err != nil { return nil, err } - if n != 0 { + if n != 0 { // Successful insert return pf, nil } @@ -59,10 +59,7 @@ func TryInsertFile(ctx context.Context, pf *PackageFile) (*PackageFile, error) { } has, err := db.GetEngine(ctx).Get(key) if has { - if n == 0 { - err = ErrDuplicatePackageFile - } - return key, err + return key, ErrDuplicatePackageFile } else if err == nil { return TryInsertFile(ctx, pf) } diff --git a/models/packages/package_test.go b/models/packages/package_test.go index 735688a731e0e..d5e142f8d6e4b 100644 --- a/models/packages/package_test.go +++ b/models/packages/package_test.go @@ -30,10 +30,10 @@ func TestHasOwnerPackages(t *testing.T) { p, err := packages_model.TryInsertPackage(db.DefaultContext, &packages_model.Package{ OwnerID: owner.ID, + Name: "package", LowerName: "package", }) assert.NotNil(t, p) - assert.NoError(t, err) // A package without package versions gets automatically cleaned up and should return false has, err := packages_model.HasOwnerPackages(db.DefaultContext, owner.ID) @@ -42,6 +42,7 @@ func TestHasOwnerPackages(t *testing.T) { pv, err := packages_model.GetOrInsertVersion(db.DefaultContext, &packages_model.PackageVersion{ PackageID: p.ID, + Version: "internal", LowerVersion: "internal", IsInternal: true, }) @@ -55,6 +56,7 @@ func TestHasOwnerPackages(t *testing.T) { pv, err = packages_model.GetOrInsertVersion(db.DefaultContext, &packages_model.PackageVersion{ PackageID: p.ID, + Version: "normal", LowerVersion: "normal", IsInternal: false, }) diff --git a/models/packages/package_version.go b/models/packages/package_version.go index 7d39464c1e2c0..5ea27b7481e21 100644 --- a/models/packages/package_version.go +++ b/models/packages/package_version.go @@ -41,7 +41,7 @@ func GetOrInsertVersion(ctx context.Context, pv *PackageVersion) (*PackageVersio if err != nil { return nil, err } - if n != 0 { + if n != 0 { // Successful insert return pv, nil } @@ -52,10 +52,7 @@ func GetOrInsertVersion(ctx context.Context, pv *PackageVersion) (*PackageVersio has, err := db.GetEngine(ctx).Get(key) if has { - if n == 0 { - err = ErrDuplicatePackageVersion - } - return key, err + return key, ErrDuplicatePackageVersion } else if err == nil { return GetOrInsertVersion(ctx, pv) } From b3db6dae9bb048189705b547e77580a60a079a82 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Tue, 7 Feb 2023 08:04:29 +0000 Subject: [PATCH 09/29] attempt to fix postgres Signed-off-by: Andrew Thornton --- models/db/common.go | 35 ++++++++++++++++++++++++++++++++++- 1 file changed, 34 insertions(+), 1 deletion(-) diff --git a/models/db/common.go b/models/db/common.go index 6b42fa1a094ca..05ef674938375 100644 --- a/models/db/common.go +++ b/models/db/common.go @@ -10,10 +10,12 @@ import ( "strings" "time" + "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/util" "xorm.io/builder" + "xorm.io/xorm/convert" "xorm.io/xorm/dialects" "xorm.io/xorm/schemas" ) @@ -150,7 +152,13 @@ func InsertOnConflictDoNothing(ctx context.Context, bean interface{}) (int64, er _, _ = sb.WriteString(",?") } switch { - case setting.Database.UseSQLite3 || setting.Database.UsePostgreSQL: + case setting.Database.UsePostgreSQL: + if autoIncrCol != nil { + _, _ = fmt.Fprintf(sb, ") RETURNING %s ON CONFLICT DO NOTHING", x.Dialect().Quoter().Quote(autoIncrCol.Name)) + } else { + _, _ = sb.WriteString(") ON CONFLICT DO NOTHING") + } + case setting.Database.UseSQLite3: _, _ = sb.WriteString(") ON CONFLICT DO NOTHING") case setting.Database.UseMySQL: if autoIncrCol != nil { @@ -198,6 +206,31 @@ func InsertOnConflictDoNothing(ctx context.Context, bean interface{}) (int64, er return 0, fmt.Errorf("database type not supported") } args[0] = sb.String() + + if autoIncrCol != nil { + switch { + case setting.Database.UsePostgreSQL: + res, err := e.Query(args...) + if err != nil { + return 0, err + } + if len(res) == 0 { + return 0, nil + } + + aiValue, err := table.AutoIncrColumn().ValueOf(bean) + if err != nil { + log.Error("unable to get value for autoincrcol of %#v %v", bean, err) + } + + if aiValue == nil || !aiValue.IsValid() || !aiValue.CanSet() { + return int64(len(res)), nil + } + + id := res[0][autoIncrCol.Name] + return int64(len(res)), convert.AssignValue(*aiValue, id) + } + } res, err := e.Exec(args...) if err != nil { return 0, err From 5bc49249c9d3868af45b52136c720673fbc3f691 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Tue, 7 Feb 2023 08:05:30 +0000 Subject: [PATCH 10/29] placate lint Signed-off-by: Andrew Thornton --- models/packages/package_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/models/packages/package_test.go b/models/packages/package_test.go index d5e142f8d6e4b..45c2a7fe17a03 100644 --- a/models/packages/package_test.go +++ b/models/packages/package_test.go @@ -34,6 +34,7 @@ func TestHasOwnerPackages(t *testing.T) { LowerName: "package", }) assert.NotNil(t, p) + assert.NoError(t, err) // A package without package versions gets automatically cleaned up and should return false has, err := packages_model.HasOwnerPackages(db.DefaultContext, owner.ID) From 8f7987cf6a4795020b122dd217e9047fdb55ea7d Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Tue, 7 Feb 2023 08:14:22 +0000 Subject: [PATCH 11/29] fix mssql? Signed-off-by: Andrew Thornton --- models/db/common.go | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/models/db/common.go b/models/db/common.go index 05ef674938375..681f3a0cd924f 100644 --- a/models/db/common.go +++ b/models/db/common.go @@ -200,7 +200,12 @@ func InsertOnConflictDoNothing(ctx context.Context, bean interface{}) (int64, er for range colNames[1:] { _, _ = sb.WriteString(",?") } - _, _ = sb.WriteString(");") + _, _ = sb.WriteString(")") + if autoIncrCol != nil { + _, _ = sb.WriteString(" OUTPUT inserted.") + _, _ = sb.WriteString(autoIncrCol.Name) + } + _, _ = sb.WriteString(";") args = append(uniqueArgs, args[1:]...) default: return 0, fmt.Errorf("database type not supported") @@ -209,7 +214,7 @@ func InsertOnConflictDoNothing(ctx context.Context, bean interface{}) (int64, er if autoIncrCol != nil { switch { - case setting.Database.UsePostgreSQL: + case setting.Database.UsePostgreSQL, setting.Database.UseMSSQL: res, err := e.Query(args...) if err != nil { return 0, err From fc5d9aa73e89bf0b81cc34b1d557c81a29988f75 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Tue, 7 Feb 2023 08:26:57 +0000 Subject: [PATCH 12/29] hopefully fix psql Signed-off-by: Andrew Thornton --- models/db/common.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/models/db/common.go b/models/db/common.go index 681f3a0cd924f..4811b2c2b5528 100644 --- a/models/db/common.go +++ b/models/db/common.go @@ -153,10 +153,9 @@ func InsertOnConflictDoNothing(ctx context.Context, bean interface{}) (int64, er } switch { case setting.Database.UsePostgreSQL: + _, _ = sb.WriteString(") ON CONFLICT DO NOTHING") if autoIncrCol != nil { - _, _ = fmt.Fprintf(sb, ") RETURNING %s ON CONFLICT DO NOTHING", x.Dialect().Quoter().Quote(autoIncrCol.Name)) - } else { - _, _ = sb.WriteString(") ON CONFLICT DO NOTHING") + _, _ = fmt.Fprintf(sb, " RETURNING %s", autoIncrCol.Name) } case setting.Database.UseSQLite3: _, _ = sb.WriteString(") ON CONFLICT DO NOTHING") From 7ec881f2c62cbb19d2cd0cc335b68f27c092523d Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Tue, 7 Feb 2023 08:32:10 +0000 Subject: [PATCH 13/29] get more info mssql Signed-off-by: Andrew Thornton --- models/db/common.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/models/db/common.go b/models/db/common.go index 4811b2c2b5528..d637ba314c9aa 100644 --- a/models/db/common.go +++ b/models/db/common.go @@ -216,7 +216,7 @@ func InsertOnConflictDoNothing(ctx context.Context, bean interface{}) (int64, er case setting.Database.UsePostgreSQL, setting.Database.UseMSSQL: res, err := e.Query(args...) if err != nil { - return 0, err + return 0, fmt.Errorf("error in query: %s, %w", args[0], err) } if len(res) == 0 { return 0, nil @@ -232,6 +232,10 @@ func InsertOnConflictDoNothing(ctx context.Context, bean interface{}) (int64, er } id := res[0][autoIncrCol.Name] + err = convert.AssignValue(*aiValue, id) + if err != nil { + return int64(len(res)), fmt.Errorf("error in assignvalue %v %v %w", id, res, err) + } return int64(len(res)), convert.AssignValue(*aiValue, id) } } From d8aa794e3cb296d65e00a1603cd7965eec430ea7 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Tue, 7 Feb 2023 09:09:07 +0000 Subject: [PATCH 14/29] perhaps inserted should be INSERTED? Signed-off-by: Andrew Thornton --- models/db/common.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/models/db/common.go b/models/db/common.go index d637ba314c9aa..4b00710cbd32b 100644 --- a/models/db/common.go +++ b/models/db/common.go @@ -201,7 +201,7 @@ func InsertOnConflictDoNothing(ctx context.Context, bean interface{}) (int64, er } _, _ = sb.WriteString(")") if autoIncrCol != nil { - _, _ = sb.WriteString(" OUTPUT inserted.") + _, _ = sb.WriteString(" OUTPUT INSERTED.") _, _ = sb.WriteString(autoIncrCol.Name) } _, _ = sb.WriteString(";") From 3f390453c71da354d2f80ae8c6fb5dd36d2cf46a Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Tue, 7 Feb 2023 09:56:52 +0000 Subject: [PATCH 15/29] fix mssql bug Signed-off-by: Andrew Thornton --- models/db/common.go | 43 +++++++++++++++++++++++++++++++------------ 1 file changed, 31 insertions(+), 12 deletions(-) diff --git a/models/db/common.go b/models/db/common.go index 4b00710cbd32b..60cdcd77245fb 100644 --- a/models/db/common.go +++ b/models/db/common.go @@ -72,22 +72,41 @@ func InsertOnConflictDoNothing(ctx context.Context, bean interface{}) (int64, er continue } + var val any + switch fieldVal.Type().Kind() { + case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64: + val = fieldVal.Int() + case reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64: + val = fieldVal.Uint() + case reflect.Float32, reflect.Float64: + val = fieldVal.Float() + case reflect.Complex64, reflect.Complex128: + val = fieldVal.Complex() + case reflect.String: + val = fieldVal.String() + case reflect.Bool: + valBool := fieldVal.Bool() + + if setting.Database.UseMSSQL { + if valBool { + val = 1 + } else { + val = 0 + } + } else { + val = valBool + } + default: + val = fieldVal.Interface() + } + if fieldVal.IsZero() { emptyColNames = append(emptyColNames, col.Name) - switch fieldVal.Type().Kind() { - case reflect.Int, reflect.Int8, reflect.Uint8, reflect.Int16, reflect.Uint16, reflect.Int32, reflect.Uint32, reflect.Int64, reflect.Uint64, reflect.Float32, reflect.Float64, reflect.Complex64, reflect.Complex128: - emptyArgs = append(emptyArgs, 0) - case reflect.String: - emptyArgs = append(emptyArgs, "") - case reflect.Bool: - emptyArgs = append(emptyArgs, false) - default: - emptyArgs = append(emptyArgs, nil) - } + emptyArgs = append(emptyArgs, val) continue } colNames = append(colNames, col.Name) - args = append(args, fieldVal.Interface()) + args = append(args, val) } } @@ -186,7 +205,7 @@ func InsertOnConflictDoNothing(ctx context.Context, bean interface{}) (int64, er _, _ = sb.WriteString(" AND src.") _, _ = sb.WriteString(uniqueCol) _, _ = sb.WriteString("= target.") - _, _ = sb.WriteString(uniqueCols[0]) + _, _ = sb.WriteString(uniqueCol) } _, _ = sb.WriteString(" WHEN NOT MATCHED THEN INSERT (") _, _ = sb.WriteString(colNames[0]) From 15855df98d87c9f4e9674712c5da93b54b7823cc Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Tue, 7 Feb 2023 11:16:29 +0000 Subject: [PATCH 16/29] add comments and slight restructure Signed-off-by: Andrew Thornton --- models/db/common.go | 409 +++++++++++++++++++++++++++----------------- 1 file changed, 248 insertions(+), 161 deletions(-) diff --git a/models/db/common.go b/models/db/common.go index 60cdcd77245fb..e5db4cfc36e8c 100644 --- a/models/db/common.go +++ b/models/db/common.go @@ -6,6 +6,7 @@ package db import ( "context" "fmt" + "io" "reflect" "strings" "time" @@ -29,6 +30,10 @@ func BuildCaseInsensitiveLike(key, value string) builder.Cond { return builder.Like{"UPPER(" + key + ")", strings.ToUpper(value)} } +// InsertOnConflictDoNothing will attempt to insert the provided bean but if there is a conflict it will not error out +// This function will update the ID of the provided bean if there is an insertion +// This does not do all of the conversions that xorm would do automatically but it does quite a number of them +// once xorm has a working InsertOnConflictDoNothing this function could be removed. func InsertOnConflictDoNothing(ctx context.Context, bean interface{}) (int64, error) { e := GetEngine(ctx) @@ -41,113 +46,21 @@ func InsertOnConflictDoNothing(ctx context.Context, bean interface{}) (int64, er autoIncrCol := table.AutoIncrColumn() cols := table.Columns() - colNames := make([]string, 0, len(cols)) - args := make([]any, 1, len(cols)) - emptyColNames := make([]string, 0, len(cols)) - emptyArgs := make([]any, 0, len(cols)) - val := reflect.ValueOf(bean) - elem := val.Elem() - for _, col := range cols { - if fieldIdx := col.FieldIndex; fieldIdx != nil { - fieldVal := elem.FieldByIndex(fieldIdx) - if col.IsCreated || col.IsUpdated { - t := time.Now() - result, err := dialects.FormatColumnTime(x.Dialect(), x.DatabaseTZ, col, t) - if err != nil { - return 0, err - } - - switch fieldVal.Type().Kind() { - case reflect.Struct: - fieldVal.Set(reflect.ValueOf(t).Convert(fieldVal.Type())) - case reflect.Int, reflect.Int64, reflect.Int32: - fieldVal.SetInt(t.Unix()) - case reflect.Uint, reflect.Uint64, reflect.Uint32: - fieldVal.SetUint(uint64(t.Unix())) - } - - colNames = append(colNames, col.Name) - args = append(args, result) - continue - } - - var val any - switch fieldVal.Type().Kind() { - case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64: - val = fieldVal.Int() - case reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64: - val = fieldVal.Uint() - case reflect.Float32, reflect.Float64: - val = fieldVal.Float() - case reflect.Complex64, reflect.Complex128: - val = fieldVal.Complex() - case reflect.String: - val = fieldVal.String() - case reflect.Bool: - valBool := fieldVal.Bool() - - if setting.Database.UseMSSQL { - if valBool { - val = 1 - } else { - val = 0 - } - } else { - val = valBool - } - default: - val = fieldVal.Interface() - } - - if fieldVal.IsZero() { - emptyColNames = append(emptyColNames, col.Name) - emptyArgs = append(emptyArgs, val) - continue - } - colNames = append(colNames, col.Name) - args = append(args, val) - } + colNames, args, emptyColNames, emptyArgs, err := getColNamesAndArgsFromBean(bean, cols) + if err != nil { + return 0, err } if len(colNames) == 0 { - return 0, fmt.Errorf("empty bean") - } - - uniqueCols := make([]string, 0, len(cols)) - uniqueArgs := make([]interface{}, 1, len(cols)) - for _, index := range table.Indexes { - if index.Type != schemas.UniqueType { - continue - } - indexCol: - for _, iCol := range index.Cols { - for _, uCol := range uniqueCols { - if uCol == iCol { - continue indexCol - } - } - for i, col := range colNames { - if col == iCol { - uniqueCols = append(uniqueCols, col) - uniqueArgs = append(uniqueArgs, args[i+1]) - continue indexCol - } - } - for i, col := range emptyColNames { - if col == iCol { - colNames = append(colNames, col) - args = append(args, emptyArgs[i]) - uniqueCols = append(uniqueCols, col) - uniqueArgs = append(uniqueArgs, emptyArgs[i]) - continue indexCol - } - } - } + return 0, fmt.Errorf("provided bean to insert has all empty values") } + // MSSQL needs to separately pass in the columns with the unique constraint and we need to + // include empty columns which are in the constraint in the insert for other dbs + uniqueCols, uniqueArgs, colNames, args := addInUniqueCols(colNames, args, emptyColNames, emptyArgs, table) if len(uniqueCols) == 0 { - return 0, fmt.Errorf("empty bean") + return 0, fmt.Errorf("provided bean has no unique constraints") } sb := &strings.Builder{} @@ -187,77 +100,43 @@ func InsertOnConflictDoNothing(ctx context.Context, bean interface{}) (int64, er } } case setting.Database.UseMSSQL: - _, _ = sb.WriteString("MERGE ") - _, _ = sb.WriteString(x.Dialect().Quoter().Quote(tableName)) - _, _ = sb.WriteString(" WITH (HOLDLOCK) AS target USING (SELECT ") - - _, _ = sb.WriteString("? AS ") - _, _ = sb.WriteString(uniqueCols[0]) - for _, uniqueCol := range uniqueCols[1:] { - _, _ = sb.WriteString(", ? AS ") - _, _ = sb.WriteString(uniqueCol) - } - _, _ = sb.WriteString(") AS src ON src.") - _, _ = sb.WriteString(uniqueCols[0]) - _, _ = sb.WriteString("= target.") - _, _ = sb.WriteString(uniqueCols[0]) - for _, uniqueCol := range uniqueCols[1:] { - _, _ = sb.WriteString(" AND src.") - _, _ = sb.WriteString(uniqueCol) - _, _ = sb.WriteString("= target.") - _, _ = sb.WriteString(uniqueCol) - } - _, _ = sb.WriteString(" WHEN NOT MATCHED THEN INSERT (") - _, _ = sb.WriteString(colNames[0]) - for _, colName := range colNames[1:] { - _, _ = sb.WriteString(",") - _, _ = sb.WriteString(colName) - } - _, _ = sb.WriteString(") VALUES (") - _, _ = sb.WriteString("?") - for range colNames[1:] { - _, _ = sb.WriteString(",?") - } - _, _ = sb.WriteString(")") - if autoIncrCol != nil { - _, _ = sb.WriteString(" OUTPUT INSERTED.") - _, _ = sb.WriteString(autoIncrCol.Name) - } - _, _ = sb.WriteString(";") + generateInsertNoConflictSQLForMSSQL(sb, tableName, colNames, args, uniqueCols, autoIncrCol) args = append(uniqueArgs, args[1:]...) default: return 0, fmt.Errorf("database type not supported") } args[0] = sb.String() - if autoIncrCol != nil { - switch { - case setting.Database.UsePostgreSQL, setting.Database.UseMSSQL: - res, err := e.Query(args...) - if err != nil { - return 0, fmt.Errorf("error in query: %s, %w", args[0], err) - } - if len(res) == 0 { - return 0, nil - } + if autoIncrCol != nil && (setting.Database.UsePostgreSQL || setting.Database.UseMSSQL) { + // Postgres and MSSQL do not use the LastInsertID mechanism + // Therefore use query rather than exec and read the last provided ID back in - aiValue, err := table.AutoIncrColumn().ValueOf(bean) - if err != nil { - log.Error("unable to get value for autoincrcol of %#v %v", bean, err) - } + res, err := e.Query(args...) + if err != nil { + return 0, fmt.Errorf("error in query: %s, %w", args[0], err) + } + if len(res) == 0 { + // this implies there was a conflict + return 0, nil + } - if aiValue == nil || !aiValue.IsValid() || !aiValue.CanSet() { - return int64(len(res)), nil - } + aiValue, err := table.AutoIncrColumn().ValueOf(bean) + if err != nil { + log.Error("unable to get value for autoincrcol of %#v %v", bean, err) + } - id := res[0][autoIncrCol.Name] - err = convert.AssignValue(*aiValue, id) - if err != nil { - return int64(len(res)), fmt.Errorf("error in assignvalue %v %v %w", id, res, err) - } - return int64(len(res)), convert.AssignValue(*aiValue, id) + if aiValue == nil || !aiValue.IsValid() || !aiValue.CanSet() { + return int64(len(res)), nil } + + id := res[0][autoIncrCol.Name] + err = convert.AssignValue(*aiValue, id) + if err != nil { + return int64(len(res)), fmt.Errorf("error in assignvalue %v %v %w", id, res, err) + } + return int64(len(res)), convert.AssignValue(*aiValue, id) } + res, err := e.Exec(args...) if err != nil { return 0, err @@ -273,8 +152,216 @@ func InsertOnConflictDoNothing(ctx context.Context, bean interface{}) (int64, er if err != nil { return n, err } - elem.FieldByName(autoIncrCol.FieldName).SetInt(id) + reflect.ValueOf(bean).Elem().FieldByName(autoIncrCol.FieldName).SetInt(id) } return res.RowsAffected() } + +// generateInsertNoConflictSQLForMSSQL writes the INSERT ... ON CONFLICT sql variant for MSSQL +// MSSQL uses MERGE WITH ... but needs to pre-select the unique cols first +// then WHEN NOT MATCHED INSERT - this is kind of the opposite way round from INSERT ... ON CONFLICT +func generateInsertNoConflictSQLForMSSQL(sb io.StringWriter, tableName string, colNames []string, args []any, uniqueCols []string, autoIncrCol *schemas.Column) { + _, _ = sb.WriteString("MERGE ") + _, _ = sb.WriteString(x.Dialect().Quoter().Quote(tableName)) + _, _ = sb.WriteString(" WITH (HOLDLOCK) AS target USING (SELECT ") + + _, _ = sb.WriteString("? AS ") + _, _ = sb.WriteString(uniqueCols[0]) + for _, uniqueCol := range uniqueCols[1:] { + _, _ = sb.WriteString(", ? AS ") + _, _ = sb.WriteString(uniqueCol) + } + _, _ = sb.WriteString(") AS src ON src.") + _, _ = sb.WriteString(uniqueCols[0]) + _, _ = sb.WriteString("= target.") + _, _ = sb.WriteString(uniqueCols[0]) + for _, uniqueCol := range uniqueCols[1:] { + _, _ = sb.WriteString(" AND src.") + _, _ = sb.WriteString(uniqueCol) + _, _ = sb.WriteString("= target.") + _, _ = sb.WriteString(uniqueCol) + } + _, _ = sb.WriteString(" WHEN NOT MATCHED THEN INSERT (") + _, _ = sb.WriteString(colNames[0]) + for _, colName := range colNames[1:] { + _, _ = sb.WriteString(",") + _, _ = sb.WriteString(colName) + } + _, _ = sb.WriteString(") VALUES (") + _, _ = sb.WriteString("?") + for range colNames[1:] { + _, _ = sb.WriteString(",?") + } + _, _ = sb.WriteString(")") + if autoIncrCol != nil { + _, _ = sb.WriteString(" OUTPUT INSERTED.") + _, _ = sb.WriteString(autoIncrCol.Name) + } + _, _ = sb.WriteString(";") +} + +func addInUniqueCols(colNames []string, args []any, emptyColNames []string, emptyArgs []any, table *schemas.Table) (uniqueCols []string, uniqueArgs []any, insertCols []string, insertArgs []any) { + uniqueCols = make([]string, 0, len(table.Columns())) + uniqueArgs = make([]interface{}, 1, len(uniqueCols)+1) // leave uniqueArgs[0] empty to put the SQL in + for _, index := range table.Indexes { + if index.Type != schemas.UniqueType { + continue + } + indexCol: + for _, iCol := range index.Cols { + for _, uCol := range uniqueCols { + if uCol == iCol { + continue indexCol + } + } + for i, col := range colNames { + if col == iCol { + uniqueCols = append(uniqueCols, col) + uniqueArgs = append(uniqueArgs, args[i+1]) + continue indexCol + } + } + for i, col := range emptyColNames { + if col == iCol { + // Always include empty unique columns in the insert statement + colNames = append(colNames, col) + args = append(args, emptyArgs[i]) + uniqueCols = append(uniqueCols, col) + uniqueArgs = append(uniqueArgs, emptyArgs[i]) + continue indexCol + } + } + } + } + return uniqueCols, uniqueArgs, colNames, args +} + +func getColNamesAndArgsFromBean(bean interface{}, cols []*schemas.Column) (colNames []string, args []any, emptyColNames []string, emptyArgs []any, err error) { + colNames = make([]string, len(cols)) + args = make([]any, len(cols)+1) // Leave args[0] to put the SQL in + maxNonEmpty := 0 + minEmpty := len(cols) + + val := reflect.ValueOf(bean) + elem := val.Elem() + for _, col := range cols { + if fieldIdx := col.FieldIndex; fieldIdx != nil { + fieldVal := elem.FieldByIndex(fieldIdx) + if col.IsCreated || col.IsUpdated { + result, err := setCurrentTime(fieldVal, col) + if err != nil { + return nil, nil, nil, nil, err + } + + colNames[maxNonEmpty] = col.Name + maxNonEmpty++ + args[maxNonEmpty] = result + continue + } + + val, err := getValueFromField(fieldVal, col) + if err != nil { + return nil, nil, nil, nil, err + } + if fieldVal.IsZero() { + args[minEmpty] = val // remember args is 1-based not 0-based + minEmpty-- + colNames[minEmpty] = col.Name + continue + } + colNames[maxNonEmpty] = col.Name + maxNonEmpty++ + args[maxNonEmpty] = val + } + } + + return colNames[:maxNonEmpty], args[:maxNonEmpty+1], colNames[maxNonEmpty:], args[maxNonEmpty+1:], nil +} + +func setCurrentTime(fieldVal reflect.Value, col *schemas.Column) (interface{}, error) { + t := time.Now() + result, err := dialects.FormatColumnTime(x.Dialect(), x.DatabaseTZ, col, t) + if err != nil { + return result, err + } + + switch fieldVal.Type().Kind() { + case reflect.Struct: + fieldVal.Set(reflect.ValueOf(t).Convert(fieldVal.Type())) + case reflect.Int, reflect.Int64, reflect.Int32: + fieldVal.SetInt(t.Unix()) + case reflect.Uint, reflect.Uint64, reflect.Uint32: + fieldVal.SetUint(uint64(t.Unix())) + } + return result, nil +} + +func getValueFromField(fieldVal reflect.Value, col *schemas.Column) (any, error) { + switch fieldVal.Type().Kind() { + case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64: + return fieldVal.Int(), nil + case reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64: + return fieldVal.Uint(), nil + case reflect.Float32, reflect.Float64: + return fieldVal.Float(), nil + case reflect.Complex64, reflect.Complex128: + return fieldVal.Complex(), nil + case reflect.String: + return fieldVal.String(), nil + case reflect.Bool: + valBool := fieldVal.Bool() + + if setting.Database.UseMSSQL { + if valBool { + return 1, nil + } else { + return 0, nil + } + } else { + return valBool, nil + } + default: + } + + if fieldVal.CanAddr() { + if fieldConvert, ok := fieldVal.Addr().Interface().(convert.Conversion); ok { + data, err := fieldConvert.ToDB() + if err != nil { + return nil, err + } + if data == nil { + if col.Nullable { + return nil, nil + } + data = []byte{} + } + if col.SQLType.IsBlob() { + return data, nil + } + return string(data), nil + } + } + + isNil := fieldVal.Kind() == reflect.Ptr && fieldVal.IsNil() + if !isNil { + if fieldConvert, ok := fieldVal.Interface().(convert.Conversion); ok { + data, err := fieldConvert.ToDB() + if err != nil { + return nil, err + } + if data == nil { + if col.Nullable { + return nil, nil + } + data = []byte{} + } + if col.SQLType.IsBlob() { + return data, nil + } + return string(data), nil + } + } + + return fieldVal.Interface(), nil +} From 38d540b71fc1bc1f28cacd6ea20a22ea7105ddd2 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Tue, 7 Feb 2023 11:51:36 +0000 Subject: [PATCH 17/29] slight adjustments Signed-off-by: Andrew Thornton --- models/db/common.go | 30 ++++++++++----------- models/packages/package.go | 40 ++++++++++++++------------- models/packages/package_file.go | 43 +++++++++++++++++------------- models/packages/package_version.go | 39 +++++++++++++++------------ 4 files changed, 83 insertions(+), 69 deletions(-) diff --git a/models/db/common.go b/models/db/common.go index e5db4cfc36e8c..0df7f51cd9acb 100644 --- a/models/db/common.go +++ b/models/db/common.go @@ -34,13 +34,13 @@ func BuildCaseInsensitiveLike(key, value string) builder.Cond { // This function will update the ID of the provided bean if there is an insertion // This does not do all of the conversions that xorm would do automatically but it does quite a number of them // once xorm has a working InsertOnConflictDoNothing this function could be removed. -func InsertOnConflictDoNothing(ctx context.Context, bean interface{}) (int64, error) { +func InsertOnConflictDoNothing(ctx context.Context, bean interface{}) (bool, error) { e := GetEngine(ctx) tableName := x.TableName(bean, true) table, err := x.TableInfo(bean) if err != nil { - return 0, err + return false, err } autoIncrCol := table.AutoIncrColumn() @@ -49,18 +49,18 @@ func InsertOnConflictDoNothing(ctx context.Context, bean interface{}) (int64, er colNames, args, emptyColNames, emptyArgs, err := getColNamesAndArgsFromBean(bean, cols) if err != nil { - return 0, err + return false, err } if len(colNames) == 0 { - return 0, fmt.Errorf("provided bean to insert has all empty values") + return false, fmt.Errorf("provided bean to insert has all empty values") } // MSSQL needs to separately pass in the columns with the unique constraint and we need to // include empty columns which are in the constraint in the insert for other dbs uniqueCols, uniqueArgs, colNames, args := addInUniqueCols(colNames, args, emptyColNames, emptyArgs, table) if len(uniqueCols) == 0 { - return 0, fmt.Errorf("provided bean has no unique constraints") + return false, fmt.Errorf("provided bean has no unique constraints") } sb := &strings.Builder{} @@ -103,7 +103,7 @@ func InsertOnConflictDoNothing(ctx context.Context, bean interface{}) (int64, er generateInsertNoConflictSQLForMSSQL(sb, tableName, colNames, args, uniqueCols, autoIncrCol) args = append(uniqueArgs, args[1:]...) default: - return 0, fmt.Errorf("database type not supported") + return false, fmt.Errorf("database type not supported") } args[0] = sb.String() @@ -113,11 +113,11 @@ func InsertOnConflictDoNothing(ctx context.Context, bean interface{}) (int64, er res, err := e.Query(args...) if err != nil { - return 0, fmt.Errorf("error in query: %s, %w", args[0], err) + return false, fmt.Errorf("error in query: %s, %w", args[0], err) } if len(res) == 0 { // this implies there was a conflict - return 0, nil + return false, nil } aiValue, err := table.AutoIncrColumn().ValueOf(bean) @@ -126,36 +126,36 @@ func InsertOnConflictDoNothing(ctx context.Context, bean interface{}) (int64, er } if aiValue == nil || !aiValue.IsValid() || !aiValue.CanSet() { - return int64(len(res)), nil + return true, nil } id := res[0][autoIncrCol.Name] err = convert.AssignValue(*aiValue, id) if err != nil { - return int64(len(res)), fmt.Errorf("error in assignvalue %v %v %w", id, res, err) + return true, fmt.Errorf("error in assignvalue %v %v %w", id, res, err) } - return int64(len(res)), convert.AssignValue(*aiValue, id) + return true, nil } res, err := e.Exec(args...) if err != nil { - return 0, err + return false, err } n, err := res.RowsAffected() if err != nil { - return n, err + return n != 0, err } if n != 0 && autoIncrCol != nil { id, err := res.LastInsertId() if err != nil { - return n, err + return true, err } reflect.ValueOf(bean).Elem().FieldByName(autoIncrCol.FieldName).SetInt(id) } - return res.RowsAffected() + return n != 0, err } // generateInsertNoConflictSQLForMSSQL writes the INSERT ... ON CONFLICT sql variant for MSSQL diff --git a/models/packages/package.go b/models/packages/package.go index 720f93de0b513..2ff541bfe15d6 100644 --- a/models/packages/package.go +++ b/models/packages/package.go @@ -152,27 +152,31 @@ type Package struct { // TryInsertPackage inserts a package. If a package exists already, ErrDuplicatePackage is returned func TryInsertPackage(ctx context.Context, p *Package) (*Package, error) { - n, err := db.InsertOnConflictDoNothing(ctx, p) - if err != nil { - return nil, err - } - if n != 0 { - return p, nil - } + for i := 0; i <= 5; i++ { + inserted, err := db.InsertOnConflictDoNothing(ctx, p) + if err != nil || inserted { + return p, err + } - key := &Package{ - OwnerID: p.OwnerID, - Type: p.Type, - LowerName: p.LowerName, - } + key := &Package{ + OwnerID: p.OwnerID, + Type: p.Type, + LowerName: p.LowerName, + } - has, err := db.GetEngine(ctx).Get(key) - if has { - return key, ErrDuplicatePackage - } else if err == nil { - return TryInsertPackage(ctx, p) + has, err := db.GetEngine(ctx).Get(key) + if has { + return key, ErrDuplicatePackage + } else if err != nil { + return key, err + } + // This really should never happen and can only happen if this function + // is being called outside of a transaction and between the on conflict insert failing + // the conlicting item is removed. + // } - return nil, err + // If that weird case has happened 5 times in a row - there is something very odd going on! + return p, fmt.Errorf("unable to insert on conflict but yet not able to get from the db") } // DeletePackageByID deletes a package by id diff --git a/models/packages/package_file.go b/models/packages/package_file.go index 42b3ad2e9a1bf..c157ac3b57f2e 100644 --- a/models/packages/package_file.go +++ b/models/packages/package_file.go @@ -5,6 +5,7 @@ package packages import ( "context" + "fmt" "strconv" "strings" "time" @@ -44,27 +45,31 @@ type PackageFile struct { // TryInsertFile inserts a file. If the file exists already ErrDuplicatePackageFile is returned func TryInsertFile(ctx context.Context, pf *PackageFile) (*PackageFile, error) { - n, err := db.InsertOnConflictDoNothing(ctx, pf) - if err != nil { - return nil, err - } - if n != 0 { // Successful insert - return pf, nil - } + for i := 0; i <= 5; i++ { + inserted, err := db.InsertOnConflictDoNothing(ctx, pf) + if err != nil || inserted { + return pf, err + } - key := &PackageFile{ - VersionID: pf.VersionID, - LowerName: pf.LowerName, - CompositeKey: pf.CompositeKey, - } - has, err := db.GetEngine(ctx).Get(key) - if has { - return key, ErrDuplicatePackageFile - } else if err == nil { - return TryInsertFile(ctx, pf) + key := &PackageFile{ + VersionID: pf.VersionID, + LowerName: pf.LowerName, + CompositeKey: pf.CompositeKey, + } + has, err := db.GetEngine(ctx).Get(key) + if has { + return key, ErrDuplicatePackageFile + } + if err != nil { + return key, err + } + // This really should never happen and can only happen if this function + // is being called outside of a transaction and between the on conflict insert failing + // the conlicting item is removed. + // } - - return key, err + // If that weird case has happened 5 times in a row - there is something very odd going on! + return pf, fmt.Errorf("unable to insert on conflict but yet not able to get from the db") } // GetFilesByVersionID gets all files of a version diff --git a/models/packages/package_version.go b/models/packages/package_version.go index 5ea27b7481e21..d2954f472d0a4 100644 --- a/models/packages/package_version.go +++ b/models/packages/package_version.go @@ -5,6 +5,7 @@ package packages import ( "context" + "fmt" "strconv" "strings" @@ -37,26 +38,30 @@ type PackageVersion struct { // GetOrInsertVersion inserts a version. If the same version exist already ErrDuplicatePackageVersion is returned func GetOrInsertVersion(ctx context.Context, pv *PackageVersion) (*PackageVersion, error) { - n, err := db.InsertOnConflictDoNothing(ctx, pv) - if err != nil { - return nil, err - } - if n != 0 { // Successful insert - return pv, nil - } + for i := 0; i <= 5; i++ { + inserted, err := db.InsertOnConflictDoNothing(ctx, pv) + if err != nil || inserted { + return pv, err + } - key := &PackageVersion{ - PackageID: pv.PackageID, - LowerVersion: pv.LowerVersion, - } + key := &PackageVersion{ + PackageID: pv.PackageID, + LowerVersion: pv.LowerVersion, + } - has, err := db.GetEngine(ctx).Get(key) - if has { - return key, ErrDuplicatePackageVersion - } else if err == nil { - return GetOrInsertVersion(ctx, pv) + has, err := db.GetEngine(ctx).Get(key) + if has { + return key, ErrDuplicatePackageVersion + } else if err != nil { + return key, err + } + // This really should never happen and can only happen if this function + // is being called outside of a transaction and between the on conflict insert failing + // the conlicting item is removed. + // } - return nil, err + // If that weird case has happened 5 times in a row - there is something very odd going on! + return pv, fmt.Errorf("unable to insert on conflict but yet not able to get from the db") } // UpdateVersion updates a version From a941cba928b097e6ddd06b511654015fc91b42db Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Tue, 7 Feb 2023 14:32:52 +0000 Subject: [PATCH 18/29] placate the linter Signed-off-by: Andrew Thornton --- models/db/common.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/models/db/common.go b/models/db/common.go index 0df7f51cd9acb..464dce0b5213d 100644 --- a/models/db/common.go +++ b/models/db/common.go @@ -315,12 +315,10 @@ func getValueFromField(fieldVal reflect.Value, col *schemas.Column) (any, error) if setting.Database.UseMSSQL { if valBool { return 1, nil - } else { - return 0, nil } - } else { - return valBool, nil + return 0, nil } + return valBool, nil default: } From 5ef790204f6657a30c6bb6b7a3a2ff48b0c093a4 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Tue, 7 Feb 2023 15:53:08 +0000 Subject: [PATCH 19/29] as per wxiaoguang Signed-off-by: Andrew Thornton --- models/db/common.go | 140 ++++++++++++++--------------- models/packages/package.go | 38 ++++---- models/packages/package_file.go | 40 ++++----- models/packages/package_version.go | 37 ++++---- 4 files changed, 119 insertions(+), 136 deletions(-) diff --git a/models/db/common.go b/models/db/common.go index 464dce0b5213d..0178f7719c213 100644 --- a/models/db/common.go +++ b/models/db/common.go @@ -6,7 +6,6 @@ package db import ( "context" "fmt" - "io" "reflect" "strings" "time" @@ -63,57 +62,24 @@ func InsertOnConflictDoNothing(ctx context.Context, bean interface{}) (bool, err return false, fmt.Errorf("provided bean has no unique constraints") } - sb := &strings.Builder{} + var insertArgs []any + switch { - case setting.Database.UseSQLite3 || setting.Database.UsePostgreSQL || setting.Database.UseMySQL: - _, _ = sb.WriteString("INSERT ") - if setting.Database.UseMySQL && autoIncrCol == nil { - _, _ = sb.WriteString("IGNORE ") - } - _, _ = sb.WriteString("INTO ") - _, _ = sb.WriteString(x.Dialect().Quoter().Quote(tableName)) - _, _ = sb.WriteString(" (") - _, _ = sb.WriteString(colNames[0]) - for _, colName := range colNames[1:] { - _, _ = sb.WriteString(",") - _, _ = sb.WriteString(colName) - } - _, _ = sb.WriteString(") VALUES (") - _, _ = sb.WriteString("?") - for range colNames[1:] { - _, _ = sb.WriteString(",?") - } - switch { - case setting.Database.UsePostgreSQL: - _, _ = sb.WriteString(") ON CONFLICT DO NOTHING") - if autoIncrCol != nil { - _, _ = fmt.Fprintf(sb, " RETURNING %s", autoIncrCol.Name) - } - case setting.Database.UseSQLite3: - _, _ = sb.WriteString(") ON CONFLICT DO NOTHING") - case setting.Database.UseMySQL: - if autoIncrCol != nil { - _, _ = sb.WriteString(") ON DUPLICATE KEY UPDATE ") - _, _ = sb.WriteString(autoIncrCol.Name) - _, _ = sb.WriteString(" = ") - _, _ = sb.WriteString(autoIncrCol.Name) - } - } + case setting.Database.UseSQLite3 || setting.Database.UsePostgreSQL || setting.Data + insertArgs = generateInsertNoConflictSQLAndArgs(tableName, colNames, args, autoIncrCol) case setting.Database.UseMSSQL: - generateInsertNoConflictSQLForMSSQL(sb, tableName, colNames, args, uniqueCols, autoIncrCol) - args = append(uniqueArgs, args[1:]...) + insertArgs = generateInsertNoConflictSQLAndArgsForMSSQL(tableName, colNames, args, uniqueCols, uniqueArgs, autoIncrCol) default: return false, fmt.Errorf("database type not supported") } - args[0] = sb.String() if autoIncrCol != nil && (setting.Database.UsePostgreSQL || setting.Database.UseMSSQL) { // Postgres and MSSQL do not use the LastInsertID mechanism // Therefore use query rather than exec and read the last provided ID back in - res, err := e.Query(args...) + res, err := e.Query(insertArgs...) if err != nil { - return false, fmt.Errorf("error in query: %s, %w", args[0], err) + return false, fmt.Errorf("error in query: %s, %w", insertArgs[0], err) } if len(res) == 0 { // this implies there was a conflict @@ -158,47 +124,75 @@ func InsertOnConflictDoNothing(ctx context.Context, bean interface{}) (bool, err return n != 0, err } -// generateInsertNoConflictSQLForMSSQL writes the INSERT ... ON CONFLICT sql variant for MSSQL -// MSSQL uses MERGE WITH ... but needs to pre-select the unique cols first +// generateInsertNoConflictSQLAndArgs will create the correct insert code for most of the DBs except MSSQL +func generateInsertNoConflictSQLAndArgs(tableName string, colNames []string, args []any, autoIncrCol *schemas.Column) (insertArgs []any) { + sb := &strings.Builder{} + + quote := x.Dialect().Quoter().Quote + write := func(args ...string) { + for _, arg := range args { + _, _ = sb.WriteString(arg) + } + } + write("INSERT ") + if setting.Database.UseMySQL && autoIncrCol == nil { + write("IGNORE ") + } + write("INTO ", quote(tableName), " (") + _ = x.Dialect().Quoter().JoinWrite(sb, colNames, ",") + write(") VALUES (?") + for range colNames[1:] { + write(",?") + } + switch { + case setting.Database.UsePostgreSQL: + write(") ON CONFLICT DO NOTHING") + if autoIncrCol != nil { + write(" RETURNING ", quote(autoIncrCol.Name)) + } + case setting.Database.UseSQLite3: + write(") ON CONFLICT DO NOTHING") + case setting.Database.UseMySQL: + if autoIncrCol != nil { + write(") ON DUPLICATE KEY UPDATE ", quote(autoIncrCol.Name), " = ", quote(autoIncrCol.Name)) + } + } + args[0] = sb.String() + return args +} + +// generateInsertNoConflictSQLAndArgsForMSSQL writes the INSERT ... ON CONFLICT sql variant for MSSQL +// MSSQL uses MERGE WITH ... but needs to pre-select the unique cols first // then WHEN NOT MATCHED INSERT - this is kind of the opposite way round from INSERT ... ON CONFLICT -func generateInsertNoConflictSQLForMSSQL(sb io.StringWriter, tableName string, colNames []string, args []any, uniqueCols []string, autoIncrCol *schemas.Column) { - _, _ = sb.WriteString("MERGE ") - _, _ = sb.WriteString(x.Dialect().Quoter().Quote(tableName)) - _, _ = sb.WriteString(" WITH (HOLDLOCK) AS target USING (SELECT ") +func generateInsertNoConflictSQLAndArgsForMSSQL(tableName string, colNames []string, args []any, uniqueCols []string, uniqueArgs []any, autoIncrCol *schemas.Column) (insertArgs []any) { + sb := &strings.Builder{} - _, _ = sb.WriteString("? AS ") - _, _ = sb.WriteString(uniqueCols[0]) - for _, uniqueCol := range uniqueCols[1:] { - _, _ = sb.WriteString(", ? AS ") - _, _ = sb.WriteString(uniqueCol) + quote := x.Dialect().Quoter().Quote + write := func(args ...string) { + for _, arg := range args { + _, _ = sb.WriteString(arg) + } } - _, _ = sb.WriteString(") AS src ON src.") - _, _ = sb.WriteString(uniqueCols[0]) - _, _ = sb.WriteString("= target.") - _, _ = sb.WriteString(uniqueCols[0]) + + write("MERGE ", quote(tableName), " WITH (HOLDLOCK) AS target USING (SELECT ? AS ") + _ = x.Dialect().Quoter().JoinWrite(sb, uniqueCols, ", ? AS ") + write(") AS src ON src.", quote(uniqueCols[0]), "= target.", quote(uniqueCols[0])) for _, uniqueCol := range uniqueCols[1:] { - _, _ = sb.WriteString(" AND src.") - _, _ = sb.WriteString(uniqueCol) - _, _ = sb.WriteString("= target.") - _, _ = sb.WriteString(uniqueCol) - } - _, _ = sb.WriteString(" WHEN NOT MATCHED THEN INSERT (") - _, _ = sb.WriteString(colNames[0]) - for _, colName := range colNames[1:] { - _, _ = sb.WriteString(",") - _, _ = sb.WriteString(colName) + write(" AND src.", quote(uniqueCol), "= target.", quote(uniqueCol)) } - _, _ = sb.WriteString(") VALUES (") - _, _ = sb.WriteString("?") + write(" WHEN NOT MATCHED THEN INSERT (") + _ = x.Dialect().Quoter().JoinWrite(sb, colNames, ",") + write(") VALUES (?") for range colNames[1:] { - _, _ = sb.WriteString(",?") + write(", ?") } - _, _ = sb.WriteString(")") + write(")") if autoIncrCol != nil { - _, _ = sb.WriteString(" OUTPUT INSERTED.") - _, _ = sb.WriteString(autoIncrCol.Name) + write(" OUTPUT INSERTED.", quote(autoIncrCol.Name)) } - _, _ = sb.WriteString(";") + write(";") + + return append(uniqueArgs, args[1:]...) } func addInUniqueCols(colNames []string, args []any, emptyColNames []string, emptyArgs []any, table *schemas.Table) (uniqueCols []string, uniqueArgs []any, insertCols []string, insertArgs []any) { diff --git a/models/packages/package.go b/models/packages/package.go index 2ff541bfe15d6..7f84c5053785a 100644 --- a/models/packages/package.go +++ b/models/packages/package.go @@ -152,30 +152,26 @@ type Package struct { // TryInsertPackage inserts a package. If a package exists already, ErrDuplicatePackage is returned func TryInsertPackage(ctx context.Context, p *Package) (*Package, error) { - for i := 0; i <= 5; i++ { - inserted, err := db.InsertOnConflictDoNothing(ctx, p) - if err != nil || inserted { - return p, err - } + inserted, err := db.InsertOnConflictDoNothing(ctx, p) + if err != nil || inserted { + return p, err + } - key := &Package{ - OwnerID: p.OwnerID, - Type: p.Type, - LowerName: p.LowerName, - } + key := &Package{ + OwnerID: p.OwnerID, + Type: p.Type, + LowerName: p.LowerName, + } - has, err := db.GetEngine(ctx).Get(key) - if has { - return key, ErrDuplicatePackage - } else if err != nil { - return key, err - } - // This really should never happen and can only happen if this function - // is being called outside of a transaction and between the on conflict insert failing - // the conlicting item is removed. - // + has, err := db.GetEngine(ctx).Get(key) + if has { + return key, ErrDuplicatePackage + } else if err != nil { + return key, err } - // If that weird case has happened 5 times in a row - there is something very odd going on! + // This really should never happen and can only happen if this function + // is being called outside of a transaction and between the on conflict insert failing + // the conlicting item is removed. return p, fmt.Errorf("unable to insert on conflict but yet not able to get from the db") } diff --git a/models/packages/package_file.go b/models/packages/package_file.go index c157ac3b57f2e..2f133989c3a20 100644 --- a/models/packages/package_file.go +++ b/models/packages/package_file.go @@ -45,30 +45,26 @@ type PackageFile struct { // TryInsertFile inserts a file. If the file exists already ErrDuplicatePackageFile is returned func TryInsertFile(ctx context.Context, pf *PackageFile) (*PackageFile, error) { - for i := 0; i <= 5; i++ { - inserted, err := db.InsertOnConflictDoNothing(ctx, pf) - if err != nil || inserted { - return pf, err - } + inserted, err := db.InsertOnConflictDoNothing(ctx, pf) + if err != nil || inserted { + return pf, err + } - key := &PackageFile{ - VersionID: pf.VersionID, - LowerName: pf.LowerName, - CompositeKey: pf.CompositeKey, - } - has, err := db.GetEngine(ctx).Get(key) - if has { - return key, ErrDuplicatePackageFile - } - if err != nil { - return key, err - } - // This really should never happen and can only happen if this function - // is being called outside of a transaction and between the on conflict insert failing - // the conlicting item is removed. - // + key := &PackageFile{ + VersionID: pf.VersionID, + LowerName: pf.LowerName, + CompositeKey: pf.CompositeKey, + } + has, err := db.GetEngine(ctx).Get(key) + if has { + return key, ErrDuplicatePackageFile + } + if err != nil { + return key, err } - // If that weird case has happened 5 times in a row - there is something very odd going on! + // This really should never happen and can only happen if this function + // is being called outside of a transaction and between the on conflict insert failing + // the conlicting item is removed. return pf, fmt.Errorf("unable to insert on conflict but yet not able to get from the db") } diff --git a/models/packages/package_version.go b/models/packages/package_version.go index d2954f472d0a4..e8ca8ed8ae63e 100644 --- a/models/packages/package_version.go +++ b/models/packages/package_version.go @@ -38,29 +38,26 @@ type PackageVersion struct { // GetOrInsertVersion inserts a version. If the same version exist already ErrDuplicatePackageVersion is returned func GetOrInsertVersion(ctx context.Context, pv *PackageVersion) (*PackageVersion, error) { - for i := 0; i <= 5; i++ { - inserted, err := db.InsertOnConflictDoNothing(ctx, pv) - if err != nil || inserted { - return pv, err - } + inserted, err := db.InsertOnConflictDoNothing(ctx, pv) + if err != nil || inserted { + return pv, err + } - key := &PackageVersion{ - PackageID: pv.PackageID, - LowerVersion: pv.LowerVersion, - } + key := &PackageVersion{ + PackageID: pv.PackageID, + LowerVersion: pv.LowerVersion, + } - has, err := db.GetEngine(ctx).Get(key) - if has { - return key, ErrDuplicatePackageVersion - } else if err != nil { - return key, err - } - // This really should never happen and can only happen if this function - // is being called outside of a transaction and between the on conflict insert failing - // the conlicting item is removed. - // + has, err := db.GetEngine(ctx).Get(key) + if has { + return key, ErrDuplicatePackageVersion + } else if err != nil { + return key, err } - // If that weird case has happened 5 times in a row - there is something very odd going on! + // This really should never happen and can only happen if this function + // is being called outside of a transaction and between the on conflict insert failing + // the conlicting item is removed. + // return pv, fmt.Errorf("unable to insert on conflict but yet not able to get from the db") } From 04efbf9c6f44600189059c5bcae55df1cb9de913 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Tue, 7 Feb 2023 16:48:45 +0000 Subject: [PATCH 20/29] more comments Signed-off-by: Andrew Thornton --- models/db/common.go | 108 ++++++++++++++++++++++++++++---------------- 1 file changed, 68 insertions(+), 40 deletions(-) diff --git a/models/db/common.go b/models/db/common.go index 0178f7719c213..9ae2867daa442 100644 --- a/models/db/common.go +++ b/models/db/common.go @@ -44,9 +44,9 @@ func InsertOnConflictDoNothing(ctx context.Context, bean interface{}) (bool, err autoIncrCol := table.AutoIncrColumn() - cols := table.Columns() + columns := table.Columns() - colNames, args, emptyColNames, emptyArgs, err := getColNamesAndArgsFromBean(bean, cols) + colNames, values, zeroedColNames, zeroedValues, err := getColNamesAndValuesFromBean(bean, columns) if err != nil { return false, err } @@ -57,7 +57,7 @@ func InsertOnConflictDoNothing(ctx context.Context, bean interface{}) (bool, err // MSSQL needs to separately pass in the columns with the unique constraint and we need to // include empty columns which are in the constraint in the insert for other dbs - uniqueCols, uniqueArgs, colNames, args := addInUniqueCols(colNames, args, emptyColNames, emptyArgs, table) + uniqueCols, uniqueValues, colNames, values := addInUniqueCols(colNames, values, zeroedColNames, zeroedValues, table) if len(uniqueCols) == 0 { return false, fmt.Errorf("provided bean has no unique constraints") } @@ -65,10 +65,10 @@ func InsertOnConflictDoNothing(ctx context.Context, bean interface{}) (bool, err var insertArgs []any switch { - case setting.Database.UseSQLite3 || setting.Database.UsePostgreSQL || setting.Data - insertArgs = generateInsertNoConflictSQLAndArgs(tableName, colNames, args, autoIncrCol) + case setting.Database.UseSQLite3 || setting.Database.UsePostgreSQL || setting.Database.UseMySQL: + insertArgs = generateInsertNoConflictSQLAndArgs(tableName, colNames, values, autoIncrCol) case setting.Database.UseMSSQL: - insertArgs = generateInsertNoConflictSQLAndArgsForMSSQL(tableName, colNames, args, uniqueCols, uniqueArgs, autoIncrCol) + insertArgs = generateInsertNoConflictSQLAndArgsForMSSQL(tableName, colNames, values, uniqueCols, uniqueValues, autoIncrCol) default: return false, fmt.Errorf("database type not supported") } @@ -103,7 +103,7 @@ func InsertOnConflictDoNothing(ctx context.Context, bean interface{}) (bool, err return true, nil } - res, err := e.Exec(args...) + res, err := e.Exec(values...) if err != nil { return false, err } @@ -195,20 +195,30 @@ func generateInsertNoConflictSQLAndArgsForMSSQL(tableName string, colNames []str return append(uniqueArgs, args[1:]...) } -func addInUniqueCols(colNames []string, args []any, emptyColNames []string, emptyArgs []any, table *schemas.Table) (uniqueCols []string, uniqueArgs []any, insertCols []string, insertArgs []any) { +// addInUniqueCols determines the columns that refer to unique constraints and creates slices for these +// as they're needed by MSSQL. In addition, any columns which are zero-valued but are part of a constraint +// are added back in to the colNames and args +func addInUniqueCols(colNames []string, args []any, zeroedColNames []string, emptyArgs []any, table *schemas.Table) (uniqueCols []string, uniqueArgs []any, insertCols []string, insertArgs []any) { uniqueCols = make([]string, 0, len(table.Columns())) uniqueArgs = make([]interface{}, 1, len(uniqueCols)+1) // leave uniqueArgs[0] empty to put the SQL in + + // Iterate across the indexes in the provided table for _, index := range table.Indexes { if index.Type != schemas.UniqueType { continue } + + // index is a Unique constraint indexCol: for _, iCol := range index.Cols { for _, uCol := range uniqueCols { if uCol == iCol { + // column is already included in uniqueCols so we don't need to add it again continue indexCol } } + + // Now iterate across colNames and add to the uniqueCols for i, col := range colNames { if col == iCol { uniqueCols = append(uniqueCols, col) @@ -216,9 +226,12 @@ func addInUniqueCols(colNames []string, args []any, emptyColNames []string, empt continue indexCol } } - for i, col := range emptyColNames { + + // If we still haven't found the column we need to look in the emptyColumns and add + // it back into colNames and args as well as uniqueCols/uniqueArgs + for i, col := range zeroedColNames { if col == iCol { - // Always include empty unique columns in the insert statement + // Always include empty unique columns in the insert statement as otherwise the insert no conflict will pass colNames = append(colNames, col) args = append(args, emptyArgs[i]) uniqueCols = append(uniqueCols, col) @@ -231,9 +244,18 @@ func addInUniqueCols(colNames []string, args []any, emptyColNames []string, empt return uniqueCols, uniqueArgs, colNames, args } -func getColNamesAndArgsFromBean(bean interface{}, cols []*schemas.Column) (colNames []string, args []any, emptyColNames []string, emptyArgs []any, err error) { +// getColNamesAndValuesFromBean reads the provided bean, providing two pairs of linked slices: +// +// - colNames and values +// - zeroedColNames and zeroedValues +// +// colNames contains the names of the columns that have non-zero values in the provided bean +// values contains the values - with one exception - values is 1-based so that values[0] is deliberately left zero +// +// emptyyColNames and zeroedValues accounts for the other columns - with zeroedValues containing the zero values +func getColNamesAndValuesFromBean(bean interface{}, cols []*schemas.Column) (colNames []string, values []any, zeroedColNames []string, zeroedValues []any, err error) { colNames = make([]string, len(cols)) - args = make([]any, len(cols)+1) // Leave args[0] to put the SQL in + values = make([]any, len(cols)+1) // Leave args[0] to put the SQL in maxNonEmpty := 0 minEmpty := len(cols) @@ -250,7 +272,7 @@ func getColNamesAndArgsFromBean(bean interface{}, cols []*schemas.Column) (colNa colNames[maxNonEmpty] = col.Name maxNonEmpty++ - args[maxNonEmpty] = result + values[maxNonEmpty] = result continue } @@ -259,18 +281,18 @@ func getColNamesAndArgsFromBean(bean interface{}, cols []*schemas.Column) (colNa return nil, nil, nil, nil, err } if fieldVal.IsZero() { - args[minEmpty] = val // remember args is 1-based not 0-based + values[minEmpty] = val // remember args is 1-based not 0-based minEmpty-- colNames[minEmpty] = col.Name continue } colNames[maxNonEmpty] = col.Name maxNonEmpty++ - args[maxNonEmpty] = val + values[maxNonEmpty] = val } } - return colNames[:maxNonEmpty], args[:maxNonEmpty+1], colNames[maxNonEmpty:], args[maxNonEmpty+1:], nil + return colNames[:maxNonEmpty], values[:maxNonEmpty+1], colNames[maxNonEmpty:], values[maxNonEmpty+1:], nil } func setCurrentTime(fieldVal reflect.Value, col *schemas.Column) (interface{}, error) { @@ -291,31 +313,10 @@ func setCurrentTime(fieldVal reflect.Value, col *schemas.Column) (interface{}, e return result, nil } +// getValueFromField extracts the reflected value from the provided fieldVal +// this keeps the type and makes such that zero values work in the SQL Insert above func getValueFromField(fieldVal reflect.Value, col *schemas.Column) (any, error) { - switch fieldVal.Type().Kind() { - case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64: - return fieldVal.Int(), nil - case reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64: - return fieldVal.Uint(), nil - case reflect.Float32, reflect.Float64: - return fieldVal.Float(), nil - case reflect.Complex64, reflect.Complex128: - return fieldVal.Complex(), nil - case reflect.String: - return fieldVal.String(), nil - case reflect.Bool: - valBool := fieldVal.Bool() - - if setting.Database.UseMSSQL { - if valBool { - return 1, nil - } - return 0, nil - } - return valBool, nil - default: - } - + // Handle pointers to convert.Conversion if fieldVal.CanAddr() { if fieldConvert, ok := fieldVal.Addr().Interface().(convert.Conversion); ok { data, err := fieldConvert.ToDB() @@ -335,6 +336,7 @@ func getValueFromField(fieldVal reflect.Value, col *schemas.Column) (any, error) } } + // Handle nil pointer to convert.Conversion isNil := fieldVal.Kind() == reflect.Ptr && fieldVal.IsNil() if !isNil { if fieldConvert, ok := fieldVal.Interface().(convert.Conversion); ok { @@ -355,5 +357,31 @@ func getValueFromField(fieldVal reflect.Value, col *schemas.Column) (any, error) } } + // Handle common primitive types + switch fieldVal.Type().Kind() { + case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64: + return fieldVal.Int(), nil + case reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64: + return fieldVal.Uint(), nil + case reflect.Float32, reflect.Float64: + return fieldVal.Float(), nil + case reflect.Complex64, reflect.Complex128: + return fieldVal.Complex(), nil + case reflect.String: + return fieldVal.String(), nil + case reflect.Bool: + valBool := fieldVal.Bool() + + if setting.Database.UseMSSQL { + if valBool { + return 1, nil + } + return 0, nil + } + return valBool, nil + default: + } + + // just return the interface return fieldVal.Interface(), nil } From 2283b23dedb32731db239f4e2605eae25d27063a Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Tue, 7 Feb 2023 18:01:41 +0000 Subject: [PATCH 21/29] missed setting sql Signed-off-by: Andrew Thornton --- models/db/common.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/models/db/common.go b/models/db/common.go index 9ae2867daa442..aa9d814606e28 100644 --- a/models/db/common.go +++ b/models/db/common.go @@ -191,7 +191,7 @@ func generateInsertNoConflictSQLAndArgsForMSSQL(tableName string, colNames []str write(" OUTPUT INSERTED.", quote(autoIncrCol.Name)) } write(";") - + uniqueArgs[0] = sb.String() return append(uniqueArgs, args[1:]...) } From a282e66942bbd575655c2db8a2f9e627b0769513 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Tue, 7 Feb 2023 19:18:41 +0000 Subject: [PATCH 22/29] add testcases Signed-off-by: Andrew Thornton --- models/db/common.go | 62 ++++++---- tests/integration/db_common_test.go | 182 ++++++++++++++++++++++++++++ 2 files changed, 221 insertions(+), 23 deletions(-) create mode 100644 tests/integration/db_common_test.go diff --git a/models/db/common.go b/models/db/common.go index aa9d814606e28..47bed3d3d6260 100644 --- a/models/db/common.go +++ b/models/db/common.go @@ -57,8 +57,8 @@ func InsertOnConflictDoNothing(ctx context.Context, bean interface{}) (bool, err // MSSQL needs to separately pass in the columns with the unique constraint and we need to // include empty columns which are in the constraint in the insert for other dbs - uniqueCols, uniqueValues, colNames, values := addInUniqueCols(colNames, values, zeroedColNames, zeroedValues, table) - if len(uniqueCols) == 0 { + uniqueColValMap, colNames, values := addInUniqueCols(colNames, values, zeroedColNames, zeroedValues, table) + if len(uniqueColValMap) == 0 { return false, fmt.Errorf("provided bean has no unique constraints") } @@ -68,7 +68,7 @@ func InsertOnConflictDoNothing(ctx context.Context, bean interface{}) (bool, err case setting.Database.UseSQLite3 || setting.Database.UsePostgreSQL || setting.Database.UseMySQL: insertArgs = generateInsertNoConflictSQLAndArgs(tableName, colNames, values, autoIncrCol) case setting.Database.UseMSSQL: - insertArgs = generateInsertNoConflictSQLAndArgsForMSSQL(tableName, colNames, values, uniqueCols, uniqueValues, autoIncrCol) + insertArgs = generateInsertNoConflictSQLAndArgsForMSSQL(table, tableName, colNames, values, uniqueColValMap, autoIncrCol) default: return false, fmt.Errorf("database type not supported") } @@ -103,7 +103,7 @@ func InsertOnConflictDoNothing(ctx context.Context, bean interface{}) (bool, err return true, nil } - res, err := e.Exec(values...) + res, err := e.Exec(insertArgs...) if err != nil { return false, err } @@ -164,7 +164,7 @@ func generateInsertNoConflictSQLAndArgs(tableName string, colNames []string, arg // generateInsertNoConflictSQLAndArgsForMSSQL writes the INSERT ... ON CONFLICT sql variant for MSSQL // MSSQL uses MERGE WITH ... but needs to pre-select the unique cols first // then WHEN NOT MATCHED INSERT - this is kind of the opposite way round from INSERT ... ON CONFLICT -func generateInsertNoConflictSQLAndArgsForMSSQL(tableName string, colNames []string, args []any, uniqueCols []string, uniqueArgs []any, autoIncrCol *schemas.Column) (insertArgs []any) { +func generateInsertNoConflictSQLAndArgsForMSSQL(table *schemas.Table, tableName string, colNames []string, args []any, uniqueColValMap map[string]any, autoIncrCol *schemas.Column) (insertArgs []any) { sb := &strings.Builder{} quote := x.Dialect().Quoter().Quote @@ -173,14 +173,31 @@ func generateInsertNoConflictSQLAndArgsForMSSQL(tableName string, colNames []str _, _ = sb.WriteString(arg) } } + uniqueCols := make([]string, 0, len(uniqueColValMap)) + for colName := range uniqueColValMap { + uniqueCols = append(uniqueCols, colName) + } write("MERGE ", quote(tableName), " WITH (HOLDLOCK) AS target USING (SELECT ? AS ") _ = x.Dialect().Quoter().JoinWrite(sb, uniqueCols, ", ? AS ") - write(") AS src ON src.", quote(uniqueCols[0]), "= target.", quote(uniqueCols[0])) - for _, uniqueCol := range uniqueCols[1:] { - write(" AND src.", quote(uniqueCol), "= target.", quote(uniqueCol)) + write(") AS src ON (") + countUniques := 0 + for _, index := range table.Indexes { + if index.Type != schemas.UniqueType { + continue + } + if countUniques > 0 { + write(" OR ") + } + countUniques++ + write("(") + write("src.", quote(index.Cols[0]), "= target.", quote(index.Cols[0])) + for _, col := range index.Cols[1:] { + write(" AND src.", quote(col), "= target.", quote(col)) + } + write(")") } - write(" WHEN NOT MATCHED THEN INSERT (") + write(") WHEN NOT MATCHED THEN INSERT (") _ = x.Dialect().Quoter().JoinWrite(sb, colNames, ",") write(") VALUES (?") for range colNames[1:] { @@ -191,16 +208,19 @@ func generateInsertNoConflictSQLAndArgsForMSSQL(tableName string, colNames []str write(" OUTPUT INSERTED.", quote(autoIncrCol.Name)) } write(";") - uniqueArgs[0] = sb.String() + uniqueArgs := make([]any, 0, len(uniqueColValMap)+len(args)) + uniqueArgs = append(uniqueArgs, sb.String()) + for _, col := range uniqueCols { + uniqueArgs = append(uniqueArgs, uniqueColValMap[col]) + } return append(uniqueArgs, args[1:]...) } // addInUniqueCols determines the columns that refer to unique constraints and creates slices for these // as they're needed by MSSQL. In addition, any columns which are zero-valued but are part of a constraint // are added back in to the colNames and args -func addInUniqueCols(colNames []string, args []any, zeroedColNames []string, emptyArgs []any, table *schemas.Table) (uniqueCols []string, uniqueArgs []any, insertCols []string, insertArgs []any) { - uniqueCols = make([]string, 0, len(table.Columns())) - uniqueArgs = make([]interface{}, 1, len(uniqueCols)+1) // leave uniqueArgs[0] empty to put the SQL in +func addInUniqueCols(colNames []string, args []any, zeroedColNames []string, emptyArgs []any, table *schemas.Table) (uniqueColValMap map[string]any, insertCols []string, insertArgs []any) { + uniqueColValMap = make(map[string]any) // Iterate across the indexes in the provided table for _, index := range table.Indexes { @@ -211,18 +231,15 @@ func addInUniqueCols(colNames []string, args []any, zeroedColNames []string, emp // index is a Unique constraint indexCol: for _, iCol := range index.Cols { - for _, uCol := range uniqueCols { - if uCol == iCol { - // column is already included in uniqueCols so we don't need to add it again - continue indexCol - } + if _, has := uniqueColValMap[iCol]; has { + // column is already included in uniqueCols so we don't need to add it again + continue indexCol } // Now iterate across colNames and add to the uniqueCols for i, col := range colNames { if col == iCol { - uniqueCols = append(uniqueCols, col) - uniqueArgs = append(uniqueArgs, args[i+1]) + uniqueColValMap[col] = args[i+1] continue indexCol } } @@ -234,14 +251,13 @@ func addInUniqueCols(colNames []string, args []any, zeroedColNames []string, emp // Always include empty unique columns in the insert statement as otherwise the insert no conflict will pass colNames = append(colNames, col) args = append(args, emptyArgs[i]) - uniqueCols = append(uniqueCols, col) - uniqueArgs = append(uniqueArgs, emptyArgs[i]) + uniqueColValMap[col] = emptyArgs[i] continue indexCol } } } } - return uniqueCols, uniqueArgs, colNames, args + return uniqueColValMap, colNames, args } // getColNamesAndValuesFromBean reads the provided bean, providing two pairs of linked slices: diff --git a/tests/integration/db_common_test.go b/tests/integration/db_common_test.go new file mode 100644 index 0000000000000..af87616a5e2fe --- /dev/null +++ b/tests/integration/db_common_test.go @@ -0,0 +1,182 @@ +// Copyright 2022 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package integration + +import ( + "testing" + + "code.gitea.io/gitea/models/db" + "code.gitea.io/gitea/tests" + "github.com/stretchr/testify/assert" +) + +func TestInsertOnConflictDoNothing(t *testing.T) { + defer tests.PrepareTestEnv(t)() + + ctx := db.DefaultContext + e := db.GetEngine(ctx) + t.Run("NoUnique", func(t *testing.T) { + type NoUniques struct { + ID int64 `xorm:"pk autoincr"` + Data string + } + _ = e.Sync2(&NoUniques{}) + + has, err := db.InsertOnConflictDoNothing(ctx, &NoUniques{Data: "shouldErr"}) + assert.Error(t, err) + assert.False(t, has) + + has, err = db.InsertOnConflictDoNothing(ctx, &NoUniques{Data: ""}) + assert.Error(t, err) + assert.False(t, has) + }) + + t.Run("OneUnique", func(t *testing.T) { + type OneUnique struct { + ID int64 `xorm:"pk autoincr"` + Data string `xorm:"UNIQUE NOT NULL"` + } + + _ = e.Sync2(&OneUnique{}) + _, _ = e.Exec("DELETE FROM one_unique") + + has, err := db.InsertOnConflictDoNothing(ctx, &OneUnique{}) + assert.Error(t, err) + assert.False(t, has) + + toInsert := &OneUnique{Data: "test"} + + has, err = db.InsertOnConflictDoNothing(ctx, toInsert) + assert.NoError(t, err) + assert.True(t, has) + assert.NotEqual(t, 0, toInsert.ID) + + has, err = db.InsertOnConflictDoNothing(ctx, &OneUnique{Data: "test2"}) + assert.NoError(t, err) + assert.True(t, has) + + has, err = db.InsertOnConflictDoNothing(ctx, &OneUnique{Data: "test"}) + assert.NoError(t, err) + assert.False(t, has) + }) + + t.Run("MultiUnique", func(t *testing.T) { + type MultiUnique struct { + ID int64 `xorm:"pk autoincr"` + NotUnique string + Data1 string `xorm:"UNIQUE(s) NOT NULL"` + Data2 string `xorm:"UNIQUE(s) NOT NULL"` + } + + _ = e.Sync2(&MultiUnique{}) + _, _ = e.Exec("DELETE FROM multi_unique") + + has, err := db.InsertOnConflictDoNothing(ctx, &MultiUnique{}) + assert.Error(t, err) + assert.False(t, has) + + has, err = db.InsertOnConflictDoNothing(ctx, &MultiUnique{Data1: "test", NotUnique: "t1"}) + assert.NoError(t, err) + assert.True(t, has) + + has, err = db.InsertOnConflictDoNothing(ctx, &MultiUnique{Data2: "test2", NotUnique: "t1"}) + assert.NoError(t, err) + assert.True(t, has) + has, err = db.InsertOnConflictDoNothing(ctx, &MultiUnique{Data2: "test2", NotUnique: "t2"}) + assert.NoError(t, err) + assert.False(t, has) + + has, err = db.InsertOnConflictDoNothing(ctx, &MultiUnique{Data1: "test", NotUnique: "t2"}) + assert.NoError(t, err) + assert.False(t, has) + + has, err = db.InsertOnConflictDoNothing(ctx, &MultiUnique{Data1: "test", Data2: "test2", NotUnique: "t1"}) + assert.NoError(t, err) + assert.True(t, has) + + has, err = db.InsertOnConflictDoNothing(ctx, &MultiUnique{Data1: "test", Data2: "test2", NotUnique: "t2"}) + assert.NoError(t, err) + assert.False(t, has) + }) + + t.Run("MultiMultiUnique", func(t *testing.T) { + type MultiMultiUnique struct { + ID int64 `xorm:"pk autoincr"` + Data0 string `xorm:"UNIQUE NOT NULL"` + Data1 string `xorm:"UNIQUE(s) NOT NULL"` + Data2 string `xorm:"UNIQUE(s) NOT NULL"` + } + + _ = e.Sync2(&MultiMultiUnique{}) + _, _ = e.Exec("DELETE FROM multi_multi_unique") + + has, err := db.InsertOnConflictDoNothing(ctx, &MultiMultiUnique{}) + assert.Error(t, err) + assert.False(t, has) + + has, err = db.InsertOnConflictDoNothing(ctx, &MultiMultiUnique{Data1: "test", Data0: "t1"}) + assert.NoError(t, err) + assert.True(t, has) + + has, err = db.InsertOnConflictDoNothing(ctx, &MultiMultiUnique{Data2: "test2", Data0: "t1"}) + assert.NoError(t, err) + assert.False(t, has) + + has, err = db.InsertOnConflictDoNothing(ctx, &MultiMultiUnique{Data2: "test2", Data0: "t2"}) + assert.NoError(t, err) + assert.True(t, has) + + has, err = db.InsertOnConflictDoNothing(ctx, &MultiMultiUnique{Data2: "test2", Data0: "t2"}) + assert.NoError(t, err) + assert.False(t, has) + + has, err = db.InsertOnConflictDoNothing(ctx, &MultiMultiUnique{Data1: "test", Data0: "t2"}) + assert.NoError(t, err) + assert.False(t, has) + + has, err = db.InsertOnConflictDoNothing(ctx, &MultiMultiUnique{Data1: "test", Data2: "test2", Data0: "t3"}) + assert.NoError(t, err) + assert.True(t, has) + + has, err = db.InsertOnConflictDoNothing(ctx, &MultiMultiUnique{Data1: "test", Data2: "test2", Data0: "t2"}) + assert.NoError(t, err) + assert.False(t, has) + }) + + t.Run("NoPK", func(t *testing.T) { + type NoPrimaryKey struct { + NotID int64 + Uniqued string `xorm:"UNIQUE"` + } + + err := e.Sync2(&NoPrimaryKey{}) + assert.NoError(t, err) + _, _ = e.Exec("DELETE FROM no_primary_key") + + empty := &NoPrimaryKey{} + has, err := db.InsertOnConflictDoNothing(ctx, empty) + assert.Error(t, err) + assert.False(t, has) + + has, err = db.InsertOnConflictDoNothing(ctx, &NoPrimaryKey{Uniqued: "1"}) + assert.NoError(t, err) + assert.True(t, has) + + has, err = db.InsertOnConflictDoNothing(ctx, &NoPrimaryKey{NotID: 1}) + assert.NoError(t, err) + assert.True(t, has) + + has, err = db.InsertOnConflictDoNothing(ctx, &NoPrimaryKey{NotID: 2}) + assert.NoError(t, err) + assert.False(t, has) + + has, err = db.InsertOnConflictDoNothing(ctx, &NoPrimaryKey{NotID: 2, Uniqued: "2"}) + assert.NoError(t, err) + assert.True(t, has) + + has, err = db.InsertOnConflictDoNothing(ctx, &NoPrimaryKey{NotID: 1, Uniqued: "2"}) + assert.NoError(t, err) + assert.False(t, has) + }) +} From 62b1e20f8fa26582843615a2d9a47d36fd8f57b6 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Tue, 7 Feb 2023 19:24:13 +0000 Subject: [PATCH 23/29] fix fmt Signed-off-by: Andrew Thornton --- tests/integration/db_common_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/integration/db_common_test.go b/tests/integration/db_common_test.go index af87616a5e2fe..f0ecc7d34ed1a 100644 --- a/tests/integration/db_common_test.go +++ b/tests/integration/db_common_test.go @@ -8,6 +8,7 @@ import ( "code.gitea.io/gitea/models/db" "code.gitea.io/gitea/tests" + "github.com/stretchr/testify/assert" ) From f1222e8e750bda3a5a723ff7fe2428f383b25a79 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Tue, 7 Feb 2023 19:44:26 +0000 Subject: [PATCH 24/29] slight bug in mysql Signed-off-by: Andrew Thornton --- models/db/common.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/models/db/common.go b/models/db/common.go index 47bed3d3d6260..4c5a464f977d9 100644 --- a/models/db/common.go +++ b/models/db/common.go @@ -155,6 +155,8 @@ func generateInsertNoConflictSQLAndArgs(tableName string, colNames []string, arg case setting.Database.UseMySQL: if autoIncrCol != nil { write(") ON DUPLICATE KEY UPDATE ", quote(autoIncrCol.Name), " = ", quote(autoIncrCol.Name)) + } else { + write(")") } } args[0] = sb.String() From 25abc72fbb9859f480744a58c29760c3bb3de1de Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Wed, 8 Feb 2023 14:18:04 +0000 Subject: [PATCH 25/29] as per wxiaoguang Signed-off-by: Andrew Thornton --- tests/integration/db_common_test.go | 138 ++++++++++++++++------------ 1 file changed, 81 insertions(+), 57 deletions(-) diff --git a/tests/integration/db_common_test.go b/tests/integration/db_common_test.go index f0ecc7d34ed1a..98f6b26943e98 100644 --- a/tests/integration/db_common_test.go +++ b/tests/integration/db_common_test.go @@ -24,13 +24,17 @@ func TestInsertOnConflictDoNothing(t *testing.T) { } _ = e.Sync2(&NoUniques{}) - has, err := db.InsertOnConflictDoNothing(ctx, &NoUniques{Data: "shouldErr"}) + toInsert := &NoUniques{Data: "shouldErr"} + inserted, err := db.InsertOnConflictDoNothing(ctx, toInsert) assert.Error(t, err) - assert.False(t, has) + assert.False(t, inserted) + assert.Equal(t, int64(0), toInsert.ID) - has, err = db.InsertOnConflictDoNothing(ctx, &NoUniques{Data: ""}) + toInsert = &NoUniques{Data: ""} + inserted, err = db.InsertOnConflictDoNothing(ctx, toInsert) assert.Error(t, err) - assert.False(t, has) + assert.False(t, inserted) + assert.Equal(t, int64(0), toInsert.ID) }) t.Run("OneUnique", func(t *testing.T) { @@ -42,24 +46,29 @@ func TestInsertOnConflictDoNothing(t *testing.T) { _ = e.Sync2(&OneUnique{}) _, _ = e.Exec("DELETE FROM one_unique") - has, err := db.InsertOnConflictDoNothing(ctx, &OneUnique{}) + toInsert := &OneUnique{} + inserted, err := db.InsertOnConflictDoNothing(ctx, toInsert) assert.Error(t, err) - assert.False(t, has) + assert.False(t, inserted) + assert.Equal(t, int64(0), toInsert.ID) - toInsert := &OneUnique{Data: "test"} - - has, err = db.InsertOnConflictDoNothing(ctx, toInsert) + toInsert = &OneUnique{Data: "test"} + inserted, err = db.InsertOnConflictDoNothing(ctx, toInsert) assert.NoError(t, err) - assert.True(t, has) - assert.NotEqual(t, 0, toInsert.ID) + assert.True(t, inserted) + assert.NotEqual(t, int64(0), toInsert.ID) - has, err = db.InsertOnConflictDoNothing(ctx, &OneUnique{Data: "test2"}) + toInsert = &OneUnique{Data: "test2"} + inserted, err = db.InsertOnConflictDoNothing(ctx, toInsert) assert.NoError(t, err) - assert.True(t, has) + assert.True(t, inserted) + assert.NotEqual(t, int64(0), toInsert.ID) - has, err = db.InsertOnConflictDoNothing(ctx, &OneUnique{Data: "test"}) + toInsert = &OneUnique{Data: "test"} + inserted, err = db.InsertOnConflictDoNothing(ctx, toInsert) assert.NoError(t, err) - assert.False(t, has) + assert.False(t, inserted) + assert.Equal(t, int64(0), toInsert.ID) }) t.Run("MultiUnique", func(t *testing.T) { @@ -73,32 +82,47 @@ func TestInsertOnConflictDoNothing(t *testing.T) { _ = e.Sync2(&MultiUnique{}) _, _ = e.Exec("DELETE FROM multi_unique") - has, err := db.InsertOnConflictDoNothing(ctx, &MultiUnique{}) + toInsert := &MultiUnique{} + inserted, err := db.InsertOnConflictDoNothing(ctx, toInsert) assert.Error(t, err) - assert.False(t, has) + assert.False(t, inserted) + assert.Equal(t, int64(0), toInsert.ID) - has, err = db.InsertOnConflictDoNothing(ctx, &MultiUnique{Data1: "test", NotUnique: "t1"}) + toInsert = &MultiUnique{Data1: "test", NotUnique: "t1"} + inserted, err = db.InsertOnConflictDoNothing(ctx, toInsert) assert.NoError(t, err) - assert.True(t, has) + assert.True(t, inserted) + assert.NotEqual(t, int64(0), toInsert.ID) - has, err = db.InsertOnConflictDoNothing(ctx, &MultiUnique{Data2: "test2", NotUnique: "t1"}) + toInsert = &MultiUnique{Data1: "test2", NotUnique: "t1"} + inserted, err = db.InsertOnConflictDoNothing(ctx, toInsert) assert.NoError(t, err) - assert.True(t, has) - has, err = db.InsertOnConflictDoNothing(ctx, &MultiUnique{Data2: "test2", NotUnique: "t2"}) + assert.True(t, inserted) + assert.NotEqual(t, int64(0), toInsert.ID) + + toInsert = &MultiUnique{Data1: "test2", NotUnique: "t2"} + inserted, err = db.InsertOnConflictDoNothing(ctx, toInsert) assert.NoError(t, err) - assert.False(t, has) + assert.False(t, inserted) + assert.Equal(t, int64(0), toInsert.ID) - has, err = db.InsertOnConflictDoNothing(ctx, &MultiUnique{Data1: "test", NotUnique: "t2"}) + toInsert = &MultiUnique{Data1: "test", NotUnique: "t2"} + inserted, err = db.InsertOnConflictDoNothing(ctx, toInsert) assert.NoError(t, err) - assert.False(t, has) + assert.False(t, inserted) + assert.Equal(t, int64(0), toInsert.ID) - has, err = db.InsertOnConflictDoNothing(ctx, &MultiUnique{Data1: "test", Data2: "test2", NotUnique: "t1"}) + toInsert = &MultiUnique{Data1: "test", Data2: "test2", NotUnique: "t1"} + inserted, err = db.InsertOnConflictDoNothing(ctx, toInsert) assert.NoError(t, err) - assert.True(t, has) + assert.True(t, inserted) + assert.NotEqual(t, int64(0), toInsert.ID) - has, err = db.InsertOnConflictDoNothing(ctx, &MultiUnique{Data1: "test", Data2: "test2", NotUnique: "t2"}) + toInsert = &MultiUnique{Data1: "test", Data2: "test2", NotUnique: "t2"} + inserted, err = db.InsertOnConflictDoNothing(ctx, toInsert) assert.NoError(t, err) - assert.False(t, has) + assert.False(t, inserted) + assert.Equal(t, int64(0), toInsert.ID) }) t.Run("MultiMultiUnique", func(t *testing.T) { @@ -112,37 +136,37 @@ func TestInsertOnConflictDoNothing(t *testing.T) { _ = e.Sync2(&MultiMultiUnique{}) _, _ = e.Exec("DELETE FROM multi_multi_unique") - has, err := db.InsertOnConflictDoNothing(ctx, &MultiMultiUnique{}) + inserted, err := db.InsertOnConflictDoNothing(ctx, &MultiMultiUnique{}) assert.Error(t, err) - assert.False(t, has) + assert.False(t, inserted) - has, err = db.InsertOnConflictDoNothing(ctx, &MultiMultiUnique{Data1: "test", Data0: "t1"}) + inserted, err = db.InsertOnConflictDoNothing(ctx, &MultiMultiUnique{Data1: "test", Data0: "t1"}) assert.NoError(t, err) - assert.True(t, has) + assert.True(t, inserted) - has, err = db.InsertOnConflictDoNothing(ctx, &MultiMultiUnique{Data2: "test2", Data0: "t1"}) + inserted, err = db.InsertOnConflictDoNothing(ctx, &MultiMultiUnique{Data2: "test2", Data0: "t1"}) assert.NoError(t, err) - assert.False(t, has) + assert.False(t, inserted) - has, err = db.InsertOnConflictDoNothing(ctx, &MultiMultiUnique{Data2: "test2", Data0: "t2"}) + inserted, err = db.InsertOnConflictDoNothing(ctx, &MultiMultiUnique{Data2: "test2", Data0: "t2"}) assert.NoError(t, err) - assert.True(t, has) + assert.True(t, inserted) - has, err = db.InsertOnConflictDoNothing(ctx, &MultiMultiUnique{Data2: "test2", Data0: "t2"}) + inserted, err = db.InsertOnConflictDoNothing(ctx, &MultiMultiUnique{Data2: "test2", Data0: "t2"}) assert.NoError(t, err) - assert.False(t, has) + assert.False(t, inserted) - has, err = db.InsertOnConflictDoNothing(ctx, &MultiMultiUnique{Data1: "test", Data0: "t2"}) + inserted, err = db.InsertOnConflictDoNothing(ctx, &MultiMultiUnique{Data1: "test", Data0: "t2"}) assert.NoError(t, err) - assert.False(t, has) + assert.False(t, inserted) - has, err = db.InsertOnConflictDoNothing(ctx, &MultiMultiUnique{Data1: "test", Data2: "test2", Data0: "t3"}) + inserted, err = db.InsertOnConflictDoNothing(ctx, &MultiMultiUnique{Data1: "test", Data2: "test2", Data0: "t3"}) assert.NoError(t, err) - assert.True(t, has) + assert.True(t, inserted) - has, err = db.InsertOnConflictDoNothing(ctx, &MultiMultiUnique{Data1: "test", Data2: "test2", Data0: "t2"}) + inserted, err = db.InsertOnConflictDoNothing(ctx, &MultiMultiUnique{Data1: "test", Data2: "test2", Data0: "t2"}) assert.NoError(t, err) - assert.False(t, has) + assert.False(t, inserted) }) t.Run("NoPK", func(t *testing.T) { @@ -156,28 +180,28 @@ func TestInsertOnConflictDoNothing(t *testing.T) { _, _ = e.Exec("DELETE FROM no_primary_key") empty := &NoPrimaryKey{} - has, err := db.InsertOnConflictDoNothing(ctx, empty) + inserted, err := db.InsertOnConflictDoNothing(ctx, empty) assert.Error(t, err) - assert.False(t, has) + assert.False(t, inserted) - has, err = db.InsertOnConflictDoNothing(ctx, &NoPrimaryKey{Uniqued: "1"}) + inserted, err = db.InsertOnConflictDoNothing(ctx, &NoPrimaryKey{Uniqued: "1"}) assert.NoError(t, err) - assert.True(t, has) + assert.True(t, inserted) - has, err = db.InsertOnConflictDoNothing(ctx, &NoPrimaryKey{NotID: 1}) + inserted, err = db.InsertOnConflictDoNothing(ctx, &NoPrimaryKey{NotID: 1}) assert.NoError(t, err) - assert.True(t, has) + assert.True(t, inserted) - has, err = db.InsertOnConflictDoNothing(ctx, &NoPrimaryKey{NotID: 2}) + inserted, err = db.InsertOnConflictDoNothing(ctx, &NoPrimaryKey{NotID: 2}) assert.NoError(t, err) - assert.False(t, has) + assert.False(t, inserted) - has, err = db.InsertOnConflictDoNothing(ctx, &NoPrimaryKey{NotID: 2, Uniqued: "2"}) + inserted, err = db.InsertOnConflictDoNothing(ctx, &NoPrimaryKey{NotID: 2, Uniqued: "2"}) assert.NoError(t, err) - assert.True(t, has) + assert.True(t, inserted) - has, err = db.InsertOnConflictDoNothing(ctx, &NoPrimaryKey{NotID: 1, Uniqued: "2"}) + inserted, err = db.InsertOnConflictDoNothing(ctx, &NoPrimaryKey{NotID: 1, Uniqued: "2"}) assert.NoError(t, err) - assert.False(t, has) + assert.False(t, inserted) }) } From 028b5a6e29147137f5d7c4a24c54a51ca6eda48b Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Wed, 8 Feb 2023 23:53:31 +0000 Subject: [PATCH 26/29] split functions out and just use ignore Signed-off-by: Andrew Thornton --- models/db/common.go | 73 ++++++++++++++++++++++++++++++++------------- 1 file changed, 52 insertions(+), 21 deletions(-) diff --git a/models/db/common.go b/models/db/common.go index 4c5a464f977d9..89fa19ef5b356 100644 --- a/models/db/common.go +++ b/models/db/common.go @@ -65,8 +65,12 @@ func InsertOnConflictDoNothing(ctx context.Context, bean interface{}) (bool, err var insertArgs []any switch { - case setting.Database.UseSQLite3 || setting.Database.UsePostgreSQL || setting.Database.UseMySQL: - insertArgs = generateInsertNoConflictSQLAndArgs(tableName, colNames, values, autoIncrCol) + case setting.Database.UseSQLite3: + insertArgs = generateInsertNoConflictSQLAndArgsForSQLite(tableName, colNames, values) + case setting.Database.UsePostgreSQL: + insertArgs = generateInsertNoConflictSQLAndArgsForPostgres(tableName, colNames, values, autoIncrCol) + case setting.Database.UseMySQL: + insertArgs = generateInsertNoConflictSQLAndArgsForMySQL(tableName, colNames, values) case setting.Database.UseMSSQL: insertArgs = generateInsertNoConflictSQLAndArgsForMSSQL(table, tableName, colNames, values, uniqueColValMap, autoIncrCol) default: @@ -124,8 +128,8 @@ func InsertOnConflictDoNothing(ctx context.Context, bean interface{}) (bool, err return n != 0, err } -// generateInsertNoConflictSQLAndArgs will create the correct insert code for most of the DBs except MSSQL -func generateInsertNoConflictSQLAndArgs(tableName string, colNames []string, args []any, autoIncrCol *schemas.Column) (insertArgs []any) { +// generateInsertNoConflictSQLAndArgsForSQLite will create the correct insert code for SQLite +func generateInsertNoConflictSQLAndArgsForSQLite(tableName string, colNames []string, args []any) (insertArgs []any) { sb := &strings.Builder{} quote := x.Dialect().Quoter().Quote @@ -134,31 +138,58 @@ func generateInsertNoConflictSQLAndArgs(tableName string, colNames []string, arg _, _ = sb.WriteString(arg) } } - write("INSERT ") - if setting.Database.UseMySQL && autoIncrCol == nil { - write("IGNORE ") - } - write("INTO ", quote(tableName), " (") + write("INSERT INTO ", quote(tableName), " (") _ = x.Dialect().Quoter().JoinWrite(sb, colNames, ",") write(") VALUES (?") for range colNames[1:] { write(",?") } - switch { - case setting.Database.UsePostgreSQL: - write(") ON CONFLICT DO NOTHING") - if autoIncrCol != nil { - write(" RETURNING ", quote(autoIncrCol.Name)) + write(") ON CONFLICT DO NOTHING") + args[0] = sb.String() + return args +} + +// generateInsertNoConflictSQLAndArgsForPostgres will create the correct insert code for Postgres +func generateInsertNoConflictSQLAndArgsForPostgres(tableName string, colNames []string, args []any, autoIncrCol *schemas.Column) (insertArgs []any) { + sb := &strings.Builder{} + + quote := x.Dialect().Quoter().Quote + write := func(args ...string) { + for _, arg := range args { + _, _ = sb.WriteString(arg) } - case setting.Database.UseSQLite3: - write(") ON CONFLICT DO NOTHING") - case setting.Database.UseMySQL: - if autoIncrCol != nil { - write(") ON DUPLICATE KEY UPDATE ", quote(autoIncrCol.Name), " = ", quote(autoIncrCol.Name)) - } else { - write(")") + } + write("INSERT INTO ", quote(tableName), " (") + _ = x.Dialect().Quoter().JoinWrite(sb, colNames, ",") + write(") VALUES (?") + for range colNames[1:] { + write(",?") + } + write(") ON CONFLICT DO NOTHING") + if autoIncrCol != nil { + write(" RETURNING ", quote(autoIncrCol.Name)) + } + args[0] = sb.String() + return args +} + +// generateInsertNoConflictSQLAndArgsForMySQL will create the correct insert code for MySQL +func generateInsertNoConflictSQLAndArgsForMySQL(tableName string, colNames []string, args []any) (insertArgs []any) { + sb := &strings.Builder{} + + quote := x.Dialect().Quoter().Quote + write := func(args ...string) { + for _, arg := range args { + _, _ = sb.WriteString(arg) } } + write("INSERT IGNORE INTO ", quote(tableName), " (") + _ = x.Dialect().Quoter().JoinWrite(sb, colNames, ",") + write(") VALUES (?") + for range colNames[1:] { + write(",?") + } + write(")") args[0] = sb.String() return args } From ac6862ae4749a9c3748f99df96c368c673cb7094 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Thu, 9 Feb 2023 19:25:19 +0000 Subject: [PATCH 27/29] remove unnecessary mutex Signed-off-by: Andrew Thornton --- routers/api/packages/container/blob.go | 7 ------- 1 file changed, 7 deletions(-) diff --git a/routers/api/packages/container/blob.go b/routers/api/packages/container/blob.go index f0457c55e19c3..ba969778246c0 100644 --- a/routers/api/packages/container/blob.go +++ b/routers/api/packages/container/blob.go @@ -10,7 +10,6 @@ import ( "fmt" "os" "strings" - "sync" "code.gitea.io/gitea/models/db" packages_model "code.gitea.io/gitea/models/packages" @@ -22,8 +21,6 @@ import ( packages_service "code.gitea.io/gitea/services/packages" ) -var uploadVersionMutex sync.Mutex - // saveAsPackageBlob creates a package blob from an upload // The uploaded blob gets stored in a special upload version to link them to the package/image func saveAsPackageBlob(hsr packages_module.HashedSizeReader, pci *packages_service.PackageCreationInfo) (*packages_model.PackageBlob, error) { @@ -93,9 +90,6 @@ func mountBlob(pi *packages_service.PackageInfo, pb *packages_model.PackageBlob) func getOrCreateUploadVersion(pi *packages_service.PackageInfo) (*packages_model.PackageVersion, error) { var uploadVersion *packages_model.PackageVersion - // FIXME: Replace usage of mutex with database transaction - // https://github.com/go-gitea/gitea/pull/21862 - uploadVersionMutex.Lock() err := db.WithTx(db.DefaultContext, func(ctx context.Context) error { created := true p := &packages_model.Package{ @@ -140,7 +134,6 @@ func getOrCreateUploadVersion(pi *packages_service.PackageInfo) (*packages_model return nil }) - uploadVersionMutex.Unlock() return uploadVersion, err } From bf18cf8db44040640cd38c11b0e0926ec639494b Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Sun, 12 Mar 2023 11:32:32 +0000 Subject: [PATCH 28/29] add a few comments to test; Signed-off-by: Andrew Thornton --- tests/integration/db_common_test.go | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/tests/integration/db_common_test.go b/tests/integration/db_common_test.go index 98f6b26943e98..0b4c22aa9d6d6 100644 --- a/tests/integration/db_common_test.go +++ b/tests/integration/db_common_test.go @@ -24,12 +24,14 @@ func TestInsertOnConflictDoNothing(t *testing.T) { } _ = e.Sync2(&NoUniques{}) + // InsertOnConflictDoNothing does not work if there is no unique constraint toInsert := &NoUniques{Data: "shouldErr"} inserted, err := db.InsertOnConflictDoNothing(ctx, toInsert) assert.Error(t, err) assert.False(t, inserted) assert.Equal(t, int64(0), toInsert.ID) + // InsertOnConflictDoNothing does not work if there is no unique constraint toInsert = &NoUniques{Data: ""} inserted, err = db.InsertOnConflictDoNothing(ctx, toInsert) assert.Error(t, err) @@ -46,24 +48,28 @@ func TestInsertOnConflictDoNothing(t *testing.T) { _ = e.Sync2(&OneUnique{}) _, _ = e.Exec("DELETE FROM one_unique") + // Cannot insert if the unique field is NULL toInsert := &OneUnique{} inserted, err := db.InsertOnConflictDoNothing(ctx, toInsert) assert.Error(t, err) assert.False(t, inserted) assert.Equal(t, int64(0), toInsert.ID) + // Successfully insert test toInsert = &OneUnique{Data: "test"} inserted, err = db.InsertOnConflictDoNothing(ctx, toInsert) assert.NoError(t, err) assert.True(t, inserted) assert.NotEqual(t, int64(0), toInsert.ID) + // Successfully insert test2 toInsert = &OneUnique{Data: "test2"} inserted, err = db.InsertOnConflictDoNothing(ctx, toInsert) assert.NoError(t, err) assert.True(t, inserted) assert.NotEqual(t, int64(0), toInsert.ID) + // Successfully not insert test toInsert = &OneUnique{Data: "test"} inserted, err = db.InsertOnConflictDoNothing(ctx, toInsert) assert.NoError(t, err) @@ -82,42 +88,49 @@ func TestInsertOnConflictDoNothing(t *testing.T) { _ = e.Sync2(&MultiUnique{}) _, _ = e.Exec("DELETE FROM multi_unique") + // Cannot insert if the unique fields are null toInsert := &MultiUnique{} inserted, err := db.InsertOnConflictDoNothing(ctx, toInsert) assert.Error(t, err) assert.False(t, inserted) assert.Equal(t, int64(0), toInsert.ID) + // successfully insert test, t1 toInsert = &MultiUnique{Data1: "test", NotUnique: "t1"} inserted, err = db.InsertOnConflictDoNothing(ctx, toInsert) assert.NoError(t, err) assert.True(t, inserted) assert.NotEqual(t, int64(0), toInsert.ID) + // successfully insert test2, t1 toInsert = &MultiUnique{Data1: "test2", NotUnique: "t1"} inserted, err = db.InsertOnConflictDoNothing(ctx, toInsert) assert.NoError(t, err) assert.True(t, inserted) assert.NotEqual(t, int64(0), toInsert.ID) + // successfully don't insert test2, t2 toInsert = &MultiUnique{Data1: "test2", NotUnique: "t2"} inserted, err = db.InsertOnConflictDoNothing(ctx, toInsert) assert.NoError(t, err) assert.False(t, inserted) assert.Equal(t, int64(0), toInsert.ID) + // successfully don't insert test, t2 toInsert = &MultiUnique{Data1: "test", NotUnique: "t2"} inserted, err = db.InsertOnConflictDoNothing(ctx, toInsert) assert.NoError(t, err) assert.False(t, inserted) assert.Equal(t, int64(0), toInsert.ID) + // successfully insert test/test2, t2 toInsert = &MultiUnique{Data1: "test", Data2: "test2", NotUnique: "t1"} inserted, err = db.InsertOnConflictDoNothing(ctx, toInsert) assert.NoError(t, err) assert.True(t, inserted) assert.NotEqual(t, int64(0), toInsert.ID) + // successfully don't insert test/test2, t2 toInsert = &MultiUnique{Data1: "test", Data2: "test2", NotUnique: "t2"} inserted, err = db.InsertOnConflictDoNothing(ctx, toInsert) assert.NoError(t, err) From 1a6f5df0e5b839f39530b8f396026d66e1b8ef81 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Sun, 12 Mar 2023 12:25:52 +0000 Subject: [PATCH 29/29] fix broken merge Signed-off-by: Andrew Thornton --- models/db/common.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/models/db/common.go b/models/db/common.go index 2b1a05d394239..aa529ef87e8dd 100644 --- a/models/db/common.go +++ b/models/db/common.go @@ -65,19 +65,19 @@ func InsertOnConflictDoNothing(ctx context.Context, bean interface{}) (bool, err var insertArgs []any switch { - case setting.Database.UseSQLite3: + case setting.Database.Type.IsSQLite3(): insertArgs = generateInsertNoConflictSQLAndArgsForSQLite(tableName, colNames, values) - case setting.Database.UsePostgreSQL: + case setting.Database.Type.IsPostgreSQL(): insertArgs = generateInsertNoConflictSQLAndArgsForPostgres(tableName, colNames, values, autoIncrCol) - case setting.Database.UseMySQL: + case setting.Database.Type.IsMySQL(): insertArgs = generateInsertNoConflictSQLAndArgsForMySQL(tableName, colNames, values) - case setting.Database.UseMSSQL: + case setting.Database.Type.IsMSSQL(): insertArgs = generateInsertNoConflictSQLAndArgsForMSSQL(table, tableName, colNames, values, uniqueColValMap, autoIncrCol) default: return false, fmt.Errorf("database type not supported") } - if autoIncrCol != nil && (setting.Database.UsePostgreSQL || setting.Database.UseMSSQL) { + if autoIncrCol != nil && (setting.Database.Type.IsPostgreSQL() || setting.Database.Type.IsMSSQL()) { // Postgres and MSSQL do not use the LastInsertID mechanism // Therefore use query rather than exec and read the last provided ID back in @@ -421,7 +421,7 @@ func getValueFromField(fieldVal reflect.Value, col *schemas.Column) (any, error) case reflect.Bool: valBool := fieldVal.Bool() - if setting.Database.UseMSSQL { + if setting.Database.Type.IsMSSQL() { if valBool { return 1, nil }