From f52d9530c742f77b0290282452f1b8c5065e9027 Mon Sep 17 00:00:00 2001 From: Daniel Fava Date: Tue, 10 Sep 2024 13:02:49 +0200 Subject: [PATCH 1/3] Improve error handling by: - not eating the underlying sql error when SingleOf is called and an error happens - implementing a QuerySqlError --- querysql/querysql.go | 13 +++++++-- querysql/querysql_test.go | 46 ++++++++++++++++++++++++++++--- querysql/scanner.go | 57 +++++++++++++++++++++++++++++++++------ 3 files changed, 103 insertions(+), 13 deletions(-) diff --git a/querysql/querysql.go b/querysql/querysql.go index 861212f..e357ee1 100644 --- a/querysql/querysql.go +++ b/querysql/querysql.go @@ -139,12 +139,21 @@ func (rs *ResultSets) processDispatcherSelect() error { // `Call[MyStruct](func(MyStruct) error { ... })` func NextResult[T any](rs *ResultSets, typ func() Result[T]) (T, error) { result := typ() + var zero T if err := Next(rs, result); err != nil { - var zero T return zero, err } - return result.Result() + // The Next() above can return nil but still set rs.Err + // If that's the case, Result() will return empty results for + // iterScanner and sliceScanner and no errors. + // When it comes to singleScanner, it returns + // the ErrZeroRowsExpectedOne wrapped around the underlying error (rs.Err) + v, errFunc := result.Result() + if errFunc != nil { + return zero, errFunc(rs.Err) + } + return v, nil } func MustNextResult[T any](rs *ResultSets, typ func() Result[T]) T { diff --git a/querysql/querysql_test.go b/querysql/querysql_test.go index ac22587..641021b 100644 --- a/querysql/querysql_test.go +++ b/querysql/querysql_test.go @@ -3,6 +3,7 @@ package querysql import ( "context" "database/sql" + "errors" "fmt" "io" "testing" @@ -447,7 +448,7 @@ func TestEmptyScalar(t *testing.T) { rs := New(context.Background(), sqldb, qry) rows := rs.Rows _, err := NextResult(rs, SingleOf[int]) - assert.Equal(t, ErrZeroRowsExpectedOne, err) + assert.Equal(t, NewZeroRowsExpectedOne(sql.ErrNoRows), err) assert.True(t, isClosed(rows)) } @@ -461,18 +462,57 @@ func TestEmptyStruct(t *testing.T) { rs := New(context.Background(), sqldb, qry) rows := rs.Rows _, err := NextResult(rs, SingleOf[row]) - assert.Equal(t, ErrZeroRowsExpectedOne, err) + assert.Equal(t, NewZeroRowsExpectedOne(sql.ErrNoRows), err) assert.True(t, isClosed(rows)) assert.True(t, rs.Done()) } +func TestEmptyResultWithError(t *testing.T) { + qry := ` +if OBJECT_ID('dbo.MyUsers', 'U') is not null drop table MyUsers +create table MyUsers ( + ID INT IDENTITY(1,1) PRIMARY KEY, + Username NVARCHAR(50) not null, + Userage int +); +insert into MyUsers (Userage) +output inserted.ID +values (42); +` + // We run the query above in two ways: + // - first with ExecContext + // - second with SingleOf + // The run with ExecContext returns an error E + // The run with SingleOf returns a QuerySqlError wrapped around E + + // ExecContext error + _, errExec := ExecContext(context.Background(), sqldb, qry, "world") + assert.Error(t, errExec) + assert.Equal(t, + "mssql: Cannot insert the value NULL into column 'Username', table 'master.dbo.MyUsers'; column does not allow nulls. INSERT fails.", + errExec.Error(), + ) + + // SingleOf error + rs := New(context.Background(), sqldb, qry) + _ = rs.Rows + _, errSingle := NextResult(rs, SingleOf[int]) + assert.Error(t, errSingle) + // The errSingle has the same underlying error as the errExec + assert.True(t, errors.Is(errSingle, errExec)) + // But the errSingle is not the same error as the errExec because, + // in addition to the underlying error, errSingle also contains + // the information that we called Single and didn't get any value back + assert.False(t, errors.Is(errExec, errSingle)) +} + func TestManyScalar(t *testing.T) { qry := `select 1 union all select 2` rs := New(context.Background(), sqldb, qry) rows := rs.Rows _, err := NextResult(rs, SingleOf[int]) - assert.Equal(t, ErrManyRowsExpectedOne, err) + assert.Equal(t, NewManyRowsExpectedOne(), err) assert.True(t, isClosed(rows)) assert.True(t, rs.Done()) } diff --git a/querysql/scanner.go b/querysql/scanner.go index d9a7044..b9abc4d 100644 --- a/querysql/scanner.go +++ b/querysql/scanner.go @@ -5,16 +5,52 @@ import ( "fmt" ) -var ErrZeroRowsExpectedOne = fmt.Errorf("query: 0 rows, expected 1: %w", sql.ErrNoRows) -var ErrManyRowsExpectedOne = fmt.Errorf("query: more than 1 row (use sliceScanner?)") +type QuerySqlError struct { + fmtString string + err error +} + +func NewManyRowsExpectedOne() QuerySqlError { + return QuerySqlError{ + fmtString: "query: more than 1 row (use sliceScanner?)", + } +} + +func NewZeroRowsExpectedOne(underlying error) QuerySqlError { + return QuerySqlError{ + fmtString: "query: 0 rows, expected 1: %w", + err: underlying, + } +} + +func (e QuerySqlError) Error() string { + return fmt.Sprintf(e.fmtString, e.err) +} + +func (e QuerySqlError) Is(other error) bool { + t, ok := other.(*QuerySqlError) + if !ok { + // Check if the underlying error matches + return e.err.Error() == other.Error() + } + return e.fmtString == t.fmtString && e.err == t.err +} + +func (e QuerySqlError) Unwrap() error { + return e.err +} + +var _ error = QuerySqlError{} // Make sure QuerySqlError implements the error interface type Target interface { ScanRow(*sql.Rows) error } +type errorWrapper func(error) error + type Result[T any] interface { Target - Result() (T, error) + Result() (T, errorWrapper) } type RowScanner[T any] struct { @@ -80,17 +116,22 @@ func SingleOf[T any]() Result[T] { return singleInto(&value) } -func (rv *singleScanner[T]) Result() (T, error) { +func (rv *singleScanner[T]) Result() (T, errorWrapper) { if !rv.hasRead { var zero T - return zero, ErrZeroRowsExpectedOne + return zero, func(e error) error { + if e == nil { + e = sql.ErrNoRows + } + return NewZeroRowsExpectedOne(e) + } } return *rv.target, nil } func (rv *singleScanner[T]) ScanRow(rows *sql.Rows) error { if rv.hasRead { - return ErrManyRowsExpectedOne + return NewManyRowsExpectedOne() } if err := rv.scanRow(rows); err != nil { return err @@ -130,7 +171,7 @@ func SliceOf[T any]() Result[[]T] { return sliceInto(&result) } -func (rv *sliceScanner[T]) Result() ([]T, error) { +func (rv *sliceScanner[T]) Result() ([]T, errorWrapper) { return *rv.slicePointer, nil } @@ -153,7 +194,7 @@ type iterScanner[T any] struct { visit func(T) error } -func (scanner *iterScanner[T]) Result() (int, error) { +func (scanner *iterScanner[T]) Result() (int, errorWrapper) { return scanner.count, nil } From e78cba7e6909cf1df2117f273964d442dea147b6 Mon Sep 17 00:00:00 2001 From: Daniel Fava Date: Fri, 20 Sep 2024 08:29:41 +0200 Subject: [PATCH 2/3] Table driven test --- querysql/querysql_test.go | 60 ++++++++++++++++++++++----------------- 1 file changed, 34 insertions(+), 26 deletions(-) diff --git a/querysql/querysql_test.go b/querysql/querysql_test.go index 641021b..f450bb3 100644 --- a/querysql/querysql_test.go +++ b/querysql/querysql_test.go @@ -468,7 +468,18 @@ func TestEmptyStruct(t *testing.T) { } func TestEmptyResultWithError(t *testing.T) { - qry := ` + // We run the queries in two ways: + // - first with ExecContext + // - second with SingleOf + // The run with ExecContext returns an error E + // The run with SingleOf returns a QuerySqlError wrapped around E + testcases := []struct { + name string + query string + expected string + }{ + { + query: ` if OBJECT_ID('dbo.MyUsers', 'U') is not null drop table MyUsers create table MyUsers ( ID INT IDENTITY(1,1) PRIMARY KEY, @@ -478,32 +489,29 @@ create table MyUsers ( insert into MyUsers (Userage) output inserted.ID values (42); -` - // We run the query above in two ways: - // - first with ExecContext - // - second with SingleOf - // The run with ExecContext returns an error E - // The run with SingleOf returns a QuerySqlError wrapped around E - - // ExecContext error - _, errExec := ExecContext(context.Background(), sqldb, qry, "world") - assert.Error(t, errExec) - assert.Equal(t, - "mssql: Cannot insert the value NULL into column 'Username', table 'master.dbo.MyUsers'; column does not allow nulls. INSERT fails.", - errExec.Error(), - ) +`, + expected: "mssql: Cannot insert the value NULL into column 'Username', table 'master.dbo.MyUsers'; column does not allow nulls. INSERT fails.", + }, + } - // SingleOf error - rs := New(context.Background(), sqldb, qry) - _ = rs.Rows - _, errSingle := NextResult(rs, SingleOf[int]) - assert.Error(t, errSingle) - // The errSingle has the same underlying error as the errExec - assert.True(t, errors.Is(errSingle, errExec)) - // But the errSingle is not the same error as the errExec because, - // in addition to the underlying error, errSingle also contains - // the information that we called Single and didn't get any value back - assert.False(t, errors.Is(errExec, errSingle)) + for _, tc := range testcases { + // ExecContext error + _, errExec := ExecContext(context.Background(), sqldb, tc.query, "world") + assert.Error(t, errExec) + assert.Equal(t, tc.expected, errExec.Error()) + + // SingleOf error + rs := New(context.Background(), sqldb, tc.query) + _ = rs.Rows + _, errSingle := NextResult(rs, SingleOf[int]) + assert.Error(t, errSingle) + // The errSingle has the same underlying error as the errExec + assert.True(t, errors.Is(errSingle, errExec)) + // But the errSingle is not the same error as the errExec because, + // in addition to the underlying error, errSingle also contains + // the information that we called Single and didn't get any value back + assert.False(t, errors.Is(errExec, errSingle)) + } } func TestManyScalar(t *testing.T) { From 2c0a736deae592dda9c76ec3758ee6caae906b94 Mon Sep 17 00:00:00 2001 From: Daniel Fava Date: Fri, 20 Sep 2024 08:47:16 +0200 Subject: [PATCH 3/3] fk test --- querysql/querysql_test.go | 73 ++++++++++++++++++++++++++------------- 1 file changed, 49 insertions(+), 24 deletions(-) diff --git a/querysql/querysql_test.go b/querysql/querysql_test.go index f450bb3..1a7622e 100644 --- a/querysql/querysql_test.go +++ b/querysql/querysql_test.go @@ -480,37 +480,62 @@ func TestEmptyResultWithError(t *testing.T) { }{ { query: ` -if OBJECT_ID('dbo.MyUsers', 'U') is not null drop table MyUsers -create table MyUsers ( +if OBJECT_ID('dbo.MyUser', 'U') is not null drop table MyUser +create table MyUser ( ID INT IDENTITY(1,1) PRIMARY KEY, Username NVARCHAR(50) not null, Userage int ); -insert into MyUsers (Userage) +insert into MyUser (Userage) output inserted.ID values (42); `, - expected: "mssql: Cannot insert the value NULL into column 'Username', table 'master.dbo.MyUsers'; column does not allow nulls. INSERT fails.", + expected: "mssql: Cannot insert the value NULL into column 'Username', table 'master.dbo.MyUser'; column does not allow nulls. INSERT fails.", + }, + { + query: ` + if OBJECT_ID('dbo.Employee', 'U') is not null drop table Employee + if OBJECT_ID('dbo.Department', 'U') is not null drop table Department + create table Department ( + ID INT IDENTITY(1,1) PRIMARY KEY, + Name NVARCHAR(50) not null, + ); + create table Employee ( + ID INT IDENTITY(1,1) PRIMARY KEY, + Name NVARCHAR(50) not null, + Department int + constraint fk_Department foreign key references Department(ID), + ); + + insert into Employee(Name, Department) values ('Bob', 1); + select @@rowcount; + `, + expected: "mssql: The INSERT statement conflicted with the FOREIGN KEY constraint \"fk_Department\". The conflict occurred in database \"master\", table \"dbo.Department\", column 'ID'.", + // TODO(dsf) }, } - for _, tc := range testcases { - // ExecContext error - _, errExec := ExecContext(context.Background(), sqldb, tc.query, "world") - assert.Error(t, errExec) - assert.Equal(t, tc.expected, errExec.Error()) - - // SingleOf error - rs := New(context.Background(), sqldb, tc.query) - _ = rs.Rows - _, errSingle := NextResult(rs, SingleOf[int]) - assert.Error(t, errSingle) - // The errSingle has the same underlying error as the errExec - assert.True(t, errors.Is(errSingle, errExec)) - // But the errSingle is not the same error as the errExec because, - // in addition to the underlying error, errSingle also contains - // the information that we called Single and didn't get any value back - assert.False(t, errors.Is(errExec, errSingle)) + for i, tc := range testcases { + t.Run(fmt.Sprintf("%d:%s", i, tc.name), func(t *testing.T) { + // ExecContext error + _, errExec := ExecContext(context.Background(), sqldb, tc.query, "world") + require.Error(t, errExec) + require.Equal(t, tc.expected, errExec.Error()) + + // SingleOf error + rs := New(context.Background(), sqldb, tc.query) + _ = rs.Rows + _, errSingle := NextResult(rs.EnsureDoneAfterNext(), SingleOf[int]) + require.Error(t, errSingle) + + fmt.Printf("single(%s)\n exec(%s)", errSingle.Error(), errExec.Error()) + // The errSingle has the same underlying error as the errExec + require.True(t, errors.Is(errSingle, errExec), fmt.Sprintf("single(%s) exec(%s)", errSingle.Error(), errExec.Error())) + // But the errSingle is not the same error as the errExec because, + // in addition to the underlying error, errSingle also contains + // the information that we called Single and didn't get any value back + require.False(t, errors.Is(errExec, errSingle)) + }) } } @@ -653,12 +678,12 @@ func TestStructScanError(t *testing.T) { func TestExecContext(t *testing.T) { qry := ` -if OBJECT_ID('dbo.MyUsers', 'U') is not null drop table MyUsers -create table MyUsers ( +if OBJECT_ID('dbo.MyUser', 'U') is not null drop table MyUser +create table MyUser ( ID INT IDENTITY(1,1) PRIMARY KEY, Username NVARCHAR(50) ); -insert into MyUsers (Username) values ('JohnDoe'); +insert into MyUser (Username) values ('JohnDoe'); -- logging select _log='info', Y = 'one';