From 7f365bf60b8627dfc5f21d4d8d422d43d2a5c1a0 Mon Sep 17 00:00:00 2001 From: Dmitry Wagin Date: Sun, 8 Dec 2019 09:49:20 +0300 Subject: [PATCH] Enforce singular behavior despite Prefer: return (#1417) --- CHANGELOG.md | 1 + src/PostgREST/App.hs | 39 ++++++-------- test/Feature/SingularSpec.hs | 101 +++++++++++++++++++++++++++++------ 3 files changed, 101 insertions(+), 40 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2e2940fc7..81878c5a7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ This project adheres to [Semantic Versioning](http://semver.org/). ### Added +- #1417, `Accept: application/vnd.pgrst.object+json` behavior is now enforced for POST/PATCH/DELETE regardless of `Prefer: return=representation/minimal` - @dwagin - #1415, Add support for user defined socket permission via `server-unix-socket-mode` config option - @Dansvidania - #1383, Add support for HEAD request - @steve-chavez - #1378, Add support for `Prefer: count=planned` and `Prefer: count=estimated` on GET /table - @steve-chavez diff --git a/src/PostgREST/App.hs b/src/PostgREST/App.hs index baaabe51f..b9ca35544 100644 --- a/src/PostgREST/App.hs +++ b/src/PostgREST/App.hs @@ -175,7 +175,6 @@ app dbStructure proc cols conf apiRequest = ] if contentType == CTSingularJSON && queryTotal /= 1 - && iPreferRepresentation apiRequest == Full then do HT.condemn return . errorResponseFor . singularityError $ queryTotal @@ -193,28 +192,23 @@ app dbStructure proc cols conf apiRequest = (iPreferRepresentation apiRequest) [] row <- H.statement (toS $ pjRaw pJson) stm let (_, queryTotal, _, body) = row - - updateIsNoOp = S.null cols + updateIsNoOp = S.null cols contentRangeHeader = contentRangeH 0 (queryTotal - 1) $ - if shouldCount then Just queryTotal else Nothing - minimalHeaders = [contentRangeHeader] - fullHeaders = toHeader contentType : minimalHeaders - + if shouldCount then Just queryTotal else Nothing + headers | iPreferRepresentation apiRequest == Full = [toHeader contentType, contentRangeHeader] + | otherwise = [contentRangeHeader] status | queryTotal == 0 && not updateIsNoOp = status404 | iPreferRepresentation apiRequest == Full = status200 | otherwise = status204 - - case (contentType, iPreferRepresentation apiRequest) of - (CTSingularJSON, Full) - | queryTotal == 1 -> return $ responseLBS status fullHeaders (toS body) - | otherwise -> HT.condemn >> (return . errorResponseFor . singularityError) queryTotal - - (_, Full) -> - return $ responseLBS status fullHeaders (toS body) - - (_, _) -> - return $ responseLBS status minimalHeaders mempty - + if contentType == CTSingularJSON + && queryTotal /= 1 + then do + HT.condemn + return . errorResponseFor . singularityError $ queryTotal + else + return $ if iPreferRepresentation apiRequest == Full + then responseLBS status headers (toS body) + else responseLBS status headers mempty (ActionSingleUpsert, TargetIdent (QualifiedIdentifier tSchema tName), Just ProcessedJSON{pjRaw, pjType, pjKeys}) -> case mutateSqlParts tSchema tName of @@ -257,18 +251,17 @@ app dbStructure proc cols conf apiRequest = (iPreferRepresentation apiRequest) [] row <- H.statement mempty stm let (_, queryTotal, _, body) = row - r = contentRangeH 1 0 $ + contentRangeHeader = contentRangeH 1 0 $ if shouldCount then Just queryTotal else Nothing if contentType == CTSingularJSON && queryTotal /= 1 - && iPreferRepresentation apiRequest == Full then do HT.condemn return . errorResponseFor . singularityError $ queryTotal else return $ if iPreferRepresentation apiRequest == Full - then responseLBS status200 [toHeader contentType, r] (toS body) - else responseLBS status204 [r] "" + then responseLBS status200 [toHeader contentType, contentRangeHeader] (toS body) + else responseLBS status204 [contentRangeHeader] "" (ActionInfo, TargetIdent (QualifiedIdentifier tSchema tTable), Nothing) -> let mTable = find (\t -> tableName t == tTable && tableSchema t == tSchema) (dbTables dbStructure) in diff --git a/test/Feature/SingularSpec.hs b/test/Feature/SingularSpec.hs index 4e1633717..28d5de158 100644 --- a/test/Feature/SingularSpec.hs +++ b/test/Feature/SingularSpec.hs @@ -56,7 +56,7 @@ spec = _ <- post "/addresses" [json| { id: 98, address: "xxx" } |] _ <- post "/addresses" [json| { id: 99, address: "yyy" } |] p <- request methodPatch "/addresses?id=gt.0" - [("Prefer", "return=representation"), singular] + [singular] [json| { address: "zzz" } |] liftIO $ do simpleStatus p `shouldBe` notAcceptable406 @@ -65,7 +65,29 @@ spec = -- the rows should not be updated, either get "/addresses?id=eq.98" `shouldRespondWith` [str|[{"id":98,"address":"xxx"}]|] + it "raises an error for multiple rows with return=rep" $ do + _ <- post "/addresses" [json| { id: 100, address: "xxx" } |] + _ <- post "/addresses" [json| { id: 101, address: "yyy" } |] + p <- request methodPatch "/addresses?id=gt.0" + [("Prefer", "return=representation"), singular] + [json| { address: "zzz" } |] + liftIO $ do + simpleStatus p `shouldBe` notAcceptable406 + isErrorFormat (simpleBody p) `shouldBe` True + + -- the rows should not be updated, either + get "/addresses?id=eq.100" `shouldRespondWith` [str|[{"id":100,"address":"xxx"}]|] + it "raises an error for zero rows" $ + request methodPatch "/items?id=gt.0&id=lt.0" + [singular] [json|{"id":1}|] + `shouldRespondWith` + [str|{"details":"Results contain 0 rows, application/vnd.pgrst.object+json requires 1 row","message":"JSON object requested, multiple (or no) rows returned"}|] + { matchStatus = 406 + , matchHeaders = ["Content-Type" <:> "application/vnd.pgrst.object+json; charset=utf-8"] + } + + it "raises an error for zero rows with return=rep" $ request methodPatch "/items?id=gt.0&id=lt.0" [("Prefer", "return=representation"), singular] [json|{"id":1}|] `shouldRespondWith` @@ -79,20 +101,20 @@ spec = p <- request methodPost "/addresses" [("Prefer", "return=representation"), singular] - [json| [ { id: 100, address: "xxx" } ] |] - liftIO $ simpleBody p `shouldBe` [str|{"id":100,"address":"xxx"}|] + [json| [ { id: 102, address: "xxx" } ] |] + liftIO $ simpleBody p `shouldBe` [str|{"id":102,"address":"xxx"}|] it "works for one row even with return=minimal" $ do request methodPost "/addresses" [("Prefer", "return=minimal"), singular] - [json| [ { id: 101, address: "xxx" } ] |] + [json| [ { id: 103, address: "xxx" } ] |] `shouldRespondWith` "" { matchStatus = 201 , matchHeaders = ["Content-Range" <:> "*/*"] } -- and the element should exist - get "/addresses?id=eq.101" - `shouldRespondWith` [str|[{"id":101,"address":"xxx"}]|] + get "/addresses?id=eq.103" + `shouldRespondWith` [str|[{"id":103,"address":"xxx"}]|] { matchStatus = 200 , matchHeaders = [] } @@ -100,23 +122,47 @@ spec = it "raises an error when attempting to create multiple entities" $ do p <- request methodPost "/addresses" - [("Prefer", "return=representation"), singular] + [singular] [json| [ { id: 200, address: "xxx" }, { id: 201, address: "yyy" } ] |] liftIO $ simpleStatus p `shouldBe` notAcceptable406 -- the rows should not exist, either get "/addresses?id=eq.200" `shouldRespondWith` "[]" - it "return=minimal allows request to create multiple elements" $ + it "raises an error when attempting to create multiple entities with return=rep" $ do + p <- request methodPost + "/addresses" + [("Prefer", "return=representation"), singular] + [json| [ { id: 202, address: "xxx" }, { id: 203, address: "yyy" } ] |] + liftIO $ simpleStatus p `shouldBe` notAcceptable406 + + -- the rows should not exist, either + get "/addresses?id=eq.202" `shouldRespondWith` "[]" + + it "raises an error regardless of return=minimal" $ do request methodPost "/addresses" - [("Prefer", "return=minimal"), singular] - [json| [ { id: 200, address: "xxx" }, { id: 201, address: "yyy" } ] |] - `shouldRespondWith` "" - { matchStatus = 201 - , matchHeaders = ["Content-Range" <:> "*/*"] - } + [("Prefer", "return=minimal"), singular] + [json| [ { id: 204, address: "xxx" }, { id: 205, address: "yyy" } ] |] + `shouldRespondWith` + [str|{"details":"Results contain 2 rows, application/vnd.pgrst.object+json requires 1 row","message":"JSON object requested, multiple (or no) rows returned"}|] + { matchStatus = 406 + , matchHeaders = ["Content-Type" <:> "application/vnd.pgrst.object+json; charset=utf-8"] + } + + -- the rows should not exist, either + get "/addresses?id=eq.204" `shouldRespondWith` "[]" it "raises an error when creating zero entities" $ + request methodPost "/addresses" + [singular] + [json| [ ] |] + `shouldRespondWith` + [str|{"details":"Results contain 0 rows, application/vnd.pgrst.object+json requires 1 row","message":"JSON object requested, multiple (or no) rows returned"}|] + { matchStatus = 406 + , matchHeaders = ["Content-Type" <:> "application/vnd.pgrst.object+json; charset=utf-8"] + } + + it "raises an error when creating zero entities with return=rep" $ request methodPost "/addresses" [("Prefer", "return=representation"), singular] [json| [ ] |] @@ -134,18 +180,39 @@ spec = liftIO $ simpleBody p `shouldBe` [str|{"id":11}|] it "raises an error when attempting to delete multiple entities" $ do - let firstItems = "/items?id=gt.0&id=lt.11" + let firstItems = "/items?id=gt.0&id=lt.6" + request methodDelete firstItems + [singular] "" + `shouldRespondWith` 406 + + get firstItems + `shouldRespondWith` [json| [{"id":1},{"id":2},{"id":3},{"id":4},{"id":5}] |] + { matchStatus = 200 + , matchHeaders = ["Content-Range" <:> "0-4/*"] + } + + it "raises an error when attempting to delete multiple entities with return=rep" $ do + let firstItems = "/items?id=gt.5&id=lt.11" request methodDelete firstItems [("Prefer", "return=representation"), singular] "" `shouldRespondWith` 406 get firstItems - `shouldRespondWith` [json| [{"id":1},{"id":2},{"id":3},{"id":4},{"id":5},{"id":6},{"id":7},{"id":8},{"id":9},{"id":10}] |] + `shouldRespondWith` [json| [{"id":6},{"id":7},{"id":8},{"id":9},{"id":10}] |] { matchStatus = 200 - , matchHeaders = ["Content-Range" <:> "0-9/*"] + , matchHeaders = ["Content-Range" <:> "0-4/*"] } it "raises an error when deleting zero entities" $ + request methodDelete "/items?id=lt.0" + [singular] "" + `shouldRespondWith` + [str|{"details":"Results contain 0 rows, application/vnd.pgrst.object+json requires 1 row","message":"JSON object requested, multiple (or no) rows returned"}|] + { matchStatus = 406 + , matchHeaders = ["Content-Type" <:> "application/vnd.pgrst.object+json; charset=utf-8"] + } + + it "raises an error when deleting zero entities with return=rep" $ request methodDelete "/items?id=lt.0" [("Prefer", "return=representation"), singular] "" `shouldRespondWith`