Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add the number of rows affected in hint field of response of SingularityError #2874

Closed
wants to merge 1 commit into from
Closed

Add the number of rows affected in hint field of response of SingularityError #2874

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ This project adheres to [Semantic Versioning](http://semver.org/).
+ Note: data representations require Postgres 10 (Postgres 11 if using `IN` predicates); data representations are not implemented for RPC
- #2647, Allow to verify the PostgREST version in SQL: `select distinct application_name from pg_stat_activity`. - @laurenceisla
- #2856, Add the `--version` CLI option that prints the version information - @laurenceisla
- #1655, Add the number of rows affected to the `hint` field of response of `SingularityError`. - @taimoorzaeem
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- #1655, Add the number of rows affected to the `hint` field of response of `SingularityError`. - @taimoorzaeem
- #1655, Add the number of rows affected to the `hint` field of a singular response error. - @taimoorzaeem

SingularityError may confuse users, because it's an internal identifier in the code.


### Fixed

Expand Down
2 changes: 1 addition & 1 deletion src/PostgREST/Error.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -531,7 +531,7 @@ instance JSON.ToJSON Error where
"code" .= ApiRequestErrorCode16,
"message" .= ("JSON object requested, multiple (or no) rows returned" :: Text),
"details" .= T.unwords ["Results contain", show n, "rows,", T.decodeUtf8 (MediaType.toMime MTSingularJSON), "requires 1 row"],
"hint" .= JSON.Null]
"hint" .= (show n :: Text)]

toJSON (PgErr err) = JSON.toJSON err
toJSON (ApiRequestError err) = JSON.toJSON err
Expand Down
32 changes: 16 additions & 16 deletions test/spec/Feature/Query/SingularSpec.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ spec =
[("Prefer", "tx=commit"), singular]
[json| { address: "zzz" } |]
`shouldRespondWith`
[json|{"details":"Results contain 4 rows, application/vnd.pgrst.object+json requires 1 row","message":"JSON object requested, multiple (or no) rows returned","code":"PGRST116","hint":null}|]
[json|{"details":"Results contain 4 rows, application/vnd.pgrst.object+json requires 1 row","message":"JSON object requested, multiple (or no) rows returned","code":"PGRST116","hint":"4"}|]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, just noticing that we already have the affected rows on details. @laurenceisla Perhaps they were added after #1655 was opened and we forgot to close?

Copy link
Member

@laurenceisla laurenceisla Jul 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I understood that it was already present in the details, as shown in the issue's OP, but that an easy access to the number of rows was needed (as an alternative to the 404, 412), hence only the number of rows in the hint. That number is present in the details even in version 7.0.0, which was launched before the issue was opened, see this diff from 2017.

But maybe that wasn't contemplated in the issue too. , if that's the case, then yeah the 404, 412 should be the better solution.

