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
* move requiring order to ApiRequest

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
+ - #2156, #2211, 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
+   + It requires an explicit `order` on a unique column(s)
-   + 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
  - #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 (..),
+import PostgREST.DbStructure             (DbStructure (..))
-                                          findIfView)
 import PostgREST.DbStructure.Identifiers (FieldName,
                                           QualifiedIdentifier (..),
                                           Schema)
@@ -347,10 +346,7 @@ handleCreate identifier@QualifiedIdentifier{..} context@RequestContext{..} = do
       response HTTP.status201 headers mempty
 
 handleUpdate :: QualifiedIdentifier -> RequestContext -> DbHandler Wai.Response
-handleUpdate identifier context@(RequestContext _ ctxDbStructure ApiRequest{..} _) = do
+handleUpdate identifier context@(RequestContext _ _ ApiRequest{..} _) = do
-  when (iTopLevelRange /= RangeQuery.allRange && findIfView identifier (dbTables ctxDbStructure)) $
-    throwError $ Error.NotImplemented "limit/offset is not implemented for views"
-
   WriteQueryResult{..} <- writeQuery MutationUpdate identifier False mempty context
 
   let
@@ -365,11 +361,12 @@ handleUpdate identifier context@(RequestContext _ ctxDbStructure ApiRequest{..}
       RangeQuery.contentRangeH 0 (resQueryTotal - 1) $
         if shouldCount iPreferCount then Just resQueryTotal else Nothing
 
-  failNotSingular iAcceptContentType resQueryTotal $
+  failChangesOffLimits (RangeQuery.rangeLimit iTopLevelRange) resQueryTotal =<<
-    if fullRepr then
+    failNotSingular iAcceptContentType resQueryTotal (
-      response status (contentTypeHeaders context ++ [contentRangeHeader]) (LBS.fromStrict resBody)
+      if fullRepr then
-    else
+        response status (contentTypeHeaders context ++ [contentRangeHeader]) (LBS.fromStrict resBody)
-      response status [contentRangeHeader] mempty
+      else
+        response status [contentRangeHeader] mempty)
 
 handleSingleUpsert :: QualifiedIdentifier -> RequestContext-> DbHandler Wai.Response
 handleSingleUpsert identifier context@(RequestContext _ ctxDbStructure ApiRequest{..} _) = do
@@ -398,10 +395,7 @@ handleSingleUpsert identifier context@(RequestContext _ ctxDbStructure ApiReques
       response HTTP.status204 [] mempty
 
 handleDelete :: QualifiedIdentifier -> RequestContext -> DbHandler Wai.Response
-handleDelete identifier context@(RequestContext _ ctxDbStructure ApiRequest{..} _) = do
+handleDelete identifier context@(RequestContext _ _ ApiRequest{..} _) = do
-  when (iTopLevelRange /= RangeQuery.allRange && findIfView identifier (dbTables ctxDbStructure)) $
-    throwError $ Error.NotImplemented "limit/offset is not implemented for views"
-
   WriteQueryResult{..} <- writeQuery MutationDelete identifier False mempty context
 
   let
@@ -410,13 +404,14 @@ handleDelete identifier context@(RequestContext _ ctxDbStructure ApiRequest{..}
       RangeQuery.contentRangeH 1 0 $
         if shouldCount iPreferCount then Just resQueryTotal else Nothing
 
-  failNotSingular iAcceptContentType resQueryTotal $
+  failChangesOffLimits (RangeQuery.rangeLimit iTopLevelRange) resQueryTotal =<<
-    if iPreferRepresentation == Full then
+    failNotSingular iAcceptContentType resQueryTotal (
-      response HTTP.status200
+      if iPreferRepresentation == Full then
-        (contentTypeHeaders context ++ [contentRangeHeader])
+        response HTTP.status200
-        (LBS.fromStrict resBody)
+          (contentTypeHeaders context ++ [contentRangeHeader])
-    else
+          (LBS.fromStrict resBody)
-      response HTTP.status204 [contentRangeHeader] mempty
+      else
+        response HTTP.status204 [contentRangeHeader] mempty)
 
 handleInfo :: Monad m => QualifiedIdentifier -> RequestContext -> Handler m Wai.Response
 handleInfo identifier RequestContext{..} =
@@ -584,6 +579,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
+     -- TODO Find a better way to separate tables and views
-  , tableIsView      :: Bool
+   , 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

@@ -67,6 +67,7 @@ instance PgrstError ApiRequestError where
   status ParseRequestError{}   = HTTP.status400
   status QueryParamError{}     = HTTP.status400
   status UnacceptableSchema{}  = HTTP.status406
+  status LimitNoOrderError     = HTTP.status400
 
   headers _ = [ContentType.toHeader CTApplicationJSON]
 
@@ -117,6 +118,12 @@ instance JSON.ToJSON ApiRequestError where
     "details" .= JSON.Null,
     "hint"    .= ("Verify that '" <> resource <> "' is included in the 'select' query parameter." :: Text)]
 
+  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 (NoRelBetween parent child schema) = JSON.object [
     "code"    .= SchemaCacheErrorCode00,
     "message" .= ("Could not find a relationship between '" <> parent <> "' and '" <> child <> "' in the schema cache" :: Text),
@@ -314,9 +321,9 @@ data Error
   | JwtTokenInvalid Text
   | JwtTokenMissing
   | JwtTokenRequired
-  | NotImplemented Text
   | NoSchemaCacheError
   | NotFound
+  | OffLimitsChangesError Int64 Integer
   | PgErr PgError
   | PutMatchingPkError
   | PutRangeNotAllowedError
@@ -333,10 +340,10 @@ instance PgrstError Error where
   status JwtTokenRequired        = HTTP.unauthorized401
   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 +416,10 @@ instance JSON.ToJSON Error where
     "details" .= JSON.Null,
     "hint"    .= JSON.Null]
 
-  toJSON (NotImplemented msg) = JSON.object [
+  toJSON (OffLimitsChangesError n maxs) = JSON.object [
-    "code"    .= GeneralErrorCode07,
+    "code"    .= ApiRequestErrorCode10,
-    "message" .= msg,
+    "message" .= ("The maximum number of rows allowed to change was surpassed" :: Text),
-    "details" .= JSON.Null,
+    "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 +452,8 @@ data ErrorCode
   | ApiRequestErrorCode06
   | ApiRequestErrorCode07
   | ApiRequestErrorCode08
+  | ApiRequestErrorCode09
+  | ApiRequestErrorCode10
   -- Schema Cache errors
   | SchemaCacheErrorCode00
   | SchemaCacheErrorCode01
@@ -468,7 +477,6 @@ data ErrorCode
   | GeneralErrorCode04
   | GeneralErrorCode05
   | GeneralErrorCode06
-  | GeneralErrorCode07
 
 instance JSON.ToJSON ErrorCode where
   toJSON e = JSON.toJSON (buildErrorCode e)
@@ -490,6 +498,8 @@ buildErrorCode code = "PGRST" <> case code of
   ApiRequestErrorCode06  -> "106"
   ApiRequestErrorCode07  -> "107"
   ApiRequestErrorCode08  -> "108"
+  ApiRequestErrorCode09  -> "109"
+  ApiRequestErrorCode10  -> "110"
 
   SchemaCacheErrorCode00 -> "200"
   SchemaCacheErrorCode01 -> "201"
@@ -513,4 +523,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 logicForest && null joinConditions_
-  <> " " <>
+    then mempty
-  (if null ordts then mempty else "ORDER BY " <> intercalateSnippet ", " (map (pgFmtOrderTerm qi) ordts)) <> " " <>
+    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 <> " " <>
+      whereLogic <> " " <>
-      "ORDER BY " <> SQL.sql rangeIdF <> " " <> limitOffsetF range <>
+      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/ApiRequest.hs

@@ -185,6 +185,7 @@ apiRequest conf@AppConfig{..} dbStructure req reqBody queryparams@QueryParams{..
   | isInvalidRange = Left InvalidRange
   | shouldParsePayload && isLeft payload = either (Left . InvalidBody) witness payload
   | not expectParams && not (L.null qsParams) = Left $ ParseRequestError "Unexpected param or filter missing operator" ("Failed to parse " <> show qsParams)
+  | method `elem` ["PATCH", "DELETE"] && not (null qsRanges) && null qsOrder = Left LimitNoOrderError
   | otherwise = do
      acceptContentType <- findAcceptContentType conf action path accepts
      checkedTarget <- target

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

@@ -65,6 +65,7 @@ data ApiRequestError
   | InvalidBody ByteString
   | InvalidFilters
   | InvalidRange
+  | LimitNoOrderError
   | NoRelBetween Text Text Text
   | NoRpc Text Text [Text] Bool ContentType Bool
   | NotEmbedded Text
@@ -135,13 +136,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]
       }
 

test/spec/Feature/Query/DeleteSpec.hs

@@ -126,7 +126,7 @@ spec =
             , { "id": 3, "name": "item-3" }
             ]|]
 
-        request methodDelete "/limited_delete_items?limit=1&offset=1"
+        request methodDelete "/limited_delete_items?order=id&limit=1&offset=1"
             [("Prefer", "tx=commit")]
             mempty
           `shouldRespondWith`
@@ -158,7 +158,7 @@ spec =
             , { "id": 3, "name": "item-3" }
             ]|]
 
