Add limited UPDATE/DELETE(tables only) #2195
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -29,6 +29,7 @@ import PostgREST.DbStructure.Table (Table (..)) | |
| import PostgREST.Request.Preferences (PreferResolution (..)) | ||
|
|
||
| import PostgREST.Query.SqlFragment | ||
| import PostgREST.RangeQuery (allRange) | ||
| import PostgREST.Request.Types | ||
|
|
||
| import Protolude | ||
|
|
@@ -105,28 +106,63 @@ mutateRequestToQuery (Insert mainQi iCols body onConflct putConditions returning | |
| ]) | ||
| where | ||
| cols = BS.intercalate ", " $ pgFmtIdent <$> S.toList iCols | ||
|
|
||
| mutateRequestToQuery (Update mainQi uCols body logicForest (range, rangeId) returnings) | ||
| | S.null uCols = | ||
| -- 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 | ||
| SQL.sql $ "SELECT " <> emptyBodyReturnedColumns <> " FROM " <> fromQi mainQi <> " WHERE false" | ||
|
|
||
| | range == allRange = | ||
| "WITH " <> normalizedBody body <> " " <> | ||
| "UPDATE " <> mainTbl <> " SET " <> SQL.sql nonRangeCols <> " " <> | ||
| "FROM (SELECT * FROM json_populate_recordset (null::" <> mainTbl <> " , " <> SQL.sql selectBody <> " )) _ " <> | ||
| whereLogic <> " " <> | ||
| SQL.sql (returningF mainQi returnings) | ||
|
|
||
| | otherwise = | ||
| "WITH " <> normalizedBody body <> ", " <> | ||
| "pgrst_update_body AS (SELECT * FROM json_populate_recordset (null::" <> mainTbl <> " , " <> SQL.sql selectBody <> " ) LIMIT 1), " <> | ||
| "pgrst_affected_rows AS (" <> | ||
| "SELECT " <> SQL.sql rangeIdF <> " FROM " <> mainTbl <> | ||
| whereLogic <> " " <> | ||
| "ORDER BY " <> SQL.sql rangeIdF <> " " <> limitOffsetF range <> | ||
|
There was a problem hiding this comment. Do we need to order this by There was a problem hiding this comment.
Yes, as mentioned in the crunchy data blog post:
If the client doesn't specify an I guess we could replace the implicit There was a problem hiding this comment. I'd say we just use the |
||
| ") " <> | ||
| "UPDATE " <> mainTbl <> " SET " <> SQL.sql rangeCols <> | ||
| "FROM pgrst_affected_rows " <> | ||
| "WHERE " <> SQL.sql whereRangeIdF <> " " <> | ||
| SQL.sql (returningF mainQi returnings) | ||
|
|
||
| where | ||
| whereLogic = if null logicForest then mempty else " WHERE " <> intercalateSnippet " AND " (pgFmtLogicTree mainQi <$> logicForest) | ||
| mainTbl = SQL.sql (fromQi mainQi) | ||
| emptyBodyReturnedColumns = if null returnings then "NULL" else BS.intercalate ", " (pgFmtColumn (QualifiedIdentifier mempty $ qiName mainQi) <$> returnings) | ||
| nonRangeCols = BS.intercalate ", " (pgFmtIdent <> const " = _." <> pgFmtIdent <$> S.toList uCols) | ||
| rangeCols = BS.intercalate ", " ((\col -> pgFmtIdent col <> " = (SELECT " <> pgFmtIdent col <> " FROM pgrst_update_body) ") <$> S.toList uCols) | ||
| (whereRangeIdF, rangeIdF) = mutRangeF mainQi rangeId | ||
|
|
||
| mutateRequestToQuery (Delete mainQi logicForest (range, rangeId) returnings) | ||
| | range == allRange = | ||
| "DELETE FROM " <> SQL.sql (fromQi mainQi) <> " " <> | ||
| whereLogic <> " " <> | ||
| SQL.sql (returningF mainQi returnings) | ||
|
|
||
| | otherwise = | ||
| "WITH " <> | ||
| "pgrst_affected_rows AS (" <> | ||
| "SELECT " <> SQL.sql rangeIdF <> " FROM " <> SQL.sql (fromQi mainQi) <> | ||
| whereLogic <> " " <> | ||
| "ORDER BY " <> SQL.sql rangeIdF <> " " <> limitOffsetF range <> | ||
| ") " <> | ||
| "DELETE FROM " <> SQL.sql (fromQi mainQi) <> " " <> | ||
| "USING pgrst_affected_rows " <> | ||
| "WHERE " <> SQL.sql whereRangeIdF <> " " <> | ||
| SQL.sql (returningF mainQi returnings) | ||
|
|
||
| where | ||
| whereLogic = if null logicForest then mempty else " WHERE " <> intercalateSnippet " AND " (pgFmtLogicTree mainQi <$> logicForest) | ||
| (whereRangeIdF, rangeIdF) = mutRangeF mainQi rangeId | ||
|
|
||
| requestToCallProcQuery :: CallRequest -> SQL.Snippet | ||
| requestToCallProcQuery (FunctionCall qi params args returnsScalar multipleCall returnings) = | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,6 +17,7 @@ module PostgREST.Query.SqlFragment | |
| , fromQi | ||
| , limitOffsetF | ||
| , locationF | ||
| , mutRangeF | ||
| , normalizedBody | ||
| , pgFmtColumn | ||
| , pgFmtIdent | ||
|
|
@@ -334,3 +335,12 @@ unknownLiteral = unknownEncoder . encodeUtf8 | |
| intercalateSnippet :: ByteString -> [SQL.Snippet] -> SQL.Snippet | ||
| intercalateSnippet _ [] = mempty | ||
| intercalateSnippet frag snippets = foldr1 (\a b -> a <> SQL.sql frag <> b) snippets | ||
|
|
||
| -- the "ctid" system column is always available to tables | ||
| mutRangeF :: QualifiedIdentifier -> [FieldName] -> (SqlFragment, SqlFragment) | ||
| mutRangeF mainQi rangeId = ( | ||
| BS.intercalate " AND " $ | ||
| (\col -> pgFmtColumn mainQi col <> " = " <> pgFmtColumn (QualifiedIdentifier mempty "pgrst_affected_rows") col) <$> | ||
| (if null rangeId then ["ctid"] else rangeId) | ||
| , if null rangeId then pgFmtColumn mainQi "ctid" else BS.intercalate ", " (pgFmtColumn mainQi <$> rangeId) | ||
| ) | ||
steve-chavez marked this conversation as resolved.
Show resolved
Hide resolved
Comment on lines
+338
to
+346
There was a problem hiding this comment. With the current implementation, you don't need pk columns at all. You can just do it with the There was a problem hiding this comment. Given what I mentioned below, it does make a lot of sense to order by INSERT INTO projects VALUES (1, 'Windows 7', 1);
INSERT INTO projects VALUES (2, 'Windows 10', 1);
INSERT INTO projects VALUES (5, 'Orphan', NULL);
INSERT INTO projects VALUES (4, 'OSX', 2);
INSERT INTO projects VALUES (3, 'IOS', 2);Gives through http: GET /projects
[{"id":1,"name":"Windows 7","client_id":1},
{"id":2,"name":"Windows 10","client_id":1},
{"id":5,"name":"Orphan","client_id":null},
{"id":4,"name":"OSX","client_id":2},
{"id":3,"name":"IOS","client_id":2}]Which is the same as doing There was a problem hiding this comment. So basically with There was a problem hiding this comment. The only thing bad about preferring There was a problem hiding this comment. So because of the above - the need to do an special Later on, it would also be weird to tell users to expose There was a problem hiding this comment.
Forgot about that, so I can't suggest ordering by a pk because a view might have more rows than pk values as in the example you showed here. Got it.
Ah, yes I understand the mechanism however the "implicit" part got me thinking into a server side default for That would basically solve the "postgrest let's you delete all your rows by default" problem - sometimes some users make the mistake to send deletes without filters, this won't stop even with So I think enforcing a There was a problem hiding this comment. I see. A server-side default, which is overridable from the client-side, would be completely unrelated to this issue here, though. The implicit row-count here would just be to avoid mistakes in the There was a problem hiding this comment.
@wolfgangwalther Now that I've realized, this would mean that So how about this. I'll go ahead and apply the (I do see There was a problem hiding this comment. And now that I think about it. If we enforce So the above problem would be no more? I mean because we'd be already fulfilling our part of the contract - the affected rows would not exceed the There was a problem hiding this comment.
Ok.
I think it would still have very odd interactions with |
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this change always adds a
WHEREclause to theUPDATEquery - even if the request is completely unconditional.This breaks using
pg-safeupdate. The request now goes through instead of failing because of a previously unconditionalUPDATEquery.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that
pg-safeupdateis not required anymore because we're disallowing unfiltered patch/unfiltered delete unless alimitis applied.Yeah, because of the above, if no filter is added then
WHERE falseis done.One thing better with the
pg-safeupdatesafeguard is that it fails with an error while theWHERE falsewill not do anything.That's only the case of unfiltered UPDATE though. For unfiltered DELETE, we're going to fail with an error.
(I need to go back to this one because it will be inconsistent otherwise)
Would we need to keep compatibility with
pg-safeupdatenow that we have this? Why?Not sure but maybe we could do
LIMIT 0instead ofWHERE false.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, hell. I thought this was a review on #2311.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressing this in #2405
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the change in this PR still allows the full table update without conditionals. The one that disallowed this was #2311 if I'm not mistaken.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Compare #2418