Merge branch 'smh-fix-context-handled-test' into 'master'

Fix flaky test TestTxQuery_ContextHandler

Closes #3415

See merge request gitlab-org/gitaly!3140

2 files changed, 0 insertions, 327 deletions

diff --git a/internal/praefect/datastore/glsql/postgres.go b/internal/praefect/datastore/glsql/postgres.go
index 8259c5f50..58627ab48 100644
--- a/internal/praefect/datastore/glsql/postgres.go
+++ b/internal/praefect/datastore/glsql/postgres.go
@@ -8,7 +8,6 @@ import (
 	// Blank import to enable integration of github.com/lib/pq into database/sql
 	_ "github.com/lib/pq"
 	migrate "github.com/rubenv/sql-migrate"
-	"github.com/sirupsen/logrus"
 	"gitlab.com/gitlab-org/gitaly/internal/praefect/config"
 	"gitlab.com/gitlab-org/gitaly/internal/praefect/datastore/migrations"
 )
@@ -60,74 +59,6 @@ type ListenHandler interface {
 	Connected()
 }
 
-// TxQuery runs operations inside transaction and commits|rollbacks on Done.
-type TxQuery interface {
-	// Exec calls op function with provided ctx.
-	// Returns true on success and false in case operation failed or wasn't called because of previously failed op.
-	Exec(ctx context.Context, op func(context.Context, *sql.Tx) error) bool
-	// Done must be called after work is finished to complete transaction.
-	// errPtr must not be nil.
-	// COMMIT will be executed if no errors happen during TxQuery usage.
-	// Otherwise it will be ROLLBACK operation.
-	Done(errPtr *error)
-}
-
-// NewTxQuery creates entity that allows to run queries in scope of a transaction.
-// It always returns non-nil value.
-func NewTxQuery(ctx context.Context, logger logrus.FieldLogger, db *sql.DB) TxQuery {
-	tx, err := db.BeginTx(ctx, nil)
-	return &txQuery{
-		tx:     tx,
-		err:    err,
-		logger: logger,
-	}
-}
-
-type txQuery struct {
-	tx     *sql.Tx
-	err    error
-	logger logrus.FieldLogger
-}
-
-// Exec calls op function with provided ctx.
-// Returns true on success and false in case operation failed or wasn't called because of previously failed op.
-func (txq *txQuery) Exec(ctx context.Context, op func(context.Context, *sql.Tx) error) bool {
-	if txq.err != nil {
-		return false
-	}
-
-	txq.err = op(ctx, txq.tx)
-	return txq.err == nil
-}
-
-// Done must be called after work is finished to complete transaction.
-// errPtr must not be nil.
-// COMMIT will be executed if no errors happen during txQuery usage.
-// Otherwise it will be ROLLBACK operation.
-func (txq *txQuery) Done(errPtr *error) {
-	if txq.err == nil {
-		txq.err = txq.tx.Commit()
-		if txq.err != nil {
-			txq.log(txq.err, "commit failed")
-		}
-	} else {
-		// Don't overwrite txq.err because it's already non-nil
-		if err := txq.tx.Rollback(); err != nil {
-			txq.log(err, "rollback failed")
-		}
-	}
-
-	if *errPtr == nil {
-		*errPtr = txq.err
-	}
-}
-
-func (txq *txQuery) log(err error, msg string) {
-	if txq.logger != nil {
-		txq.logger.WithError(err).Error(msg)
-	}
-}
-
 // DestProvider returns list of pointers that will be used to scan values into.
 type DestProvider interface {
 	// To returns list of pointers.
diff --git a/internal/praefect/datastore/glsql/postgres_test.go b/internal/praefect/datastore/glsql/postgres_test.go
index 33db541df..ba0891380 100644
--- a/internal/praefect/datastore/glsql/postgres_test.go
+++ b/internal/praefect/datastore/glsql/postgres_test.go
@@ -3,19 +3,12 @@
 package glsql
 
 import (
-	"bytes"
-	"context"
-	"database/sql"
-	"errors"
 	"os"
 	"strconv"
-	"strings"
 	"testing"
 
-	"github.com/sirupsen/logrus"
 	"github.com/stretchr/testify/require"
 	"gitlab.com/gitlab-org/gitaly/internal/praefect/config"
-	"gitlab.com/gitlab-org/gitaly/internal/testhelper"
 )
 
 func TestOpenDB(t *testing.T) {
@@ -47,257 +40,6 @@ func TestOpenDB(t *testing.T) {
 	})
 }
 
