fix: Return status code 200 when PATCHing without changing rows

This partially reverts PostgREST#1257 / PostgREST#1272 / 5535317 where the 404 was introduced. A 406 error is still returned when requesting a single object via accept header. Returning an error when no rows are changed can be introduced through a different syntax again, see the discussion in PostgREST#2164. Fixes PostgREST#2343 Signed-off-by: Wolfgang Walther <[email protected]>
wolfgangwalther · Oct 27, 2022 · 38dd893 · 38dd893
1 parent 9be6747
commit 38dd893
Show file tree

Hide file tree

Showing 3 changed files with 15 additions and 19 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -15,6 +15,7 @@ This project adheres to [Semantic Versioning](http://semver.org/).
  - #2458, Fix a regression with the location header when inserting into views with PKs from multiple tables - @wolfgangwalther
  - #2356, Fix a regression in openapi output with mode follow-privileges - @wolfgangwalther
  - #2283, Fix infinite recursion when loading schema cache with self-referencing view - @wolfgangwalther
+ - #2343, Return status code 200 for PATCH requests which don't affect any rows - @wolfgangwalther
 
 ### Deprecated
 

diff --git a/src/PostgREST/Response.hs b/src/PostgREST/Response.hs
@@ -18,7 +18,6 @@ import qualified Data.Aeson                as JSON
 import qualified Data.ByteString.Char8     as BS
 import qualified Data.ByteString.Lazy      as LBS
 import qualified Data.HashMap.Strict       as HM
-import qualified Data.Set                  as S
 import           Data.Text.Read            (decimal)
 import qualified Network.HTTP.Types.Header as HTTP
 import qualified Network.HTTP.Types.Status as HTTP
@@ -121,21 +120,17 @@ updateResponse ctxApiRequest@ApiRequest{..} resultSet = case resultSet of
   RSStandard{..} -> do
     let
       response = gucResponse rsGucStatus rsGucHeaders
-      fullRepr = iPreferRepresentation == Full
-      updateIsNoOp = S.null iColumns
-      status
-        | rsQueryTotal == 0 && not updateIsNoOp = HTTP.status404
-        | fullRepr = HTTP.status200
-        | otherwise = HTTP.status204
       contentRangeHeader =
         RangeQuery.contentRangeH 0 (rsQueryTotal - 1) $
           if shouldCount iPreferCount then Just rsQueryTotal else Nothing
       headers = [contentRangeHeader]
 
-    if fullRepr then
-        response status (headers ++ contentTypeHeaders ctxApiRequest) (LBS.fromStrict rsBody)
+    if iPreferRepresentation == Full then
+        response HTTP.status200
+          (headers ++ contentTypeHeaders ctxApiRequest)
+          (LBS.fromStrict rsBody)
       else
-        response status headers mempty
+        response HTTP.status204 headers mempty
 
   RSPlan plan ->
     Wai.responseLBS HTTP.status200 (contentTypeHeaders ctxApiRequest) $ LBS.fromStrict plan

diff --git a/test/spec/Feature/Query/UpdateSpec.hs b/test/spec/Feature/Query/UpdateSpec.hs
@@ -28,12 +28,12 @@ spec = do
             `shouldRespondWith` 404
 
     context "on an empty table" $
-      it "indicates no records found to update by returning 404" $
+      it "succeeds with status code 204" $
         request methodPatch "/empty_table" []
             [json| { "extra":20 } |]
           `shouldRespondWith`
             ""
-            { matchStatus  = 404,
+            { matchStatus  = 204,
               matchHeaders = [matchHeaderAbsent hContentType]
             }
 
@@ -71,14 +71,14 @@ spec = do
           [("Prefer", "return=representation")] [json| { "id":999999 } |]
           `shouldRespondWith` "[]"
           {
-            matchStatus  = 404,
+            matchStatus  = 200,
             matchHeaders = []
           }
 
-      it "gives a 404 when no rows updated" $
+      it "returns status code 200 when no rows updated" $
         request methodPatch "/items?id=eq.99999999" []
           [json| { "id": 42 } |]
-            `shouldRespondWith` 404
+            `shouldRespondWith` 204
 
       it "returns updated object as array when return=rep" $
         request methodPatch "/items?id=eq.2"
@@ -137,13 +137,13 @@ spec = do
               matchHeaders = [matchContentTypeJson, "Content-Range" <:> "0-0/*"]
             }
 
-        it "indicates no records updated by returning 404" $
+        it "returns empty array when no rows updated and return=rep" $
           request methodPatch
             "/items?always_true=eq.false"
             [("Prefer", "return=representation")]
             [json| { id: 100 } |]
             `shouldRespondWith` "[]"
-            { matchStatus  = 404,
+            { matchStatus  = 200,
               matchHeaders = []
             }
 
@@ -304,9 +304,9 @@ spec = do
           `shouldRespondWith`
             [json|[{"id": 1, "body": "Some real content", "owner": "postgrest_test_anonymous"}]|]
 
-      it "ignores json keys and gives 404 if no record updated" $
+      it "ignores json keys and gives 200 if no record updated" $
         request methodPatch "/articles?id=eq.2001&columns=body" [("Prefer", "return=representation")]
-          [json| {"body": "Some real content", "smth": "here", "other": "stuff", "fake_id": 13} |] `shouldRespondWith` 404
+          [json| {"body": "Some real content", "smth": "here", "other": "stuff", "fake_id": 13} |] `shouldRespondWith` 200
 
   context "tables with self reference foreign keys" $ do
     it "embeds children after update" $