fix: no empty tx on bad HTTP method

CHANGELOG.md

@@ -54,7 +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
+ - #2364, "404 Not Found" on nested routes and "405 Method Not Allowed" errors no longer start an empty database transaction - @steve-chavez
 
 ### Changed
 

postgrest.cabal

@@ -2,7 +2,7 @@ name:               postgrest
 version:            9.0.1.20220630
 synopsis:           REST API for any Postgres database
 description:        Reads the schema of a PostgreSQL database and creates RESTful routes
-                    for tables, views, and functions, supporting all HTTP verbs that security
+                    for tables, views, and functions, supporting all HTTP methods that security
                     permits.
 license:            MIT
 license-file:       LICENSE

src/PostgREST/Error.hs

@@ -54,12 +54,12 @@ class (JSON.ToJSON a) => PgrstError a where
   errorResponseFor err = responseLBS (status err) (headers err) $ errorPayload err
 
 instance PgrstError ApiRequestError where
-  status ActionInappropriate     = HTTP.status405
   status AmbiguousRelBetween{}   = HTTP.status300
   status AmbiguousRpc{}          = HTTP.status300
   status MediaTypeError{}        = HTTP.status415
   status InvalidBody{}           = HTTP.status400
   status InvalidFilters          = HTTP.status405
+  status InvalidRpcMethod{}      = HTTP.status405
   status InvalidRange            = HTTP.status416
   status NotFound                = HTTP.status404
   status NoRelBetween{}          = HTTP.status400
@@ -69,6 +69,7 @@ instance PgrstError ApiRequestError where
   status PutRangeNotAllowedError = HTTP.status400
   status QueryParamError{}       = HTTP.status400
   status UnacceptableSchema{}    = HTTP.status406
+  status UnsupportedMethod{}     = HTTP.status405
   status LimitNoOrderError       = HTTP.status400
 
   headers _ = [MediaType.toContentType MTApplicationJSON]
@@ -79,9 +80,9 @@ instance JSON.ToJSON ApiRequestError where
     "message" .= message,
     "details" .= details,
     "hint"    .= JSON.Null]