{ matchStatus = 406
, matchHeaders = [ matchContentTypeSingular
, "Preference-Applied" <:> "tx=commit" ]
Expand All @@ -85,7 +85,7 @@ spec =
[("Prefer", "tx=commit"), ("Prefer", "return=representation"), singular]
[json| { address: "zzz" } |]
`shouldRespondWith`
[json|{"details":"Results contain 4 rows, application/vnd.pgrst.object+json requires 1 row","message":"JSON object requested, multiple (or no) rows returned","code":"PGRST116","hint":null}|]
[json|{"details":"Results contain 4 rows, application/vnd.pgrst.object+json requires 1 row","message":"JSON object requested, multiple (or no) rows returned","code":"PGRST116","hint":"4"}|]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@taimoorzaeem Besides the hint (which is likely unnecessary now), we can make an improvement to the error message:

Suggested change
[json|{"details":"Results contain 4 rows, application/vnd.pgrst.object+json requires 1 row","message":"JSON object requested, multiple (or no) rows returned","code":"PGRST116","hint":"4"}|]
[json|{"details":"The result contains 4 rows","message":"JSON object requested, multiple (or no) rows returned","code":"PGRST116"}|]

The reasoning is that I've been noticing that users ignore the error message when they see the application/vnd.pgrst.object+json. This happens when they use PostgREST through client libraries, like https://github.com/supabase/postgrest-js.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also technically the error message is not accurate since it should refer to json array elements or array length and not "rows". It could be reworded.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@steve-chavez "rows" is more consistent with other error messages and also since "rows" refer to the rows returned from the database so it is accurate IMO. Perhaps maybe no need to reword that.

{ matchStatus = 406
, matchHeaders = [ matchContentTypeSingular
, "Preference-Applied" <:> "tx=commit" ]
Expand All @@ -100,7 +100,7 @@ spec =
request methodPatch "/items?id=gt.0&id=lt.0"
[singular] [json|{"id":1}|]
`shouldRespondWith`
[json|{"details":"Results contain 0 rows, application/vnd.pgrst.object+json requires 1 row","message":"JSON object requested, multiple (or no) rows returned","code":"PGRST116","hint":null}|]
[json|{"details":"Results contain 0 rows, application/vnd.pgrst.object+json requires 1 row","message":"JSON object requested, multiple (or no) rows returned","code":"PGRST116","hint":"0"}|]
{ matchStatus = 406
, matchHeaders = [matchContentTypeSingular]
}
Expand All @@ -109,7 +109,7 @@ spec =
request methodPatch "/items?id=gt.0&id=lt.0"
[("Prefer", "return=representation"), singular] [json|{"id":1}|]
`shouldRespondWith`
[json|{"details":"Results contain 0 rows, application/vnd.pgrst.object+json requires 1 row","message":"JSON object requested, multiple (or no) rows returned","code":"PGRST116","hint":null}|]
[json|{"details":"Results contain 0 rows, application/vnd.pgrst.object+json requires 1 row","message":"JSON object requested, multiple (or no) rows returned","code":"PGRST116","hint":"0"}|]
{ matchStatus = 406
, matchHeaders = [matchContentTypeSingular]
}
Expand Down Expand Up @@ -141,7 +141,7 @@ spec =
[("Prefer", "tx=commit"), singular]
[json| [ { id: 200, address: "xxx" }, { id: 201, address: "yyy" } ] |]
`shouldRespondWith`
[json|{"details":"Results contain 2 rows, application/vnd.pgrst.object+json requires 1 row","message":"JSON object requested, multiple (or no) rows returned","code":"PGRST116","hint":null}|]
[json|{"details":"Results contain 2 rows, application/vnd.pgrst.object+json requires 1 row","message":"JSON object requested, multiple (or no) rows returned","code":"PGRST116","hint":"2"}|]
{ matchStatus = 406
, matchHeaders = [ matchContentTypeSingular
, "Preference-Applied" <:> "tx=commit" ]
Expand All @@ -157,7 +157,7 @@ spec =
[("Prefer", "tx=commit"), ("Prefer", "return=representation"), singular]
[json| [ { id: 202, address: "xxx" }, { id: 203, address: "yyy" } ] |]
`shouldRespondWith`
[json|{"details":"Results contain 2 rows, application/vnd.pgrst.object+json requires 1 row","message":"JSON object requested, multiple (or no) rows returned","code":"PGRST116","hint":null}|]
[json|{"details":"Results contain 2 rows, application/vnd.pgrst.object+json requires 1 row","message":"JSON object requested, multiple (or no) rows returned","code":"PGRST116","hint":"2"}|]
{ matchStatus = 406
, matchHeaders = [ matchContentTypeSingular
, "Preference-Applied" <:> "tx=commit" ]
Expand All @@ -173,7 +173,7 @@ spec =
[("Prefer", "tx=commit"), ("Prefer", "return=minimal"), singular]
[json| [ { id: 204, address: "xxx" }, { id: 205, address: "yyy" } ] |]
`shouldRespondWith`
[json|{"details":"Results contain 2 rows, application/vnd.pgrst.object+json requires 1 row","message":"JSON object requested, multiple (or no) rows returned","code":"PGRST116","hint":null}|]
[json|{"details":"Results contain 2 rows, application/vnd.pgrst.object+json requires 1 row","message":"JSON object requested, multiple (or no) rows returned","code":"PGRST116","hint":"2"}|]
{ matchStatus = 406
, matchHeaders = [ matchContentTypeSingular
, "Preference-Applied" <:> "tx=commit" ]
Expand All @@ -189,7 +189,7 @@ spec =
[singular]
[json| [ ] |]
`shouldRespondWith`
[json|{"details":"Results contain 0 rows, application/vnd.pgrst.object+json requires 1 row","message":"JSON object requested, multiple (or no) rows returned","code":"PGRST116","hint":null}|]
[json|{"details":"Results contain 0 rows, application/vnd.pgrst.object+json requires 1 row","message":"JSON object requested, multiple (or no) rows returned","code":"PGRST116","hint":"0"}|]
{ matchStatus = 406
, matchHeaders = [matchContentTypeSingular]
}
Expand All @@ -199,7 +199,7 @@ spec =
[("Prefer", "return=representation"), singular]
[json| [ ] |]
`shouldRespondWith`
[json|{"details":"Results contain 0 rows, application/vnd.pgrst.object+json requires 1 row","message":"JSON object requested, multiple (or no) rows returned","code":"PGRST116","hint":null}|]
[json|{"details":"Results contain 0 rows, application/vnd.pgrst.object+json requires 1 row","message":"JSON object requested, multiple (or no) rows returned","code":"PGRST116","hint":"0"}|]
{ matchStatus = 406
, matchHeaders = [matchContentTypeSingular]
}
Expand All @@ -222,7 +222,7 @@ spec =
[("Prefer", "tx=commit"), singular]
""
`shouldRespondWith`
[json|{"details":"Results contain 5 rows, application/vnd.pgrst.object+json requires 1 row","message":"JSON object requested, multiple (or no) rows returned","code":"PGRST116","hint":null}|]
[json|{"details":"Results contain 5 rows, application/vnd.pgrst.object+json requires 1 row","message":"JSON object requested, multiple (or no) rows returned","code":"PGRST116","hint":"5"}|]
{ matchStatus = 406
, matchHeaders = [ matchContentTypeSingular
, "Preference-Applied" <:> "tx=commit" ]
Expand All @@ -240,7 +240,7 @@ spec =
request methodDelete "/items?id=gt.5&id=lt.11"
[("Prefer", "tx=commit"), ("Prefer", "return=representation"), singular] ""
`shouldRespondWith`
[json|{"details":"Results contain 5 rows, application/vnd.pgrst.object+json requires 1 row","message":"JSON object requested, multiple (or no) rows returned","code":"PGRST116","hint":null}|]
[json|{"details":"Results contain 5 rows, application/vnd.pgrst.object+json requires 1 row","message":"JSON object requested, multiple (or no) rows returned","code":"PGRST116","hint":"5"}|]
{ matchStatus = 406
, matchHeaders = [ matchContentTypeSingular
, "Preference-Applied" <:> "tx=commit" ]
Expand All @@ -257,7 +257,7 @@ spec =
request methodDelete "/items?id=lt.0"
[singular] ""
`shouldRespondWith`
[json|{"details":"Results contain 0 rows, application/vnd.pgrst.object+json requires 1 row","message":"JSON object requested, multiple (or no) rows returned","code":"PGRST116","hint":null}|]
[json|{"details":"Results contain 0 rows, application/vnd.pgrst.object+json requires 1 row","message":"JSON object requested, multiple (or no) rows returned","code":"PGRST116","hint":"0"}|]
{ matchStatus = 406
, matchHeaders = [matchContentTypeSingular]
}
Expand All @@ -266,7 +266,7 @@ spec =
request methodDelete "/items?id=lt.0"
[("Prefer", "return=representation"), singular] ""
`shouldRespondWith`
[json|{"details":"Results contain 0 rows, application/vnd.pgrst.object+json requires 1 row","message":"JSON object requested, multiple (or no) rows returned","code":"PGRST116","hint":null}|]
[json|{"details":"Results contain 0 rows, application/vnd.pgrst.object+json requires 1 row","message":"JSON object requested, multiple (or no) rows returned","code":"PGRST116","hint":"0"}|]
{ matchStatus = 406
, matchHeaders = [matchContentTypeSingular]
}
Expand All @@ -276,7 +276,7 @@ spec =
request methodPost "/rpc/getproject"
[singular] [json|{ "id": 9999999}|]
`shouldRespondWith`
[json|{"details":"Results contain 0 rows, application/vnd.pgrst.object+json requires 1 row","message":"JSON object requested, multiple (or no) rows returned","code":"PGRST116","hint":null}|]
[json|{"details":"Results contain 0 rows, application/vnd.pgrst.object+json requires 1 row","message":"JSON object requested, multiple (or no) rows returned","code":"PGRST116","hint":"0"}|]
{ matchStatus = 406
, matchHeaders = [matchContentTypeSingular]
}
Expand All @@ -299,7 +299,7 @@ spec =
request methodPost "/rpc/getallprojects"
[singular] "{}"
`shouldRespondWith`
[json|{"details":"Results contain 5 rows, application/vnd.pgrst.object+json requires 1 row","message":"JSON object requested, multiple (or no) rows returned","code":"PGRST116","hint":null}|]
[json|{"details":"Results contain 5 rows, application/vnd.pgrst.object+json requires 1 row","message":"JSON object requested, multiple (or no) rows returned","code":"PGRST116","hint":"5"}|]
{ matchStatus = 406
, matchHeaders = [matchContentTypeSingular]
}
Expand All @@ -314,7 +314,7 @@ spec =
[("Prefer", "tx=commit"), singular]
[json| {"id_l": 1, "id_h": 2, "name": "changed"} |]
`shouldRespondWith`
[json|{"details":"Results contain 2 rows, application/vnd.pgrst.object+json requires 1 row","message":"JSON object requested, multiple (or no) rows returned","code":"PGRST116","hint":null}|]
[json|{"details":"Results contain 2 rows, application/vnd.pgrst.object+json requires 1 row","message":"JSON object requested, multiple (or no) rows returned","code":"PGRST116","hint":"2"}|]
{ matchStatus = 406
, matchHeaders = [ matchContentTypeSingular
, "Preference-Applied" <:> "tx=commit" ]
Expand Down