Add the number of rows affected in hint field of response of SingularityError #2874
Conversation
| @@ -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 | |||
There was a problem hiding this comment.
| - #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.
| @@ -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"}|] | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
| @@ -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"}|] | |||
There was a problem hiding this comment.
@taimoorzaeem Besides the hint (which is likely unnecessary now), we can make an improvement to the error message:
| [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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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.
|
Because we are making changes in a different direction now, I am closing this pull request. |
Fixes issue #1655.