Skip to content

Commit

Permalink
Remove PK verification and delete using all the keys as filters
Browse files Browse the repository at this point in the history
  • Loading branch information
@laurenceisla
laurenceisla committed Jul 28, 2022
1 parent 1ffef79 commit fecfa21
Show file tree
Hide file tree
Showing 3 changed files with 134 additions and 77 deletions.
8 changes: 2 additions & 6 deletions src/PostgREST/App.hs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -397,12 +397,8 @@ handleSingleUpsert identifier context@(RequestContext _ ctxDbStructure ApiReques
response HTTP.status204 [] mempty

handleDelete :: QualifiedIdentifier -> RequestContext -> DbHandler Wai.Response
handleDelete identifier context@RequestContext{..} = do
let
ApiRequest{..} = ctxApiRequest
pkCols = maybe mempty tablePKCols $ HM.lookup identifier $ dbTables ctxDbStructure

WriteQueryResult{..} <- writeQuery MutationDelete identifier False pkCols context
handleDelete identifier context@(RequestContext _ _ ApiRequest{..} _) = do
WriteQueryResult{..} <- writeQuery MutationDelete identifier False mempty context

let
response = gucResponse resGucStatus resGucHeaders
Expand Down
6 changes: 2 additions & 4 deletions src/PostgREST/Request/DbRequestBuilder.hs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -331,7 +331,7 @@ mutateRequest mutation schema tName ApiRequest{..} pkCols readReq = mapLeft ApiR
Left InvalidFilters
MutationDelete
| noBodyFilterLimit -> Left $ NoBodyFilterLimit "delete"
| otherwise -> Right $ Delete qi deleteCols body combinedLogic iTopLevelRange rootOrder returnings
| otherwise -> Right $ Delete qi iColumns body combinedLogic iTopLevelRange rootOrder returnings

where
confCols = fromMaybe pkCols qsOnConflict
Expand All @@ -345,9 +345,7 @@ mutateRequest mutation schema tName ApiRequest{..} pkCols readReq = mapLeft ApiR
rootOrder = maybe [] snd $ find (\(x, _) -> null x) qsOrder
combinedLogic = foldr addFilterToLogicForest logic qsFiltersRoot
body = payRaw <$> iPayload -- the body is assumed to be json at this stage(ApiRequest validates)
payloadHasPks = all (`elem` iColumns) pkCols
noBodyFilterLimit = null qsColumns && null qsLogic && null qsFilters && null qsRanges && (isNothing body || not payloadHasPks)
deleteCols = if null qsColumns then S.fromList pkCols else iColumns
noBodyFilterLimit = null qsColumns && null qsLogic && null qsFilters && null qsRanges && (isNothing body || S.null iColumns)

callRequest :: ProcDescription -> ApiRequest -> ReadRequest -> CallRequest
callRequest proc apiReq readReq = FunctionCall {
Expand Down
197 changes: 130 additions & 67 deletions test/spec/Feature/Query/DeleteSpec.hs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ spec =
, matchHeaders = ["Content-Range" <:> "*/*"]
}

it "works with a simple primary key filter in the json body" $ do
it "works with a key in the json body (simple pk table)" $ do
get "/body_delete_items"
`shouldRespondWith`
[json|[
Expand All @@ -81,7 +81,7 @@ spec =

request methodDelete "/body_delete_items"
[("Prefer", "tx=commit"), ("Prefer", "count=exact")]
[json| { "id": 3 } |]
[json| { "name": "item-3" } |]
`shouldRespondWith`
""
{ matchStatus = 204
Expand All @@ -103,7 +103,7 @@ spec =
`shouldRespondWith` ""
{ matchStatus = 204 }

it "works with a simple primary key filter and ignores other keys in the json body" $ do
it "works with more than one key in the json body (simple pk table)" $ do
get "/body_delete_items"
`shouldRespondWith`
[json|[
Expand All @@ -114,7 +114,7 @@ spec =

request methodDelete "/body_delete_items"
[("Prefer", "tx=commit"), ("Prefer", "count=exact")]
[json| { "id": 3, "name": "item-2", "other": "value" } |]
[json| { "id": 3, "name": "item-3" } |]
`shouldRespondWith`
""
{ matchStatus = 204
Expand All @@ -136,7 +136,7 @@ spec =
`shouldRespondWith` ""
{ matchStatus = 204 }

it "works with a composite primary key filter in the json body" $ do
it "works with a key in the json body (composite pk table)" $ do
get "/body_delete_items_cpk"
`shouldRespondWith`
[json|[
Expand All @@ -147,7 +147,7 @@ spec =

request methodDelete "/body_delete_items_cpk"
[("Prefer", "tx=commit"), ("Prefer", "count=exact")]
[json| { "id": 3, "name": "item-3" } |]
[json| { "name": "item-3" } |]
`shouldRespondWith`
""
{ matchStatus = 204
Expand All @@ -169,7 +169,7 @@ spec =
`shouldRespondWith` ""
{ matchStatus = 204 }

it "works with a composite primary key filter and ignores other keys in the json body" $ do
it "works with more than one key in the json body (composite pk table)" $ do
get "/body_delete_items_cpk"
`shouldRespondWith`
[json|[
Expand All @@ -180,7 +180,7 @@ spec =

request methodDelete "/body_delete_items_cpk"
[("Prefer", "tx=commit"), ("Prefer", "count=exact")]
[json| { "id": 3, "name": "item-3", "observation": "an observation", "other": "value" } |]
[json| { "id": 3, "name": "item-3" } |]
`shouldRespondWith`
""
{ matchStatus = 204
Expand Down Expand Up @@ -280,9 +280,9 @@ spec =
, { "id": 3, "name": "item-3", "observation": null }
]|]

request methodDelete "/body_delete_items_cpk?columns=id,name"
request methodDelete "/body_delete_items_cpk?columns=name"
[("Prefer", "tx=commit"), ("Prefer", "count=exact")]
[json|{ "id": 1, "name": "item-1", "observation": "an observation", "other": "value" }|]
[json|{ "id": 2, "name": "item-1", "observation": "an observation", "other": "value" }|]
`shouldRespondWith`
""
{ matchStatus = 204
Expand Down Expand Up @@ -491,52 +491,6 @@ spec =
, matchHeaders = [matchContentTypeJson]
}

context "known route, no records matched" $
it "includes [] body if return=rep" $
request methodDelete "/items?id=eq.101"
[("Prefer", "return=representation")] ""
`shouldRespondWith` "[]"
{ matchStatus = 200
, matchHeaders = ["Content-Range" <:> "*/*"]
}

context "totally unknown route" $
it "fails with 404" $
request methodDelete "/foozle?id=eq.101" [] "" `shouldRespondWith` 404

context "table with limited privileges" $ do
it "fails deleting the row when return=representation and selecting all the columns" $
request methodDelete "/app_users?id=eq.1" [("Prefer", "return=representation")] mempty
`shouldRespondWith` 401

it "succeeds deleting the row when return=representation and selecting only the privileged columns" $
request methodDelete "/app_users?id=eq.1&select=id,email" [("Prefer", "return=representation")]
[json| { "password": "passxyz" } |]
`shouldRespondWith` [json|[ { "id": 1, "email": "[email protected]" } ]|]
{ matchStatus = 200
, matchHeaders = ["Content-Range" <:> "*/*"]
}

it "suceeds deleting the row with no explicit select when using return=minimal" $
request methodDelete "/app_users?id=eq.2"
[("Prefer", "return=minimal")]
mempty
`shouldRespondWith`
""
{ matchStatus = 204
, matchHeaders = [matchHeaderAbsent hContentType]
}

it "suceeds deleting the row with no explicit select by default" $
request methodDelete "/app_users?id=eq.3"
[]
mempty
`shouldRespondWith`
""
{ matchStatus = 204
, matchHeaders = [matchHeaderAbsent hContentType]
}

context "limited delete" $ do
it "works with the limit and offset query params" $ do
get "/limited_delete_items"
Expand Down Expand Up @@ -758,7 +712,7 @@ spec =
{ matchStatus = 204 }

context "bulk deletes" $ do
it "can delete tables with simple pk" $ do
it "can delete tables with a key in the json body (simple pk table)" $ do
get "/bulk_delete_items"
`shouldRespondWith`
[json|[
Expand Down Expand Up @@ -791,7 +745,7 @@ spec =
`shouldRespondWith` ""
{ matchStatus = 204 }

it "can delete tables with simple pk and ignores other keys in the json body" $ do
it "can delete tables with more than one key in the json body (simple pk table)" $ do
get "/bulk_delete_items"
`shouldRespondWith`
[json|[
Expand All @@ -803,8 +757,8 @@ spec =
request methodDelete "/bulk_delete_items"
[("Prefer", "tx=commit"), ("Prefer", "count=exact")]
[json|[
{ "id": 1, "name": "item", "other": "value" }
, { "id": 3, "name": null, "other": "value" }
{ "id": 1, "name": "item-1" }
, { "id": 3, "name": "item-3" }
]|]
`shouldRespondWith`
""
Expand All @@ -824,7 +778,7 @@ spec =
`shouldRespondWith` ""
{ matchStatus = 204 }

it "can delete tables with composite pk" $ do
it "can delete tables with a key in the json body (composite pk table)" $ do
get "/bulk_delete_items_cpk"
`shouldRespondWith`
[json|[
Expand All @@ -836,8 +790,8 @@ spec =
request methodDelete "/bulk_delete_items_cpk"
[("Prefer", "tx=commit"), ("Prefer", "count=exact")]
[json|[
{ "id": 1, "name": "item-1" }
, { "id": 2, "name": "item-2" }
{ "name": "item-1" }
, { "name": "item-2" }
]|]
`shouldRespondWith`
""
Expand All @@ -857,7 +811,7 @@ spec =
`shouldRespondWith` ""
{ matchStatus = 204 }

it "can delete tables with composite pk and ignores other keys in the json body" $ do
it "can delete tables with more than one key in the json body (composite pk table)" $ do
get "/bulk_delete_items_cpk"
`shouldRespondWith`
[json|[
Expand All @@ -869,8 +823,8 @@ spec =
request methodDelete "/bulk_delete_items_cpk"
[("Prefer", "tx=commit"), ("Prefer", "count=exact")]
[json|[
{ "id": 1, "name": "item-1", "observation": "an observation", "other": "value" }
, { "id": 2, "name": "item-2", "observation": "none", "other": "value" }
{ "id": 1, "name": "item-1" }
, { "id": 2, "name": "item-2" }
]|]
`shouldRespondWith`
""
Expand Down Expand Up @@ -1001,7 +955,6 @@ spec =
{ matchStatus = 204 }

context "with composite pk" $ do

it "ignores json keys not included in ?columns" $ do
get "/bulk_delete_items_cpk"
`shouldRespondWith`
Expand Down Expand Up @@ -1036,3 +989,113 @@ spec =
[json| {"tbl_name": "bulk_delete_items_cpk"} |]
`shouldRespondWith` ""
{ matchStatus = 204 }

context "known route, no records matched" $ do
it "includes [] body if return=rep with filters in the query string" $
request methodDelete "/items?id=eq.101"
[("Prefer", "return=representation")] ""
`shouldRespondWith` "[]"
{ matchStatus = 200
, matchHeaders = ["Content-Range" <:> "*/*"]
}

it "includes [] body if return=rep with keys in the json body" $
request methodDelete "/body_delete_items"
[("Prefer", "return=representation")]
[json| { "id": 2, "name": "item-3" } |]
`shouldRespondWith` "[]"
{ matchStatus = 200
, matchHeaders = ["Content-Range" <:> "*/*"]
}

it "includes [] body if return=rep with ?columns" $ do
request methodDelete "/body_delete_items?columns=id,name"
[("Prefer", "return=representation")]
[json| { "id": 5, "name": "item-3" } |]
`shouldRespondWith` "[]"
{ matchStatus = 200
, matchHeaders = ["Content-Range" <:> "*/*"]
}

request methodDelete "/body_delete_items?columns=id"
[("Prefer", "return=representation")]
[json| { "name": "item-3" } |]
`shouldRespondWith` "[]"
{ matchStatus = 200
, matchHeaders = ["Content-Range" <:> "*/*"]
}

request methodDelete "/body_delete_items?columns=id"
[("Prefer", "return=representation")]
[json| {} |]
`shouldRespondWith` "[]"
{ matchStatus = 200
, matchHeaders = ["Content-Range" <:> "*/*"]
}

request methodDelete "/body_delete_items?columns=id,name"
[("Prefer", "return=representation")]
[json|[
{ "id": 5, "name": "item-3" }
, { "id": 6, "name": "item-2" }
]|]
`shouldRespondWith` "[]"
{ matchStatus = 200
, matchHeaders = ["Content-Range" <:> "*/*"]
}

request methodDelete "/body_delete_items?columns=id"
[("Prefer", "return=representation")]
[json|[
{ "name": "item-3" }
, { "observation": "an observation" }
]|]
`shouldRespondWith` "[]"
{ matchStatus = 200
, matchHeaders = ["Content-Range" <:> "*/*"]
}

request methodDelete "/body_delete_items?columns=id"
[("Prefer", "return=representation")]
[json| [{}] |]
`shouldRespondWith` "[]"
{ matchStatus = 200
, matchHeaders = ["Content-Range" <:> "*/*"]
}

context "totally unknown route" $
it "fails with 404" $
request methodDelete "/foozle?id=eq.101" [] "" `shouldRespondWith` 404

context "table with limited privileges" $ do
it "fails deleting the row when return=representation and selecting all the columns" $
request methodDelete "/app_users?id=eq.1" [("Prefer", "return=representation")] mempty
`shouldRespondWith` 401

it "succeeds deleting the row when return=representation and selecting only the privileged columns" $
request methodDelete "/app_users?id=eq.1&select=id,email" [("Prefer", "return=representation")]
[json| { "password": "passxyz" } |]
`shouldRespondWith` [json|[ { "id": 1, "email": "[email protected]" } ]|]
{ matchStatus = 200
, matchHeaders = ["Content-Range" <:> "*/*"]
}

it "suceeds deleting the row with no explicit select when using return=minimal" $
request methodDelete "/app_users?id=eq.2"
[("Prefer", "return=minimal")]
mempty
`shouldRespondWith`
""
{ matchStatus = 204
, matchHeaders = [matchHeaderAbsent hContentType]
}

it "suceeds deleting the row with no explicit select by default" $
request methodDelete "/app_users?id=eq.3"
[]
mempty
`shouldRespondWith`
""
{ matchStatus = 204
, matchHeaders = [matchHeaderAbsent hContentType]
}

0 comments on commit fecfa21

Please sign in to comment.