TT-17841: improved tests for persistent storage

.github/workflows/ci-tests.yml

@@ -192,6 +192,8 @@ jobs:
           # Redis; empty elsewhere so the test self-skips.
           TEST_IAM_REDIS_ADDR: ${{ matrix.storagetype == 'iam' && 'localhost:6379' || '' }}
           TEST_IAM_REDIS_PASS: ${{ matrix.storagetype == 'iam' && 'tokenAAA' || '' }}
+          # Postgres DSN must match the credentials set by bin/Taskfile-db.yml start-postgres.
+          postgres_test_dsn: ${{ matrix.database == 'postgres' && 'host=localhost port=5432 user=testuser password=testpass dbname=testdb sslmode=disable' || '' }}
         run: task test-${{ matrix.storagetype }}
       - uses: actions/upload-artifact@v4
         with:
@@ -269,7 +271,7 @@ jobs:
             -Dsonar.sources=.
             -Dsonar.coverage.exclusions=**/*_test.go,**/mocks/*.go,**/tempmocks/*.go
             -Dsonar.exclusions=**/mocks/*.go,**/tempmocks/*.go
-            -Dsonar.test.inclusions=**/*_test.go
+            -Dsonar.test.inclusions=**/*_test.go,**/internal/testutil/**
             -Dsonar.tests=.
             -Dsonar.go.coverage.reportPaths=merged-coverage.cov
             -Dsonar.go.golangci-lint.reportPaths=golanglint.xml

Taskfile.yml

@@ -24,11 +24,19 @@ tasks:
           echo "STORAGE_TYPE and DB must be set."
           exit 1
         fi
-        for pkg in $(go list ./{{.STORAGE_TYPE}}/...); do
+        tags=""
+        if [[ "{{.DB}}" != "none" && -n "{{.DB}}" ]]; then
+          tags="-tags {{.DB}}{{.DB_VERSION}}"
+        fi
+        # Only iterate packages that have test files under the active build tag.
+        # Packages with no tests under the current tag still get coverage via
+        # -coverpkg instrumentation from other packages' test runs, but we avoid
+        # generating zero-run coverage files that can contain empty lines and
+        # break gocovmerge.
+        for pkg in $(go list $tags -f '{{`{{`}}if or .TestGoFiles .XTestGoFiles{{`}}`}}{{`{{`}}.ImportPath{{`}}`}}{{`{{`}}end{{`}}`}}' ./{{.STORAGE_TYPE}}/...); do
           pkg_base=$(basename $pkg)
           coveragefile="${pkg_base}.cov"
-          tags=$([[ ${pkg} == *"driver"* ]] && echo "-tags {{.DB}}{{.DB_VERSION}} " || echo "")
-          go test -failfast -timeout $TEST_TIMEOUT -race -cover $tags -coverprofile=${coveragefile} -v ${pkg}
+          go test -failfast -timeout $TEST_TIMEOUT -race -cover -coverpkg=./{{.STORAGE_TYPE}}/... $tags -coverprofile=${coveragefile} -v ${pkg}
           if [ -f "${coveragefile}" ]; then
             mkdir -p "coverage/$(dirname ${pkg})"
             mv ${coveragefile} "coverage/${pkg}.cov"
@@ -39,6 +47,13 @@ tasks:
     desc: "Merge coverage files into a single file"
     cmds:
       - |
+        # Strip empty lines from coverage files before merging. Go 1.25 can write
+        # them for packages with zero test runs when -coverpkg is active, which
+        # causes gocovmerge's cover.ParseProfiles to fail on the empty line.
+        # sed edits in place, so a file consisting only of blank lines is still
+        # fixed (the grep+mv form skipped the mv when grep matched nothing).
+        find . -name '*.cov' -type f -exec sed -i.bak '/^[[:space:]]*$/d' {} +
+        find . -name '*.cov.bak' -type f -delete
         find . -name '*.cov' -type f -exec gocovmerge {} + > merged-coverage.cov
 
   test-persistent:

persistent/conformance_mgo_test.go

@@ -0,0 +1,38 @@
+//go:build mongo4.4 || mongo4.2 || mongo4.0 || mongo3.6 || mongo3.4 || mongo3.2 || mongo3.0 || mongo2.6
+// +build mongo4.4 mongo4.2 mongo4.0 mongo3.6 mongo3.4 mongo3.2 mongo3.0 mongo2.6
+
+package persistent_test
+
+import (
+	"context"
+	"testing"
+
+	"github.com/TykTechnologies/storage/persistent"
+	"github.com/TykTechnologies/storage/persistent/internal/testutil"
+	"github.com/TykTechnologies/storage/persistent/model"
+	"github.com/stretchr/testify/require"
+)
+
+type mgoConformanceEntity struct {
+	ID   model.ObjectID `bson:"_id,omitempty"`
+	Name string         `bson:"name"`
+}
+
+func (e *mgoConformanceEntity) TableName() string             { return "conformance_entities" }
+func (e *mgoConformanceEntity) GetObjectID() model.ObjectID   { return e.ID }
+func (e *mgoConformanceEntity) SetObjectID(id model.ObjectID) { e.ID = id }
+
+func TestConformanceMgo(t *testing.T) {
+	storage, err := persistent.NewPersistentStorage(&persistent.ClientOpts{
+		ConnectionString: "mongodb://localhost:27017/test",
+		Type:             persistent.Mgo,
+	})
+	require.NoError(t, err)
+	require.NoError(t, storage.Ping(context.Background()))
+
+	testutil.RunSuite(t, testutil.Suite{
+		Storage:   storage,
+		NewObject: func() model.DBObject { return &mgoConformanceEntity{} },
+		IDFilter:  func(id model.ObjectID) model.DBM { return model.DBM{"_id": id} },
+	})
+}

persistent/conformance_mongo_test.go

@@ -0,0 +1,38 @@
+//go:build mongo7.0 || mongo7 || mongo6.0 || mongo6 || mongo4.4 || mongo4.2 || mongo4.0 || mongo3.6 || mongo3.4 || mongo3.2 || mongo3.0 || mongo2.6
+// +build mongo7.0 mongo7 mongo6.0 mongo6 mongo4.4 mongo4.2 mongo4.0 mongo3.6 mongo3.4 mongo3.2 mongo3.0 mongo2.6
+
+package persistent_test
+
+import (
+	"context"
+	"testing"
+
+	"github.com/TykTechnologies/storage/persistent"
+	"github.com/TykTechnologies/storage/persistent/internal/testutil"
+	"github.com/TykTechnologies/storage/persistent/model"
+	"github.com/stretchr/testify/require"
+)
+
+type mongoConformanceEntity struct {
+	ID   model.ObjectID `bson:"_id,omitempty"`
+	Name string         `bson:"name"`
+}
+
+func (e *mongoConformanceEntity) TableName() string             { return "conformance_entities" }
+func (e *mongoConformanceEntity) GetObjectID() model.ObjectID   { return e.ID }
+func (e *mongoConformanceEntity) SetObjectID(id model.ObjectID) { e.ID = id }
+
+func TestConformanceOfficialMongo(t *testing.T) {
+	storage, err := persistent.NewPersistentStorage(&persistent.ClientOpts{
+		ConnectionString: "mongodb://localhost:27017/test",
+		Type:             persistent.OfficialMongo,
+	})
+	require.NoError(t, err)
+	require.NoError(t, storage.Ping(context.Background()))
+
+	testutil.RunSuite(t, testutil.Suite{
+		Storage:   storage,
+		NewObject: func() model.DBObject { return &mongoConformanceEntity{} },
+		IDFilter:  func(id model.ObjectID) model.DBM { return model.DBM{"_id": id} },
+	})
+}

persistent/conformance_postgres_test.go

@@ -0,0 +1,49 @@
+//go:build postgres || postgres16.10 || postgres16.1 || postgres15.0 || postgres15 || postgres14.11 || postgres13.3 || postgres12.22
+// +build postgres postgres16.10 postgres16.1 postgres15.0 postgres15 postgres14.11 postgres13.3 postgres12.22
+
+package persistent_test
+
+import (
+	"context"
+	"os"
+	"testing"
+
+	"github.com/TykTechnologies/storage/persistent"
+	"github.com/TykTechnologies/storage/persistent/internal/testutil"
+	"github.com/TykTechnologies/storage/persistent/model"
+	"github.com/stretchr/testify/require"
+)
+
+type pgConformanceEntity struct {
+	ID   model.ObjectID `json:"id" gorm:"primaryKey"`
+	Name string         `json:"name"`
+}
+
+func (e *pgConformanceEntity) TableName() string             { return "conformance_entities" }
+func (e *pgConformanceEntity) GetObjectID() model.ObjectID   { return e.ID }
+func (e *pgConformanceEntity) SetObjectID(id model.ObjectID) { e.ID = id }
+
+func postgresConnStr() string {
+	if dsn := os.Getenv("postgres_test_dsn"); dsn != "" {
+		return dsn
+	}
+
+	// Fallback matches the Docker credentials set by bin/Taskfile-db.yml start-postgres.
+	// In CI, postgres_test_dsn is set explicitly in the workflow env.
+	return "host=localhost port=5432 user=testuser password=testpass dbname=testdb sslmode=disable"
+}
+
+func TestConformancePostgres(t *testing.T) {
+	storage, err := persistent.NewPersistentStorage(&persistent.ClientOpts{
+		ConnectionString: postgresConnStr(),
+		Type:             persistent.Postgres,
+	})
+	require.NoError(t, err)
+	require.NoError(t, storage.Ping(context.Background()))
+
+	testutil.RunSuite(t, testutil.Suite{
+		Storage:   storage,
+		NewObject: func() model.DBObject { return &pgConformanceEntity{} },
+		IDFilter:  func(id model.ObjectID) model.DBM { return model.DBM{"id": id} },
+	})
+}

persistent/internal/driver/mgo/mgo.go

@@ -20,6 +20,10 @@ import (
 
 var _ types.PersistentStorage = &mgoDriver{}
 
+// upsertMaxRetries bounds retries of an upsert that lost the insert race with a
+// concurrent upsert of the same document (duplicate-key error on pre-5.0 servers).
+const upsertMaxRetries = 3
+
 type mgoDriver struct {
 	*lifeCycle
 	lastConnAttempt time.Time
@@ -501,11 +505,21 @@ func (d *mgoDriver) Upsert(ctx context.Context, row model.DBObject, query, updat
 
 	col := sess.DB("").C(row.TableName())
 
-	_, err := col.Find(query).Apply(mgo.Change{
-		Update:    update,
-		Upsert:    true,
-		ReturnNew: true,
-	}, row)
+	// Concurrent upserts on the same not-yet-existing document can race on the
+	// insert path: two executions both miss, both try to insert, and one loses
+	// with a duplicate-key error. Servers before 5.0 do not retry this
+	// internally, so retry here; the losing call now sees the winner's document.
+	var err error
+	for attempt := 0; attempt < upsertMaxRetries; attempt++ {
+		_, err = col.Find(query).Apply(mgo.Change{
+			Update:    update,
+			Upsert:    true,
+			ReturnNew: true,
+		}, row)
+		if !mgo.IsDup(err) {
+			break
+		}
+	}
 
 	return d.handleStoreError(err)
 }

persistent/internal/driver/mongo/life_cycle_test.go

@@ -1,5 +1,5 @@
-//go:build mongo7 || mongo6 || mongo4.4 || mongo4.2 || mongo4.0 || mongo3.6 || mongo3.4 || mongo3.2 || mongo3.0 || mongo2.6
-// +build mongo7 mongo6 mongo4.4 mongo4.2 mongo4.0 mongo3.6 mongo3.4 mongo3.2 mongo3.0 mongo2.6
+//go:build mongo7.0 || mongo7 || mongo6.0 || mongo6 || mongo4.4 || mongo4.2 || mongo4.0 || mongo3.6 || mongo3.4 || mongo3.2 || mongo3.0 || mongo2.6
+// +build mongo7.0 mongo7 mongo6.0 mongo6 mongo4.4 mongo4.2 mongo4.0 mongo3.6 mongo3.4 mongo3.2 mongo3.0 mongo2.6
 
 package mongo
 

persistent/internal/driver/mongo/migrations_test.go

@@ -1,5 +1,5 @@
-//go:build mongo7 || mongo6 || mongo4.4 || mongo4.2 || mongo4.0 || mongo3.6 || mongo3.4 || mongo3.2 || mongo3.0 || mongo2.6
-// +build mongo7 mongo6 mongo4.4 mongo4.2 mongo4.0 mongo3.6 mongo3.4 mongo3.2 mongo3.0 mongo2.6
+//go:build mongo7.0 || mongo7 || mongo6.0 || mongo6 || mongo4.4 || mongo4.2 || mongo4.0 || mongo3.6 || mongo3.4 || mongo3.2 || mongo3.0 || mongo2.6
+// +build mongo7.0 mongo7 mongo6.0 mongo6 mongo4.4 mongo4.2 mongo4.0 mongo3.6 mongo3.4 mongo3.2 mongo3.0 mongo2.6
 
 package mongo
 

persistent/internal/driver/mongo/mongo.go

@@ -3,6 +3,7 @@ package mongo
 import (
 	"context"
 	"errors"
+
 	"go.mongodb.org/mongo-driver/bson"
 	"go.mongodb.org/mongo-driver/bson/primitive"
 	"go.mongodb.org/mongo-driver/mongo"
@@ -16,6 +17,10 @@ import (
 
 var _ types.PersistentStorage = &mongoDriver{}
 
+// upsertMaxRetries bounds retries of an upsert that lost the insert race with a
+// concurrent upsert of the same document (duplicate-key error on pre-5.0 servers).
+const upsertMaxRetries = 3
+
 type mongoDriver struct {
 	*lifeCycle
 	options *types.ClientOpts
@@ -421,7 +426,18 @@ func (d *mongoDriver) Upsert(ctx context.Context, row model.DBObject, query, upd
 	coll := d.client.Database(d.database).Collection(row.TableName())
 
 	opts := options.FindOneAndUpdate().SetUpsert(true).SetReturnDocument(options.After)
-	err := coll.FindOneAndUpdate(ctx, query, update, opts).Decode(row)
+
+	// Concurrent upserts on the same not-yet-existing document can race on the
+	// insert path: two executions both miss, both try to insert, and one loses
+	// with a duplicate-key error. Servers before 5.0 do not retry this
+	// internally, so retry here; the losing call now sees the winner's document.
+	var err error
+	for attempt := 0; attempt < upsertMaxRetries; attempt++ {
+		err = coll.FindOneAndUpdate(ctx, query, update, opts).Decode(row)
+		if !mongo.IsDuplicateKeyError(err) {
+			break
+		}
+	}
 
 	return d.handleStoreError(err)
 }

persistent/internal/driver/mongo/mongo_test.go

@@ -1,5 +1,5 @@
-//go:build mongo7 || mongo6 || mongo4.4 || mongo4.2 || mongo4.0 || mongo3.6 || mongo3.4 || mongo3.2 || mongo3.0 || mongo2.6
-// +build mongo7 mongo6 mongo4.4 mongo4.2 mongo4.0 mongo3.6 mongo3.4 mongo3.2 mongo3.0 mongo2.6
+//go:build mongo7.0 || mongo7 || mongo6.0 || mongo6 || mongo4.4 || mongo4.2 || mongo4.0 || mongo3.6 || mongo3.4 || mongo3.2 || mongo3.0 || mongo2.6
+// +build mongo7.0 mongo7 mongo6.0 mongo6 mongo4.4 mongo4.2 mongo4.0 mongo3.6 mongo3.4 mongo3.2 mongo3.0 mongo2.6
 
 package mongo
 

persistent/internal/driver/mongo/query_test.go

@@ -1,5 +1,5 @@
-//go:build mongo7 || mongo6 || mongo4.4 || mongo4.2 || mongo4.0 || mongo3.6 || mongo3.4 || mongo3.2 || mongo3.0 || mongo2.6
-// +build mongo7 mongo6 mongo4.4 mongo4.2 mongo4.0 mongo3.6 mongo3.4 mongo3.2 mongo3.0 mongo2.6
+//go:build mongo7.0 || mongo7 || mongo6.0 || mongo6 || mongo4.4 || mongo4.2 || mongo4.0 || mongo3.6 || mongo3.4 || mongo3.2 || mongo3.0 || mongo2.6
+// +build mongo7.0 mongo7 mongo6.0 mongo6 mongo4.4 mongo4.2 mongo4.0 mongo3.6 mongo3.4 mongo3.2 mongo3.0 mongo2.6
 
 package mongo
 

persistent/internal/driver/mongo/registry_test.go

@@ -1,5 +1,5 @@
-//go:build mongo7 || mongo6 || mongo4.4 || mongo4.2 || mongo4.0 || mongo3.6 || mongo3.4 || mongo3.2 || mongo3.0 || mongo2.6
-// +build mongo7 mongo6 mongo4.4 mongo4.2 mongo4.0 mongo3.6 mongo3.4 mongo3.2 mongo3.0 mongo2.6
+//go:build mongo7.0 || mongo7 || mongo6.0 || mongo6 || mongo4.4 || mongo4.2 || mongo4.0 || mongo3.6 || mongo3.4 || mongo3.2 || mongo3.0 || mongo2.6
+// +build mongo7.0 mongo7 mongo6.0 mongo6 mongo4.4 mongo4.2 mongo4.0 mongo3.6 mongo3.4 mongo3.2 mongo3.0 mongo2.6
 
 package mongo