limited update/delete requires explicit order

* limited update/delete now works on views with explicit order
* no default order, enforce order presence
* apply row count to ensure limited mutations

CHANGELOG.md

@@ -18,10 +18,8 @@ This project adheres to [Semantic Versioning](http://semver.org/).
    + #1689, Add the ability to run without `db-anon-role` disabling anonymous access. - @wolfgangwalther
  - #1543, Allow access to fields of composite types in select=, order= and filters through JSON operators -> and ->>. - @wolfgangwalther
  - #2075, Allow access to array items in ?select=, ?order= and filters through JSON operators -> and ->>. - @wolfgangwalther
- - #2156, Allow applying `limit/offset` to UPDATE/DELETE to only affect a subset of rows - @steve-chavez
-   + Uses the table primary key, so it needs a select privilege on the primary key columns
-   + If no primary key is available, it will fallback to using the "ctid" system column(will also require a select privilege on it)
-   + Doesn't work on views and it will throw an error if tried
+ - #2156, #2211, Allow applying `limit/offset` to UPDATE/DELETE to only affect a subset of rows - @steve-chavez
+   + It requires an explicit `order` on a unique column(s)
  - #1917, Add error codes with the `"PGRST"` prefix to the error response body to differentiate PostgREST errors from PostgreSQL errors - @laurenceisla
  - #1917, Normalize the error response body by always having the `detail` and `hint` error fields with a `null` value if they are empty - @laurenceisla
  - #2176, Errors raised with `SQLSTATE` now include the message and the code in the response body - @laurenceisla

src/PostgREST/App.hs

@@ -62,8 +62,7 @@ import PostgREST.Config                  (AppConfig (..),
                                           OpenAPIMode (..))
 import PostgREST.Config.PgVersion        (PgVersion (..))
 import PostgREST.ContentType             (ContentType (..))
-import PostgREST.DbStructure             (DbStructure (..),
-                                          findIfView)
+import PostgREST.DbStructure             (DbStructure (..))
 import PostgREST.DbStructure.Identifiers (FieldName,
                                           QualifiedIdentifier (..),
                                           Schema)
@@ -347,9 +346,9 @@ handleCreate identifier@QualifiedIdentifier{..} context@RequestContext{..} = do
       response HTTP.status201 headers mempty
 
 handleUpdate :: QualifiedIdentifier -> RequestContext -> DbHandler Wai.Response
-handleUpdate identifier context@(RequestContext _ ctxDbStructure ApiRequest{..} _) = do
-  when (iTopLevelRange /= RangeQuery.allRange && findIfView identifier (dbTables ctxDbStructure)) $
-    throwError $ Error.NotImplemented "limit/offset is not implemented for views"
+handleUpdate identifier context@(RequestContext _ _ ApiRequest{..} _) = do
+  when (iTopLevelRange /= RangeQuery.allRange && null (qsOrder iQueryParams)) $
+    throwError Error.LimitNoOrderError
 
   WriteQueryResult{..} <- writeQuery MutationUpdate identifier False mempty context
 
@@ -365,11 +364,12 @@ handleUpdate identifier context@(RequestContext _ ctxDbStructure ApiRequest{..}
       RangeQuery.contentRangeH 0 (resQueryTotal - 1) $
         if shouldCount iPreferCount then Just resQueryTotal else Nothing
 
-  failNotSingular iAcceptContentType resQueryTotal $
-    if fullRepr then
-      response status (contentTypeHeaders context ++ [contentRangeHeader]) (LBS.fromStrict resBody)
-    else
-      response status [contentRangeHeader] mempty
+  failChangesOffLimits (RangeQuery.rangeLimit iTopLevelRange) resQueryTotal =<<
+    failNotSingular iAcceptContentType resQueryTotal (
+      if fullRepr then
+        response status (contentTypeHeaders context ++ [contentRangeHeader]) (LBS.fromStrict resBody)
+      else
+        response status [contentRangeHeader] mempty)
 
 handleSingleUpsert :: QualifiedIdentifier -> RequestContext-> DbHandler Wai.Response
 handleSingleUpsert identifier context@(RequestContext _ ctxDbStructure ApiRequest{..} _) = do
@@ -398,9 +398,9 @@ handleSingleUpsert identifier context@(RequestContext _ ctxDbStructure ApiReques
       response HTTP.status204 [] mempty
 
 handleDelete :: QualifiedIdentifier -> RequestContext -> DbHandler Wai.Response
-handleDelete identifier context@(RequestContext _ ctxDbStructure ApiRequest{..} _) = do
-  when (iTopLevelRange /= RangeQuery.allRange && findIfView identifier (dbTables ctxDbStructure)) $
-    throwError $ Error.NotImplemented "limit/offset is not implemented for views"
+handleDelete identifier context@(RequestContext _ _ ApiRequest{..} _) = do
+  when (iTopLevelRange /= RangeQuery.allRange && null (qsOrder iQueryParams)) $
+    throwError Error.LimitNoOrderError
 
   WriteQueryResult{..} <- writeQuery MutationDelete identifier False mempty context
 
@@ -410,13 +410,14 @@ handleDelete identifier context@(RequestContext _ ctxDbStructure ApiRequest{..}
       RangeQuery.contentRangeH 1 0 $
         if shouldCount iPreferCount then Just resQueryTotal else Nothing
 
-  failNotSingular iAcceptContentType resQueryTotal $
-    if iPreferRepresentation == Full then
-      response HTTP.status200
-        (contentTypeHeaders context ++ [contentRangeHeader])
-        (LBS.fromStrict resBody)
-    else
-      response HTTP.status204 [contentRangeHeader] mempty
+  failChangesOffLimits (RangeQuery.rangeLimit iTopLevelRange) resQueryTotal =<<
+    failNotSingular iAcceptContentType resQueryTotal (
+      if iPreferRepresentation == Full then
+        response HTTP.status200
+          (contentTypeHeaders context ++ [contentRangeHeader])
+          (LBS.fromStrict resBody)
+      else
+        response HTTP.status204 [contentRangeHeader] mempty)
 
 handleInfo :: Monad m => QualifiedIdentifier -> RequestContext -> Handler m Wai.Response
 handleInfo identifier RequestContext{..} =
@@ -584,6 +585,16 @@ failNotSingular contentType queryTotal response =
   else
     return response
 
+failChangesOffLimits :: Maybe Integer -> Int64 -> Wai.Response -> DbHandler Wai.Response
+failChangesOffLimits (Just maxChanges) queryTotal response =
+  if queryTotal > fromIntegral maxChanges
+  then do
+      lift SQL.condemn
+      throwError $ Error.OffLimitsChangesError queryTotal maxChanges
+  else
+    return response
+failChangesOffLimits _ _ response = return response
+
 shouldCount :: Maybe PreferCount -> Bool
 shouldCount preferCount =
   preferCount == Just ExactCount || preferCount == Just EstimatedCount

src/PostgREST/DbStructure.hs

@@ -23,7 +23,6 @@ module PostgREST.DbStructure
   , queryDbStructure
   , accessibleTables
   , accessibleProcs
-  , findIfView
   , schemaDescription
   ) where
 
@@ -66,9 +65,6 @@ data DbStructure = DbStructure
   }
   deriving (Generic, JSON.ToJSON)
 
-findIfView :: QualifiedIdentifier -> TablesMap -> Bool
-findIfView identifier tbls = maybe False tableIsView $ M.lookup identifier tbls
-
 -- | A view foreign key or primary key dependency detected on its source table
 data ViewKeyDependency = ViewKeyDependency {
   keyDepTable :: QualifiedIdentifier

src/PostgREST/DbStructure/Table.hs

@@ -21,8 +21,8 @@ data Table = Table
   { tableSchema      :: Schema
   , tableName        :: TableName
   , tableDescription :: Maybe Text
-    -- TODO Find a better way to separate tables and views
-  , tableIsView      :: Bool
+     -- TODO Find a better way to separate tables and views
+   , tableIsView      :: Bool
     -- The following fields identify what can be done on the table/view, they're not related to the privileges granted to it
   , tableInsertable  :: Bool
   , tableUpdatable   :: Bool

src/PostgREST/Error.hs

@@ -314,9 +314,10 @@ data Error
   | JwtTokenInvalid Text
   | JwtTokenMissing
   | JwtTokenRequired
-  | NotImplemented Text
+  | LimitNoOrderError
   | NoSchemaCacheError
   | NotFound
+  | OffLimitsChangesError Int64 Integer
   | PgErr PgError
   | PutMatchingPkError
   | PutRangeNotAllowedError
@@ -331,12 +332,13 @@ instance PgrstError Error where
   status JwtTokenInvalid{}       = HTTP.unauthorized401
   status JwtTokenMissing         = HTTP.status500
   status JwtTokenRequired        = HTTP.unauthorized401
+  status LimitNoOrderError       = HTTP.status400
   status NoSchemaCacheError      = HTTP.status503
   status NotFound                = HTTP.status404
+  status OffLimitsChangesError{} = HTTP.status400
   status (PgErr err)             = status err
   status PutMatchingPkError      = HTTP.status400
   status PutRangeNotAllowedError = HTTP.status400
-  status (NotImplemented _)      = HTTP.status501
   status SingularityError{}      = HTTP.status406
   status UnsupportedVerb{}       = HTTP.status405
 
@@ -409,10 +411,16 @@ instance JSON.ToJSON Error where
     "details" .= JSON.Null,
     "hint"    .= JSON.Null]
 
-  toJSON (NotImplemented msg) = JSON.object [
-    "code"    .= GeneralErrorCode07,
-    "message" .= msg,
+  toJSON LimitNoOrderError = JSON.object [
+    "code"    .= ApiRequestErrorCode09,
+    "message" .= ("A 'limit' was applied without an explicit 'order'":: Text),
     "details" .= JSON.Null,
+    "hint"    .= ("Apply an 'order' using unique column(s)" :: Text)]
+
+  toJSON (OffLimitsChangesError n maxs) = JSON.object [
+    "code"    .= ApiRequestErrorCode10,
+    "message" .= ("The maximum number of rows allowed to change was surpassed" :: Text),
+    "details" .= T.unwords ["Results contain", show n, "rows changed but the maximum number allowed is", show maxs],
     "hint"    .= JSON.Null]
 
   toJSON NotFound = JSON.object []
@@ -445,6 +453,8 @@ data ErrorCode
   | ApiRequestErrorCode06
   | ApiRequestErrorCode07
   | ApiRequestErrorCode08
+  | ApiRequestErrorCode09
+  | ApiRequestErrorCode10
   -- Schema Cache errors
   | SchemaCacheErrorCode00
   | SchemaCacheErrorCode01
@@ -468,7 +478,6 @@ data ErrorCode
   | GeneralErrorCode04
   | GeneralErrorCode05
   | GeneralErrorCode06
-  | GeneralErrorCode07
 
 instance JSON.ToJSON ErrorCode where
   toJSON e = JSON.toJSON (buildErrorCode e)
@@ -490,6 +499,8 @@ buildErrorCode code = "PGRST" <> case code of
   ApiRequestErrorCode06  -> "106"
   ApiRequestErrorCode07  -> "107"
   ApiRequestErrorCode08  -> "108"
+  ApiRequestErrorCode09  -> "109"
+  ApiRequestErrorCode10  -> "110"
 
   SchemaCacheErrorCode00 -> "200"
   SchemaCacheErrorCode01 -> "201"
@@ -513,4 +524,3 @@ buildErrorCode code = "PGRST" <> case code of
   GeneralErrorCode04     -> "504"
   GeneralErrorCode05     -> "505"
   GeneralErrorCode06     -> "506"
-  GeneralErrorCode07     -> "507"

src/PostgREST/Query/QueryBuilder.hs

@@ -39,9 +39,10 @@ readRequestToQuery (Node (Select colSelects mainQi tblAlias implJoins logicFores
   intercalateSnippet ", " ((pgFmtSelectItem qi <$> colSelects) ++ selects) <>
   "FROM " <> SQL.sql (BS.intercalate ", " (tabl : implJs)) <> " " <>
   intercalateSnippet " " joins <> " " <>
-  (if null logicForest && null joinConditions_ then mempty else "WHERE " <> intercalateSnippet " AND " (map (pgFmtLogicTree qi) logicForest ++ map pgFmtJoinCondition joinConditions_))
-  <> " " <>
-  (if null ordts then mempty else "ORDER BY " <> intercalateSnippet ", " (map (pgFmtOrderTerm qi) ordts)) <> " " <>
+  (if null logicForest && null joinConditions_
+    then mempty
+    else "WHERE " <> intercalateSnippet " AND " (map (pgFmtLogicTree qi) logicForest ++ map pgFmtJoinCondition joinConditions_)) <> " " <>
+  orderF qi ordts <> " " <>
   limitOffsetF range
   where
     implJs = fromQi <$> implJoins
@@ -106,7 +107,7 @@ 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)
+mutateRequestToQuery (Update mainQi uCols body logicForest range ordts 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=
@@ -125,8 +126,9 @@ mutateRequestToQuery (Update mainQi uCols body logicForest (range, rangeId) retu
     "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 <>
+      whereLogic <> " " <>
+      orderF mainQi ordts <> " " <>
+      limitOffsetF range <>
     ") " <>
     "UPDATE " <> mainTbl <> " SET " <> SQL.sql rangeCols <>
     "FROM pgrst_affected_rows " <>
@@ -139,9 +141,9 @@ mutateRequestToQuery (Update mainQi uCols body logicForest (range, rangeId) retu
     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
+    (whereRangeIdF, rangeIdF) = mutRangeF mainQi (fst . otTerm <$> ordts)
 
-mutateRequestToQuery (Delete mainQi logicForest (range, rangeId) returnings)
+mutateRequestToQuery (Delete mainQi logicForest range ordts returnings)
   | range == allRange =
     "DELETE FROM " <> SQL.sql (fromQi mainQi) <> " " <>
     whereLogic <> " " <>
@@ -152,7 +154,8 @@ mutateRequestToQuery (Delete mainQi logicForest (range, rangeId) returnings)
     "pgrst_affected_rows AS (" <>
       "SELECT " <> SQL.sql rangeIdF <> " FROM " <> SQL.sql (fromQi mainQi) <>
        whereLogic <> " " <>
-      "ORDER BY " <> SQL.sql rangeIdF <> " " <> limitOffsetF range <>
+      orderF mainQi ordts <> " " <>
+      limitOffsetF range <>
     ") " <>
     "DELETE FROM " <> SQL.sql (fromQi mainQi) <> " " <>
     "USING pgrst_affected_rows " <>
@@ -161,7 +164,7 @@ mutateRequestToQuery (Delete mainQi logicForest (range, rangeId) returnings)
 
   where
     whereLogic = if null logicForest then mempty else " WHERE " <> intercalateSnippet " AND " (pgFmtLogicTree mainQi <$> logicForest)
-    (whereRangeIdF, rangeIdF) = mutRangeF mainQi rangeId
+    (whereRangeIdF, rangeIdF) = mutRangeF mainQi (fst . otTerm <$> ordts)
 
 requestToCallProcQuery :: CallRequest -> SQL.Snippet
 requestToCallProcQuery (FunctionCall qi params args returnsScalar multipleCall returnings) =

src/PostgREST/Query/SqlFragment.hs

@@ -20,6 +20,7 @@ module PostgREST.Query.SqlFragment
   , locationF
   , mutRangeF
   , normalizedBody
+  , orderF
   , pgFmtColumn
   , pgFmtIdent
   , pgFmtJoinCondition
@@ -331,6 +332,17 @@ currentSettingF setting =
   -- nullif is used because of https://gist.github.com/steve-chavez/8d7033ea5655096903f3b52f8ed09a15
   "nullif(current_setting('" <> setting <> "', true), '')"
 
+mutRangeF :: QualifiedIdentifier -> [FieldName] -> (SqlFragment, SqlFragment)
+mutRangeF mainQi rangeId =
+  (
+    BS.intercalate " AND " $ (\col -> pgFmtColumn mainQi col <> " = " <> pgFmtColumn (QualifiedIdentifier mempty "pgrst_affected_rows") col) <$> rangeId
+  , BS.intercalate ", " (pgFmtColumn mainQi <$> rangeId)
+  )
+
+orderF :: QualifiedIdentifier -> [OrderTerm] -> SQL.Snippet
+orderF _ []    = mempty
+orderF qi ordts = "ORDER BY " <> intercalateSnippet ", " (pgFmtOrderTerm qi <$> ordts)
+
 -- Hasql Snippet utilities
 unknownEncoder :: ByteString -> SQL.Snippet
 unknownEncoder = SQL.encoderAndParam (HE.nonNullable HE.unknown)
@@ -341,12 +353,3 @@ 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)
-  )

src/PostgREST/Request/DbRequestBuilder.hs

@@ -258,7 +258,9 @@ addFilters ApiRequest{..} rReq =
 
 addOrders :: ApiRequest -> ReadRequest -> Either ApiRequestError ReadRequest
 addOrders ApiRequest{..} rReq =
-  foldr addOrderToNode (Right rReq) qsOrder
+  case iAction of
+    ActionMutate _ -> Right rReq
+    _              -> foldr addOrderToNode (Right rReq) qsOrder
   where
     QueryParams.QueryParams{..} = iQueryParams
 
@@ -305,7 +307,7 @@ mutateRequest mutation schema tName ApiRequest{..} pkCols readReq = mapLeft ApiR
   case mutation of
     MutationCreate ->
       Right $ Insert qi iColumns body ((,) <$> iPreferResolution <*> Just confCols) [] returnings
-    MutationUpdate -> Right $ Update qi iColumns body combinedLogic (iTopLevelRange, pkCols) returnings
+    MutationUpdate -> Right $ Update qi iColumns body combinedLogic iTopLevelRange rootOrder returnings
     MutationSingleUpsert ->
         if null qsLogic &&
            qsFilterFields == S.fromList pkCols &&
@@ -316,7 +318,7 @@ mutateRequest mutation schema tName ApiRequest{..} pkCols readReq = mapLeft ApiR
           then Right $ Insert qi iColumns body (Just (MergeDuplicates, pkCols)) combinedLogic returnings
         else
           Left InvalidFilters
-    MutationDelete -> Right $ Delete qi combinedLogic (iTopLevelRange, pkCols) returnings
+    MutationDelete -> Right $ Delete qi combinedLogic iTopLevelRange rootOrder returnings
   where
     confCols = fromMaybe pkCols qsOnConflict
     QueryParams.QueryParams{..} = iQueryParams
@@ -328,6 +330,7 @@ mutateRequest mutation schema tName ApiRequest{..} pkCols readReq = mapLeft ApiR
     -- update/delete filters can be only on the root table
     filters = map snd qsFiltersRoot
     logic = map snd qsLogic
+    rootOrder = maybe [] snd $ find (\(x, _) -> null x) qsOrder
     combinedLogic = foldr addFilterToLogicForest logic filters
     body = payRaw <$> iPayload -- the body is assumed to be json at this stage(ApiRequest validates)
 

src/PostgREST/Request/Types.hs

@@ -135,13 +135,15 @@ data MutateQuery
       , updCols   :: S.Set FieldName
       , updBody   :: Maybe LBS.ByteString
       , where_    :: [LogicTree]
-      , mutRange  :: (NonnegRange, [FieldName])
+      , mutRange  :: NonnegRange
+      , mutOrder  :: [OrderTerm]
       , returning :: [FieldName]
       }
   | Delete
       { in_       :: QualifiedIdentifier
       , where_    :: [LogicTree]
-      , mutRange  :: (NonnegRange, [FieldName])
+      , mutRange  :: NonnegRange
+      , mutOrder  :: [OrderTerm]
       , returning :: [FieldName]
       }