Merge pull request rapidpro#457 from nyaruka/query_errors

😵 Add new error type for failed SQL queries
Ilhasoft · Jul 9, 2021 · 819300f · 819300f
2 parents 9b268b9 + 96970fe
commit 819300f
Show file tree

Hide file tree

Showing 5 changed files with 77 additions and 3 deletions.
diff --git a/core/tasks/handler/worker.go b/core/tasks/handler/worker.go
@@ -21,6 +21,7 @@ import (
 	"github.com/nyaruka/mailroom/core/queue"
 	"github.com/nyaruka/mailroom/core/runner"
 	"github.com/nyaruka/mailroom/runtime"
+	"github.com/nyaruka/mailroom/utils/dbutil"
 	"github.com/nyaruka/mailroom/utils/locker"
 	"github.com/nyaruka/null"
 	"github.com/pkg/errors"
@@ -170,6 +171,10 @@ func handleContactEvent(ctx context.Context, rt *runtime.Runtime, task *queue.Ta
 				"event":      event,
 			})
 
+			if qerr := dbutil.AsQueryError(err); qerr != nil {
+				log.WithFields(qerr.Fields())
+			}
+
 			contactEvent.ErrorCount++
 			if contactEvent.ErrorCount < 3 {
 				rc := rt.RP.Get()

diff --git a/utils/dbutil/errors.go b/utils/dbutil/errors.go
@@ -1,6 +1,12 @@
 package dbutil
 
-import "github.com/lib/pq"
+import (
+	"errors"
+	"fmt"
+
+	"github.com/lib/pq"
+	"github.com/sirupsen/logrus"
+)
 
 // IsUniqueViolation returns true if the given error is a violation of unique constraint
 func IsUniqueViolation(err error) bool {
@@ -9,3 +15,41 @@ func IsUniqueViolation(err error) bool {
 	}
 	return false
 }
+
+// QueryError is an error type for failed SQL queries
+type QueryError struct {
+	cause   error
+	message string
+	sql     string
+	sqlArgs []interface{}
+}
+
+func (e *QueryError) Error() string {
+	return e.message + ": " + e.cause.Error()
+}
+
+func (e *QueryError) Unwrap() error {
+	return e.cause
+}
+
+func (e *QueryError) Fields() logrus.Fields {
+	return logrus.Fields{
+		"sql":      fmt.Sprintf("%.1000s", e.sql),
+		"sql_args": e.sqlArgs,
+	}
+}
+
+func NewQueryErrorf(cause error, sql string, sqlArgs []interface{}, message string, msgArgs ...interface{}) error {
+	return &QueryError{
+		cause:   cause,
+		message: fmt.Sprintf(message, msgArgs...),
+		sql:     sql,
+		sqlArgs: sqlArgs,
+	}
+}
+
+func AsQueryError(err error) *QueryError {
+	var qerr *QueryError
+	errors.As(err, &qerr)
+	return qerr
+}
diff --git a/utils/dbutil/errors_test.go b/utils/dbutil/errors_test.go
@@ -5,6 +5,7 @@ import (
 
 	"github.com/lib/pq"
 	"github.com/nyaruka/mailroom/utils/dbutil"
+	"github.com/sirupsen/logrus"
 
 	"github.com/pkg/errors"
 	"github.com/stretchr/testify/assert"
@@ -16,3 +17,27 @@ func TestIsUniqueViolation(t *testing.T) {
 	assert.True(t, dbutil.IsUniqueViolation(err))
 	assert.False(t, dbutil.IsUniqueViolation(errors.New("boom")))
 }
+
+func TestQueryError(t *testing.T) {
+	var err error = &pq.Error{Code: pq.ErrorCode("22025"), Message: "unsupported Unicode escape sequence"}
+
+	qerr := dbutil.NewQueryErrorf(err, "SELECT * FROM foo WHERE id = $1", []interface{}{234}, "error selecting foo %d", 234)
+	assert.Error(t, qerr)
+	assert.Equal(t, `error selecting foo 234: pq: unsupported Unicode escape sequence`, qerr.Error())
+
+	// can unwrap to the original error
+	var pqerr *pq.Error
+	assert.True(t, errors.As(qerr, &pqerr))
+	assert.Equal(t, err, pqerr)
+
+	// can unwrap a wrapped error to find the first query error
+	wrapped := errors.Wrap(errors.Wrap(qerr, "error doing this"), "error doing that")
+	unwrapped := dbutil.AsQueryError(wrapped)
+	assert.Equal(t, qerr, unwrapped)
+
+	// nil if error was never a query error
+	wrapped = errors.Wrap(errors.New("error doing this"), "error doing that")
+	assert.Nil(t, dbutil.AsQueryError(wrapped))
+
+	assert.Equal(t, logrus.Fields{"sql": "SELECT * FROM foo WHERE id = $1", "sql_args": []interface{}{234}}, unwrapped.Fields())
+}
diff --git a/utils/dbutil/query.go b/utils/dbutil/query.go
@@ -29,7 +29,7 @@ func BulkQuery(ctx context.Context, tx Queryer, query string, structs []interfac
 
 	rows, err := tx.QueryxContext(ctx, bulkQuery, args...)
 	if err != nil {
-		return errors.Wrapf(err, "error making bulk query: %.5000s args=%v", bulkQuery, args)
+		return NewQueryErrorf(err, bulkQuery, args, "error making bulk query")
 	}
 	defer rows.Close()
 

diff --git a/utils/dbutil/query_test.go b/utils/dbutil/query_test.go
@@ -79,6 +79,6 @@ func TestBulkQuery(t *testing.T) {
 	// try with a struct that is invalid
 	foo4 := &foo{Name: "Jonny", Age: 34}
 	err = dbutil.BulkQuery(ctx, db, `INSERT INTO foo (name, age) VALUES(:name, :age)`, []interface{}{foo4})
-	assert.EqualError(t, err, "error making bulk query: INSERT INTO foo (name, age) VALUES($1, $2) args=[Jonny 34]: pq: value too long for type character varying(3)")
+	assert.EqualError(t, err, "error making bulk query: pq: value too long for type character varying(3)")
 	assert.Equal(t, 0, foo4.ID)
 }