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

jarifibrahim · 2018-10-17T06:36:48Z

The search API uses values in the query to generate the SQL query. The backend throws internal server error if a query like foo:123 is sent where foo is of type bool. This PR fixes this issue.

With this PR

We return Bad Request with error like

error listing work items for expression '
    {"$AND":[
        { "number": { "$EQ" : "asd" } },
        { "space": { "$EQ" : "981fbeef-aa1f-4429-8301-70dc176079da" } }
    ]}': invalid input syntax for integer: "asd"

instead of Internal Server Error

Fixes openshiftio/openshift.io#4429
Depends on #2322

Todo

Add missing tests at the repository level. (We have very few tests for the Filter() method at the repository level)

jarifibrahim · 2018-10-18T09:29:27Z

search/search_repository.go

+		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)


This generates errors which look like

error listing work items for expression ' {"$AND":[ { "number": { "$EQ" : "asd" } }, { "space": { "$EQ" : "981fbeef-aa1f-4429-8301-70dc176079da" } } ]}': invalid input syntax for integer: "asd"

codecov · 2018-10-18T09:44:18Z

Codecov Report

Merging #2321 into master will increase coverage by 0.02%.
The diff coverage is 75%.

@@            Coverage Diff             @@
##           master    #2321      +/-   ##
==========================================
+ Coverage   70.13%   70.15%   +0.02%     
==========================================
  Files         171      171              
  Lines       16621    16629       +8     
==========================================
+ Hits        11657    11666       +9     
+ Misses       3832     3831       -1     
  Partials     1132     1132

Impacted Files	Coverage Δ
search/search_repository.go	`77.57% <100%> (+1.13%)`	⬆️
gormsupport/postgres.go	`42.42% <60%> (+3.13%)`	⬆️
workitem/workitem_repository.go	`67.79% <0%> (-0.23%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3d163c7...8ec44e1. Read the comment docs.

kwk

LGTM

baijum · 2018-10-23T11:21:45Z

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/9.5/static/errcodes-appendix.html


Shall we use non-version link: https://www.postgresql.org/docs/current/static/errcodes-appendix.html

Yes, I have changed it a15afc9

baijum · 2018-10-23T11:22:11Z

gormsupport/postgres.go

@@ -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/9.5/static/errcodes-appendix.html


Use https://www.postgresql.org/docs/current/static/errcodes-appendix.html

Yes a15afc9

baijum · 2018-10-23T11:30:20Z

search/search_repository_blackbox_test.go

+
+		// 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"}`


The curly bracket is not closed. The assertion should check for the message.

Thank you. I've fixed it 5f559bc

The error message depends on the input value. Do you think we should still do it?
Error for an integer would be different from the error for a string.

baijum · 2018-10-23T11:30:31Z

search/search_repository_blackbox_test.go

+			require.Error(t, err)
+			assert.Equal(t, 0, count)
+
+			filter = `{"number":{"$EQ":"*"}`


The curly bracket is not closed. The assertion should check for the message.

Thank you. I've fixed it 5f559bc

#2321 (comment)

baijum · 2018-10-23T11:30:50Z

search/search_repository_blackbox_test.go

