fix: no empty tx on Not Found error

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
 

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", "*")

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
 

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,
+      { pathName       :: Text
-        pName          :: Text,
+      , pathIsProc     :: Bool
-        pHasRpc        :: Bool,
+      , pathIsDefSpec  :: Bool
-        pIsDefaultSpec :: Bool,
+      , pathIsRootSpec :: Bool
-        pIsRootSpec    :: 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=<value>`, 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 =
+userApiRequest conf dbStructure req reqBody = do
-  apiRequest conf dbStructure req reqBody =<< first QueryParamError (QueryParams.parse (rawQueryString req))
+  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
+getPathInfo :: AppConfig -> [Text] -> Either ApiRequestError PathInfo
-apiRequest conf@AppConfig{..} dbStructure req reqBody queryparams@QueryParams{..}
+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}
+      "HEAD"     | pathIsDefSpec -> ActionInspect{isHead=True}
-                 | isTargetingProc        -> ActionInvoke InvHead
+                 | pathIsProc    -> ActionInvoke InvHead
-                 | otherwise              -> ActionRead{isHead=True}
+                 | otherwise     -> ActionRead{isHead=True}
-      "GET"      | isTargetingDefaultSpec -> ActionInspect{isHead=False}
+      "GET"      | pathIsDefSpec -> ActionInspect{isHead=False}
-                 | isTargetingProc        -> ActionInvoke InvGet
+                 | pathIsProc    -> ActionInvoke InvGet
-                 | otherwise              -> ActionRead{isHead=False}
+                 | otherwise     -> ActionRead{isHead=False}
-      "POST"    -> if isTargetingProc
+      "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]

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

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
+        [("Accept","application/openapi+json")] "" `shouldRespondWith` 404
-
-    it "accepts application/json and responds with 404" $
-      request methodGet "/"
-        [("Accept","application/json")] "" `shouldRespondWith` 404

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 "/"