From 6d7bf9faa916471ad1bd70cf0e1926599d63671b Mon Sep 17 00:00:00 2001 From: steve-chavez Date: Sat, 9 Jul 2022 14:27:27 -0500 Subject: [PATCH] fix: no empty tx on Not Found error --- CHANGELOG.md | 1 + src/PostgREST/App.hs | 10 +- src/PostgREST/Error.hs | 5 +- src/PostgREST/Request/ApiRequest.hs | 93 +++++++++---------- src/PostgREST/Request/Types.hs | 1 + .../Feature/OpenApi/DisabledOpenApiSpec.hs | 8 +- test/spec/Feature/Query/ErrorSpec.hs | 7 ++ 7 files changed, 62 insertions(+), 63 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 22f63ad9f..6590ddc75 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -54,6 +54,7 @@ This project adheres to [Semantic Versioning](http://semver.org/). - #1959, An accidental full table PATCH(without filters) is not possible anymore, it requires filters or a `limit` parameter - @steve-chavez, @laurenceisla - #2317, Increase the `db-pool-timeout` to 1 hour to prevent frequent high connection latency - @steve-chavez - #2341, The search path now correctly identifies schemas with uppercase and special characters in their names (regression) - @laurenceisla + - #2364, "404 Not Found" on nested routes doesn't start an empty database transaction - @steve-chavez ### Changed diff --git a/src/PostgREST/App.hs b/src/PostgREST/App.hs index d220d0266..7cb46fe64 100644 --- a/src/PostgREST/App.hs +++ b/src/PostgREST/App.hs @@ -54,6 +54,7 @@ import qualified PostgREST.Query.Statements as Statements import qualified PostgREST.RangeQuery as RangeQuery import qualified PostgREST.Request.ApiRequest as ApiRequest import qualified PostgREST.Request.DbRequestBuilder as ReqBuilder +import qualified PostgREST.Request.Types as ApiRequestTypes import PostgREST.AppState (AppState) import PostgREST.Auth (AuthResult (..)) @@ -242,10 +243,10 @@ handleRequest context@(RequestContext _ _ ApiRequest{..} _) = handleInvoke invMethod proc context (ActionInspect headersOnly, TargetDefaultSpec tSchema) -> handleOpenApi headersOnly tSchema context - (ActionUnknown verb, _) -> - throwError $ Error.UnsupportedVerb verb _ -> - throwError Error.NotFound + -- This is unreachable as the ApiRequest.hs rejects it before + -- TODO Refactor the Action/Target types to remove this line + throwError $ Error.ApiRequestError ApiRequestTypes.NotFound handleRead :: Bool -> QualifiedIdentifier -> RequestContext -> DbHandler Wai.Response handleRead headersOnly identifier context@RequestContext{..} = do @@ -419,7 +420,8 @@ handleInfo identifier RequestContext{..} = Just table -> return $ Wai.responseLBS HTTP.status200 [allOrigins, allowH table] mempty Nothing -> - throwError Error.NotFound + -- TODO is this right? When no tbl is found on the schema cache we disallow OPTIONS? + throwError $ Error.ApiRequestError ApiRequestTypes.NotFound where tbl = HM.lookup identifier (dbTables ctxDbStructure) allOrigins = ("Access-Control-Allow-Origin", "*") diff --git a/src/PostgREST/Error.hs b/src/PostgREST/Error.hs index d35bec9a9..365cf561d 100644 --- a/src/PostgREST/Error.hs +++ b/src/PostgREST/Error.hs @@ -61,6 +61,7 @@ instance PgrstError ApiRequestError where status InvalidBody{} = HTTP.status400 status InvalidFilters = HTTP.status405 status InvalidRange = HTTP.status416 + status NotFound = HTTP.status404 status NoRelBetween{} = HTTP.status400 status NoRpc{} = HTTP.status404 status NotEmbedded{} = HTTP.status400 @@ -113,6 +114,7 @@ instance JSON.ToJSON ApiRequestError where "message" .= ("None of these media types are available: " <> T.intercalate ", " (map T.decodeUtf8 cts)), "details" .= JSON.Null, "hint" .= JSON.Null] + toJSON NotFound = JSON.object [] toJSON (NotEmbedded resource) = JSON.object [ "code" .= ApiRequestErrorCode08, "message" .= ("Cannot apply filter because '" <> resource <> "' is not an embedded resource in this request" :: Text), @@ -311,7 +313,6 @@ data Error | JwtTokenMissing | JwtTokenRequired | NoSchemaCacheError - | NotFound | OffLimitsChangesError Int64 Integer | PgErr PgError | PutMatchingPkError @@ -327,7 +328,6 @@ instance PgrstError Error where status JwtTokenMissing = HTTP.status500 status JwtTokenRequired = HTTP.unauthorized401 status NoSchemaCacheError = HTTP.status503 - status NotFound = HTTP.status404 status OffLimitsChangesError{} = HTTP.status400 status (PgErr err) = status err status PutMatchingPkError = HTTP.status400 @@ -404,7 +404,6 @@ instance JSON.ToJSON Error where "details" .= JSON.Null, "hint" .= JSON.Null] - toJSON NotFound = JSON.object [] toJSON (PgErr err) = JSON.toJSON err toJSON (ApiRequestError err) = JSON.toJSON err diff --git a/src/PostgREST/Request/ApiRequest.hs b/src/PostgREST/Request/ApiRequest.hs index 08ad24e7a..45df97998 100644 --- a/src/PostgREST/Request/ApiRequest.hs +++ b/src/PostgREST/Request/ApiRequest.hs @@ -100,20 +100,17 @@ data Action | ActionUnknown Text deriving Eq -- | The path info that will be mapped to a target (used to handle validations and errors before defining the Target) -data Path +data PathInfo = PathInfo - { pSchema :: Schema, - pName :: Text, - pHasRpc :: Bool, - pIsDefaultSpec :: Bool, - pIsRootSpec :: Bool + { pathName :: Text + , pathIsProc :: Bool + , pathIsDefSpec :: Bool + , pathIsRootSpec :: Bool } - | PathUnknown -- | The target db object of a user action data Target = TargetIdent QualifiedIdentifier | TargetProc{tProc :: ProcDescription, tpIsRootSpec :: Bool} | TargetDefaultSpec{tdsSchema :: Schema} -- The default spec offered at root "/" - | TargetUnknown -- | RPC query param value `/rpc/func?v=`, used for VARIADIC functions on form-urlencoded POST and GETs -- | It can be fixed `?v=1` or repeated `?v=1&v=2&v=3. @@ -178,13 +175,26 @@ data ApiRequest = ApiRequest { -- | Examines HTTP request and translates it into user intent. userApiRequest :: AppConfig -> DbStructure -> Request -> RequestBody -> Either ApiRequestError ApiRequest -userApiRequest conf dbStructure req reqBody = - apiRequest conf dbStructure req reqBody =<< first QueryParamError (QueryParams.parse (rawQueryString req)) +userApiRequest conf dbStructure req reqBody = do + qPrms <- first QueryParamError (QueryParams.parse (rawQueryString req)) + pInfo <- getPathInfo conf $ pathInfo req + apiRequest conf dbStructure req reqBody qPrms pInfo -apiRequest :: AppConfig -> DbStructure -> Request -> RequestBody -> QueryParams.QueryParams -> Either ApiRequestError ApiRequest -apiRequest conf@AppConfig{..} dbStructure req reqBody queryparams@QueryParams{..} +getPathInfo :: AppConfig -> [Text] -> Either ApiRequestError PathInfo +getPathInfo AppConfig{configOpenApiMode, configDbRootSpec} path = + case path of + [] -> case configDbRootSpec of + Just (QualifiedIdentifier _ pathName) -> Right $ PathInfo pathName True False True + Nothing | configOpenApiMode == OADisabled -> Left NotFound + | otherwise -> Right $ PathInfo mempty False True False + [table] -> Right $ PathInfo table False False False + ["rpc", pName] -> Right $ PathInfo pName True False False + _ -> Left NotFound + +apiRequest :: AppConfig -> DbStructure -> Request -> RequestBody -> QueryParams.QueryParams -> PathInfo -> Either ApiRequestError ApiRequest +apiRequest conf@AppConfig{..} dbStructure req reqBody queryparams@QueryParams{..} path@PathInfo{pathName, pathIsProc, pathIsRootSpec, pathIsDefSpec} | isJust profile && fromJust profile `notElem` configDbSchemas = Left $ UnacceptableSchema $ toList configDbSchemas - | isTargetingProc && method `notElem` ["HEAD", "GET", "POST"] = Left ActionInappropriate + | pathIsProc && method `notElem` ["HEAD", "GET", "POST"] = Left ActionInappropriate | isInvalidRange = Left InvalidRange | shouldParsePayload && isLeft payload = either (Left . InvalidBody) witness payload | not expectParams && not (L.null qsParams) = Left $ ParseRequestError "Unexpected param or filter missing operator" ("Failed to parse " <> show qsParams) @@ -217,14 +227,8 @@ apiRequest conf@AppConfig{..} dbStructure req reqBody queryparams@QueryParams{.. where accepts = maybe [MTAny] (map MediaType.decodeMediaType . parseHttpAccept) $ lookupHeader "accept" - expectParams = isTargetingProc && method /= "POST" + expectParams = pathIsProc && method /= "POST" - isTargetingProc = case path of - PathInfo{pHasRpc, pIsRootSpec} -> pHasRpc || pIsRootSpec - _ -> False - isTargetingDefaultSpec = case path of - PathInfo{pIsDefaultSpec=True} -> True - _ -> False contentMediaType = maybe MTApplicationJSON MediaType.decodeMediaType $ lookupHeader "content-type" columns = case action of @@ -243,12 +247,12 @@ apiRequest conf@AppConfig{..} dbStructure req reqBody queryparams@QueryParams{.. (Just RawJSON{}, Just cls) -> cls _ -> S.empty payload :: Either ByteString Payload - payload = case (contentMediaType, isTargetingProc) of + payload = case (contentMediaType, pathIsProc) of (MTApplicationJSON, _) -> if isJust columns then Right $ RawJSON reqBody else note "All object keys must match" . payloadAttributes reqBody - =<< if LBS.null reqBody && isTargetingProc + =<< if LBS.null reqBody && pathIsProc then Right emptyObject else first BS.pack $ JSON.eitherDecode reqBody (MTTextCSV, _) -> do @@ -266,13 +270,13 @@ apiRequest conf@AppConfig{..} dbStructure req reqBody queryparams@QueryParams{.. case method of -- The HEAD method is identical to GET except that the server MUST NOT return a message-body in the response -- From https://www.w3.org/Protocols/rfc2616/rfc2616-sec9.html#sec9.4 - "HEAD" | isTargetingDefaultSpec -> ActionInspect{isHead=True} - | isTargetingProc -> ActionInvoke InvHead - | otherwise -> ActionRead{isHead=True} - "GET" | isTargetingDefaultSpec -> ActionInspect{isHead=False} - | isTargetingProc -> ActionInvoke InvGet - | otherwise -> ActionRead{isHead=False} - "POST" -> if isTargetingProc + "HEAD" | pathIsDefSpec -> ActionInspect{isHead=True} + | pathIsProc -> ActionInvoke InvHead + | otherwise -> ActionRead{isHead=True} + "GET" | pathIsDefSpec -> ActionInspect{isHead=False} + | pathIsProc -> ActionInvoke InvGet + | otherwise -> ActionRead{isHead=False} + "POST" -> if pathIsProc then ActionInvoke InvPost else ActionMutate MutationCreate "PATCH" -> ActionMutate MutationUpdate @@ -295,19 +299,17 @@ apiRequest conf@AppConfig{..} dbStructure req reqBody queryparams@QueryParams{.. where contentProfile = Just $ maybe defaultSchema T.decodeUtf8 $ lookupHeader "Content-Profile" acceptProfile = Just $ maybe defaultSchema T.decodeUtf8 $ lookupHeader "Accept-Profile" + schema = fromMaybe defaultSchema profile - target = - let + + target + | pathIsProc = (`TargetProc` pathIsRootSpec) <$> callFindProc schema pathName + | pathIsDefSpec = Right $ TargetDefaultSpec schema + | otherwise = Right $ TargetIdent $ QualifiedIdentifier schema pathName + where callFindProc procSch procNam = findProc (QualifiedIdentifier procSch procNam) payloadColumns (preferParameters == Just SingleObject) (dbProcs dbStructure) contentMediaType (action == ActionInvoke InvPost) - in - case path of - PathInfo{pSchema, pName, pHasRpc, pIsRootSpec, pIsDefaultSpec} - | pHasRpc || pIsRootSpec -> (`TargetProc` pIsRootSpec) <$> callFindProc pSchema pName - | pIsDefaultSpec -> Right $ TargetDefaultSpec pSchema - | otherwise -> Right $ TargetIdent $ QualifiedIdentifier pSchema pName - PathUnknown -> Right TargetUnknown shouldParsePayload = case (action, contentMediaType) of (ActionMutate MutationCreate, _) -> True @@ -324,15 +326,6 @@ apiRequest conf@AppConfig{..} dbStructure req reqBody queryparams@QueryParams{.. (MTUrlEncoded, ActionInvoke InvPost) -> targetToJsonRpcParams (rightToMaybe target) $ (T.decodeUtf8 *** T.decodeUtf8) <$> parseSimpleQuery (LBS.toStrict reqBody) _ | shouldParsePayload -> rightToMaybe payload | otherwise -> Nothing - path = - case pathInfo req of - [] -> case configDbRootSpec of - Just (QualifiedIdentifier pSch pName) -> PathInfo (if pSch == mempty then schema else pSch) pName False False True - Nothing | configOpenApiMode == OADisabled -> PathUnknown - | otherwise -> PathInfo schema "" False True False - [table] -> PathInfo schema table False False False - ["rpc", pName] -> PathInfo schema pName True False False - _ -> PathUnknown method = requestMethod req hdrs = requestHeaders req lookupHeader = flip lookup hdrs @@ -409,7 +402,7 @@ payloadAttributes raw json = where emptyPJArray = ProcessedJSON (JSON.encode emptyArray) S.empty -findAcceptMediaType :: AppConfig -> Action -> Path -> [MediaType] -> Either ApiRequestError MediaType +findAcceptMediaType :: AppConfig -> Action -> PathInfo -> [MediaType] -> Either ApiRequestError MediaType findAcceptMediaType conf action path accepts = case mutuallyAgreeable (requestMediaTypes conf action path) accepts of Just ct -> @@ -417,7 +410,7 @@ findAcceptMediaType conf action path accepts = Nothing -> Left . MediaTypeError $ map MediaType.toMime accepts -requestMediaTypes :: AppConfig -> Action -> Path -> [MediaType] +requestMediaTypes :: AppConfig -> Action -> PathInfo -> [MediaType] requestMediaTypes conf action path = case action of ActionRead _ -> defaultMediaTypes ++ rawMediaTypes @@ -429,7 +422,7 @@ requestMediaTypes conf action path = invokeMediaTypes = defaultMediaTypes ++ rawMediaTypes - ++ [MTOpenAPI | pIsRootSpec path] + ++ [MTOpenAPI | pathIsRootSpec path] defaultMediaTypes = [MTApplicationJSON, MTSingularJSON, MTGeoJSON, MTTextCSV] rawMediaTypes = configRawMediaTypes conf `union` [MTOctetStream, MTTextPlain, MTTextXML] diff --git a/src/PostgREST/Request/Types.hs b/src/PostgREST/Request/Types.hs index 5b0f56877..6964baa98 100644 --- a/src/PostgREST/Request/Types.hs +++ b/src/PostgREST/Request/Types.hs @@ -55,6 +55,7 @@ data ApiRequestError | InvalidFilters | InvalidRange | LimitNoOrderError + | NotFound | NoRelBetween Text Text Text | NoRpc Text Text [Text] Bool MediaType Bool | NotEmbedded Text diff --git a/test/spec/Feature/OpenApi/DisabledOpenApiSpec.hs b/test/spec/Feature/OpenApi/DisabledOpenApiSpec.hs index 8925f7738..f962409d4 100644 --- a/test/spec/Feature/OpenApi/DisabledOpenApiSpec.hs +++ b/test/spec/Feature/OpenApi/DisabledOpenApiSpec.hs @@ -11,10 +11,6 @@ import Protolude spec :: SpecWith ((), Application) spec = describe "Disabled OpenApi" $ do - it "does not accept application/openapi+json and responds with 415" $ + it "responds with 404" $ request methodGet "/" - [("Accept","application/openapi+json")] "" `shouldRespondWith` 415 - - it "accepts application/json and responds with 404" $ - request methodGet "/" - [("Accept","application/json")] "" `shouldRespondWith` 404 + [("Accept","application/openapi+json")] "" `shouldRespondWith` 404 diff --git a/test/spec/Feature/Query/ErrorSpec.hs b/test/spec/Feature/Query/ErrorSpec.hs index 463af21e6..897c22666 100644 --- a/test/spec/Feature/Query/ErrorSpec.hs +++ b/test/spec/Feature/Query/ErrorSpec.hs @@ -18,6 +18,13 @@ spec = do it "gives 404 when requesting a nonexistent table in this nonexistent schema" $ get "/nonexistent_table" `shouldRespondWith` 404 + describe "Non existent URL" $ do + it "gives 404 on a single nested route" $ + get "/projects/nested" `shouldRespondWith` 404 + + it "gives 404 on a double nested route" $ + get "/projects/nested/double" `shouldRespondWith` 404 + describe "Unsupported HTTP methods" $ do it "should return 405 for CONNECT method" $ request methodConnect "/"