CheckTxOptionsCompatibility using Connection.TransactionOptions

ungerik · ungerik · commit 36657e4b6fe2 · 2021-03-14T15:32:27.000+01:00
diff --git a/config.go b/config.go
@@ -11,16 +11,17 @@ import (
 // Config for a connection.
 // For tips see https://www.alexedwards.net/blog/configuring-sqldb
 type Config struct {
-	Driver          string            `json:"driver"`
-	Host            string            `json:"host"`
-	Port            uint16            `json:"port,omitempty"`
-	User            string            `json:"user,omitempty"`
-	Password        string            `json:"password,omitempty"`
-	Database        string            `json:"database"`
-	Extra           map[string]string `json:"misc,omitempty"`
-	MaxOpenConns    int               `json:"maxOpenConns,omitempty"`
-	MaxIdleConns    int               `json:"maxIdleConns,omitempty"`
-	ConnMaxLifetime time.Duration     `json:"connMaxLifetime,omitempty"`
+	Driver                string             `json:"driver"`
+	Host                  string             `json:"host"`
+	Port                  uint16             `json:"port,omitempty"`
+	User                  string             `json:"user,omitempty"`
+	Password              string             `json:"password,omitempty"`
+	Database              string             `json:"database"`
+	Extra                 map[string]string  `json:"misc,omitempty"`
+	MaxOpenConns          int                `json:"maxOpenConns,omitempty"`
+	MaxIdleConns          int                `json:"maxIdleConns,omitempty"`
+	ConnMaxLifetime       time.Duration      `json:"connMaxLifetime,omitempty"`
+	DefaultIsolationLevel sql.IsolationLevel `json:"-"`
 }
 
 // ConnectURL for connecting to a database
diff --git a/connection.go b/connection.go
@@ -128,6 +128,11 @@ type Connection interface {
 	// IsTransaction returns if the connection is a transaction
 	IsTransaction() bool
 
+	// TransactionOptions returns the sql.TxOptions of the
+	// current transaction and true as second result value,
+	// or false if the connection is not a transaction.
+	TransactionOptions() (*sql.TxOptions, bool)
+
 	// Begin a new transaction.
 	// Returns ErrWithinTransaction if the connection
 	// is already within a transaction.
diff --git a/errors.go b/errors.go
@@ -73,7 +73,7 @@ func (e connectionWithError) Stats() sql.DBStats {
 }
 
 func (e connectionWithError) Config() *Config {
-	return nil
+	return new(Config)
 }
 
 func (e connectionWithError) Exec(query string, args ...interface{}) error {
@@ -148,6 +148,10 @@ func (e connectionWithError) IsTransaction() bool {
 	return false
 }
 
+func (ce connectionWithError) TransactionOptions() (*sql.TxOptions, bool) {
+	return nil, false
+}
+
 func (e connectionWithError) Begin(opts *sql.TxOptions) (Connection, error) {
 	return nil, e.err
 }
diff --git a/mockconn/connection.go b/mockconn/connection.go
@@ -153,16 +153,19 @@ func (conn *connection) QueryRows(query string, args ...interface{}) sqldb.RowsS
 	return conn.rowsProvider.QueryRows(conn.structFieldNamer, query, args...)
 }
 
-// IsTransaction returns if the connection is a transaction
 func (conn *connection) IsTransaction() bool {
 	return false
 }
 
+func (conn *connection) TransactionOptions() (*sql.TxOptions, bool) {
+	return nil, false
+}
+
 func (conn *connection) Begin(opts *sql.TxOptions) (sqldb.Connection, error) {
 	if conn.queryWriter != nil {
 		fmt.Fprint(conn.queryWriter, "BEGIN")
 	}
-	return transaction{conn}, nil
+	return transaction{conn, opts}, nil
 }
 
 func (conn *connection) Commit() error {
diff --git a/mockconn/connection_test.go b/mockconn/connection_test.go
@@ -24,6 +24,7 @@ type testRow struct {
 	Str           string  `db:"str"`
 	StrPtr        *string `db:"str_ptr"`
 	NilPtr        *byte   `db:"nil_ptr"`
+	ReadOnly      int     `db:"read_only,readonly"`
 	Ignore        int     `db:"-"`
 	UntaggedField int
 	CreatedAt     time.Time    `db:"created_at"`
diff --git a/mockconn/transaction.go b/mockconn/transaction.go
@@ -10,19 +10,24 @@ import (
 
 type transaction struct {
 	*connection
+	opts *sql.TxOptions
 }
 
 func (conn transaction) WithContext(ctx context.Context) sqldb.Connection {
 	return transaction{
 		connection: conn.connection.WithContext(ctx).(*connection), // TODO better way than type cast?
+		opts:       conn.opts,
 	}
 }
 
-// IsTransaction returns if the connection is a transaction
 func (conn transaction) IsTransaction() bool {
 	return true
 }
 
+func (conn transaction) TransactionOptions() (*sql.TxOptions, bool) {
+	return conn.opts, true
+}
+
 func (conn transaction) Begin(opts *sql.TxOptions) (sqldb.Connection, error) {
 	return nil, sqldb.ErrWithinTransaction
 }
diff --git a/pqconn/connection.go b/pqconn/connection.go
@@ -17,6 +17,8 @@ func New(ctx context.Context, config *sqldb.Config) (sqldb.Connection, error) {
 	if config.Driver != "postgres" {
 		return nil, fmt.Errorf(`invalid driver %q, pqconn expects "postgres"`, config.Driver)
 	}
+	config.DefaultIsolationLevel = sql.LevelReadCommitted // postgres default
+
 	db, err := config.Connect(ctx)
 	if err != nil {
 		return nil, err
@@ -166,6 +168,10 @@ func (conn *connection) IsTransaction() bool {
 	return false
 }
 
+func (conn *connection) TransactionOptions() (*sql.TxOptions, bool) {
+	return nil, false
+}
+
 func (conn *connection) Begin(opts *sql.TxOptions) (sqldb.Connection, error) {
 	tx, err := conn.db.BeginTx(conn.ctx, opts)
 	if err != nil {
@@ -174,6 +180,7 @@ func (conn *connection) Begin(opts *sql.TxOptions) (sqldb.Connection, error) {
 	return &transaction{
 		connection:       conn,
 		tx:               tx,
+		opts:             opts,
 		structFieldNamer: conn.structFieldNamer,
 	}, nil
 }
diff --git a/pqconn/transaction.go b/pqconn/transaction.go
@@ -11,13 +11,15 @@ import (
 type transaction struct {
 	*connection
 	tx               *sql.Tx
+	opts             *sql.TxOptions
 	structFieldNamer sqldb.StructFieldNamer
 }
 
 func (conn *transaction) WithContext(ctx context.Context) sqldb.Connection {
 	return &transaction{
 		connection:       conn.connection.WithContext(ctx).(*connection), // TODO better way than type cast?
 		tx:               conn.tx,
+		opts:             conn.opts,
 		structFieldNamer: conn.structFieldNamer,
 	}
 }
@@ -26,6 +28,7 @@ func (conn *transaction) WithStructFieldNamer(namer sqldb.StructFieldNamer) sqld
 	return &transaction{
 		connection:       conn.connection,
 		tx:               conn.tx,
+		opts:             conn.opts,
 		structFieldNamer: namer,
 	}
 }
@@ -89,11 +92,14 @@ func (conn *transaction) QueryRows(query string, args ...interface{}) sqldb.Rows
 	return impl.NewRowsScanner(conn.connection.ctx, rows, conn.structFieldNamer, query, args)
 }
 
-// IsTransaction returns if the connection is a transaction
 func (conn *transaction) IsTransaction() bool {
 	return true
 }
 
+func (conn *transaction) TransactionOptions() (*sql.TxOptions, bool) {
+	return conn.opts, true
+}
+
 func (conn *transaction) Begin(opts *sql.TxOptions) (sqldb.Connection, error) {
 	return nil, sqldb.ErrWithinTransaction
 }
diff --git a/structfieldnamer_test.go b/structfieldnamer_test.go
@@ -58,7 +58,7 @@ func TestStructFieldTagNaming_StructFieldName(t *testing.T) {
 		{name: "read_only", structField: st.Field(2), wantName: "read_only", wantFlags: FieldFlagReadOnly, wantOk: true},
 		{name: "untagged_field", structField: st.Field(3), wantName: "untagged_field", wantFlags: 0, wantOk: true},
 		{name: "ignore", structField: st.Field(4), wantName: "", wantFlags: 0, wantOk: false},
-		{name: "pk_read_only", structField: st.Field(5), wantName: "pk_read_only", wantFlags: FieldFlagPrimaryKey + FieldFlagReadOnly, wantOk: true},
+		{name: "pk_read_only", structField: st.Field(5), wantName: "pk_read_only", wantFlags: FieldFlagPrimaryKey | FieldFlagReadOnly, wantOk: true},
 		{name: "no_flag", structField: st.Field(6), wantName: "no_flag", wantFlags: 0, wantOk: true},
 		{name: "malformed_flags", structField: st.Field(7), wantName: "malformed_flags", wantFlags: FieldFlagReadOnly, wantOk: true},
 		{name: "Embedded", structField: st.Field(8), wantName: "", wantFlags: 0, wantOk: true},
diff --git a/transaction.go b/transaction.go
@@ -10,10 +10,16 @@ import (
 // Transaction returns all errors from txFunc or transaction commit errors happening after txFunc.
 // If parentConn is already a transaction, then it is passed through to txFunc unchanged as tx Connection
 // and no parentConn.Begin, Commit, or Rollback calls will occour within this Transaction call.
+// An error is returned, if the requested transaction options passed via opts
+// are stricter than the options of the parent transaction.
 // Errors and panics from txFunc will rollback the transaction if parentConn was not already a transaction.
 // Recovered panics are re-paniced and rollback errors after a panic are logged with ErrLogger.
 func Transaction(parentConn Connection, opts *sql.TxOptions, txFunc func(tx Connection) error) (err error) {
-	if parentConn.IsTransaction() {
+	if parentOpts, parentIsTx := parentConn.TransactionOptions(); parentIsTx {
+		err = CheckTxOptionsCompatibility(parentOpts, opts, parentConn.Config().DefaultIsolationLevel)
+		if err != nil {
+			return err
+		}
 		return txFunc(parentConn)
 	}
 
@@ -52,3 +58,34 @@ func Transaction(parentConn Connection, opts *sql.TxOptions, txFunc func(tx Conn
 
 	return txFunc(tx)
 }
+
+// CheckTxOptionsCompatibility returns an error
+// if the parent transaction options are less strict than the child options.
+func CheckTxOptionsCompatibility(parent, child *sql.TxOptions, defaultIsolation sql.IsolationLevel) error {
+	var (
+		parentReadOnly  = false
+		parentIsolation = defaultIsolation
+		childReadOnly   = false
+		childIsolation  = defaultIsolation
+	)
+	if parent != nil {
+		parentReadOnly = parent.ReadOnly
+		if parent.Isolation != sql.LevelDefault {
+			parentIsolation = parent.Isolation
+		}
+	}
+	if child != nil {
+		childReadOnly = child.ReadOnly
+		if child.Isolation != sql.LevelDefault {
+			childIsolation = child.Isolation
+		}
+	}
+
+	if parentReadOnly && !childReadOnly {
+		return errors.New("parent transaction is read-only but child is not")
+	}
+	if parentIsolation < childIsolation {
+		return fmt.Errorf("parent transaction isolation level '%s' is less strict child level '%s'", parentIsolation, childIsolation)
+	}
+	return nil
+}
diff --git a/transaction_test.go b/transaction_test.go
@@ -0,0 +1,122 @@
+package sqldb
+
+import (
+	"database/sql"
+	"testing"
+)
+
+func TestCheckTxOptionsCompatibility(t *testing.T) {
+	type args struct {
+		parent           *sql.TxOptions
+		child            *sql.TxOptions
+		defaultIsolation sql.IsolationLevel
+	}
+	tests := []struct {
+		name    string
+		args    args
+		wantErr bool
+	}{
+		{
+			name: "nil, nil",
+			args: args{
+				parent: nil,
+				child:  nil,
+			},
+		},
+		{
+			name: "nil, default",
+			args: args{
+				parent: nil,
+				child:  &sql.TxOptions{},
+			},
+		},
+		{
+			name: "default, nil",
+			args: args{
+				parent: &sql.TxOptions{},
+				child:  nil,
+			},
+		},
+		{
+			name: "default, default",
+			args: args{
+				parent: &sql.TxOptions{},
+				child:  &sql.TxOptions{},
+			},
+		},
+		{
+			name: "nil, ReadOnly",
+			args: args{
+				parent: nil,
+				child:  &sql.TxOptions{ReadOnly: true},
+			},
+		},
+		{
+			name: "ReadOnly, ReadOnly",
+			args: args{
+				parent: nil,
+				child:  &sql.TxOptions{ReadOnly: true},
+			},
+			wantErr: false,
+		},
+		{
+			name: "ReadOnly, nil",
+			args: args{
+				parent: &sql.TxOptions{ReadOnly: true},
+				child:  nil,
+			},
+			wantErr: true,
+		},
+		{
+			name: "ReadCommitted, ReadCommitted",
+			args: args{
+				parent: &sql.TxOptions{Isolation: sql.LevelReadCommitted},
+				child:  &sql.TxOptions{Isolation: sql.LevelReadCommitted},
+			},
+		},
+		{
+			name: "Serializable, ReadCommitted",
+			args: args{
+				parent: &sql.TxOptions{Isolation: sql.LevelSerializable},
+				child:  &sql.TxOptions{Isolation: sql.LevelReadCommitted},
+			},
+		},
+		{
+			name: "ReadCommitted, Serializable",
+			args: args{
+				parent: &sql.TxOptions{Isolation: sql.LevelReadCommitted},
+				child:  &sql.TxOptions{Isolation: sql.LevelSerializable},
+			},
+			wantErr: true,
+		},
+		{
+			name: "ReadCommitted, Serializable/ReadOnly",
+			args: args{
+				parent: &sql.TxOptions{Isolation: sql.LevelReadCommitted},
+				child:  &sql.TxOptions{Isolation: sql.LevelSerializable, ReadOnly: true},
+			},
+			wantErr: true,
+		},
+		{
+			name: "ReadCommitted/ReadOnly, ReadCommitted/ReadOnly",
+			args: args{
+				parent: &sql.TxOptions{Isolation: sql.LevelReadCommitted, ReadOnly: true},
+				child:  &sql.TxOptions{Isolation: sql.LevelReadCommitted, ReadOnly: true},
+			},
+		},
+		{
+			name: "Serializable/ReadOnly, ReadCommitted/ReadOnly",
+			args: args{
+				parent: &sql.TxOptions{Isolation: sql.LevelSerializable, ReadOnly: true},
+				child:  &sql.TxOptions{Isolation: sql.LevelReadCommitted, ReadOnly: true},
+			},
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if err := CheckTxOptionsCompatibility(tt.args.parent, tt.args.child, tt.args.defaultIsolation); (err != nil) != tt.wantErr {
+				t.Errorf("CheckTxOptionsCompatibility() error = %v, wantErr %v", err, tt.wantErr)
+			}
+		})
+	}
+}

Original file line number	Diff line number	Diff line change
`@@ -73,7 +73,7 @@ func (e connectionWithError) Stats() sql.DBStats {`
`73`	`73`	`}`
`74`	`74`
`75`	`75`	`func (e connectionWithError) Config() *Config {`
`76`		`- return nil`
	`76`	`+ return new(Config)`
`77`	`77`	`}`
`78`	`78`
`79`	`79`	`func (e connectionWithError) Exec(query string, args ...interface{}) error {`
`@@ -148,6 +148,10 @@ func (e connectionWithError) IsTransaction() bool {`
`148`	`148`	`return false`
`149`	`149`	`}`
`150`	`150`
	`151`	`+func (ce connectionWithError) TransactionOptions() (*sql.TxOptions, bool) {`
	`152`	`+ return nil, false`
	`153`	`+}`
	`154`	`+`
`151`	`155`	`func (e connectionWithError) Begin(opts *sql.TxOptions) (Connection, error) {`
`152`	`156`	`return nil, e.err`
`153`	`157`	`}`
Original file line number	Diff line number	Diff line change
`@@ -153,16 +153,19 @@ func (conn *connection) QueryRows(query string, args ...interface{}) sqldb.RowsS`
`153`	`153`	`return conn.rowsProvider.QueryRows(conn.structFieldNamer, query, args...)`
`154`	`154`	`}`
`155`	`155`
`156`		`-// IsTransaction returns if the connection is a transaction`
`157`	`156`	`func (conn *connection) IsTransaction() bool {`
`158`	`157`	`return false`
`159`	`158`	`}`
`160`	`159`
	`160`	`+func (conn connection) TransactionOptions() (sql.TxOptions, bool) {`
	`161`	`+ return nil, false`
	`162`	`+}`
	`163`	`+`
`161`	`164`	`func (conn connection) Begin(opts sql.TxOptions) (sqldb.Connection, error) {`
`162`	`165`	`if conn.queryWriter != nil {`
`163`	`166`	`fmt.Fprint(conn.queryWriter, "BEGIN")`
`164`	`167`	`}`
`165`		`- return transaction{conn}, nil`
	`168`	`+ return transaction{conn, opts}, nil`
`166`	`169`	`}`
`167`	`170`
`168`	`171`	`func (conn *connection) Commit() error {`
Original file line number	Diff line number	Diff line change
`@@ -10,19 +10,24 @@ import (`
`10`	`10`
`11`	`11`	`type transaction struct {`
`12`	`12`	`*connection`
	`13`	`+ opts *sql.TxOptions`
`13`	`14`	`}`
`14`	`15`
`15`	`16`	`func (conn transaction) WithContext(ctx context.Context) sqldb.Connection {`
`16`	`17`	`return transaction{`
`17`	`18`	`connection: conn.connection.WithContext(ctx).(*connection), // TODO better way than type cast?`
	`19`	`+ opts: conn.opts,`
`18`	`20`	`}`
`19`	`21`	`}`
`20`	`22`
`21`		`-// IsTransaction returns if the connection is a transaction`
`22`	`23`	`func (conn transaction) IsTransaction() bool {`
`23`	`24`	`return true`
`24`	`25`	`}`
`25`	`26`
	`27`	`+func (conn transaction) TransactionOptions() (*sql.TxOptions, bool) {`
	`28`	`+ return conn.opts, true`
	`29`	`+}`
	`30`	`+`
`26`	`31`	`func (conn transaction) Begin(opts *sql.TxOptions) (sqldb.Connection, error) {`
`27`	`32`	`return nil, sqldb.ErrWithinTransaction`
`28`	`33`	`}`
Original file line number	Diff line number	Diff line change
`@@ -11,13 +11,15 @@ import (`
`11`	`11`	`type transaction struct {`
`12`	`12`	`*connection`
`13`	`13`	`tx *sql.Tx`
	`14`	`+ opts *sql.TxOptions`
`14`	`15`	`structFieldNamer sqldb.StructFieldNamer`
`15`	`16`	`}`
`16`	`17`
`17`	`18`	`func (conn *transaction) WithContext(ctx context.Context) sqldb.Connection {`
`18`	`19`	`return &transaction{`
`19`	`20`	`connection: conn.connection.WithContext(ctx).(*connection), // TODO better way than type cast?`
`20`	`21`	`tx: conn.tx,`
	`22`	`+ opts: conn.opts,`
`21`	`23`	`structFieldNamer: conn.structFieldNamer,`
`22`	`24`	`}`
`23`	`25`	`}`
`@@ -26,6 +28,7 @@ func (conn *transaction) WithStructFieldNamer(namer sqldb.StructFieldNamer) sqld`
`26`	`28`	`return &transaction{`
`27`	`29`	`connection: conn.connection,`
`28`	`30`	`tx: conn.tx,`
	`31`	`+ opts: conn.opts,`
`29`	`32`	`structFieldNamer: namer,`
`30`	`33`	`}`
`31`	`34`	`}`
`@@ -89,11 +92,14 @@ func (conn *transaction) QueryRows(query string, args ...interface{}) sqldb.Rows`
`89`	`92`	`return impl.NewRowsScanner(conn.connection.ctx, rows, conn.structFieldNamer, query, args)`
`90`	`93`	`}`
`91`	`94`
`92`		`-// IsTransaction returns if the connection is a transaction`
`93`	`95`	`func (conn *transaction) IsTransaction() bool {`
`94`	`96`	`return true`
`95`	`97`	`}`
`96`	`98`
	`99`	`+func (conn transaction) TransactionOptions() (sql.TxOptions, bool) {`
	`100`	`+ return conn.opts, true`
	`101`	`+}`
	`102`	`+`
`97`	`103`	`func (conn transaction) Begin(opts sql.TxOptions) (sqldb.Connection, error) {`
`98`	`104`	`return nil, sqldb.ErrWithinTransaction`
`99`	`105`	`}`