diff --git a/CHANGELOG.md b/CHANGELOG.md index a84789e2ab..b4d8fb8217 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,10 +14,12 @@ This project adheres to [Semantic Versioning](http://semver.org/). - #1530, Fix how the PostgREST version is shown in the help text when the `.git` directory is not available - @monacoremo - #1094, Fix expired JWTs starting an empty transaction on the db - @steve-chavez + - #1475, Fix location header for POST request with select= without PK (#1162) - @wolfgangwalther ### Changed - #1522, #1528, #1535, Docker images are now built from scratch based on a the static PostgREST executable (#1494) and with Nix instead of a `Dockerfile`. This reduces the compressed image size from over 30mb to about 4mb - @monacoremo + - #1475, Location header for POST request is only included when PK is available on the table (#1461) - @wolfgangwalther ## [7.0.1] - 2020-05-18 diff --git a/src/PostgREST/App.hs b/src/PostgREST/App.hs index 8c8f640254..2f03ca8d71 100644 --- a/src/PostgREST/App.hs +++ b/src/PostgREST/App.hs @@ -361,7 +361,7 @@ app dbStructure proc cols conf apiRequest = let readReq = readRequest s t maxRows (dbRelations dbStructure) apiRequest returnings :: ReadRequest -> Either Response [FieldName] - returnings rr = Right (returningCols rr) + returnings rr = Right (returningCols rr []) in (,,,) <$> (readRequestToQuery <$> readReq) <*> diff --git a/src/PostgREST/DbRequestBuilder.hs b/src/PostgREST/DbRequestBuilder.hs index e071384880..9e76d63332 100644 --- a/src/PostgREST/DbRequestBuilder.hs +++ b/src/PostgREST/DbRequestBuilder.hs @@ -316,7 +316,7 @@ mutateRequest schema tName apiRequest cols pkCols readReq = mapLeft errorRespons returnings = if iPreferRepresentation apiRequest == None then [] - else returningCols readReq + else returningCols readReq pkCols filters = map snd <$> mapM pRequestFilter mutateFilters logic = map snd <$> mapM pRequestLogicTree logicFilters combinedLogic = foldr addFilterToLogicForest <$> logic <*> filters @@ -324,8 +324,11 @@ mutateRequest schema tName apiRequest cols pkCols readReq = mapLeft errorRespons (mutateFilters, logicFilters) = join (***) onlyRoot (iFilters apiRequest, iLogic apiRequest) onlyRoot = filter (not . ( "." `isInfixOf` ) . fst) -returningCols :: ReadRequest -> [FieldName] -returningCols rr@(Node _ forest) = returnings +returningCols :: ReadRequest -> [FieldName] -> [FieldName] +returningCols rr@(Node _ forest) pkCols + -- if * is part of the select, we must not add pk or fk columns manually - otherwise those would be selected and output twice + | "*" `elem` fldNames = ["*"] + | otherwise = returnings where fldNames = fstFieldNames rr -- Without fkCols, when a mutateRequest to /projects?select=name,clients(name) occurs, the RETURNING SQL part would be @@ -343,7 +346,8 @@ returningCols rr@(Node _ forest) = returnings -- However if the "client_id" is present, e.g. mutateRequest to /projects?select=client_id,name,clients(name) -- we would get `RETURNING client_id, name, client_id` and then we would produce the "column reference \"client_id\" is ambiguous" -- error from PostgreSQL. So we deduplicate with Set: - returnings = S.toList . S.fromList $ fldNames ++ (colName <$> fkCols) + -- We are adding the primary key columns as well to make sure, that a proper location header can always be built for INSERT/POST + returnings = S.toList . S.fromList $ fldNames ++ (colName <$> fkCols) ++ pkCols -- Traditional filters(e.g. id=eq.1) are added as root nodes of the LogicTree -- they are later concatenated with AND in the QueryBuilder diff --git a/src/PostgREST/Private/QueryFragment.hs b/src/PostgREST/Private/QueryFragment.hs index cbafb4db11..a5aca6d029 100644 --- a/src/PostgREST/Private/QueryFragment.hs +++ b/src/PostgREST/Private/QueryFragment.hs @@ -87,10 +87,12 @@ asBinaryF fieldName = "coalesce(string_agg(_postgrest_t." <> pgFmtIdent fieldNam locationF :: [Text] -> SqlFragment locationF pKeys = [qc|( WITH data AS (SELECT row_to_json(_) AS row FROM {sourceCTEName} AS _ LIMIT 1) - SELECT array_agg(json_data.key || '=' || coalesce('eq.' || json_data.value, 'is.null')) + SELECT array_agg(json_data.key || '=eq.' || json_data.value) FROM data CROSS JOIN json_each_text(data.row) AS json_data - {("WHERE json_data.key IN ('" <> intercalate "','" pKeys <> "')") `emptyOnFalse` null pKeys} + WHERE json_data.key IN ('{fmtPKeys}') )|] + where + fmtPKeys = intercalate "','" pKeys fromQi :: QualifiedIdentifier -> SqlFragment fromQi t = (if s == "" then "" else pgFmtIdent s <> ".") <> pgFmtIdent n diff --git a/test/Feature/InsertSpec.hs b/test/Feature/InsertSpec.hs index 7dca5c4549..01ce8fcf74 100644 --- a/test/Feature/InsertSpec.hs +++ b/test/Feature/InsertSpec.hs @@ -104,6 +104,25 @@ spec actualPgVersion = do , "Content-Range" <:> "*/*" ] } + it "should not throw and return location header when selecting without PK" $ + request methodPost "/projects?select=name,client_id" [("Prefer", "return=representation")] + [str|{"id":10,"name":"New Project","client_id":2}|] `shouldRespondWith` + [str|[{"name":"New Project","client_id":2}]|] + { matchStatus = 201 + , matchHeaders = [ matchContentTypeJson + , "Location" <:> "/projects?id=eq.10" + , "Content-Range" <:> "*/*" ] + } + + context "requesting no representation" $ + it "should not throw and return location header when selecting without PK" $ + request methodPost "/projects?select=name,client_id" [] + [str|{"id":11,"name":"New Project","client_id":2}|] `shouldRespondWith` "" + { matchStatus = 201 + , matchHeaders = [ "Location" <:> "/projects?id=eq.11" + , "Content-Range" <:> "*/*" ] + } + context "from an html form" $ it "accepts disparate json types" $ do p <- request methodPost "/menagerie" @@ -139,11 +158,11 @@ spec actualPgVersion = do } context "into a table with no pk" $ do - it "succeeds with 201 and a link including all fields" $ do + it "succeeds with 201 but no location header" $ do p <- post "/no_pk" [json| { "a":"foo", "b":"bar" } |] liftIO $ do simpleBody p `shouldBe` "" - simpleHeaders p `shouldSatisfy` matchHeader hLocation "/no_pk\\?a=eq.foo&b=eq.bar" + lookup hLocation (simpleHeaders p) `shouldBe` Nothing simpleStatus p `shouldBe` created201 it "returns full details of inserted record if asked" $ do @@ -152,7 +171,7 @@ spec actualPgVersion = do [json| { "a":"bar", "b":"baz" } |] liftIO $ do simpleBody p `shouldBe` [json| [{ "a":"bar", "b":"baz" }] |] - simpleHeaders p `shouldSatisfy` matchHeader hLocation "/no_pk\\?a=eq.bar&b=eq.baz" + lookup hLocation (simpleHeaders p) `shouldBe` Nothing simpleStatus p `shouldBe` created201 it "returns empty array when no items inserted, and return=rep" $ do @@ -169,7 +188,7 @@ spec actualPgVersion = do [json| { "a":null, "b":"foo" } |] liftIO $ do simpleBody p `shouldBe` [json| [{ "a":null, "b":"foo" }] |] - simpleHeaders p `shouldSatisfy` matchHeader hLocation "/no_pk\\?a=is.null&b=eq.foo" + lookup hLocation (simpleHeaders p) `shouldBe` Nothing simpleStatus p `shouldBe` created201 context "with compound pk supplied" $ @@ -238,24 +257,20 @@ spec actualPgVersion = do context "jsonb" $ do it "serializes nested object" $ do let inserted = [json| { "data": { "foo":"bar" } } |] - location = "/json?data=eq.%7B%22foo%22%3A%22bar%22%7D" request methodPost "/json" [("Prefer", "return=representation")] inserted `shouldRespondWith` [str|[{"data":{"foo":"bar"}}]|] { matchStatus = 201 - , matchHeaders = ["Location" <:> location] } it "serializes nested array" $ do let inserted = [json| { "data": [1,2,3] } |] - location = "/json?data=eq.%5B1%2C2%2C3%5D" request methodPost "/json" [("Prefer", "return=representation")] inserted `shouldRespondWith` [str|[{"data":[1,2,3]}]|] { matchStatus = 201 - , matchHeaders = ["Location" <:> location] } context "empty objects" $ do @@ -358,8 +373,7 @@ spec actualPgVersion = do "a,b\nbar,baz" `shouldRespondWith` "a,b\nbar,baz" { matchStatus = 201 - , matchHeaders = ["Content-Type" <:> "text/csv; charset=utf-8", - "Location" <:> "/no_pk?a=eq.bar&b=eq.baz"] + , matchHeaders = ["Content-Type" <:> "text/csv; charset=utf-8"] } it "can post nulls" $ @@ -368,8 +382,7 @@ spec actualPgVersion = do "a,b\nNULL,foo" `shouldRespondWith` "a,b\n,foo" { matchStatus = 201 - , matchHeaders = ["Content-Type" <:> "text/csv; charset=utf-8", - "Location" <:> "/no_pk?a=is.null&b=eq.foo"] + , matchHeaders = ["Content-Type" <:> "text/csv; charset=utf-8"] } it "only returns the requested column header with its associated data" $ @@ -393,8 +406,8 @@ spec actualPgVersion = do context "with unicode values" $ it "succeeds and returns usable location header" $ do - let payload = [json| { "a":"圍棋", "b":"¥" } |] - p <- request methodPost "/no_pk" + let payload = [json| { "k":"圍棋", "extra":"¥" } |] + p <- request methodPost "/simple_pk?select=extra,k" [("Prefer", "return=representation")] payload liftIO $ do @@ -402,7 +415,7 @@ spec actualPgVersion = do simpleStatus p `shouldBe` created201 let Just location = lookup hLocation $ simpleHeaders p - r <- get location + r <- get (location <> "&select=extra,k") liftIO $ simpleBody r `shouldBe` "["<>payload<>"]" describe "Patching record" $ do