-  toJSON ActionInappropriate = JSON.object [
+  toJSON (InvalidRpcMethod method) = JSON.object [
     "code"    .= ApiRequestErrorCode01,
-    "message" .= ("Bad Request" :: Text),
+    "message" .= ("Cannot use the " <> T.decodeUtf8 method <> " method on RPC"),
     "details" .= JSON.Null,
     "hint"    .= JSON.Null]
   toJSON (InvalidBody errorMessage) = JSON.object [
@@ -133,6 +134,12 @@ instance JSON.ToJSON ApiRequestError where
     "details" .= JSON.Null,
     "hint"    .= JSON.Null]
 
+  toJSON (UnsupportedMethod method) = JSON.object [
+    "code"    .= ApiRequestErrorCode17,
+    "message" .= ("Unsupported HTTP method: " <> T.decodeUtf8 method),
+    "details" .= JSON.Null,
+    "hint"    .= JSON.Null]
+
   toJSON (NoRelBetween parent child schema) = JSON.object [
     "code"    .= SchemaCacheErrorCode00,
     "message" .= ("Could not find a relationship between '" <> parent <> "' and '" <> child <> "' in the schema cache" :: Text),
@@ -317,7 +324,6 @@ data Error
   | PgErr PgError
   | PutMatchingPkError
   | SingularityError Integer
-  | UnsupportedVerb Text
 
 instance PgrstError Error where
   status (ApiRequestError err)   = status err
@@ -332,7 +338,6 @@ instance PgrstError Error where
   status (PgErr err)             = status err
   status PutMatchingPkError      = HTTP.status400
   status SingularityError{}      = HTTP.status406
-  status UnsupportedVerb{}       = HTTP.status405
 
   headers (ApiRequestError err)  = headers err
   headers (JwtTokenInvalid m)    = [MediaType.toContentType MTApplicationJSON, invalidTokenHeader m]
@@ -398,12 +403,6 @@ instance JSON.ToJSON Error where
     "details" .= T.unwords ["Results contain", show n, "rows,", T.decodeUtf8 (MediaType.toMime MTSingularJSON), "requires 1 row"],
     "hint"    .= JSON.Null]
 
-  toJSON (UnsupportedVerb verb) = JSON.object [
-    "code"    .= ApiRequestErrorCode17,
-    "message" .= ("Unsupported HTTP verb: " <> verb),
-    "details" .= JSON.Null,
-    "hint"    .= JSON.Null]
-
   toJSON (PgErr err) = JSON.toJSON err
   toJSON (ApiRequestError err) = JSON.toJSON err
 

src/PostgREST/Request/ApiRequest.hs

@@ -97,7 +97,6 @@ data Action
   | ActionInvoke InvokeMethod
   | ActionInfo
   | ActionInspect {isHead :: Bool}
-  | 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 PathInfo
@@ -152,7 +151,7 @@ targetToJsonRpcParams target params =
   if it is an action we are able to perform.
 -}
 data ApiRequest = ApiRequest {
-    iAction               :: Action                           -- ^ Similar but not identical to HTTP verb, e.g. Create/Invoke both POST
+    iAction               :: Action                           -- ^ Similar but not identical to HTTP method, e.g. Create/Invoke both POST
   , iRange                :: HM.HashMap Text NonnegRange      -- ^ Requested range of rows within response
   , iTopLevelRange        :: NonnegRange                      -- ^ Requested range of rows from the top level
   , iTarget               :: Target                           -- ^ The target, be it calling a proc or accessing a table
@@ -176,9 +175,10 @@ 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 = do
-  qPrms <- 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
+  act <- getAction pInfo $ requestMethod req
+  apiRequest conf dbStructure req reqBody qPrms pInfo act
 
 getPathInfo :: AppConfig -> [Text] -> Either ApiRequestError PathInfo
 getPathInfo AppConfig{configOpenApiMode, configDbRootSpec} path =
@@ -191,10 +191,30 @@ getPathInfo AppConfig{configOpenApiMode, configDbRootSpec} path =
     ["rpc", pName] -> Right $ PathInfo pName True False False
     _              -> Left NotFound
 
-apiRequest :: AppConfig -> DbStructure -> Request -> RequestBody -> QueryParams.QueryParams -> PathInfo -> Either ApiRequestError ApiRequest
+getAction :: PathInfo -> ByteString -> Either ApiRequestError Action
-apiRequest conf@AppConfig{..} dbStructure req reqBody queryparams@QueryParams{..} path@PathInfo{pathName, pathIsProc, pathIsRootSpec, pathIsDefSpec}
+getAction PathInfo{pathIsProc, pathIsDefSpec} method =
+  if pathIsProc && method `notElem` ["HEAD", "GET", "POST"]
+    then Left $ InvalidRpcMethod method
+    else 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"     | pathIsDefSpec -> Right $ ActionInspect{isHead=True}
+                 | pathIsProc    -> Right $ ActionInvoke InvHead
+                 | otherwise     -> Right $ ActionRead{isHead=True}
+      "GET"      | pathIsDefSpec -> Right $ ActionInspect{isHead=False}
+                 | pathIsProc    -> Right $ ActionInvoke InvGet
+                 | otherwise     -> Right $ ActionRead{isHead=False}
+      "POST"     | pathIsProc    -> Right $ ActionInvoke InvPost
+                 | otherwise     -> Right $ ActionMutate MutationCreate
+      "PATCH"                    -> Right $ ActionMutate MutationUpdate
+      "PUT"                      -> Right $ ActionMutate MutationSingleUpsert
+      "DELETE"                   -> Right $ ActionMutate MutationDelete
+      "OPTIONS"                  -> Right ActionInfo
+      _                          -> Left $ UnsupportedMethod method
+
+apiRequest :: AppConfig -> DbStructure -> Request -> RequestBody -> QueryParams.QueryParams -> PathInfo -> Action -> Either ApiRequestError ApiRequest
+apiRequest conf@AppConfig{..} dbStructure req reqBody queryparams@QueryParams{..} path@PathInfo{pathName, pathIsProc, pathIsRootSpec, pathIsDefSpec} action
   | isJust profile && fromJust profile `notElem` configDbSchemas = Left $ UnacceptableSchema $ toList configDbSchemas
-  | 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)
@@ -266,24 +286,6 @@ apiRequest conf@AppConfig{..} dbStructure req reqBody queryparams@QueryParams{..
     (MTOctetStream, True) -> Right $ RawPay reqBody
     (ct, _) -> Left $ "Content-Type not acceptable: " <> MediaType.toMime ct
   topLevelRange = fromMaybe allRange $ HM.lookup "limit" ranges -- if no limit is specified, get all the request rows
-  action =
-    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"     | 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
-      "PUT"     -> ActionMutate MutationSingleUpsert
-      "DELETE"  -> ActionMutate MutationDelete
-      "OPTIONS" -> ActionInfo
-      _         -> ActionUnknown $ T.decodeUtf8 method
 
   defaultSchema = NonEmptyList.head configDbSchemas
   profile
@@ -305,7 +307,7 @@ apiRequest conf@AppConfig{..} dbStructure req reqBody queryparams@QueryParams{..
   target
     | pathIsProc    = (`TargetProc` pathIsRootSpec) <$> callFindProc schema pathName
     | pathIsDefSpec = Right $ TargetDefaultSpec schema
-    | otherwise      = Right $ TargetIdent $ QualifiedIdentifier schema pathName
+    | otherwise     = Right $ TargetIdent $ QualifiedIdentifier schema pathName
     where
       callFindProc procSch procNam = findProc
         (QualifiedIdentifier procSch procNam) payloadColumns (preferParameters == Just SingleObject) (dbProcs dbStructure)

src/PostgREST/Request/Types.hs

@@ -47,13 +47,13 @@ import Protolude
 
 
 data ApiRequestError
-  = ActionInappropriate
+  = AmbiguousRelBetween Text Text [Relationship]
-  | AmbiguousRelBetween Text Text [Relationship]
   | AmbiguousRpc [ProcDescription]
   | MediaTypeError [ByteString]
   | InvalidBody ByteString
   | InvalidFilters
   | InvalidRange
+  | InvalidRpcMethod ByteString
   | LimitNoOrderError
   | NotFound
   | NoRelBetween Text Text Text
@@ -63,6 +63,7 @@ data ApiRequestError
   | PutRangeNotAllowedError
   | QueryParamError QPError
   | UnacceptableSchema [Text]
+  | UnsupportedMethod ByteString
 
 data QPError = QPError Text Text
 

test/spec/Feature/OptionsSpec.hs

@@ -16,7 +16,7 @@ import SpecHelper
 spec :: PgVersion -> SpecWith ((), Application)
 spec actualPgVersion = describe "Allow header" $ do
   context "a table" $ do
-    it "includes read/write verbs for writeable table" $ do
+    it "includes read/write methods for writeable table" $ do
       r <- request methodOptions "/items" [] ""
       liftIO $
         simpleHeaders r `shouldSatisfy`
@@ -24,7 +24,7 @@ spec actualPgVersion = describe "Allow header" $ do
 
   when (actualPgVersion >= pgVersion100) $
     context "a partitioned table" $ do
-      it "includes read/write verbs for writeable partitioned tables" $ do
+      it "includes read/write methods for writeable partitioned tables" $ do
         r <- request methodOptions "/car_models" [] ""
         liftIO $
           simpleHeaders r `shouldSatisfy`
@@ -37,50 +37,50 @@ spec actualPgVersion = describe "Allow header" $ do
 
   context "a view" $ do
     context "auto updatable" $ do
-      it "includes read/write verbs for auto updatable views with pk" $ do
+      it "includes read/write methods for auto updatable views with pk" $ do
         r <- request methodOptions "/projects_auto_updatable_view_with_pk" [] ""
         liftIO $
           simpleHeaders r `shouldSatisfy`
             matchHeader "Allow" "OPTIONS,GET,HEAD,POST,PUT,PATCH,DELETE"
 
-      it "includes read/write verbs for auto updatable views without pk" $ do
+      it "includes read/write methods for auto updatable views without pk" $ do
         r <- request methodOptions "/projects_auto_updatable_view_without_pk" [] ""
         liftIO $
           simpleHeaders r `shouldSatisfy`
             matchHeader "Allow" "OPTIONS,GET,HEAD,POST,PATCH,DELETE"
 
     context "non auto updatable" $ do
-      it "includes read verbs for non auto updatable views" $ do
+      it "includes read methods for non auto updatable views" $ do
         r <- request methodOptions "/projects_view_without_triggers" [] ""
         liftIO $
           simpleHeaders r `shouldSatisfy`
             matchHeader "Allow" "OPTIONS,GET,HEAD"
 
-      it "includes read/write verbs for insertable, updatable and deletable views with pk" $ do
+      it "includes read/write methods for insertable, updatable and deletable views with pk" $ do
         r <- request methodOptions "/projects_view_with_all_triggers_with_pk" [] ""
         liftIO $
           simpleHeaders r `shouldSatisfy`
             matchHeader "Allow" "OPTIONS,GET,HEAD,POST,PUT,PATCH,DELETE"
 
-      it "includes read/write verbs for insertable, updatable and deletable views without pk" $ do
+      it "includes read/write methods for insertable, updatable and deletable views without pk" $ do
         r <- request methodOptions "/projects_view_with_all_triggers_without_pk" [] ""
         liftIO $
           simpleHeaders r `shouldSatisfy`
             matchHeader "Allow" "OPTIONS,GET,HEAD,POST,PATCH,DELETE"
 
-      it "includes read and insert verbs for insertable views" $ do
+      it "includes read and insert methods for insertable views" $ do
         r <- request methodOptions "/projects_view_with_insert_trigger" [] ""
         liftIO $
           simpleHeaders r `shouldSatisfy`
             matchHeader "Allow" "OPTIONS,GET,HEAD,POST"
 
-      it "includes read and update verbs for updatable views" $ do
+      it "includes read and update methods for updatable views" $ do
         r <- request methodOptions "/projects_view_with_update_trigger" [] ""
         liftIO $
           simpleHeaders r `shouldSatisfy`
             matchHeader "Allow" "OPTIONS,GET,HEAD,PATCH"
 
-      it "includes read and delete verbs for deletable views" $ do
+      it "includes read and delete methods for deletable views" $ do
         r <- request methodOptions "/projects_view_with_delete_trigger" [] ""
         liftIO $
           simpleHeaders r `shouldSatisfy`

test/spec/Feature/Query/ErrorSpec.hs

@@ -35,7 +35,7 @@ spec = do
             {"hint": null,
              "details": null,
              "code": "PGRST117",
-             "message":"Unsupported HTTP verb: CONNECT"}|]
+             "message":"Unsupported HTTP method: CONNECT"}|]
           { matchStatus = 405 }
 
     it "should return 405 for TRACE method" $
