Skip to content

Commit

Permalink
fix: PUT responding with 200 when rows are inserted
Browse files Browse the repository at this point in the history
  • Loading branch information
taimoorzaeem committed Sep 4, 2023
1 parent 07fef25 commit 479f924
Show file tree
Hide file tree
Showing 8 changed files with 48 additions and 15 deletions.
3 changes: 2 additions & 1 deletion src/PostgREST/Query.hs
Original file line number Diff line number Diff line change
Expand Up @@ -187,13 +187,14 @@ openApiQuery sCache pgVer AppConfig{..} tSchema =
writeQuery :: MutateReadPlan -> ApiRequest -> AppConfig -> DbHandler ResultSet
writeQuery MutateReadPlan{mrReadPlan, mrMutatePlan, mrResAgg} apiReq@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
(iAcceptMediaType apiReq)
mrResAgg
preferRepresentation
Expand Down
6 changes: 3 additions & 3 deletions src/PostgREST/Query/QueryBuilder.hs
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ 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 " <> " 'inserted' = " <> setConfigLocal mempty ("pgrst.mutation","inserted") <> " AND " <> intercalateSnippet " AND " (pgFmtLogicTree (QualifiedIdentifier mempty "pgrst_body") <$> putConditions)) <>
maybe mempty (\(oncDo, oncCols) ->
if null oncCols then
mempty
Expand All @@ -98,9 +98,9 @@ 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 then mempty else "WHERE " <> " 'updated' = " <> setConfigLocal mempty ("pgrst.mutation","updated"))
) onConflct <> " " <>
returningF mainQi returnings
returningF mainQi returnings
where
cols = intercalateSnippet ", " $ pgFmtIdent . cfName <$> iCols

Expand Down
9 changes: 9 additions & 0 deletions src/PostgREST/Query/SqlFragment.hs
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,9 @@ module PostgREST.Query.SqlFragment
, fromJsonBodyF
, responseHeadersF
, responseStatusF
, pgrstInsertedF
, pgrstUpdatedF
, currentSettingF
, returningF
, singleParameter
, sourceCTE
Expand Down Expand Up @@ -438,6 +441,12 @@ responseHeadersF = currentSettingF "response.headers"
responseStatusF :: SQL.Snippet
responseStatusF = currentSettingF "response.status"

pgrstInsertedF :: SQL.Snippet
pgrstInsertedF = "count(nullif(current_setting('pgrst.mutation'), 'inserted'))"

pgrstUpdatedF :: SQL.Snippet
pgrstUpdatedF = "count(nullif(current_setting('pgrst.mutation'), 'updated'))"

currentSettingF :: SQL.Snippet -> SQL.Snippet
currentSettingF setting =
-- nullif is used because of https://gist.github.com/steve-chavez/8d7033ea5655096903f3b52f8ed09a15
Expand Down
27 changes: 20 additions & 7 deletions src/PostgREST/Query/Statements.hs
Original file line number Diff line number Diff line change
Expand Up @@ -49,15 +49,20 @@ 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

Check warning on line 52 in src/PostgREST/Query/Statements.hs

View check run for this annotation

Codecov / codecov/patch

src/PostgREST/Query/Statements.hs#L52

Added line #L52 was not covered by tests
-- ^ the number of rows inserted (Only used with PUT)
, rsUpdated :: Maybe Int64

Check warning on line 54 in src/PostgREST/Query/Statements.hs

View check run for this annotation

Codecov / codecov/patch

src/PostgREST/Query/Statements.hs#L54

Added line #L54 was not covered by tests
-- ^ the number of rows updated (Only used with PUT)
}
| RSPlan BS.ByteString -- ^ the plan of the query


prepareWrite :: SQL.Snippet -> SQL.Snippet -> Bool -> MediaType -> ResultAggregate ->
prepareWrite :: SQL.Snippet -> SQL.Snippet -> Bool -> Bool -> MediaType -> ResultAggregate ->
Maybe PreferRepresentation -> [Text] -> Bool -> SQL.Statement () ResultSet
prepareWrite selectQuery mutateQuery isInsert mt rAgg rep pKeys =
prepareWrite selectQuery mutateQuery isInsert isPut mt rAgg rep pKeys =
SQL.dynamicallyParameterized (mtSnippet mt snippet) decodeIt
where
checkUpsert snip = if isInsert && isPut then snip else "''"
snippet =
"WITH " <> sourceCTE <> " AS (" <> mutateQuery <> ") " <>
"SELECT " <>
Expand All @@ -66,7 +71,9 @@ 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, " <>
checkUpsert pgrstInsertedF <> " AS response_inserted, " <>
checkUpsert pgrstUpdatedF <> " AS response_updated " <>
"FROM (" <> selectF <> ") _postgrest_t"

locF =
Expand All @@ -86,7 +93,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 Nothing) <$> HD.rowMaybe (standardRow False)

prepareRead :: SQL.Snippet -> SQL.Snippet -> Bool -> MediaType -> ResultAggregate -> Bool -> SQL.Statement () ResultSet
prepareRead selectQuery countQuery countTotal mt rAgg =
Expand All @@ -100,7 +107,9 @@ 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, " <>
"''" <> " AS response_updated " <>
"FROM ( SELECT * FROM " <> sourceCTE <> " ) _postgrest_t"

