From 98798360355c333dbecbe80e373178f70587257b Mon Sep 17 00:00:00 2001 From: Ibrahim Jarif Date: Wed, 24 Oct 2018 11:30:20 +0530 Subject: [PATCH] Do not throw 500 on invalid value type in search API (#2321) Fixes https://github.com/openshiftio/openshift.io/issues/4429 --- controller/search_blackbox_test.go | 40 ++++++++++++++--------- gormsupport/postgres.go | 12 +++++++ search/search_repository.go | 8 ++++- search/search_repository_blackbox_test.go | 40 +++++++++++++++++++++++ 4 files changed, 84 insertions(+), 16 deletions(-) diff --git a/controller/search_blackbox_test.go b/controller/search_blackbox_test.go index c135bd207d..d82cece46f 100644 --- a/controller/search_blackbox_test.go +++ b/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() { diff --git a/gormsupport/postgres.go b/gormsupport/postgres.go index f5a146217d..b573f006ee 100644 --- a/gormsupport/postgres.go +++ b/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 { diff --git a/search/search_repository.go b/search/search_repository.go index c74d8274a6..35cd1a4982 100644 --- a/search/search_repository.go +++ b/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 { diff --git a/search/search_repository_blackbox_test.go b/search/search_repository_blackbox_test.go index c4cb8d9b77..620cd9507b 100644 --- a/search/search_repository_blackbox_test.go +++ b/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) {