@@ -47,7 +47,7 @@ spec = do
             {"hint": null,
              "details": null,
              "code": "PGRST117",
-             "message":"Unsupported HTTP verb: TRACE"}|]
+             "message":"Unsupported HTTP method: TRACE"}|]
           { matchStatus = 405 }
 
     it "should return 405 for OTHER method" $
@@ -59,5 +59,5 @@ spec = do
             {"hint": null,
              "details": null,
              "code": "PGRST117",
-             "message":"Unsupported HTTP verb: OTHER"}|]
+             "message":"Unsupported HTTP method: OTHER"}|]
           { matchStatus = 405 }

test/spec/Feature/Query/RpcSpec.hs

@@ -516,11 +516,11 @@ spec actualPgVersion =
           simpleStatus p `shouldBe` internalServerError500
           isErrorFormat (simpleBody p) `shouldBe` True
 
-    context "unsupported verbs" $ do
+    context "unsupported method" $ do
       it "DELETE fails" $
         request methodDelete "/rpc/sayhello" [] ""
           `shouldRespondWith`
-          [json|{"message":"Bad Request","code":"PGRST101","details":null,"hint":null}|]
+          [json|{"message":"Cannot use the DELETE method on RPC","code":"PGRST101","details":null,"hint":null}|]
           { matchStatus  = 405
           , matchHeaders = [matchContentTypeJson]
           }