From be674eb41d958716e9b4fa0fe186208992f73cb8 Mon Sep 17 00:00:00 2001 From: Wolfgang Walther Date: Mon, 13 Apr 2020 19:32:35 +0200 Subject: [PATCH] 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 | 1 + src/PostgREST/QueryBuilder.hs | 9 ++- src/PostgREST/Statements.hs | 7 +- test/Feature/DeleteSpec.hs | 15 ++++ test/Feature/InsertSpec.hs | 148 +++++++++++++++++++++++++++++----- 5 files changed, 159 insertions(+), 21 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b7476417d..6d8210573 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/src/PostgREST/QueryBuilder.hs b/src/PostgREST/QueryBuilder.hs index 49e182aab..594780366 100644 --- a/src/PostgREST/QueryBuilder.hs +++ b/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, diff --git a/src/PostgREST/Statements.hs b/src/PostgREST/Statements.hs index 74012a399..9a9de8958 100644 --- a/src/PostgREST/Statements.hs +++ b/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 diff --git a/test/Feature/DeleteSpec.hs b/test/Feature/DeleteSpec.hs index 0acc52441..2de588e74 100644 --- a/test/Feature/DeleteSpec.hs +++ b/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}}]|] diff --git a/test/Feature/InsertSpec.hs b/test/Feature/InsertSpec.hs index ad9b7c336..c28d4f30c 100644 --- a/test/Feature/InsertSpec.hs +++ b/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 - request methodPatch "/items" [] [json| {} |] + it "ignores ?select= when return not set or return=minimal" $ do + 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" [] [json| [] |] + request methodPatch "/items?id=eq.1&select=id" [("Prefer", "return=minimal")] [json| { id:1 } |] `shouldRespondWith` "" { matchStatus = 204, - matchHeaders = ["Content-Range" <:> "*/*"] + matchHeaders = ["Content-Range" <:> "0-0/*"] } - request methodPatch "/items" [] [json| [{}] |] - `shouldRespondWith` "" - { - matchStatus = 204, - matchHeaders = ["Content-Range" <:> "*/*"] - } + context "when patching with an empty body" $ do + it "makes no updates and returns 204 without return= and without ?select=" $ do + request methodPatch "/items" [] [json| {} |] + `shouldRespondWith` "" + { + 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" [("Prefer", "return=representation")] [json| {} |] - `shouldRespondWith` "[]" - { - matchStatus = 200, - matchHeaders = ["Content-Range" <:> "*/*"] - } + request methodPatch "/items" [] [json| [] |] + `shouldRespondWith` "" + { + matchStatus = 204, + 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