limited update/delete on views with explicit order

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, findTable,
+import PostgREST.DbStructure             (DbStructure (..), findTable,
                                           tablePKCols)
 import PostgREST.DbStructure.Identifiers (FieldName,
                                           QualifiedIdentifier (..),
@@ -346,10 +345,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
-  when (iTopLevelRange /= RangeQuery.allRange && findIfView identifier (dbTables ctxDbStructure)) $
-    throwError $ Error.NotImplemented "limit/offset is not implemented for views"
-
+handleUpdate identifier context@(RequestContext _ _ ApiRequest{..} _) = do
   WriteQueryResult{..} <- writeQuery MutationUpdate identifier False mempty context
 
   let
@@ -395,10 +391,7 @@ handleSingleUpsert identifier context@(RequestContext _ _ ApiRequest{..} _) = do
       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
   WriteQueryResult{..} <- writeQuery MutationDelete identifier False mempty context
 
   let

src/PostgREST/DbStructure.hs

@@ -23,7 +23,6 @@ module PostgREST.DbStructure
   , queryDbStructure
   , accessibleTables
   , accessibleProcs
-  , findIfView
   , findTable
   , schemaDescription
   , tableCols
@@ -83,9 +82,6 @@ tablePKCols dbs tSchema tName = pkName <$> filter (\pk -> tSchema == (tableSchem
 findTable :: Schema -> TableName -> [Table] -> Maybe Table
 findTable tSchema tName = find (\tbl -> tableSchema tbl == tSchema && tableName tbl == tName)
 
-findIfView :: QualifiedIdentifier -> [Table] -> Bool
-findIfView identifier tbls = maybe False tableIsView (findTable (qiSchema identifier) (qiName identifier) tbls)
-
 -- | The source table column a view column refers to
 type SourceColumn = (Column, ViewColumn)
 type ViewColumn = Column
@@ -142,7 +138,6 @@ decodeTables =
                  <*> column HD.bool
                  <*> column HD.bool
                  <*> column HD.bool
-                 <*> column HD.bool
 
 decodeColumns :: [Table] -> HD.Result [Column]
 decodeColumns tables =
@@ -342,7 +337,6 @@ accessibleTables pgVer =
       n.nspname as table_schema,
       relname as table_name,
       d.description as table_description,
-      c.relkind IN ('v','m') as is_view,
       (
         c.relkind IN ('r','p')
         OR (
@@ -478,7 +472,6 @@ allTables pgVer =
       n.nspname AS table_schema,
       c.relname AS table_name,
       d.description AS table_description,
-      c.relkind IN ('v','m') as is_view,
       (
         c.relkind IN ('r','p')
         OR (

src/PostgREST/DbStructure/Table.hs

@@ -20,8 +20,6 @@ data Table = Table
   { tableSchema      :: Schema
   , tableName        :: TableName
   , tableDescription :: Maybe Text
-    -- 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/Query/QueryBuilder.hs

@@ -40,9 +40,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
@@ -107,7 +108,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=
@@ -126,8 +127,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 " <>
@@ -140,9 +142,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 <> " " <>
@@ -153,7 +155,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 " <>
@@ -162,7 +165,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

@@ -19,6 +19,7 @@ module PostgREST.Query.SqlFragment
   , locationF
   , mutRangeF
   , normalizedBody
+  , orderF
   , pgFmtColumn
   , pgFmtIdent
   , pgFmtJoinCondition
@@ -325,6 +326,19 @@ 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 =
+  -- the "ctid" system column is always available to tables, use it as default
+  let ids = if null rangeId then ["ctid"] else rangeId in
+  (
+    BS.intercalate " AND " $ (\col -> pgFmtColumn mainQi col <> " = " <> pgFmtColumn (QualifiedIdentifier mempty "pgrst_affected_rows") col) <$> ids
+  , BS.intercalate ", " (pgFmtColumn mainQi <$> ids)
+  )
+
+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)
@@ -335,12 +349,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

@@ -275,7 +275,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
 
@@ -322,7 +324,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 &&
@@ -333,7 +335,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
@@ -345,6 +347,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]
       }
 

test/spec/Feature/OpenApi/RootSpec.hs

@@ -29,7 +29,7 @@ spec =
         [json| {
             "tableName": "orders_view", "tableSchema": "test",
             "tableDeletable": true, "tableUpdatable": true,
-            "tableIsView":true, "tableInsertable": true,
+            "tableInsertable": true,
             "tableDescription": null
           } |]
         { matchHeaders = [matchContentTypeJson] }

test/spec/Feature/Query/DeleteSpec.hs

@@ -181,16 +181,23 @@ spec =
           `shouldRespondWith` ""
           { matchStatus  = 204 }
 
-      it "works on a table with a composite pk" $ do
-        get "/limited_delete_items_cpk"
+      it "fails for views without an explicit order by" $
+        request methodDelete "/limited_delete_items_view?limit=1&offset=1"
+            [("Prefer", "tx=commit")]
+            mempty
+          `shouldRespondWith`
+          400
+
+      it "works with views with an explicit order by unique col" $ do
+        get "/limited_delete_items_view"
           `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"
+        request methodDelete "/limited_delete_items_view?order=id&limit=1&offset=1"
             [("Prefer", "tx=commit")]
             mempty
           `shouldRespondWith`
@@ -200,7 +207,7 @@ spec =
                              , "Preference-Applied" <:> "tx=commit" ]
             }
 
-        get "/limited_delete_items_cpk"
+        get "/limited_delete_items_view"
           `shouldRespondWith`
             [json|[
               { "id": 1, "name": "item-1" }
@@ -209,29 +216,20 @@ spec =
 
         request methodPost "/rpc/reset_limited_items"
           [("Prefer", "tx=commit")]
-          [json| {"tbl_name": "limited_delete_items_cpk"} |]
+          [json| {"tbl_name": "limited_delete_items_view"} |]
           `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"} |]
-            { matchStatus  = 501 }
-
-      it "works with views with an inferred pk" $ do
-        pendingWith "not implemented yet"
-        get "/limited_delete_items_view"
+      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_view?limit=1&offset=1"
+        request methodDelete "/limited_delete_items_cpk_view?order=id,name&limit=1&offset=1"
             [("Prefer", "tx=commit")]
             mempty
           `shouldRespondWith`
@@ -241,7 +239,7 @@ spec =
                              , "Preference-Applied" <:> "tx=commit" ]
             }
 
-        get "/limited_delete_items_view"
+        get "/limited_delete_items_cpk_view"
           `shouldRespondWith`
             [json|[
               { "id": 1, "name": "item-1" }
@@ -250,7 +248,7 @@ spec =
 
         request methodPost "/rpc/reset_limited_items"
           [("Prefer", "tx=commit")]
-          [json| {"tbl_name": "limited_delete_items_view"} |]
+          [json| {"tbl_name": "limited_delete_items_cpk_view"} |]
           `shouldRespondWith` ""
           { matchStatus  = 204 }