Skip to content

Commit 8230128

Browse files
steve-chavezsteve-chavez
committed
fix: no empty tx on bad HTTP method
1 parent 6d7bf9f commit 8230128

File tree

8 files changed

+58
-56
lines changed

8 files changed

+58
-56
lines changed

Diff for: ‎CHANGELOG.md

+1-1
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,7 @@ This project adheres to [Semantic Versioning](http://semver.org/).
5454
- #1959, An accidental full table PATCH(without filters) is not possible anymore, it requires filters or a `limit` parameter - @steve-chavez, @laurenceisla
5555
- #2317, Increase the `db-pool-timeout` to 1 hour to prevent frequent high connection latency - @steve-chavez
5656
- #2341, The search path now correctly identifies schemas with uppercase and special characters in their names (regression) - @laurenceisla
57-
- #2364, "404 Not Found" on nested routes doesn't start an empty database transaction - @steve-chavez
57+
- #2364, "404 Not Found" on nested routes and "405 Method Not Allowed" errors no longer start an empty database transaction - @steve-chavez
5858

5959
### Changed
6060

Diff for: ‎postgrest.cabal

+1-1
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ name: postgrest
22
version: 9.0.1.20220630
33
synopsis: REST API for any Postgres database
44
description: Reads the schema of a PostgreSQL database and creates RESTful routes
5-
for tables, views, and functions, supporting all HTTP verbs that security
5+
for tables, views, and functions, supporting all HTTP methods that security
66
permits.
77
license: MIT
88
license-file: LICENSE

Diff for: ‎src/PostgREST/Error.hs

+10-11
Original file line numberDiff line numberDiff line change
@@ -54,12 +54,12 @@ class (JSON.ToJSON a) => PgrstError a where
5454
errorResponseFor err = responseLBS (status err) (headers err) $ errorPayload err
5555

5656
instance PgrstError ApiRequestError where
57-
status ActionInappropriate = HTTP.status405
5857
status AmbiguousRelBetween{} = HTTP.status300
5958
status AmbiguousRpc{} = HTTP.status300
6059
status MediaTypeError{} = HTTP.status415
6160
status InvalidBody{} = HTTP.status400
6261
status InvalidFilters = HTTP.status405
62+
status InvalidRpcMethod{} = HTTP.status405
6363
status InvalidRange = HTTP.status416
6464
status NotFound = HTTP.status404
6565
status NoRelBetween{} = HTTP.status400
@@ -69,6 +69,7 @@ instance PgrstError ApiRequestError where
6969
status PutRangeNotAllowedError = HTTP.status400
7070
status QueryParamError{} = HTTP.status400
7171
status UnacceptableSchema{} = HTTP.status406
72+
status UnsupportedMethod{} = HTTP.status405
7273
status LimitNoOrderError = HTTP.status400
7374

7475
headers _ = [MediaType.toContentType MTApplicationJSON]
@@ -79,9 +80,9 @@ instance JSON.ToJSON ApiRequestError where
7980
"message" .= message,
8081
"details" .= details,
8182
"hint" .= JSON.Null]
82-
toJSON ActionInappropriate = JSON.object [
83+
toJSON (InvalidRpcMethod method) = JSON.object [
8384
"code" .= ApiRequestErrorCode01,
84-
"message" .= ("Bad Request" :: Text),
85+
"message" .= ("Cannot use the " <> T.decodeUtf8 method <> " method on RPC"),
8586
"details" .= JSON.Null,
8687
"hint" .= JSON.Null]
8788
toJSON (InvalidBody errorMessage) = JSON.object [
@@ -133,6 +134,12 @@ instance JSON.ToJSON ApiRequestError where
133134
"details" .= JSON.Null,
134135
"hint" .= JSON.Null]
135136

137+
toJSON (UnsupportedMethod method) = JSON.object [
138+
"code" .= ApiRequestErrorCode17,
139+
"message" .= ("Unsupported HTTP method: " <> T.decodeUtf8 method),
140+
"details" .= JSON.Null,
141+
"hint" .= JSON.Null]
142+
136143
toJSON (NoRelBetween parent child schema) = JSON.object [
137144
"code" .= SchemaCacheErrorCode00,
138145
"message" .= ("Could not find a relationship between '" <> parent <> "' and '" <> child <> "' in the schema cache" :: Text),
@@ -317,7 +324,6 @@ data Error
317324
| PgErr PgError
318325
| PutMatchingPkError
319326
| SingularityError Integer
320-
| UnsupportedVerb Text
321327

322328
instance PgrstError Error where
323329
status (ApiRequestError err) = status err
@@ -332,7 +338,6 @@ instance PgrstError Error where
332338
status (PgErr err) = status err
333339
status PutMatchingPkError = HTTP.status400
334340
status SingularityError{} = HTTP.status406
335-
status UnsupportedVerb{} = HTTP.status405
336341

337342
headers (ApiRequestError err) = headers err
338343
headers (JwtTokenInvalid m) = [MediaType.toContentType MTApplicationJSON, invalidTokenHeader m]
@@ -398,12 +403,6 @@ instance JSON.ToJSON Error where
398403
"details" .= T.unwords ["Results contain", show n, "rows,", T.decodeUtf8 (MediaType.toMime MTSingularJSON), "requires 1 row"],
399404
"hint" .= JSON.Null]
400405

401-
toJSON (UnsupportedVerb verb) = JSON.object [
402-
"code" .= ApiRequestErrorCode17,
403-
"message" .= ("Unsupported HTTP verb: " <> verb),
404-
"details" .= JSON.Null,
405-
"hint" .= JSON.Null]
406-
407406
toJSON (PgErr err) = JSON.toJSON err
408407
toJSON (ApiRequestError err) = JSON.toJSON err
409408

Diff for: ‎src/PostgREST/Request/ApiRequest.hs

+28-26
Original file line numberDiff line numberDiff line change
@@ -97,7 +97,6 @@ data Action
9797
| ActionInvoke InvokeMethod
9898
| ActionInfo
9999
| ActionInspect {isHead :: Bool}
100-
| ActionUnknown Text
101100
deriving Eq
102101
-- | The path info that will be mapped to a target (used to handle validations and errors before defining the Target)
103102
data PathInfo
@@ -152,7 +151,7 @@ targetToJsonRpcParams target params =
152151
if it is an action we are able to perform.
153152
-}
154153
data ApiRequest = ApiRequest {
155-
iAction :: Action -- ^ Similar but not identical to HTTP verb, e.g. Create/Invoke both POST
154+
iAction :: Action -- ^ Similar but not identical to HTTP method, e.g. Create/Invoke both POST
156155
, iRange :: HM.HashMap Text NonnegRange -- ^ Requested range of rows within response
157156
, iTopLevelRange :: NonnegRange -- ^ Requested range of rows from the top level
158157
, iTarget :: Target -- ^ The target, be it calling a proc or accessing a table
@@ -176,9 +175,10 @@ data ApiRequest = ApiRequest {
176175
-- | Examines HTTP request and translates it into user intent.
177176
userApiRequest :: AppConfig -> DbStructure -> Request -> RequestBody -> Either ApiRequestError ApiRequest
178177
userApiRequest conf dbStructure req reqBody = do
179-
qPrms <- first QueryParamError (QueryParams.parse (rawQueryString req))
178+
qPrms <- first QueryParamError $ QueryParams.parse $ rawQueryString req
180179
pInfo <- getPathInfo conf $ pathInfo req
181-
apiRequest conf dbStructure req reqBody qPrms pInfo
180+
act <- getAction pInfo $ requestMethod req
181+
apiRequest conf dbStructure req reqBody qPrms pInfo act
182182

183183
getPathInfo :: AppConfig -> [Text] -> Either ApiRequestError PathInfo
184184
getPathInfo AppConfig{configOpenApiMode, configDbRootSpec} path =
@@ -191,10 +191,30 @@ getPathInfo AppConfig{configOpenApiMode, configDbRootSpec} path =
191191
["rpc", pName] -> Right $ PathInfo pName True False False
192192
_ -> Left NotFound
193193

194-
apiRequest :: AppConfig -> DbStructure -> Request -> RequestBody -> QueryParams.QueryParams -> PathInfo -> Either ApiRequestError ApiRequest
195-
apiRequest conf@AppConfig{..} dbStructure req reqBody queryparams@QueryParams{..} path@PathInfo{pathName, pathIsProc, pathIsRootSpec, pathIsDefSpec}
194+
getAction :: PathInfo -> ByteString -> Either ApiRequestError Action
195+
getAction PathInfo{pathIsProc, pathIsDefSpec} method =
196+
if pathIsProc && method `notElem` ["HEAD", "GET", "POST"]
197+
then Left $ InvalidRpcMethod method
198+
else case method of
199+
-- The HEAD method is identical to GET except that the server MUST NOT return a message-body in the response
200+
-- From https://www.w3.org/Protocols/rfc2616/rfc2616-sec9.html#sec9.4
201+
"HEAD" | pathIsDefSpec -> Right $ ActionInspect{isHead=True}
202+
| pathIsProc -> Right $ ActionInvoke InvHead
203+
| otherwise -> Right $ ActionRead{isHead=True}
204+
"GET" | pathIsDefSpec -> Right $ ActionInspect{isHead=False}
205+
| pathIsProc -> Right $ ActionInvoke InvGet
206+
| otherwise -> Right $ ActionRead{isHead=False}
207+
"POST" | pathIsProc -> Right $ ActionInvoke InvPost
208+
| otherwise -> Right $ ActionMutate MutationCreate
209+
"PATCH" -> Right $ ActionMutate MutationUpdate
210+
"PUT" -> Right $ ActionMutate MutationSingleUpsert
211+
"DELETE" -> Right $ ActionMutate MutationDelete
212+
"OPTIONS" -> Right ActionInfo
213+
_ -> Left $ UnsupportedMethod method
214+
215+
apiRequest :: AppConfig -> DbStructure -> Request -> RequestBody -> QueryParams.QueryParams -> PathInfo -> Action -> Either ApiRequestError ApiRequest
216+
apiRequest conf@AppConfig{..} dbStructure req reqBody queryparams@QueryParams{..} path@PathInfo{pathName, pathIsProc, pathIsRootSpec, pathIsDefSpec} action
196217
| isJust profile && fromJust profile `notElem` configDbSchemas = Left $ UnacceptableSchema $ toList configDbSchemas
197-
| pathIsProc && method `notElem` ["HEAD", "GET", "POST"] = Left ActionInappropriate
198218
| isInvalidRange = Left InvalidRange
199219
| shouldParsePayload && isLeft payload = either (Left . InvalidBody) witness payload
200220
| 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{..
266286
(MTOctetStream, True) -> Right $ RawPay reqBody
267287
(ct, _) -> Left $ "Content-Type not acceptable: " <> MediaType.toMime ct
268288
topLevelRange = fromMaybe allRange $ HM.lookup "limit" ranges -- if no limit is specified, get all the request rows
269-
action =
270-
case method of
271-
-- The HEAD method is identical to GET except that the server MUST NOT return a message-body in the response
272-
-- From https://www.w3.org/Protocols/rfc2616/rfc2616-sec9.html#sec9.4
273-
"HEAD" | pathIsDefSpec -> ActionInspect{isHead=True}
274-
| pathIsProc -> ActionInvoke InvHead
275-
| otherwise -> ActionRead{isHead=True}
276-
"GET" | pathIsDefSpec -> ActionInspect{isHead=False}
277-
| pathIsProc -> ActionInvoke InvGet
278-
| otherwise -> ActionRead{isHead=False}
279-
"POST" -> if pathIsProc
280-
then ActionInvoke InvPost
281-
else ActionMutate MutationCreate
282-
"PATCH" -> ActionMutate MutationUpdate
283-
"PUT" -> ActionMutate MutationSingleUpsert
284-
"DELETE" -> ActionMutate MutationDelete
285-
"OPTIONS" -> ActionInfo
286-
_ -> ActionUnknown $ T.decodeUtf8 method
287289

288290
defaultSchema = NonEmptyList.head configDbSchemas
289291
profile
@@ -305,7 +307,7 @@ apiRequest conf@AppConfig{..} dbStructure req reqBody queryparams@QueryParams{..
305307
target
306308
| pathIsProc = (`TargetProc` pathIsRootSpec) <$> callFindProc schema pathName
307309
| pathIsDefSpec = Right $ TargetDefaultSpec schema
308-
| otherwise = Right $ TargetIdent $ QualifiedIdentifier schema pathName
310+
| otherwise = Right $ TargetIdent $ QualifiedIdentifier schema pathName
309311
where
310312
callFindProc procSch procNam = findProc
311313
(QualifiedIdentifier procSch procNam) payloadColumns (preferParameters == Just SingleObject) (dbProcs dbStructure)

Diff for: ‎src/PostgREST/Request/Types.hs

+3-2
Original file line numberDiff line numberDiff line change
@@ -47,13 +47,13 @@ import Protolude
4747

4848

4949
data ApiRequestError
50-
= ActionInappropriate
51-
| AmbiguousRelBetween Text Text [Relationship]
50+
= AmbiguousRelBetween Text Text [Relationship]
5251
| AmbiguousRpc [ProcDescription]
5352
| MediaTypeError [ByteString]
5453
| InvalidBody ByteString
5554
| InvalidFilters
5655
| InvalidRange
56+
| InvalidRpcMethod ByteString
5757
| LimitNoOrderError
5858
| NotFound
5959
| NoRelBetween Text Text Text
@@ -63,6 +63,7 @@ data ApiRequestError
6363
| PutRangeNotAllowedError
6464
| QueryParamError QPError
6565
| UnacceptableSchema [Text]
66+
| UnsupportedMethod ByteString
6667

6768
data QPError = QPError Text Text
6869

Diff for: ‎test/spec/Feature/OptionsSpec.hs

+10-10
Original file line numberDiff line numberDiff line change
@@ -16,15 +16,15 @@ import SpecHelper
1616
spec :: PgVersion -> SpecWith ((), Application)
1717
spec actualPgVersion = describe "Allow header" $ do
1818
context "a table" $ do
19-
it "includes read/write verbs for writeable table" $ do
19+
it "includes read/write methods for writeable table" $ do
2020
r <- request methodOptions "/items" [] ""
2121
liftIO $
2222
simpleHeaders r `shouldSatisfy`
2323
matchHeader "Allow" "OPTIONS,GET,HEAD,POST,PUT,PATCH,DELETE"
2424

2525
when (actualPgVersion >= pgVersion100) $
2626
context "a partitioned table" $ do
27-
it "includes read/write verbs for writeable partitioned tables" $ do
27+
it "includes read/write methods for writeable partitioned tables" $ do
2828
r <- request methodOptions "/car_models" [] ""
2929
liftIO $
3030
simpleHeaders r `shouldSatisfy`
@@ -37,50 +37,50 @@ spec actualPgVersion = describe "Allow header" $ do
3737

3838
context "a view" $ do
3939
context "auto updatable" $ do
40-
it "includes read/write verbs for auto updatable views with pk" $ do
40+
it "includes read/write methods for auto updatable views with pk" $ do
4141
r <- request methodOptions "/projects_auto_updatable_view_with_pk" [] ""
4242
liftIO $
4343
simpleHeaders r `shouldSatisfy`
4444
matchHeader "Allow" "OPTIONS,GET,HEAD,POST,PUT,PATCH,DELETE"
4545

46-
it "includes read/write verbs for auto updatable views without pk" $ do
46+
it "includes read/write methods for auto updatable views without pk" $ do
4747
r <- request methodOptions "/projects_auto_updatable_view_without_pk" [] ""
4848
liftIO $
4949
simpleHeaders r `shouldSatisfy`
5050
matchHeader "Allow" "OPTIONS,GET,HEAD,POST,PATCH,DELETE"
5151

5252
context "non auto updatable" $ do
53-
it "includes read verbs for non auto updatable views" $ do
53+
it "includes read methods for non auto updatable views" $ do
5454
r <- request methodOptions "/projects_view_without_triggers" [] ""
5555
liftIO $
5656
simpleHeaders r `shouldSatisfy`
5757
matchHeader "Allow" "OPTIONS,GET,HEAD"
5858

59-
it "includes read/write verbs for insertable, updatable and deletable views with pk" $ do
59+
it "includes read/write methods for insertable, updatable and deletable views with pk" $ do
6060
r <- request methodOptions "/projects_view_with_all_triggers_with_pk" [] ""
6161
liftIO $
6262
simpleHeaders r `shouldSatisfy`
6363
matchHeader "Allow" "OPTIONS,GET,HEAD,POST,PUT,PATCH,DELETE"
6464

65-
it "includes read/write verbs for insertable, updatable and deletable views without pk" $ do
65+
it "includes read/write methods for insertable, updatable and deletable views without pk" $ do
6666
r <- request methodOptions "/projects_view_with_all_triggers_without_pk" [] ""
6767
liftIO $
6868
simpleHeaders r `shouldSatisfy`
6969
matchHeader "Allow" "OPTIONS,GET,HEAD,POST,PATCH,DELETE"
7070

71-
it "includes read and insert verbs for insertable views" $ do
71+
it "includes read and insert methods for insertable views" $ do
7272
r <- request methodOptions "/projects_view_with_insert_trigger" [] ""
7373
liftIO $
7474
simpleHeaders r `shouldSatisfy`
7575
matchHeader "Allow" "OPTIONS,GET,HEAD,POST"
7676

77-
it "includes read and update verbs for updatable views" $ do
77+
it "includes read and update methods for updatable views" $ do
7878
r <- request methodOptions "/projects_view_with_update_trigger" [] ""
7979
liftIO $
8080
simpleHeaders r `shouldSatisfy`
8181
matchHeader "Allow" "OPTIONS,GET,HEAD,PATCH"
8282

83-
it "includes read and delete verbs for deletable views" $ do
83+
it "includes read and delete methods for deletable views" $ do
8484
r <- request methodOptions "/projects_view_with_delete_trigger" [] ""
8585
liftIO $
8686
simpleHeaders r `shouldSatisfy`

Diff for: ‎test/spec/Feature/Query/ErrorSpec.hs

+3-3
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ spec = do
3535
{"hint": null,
3636
"details": null,
3737
"code": "PGRST117",
38-
"message":"Unsupported HTTP verb: CONNECT"}|]
38+
"message":"Unsupported HTTP method: CONNECT"}|]
3939
{ matchStatus = 405 }
4040

4141
it "should return 405 for TRACE method" $
@@ -47,7 +47,7 @@ spec = do
4747
{"hint": null,
4848
"details": null,
4949
"code": "PGRST117",
50-
"message":"Unsupported HTTP verb: TRACE"}|]
50+
"message":"Unsupported HTTP method: TRACE"}|]
5151
{ matchStatus = 405 }
5252

5353
it "should return 405 for OTHER method" $
@@ -59,5 +59,5 @@ spec = do
5959
{"hint": null,
6060
"details": null,
6161
"code": "PGRST117",
62-
"message":"Unsupported HTTP verb: OTHER"}|]
62+
"message":"Unsupported HTTP method: OTHER"}|]
6363
{ matchStatus = 405 }

Diff for: ‎test/spec/Feature/Query/RpcSpec.hs

+2-2
Original file line numberDiff line numberDiff line change
@@ -516,11 +516,11 @@ spec actualPgVersion =
516516
simpleStatus p `shouldBe` internalServerError500
517517
isErrorFormat (simpleBody p) `shouldBe` True
518518

519-
context "unsupported verbs" $ do
519+
context "unsupported method" $ do
520520
it "DELETE fails" $
521521
request methodDelete "/rpc/sayhello" [] ""
522522
`shouldRespondWith`
523-
[json|{"message":"Bad Request","code":"PGRST101","details":null,"hint":null}|]
523+
[json|{"message":"Cannot use the DELETE method on RPC","code":"PGRST101","details":null,"hint":null}|]
524524
{ matchStatus = 405
525525
, matchHeaders = [matchContentTypeJson]
526526
}

0 commit comments

Comments
 (0)