-        request methodDelete "/limited_delete_items?limit=1&id=gt.1"
+        request methodDelete "/limited_delete_items?order=id&limit=1&id=gt.1"
             [("Prefer", "tx=commit")]
             mempty
           `shouldRespondWith`
@@ -181,48 +181,33 @@ spec =
           `shouldRespondWith` ""
           { matchStatus  = 204 }
 
-      it "works on a table with a composite pk" $ do
+      it "fails without an explicit order by" $
-        get "/limited_delete_items_cpk"
+        request methodDelete "/limited_delete_items?limit=1&offset=1"
-          `shouldRespondWith`
-            [json|[
-              { "id": 1, "name": "item-1" }
-            , { "id": 2, "name": "item-2" }
-            , { "id": 3, "name": "item-3" }
-            ]|]
-
-        request methodDelete "/limited_delete_items_cpk?limit=1&offset=1"
             [("Prefer", "tx=commit")]
             mempty
           `shouldRespondWith`
-            ""
+            [json| {
-            { matchStatus  = 204
+              "code":"PGRST109",
-            , matchHeaders = [ matchHeaderAbsent hContentType
+              "hint": "Apply an 'order' using unique column(s)",
-                             , "Preference-Applied" <:> "tx=commit" ]
+              "details": null,
-            }
+              "message": "A 'limit' was applied without an explicit 'order'"
+              }|]
+            { matchStatus  = 400 }
 