+			assert.Equal(t, 0, count)
+		})
+		t.Run("UUID instead of integer", func(t *testing.T) {
+			filter := fmt.Sprintf(`{"number":{"$EQ":"%s"}`, uuid.NewV4())


The curly bracket is not closed. The assertion should check for the message.

Thank you. I've fixed it 5f559bc

#2321 (comment)

…him/fabric8-wit into search-issue-osio-4429

baijum

LGTM

# Changes **Commit:** fabric8-services/fabric8-wit@11904c7 **Author:** Konrad Kleine (193408+kwk@users.noreply.github.com) **Date:** 2018-10-10T13:49:39+02:00 Assign number to new areas/iterations and allow searching by it (fabric8-services/fabric8-wit#2311) ---- **Commit:** fabric8-services/fabric8-wit@8674b7b **Author:** Konrad Kleine (193408+kwk@users.noreply.github.com) **Date:** 2018-10-10T16:58:17+02:00 update number for existing iterations (fabric8-services/fabric8-wit#2314) ---- **Commit:** fabric8-services/fabric8-wit@e67b5e2 **Author:** nurali-techie (nurali.techie@gmail.com) **Date:** 2018-10-15T14:00:33+05:30 stop calling analytics api for cve scan reg/de-reg (fabric8-services/fabric8-wit#2316) ---- **Commit:** fabric8-services/fabric8-wit@37a913a **Author:** Baiju Muthukadan (baiju.m.mail@gmail.com) **Date:** 2018-10-15T17:07:20+05:30 Include fabric8-common as a dependency (fabric8-services/fabric8-wit#2318) ---- **Commit:** fabric8-services/fabric8-wit@e09b805 **Author:** Konrad Kleine (193408+kwk@users.noreply.github.com) **Date:** 2018-10-15T14:44:35+02:00 Use Postgres CTE to update iteration numbers (fabric8-services/fabric8-wit#2319) ---- **Commit:** fabric8-services/fabric8-wit@1724c15 **Author:** Ibrahim Jarif (jarifibrahim@gmail.com) **Date:** 2018-10-23T10:12:56+05:30 Rearrage search repository blackbox tests (fabric8-services/fabric8-wit#2322) ---- **Commit:** fabric8-services/fabric8-wit@7fda6af **Author:** Ibrahim Jarif (jarifibrahim@gmail.com) **Date:** 2018-10-23T13:49:11+05:30 Make agile default template for space creation (fabric8-services/fabric8-wit#2323) ---- **Commit:** fabric8-services/fabric8-wit@7b78cf6 **Author:** Konrad Kleine (193408+kwk@users.noreply.github.com) **Date:** 2018-10-23T11:54:01+02:00 Postpone iteration number update (fabric8-services/fabric8-wit#2325) ---- **Commit:** fabric8-services/fabric8-wit@22e3d5d **Author:** Dhriti Shikhar (dhriti.shikhar.rokz@gmail.com) **Date:** 2018-10-23T16:06:35+05:30 Enable the delete workitem endpoint (fabric8-services/fabric8-wit#2305) ---- **Commit:** fabric8-services/fabric8-wit@3d163c7 **Author:** Ibrahim Jarif (jarifibrahim@gmail.com) **Date:** 2018-10-23T17:45:11+05:30 Increase deployment timeout to 2X (fabric8-services/fabric8-wit#2327) ---- **Commit:** fabric8-services/fabric8-wit@9879836 **Author:** Ibrahim Jarif (jarifibrahim@gmail.com) **Date:** 2018-10-24T11:30:20+05:30 Do not throw 500 on invalid value type in search API (fabric8-services/fabric8-wit#2321) ---- **Commit:** fabric8-services/fabric8-wit@44739f8 **Author:** Ibrahim Jarif (jarifibrahim@gmail.com) **Date:** 2018-10-25T10:57:03+05:30 Remove redundant code and fix typo (fabric8-services/fabric8-wit#2329)

jarifibrahim added 2 commits October 17, 2018 12:05

Add regression test for search API 500

227826e

fix issue

94bc8f4

jarifibrahim changed the title ~~Add regression test for search API 500~~ WIP: Add regression test for search API 500 Oct 18, 2018

alien-ike added the work-in-progress label Oct 18, 2018

jarifibrahim commented Oct 18, 2018

View reviewed changes

jarifibrahim self-assigned this Oct 18, 2018

jarifibrahim changed the title ~~WIP: Add regression test for search API 500~~ WIP: Do not throw 500 on invalid value type in search API Oct 22, 2018

jarifibrahim added 2 commits October 23, 2018 11:33

Merge branch 'master' into search-issue-osio-4429

4d2d6de

Add test for the actual issue

018c491

jarifibrahim changed the title ~~WIP: Do not throw 500 on invalid value type in search API~~ Do not throw 500 on invalid value type in search API Oct 23, 2018

alien-ike removed the work-in-progress label Oct 23, 2018

Remove extra test

952a414

jarifibrahim requested review from kwk and baijum October 23, 2018 06:33

jarifibrahim added 🙈 bug 🔍 search labels Oct 23, 2018

jarifibrahim and others added 2 commits October 23, 2018 12:29

fixup

c5e481d

Merge branch 'master' into search-issue-osio-4429

0dc8999

kwk approved these changes Oct 23, 2018

View reviewed changes

baijum reviewed Oct 23, 2018

View reviewed changes

jarifibrahim added 4 commits October 23, 2018 17:34

add missing }

5f559bc

Fix comment

a15afc9

Merge branch 'search-issue-osio-4429' of https://github.com/jarifibra…

1920400

…him/fabric8-wit into search-issue-osio-4429

Merge branch 'master' into search-issue-osio-4429

8ec44e1

baijum approved these changes Oct 24, 2018

View reviewed changes

jarifibrahim merged commit 9879836 into fabric8-services:master Oct 24, 2018

jarifibrahim deleted the search-issue-osio-4429 branch October 24, 2018 06:00

jarifibrahim mentioned this pull request Oct 30, 2018

Update core.yaml openshiftio/saas-openshiftio#1121

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jarifibrahim commented Oct 17, 2018 •

edited

Loading

jarifibrahim Oct 18, 2018 •

edited

Loading

codecov bot commented Oct 18, 2018 •

edited

Loading

kwk left a comment

baijum Oct 23, 2018

jarifibrahim Oct 23, 2018

baijum Oct 23, 2018

jarifibrahim Oct 23, 2018

baijum Oct 23, 2018

jarifibrahim Oct 23, 2018

jarifibrahim Oct 23, 2018

baijum Oct 23, 2018

jarifibrahim Oct 23, 2018

jarifibrahim Oct 23, 2018

baijum Oct 23, 2018

jarifibrahim Oct 23, 2018

jarifibrahim Oct 23, 2018

baijum left a comment

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

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

Conversation

jarifibrahim commented Oct 17, 2018 • edited Loading

jarifibrahim Oct 18, 2018 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Oct 18, 2018 • edited Loading

Codecov Report

kwk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

baijum left a comment

Choose a reason for hiding this comment

jarifibrahim commented Oct 17, 2018 •

edited

Loading

jarifibrahim Oct 18, 2018 •

edited

Loading

codecov bot commented Oct 18, 2018 •

edited

Loading