jimmy/postgrest
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

feat: improve PGRST121 error message

Browse Source 
* Clarify the message field
* Show failed MESSAGE or DETAIL in the the PGRST121 error's details field
* Show the correct JSON format in the hint field
This commit is contained in:
Laurence Isla
2024-04-24 13:33:10 -05:00
committed by GitHub
parent 3eff4670f0
commit 69bbce5b32
4 changed files with 64 additions and 25 deletions
Download Patch File Download Diff File Expand all files Collapse all files
CHANGELOG.md
+6 -1
View File
@@ -17,6 +17,10 @@ This project adheres to [Semantic Versioning](http://semver.org/).
- #3435, Add log-level=debug, for development purposes - @steve-chavez
- #1526, Add `/metrics` endpoint on admin server - @steve-chavez
- Exposes connection pool metrics, schema cache metrics
- #3404, Show the failed MESSAGE or DETAIL in the `details` field of the `PGRST121` (could not parse RAISE 'PGRST') error - @laurenceisla
- #3404, Show extra information in the `PGRST121` (could not parse RAISE 'PGRST') error - @laurenceisla
+ Shows the failed MESSAGE or DETAIL in the `details` field
+ Shows the correct JSON format in the `hints` field
### Fixed
@@ -30,10 +34,11 @@ This project adheres to [Semantic Versioning](http://semver.org/).
- #3330, Incorrect admin server `/ready` response on slow schema cache loads - @steve-chavez
- #3340, Log when the LISTEN channel gets a notification - @steve-chavez
- #3345, Fix in-database configuration values not loading for `pgrst.server_trace_header` and `pgrst.server_cors_allowed_origins` - @laurenceisla
- #3361, Clarify PGRST204(column not found) error message - @steve-chavez
- #3361, Clarify the `PGRST204` (column not found) error message - @steve-chavez
- #3373, Remove rejected mediatype `application/vnd.pgrst.object+json` from response - @taimoorzaeem
- #3418, Fix OpenAPI not tagging a FK column correctly on O2O relationships - @laurenceisla
- #3256, Fix wrong http status for pg error `42P17 infinite recursion` - @taimoorzaeem
- #3404, Clarify the `PGRST121` (could not parse RAISE 'PGRST') error message - @laurenceisla
### Deprecated
src/PostgREST/ApiRequest/Types.hs
+7 -1
View File
@@ -25,6 +25,7 @@ module PostgREST.ApiRequest.Types
, OrderNulls(..)
, OrderTerm(..)
, QPError(..)
, RaiseError(..)
, RangeError(..)
, SingleVal
, TrileanVal(..)
@@ -95,12 +96,17 @@ data ApiRequestError
| OffLimitsChangesError Int64 Integer
| PutMatchingPkError
| SingularityError Integer
| PGRSTParseError
| PGRSTParseError RaiseError
| MaxAffectedViolationError Integer
deriving Show
data QPError = QPError Text Text
deriving Show
data RaiseError
= MsgParseError ByteString
| DetParseError ByteString
| NoDetail
deriving Show
data RangeError
= NegativeLimit
| LowerGTUpper
src/PostgREST/Error.hs
+33 -17
View File
@@ -34,6 +34,7 @@ import Network.HTTP.Types.Header (Header)
import PostgREST.ApiRequest.Types (ApiRequestError (..),
QPError (..),
RaiseError (..),
RangeError (..))
import PostgREST.MediaType (MediaType (..))
import qualified PostgREST.MediaType as MediaType
@@ -90,7 +91,7 @@ instance PgrstError ApiRequestError where
status OffLimitsChangesError{} = HTTP.status400
status PutMatchingPkError = HTTP.status400
status SingularityError{} = HTTP.status406
status PGRSTParseError = HTTP.status500
status PGRSTParseError{} = HTTP.status500
status MaxAffectedViolationError{} = HTTP.status400
headers _ = mempty
@@ -187,8 +188,11 @@ instance JSON.ToJSON ApiRequestError where
(Just "Only is null or not is null filters are allowed on embedded resources")
Nothing
toJSON PGRSTParseError = toJsonPgrstError
ApiRequestErrorCode21 "The message and detail field of RAISE 'PGRST' error expects JSON" Nothing Nothing
toJSON (PGRSTParseError raiseErr) = toJsonPgrstError
ApiRequestErrorCode21
"Could not parse JSON in the \"RAISE SQLSTATE 'PGRST'\" error"
(Just $ JSON.String $ pgrstParseErrorDetails raiseErr)
(Just $ JSON.String $ pgrstParseErrorHint raiseErr)
toJSON (InvalidPreferences prefs) = toJsonPgrstError
ApiRequestErrorCode22
@@ -387,6 +391,17 @@ relHint rels = T.intercalate ", " (hintList <$> rels)
-- An ambiguousness error cannot happen for computed relationships TODO refactor so this mempty is not needed
hintList ComputedRelationship{} = mempty
pgrstParseErrorDetails :: RaiseError -> Text
pgrstParseErrorDetails err = case err of
MsgParseError m -> "Invalid JSON value for MESSAGE: '" <> T.decodeUtf8 m <> "'"
DetParseError d -> "Invalid JSON value for DETAIL: '" <> T.decodeUtf8 d <> "'"
NoDetail -> "DETAIL is missing in the RAISE statement"
pgrstParseErrorHint :: RaiseError -> Text
pgrstParseErrorHint err = case err of
MsgParseError _ -> "MESSAGE must be a JSON object with obligatory keys: 'code', 'message' and optional keys: 'details', 'hint'."
_ -> "DETAIL must be a JSON object with obligatory keys: 'status', 'headers' and optional key: 'status_text'."
data PgError = PgError Authenticated SQL.UsageError
type Authenticated = Bool
@@ -394,9 +409,9 @@ instance PgrstError PgError where
status (PgError authed usageError) = pgErrorStatus authed usageError
headers (PgError _ (SQL.SessionUsageError (SQL.QueryError _ _ (SQL.ResultError (SQL.ServerError "PGRST" m d _ _p))))) =
case (parseMessage m, parseDetails d) of
(Just _, Just r) -> headers PGRSTParseError ++ map intoHeader (M.toList $ getHeaders r)
_ -> headers PGRSTParseError
case parseRaisePGRST m d of
Right (_, r) -> map intoHeader (M.toList $ getHeaders r)
Left e -> headers e
where
intoHeader (k,v) = (CI.mk $ T.encodeUtf8 k, T.encodeUtf8 v)
@@ -426,13 +441,13 @@ instance JSON.ToJSON SQL.QueryError where
instance JSON.ToJSON SQL.CommandError where
-- Special error raised with code PGRST, to allow full response control
toJSON (SQL.ResultError (SQL.ServerError "PGRST" m d _ _p)) =
case (parseMessage m, parseDetails d) of
(Just r, Just _) -> JSON.object [
case parseRaisePGRST m d of
Right (r, _) -> JSON.object [
"code" .= getCode r,
"message" .= getMessage r,
"details" .= checkMaybe (getDetails r),
"hint" .= checkMaybe (getHint r)]
_ -> JSON.toJSON PGRSTParseError
Left e -> JSON.toJSON e
where
checkMaybe = maybe JSON.Null JSON.String
@@ -493,9 +508,9 @@ pgErrorStatus authed (SQL.SessionUsageError (SQL.QueryError _ _ (SQL.ResultError
"42501" -> if authed then HTTP.status403 else HTTP.status401 -- insufficient privilege
'P':'T':n -> fromMaybe HTTP.status500 (HTTP.mkStatus <$> readMaybe n <*> pure m)
"PGRST" ->
case (parseMessage m, parseDetails d) of
(Just _, Just r) -> maybe (toEnum $ getStatus r) (HTTP.mkStatus (getStatus r) . T.encodeUtf8) (getStatusText r)
_ -> status PGRSTParseError
case parseRaisePGRST m d of
Right (_, r) -> maybe (toEnum $ getStatus r) (HTTP.mkStatus (getStatus r) . T.encodeUtf8) (getStatusText r)
Left e -> status e
_ -> HTTP.status400
_ -> HTTP.status500
@@ -579,11 +594,12 @@ instance JSON.FromJSON PgRaiseErrDetails where
parseJSON _ = mzero
parseMessage :: ByteString -> Maybe PgRaiseErrMessage
parseMessage = JSON.decodeStrict
parseDetails :: Maybe ByteString -> Maybe PgRaiseErrDetails
parseDetails d = JSON.decodeStrict =<< d
parseRaisePGRST :: ByteString -> Maybe ByteString -> Either ApiRequestError (PgRaiseErrMessage, PgRaiseErrDetails)
parseRaisePGRST m d = do
msgJson <- maybeToRight (PGRSTParseError $ MsgParseError m) (JSON.decodeStrict m)
det <- maybeToRight (PGRSTParseError NoDetail) d
detJson <- maybeToRight (PGRSTParseError $ DetParseError det) (JSON.decodeStrict det)
return (msgJson, detJson)
-- Error codes are grouped by common modules or characteristics
data ErrorCode
test/spec/Feature/Query/RpcSpec.hs
+18 -6
View File
@@ -1452,17 +1452,29 @@ spec actualPgVersion =
resHeaders `shouldSatisfy` elem ("X-Header", "str")
resBody `shouldBe` [json|{"code":"123","message":"ABC","details":null,"hint":null}|]
it "returns error for invalid JSON in RAISE Message field" $
it "returns error for invalid JSON in the MESSAGE option of the RAISE statement" $
get "/rpc/raise_sqlstate_invalid_json_message" `shouldRespondWith`
[json|{"code":"PGRST121","message":"The message and detail field of RAISE 'PGRST' error expects JSON","details":null,"hint":null}|]
[json|{
"code":"PGRST121",
"message":"Could not parse JSON in the \"RAISE SQLSTATE 'PGRST'\" error",
"details":"Invalid JSON value for MESSAGE: 'INVALID'",
"hint":"MESSAGE must be a JSON object with obligatory keys: 'code', 'message' and optional keys: 'details', 'hint'."}|]
{ matchStatus = 500 }
it "returns error for invalid JSON in RAISE Details field" $
it "returns error for invalid JSON in the DETAIL option of the RAISE statement" $
get "/rpc/raise_sqlstate_invalid_json_details" `shouldRespondWith`
[json|{"code":"PGRST121","message":"The message and detail field of RAISE 'PGRST' error expects JSON","details":null,"hint":null}|]
[json|{
"code":"PGRST121",
"message":"Could not parse JSON in the \"RAISE SQLSTATE 'PGRST'\" error",
"details":"Invalid JSON value for DETAIL: 'INVALID'",
"hint":"DETAIL must be a JSON object with obligatory keys: 'status', 'headers' and optional key: 'status_text'."}|]
{ matchStatus = 500 }
it "returns error for missing Details field in RAISE" $
it "returns error for missing DETAIL option in the RAISE statement" $
get "/rpc/raise_sqlstate_missing_details" `shouldRespondWith`
[json|{"code":"PGRST121","message":"The message and detail field of RAISE 'PGRST' error expects JSON","details":null,"hint":null}|]
[json|{
"code":"PGRST121",
"message":"Could not parse JSON in the \"RAISE SQLSTATE 'PGRST'\" error",
"details":"DETAIL is missing in the RAISE statement",
"hint":"DETAIL must be a JSON object with obligatory keys: 'status', 'headers' and optional key: 'status_text'."}|]
{ matchStatus = 500 }