-
-
Notifications
You must be signed in to change notification settings - Fork 748
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Bulk delete #1030
Bulk delete #1030
Conversation
d4b10e3
to
6da4a44
Compare
Are you basically attempting to make DELETE a batch operation, whereas so far it's been an individual (document endpoint) or complete (resource endpoint) operation? |
eve/methods/delete.py
Outdated
getattr(app, "on_delete_resource")(resource) | ||
getattr(app, "on_delete_resource_%s" % resource)() | ||
default_request = ParsedRequest() | ||
if config.DOMAIN[resource]['soft_delete']: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could use resource_def
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right
eve/methods/delete.py
Outdated
if config.DOMAIN[resource]['soft_delete']: | ||
# get_document should always fetch soft deleted documents from the db | ||
# callers must handle soft deleted documents | ||
default_request.show_deleted = True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why returning already deleted documents to the callback? Busines logic should have been applied on original deletion, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, in this case, I would like to offer to the client possibility to customize what he wants to delete. A possible use case can be that in the collection there are documents which should not be deleted by the client (i.e. included service for a trip, like included car or included checked baggage).
Therefore, it might be possible by modifying the query (in the case on hard delete) or modify the originals (removing what we do not what to delete) in the callback.
Hi Nicola, basically, I just want to enforce that we should delete what we fetch. Therefore, if in the URL we are allowing to specify some parameters (i.e. person with for follow url users/<regex("[a-f0-9]{24}"):person>/invoices --> lookup = {"person" : value}), we should use the some parameters to do the delete. |
@@ -38,7 +38,7 @@ def deleteitem(resource, **lookup): | |||
|
|||
|
|||
def deleteitem_internal( | |||
resource, concurrency_check=False, suppress_callbacks=False, **lookup): | |||
resource, concurrency_check=False, suppress_callbacks=False, original=None, **lookup): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing docstring update here
eve/methods/delete.py
Outdated
@@ -150,7 +149,8 @@ def deleteitem_internal( | |||
app.media.delete(original[field], resource) | |||
|
|||
id = original[resource_def['id_field']] | |||
app.data.remove(resource, {resource_def['id_field']: id}) | |||
# We should delete what we fetch. Since the id_field might not be unique, let's use the lookup |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this comment can be deleted
if resource_def['soft_delete']: | ||
# get_document should always fetch soft deleted documents from the db | ||
# callers must handle soft deleted documents | ||
default_request.show_deleted = True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this behavior should probably be explained in the Soft Delete section of the docs. Explain that the new callback will include soft-deleted documents when soft delete is active.
@@ -21,6 +21,27 @@ def test_unknown_resource(self): | |||
_, status = self.delete(url) | |||
self.assert404(status) | |||
|
|||
def test_bulk_delete_id_field(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the new DELETE behavior should be explained in the docs, probably in the Sub Resources of the features page.
f7b2cb4
to
1753b1d
Compare
Hi Nicola, thanks for your review! :) I did the update you suggested to me. Regards, |
Could you please squash all commits into one? |
Ciao Nicola, commit uniti! :) Ciao, |
Hi @nicolaiarocci, is there anything else I should do for this PR? Regards, |
Merged after a rebase and some fixes, see 63d87fc. Thanks! |
* 'master' of https://github.com/pyeve/eve: (374 commits) Changelog for pyeve#1070 use OrderedDict from backport_collections Minor changelog fixes for pyeve#1048 Support Decimal type MongoDB Changelog: add reference to proper PR Changelog update for pyeve#1042 Fix a serialization bug that randomly skips fields if "x_of" is encountered Amedeo Bussi Changelog for pyeve#1030 pep/flake, and remove duplicate test documentation improvements and fixes Bulk delete MONGO_DBNAME can be now used along with MONGO_URI Vasilis Lolis typo Amedeo91 Changelog for pyeve#1031 Bulk Embedded document resolution Delete unnecessary code Fix insidious bug in tests.TestPost class ...
Bulk delete fix +Adding callback on the bulk delete flow for customization purpose
Issue #1028
Issue #1010 --> to support the possibility to perform a bulk delete with a multi key unique id (Note: only for not soft delete, as per soft delete is possible to achieve it with customisation in the datalayer)