(countCTEF, countResultF) = countF countQuery countTotal
Expand All @@ -124,15 +133,17 @@ prepareCall rout callProcQuery selectQuery countQuery countTotal mt rAgg =
"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, " <>
"''" <> " AS response_updated " <>
"FROM (" <> selectQuery <> ") _postgrest_t"

(countCTEF, countResultF) = countF countQuery countTotal

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 Nothing) <$> HD.rowMaybe (standardRow True)

preparePlanRows :: SQL.Snippet -> Bool -> SQL.Statement () (Maybe Int64)
preparePlanRows countQuery =
Expand All @@ -150,6 +161,8 @@ 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
<*> nullableColumn HD.int8
where
splitKeyValue :: ByteString -> (ByteString, ByteString)
splitKeyValue kv =
Expand Down
6 changes: 5 additions & 1 deletion src/PostgREST/Response.hs
Original file line number Diff line number Diff line change
Expand Up @@ -153,9 +153,13 @@ singleUpsertResponse ctxApiRequest@ApiRequest{iPreferences=Preferences{..}} resu
RSStandard {..} -> do
let
response = gucResponse rsGucStatus rsGucHeaders
returnStatus =
case (rsInserted, rsUpdated) of
(Just i, Just u) -> if i > u then HTTP.status201 else HTTP.status200
_ -> HTTP.status200

Check warning on line 159 in src/PostgREST/Response.hs

View check run for this annotation

Codecov / codecov/patch

src/PostgREST/Response.hs#L159

Added line #L159 was not covered by tests

case preferRepresentation of
Just Full -> response HTTP.status200 (contentTypeHeaders ctxApiRequest ++ [toAppliedHeader Full]) (LBS.fromStrict rsBody)
Just Full -> response returnStatus (contentTypeHeaders ctxApiRequest ++ [toAppliedHeader Full]) (LBS.fromStrict rsBody)
Just None -> response HTTP.status204 [toAppliedHeader None] mempty
_ -> response HTTP.status204 [] mempty

Expand Down
2 changes: 1 addition & 1 deletion test/spec/Feature/Query/PlanSpec.hs
Original file line number Diff line number Diff line change
Expand Up @@ -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` 1.81

it "outputs the plan for application/vnd.pgrst.object" $ do
r <- request methodDelete "/projects?id=eq.6"
Expand Down
2 changes: 1 addition & 1 deletion test/spec/Feature/Query/PostGISSpec.hs
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,7 @@ spec actualPgVersion = describe "PostGIS features" $
}
]
}|]
{ matchStatus = 200
{ matchStatus = 201
, matchHeaders = ["Content-Type" <:> "application/geo+json; charset=utf-8"]
}

Expand Down
8 changes: 7 additions & 1 deletion test/spec/Feature/Query/UpsertSpec.hs
Original file line number Diff line number Diff line change
Expand Up @@ -332,6 +332,7 @@ spec actualPgVersion =
[json| [ { "name": "Java", "rank": 13 } ]|]
`shouldRespondWith`
[json| [ { "name": "Java", "rank": 13 } ]|]
{ matchStatus = 201 }

-- TODO: move this to SingularSpec?
it "succeeds if the payload has more than one row, but it only puts the first element" $ do
Expand All @@ -345,7 +346,8 @@ spec actualPgVersion =
[json| [ { "name": "Java", "rank": 19 }, { "name": "Swift", "rank": 12 } ] |]
`shouldRespondWith`
[json|{ "name": "Java", "rank": 19 }|]
{ matchHeaders = [matchContentTypeSingular] }
{ matchStatus = 201
, matchHeaders = [matchContentTypeSingular] }

it "succeeds on table with composite pk" $ do
-- assert that the next request will indeed be an update
Expand All @@ -358,6 +360,7 @@ spec actualPgVersion =
[json| [ { "first_name": "Frances M.", "last_name": "Roe", "salary": "60000", "company": "Gamma Gas", "occupation": "Railroad engineer" } ]|]
`shouldRespondWith`
[json| [ { "first_name": "Frances M.", "last_name": "Roe", "salary": "$60,000.00", "company": "Gamma Gas", "occupation": "Railroad engineer" } ]|]
{ matchStatus = 201 }

when (actualPgVersion >= pgVersion110) $
it "succeeds on a partitioned table with composite pk" $ do
Expand All @@ -371,6 +374,7 @@ spec actualPgVersion =
[json| [ { "name": "DeLorean", "year": 1981, "car_brand_name": null } ]|]
`shouldRespondWith`
[json| [ { "name": "DeLorean", "year": 1981, "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 update
Expand All @@ -383,6 +387,7 @@ spec actualPgVersion =
[json|[ { "id": 1 } ]|]
`shouldRespondWith`
[json|[ { "id": 1 } ]|]
{ matchStatus = 201 }

it "ignores the Range header" $ do
-- assert that the next request will indeed be an update
Expand All @@ -395,6 +400,7 @@ spec actualPgVersion =
[json| [ { "name": "Java", "rank": 5 } ]|]
`shouldRespondWith`
[json| [ { "name": "Java", "rank": 5 } ]|]
{ matchStatus = 201 }

-- TODO: move this to SingularSpec?
it "works with return=representation and vnd.pgrst.object+json" $
Expand Down

0 comments on commit 479f924

Please sign in to comment.