diff --git a/CHANGELOG.md b/CHANGELOG.md index ed0a3683180..e009e37dc94 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,9 @@ This project adheres to [Semantic Versioning](http://semver.org/). ### Fixed - #3015, Fix unnecessary count() on RPC returning single - @steve-chavez + - #1070, Fix HTTP status responses for upserts - @taimoorzaeem + + `PUT` returns `201` instead of `200` when rows are inserted + + `POST` with `Prefer: resolution=merge-duplicates` returns `200` instead of `201` when no rows are inserted ## [11.2.2] - 2023-10-25 diff --git a/src/PostgREST/ApiRequest/Preferences.hs b/src/PostgREST/ApiRequest/Preferences.hs index e4b3ca4f6dd..4e86610ae48 100644 --- a/src/PostgREST/ApiRequest/Preferences.hs +++ b/src/PostgREST/ApiRequest/Preferences.hs @@ -176,6 +176,7 @@ class ToHeaderValue a where data PreferResolution = MergeDuplicates | IgnoreDuplicates + deriving Eq instance ToHeaderValue PreferResolution where toHeaderValue MergeDuplicates = "resolution=merge-duplicates" diff --git a/src/PostgREST/Query.hs b/src/PostgREST/Query.hs index 79bbe5e6c81..796caee0d20 100644 --- a/src/PostgREST/Query.hs +++ b/src/PostgREST/Query.hs @@ -188,16 +188,18 @@ openApiQuery sCache pgVer AppConfig{..} tSchema = writeQuery :: MutateReadPlan -> ApiRequest -> AppConfig -> DbHandler ResultSet writeQuery MutateReadPlan{mrReadPlan, mrMutatePlan, mrResAgg, mrMedia} ApiRequest{iPreferences=Preferences{..}} conf = let - (isInsert, pkCols) = case mrMutatePlan of {Insert{insPkCols} -> (True, insPkCols); _ -> (False, mempty);} + (isPut, isInsert, pkCols) = case mrMutatePlan of {Insert{where_,insPkCols} -> ((not . null) where_, True, insPkCols); _ -> (False,False, mempty);} in lift . SQL.statement mempty $ Statements.prepareWrite (QueryBuilder.readPlanToQuery mrReadPlan) (QueryBuilder.mutatePlanToQuery mrMutatePlan) isInsert + isPut mrMedia mrResAgg preferRepresentation + preferResolution pkCols (configDbPreparedStatements conf) diff --git a/src/PostgREST/Query/QueryBuilder.hs b/src/PostgREST/Query/QueryBuilder.hs index 06e4ad1f6b5..c81772b810b 100644 --- a/src/PostgREST/Query/QueryBuilder.hs +++ b/src/PostgREST/Query/QueryBuilder.hs @@ -87,7 +87,8 @@ mutatePlanToQuery (Insert mainQi iCols body onConflct putConditions returnings _ "INSERT INTO " <> fromQi mainQi <> (if null iCols then " " else "(" <> cols <> ") ") <> fromJsonBodyF body iCols True False applyDefaults <> -- Only used for PUT - (if null putConditions then mempty else "WHERE " <> intercalateSnippet " AND " (pgFmtLogicTree (QualifiedIdentifier mempty "pgrst_body") <$> putConditions)) <> + (if null putConditions then mempty else "WHERE " <> addConfigPgrstInserted True <> " AND " <> intercalateSnippet " AND " (pgFmtLogicTree (QualifiedIdentifier mempty "pgrst_body") <$> putConditions)) <> + (if null putConditions && mergeDups then "WHERE " <> addConfigPgrstInserted True else mempty) <> maybe mempty (\(oncDo, oncCols) -> if null oncCols then mempty @@ -98,11 +99,12 @@ mutatePlanToQuery (Insert mainQi iCols body onConflct putConditions returnings _ MergeDuplicates -> if null iCols then "DO NOTHING" - else "DO UPDATE SET " <> intercalateSnippet ", " ((pgFmtIdent . cfName) <> const " = EXCLUDED." <> (pgFmtIdent . cfName) <$> iCols) + else "DO UPDATE SET " <> intercalateSnippet ", " ((pgFmtIdent . cfName) <> const " = EXCLUDED." <> (pgFmtIdent . cfName) <$> iCols) <> (if null putConditions && not mergeDups then mempty else "WHERE " <> addConfigPgrstInserted False) ) onConflct <> " " <> - returningF mainQi returnings + returningF mainQi returnings where cols = intercalateSnippet ", " $ pgFmtIdent . cfName <$> iCols + mergeDups = case onConflct of {Just (MergeDuplicates,_) -> True; _ -> False;} -- An update without a limit is always filtered with a WHERE mutatePlanToQuery (Update mainQi uCols body logicForest range ordts returnings applyDefaults) diff --git a/src/PostgREST/Query/SqlFragment.hs b/src/PostgREST/Query/SqlFragment.hs index f2d21efce61..8f80be812c1 100644 --- a/src/PostgREST/Query/SqlFragment.hs +++ b/src/PostgREST/Query/SqlFragment.hs @@ -24,6 +24,8 @@ module PostgREST.Query.SqlFragment , fromJsonBodyF , responseHeadersF , responseStatusF + , addConfigPgrstInserted + , currentSettingF , returningF , singleParameter , sourceCTE @@ -439,6 +441,11 @@ responseHeadersF = currentSettingF "response.headers" responseStatusF :: SQL.Snippet responseStatusF = currentSettingF "response.status" +addConfigPgrstInserted :: Bool -> SQL.Snippet +addConfigPgrstInserted add = + let (symbol, num) = if add then ("+", "0") else ("-", "-1") in + "set_config('pgrst.inserted', (coalesce(" <> currentSettingF "pgrst.inserted" <> "::int, 0) " <> symbol <> " 1)::text, true) <> '" <> num <> "'" + currentSettingF :: SQL.Snippet -> SQL.Snippet currentSettingF setting = -- nullif is used because of https://gist.github.com/steve-chavez/8d7033ea5655096903f3b52f8ed09a15 diff --git a/src/PostgREST/Query/Statements.hs b/src/PostgREST/Query/Statements.hs index b9d9c8f5bd4..eb398eb1522 100644 --- a/src/PostgREST/Query/Statements.hs +++ b/src/PostgREST/Query/Statements.hs @@ -49,15 +49,19 @@ data ResultSet -- ^ the HTTP headers to be added to the response , rsGucStatus :: Maybe Text -- ^ the HTTP status to be added to the response + , rsInserted :: Maybe Int64 + -- ^ the number of rows inserted (Only used with PUT) } | RSPlan BS.ByteString -- ^ the plan of the query -prepareWrite :: SQL.Snippet -> SQL.Snippet -> Bool -> MediaType -> ResultAggregate -> - Maybe PreferRepresentation -> [Text] -> Bool -> SQL.Statement () ResultSet -prepareWrite selectQuery mutateQuery isInsert mt rAgg rep pKeys = +prepareWrite :: SQL.Snippet -> SQL.Snippet -> Bool -> Bool -> MediaType -> ResultAggregate -> + Maybe PreferRepresentation -> Maybe PreferResolution -> [Text] -> Bool -> SQL.Statement () ResultSet +prepareWrite selectQuery mutateQuery isInsert isPut mt rAgg rep resolution pKeys = SQL.dynamicallyParameterized (mtSnippet mt snippet) decodeIt where + checkUpsert snip = if isInsert && (isPut || resolution == Just MergeDuplicates) then snip else "''" + pgrstInsertedF = checkUpsert "nullif(current_setting('pgrst.inserted', true),'')::int" snippet = "WITH " <> sourceCTE <> " AS (" <> mutateQuery <> ") " <> "SELECT " <> @@ -66,7 +70,8 @@ prepareWrite selectQuery mutateQuery isInsert mt rAgg rep pKeys = locF <> " AS header, " <> aggF Nothing rAgg <> " AS body, " <> responseHeadersF <> " AS response_headers, " <> - responseStatusF <> " AS response_status " <> + responseStatusF <> " AS response_status, " <> + pgrstInsertedF <> " AS response_inserted " <> "FROM (" <> selectF <> ") _postgrest_t" locF = @@ -86,7 +91,7 @@ prepareWrite selectQuery mutateQuery isInsert mt rAgg rep pKeys = decodeIt :: HD.Result ResultSet decodeIt = case mt of MTPlan{} -> planRow - _ -> fromMaybe (RSStandard Nothing 0 mempty mempty Nothing Nothing) <$> HD.rowMaybe (standardRow False) + _ -> fromMaybe (RSStandard Nothing 0 mempty mempty Nothing Nothing Nothing) <$> HD.rowMaybe (standardRow False) prepareRead :: SQL.Snippet -> SQL.Snippet -> Bool -> MediaType -> ResultAggregate -> Bool -> SQL.Statement () ResultSet prepareRead selectQuery countQuery countTotal mt rAgg = @@ -100,7 +105,8 @@ prepareRead selectQuery countQuery countTotal mt rAgg = "pg_catalog.count(_postgrest_t) AS page_total, " <> aggF Nothing rAgg <> " AS body, " <> responseHeadersF <> " AS response_headers, " <> - responseStatusF <> " AS response_status " <> + responseStatusF <> " AS response_status, " <> + "''" <> " AS response_inserted " <> "FROM ( SELECT * FROM " <> sourceCTE <> " ) _postgrest_t" (countCTEF, countResultF) = countF countQuery countTotal @@ -126,7 +132,8 @@ prepareCall rout callProcQuery selectQuery countQuery countTotal mt rAgg = else "pg_catalog.count(_postgrest_t)") <> " AS page_total, " <> aggF (Just rout) rAgg <> " AS body, " <> responseHeadersF <> " AS response_headers, " <> - responseStatusF <> " AS response_status " <> + responseStatusF <> " AS response_status, " <> + "''" <> " AS response_inserted " <> "FROM (" <> selectQuery <> ") _postgrest_t" (countCTEF, countResultF) = countF countQuery countTotal @@ -134,7 +141,7 @@ prepareCall rout callProcQuery selectQuery countQuery countTotal mt rAgg = decodeIt :: HD.Result ResultSet decodeIt = case mt of MTPlan{} -> planRow - _ -> fromMaybe (RSStandard (Just 0) 0 mempty mempty Nothing Nothing) <$> HD.rowMaybe (standardRow True) + _ -> fromMaybe (RSStandard (Just 0) 0 mempty mempty Nothing Nothing Nothing) <$> HD.rowMaybe (standardRow True) preparePlanRows :: SQL.Snippet -> Bool -> SQL.Statement () (Maybe Int64) preparePlanRows countQuery = @@ -152,6 +159,7 @@ standardRow noLocation = <*> (if noLocation then pure mempty else fmap splitKeyValue <$> arrayColumn HD.bytea) <*> column HD.bytea <*> nullableColumn HD.bytea <*> nullableColumn HD.text + <*> nullableColumn HD.int8 where splitKeyValue :: ByteString -> (ByteString, ByteString) splitKeyValue kv = diff --git a/src/PostgREST/Response.hs b/src/PostgREST/Response.hs index fee977efca0..96faaf8f3b9 100644 --- a/src/PostgREST/Response.hs +++ b/src/PostgREST/Response.hs @@ -22,6 +22,7 @@ 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 Data.Maybe (fromJust) import Data.Text.Read (decimal) import qualified Network.HTTP.Types.Header as HTTP import qualified Network.HTTP.Types.Status as HTTP @@ -35,6 +36,7 @@ import qualified PostgREST.Response.OpenAPI as OpenAPI import PostgREST.ApiRequest (ApiRequest (..), InvokeMethod (..)) import PostgREST.ApiRequest.Preferences (PreferRepresentation (..), + PreferResolution (..), Preferences (..), prefAppliedHeader, shouldCount) @@ -119,8 +121,13 @@ createResponse QualifiedIdentifier{..} MutateReadPlan{mrMutatePlan, mrMedia} ctx if shouldCount preferCount then Just rsQueryTotal else Nothing , prefHeader ] - let status = HTTP.status201 - let (headers', bod) = case preferRepresentation of + let isInsertIfGTZero i = + if i <= 0 && preferResolution == Just MergeDuplicates then + HTTP.status200 + else + HTTP.status201 + status = maybe HTTP.status200 isInsertIfGTZero rsInserted + (headers', bod) = case preferRepresentation of Just Full -> (headers ++ contentTypeHeaders mrMedia ctxApiRequest, LBS.fromStrict rsBody) Just None -> (headers, mempty) Just HeadersOnly -> (headers, mempty) @@ -142,11 +149,11 @@ updateResponse MutateReadPlan{mrMedia} ctxApiRequest@ApiRequest{iPreferences=Pre prefHeader = prefAppliedHeader $ Preferences Nothing preferRepresentation Nothing preferCount preferTransaction preferMissing preferHandling [] headers = catMaybes [contentRangeHeader, prefHeader] - let - (status, headers', body) = case preferRepresentation of - Just Full -> (HTTP.status200, headers ++ contentTypeHeaders mrMedia ctxApiRequest, LBS.fromStrict rsBody) - Just None -> (HTTP.status204, headers, mempty) - _ -> (HTTP.status204, headers, mempty) + let (status, headers', body) = + case preferRepresentation of + Just Full -> (HTTP.status200, headers ++ contentTypeHeaders mrMedia ctxApiRequest, LBS.fromStrict rsBody) + Just None -> (HTTP.status204, headers, mempty) + _ -> (HTTP.status204, headers, mempty) (ovStatus, ovHeaders) <- overrideStatusHeaders rsGucStatus rsGucHeaders status headers' @@ -162,9 +169,11 @@ singleUpsertResponse MutateReadPlan{mrMedia} ctxApiRequest@ApiRequest{iPreferenc prefHeader = maybeToList . prefAppliedHeader $ Preferences Nothing preferRepresentation Nothing preferCount preferTransaction Nothing preferHandling [] cTHeader = contentTypeHeaders mrMedia ctxApiRequest - let (status, headers, body) = + let isInsertIfGTZero i = if i > 0 then HTTP.status201 else HTTP.status200 + upsertStatus = isInsertIfGTZero $ fromJust rsInserted + (status, headers, body) = case preferRepresentation of - Just Full -> (HTTP.status200, cTHeader ++ prefHeader, LBS.fromStrict rsBody) + Just Full -> (upsertStatus, cTHeader ++ prefHeader, LBS.fromStrict rsBody) Just None -> (HTTP.status204, prefHeader, mempty) _ -> (HTTP.status204, prefHeader, mempty) (ovStatus, ovHeaders) <- overrideStatusHeaders rsGucStatus rsGucHeaders status headers diff --git a/test/spec/Feature/Query/MultipleSchemaSpec.hs b/test/spec/Feature/Query/MultipleSchemaSpec.hs index 0f726a27ee4..deaa5954574 100644 --- a/test/spec/Feature/Query/MultipleSchemaSpec.hs +++ b/test/spec/Feature/Query/MultipleSchemaSpec.hs @@ -225,13 +225,11 @@ spec = it "succeeds on PUT on the v2 schema" $ request methodPut "/children?id=eq.111" [("Content-Profile", "v2"), ("Prefer", "return=representation")] - [json| [ { "id": 111, "name": "child v2-111", "parent_id": null } ]|] + [json|[{"id": 111, "name": "child v2-111", "parent_id": null}]|] `shouldRespondWith` - [json|[{ "id": 111, "name": "child v2-111", "parent_id": null }]|] - { - matchStatus = 200 - , matchHeaders = [matchContentTypeJson, "Content-Profile" <:> "v2"] - } + [json|[{"id": 111, "name": "child v2-111", "parent_id": null}]|] + { matchStatus = 201 + , matchHeaders = [matchContentTypeJson, "Content-Profile" <:> "v2"]} context "OpenAPI output" $ do it "succeeds in reading table definition from default schema v1 if no schema is selected via header" $ do diff --git a/test/spec/Feature/Query/PlanSpec.hs b/test/spec/Feature/Query/PlanSpec.hs index 8f53c3237d5..7300ed7e99e 100644 --- a/test/spec/Feature/Query/PlanSpec.hs +++ b/test/spec/Feature/Query/PlanSpec.hs @@ -192,7 +192,7 @@ spec actualPgVersion = do liftIO $ do resHeaders `shouldSatisfy` elem ("Content-Type", "application/vnd.pgrst.plan+json; for=\"application/json\"; charset=utf-8") resStatus `shouldBe` Status { statusCode = 200, statusMessage="OK" } - totalCost `shouldBe` 1.29 + totalCost `shouldBe` 3.55 it "outputs the total cost for 2 upserts" $ do r <- request methodPost "/tiobe_pls" @@ -206,7 +206,7 @@ spec actualPgVersion = do liftIO $ do resHeaders `shouldSatisfy` elem ("Content-Type", "application/vnd.pgrst.plan+json; for=\"application/json\"; charset=utf-8") resStatus `shouldBe` Status { statusCode = 200, statusMessage="OK" } - totalCost `shouldBe` 3.27 + totalCost `shouldBe` 5.53 it "outputs the total cost for an upsert with 10 rows" $ do r <- request methodPost "/tiobe_pls" @@ -220,7 +220,7 @@ spec actualPgVersion = do liftIO $ do resHeaders `shouldSatisfy` elem ("Content-Type", "application/vnd.pgrst.plan+json; for=\"application/json\"; charset=utf-8") resStatus `shouldBe` Status { statusCode = 200, statusMessage="OK" } - totalCost `shouldBe` 3.27 + totalCost `shouldBe` 5.53 it "outputs the total cost for an upsert with 100 rows" $ do r <- request methodPost "/tiobe_pls" @@ -234,7 +234,7 @@ spec actualPgVersion = do liftIO $ do resHeaders `shouldSatisfy` elem ("Content-Type", "application/vnd.pgrst.plan+json; for=\"application/json\"; charset=utf-8") resStatus `shouldBe` Status { statusCode = 200, statusMessage="OK" } - totalCost `shouldBe` 3.27 + totalCost `shouldBe` 5.53 it "outputs the total cost for an upsert with 1000 rows" $ do r <- request methodPost "/tiobe_pls" @@ -248,7 +248,7 @@ spec actualPgVersion = do liftIO $ do resHeaders `shouldSatisfy` elem ("Content-Type", "application/vnd.pgrst.plan+json; for=\"application/json\"; charset=utf-8") resStatus `shouldBe` Status { statusCode = 200, statusMessage="OK" } - totalCost `shouldBe` 3.27 + totalCost `shouldBe` 5.53 it "outputs the plan for application/vnd.pgrst.object" $ do r <- request methodDelete "/projects?id=eq.6" diff --git a/test/spec/Feature/Query/ServerTimingSpec.hs b/test/spec/Feature/Query/ServerTimingSpec.hs index b894e781494..94a2d026867 100644 --- a/test/spec/Feature/Query/ServerTimingSpec.hs +++ b/test/spec/Feature/Query/ServerTimingSpec.hs @@ -45,11 +45,11 @@ spec = } it "works with put request" $ - request methodPut "/tiobe_pls?name=eq.Go" + request methodPut "/tiobe_pls?name=eq.Python" [("Prefer", "return=representation")] - [json| [ { "name": "Go", "rank": 19 } ]|] + [json| [ { "name": "Python", "rank": 19 } ]|] `shouldRespondWith` - [json| [ { "name": "Go", "rank": 19 } ]|] + [json| [ { "name": "Python", "rank": 19 } ]|] { matchStatus = 200 , matchHeaders = map matchServerTimingHasTiming ["jwt", "plan", "query", "render"] } diff --git a/test/spec/Feature/Query/UpsertSpec.hs b/test/spec/Feature/Query/UpsertSpec.hs index b317db14371..165eada3e52 100644 --- a/test/spec/Feature/Query/UpsertSpec.hs +++ b/test/spec/Feature/Query/UpsertSpec.hs @@ -32,6 +32,21 @@ spec actualPgVersion = , matchHeaders = ["Preference-Applied" <:> "resolution=merge-duplicates, return=representation", matchContentTypeJson] } + it "UPDATEs rows on pk conflict" $ + request methodPost "/tiobe_pls" [("Prefer", "return=representation"), ("Prefer", "resolution=merge-duplicates")] + [json| [ + { "name": "Python", "rank": 6 }, + { "name": "Java", "rank": 2 }, + { "name": "C", "rank": 1 } + ]|] `shouldRespondWith` [json| [ + { "name": "Python", "rank": 6 }, + { "name": "Java", "rank": 2 }, + { "name": "C", "rank": 1 } + ]|] + { matchStatus = 200 + , matchHeaders = ["Preference-Applied" <:> "resolution=merge-duplicates, return=representation", matchContentTypeJson] + } + it "INSERTs and UPDATEs row on composite pk conflict" $ request methodPost "/employees" [("Prefer", "return=representation"), ("Prefer", "resolution=merge-duplicates")] [json| [ @@ -62,7 +77,7 @@ spec actualPgVersion = it "succeeds when the payload has no elements" $ request methodPost "/articles" [("Prefer", "return=representation"), ("Prefer", "resolution=merge-duplicates")] [json|[]|] `shouldRespondWith` - [json|[]|] { matchStatus = 201 + [json|[]|] { matchStatus = 200 -- nothing was inserted, so it should be 200 , matchHeaders = [matchContentTypeJson] } it "INSERTs and UPDATEs rows on single unique key conflict" $ @@ -282,6 +297,7 @@ spec actualPgVersion = [json| [ { "name": "Go", "rank": 19 } ]|] `shouldRespondWith` [json| [ { "name": "Go", "rank": 19 } ]|] + { matchStatus = 201 } it "succeeds on table with composite pk" $ do -- assert that the next request will indeed be an insert @@ -294,6 +310,7 @@ spec actualPgVersion = [json| [ { "first_name": "Susan", "last_name": "Heidt", "salary": "48000", "company": "GEX", "occupation": "Railroad engineer" } ]|] `shouldRespondWith` [json| [ { "first_name": "Susan", "last_name": "Heidt", "salary": "$48,000.00", "company": "GEX", "occupation": "Railroad engineer" } ]|] + { matchStatus = 201 } when (actualPgVersion >= pgVersion110) $ it "succeeds on a partitioned table with composite pk" $ do @@ -307,6 +324,7 @@ spec actualPgVersion = [json| [ { "name": "Supra", "year": 2021 } ]|] `shouldRespondWith` [json| [ { "name": "Supra", "year": 2021, "car_brand_name": null } ]|] + { matchStatus = 201 } it "succeeds if the table has only PK cols and no other cols" $ do -- assert that the next request will indeed be an insert @@ -319,6 +337,7 @@ spec actualPgVersion = [json|[ { "id": 10 } ]|] `shouldRespondWith` [json|[ { "id": 10 } ]|] + { matchStatus = 201 } context "Updating row" $ do it "succeeds on table with single pk col" $ do @@ -401,7 +420,11 @@ spec actualPgVersion = request methodPut "/tiobe_pls?name=eq.Ruby" [("Prefer", "return=representation"), ("Accept", "application/vnd.pgrst.object+json")] [json| [ { "name": "Ruby", "rank": 11 } ]|] - `shouldRespondWith` [json|{ "name": "Ruby", "rank": 11 }|] { matchHeaders = [matchContentTypeSingular] } + `shouldRespondWith` + [json|{ "name": "Ruby", "rank": 11 }|] + { matchStatus = 201 + , matchHeaders = [matchContentTypeSingular] } + context "with a camel case pk column" $ do it "works with POST and merge-duplicates" $ do diff --git a/test/spec/Feature/RollbackSpec.hs b/test/spec/Feature/RollbackSpec.hs index 40620460691..d86176575fe 100644 --- a/test/spec/Feature/RollbackSpec.hs +++ b/test/spec/Feature/RollbackSpec.hs @@ -118,11 +118,12 @@ shouldPersistMutations reqHeaders respHeaders = do it "does persist put" $ do request methodPut "/items?id=eq.0" - reqHeaders - [json|{"id":0}|] + reqHeaders + [json|{"id":0}|] `shouldRespondWith` - [json|[{"id":0}]|] - { matchHeaders = respHeaders } + [json|[{"id":0}]|] + { matchStatus = 201 + , matchHeaders = respHeaders } get "/items?id=eq.0" `shouldRespondWith` [json|[{"id":0}]|] @@ -175,7 +176,8 @@ shouldNotPersistMutations reqHeaders respHeaders = do [json|{"id":0}|] `shouldRespondWith` [json|[{"id":0}]|] - { matchHeaders = respHeaders } + { matchStatus = 201 + , matchHeaders = respHeaders } get "/items?id=eq.0" `shouldRespondWith` [json|[]|]