diff --git a/CHANGELOG.md b/CHANGELOG.md index 0dfef74b84d..c31f7871ccb 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/src/PostgREST/App.hs b/src/PostgREST/App.hs index dadc2addb2b..e2a59c2e9cf 100644 --- a/src/PostgREST/App.hs +++ b/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 diff --git a/src/PostgREST/DbStructure.hs b/src/PostgREST/DbStructure.hs index 7404fcb6114..5e58194189e 100644 --- a/src/PostgREST/DbStructure.hs +++ b/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 diff --git a/src/PostgREST/DbStructure/Table.hs b/src/PostgREST/DbStructure/Table.hs index ed2302a9130..dc00a9c3b10 100644 --- a/src/PostgREST/DbStructure/Table.hs +++ b/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 diff --git a/src/PostgREST/Error.hs b/src/PostgREST/Error.hs index 599a823d313..8e236917137 100644 --- a/src/PostgREST/Error.hs +++ b/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" diff --git a/src/PostgREST/Query/QueryBuilder.hs b/src/PostgREST/Query/QueryBuilder.hs index 761d22caa55..440ea74d0fe 100644 --- a/src/PostgREST/Query/QueryBuilder.hs +++ b/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) = diff --git a/src/PostgREST/Query/SqlFragment.hs b/src/PostgREST/Query/SqlFragment.hs index 9ec9d95e043..2ca2e0b99b8 100644 --- a/src/PostgREST/Query/SqlFragment.hs +++ b/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) - ) diff --git a/src/PostgREST/Request/DbRequestBuilder.hs b/src/PostgREST/Request/DbRequestBuilder.hs index 3e5c00d41ed..913735751a0 100644 --- a/src/PostgREST/Request/DbRequestBuilder.hs +++ b/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) diff --git a/src/PostgREST/Request/Types.hs b/src/PostgREST/Request/Types.hs index de0feb486ad..c0e3ae19966 100644 --- a/src/PostgREST/Request/Types.hs +++ b/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] } diff --git a/test/spec/Feature/Query/DeleteSpec.hs b/test/spec/Feature/Query/DeleteSpec.hs index eb211fd4dea..3ed807a2bca 100644 --- a/test/spec/Feature/Query/DeleteSpec.hs +++ b/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,8 +181,34 @@ spec = `shouldRespondWith` "" { matchStatus = 204 } - it "works on a table with a composite pk" $ do - get "/limited_delete_items_cpk" + it "fails without an explicit order by" $ + request methodDelete "/limited_delete_items?limit=1&offset=1" + [("Prefer", "tx=commit")] + mempty + `shouldRespondWith` + [json| { + "code":"PGRST109", + "hint": "Apply an 'order' using unique column(s)", + "details": null, + "message": "A 'limit' was applied without an explicit 'order'" + }|] + { matchStatus = 400 } + + it "fails when not ordering by a unique column" $ + request methodDelete "/limited_delete_items_wnonuniq_view?order=static&limit=1" + [("Prefer", "tx=commit")] + mempty + `shouldRespondWith` + [json| { + "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 explicit order by unique col" $ do + get "/limited_delete_items_view" `shouldRespondWith` [json|[ { "id": 1, "name": "item-1" } @@ -190,7 +216,7 @@ spec = , { "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 +226,7 @@ spec = , "Preference-Applied" <:> "tx=commit" ] } - get "/limited_delete_items_cpk" + get "/limited_delete_items_view" `shouldRespondWith` [json|[ { "id": 1, "name": "item-1" } @@ -209,21 +235,12 @@ 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" } @@ -231,7 +248,7 @@ spec = , { "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 +258,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,11 +267,11 @@ 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 } - it "works on a table without a pk" $ do + 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` diff --git a/test/spec/Feature/Query/UpdateSpec.hs b/test/spec/Feature/Query/UpdateSpec.hs index 7edc25436b2..088e7a77b1d 100644 --- a/test/spec/Feature/Query/UpdateSpec.hs +++ b/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,8 +487,34 @@ spec = do `shouldRespondWith` "" { matchStatus = 204 } - it "works on a table with a composite pk" $ do - get "/limited_update_items_cpk" + it "fails without an explicit order by" $ + request methodPatch "/limited_update_items?limit=1&offset=1" + [("Prefer", "tx=commit")] + [json| {"name": "updated-item"} |] + `shouldRespondWith` + [json| { + "code":"PGRST109", + "hint": "Apply an 'order' using unique column(s)", + "details": null, + "message": "A 'limit' was applied without an explicit 'order'" + }|] + { matchStatus = 400 } + + it "fails when not ordering by a unique column" $ + request methodPatch "/limited_update_items_wnonuniq_view?order=static&limit=1" + [("Prefer", "tx=commit")] + [json| {"name": "updated-item"} |] + `shouldRespondWith` + [json| { + "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 explicit order by unique col" $ do + get "/limited_update_items_view" `shouldRespondWith` [json|[ { "id": 1, "name": "item-1" } @@ -496,7 +522,7 @@ spec = do , { "id": 3, "name": "item-3" } ]|] - request methodPatch "/limited_update_items_cpk?limit=1&offset=1" + request methodPatch "/limited_update_items_view?order=id&limit=1&offset=1" [("Prefer", "tx=commit")] [json| {"name": "updated-item"} |] `shouldRespondWith` @@ -506,7 +532,7 @@ spec = do , "Preference-Applied" <:> "tx=commit" ] } - get "/limited_update_items_cpk?order=id,name" + get "/limited_update_items_view?order=id" `shouldRespondWith` [json|[ { "id": 1, "name": "item-1" } @@ -516,21 +542,12 @@ spec = do request methodPost "/rpc/reset_limited_items" [("Prefer", "tx=commit")] - [json| {"tbl_name": "limited_update_items_cpk"} |] + [json| {"tbl_name": "limited_update_items_view"} |] `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"} |] - { matchStatus = 501 } - - it "works with views with an inferred pk" $ do - pendingWith "not implemented yet" - get "/limited_update_items_view" + 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" } @@ -538,7 +555,7 @@ spec = do , { "id": 3, "name": "item-3" } ]|] - request methodPatch "/limited_update_items_view?limit=1&offset=1" + request methodPatch "/limited_update_items_cpk_view?order=id,name&limit=1&offset=1" [("Prefer", "tx=commit")] [json| {"name": "updated-item"} |] `shouldRespondWith` @@ -548,7 +565,7 @@ spec = do , "Preference-Applied" <:> "tx=commit" ] } - get "/limited_update_items_view?order=id" + get "/limited_update_items_cpk_view?order=id,name" `shouldRespondWith` [json|[ { "id": 1, "name": "item-1" } @@ -558,11 +575,12 @@ spec = do request methodPost "/rpc/reset_limited_items" [("Prefer", "tx=commit")] - [json| {"tbl_name": "limited_update_items_view"} |] + [json| {"tbl_name": "limited_update_items_cpk_view"} |] `shouldRespondWith` "" { matchStatus = 204 } - it "works on a table without a pk" $ do + + 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` diff --git a/test/spec/fixtures/privileges.sql b/test/spec/fixtures/privileges.sql index de3a98243ea..e25b35dcc74 100644 --- a/test/spec/fixtures/privileges.sql +++ b/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; diff --git a/test/spec/fixtures/schema.sql b/test/spec/fixtures/schema.sql index 12a0a94e37e..7db770c3cac 100644 --- a/test/spec/fixtures/schema.sql +++ b/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( $$