diff --git a/CHANGELOG.md b/CHANGELOG.md index 67debfab9..1d798cae1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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 diff --git a/src/PostgREST/ApiRequest/Types.hs b/src/PostgREST/ApiRequest/Types.hs index b4c075954..e4fb6dc32 100644 --- a/src/PostgREST/ApiRequest/Types.hs +++ b/src/PostgREST/ApiRequest/Types.hs @@ -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 diff --git a/src/PostgREST/Error.hs b/src/PostgREST/Error.hs index 31288bf83..eefe57943 100644 --- a/src/PostgREST/Error.hs +++ b/src/PostgREST/Error.hs @@ -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 diff --git a/test/spec/Feature/Query/RpcSpec.hs b/test/spec/Feature/Query/RpcSpec.hs index 4887e8fac..60ae3d343 100644 --- a/test/spec/Feature/Query/RpcSpec.hs +++ b/test/spec/Feature/Query/RpcSpec.hs @@ -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 }