-        get "/limited_delete_items_cpk"
+      it "fails when not ordering by a unique column" $
-          `shouldRespondWith`
+        request methodDelete "/limited_delete_items_wnonuniq_view?order=static&limit=1"
-            [json|[
-              { "id": 1, "name": "item-1" }
-            , { "id": 3, "name": "item-3" }
-            ]|]
-
-        request methodPost "/rpc/reset_limited_items"
-          [("Prefer", "tx=commit")]
-          [json| {"tbl_name": "limited_delete_items_cpk"} |]
-          `shouldRespondWith` ""
-          { matchStatus  = 204 }
-
-      it "doesn't work with views" $
-        request methodDelete "/limited_delete_items_view?limit=1&offset=1"
             [("Prefer", "tx=commit")]
             mempty
           `shouldRespondWith`
-            [json| {"hint":null,"details":null,"code":"PGRST507","message":"limit/offset is not implemented for views"} |]
+            [json| {
-            { matchStatus  = 501 }
+              "code":"PGRST110",
+              "hint": null,
+              "details":"Results contain 3 rows changed but the maximum number allowed is 1",
+              "message":"The maximum number of rows allowed to change was surpassed"
+              }|]
+            { matchStatus  = 400 }
 
-      it "works with views with an inferred pk" $ do
+      it "works with views with an explicit order by unique col" $ do
-        pendingWith "not implemented yet"
         get "/limited_delete_items_view"
           `shouldRespondWith`
             [json|[
@@ -231,7 +216,7 @@ spec =
             , { "id": 3, "name": "item-3" }
             ]|]
 
-        request methodDelete "/limited_delete_items_view?limit=1&offset=1"
+        request methodDelete "/limited_delete_items_view?order=id&limit=1&offset=1"
             [("Prefer", "tx=commit")]
             mempty
           `shouldRespondWith`
@@ -254,7 +239,39 @@ spec =
           `shouldRespondWith` ""
           { matchStatus  = 204 }
 