-func TestTxQuery_MultipleOperationsSuccess(t *testing.T) {
-	db := getDB(t)
-	defer createBasicTable(t, db, "work_unit_test")()
-
-	ctx, cancel := testhelper.Context()
-	defer cancel()
-
-	const actions = 3
-	txq := NewTxQuery(context.TODO(), nil, db.DB)
-
-	defer func() {
-		var err error
-		txq.Done(&err)
-		require.NoError(t, err)
-
-		db.RequireRowsInTable(t, "work_unit_test", actions)
-	}()
-
-	for i := 0; i < actions; i++ {
-		require.True(
-			t,
-			txq.Exec(ctx, func(ctx context.Context, tx *sql.Tx) error {
-				_, err := tx.ExecContext(ctx, "INSERT INTO work_unit_test VALUES (DEFAULT)")
-				return err
-			}),
-			"expects row to be inserted",
-		)
-	}
-}
-
-func TestTxQuery_FailedOperationInTheMiddle(t *testing.T) {
-	db := getDB(t)
-	defer createBasicTable(t, db, "work_unit_test")()
-
-	ctx, cancel := testhelper.Context()
-	defer cancel()
-
-	txq := NewTxQuery(ctx, nil, db.DB)
-
-	defer func() {
-		var err error
-		txq.Done(&err)
-		require.EqualError(t, err, `pq: syntax error at or near "BAD"`, "expects error because of the incorrect SQL statement")
-
-		db.RequireRowsInTable(t, "work_unit_test", 0)
-	}()
-
-	require.True(t,
-		txq.Exec(ctx, func(ctx context.Context, tx *sql.Tx) error {
-			_, err := tx.ExecContext(ctx, "INSERT INTO work_unit_test(id) VALUES (DEFAULT)")
-			return err
-		}),
-		"expects row to be inserted",
-	)
-
-	require.False(t,
-		txq.Exec(ctx, func(ctx context.Context, tx *sql.Tx) error {
-			_, err := tx.ExecContext(ctx, "BAD OPERATION")
-			return err
-		}),
-		"the SQL statement is not valid, expects to be reported as failed",
-	)
-
-	require.False(t,
-		txq.Exec(ctx, func(ctx context.Context, tx *sql.Tx) error {
-			t.Fatal("this func should not be called")
-			return nil
-		}),
-		"because of previously failed SQL operation next statement expected not to be run",
-	)
-}
-
-func TestTxQuery_ContextHandled(t *testing.T) {
-	db := getDB(t)
-
-	defer createBasicTable(t, db, "work_unit_test")()
-
-	ctx, cancel := testhelper.Context()
-	defer cancel()
-
-	txq := NewTxQuery(ctx, nil, db.DB)
-
-	defer func() {
-		var err error
-		txq.Done(&err)
-		require.EqualError(t, err, "context canceled")
-
-		db.RequireRowsInTable(t, "work_unit_test", 0)
-	}()
-
-	require.True(t,
-		txq.Exec(ctx, func(ctx context.Context, tx *sql.Tx) error {
-			_, err := tx.ExecContext(ctx, "INSERT INTO work_unit_test(id) VALUES (DEFAULT)")
-			return err
-		}),
-		"expects row to be inserted",
-	)
-
-	cancel() // explicit context cancellation to simulate situation when it is expired or cancelled
-
-	require.False(t,
-		txq.Exec(ctx, func(ctx context.Context, tx *sql.Tx) error {
-			_, err := tx.ExecContext(ctx, "INSERT INTO work_unit_test(id) VALUES (DEFAULT)")
-			return err
-		}),
-		"expects failed operation because of cancelled context",
-	)
-}
-
-func TestTxQuery_FailedToCommit(t *testing.T) {
-	db := getDB(t)
-	defer createBasicTable(t, db, "work_unit_test")()
-
-	ctx, cancel := testhelper.Context()
-	defer cancel()
-
-	txq := NewTxQuery(ctx, nil, db.DB)
-
-	defer func() {
-		var err error
-		txq.Done(&err)
-		require.EqualError(t, err, sql.ErrTxDone.Error(), "expects failed COMMIT because of previously executed COMMIT statement")
-
-		db.RequireRowsInTable(t, "work_unit_test", 1)
-	}()
-
-	require.True(t,
-		txq.Exec(ctx, func(ctx context.Context, tx *sql.Tx) error {
-			_, err := tx.ExecContext(ctx, "INSERT INTO work_unit_test(id) VALUES (DEFAULT)")
-			return err
-		}),
-		"expects row to be inserted",
-	)
-
-	require.True(t,
-		txq.Exec(ctx, func(ctx context.Context, tx *sql.Tx) error {
-			require.NoError(t, tx.Commit()) // COMMIT to get error on the next attempt to COMMIT from Done method
-			return nil
-		}),
-		"expects COMMIT without issues",
-	)
-}
-
-func TestTxQuery_FailedToRollbackWithFailedOperation(t *testing.T) {
-	db := getDB(t)
-	defer createBasicTable(t, db, "work_unit_test")()
-
-	ctx, cancel := testhelper.Context()
-	defer cancel()
-
-	outBuffer := &bytes.Buffer{}
-	logger := logrus.New()
-	logger.Out = outBuffer
-	logger.Level = logrus.ErrorLevel
-	logger.Formatter = &logrus.JSONFormatter{
-		DisableTimestamp: true,
-		PrettyPrint:      false,
-	}
-
-	txq := NewTxQuery(ctx, logger, db.DB)
-
-	defer func() {
-		var err error
-		txq.Done(&err)
-		require.EqualError(t, err, "some unexpected error")
-		require.Equal(t,
-			`{"error":"sql: transaction has already been committed or rolled back","level":"error","msg":"rollback failed"}`,
-			strings.TrimSpace(outBuffer.String()),
-			"failed COMMIT/ROLLBACK operation must be logged in case of another error during transaction usage",
-		)
-
-		db.RequireRowsInTable(t, "work_unit_test", 1)
-	}()
-
-	require.True(t,
-		txq.Exec(ctx, func(ctx context.Context, tx *sql.Tx) error {
-			_, err := tx.ExecContext(ctx, "INSERT INTO work_unit_test(id) VALUES (DEFAULT)")
-			return err
-		}),
-		"expects row to be inserted",
-	)
-
-	require.False(t,
-		txq.Exec(ctx, func(ctx context.Context, tx *sql.Tx) error {
-			require.NoError(t, tx.Commit(), "expects successful COMMIT") // COMMIT to get error on the next attempt to COMMIT
-			return errors.New("some unexpected error")
-		}),
-		"expects failed operation because of explicit error returned",
-	)
-}
-
-func TestTxQuery_FailedToCommitWithFailedOperation(t *testing.T) {
-	db := getDB(t)
-	defer createBasicTable(t, db, "work_unit_test")()
-
-	ctx, cancel := testhelper.Context()
-	defer cancel()
-
-	outBuffer := &bytes.Buffer{}
-	logger := logrus.New()
-	logger.Out = outBuffer
-	logger.Level = logrus.ErrorLevel
-	logger.Formatter = &logrus.JSONFormatter{
-		DisableTimestamp: true,
-		PrettyPrint:      false,
-	}
-
-	txq := NewTxQuery(ctx, logger, db.DB)
-
-	defer func() {
-		err := errors.New("some processing error")
-		txq.Done(&err)
-		require.EqualError(t, err, "some processing error")
-		require.Equal(
-			t,
-			`{"error":"sql: transaction has already been committed or rolled back","level":"error","msg":"commit failed"}`,
-			strings.TrimSpace(outBuffer.String()),
-			"failed COMMIT/ROLLBACK operation must be logged in case of another error during transaction usage",
-		)
-
-		db.RequireRowsInTable(t, "work_unit_test", 1)
-	}()
-
-	require.True(t,
-		txq.Exec(ctx, func(ctx context.Context, tx *sql.Tx) error {
-			_, err := tx.ExecContext(ctx, "INSERT INTO work_unit_test(id) VALUES (DEFAULT)")
-			return err
-		}),
-		"expects row to be inserted",
-	)
-
-	require.True(t,
-		txq.Exec(ctx, func(ctx context.Context, tx *sql.Tx) error {
-			require.NoError(t, tx.Commit()) // COMMIT to get error on the next attempt to COMMIT
-			return nil
-		}),
-		"expects COMMIT without issues",
-	)
-}
-
-func createBasicTable(t *testing.T, db DB, tname string) func() {
-	t.Helper()
-
-	_, err := db.Exec("CREATE TABLE " + tname + "(id BIGSERIAL PRIMARY KEY, col TEXT)")
-	require.NoError(t, err)
-	return func() {
-		_, err := db.Exec("DROP TABLE IF EXISTS " + tname)
-		require.NoError(t, err)
-	}
-}
-
 func TestUint64Provider(t *testing.T) {
 	var provider Uint64Provider