Do not throw 500 on invalid value type in search API (#2321)

Fixes openshiftio/openshift.io#4429

controller/search_blackbox_test.go

@@ -369,21 +369,31 @@ func (s *searchControllerTestSuite) TestSearchWorkItemsWithoutSpaceContext() {
 }
 
 func (s *searchControllerTestSuite) TestSearchFilter() {
-	// given
-	fxt := tf.NewTestFixture(s.T(), s.DB,
-		tf.WorkItems(1, func(fxt *tf.TestFixture, idx int) error {
-			fxt.WorkItems[idx].Fields[workitem.SystemTitle] = "specialwordforsearch"
-			return nil
-		}),
-	)
-	// when
-	filter := fmt.Sprintf(`{"$AND": [{"space": "%s"}]}`, fxt.WorkItems[0].SpaceID)
-	spaceIDStr := fxt.WorkItems[0].SpaceID.String()
-	_, sr := test.ShowSearchOK(s.T(), nil, nil, s.controller, &filter, nil, nil, nil, nil, &spaceIDStr)
-	// then
-	require.NotEmpty(s.T(), sr.Data)
-	r := sr.Data[0]
-	assert.Equal(s.T(), "specialwordforsearch", r.Attributes[workitem.SystemTitle])
+	s.T().Run("ok", func(t *testing.T) {
+		// given
+		fxt := tf.NewTestFixture(t, s.DB, tf.WorkItems(1, tf.SetWorkItemTitles("specialwordforsearch")))
+		// when
+		filter := fmt.Sprintf(`{"space": "%s"}`, fxt.WorkItems[0].SpaceID)
+		spaceIDStr := fxt.WorkItems[0].SpaceID.String()
+		_, sr := test.ShowSearchOK(t, nil, nil, s.controller, &filter, nil, nil, nil, nil, &spaceIDStr)
+		// then
+		require.NotEmpty(t, sr.Data)
+		r := sr.Data[0]
+		assert.Equal(t, "specialwordforsearch", r.Attributes[workitem.SystemTitle])
+	})
+
+	// Regression test for https://github.com/openshiftio/openshift.io/issues/4429
+	s.T().Run("fail - incorrect value type", func(t *testing.T) {
+		// given
+		fxt := tf.NewTestFixture(t, s.DB, tf.WorkItems(1))
+		// when
+		filter := `{"number": "foo"}`
+		spaceIDStr := fxt.WorkItems[0].SpaceID.String()
+		_, jerr := test.ShowSearchBadRequest(t, nil, nil, s.controller, &filter, nil, nil, nil, nil, &spaceIDStr)
+		// then
+		require.NotEmpty(t, jerr)
+		require.Len(t, jerr.Errors, 1)
+	})
 }
 
 func (s *searchControllerTestSuite) TestSearchByWorkItemTypeGroup() {

gormsupport/postgres.go

@@ -7,6 +7,7 @@ const (
 	errUniqueViolation     = "23505"
 	errForeignKeyViolation = "23503"
 	errInvalidCatalogName  = "3D000"
+	errDataException       = "22" // This class contains many data representation related errors. See https://www.postgresql.org/docs/current/static/errcodes-appendix.html
 )
 
 // IsCheckViolation returns true if the error is a violation of the given check
@@ -21,6 +22,17 @@ func IsCheckViolation(err error, constraintName string) bool {
 	return pqError.Code == errCheckViolation && pqError.Constraint == constraintName
 }
 
+// IsDataException returns true if the error is of type data exception (eg: type
+// mismatch, invalid delimiter, divide by zero, ...)
+// See https://www.postgresql.org/docs/current/static/errcodes-appendix.html
+func IsDataException(err error) bool {
+	if err == nil {
+		return false
+	}
+	pqError, ok := err.(*pq.Error)
+	return ok && pqError.Code[:2] == errDataException
+}
+
 // IsInvalidCatalogName returns true if the given error says that the catalog
 // is ivalid (e.g. database does not exist)
 func IsInvalidCatalogName(err error) bool {

search/search_repository.go

@@ -9,6 +9,8 @@ import (
 	"strings"
 	"sync"
 
+	"github.com/fabric8-services/fabric8-wit/gormsupport"
+
 	"github.com/fabric8-services/fabric8-wit/closeable"
 
 	"github.com/asaskevich/govalidator"
@@ -744,9 +746,13 @@ func (r *GormSearchRepository) listItemsFromDB(ctx context.Context, criteria cri
 	rows, err := db.Rows()
 	defer closeable.Close(ctx, rows)
 	if err != nil {
+		if gormsupport.IsDataException(err) {
+			// Remove "pq: " from the original message and return it.
+			errMessage := strings.Replace(err.Error(), "pq: ", "", -1)
+			return nil, 0, errors.NewBadParameterErrorFromString(errMessage)
+		}
 		return nil, 0, errs.WithStack(err)
 	}
-
 	result := []workitem.WorkItemStorage{}
 	columns, err := rows.Columns()
 	if err != nil {

search/search_repository_blackbox_test.go

@@ -585,6 +585,46 @@ func (s *searchRepositoryBlackboxTest) TestFilter() {
 		})
 	})
 
+	s.T().Run("fail - with incorrect value type", func(t *testing.T) {
+		// given
+		t.Run("integer instead of UUID", func(t *testing.T) {
+			filter := `{"space": 123}`
+			_, count, _, _, err := s.searchRepo.Filter(context.Background(), filter, nil, nil, nil)
+			require.Error(t, err)
+			assert.Equal(t, 0, count)
+		})
+
+		t.Run("string instead of UUID", func(t *testing.T) {
+			filter := `{"space": "foo"}`
+			_, count, _, _, err := s.searchRepo.Filter(context.Background(), filter, nil, nil, nil)
+			require.Error(t, err)
+			assert.Equal(t, 0, count)
+
+		})
+
+		// Regression test for https://github.com/openshiftio/openshift.io/issues/4429
+		t.Run("string instead of integer", func(t *testing.T) {
+			filter := `{"number":{"$EQ":"asd"}}`
+			_, count, _, _, err := s.searchRepo.Filter(context.Background(), filter, nil, nil, nil)
+			// when
+			require.Error(t, err)
+			assert.Equal(t, 0, count)
+
+			filter = `{"number":{"$EQ":"*"}}`
+			_, count, _, _, err = s.searchRepo.Filter(context.Background(), filter, nil, nil, nil)
+			// when
+			require.Error(t, err)
+			assert.Equal(t, 0, count)
+		})
+		t.Run("UUID instead of integer", func(t *testing.T) {
+			filter := fmt.Sprintf(`{"number":{"$EQ":"%s"}}`, uuid.NewV4())
+			_, count, _, _, err := s.searchRepo.Filter(context.Background(), filter, nil, nil, nil)
+			// when
+			require.Error(t, err)
+			assert.Equal(t, 0, count)
+		})
+	})
+
 	s.T().Run("with parent-exists filter", func(t *testing.T) {
 
 		t.Run("no link created", func(t *testing.T) {