-      it "works on a table without a pk" $ do
+      it "works with views with an explicit order by composite pk" $ do
+        get "/limited_delete_items_cpk_view"
+          `shouldRespondWith`
+            [json|[
+              { "id": 1, "name": "item-1" }
+            , { "id": 2, "name": "item-2" }
+            , { "id": 3, "name": "item-3" }
+            ]|]
+
+        request methodDelete "/limited_delete_items_cpk_view?order=id,name&limit=1&offset=1"
+            [("Prefer", "tx=commit")]
+            mempty
+          `shouldRespondWith`
+            ""
+            { matchStatus  = 204
+            , matchHeaders = [ matchHeaderAbsent hContentType
+                             , "Preference-Applied" <:> "tx=commit" ]
+            }
+
+        get "/limited_delete_items_cpk_view"
+          `shouldRespondWith`
+            [json|[
+              { "id": 1, "name": "item-1" }
+            , { "id": 3, "name": "item-3" }
+            ]|]
+
+        request methodPost "/rpc/reset_limited_items"
+          [("Prefer", "tx=commit")]
+          [json| {"tbl_name": "limited_delete_items_cpk_view"} |]
+          `shouldRespondWith` ""
+          { matchStatus  = 204 }
+
+      it "works on a table without a pk by ordering by 'ctid'" $ do
         get "/limited_delete_items_no_pk"
           `shouldRespondWith`
             [json|[
@@ -263,7 +280,7 @@ spec =
             , { "id": 3, "name": "item-3" }
             ]|]
 
-        request methodDelete "/limited_delete_items_no_pk?limit=1&offset=1"
+        request methodDelete "/limited_delete_items_no_pk?order=ctid&limit=1&offset=1"
             [("Prefer", "tx=commit")]
             mempty
           `shouldRespondWith`

test/spec/Feature/Query/UpdateSpec.hs

@@ -397,7 +397,7 @@ spec = do
           , { "id": 3, "name": "item-3" }
           ]|]
 
-      request methodPatch "/limited_update_items?limit=2"
+      request methodPatch "/limited_update_items?order=id&limit=2"
           [("Prefer", "tx=commit")]
           [json| {"name": "updated-item"} |]
         `shouldRespondWith`
@@ -430,7 +430,7 @@ spec = do
           , { "id": 3, "name": "item-3" }
           ]|]
 
-      request methodPatch "/limited_update_items?limit=1&id=gt.2"
+      request methodPatch "/limited_update_items?order=id&limit=1&id=gt.2"
           [("Prefer", "tx=commit")]
           [json| {"name": "updated-item"} |]
         `shouldRespondWith`
@@ -463,7 +463,7 @@ spec = do
           , { "id": 3, "name": "item-3" }
           ]|]
 
-      request methodPatch "/limited_update_items?limit=1&offset=1"
+      request methodPatch "/limited_update_items?order=id&limit=1&offset=1"
           [("Prefer", "tx=commit")]
           [json| {"name": "updated-item"} |]
         `shouldRespondWith`
@@ -487,49 +487,33 @@ spec = do
         `shouldRespondWith` ""
         { matchStatus  = 204 }
 
-    it "works on a table with a composite pk" $ do
+    it "fails without an explicit order by" $
-      get "/limited_update_items_cpk"
+      request methodPatch "/limited_update_items?limit=1&offset=1"
-        `shouldRespondWith`
-          [json|[
-            { "id": 1, "name": "item-1" }
-          , { "id": 2, "name": "item-2" }
-          , { "id": 3, "name": "item-3" }
-          ]|]
-
-      request methodPatch "/limited_update_items_cpk?limit=1&offset=1"
           [("Prefer", "tx=commit")]
           [json| {"name": "updated-item"} |]
         `shouldRespondWith`
-          ""
+          [json| {
-          { matchStatus  = 204
+            "code":"PGRST109",
-          , matchHeaders = [ matchHeaderAbsent hContentType
+            "hint": "Apply an 'order' using unique column(s)",
-                           , "Preference-Applied" <:> "tx=commit" ]
+            "details": null,
-          }
+            "message": "A 'limit' was applied without an explicit 'order'"
+            }|]
+          { matchStatus  = 400 }
 
-      get "/limited_update_items_cpk?order=id,name"
+    it "fails when not ordering by a unique column" $
-        `shouldRespondWith`
+      request methodPatch "/limited_update_items_wnonuniq_view?order=static&limit=1"
-          [json|[
-            { "id": 1, "name": "item-1" }
-          , { "id": 2, "name": "updated-item" }
-          , { "id": 3, "name": "item-3" }
-          ]|]
-
-      request methodPost "/rpc/reset_limited_items"
-        [("Prefer", "tx=commit")]
-        [json| {"tbl_name": "limited_update_items_cpk"} |]
-        `shouldRespondWith` ""
-        { matchStatus  = 204 }
-
-    it "doesn't work with views" $
-      request methodPatch "/limited_update_items_view?limit=1&offset=1"
           [("Prefer", "tx=commit")]
           [json| {"name": "updated-item"} |]
         `shouldRespondWith`
