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.

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

Raise sqlstate with message does not return message (used with custom http status codes). #2176

Closed
robertsosinski opened this issue Feb 20, 2022 · 6 comments

Comments

@robertsosinski
Copy link

robertsosinski commented Feb 20, 2022

  • PostgreSQL version: 13.1
  • PostgREST version: 9.0.0
  • Operating system: Docker on Win 10

When I raise an error with sqlstate I specify a message:

RAISE sqlstate 'PT402' using
  message = 'Payment Required',
  detail = 'Quota exceeded',
  hint = 'Upgrade your plan';

However, the message does not come along with the response:

{
    "hint":"Upgrade your plan",
    "details":"Quota exceeded"
}

If I raise with a built in error, the message comes through though:

RAISE  insufficient_privilege using 
  detail = 'Quota exceeded',
  hint = 'Upgrade your plan';

You can see here:

{
    "hint": "Upgrade your plan",
    "details": "Quota exceeded",
    "code": "42501",
    "message": "insufficient_privilege"
}

Would be great to get the message specified in the raise statement when specifying custom http status codes.

Thanks!

@robertsosinski robertsosinski changed the title Raise sqlstate with message does not return `message. Raise sqlstate with message does not return message (used with custom http status codes). Feb 20, 2022
@steve-chavez
Copy link
Member

RAISE sqlstate 'PT402' using
message = 'Payment Required',
detail = 'Quota exceeded',
hint = 'Upgrade your plan';

IIRC the message there is passed as the status message. So if you do:

 RAISE sqlstate 'PT452' using
   message = 'My Special Status',
   detail = 'Quota exceeded',
   hint = 'Upgrade your plan';

It would respond with:

HTTP/1.1 452 My Special Status 

I thought this could be used for defining new status codes/messages but perhaps the most useful behavior is to auto-generate the status messages for known status codes(similar to response.status) and leave them blank for unknown status codes.


Relevant code:

instance JSON.ToJSON SQL.CommandError where
toJSON (SQL.ResultError (SQL.ServerError c m d h)) = case BS.unpack c of
'P':'T':_ -> JSON.object [
"details" .= fmap T.decodeUtf8 d,
"hint" .= fmap T.decodeUtf8 h]
_ -> JSON.object [
"code" .= (T.decodeUtf8 c :: Text),
"message" .= (T.decodeUtf8 m :: Text),
"details" .= (fmap T.decodeUtf8 d :: Maybe Text),
"hint" .= (fmap T.decodeUtf8 h :: Maybe Text)]

@robertsosinski
Copy link
Author

That makes sense, I just took a look and indeed the message field is visible with the HTTP code. I think this works, but if we could also get the same message and code in the JSON response body that built in errors get, that would be more consistent. The response would look like:

plpgsql

 RAISE sqlstate 'PT452' using
   message = 'My Special Status',
   detail = 'Quota exceeded',
   hint = 'Upgrade your plan';

response

HTTP/1.1 452 My Special Status 

{
    "hint":"Upgrade your plan",
    "details":"Quota exceeded",
    "code": "PT452",
    "message": "My Special Status"
}

This would now make customer http errors have the same fields/format as built in errors.

I'm using the message field in the response body for an interface. Because custom messages do not have it, I have to now also check the response object to try and fetch it from the status, which would require some refactoring on how I've abstracted interaction with my postgrest endpoint. As such, I'll probably just keep doing what I'm doing, and when there is no message field in the response, just put "error".

Adding the status code would also make it easier to configure error flows too.

Interested in your thoughts.

@steve-chavez
Copy link
Member

I see no problem in including the code/message in the body, IIRC it was done that way to avoid having redundant info; and as you mention, it'd better for consistency in client libraries.

@steve-chavez
Copy link
Member

Btw, on #1926 we were trying to normalize the error messages: having them all respond with a "code" field.

Some postgrest errors don't include the details and hint field too. WDYT it'd be better? To have them always shown as details: null, hint: null or continue keeping them as "undefined"?

@robertsosinski
Copy link
Author

Btw, on #1926 we were trying to normalize the error messages: having them all respond with a "code" field.

Some postgrest errors don't include the details and hint field too. WDYT it'd be better? To have them always shown as details: null, hint: null or continue keeping them as "undefined"?

That is a good question actually. On one hand, undefined would make them not appear at all in the response, thus saving--a little--response body size. However, I feel most often postgrest returns null for things that are not defined. Perhaps replicate whatever behavior is ultimately implemented by #1601?

I could see it return null for now, and if #1601 gets implemented then when nulls=stripped is specified then strip the nulls out?

@laurenceisla
Copy link
Member

Fixed in #1926

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants