Fix POST, PATCH, DELETE with ?select= and empty body or return=minimal (#1471)

* add tests for POST, PATCH, DELETE when using ?select= with empty bodies or ret=min

* fix select with empty bodies and ret=min

* improved tests for default cases, fixed computed overloaded columns on empty-body?select= PATCH

CHANGELOG.md

@@ -10,6 +10,7 @@ This project adheres to [Semantic Versioning](http://semver.org/).
 ### Fixed
 
 - #1473, Fix overloaded computed columns on RPC - @wolfgangwalther
+- #1471, Fix POST, PATCH, DELETE with ?select= and return=minimal and PATCH with empty body - @wolfgangwalther
 
 ## [7.0.0] - 2020-04-03
 

src/PostgREST/QueryBuilder.hs

@@ -94,7 +94,10 @@ mutateRequestToQuery (Insert mainQi iCols onConflct putConditions returnings) =
     cols = intercalate ", " $ pgFmtIdent <$> S.toList iCols
 mutateRequestToQuery (Update mainQi uCols logicForest returnings) =
   if S.null uCols
-    then "WITH " <> ignoredBody <> "SELECT null WHERE false" -- if there are no columns we cannot do UPDATE table SET {empty}, it'd be invalid syntax
+    -- if there are no columns we cannot do UPDATE table SET {empty}, it'd be invalid syntax
+    -- selecting an empty resultset from mainQi gives us the column names to prevent errors when using &select=
+    -- the select has to be based on "returnings" to make computed overloaded functions not throw
+    then "WITH " <> ignoredBody <> "SELECT " <> empty_body_returned_columns <> " FROM " <> fromQi mainQi <> " WHERE false"
     else
       unwords [
         "WITH " <> normalizedBody,
@@ -105,6 +108,10 @@ mutateRequestToQuery (Update mainQi uCols logicForest returnings) =
         ]
   where
     cols = intercalate ", " (pgFmtIdent <> const " = _." <> pgFmtIdent <$> S.toList uCols)
+    empty_body_returned_columns :: SqlFragment
+    empty_body_returned_columns
+      | null returnings = "NULL"
+      | otherwise       = intercalate ", " (pgFmtColumn (QualifiedIdentifier mempty $ qiName mainQi) <$> returnings)
 mutateRequestToQuery (Delete mainQi logicForest returnings) =
   unwords [
     "WITH " <> ignoredBody,

src/PostgREST/Statements.hs

@@ -55,7 +55,7 @@ createWriteStatement selectQuery mutateQuery wantSingle isInsert asCsv rep pKeys
         {locF} AS header,
         {bodyF} AS body,
         {responseHeadersF pgVer} AS response_headers
-      FROM ({selectQuery}) _postgrest_t |]
+      FROM ({selectF}) _postgrest_t |]
 
   locF =
     if isInsert && rep `elem` [Full, HeadersOnly]
@@ -72,6 +72,11 @@ createWriteStatement selectQuery mutateQuery wantSingle isInsert asCsv rep pKeys
     | wantSingle = asJsonSingleF
     | otherwise = asJsonF
 
+  selectF
+    -- prevent using any of the column names in ?select= when no response is returned from the CTE
+    | rep `elem` [None, HeadersOnly] = "SELECT * FROM " <> sourceCTEName
+    | otherwise                      = selectQuery
+
   decodeStandard :: HD.Result ResultsWithCount
   decodeStandard =
    fromMaybe (Nothing, 0, [], mempty, Right []) <$> HD.rowMaybe standardRow

test/Feature/DeleteSpec.hs

@@ -27,15 +27,30 @@ spec =
           { matchStatus  = 200
           , matchHeaders = ["Content-Range" <:> "*/1"]
           }
+
+      it "ignores ?select= when return not set or return=minimal" $ do
+        request methodDelete "/items?id=eq.3&select=id" [] ""
+          `shouldRespondWith` ""
+          { matchStatus  = 204
+          , matchHeaders = ["Content-Range" <:> "*/*"]
+          }
+        request methodDelete "/items?id=eq.3&select=id" [("Prefer", "return=minimal")] ""
+          `shouldRespondWith` ""
+          { matchStatus  = 204
+          , matchHeaders = ["Content-Range" <:> "*/*"]
+          }
+
       it "returns the deleted item and shapes the response" $
         request methodDelete "/complex_items?id=eq.2&select=id,name" [("Prefer", "return=representation")] ""
           `shouldRespondWith` [str|[{"id":2,"name":"Two"}]|]
           { matchStatus  = 200
           , matchHeaders = ["Content-Range" <:> "*/*"]
           }
+
       it "can rename and cast the selected columns" $
         request methodDelete "/complex_items?id=eq.3&select=ciId:id::text,ciName:name" [("Prefer", "return=representation")] ""
           `shouldRespondWith` [str|[{"ciId":"3","ciName":"Three"}]|]
+
       it "can embed (parent) entities" $
         request methodDelete "/tasks?id=eq.8&select=id,name,project:projects(id)" [("Prefer", "return=representation")] ""
           `shouldRespondWith` [str|[{"id":8,"name":"Code OSX","project":{"id":4}}]|]

test/Feature/InsertSpec.hs

@@ -48,6 +48,22 @@ spec actualPgVersion = do
           , matchHeaders = [matchContentTypeJson]
           }
 
+      it "ignores &select when return not set or using return=minimal" $ do
+        request methodPost "/menagerie?select=integer,varchar" []
+          [json| [{
+            "integer": 15, "double": 3.14159, "varchar": "testing!"
+          , "boolean": false, "date": "1900-01-01", "money": "$3.99"
+          , "enum": "foo"
+          }] |] `shouldRespondWith` ""
+          { matchStatus  = 201 }
+        request methodPost "/menagerie?select=integer,varchar" [("Prefer", "return=minimal")]
+          [json| [{
+            "integer": 16, "double": 3.14159, "varchar": "testing!"
+          , "boolean": false, "date": "1900-01-01", "money": "$3.99"
+          , "enum": "foo"
+          }] |] `shouldRespondWith` ""
+          { matchStatus  = 201 }
+
     context "non uniform json array" $ do
       it "rejects json array that isn't exclusivily composed of objects" $
         post "/articles"
@@ -194,6 +210,15 @@ spec actualPgVersion = do
         , matchHeaders = [matchContentTypeJson]
         }
 
+    context "with no payload" $
+      it "fails with 400 and error" $
+        post "/simple_pk" ""
+        `shouldRespondWith`
+        [json|{"message":"Error in $: not enough input"}|]
+        { matchStatus  = 400
+        , matchHeaders = [matchContentTypeJson]
+        }
+
     context "with valid json payload" $
       it "succeeds and returns 201 created" $
         post "/simple_pk" [json| { "k":"k1", "extra":"e1" } |] `shouldRespondWith` 201
@@ -251,6 +276,20 @@ spec actualPgVersion = do
           , matchHeaders = []
           }
 
+      it "successfully inserts a row with all-default columns with prefer=rep" $
+        request methodPost "/items" [("Prefer", "return=representation")] "{}"
+          `shouldRespondWith` [json|[{ id: 20 }]|]
+          { matchStatus  = 201,
+            matchHeaders = []
+          }
+
+      it "successfully inserts a row with all-default columns with prefer=rep and &select=" $
+        request methodPost "/items?select=id" [("Prefer", "return=representation")] "{}"
+          `shouldRespondWith` [json|[{ id: 21 }]|]
+          { matchStatus  = 201,
+            matchHeaders = []
+          }
+
     context "POST with ?columns parameter" $ do
       it "ignores json keys not included in ?columns" $ do
         request methodPost "/articles?columns=id,body" [("Prefer", "return=representation")]
@@ -383,6 +422,24 @@ spec actualPgVersion = do
             matchHeaders = []
           }
 
+    context "with invalid json payload" $
+      it "fails with 400 and error" $
+        request methodPatch "/simple_pk" [] "}{ x = 2"
+          `shouldRespondWith`
+          [json|{"message":"Error in $: Failed reading: not a valid json value"}|]
+          { matchStatus  = 400,
+            matchHeaders = [matchContentTypeJson]
+          }
+
+    context "with no payload" $
+      it "fails with 400 and error" $
+        request methodPatch "/items" [] ""
+          `shouldRespondWith`
+          [json|{"message":"Error in $: not enough input"}|]
+          { matchStatus  = 400,
+            matchHeaders = [matchContentTypeJson]
+          }
+
     context "in a nonempty table" $ do
       it "can update a single item" $ do
         g <- get "/items?id=eq.42"
@@ -499,35 +556,88 @@ spec actualPgVersion = do
             `shouldRespondWith` [json| [{ id: 1, computed_overload: true }] |]
             { matchHeaders = [matchContentTypeJson] }
 
-      it "makes no updates and returns 204, when patching with an empty json object/array" $ do
+      it "ignores ?select= when return not set or return=minimal" $ do
-        request methodPatch "/items" [] [json| {} |]
+        request methodPatch "/items?id=eq.1&select=id" [] [json| { id:1 } |]
           `shouldRespondWith` ""
           {
             matchStatus  = 204,
-            matchHeaders = ["Content-Range" <:> "*/*"]
+            matchHeaders = ["Content-Range" <:> "0-0/*"]
           }
-
+        request methodPatch "/items?id=eq.1&select=id" [("Prefer", "return=minimal")] [json| { id:1 } |]
-        request methodPatch "/items" [] [json| [] |]
           `shouldRespondWith` ""
           {
             matchStatus  = 204,
-            matchHeaders = ["Content-Range" <:> "*/*"]
+            matchHeaders = ["Content-Range" <:> "0-0/*"]
           }
 
-        request methodPatch "/items" [] [json| [{}] |]
+      context "when patching with an empty body" $ do
-          `shouldRespondWith` ""
+        it "makes no updates and returns 204 without return= and without ?select=" $ do
-          {
+          request methodPatch "/items" [] [json| {} |]
-            matchStatus  = 204,
+            `shouldRespondWith` ""
-            matchHeaders = ["Content-Range" <:> "*/*"]
+            {
-          }
+              matchStatus  = 204,
+              matchHeaders = ["Content-Range" <:> "*/*"]
+            }
 
-      it "makes no updates and returns 200, when patching with an empty json object and return=rep" $
+          request methodPatch "/items" [] [json| [] |]
-        request methodPatch "/items" [("Prefer", "return=representation")] [json| {} |]
+            `shouldRespondWith` ""
-          `shouldRespondWith` "[]"
+            {
-          {
+              matchStatus  = 204,
-            matchStatus  = 200,
+              matchHeaders = ["Content-Range" <:> "*/*"]
-            matchHeaders = ["Content-Range" <:> "*/*"]
+            }
-          }
+
+          request methodPatch "/items" [] [json| [{}] |]
+            `shouldRespondWith` ""
+            {
+              matchStatus  = 204,
+              matchHeaders = ["Content-Range" <:> "*/*"]
+            }
+
+        it "makes no updates and returns 204 without return= and with ?select=" $ do
+          request methodPatch "/items?select=id" [] [json| {} |]
+            `shouldRespondWith` ""
+            {
+              matchStatus  = 204,
+              matchHeaders = ["Content-Range" <:> "*/*"]
+            }
+
+          request methodPatch "/items?select=id" [] [json| [] |]
+            `shouldRespondWith` ""
+            {
+              matchStatus  = 204,
+              matchHeaders = ["Content-Range" <:> "*/*"]
+            }
+
+          request methodPatch "/items?select=id" [] [json| [{}] |]
+            `shouldRespondWith` ""
+            {
+              matchStatus  = 204,
+              matchHeaders = ["Content-Range" <:> "*/*"]
+            }
+
+        it "makes no updates and returns 200 with return=rep and without ?select=" $
+          request methodPatch "/items" [("Prefer", "return=representation")] [json| {} |]
+            `shouldRespondWith` "[]"
+            {
+              matchStatus  = 200,
+              matchHeaders = ["Content-Range" <:> "*/*"]
+            }
+
+        it "makes no updates and returns 200 with return=rep and with ?select=" $
+          request methodPatch "/items?select=id" [("Prefer", "return=representation")] [json| {} |]
+            `shouldRespondWith` "[]"
+            {
+              matchStatus  = 200,
+              matchHeaders = ["Content-Range" <:> "*/*"]
+            }
+
+        it "makes no updates and returns 200 with return=rep and with ?select= for overloaded computed columns" $
+          request methodPatch "/items?select=id,computed_overload" [("Prefer", "return=representation")] [json| {} |]
+            `shouldRespondWith` "[]"
+            {
+              matchStatus  = 200,
+              matchHeaders = ["Content-Range" <:> "*/*"]
+            }
 
     context "with unicode values" $
       it "succeeds and returns values intact" $ do