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

Author: Qu4tro

src/PostgREST/App.hs

@@ -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

@@ -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

@@ -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

@@ -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,26 +55,26 @@ 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
 
         -- 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

@@ -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 $_$;