jimmy/postgrest
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Fix wrong status 404 when PATCH request didn't change anything (#1272)

Browse Source
This commit is contained in:
Xavier Francisco
2019-04-19 13:50:19 -05:00
committed by Steve Chávez
parent 87f7e86aa7
commit 553531711b
5 changed files with 117 additions and 80 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/PostgREST/App.hs
+28 -20
View File
@@ -162,8 +162,8 @@ app dbStructure proc conf apiRequest =
, if iPreferRepresentation apiRequest == Full
then Just $ toHeader contentType
else Nothing
, Just . contentRangeH 1 0 $
toInteger <$> if shouldCount then Just queryTotal else Nothing
, Just $ contentRangeH 1 0 $
if shouldCount then Just queryTotal else Nothing
, if null pkCols
then Nothing
else (\x -> ("Preference-Applied", show x)) <$> iPreferResolution apiRequest
@@ -188,21 +188,28 @@ app dbStructure proc conf apiRequest =
(iPreferRepresentation apiRequest) []
row <- H.statement (toS $ pjRaw pJson) stm
let (_, queryTotal, _, body) = extractQueryResult row
if contentType == CTSingularJSON
&& queryTotal /= 1
&& iPreferRepresentation apiRequest == Full
then do
HT.condemn
return $ singularityError (toInteger queryTotal)
else do
let r = contentRangeH 0 (toInteger $ queryTotal-1)
(toInteger <$> if shouldCount then Just queryTotal else Nothing)
s = if iPreferRepresentation apiRequest == Full
then status200
else status204
return $ if iPreferRepresentation apiRequest == Full
then responseLBS s [toHeader contentType, r] (toS body)
else responseLBS s [r] ""
updateIsNoOp = S.null $ pjKeys pJson
contentRangeHeader = contentRangeH 0 (queryTotal - 1) $
if shouldCount then Just queryTotal else Nothing
minimalHeaders = [contentRangeHeader]
fullHeaders = toHeader contentType : minimalHeaders
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 (singularityError queryTotal)
(_, Full) ->
return $ responseLBS status fullHeaders (toS body)
(_, _) ->
return $ responseLBS status minimalHeaders mempty
(ActionSingleUpsert, TargetIdent (QualifiedIdentifier tSchema tName), Just ProcessedJSON{pjRaw, pjType, pjKeys}) ->
case mutateSqlParts tSchema tName of
@@ -246,7 +253,7 @@ app dbStructure proc conf apiRequest =
row <- H.statement mempty stm
let (_, queryTotal, _, body) = extractQueryResult row
r = contentRangeH 1 0 $
toInteger <$> if shouldCount then Just queryTotal else Nothing
if shouldCount then Just queryTotal else Nothing
if contentType == CTSingularJSON
&& queryTotal /= 1
&& iPreferRepresentation apiRequest == Full
@@ -332,9 +339,10 @@ app dbStructure proc conf apiRequest =
selectQuery = requestToQuery schema False <$> readDbRequest
countQuery = requestToCountQuery schema <$> readDbRequest
readSqlParts = (,) <$> selectQuery <*> countQuery
mutationDbRequest s t = mutateRequest apiRequest t (tablePKCols dbStructure s t) =<< fldNames
mutateSqlParts s t =
(,) <$> selectQuery
<*> (requestToQuery schema False . DbMutate <$> (mutateRequest apiRequest t (tablePKCols dbStructure s t) =<< fldNames))
<*> (requestToQuery schema False . DbMutate <$> mutationDbRequest s t)
responseContentTypeOrError :: [ContentType] -> Action -> Either Response ContentType
responseContentTypeOrError accepts action = serves contentTypesForRequest accepts
@@ -381,7 +389,7 @@ rangeStatus lower upper (Just total)
| (1 + upper - lower) < total = status206
| otherwise = status200
contentRangeH :: Integer -> Integer -> Maybe Integer -> Header
contentRangeH :: (Integral a, Show a) => a -> a -> Maybe a -> Header
contentRangeH lower upper total =
("Content-Range", headerValue)
where
src/PostgREST/Error.hs
+1 -1
View File
@@ -64,7 +64,7 @@ pgError authed e =
else [jsonType] in
responseLBS status hdrs (encodeError e)
singularityError :: Integer -> Response
singularityError :: (Integral a, Show a) => a -> Response
singularityError numRows =
responseLBS HT.status406
[toHeader CTSingularJSON]
test/Feature/InsertSpec.hs
+31 -15
View File
@@ -385,18 +385,18 @@ spec actualPgVersion = do
describe "Patching record" $ do
context "to unknown uri" $
it "gives a 404" $
it "indicates no table found by returning 404" $
request methodPatch "/fake" []
[json| { "real": false } |]
`shouldRespondWith` 404
context "on an empty table" $
it "indicates no records found to update" $
it "indicates no records found to update by returning 404" $
request methodPatch "/empty_table" []
[json| { "extra":20 } |]
`shouldRespondWith` ""
{ matchStatus = 204,
matchHeaders = ["Content-Range" <:> "*/*"]
{ matchStatus = 404,
matchHeaders = []
}
context "in a nonempty table" $ do
@@ -423,10 +423,15 @@ spec actualPgVersion = do
[("Prefer", "return=representation")] [json| { "id":999999 } |]
`shouldRespondWith` "[]"
{
matchStatus = 200,
matchHeaders = ["Content-Range" <:> "*/*"]
matchStatus = 404,
matchHeaders = []
}
it "gives a 404 when no rows updated" $
request methodPatch "/items?id=eq.99999999" []
[json| { "id": 42 } |]
`shouldRespondWith` 404
it "returns updated object as array when return=rep" $
request methodPatch "/items?id=eq.2"
[("Prefer", "return=representation")] [json| { "id":2 } |]
@@ -454,15 +459,26 @@ spec actualPgVersion = do
[json| [{ a: "keepme", b: null }] |]
{ matchHeaders = [matchContentTypeJson] }
it "can update based on a computed column" $
request methodPatch
"/items?always_true=eq.false"
[("Prefer", "return=representation")]
[json| { id: 100 } |]
`shouldRespondWith` "[]"
{ matchStatus = 200,
matchHeaders = ["Content-Range" <:> "*/*"]
}
context "filtering by a computed column" $ do
it "is successful" $
request methodPatch
"/items?is_first=eq.true"
[("Prefer", "return=representation")]
[json| { id: 100 } |]
`shouldRespondWith` [json| [{ id: 100 }] |]
{ matchStatus = 200,
matchHeaders = [matchContentTypeJson, "Content-Range" <:> "0-0/*"]
}
it "indicates no records updated by returning 404" $
request methodPatch
"/items?always_true=eq.false"
[("Prefer", "return=representation")]
[json| { id: 100 } |]
`shouldRespondWith` "[]"
{ matchStatus = 404,
matchHeaders = []
}
it "can provide a representation" $ do
_ <- post "/items"
test/Feature/SingularSpec.hs
+52 -44
View File
@@ -43,7 +43,6 @@ spec =
{ matchHeaders = ["Content-Type" <:> "application/vnd.pgrst.object+json; charset=utf-8"] }
context "when updating rows" $ do
it "works for one row" $ do
_ <- post "/addresses" [json| { id: 97, address: "A Street" } |]
request methodPatch
@@ -56,10 +55,9 @@ spec =
it "raises an error for multiple rows" $ do
_ <- 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]
[json| { address: "zzz" } |]
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
@@ -67,15 +65,16 @@ spec =
-- the rows should not be updated, either
get "/addresses?id=eq.98" `shouldRespondWith` [str|[{"id":98,"address":"xxx"}]|]
it "raises an error for zero rows" $ do
p <- request methodPatch "/items?id=gt.0&id=lt.0"
[("Prefer", "return=representation"), singular] [json|{"id":1}|]
liftIO $ do
simpleStatus p `shouldBe` notAcceptable406
isErrorFormat (simpleBody p) `shouldBe` True
it "raises an error for zero rows" $
request methodPatch "/items?id=gt.0&id=lt.0"
[("Prefer", "return=representation"), 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"]
}
context "when creating rows" $ do
it "works for one row" $ do
p <- request methodPost
"/addresses"
@@ -117,17 +116,17 @@ spec =
, matchHeaders = ["Content-Range" <:> "*/*"]
}
it "raises an error when creating zero entities" $ do
p <- request methodPost
"/addresses"
[("Prefer", "return=representation"), singular]
[json| [ ] |]
liftIO $ do
simpleStatus p `shouldBe` notAcceptable406
isErrorFormat (simpleBody p) `shouldBe` True
it "raises an error when creating zero entities" $
request methodPost "/addresses"
[("Prefer", "return=representation"), 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"]
}
context "when deleting rows" $ do
it "works for one row" $ do
p <- request methodDelete
"/items?id=eq.11"
@@ -146,21 +145,24 @@ spec =
, matchHeaders = ["Content-Range" <:> "0-9/*"]
}
it "raises an error when deleting zero entities" $ do
p <- request methodDelete "/items?id=lt.0"
[("Prefer", "return=representation"), singular] ""
liftIO $ do
simpleStatus p `shouldBe` notAcceptable406
isErrorFormat (simpleBody p) `shouldBe` True
it "raises an error when deleting zero entities" $
request methodDelete "/items?id=lt.0"
[("Prefer", "return=representation"), 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"]
}
context "when calling a stored proc" $ do
it "fails for zero rows" $ do
p <- request methodPost "/rpc/getproject"
[singular] [json|{ "id": 9999999}|]
liftIO $ do
simpleStatus p `shouldBe` notAcceptable406
isErrorFormat (simpleBody p) `shouldBe` True
it "fails for zero rows" $
request methodPost "/rpc/getproject"
[singular] [json|{ "id": 9999999}|]
`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"]
}
-- this one may be controversial, should vnd.pgrst.object include
-- the likes of 2 and "hello?"
@@ -174,20 +176,26 @@ spec =
[singular] [json|{ "id": 1}|] `shouldRespondWith`
[str|{"id":1,"name":"Windows 7","client_id":1}|]
it "fails for multiple rows" $ do
p <- request methodPost "/rpc/getallprojects" [singular] "{}"
liftIO $ do
simpleStatus p `shouldBe` notAcceptable406
isErrorFormat (simpleBody p) `shouldBe` True
it "fails for multiple rows" $
request methodPost "/rpc/getallprojects"
[singular] "{}"
`shouldRespondWith`
[str|{"details":"Results contain 5 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 "executes the proc exactly once per request" $ do
request methodPost "/rpc/getproject?select=id,name" [] [json| {"id": 1} |]
`shouldRespondWith` [str|[{"id":1,"name":"Windows 7"}]|]
p <- request methodPost "/rpc/setprojects" [singular]
[json| {"id_l": 1, "id_h": 2, "name": "changed"} |]
liftIO $ do
simpleStatus p `shouldBe` notAcceptable406
isErrorFormat (simpleBody p) `shouldBe` True
request methodPost "/rpc/setprojects" [singular]
[json| {"id_l": 1, "id_h": 2, "name": "changed"} |]
`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"]
}
-- should not actually have executed the function
request methodPost "/rpc/getproject?select=id,name" [] [json| {"id": 1} |]
test/fixtures/schema.sql
Vendored
+5
View File
@@ -116,6 +116,11 @@ CREATE FUNCTION always_true(test.items) RETURNS boolean
LANGUAGE sql STABLE
AS $$ SELECT true $$;
CREATE FUNCTION is_first(test.items) RETURNS boolean
LANGUAGE sql STABLE
AS $$ SELECT $1.id = 1 $$;
CREATE FUNCTION anti_id(test.items) RETURNS bigint
LANGUAGE sql STABLE
AS $_$ SELECT $1.id * -1 $_$;