From 7f4e851f06d33dda1bfe0b1f7d93f4cf6ba4d617 Mon Sep 17 00:00:00 2001
From: Andrew Thornton <art27@cantab.net>
Date: Sun, 4 Sep 2022 21:45:44 +0100
Subject: [PATCH] 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 <art27@cantab.net>
---
 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