-          [json| {"hint":null,"details":null,"code":"PGRST507","message":"limit/offset is not implemented for views"} |]
+          [json| {
-          { matchStatus  = 501 }
+            "code":"PGRST110",
+            "hint": null,
+            "details":"Results contain 3 rows changed but the maximum number allowed is 1",
+            "message":"The maximum number of rows allowed to change was surpassed"
+            }|]
+          { matchStatus  = 400 }
 
-    it "works with views with an inferred pk" $ do
+    it "works with views with an explicit order by unique col" $ do
-      pendingWith "not implemented yet"
       get "/limited_update_items_view"
         `shouldRespondWith`
           [json|[
@@ -538,7 +522,7 @@ spec = do
           , { "id": 3, "name": "item-3" }
           ]|]
 
-      request methodPatch "/limited_update_items_view?limit=1&offset=1"
+      request methodPatch "/limited_update_items_view?order=id&limit=1&offset=1"
           [("Prefer", "tx=commit")]
           [json| {"name": "updated-item"} |]
         `shouldRespondWith`
@@ -562,7 +546,41 @@ spec = do
         `shouldRespondWith` ""
         { matchStatus  = 204 }
 
-    it "works on a table without a pk" $ do
+    it "works with views with an explicit order by composite pk" $ do
+      get "/limited_update_items_cpk_view"
+        `shouldRespondWith`
+          [json|[
+            { "id": 1, "name": "item-1" }
+          , { "id": 2, "name": "item-2" }
+          , { "id": 3, "name": "item-3" }
+          ]|]
+
+      request methodPatch "/limited_update_items_cpk_view?order=id,name&limit=1&offset=1"
+          [("Prefer", "tx=commit")]
+          [json| {"name": "updated-item"} |]
+        `shouldRespondWith`
+          ""
+          { matchStatus  = 204
+          , matchHeaders = [ matchHeaderAbsent hContentType
+                           , "Preference-Applied" <:> "tx=commit" ]
+          }
+
+      get "/limited_update_items_cpk_view?order=id,name"
+        `shouldRespondWith`
+          [json|[
+            { "id": 1, "name": "item-1" }
+          , { "id": 2, "name": "updated-item" }
+          , { "id": 3, "name": "item-3" }
+          ]|]
+
+      request methodPost "/rpc/reset_limited_items"
+        [("Prefer", "tx=commit")]
+        [json| {"tbl_name": "limited_update_items_cpk_view"} |]
+        `shouldRespondWith` ""
+        { matchStatus  = 204 }
+
+
+    it "works on a table without a pk by ordering by 'ctid'" $ do
       get "/limited_update_items_no_pk"
         `shouldRespondWith`
           [json|[
@@ -571,7 +589,7 @@ spec = do
           , { "id": 3, "name": "item-3" }
           ]|]
 
-      request methodPatch "/limited_update_items_no_pk?limit=1"
+      request methodPatch "/limited_update_items_no_pk?order=ctid&limit=1"
           [("Prefer", "tx=commit")]
           [json| {"name": "updated-item"} |]
         `shouldRespondWith`

test/spec/fixtures/privileges.sql

@@ -168,12 +168,16 @@ GRANT ALL ON TABLE
     , limited_update_items_cpk
     , limited_update_items_no_pk
     , limited_update_items_view
+    , limited_update_items_wnonuniq_view
     , limited_delete_items
     , limited_delete_items_cpk
     , limited_delete_items_no_pk
     , limited_delete_items_view
     , plate
     , well
+    , limited_delete_items_wnonuniq_view
+    , limited_delete_items_cpk_view
+    , limited_update_items_cpk_view
 TO postgrest_test_anonymous;
 
 GRANT INSERT ON TABLE insertonly TO postgrest_test_anonymous;

test/spec/fixtures/schema.sql

@@ -2493,6 +2493,12 @@ create table limited_update_items_no_pk(
 create view limited_update_items_view as
 select * from limited_update_items;
 
+create view limited_update_items_wnonuniq_view as
+select *, 'static'::text as static from limited_update_items;
+
+create view limited_update_items_cpk_view as
+select * from limited_update_items_cpk;
+
 create table limited_delete_items(
   id int primary key
 , name text
@@ -2512,6 +2518,12 @@ create table limited_delete_items_no_pk(
 create view limited_delete_items_view as
 select * from limited_delete_items;
 
+create view limited_delete_items_wnonuniq_view as
+select *, 'static'::text as static from limited_delete_items;
+
+create view limited_delete_items_cpk_view as
+select * from limited_delete_items_cpk;
+
 create function reset_limited_items(tbl_name text default '') returns void as $_$ begin
   execute format(
   $$