Skip to content

Commit

Permalink
Fix: Delete resource that is empty returns 404
Browse files Browse the repository at this point in the history
Closes #1299
  • Loading branch information
nicolaiarocci committed Aug 9, 2019
1 parent de548b7 commit 398c78b
Show file tree
Hide file tree
Showing 3 changed files with 17 additions and 11 deletions.
3 changes: 3 additions & 0 deletions CHANGES.rst
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,15 @@ New

Fixed
~~~~~
- (*breaking*) Delete on empty resource returns 404, should return 204
(`#1299`_)
- MONGO_REPLICA_SET ignored (`#1302`_)
- Documentation typo (`#1293`_)
- Flask 1.1.1 breaks ``test_logging_info`` test (`#1296`_)
- Display the full release number on Eve frontpage.
- Update link to EveGenie repository. New maintainer: David Zisky.

.. _`#1299`: https://github.com/pyeve/eve/issues/1299
.. _`#1302`: https://github.com/pyeve/eve/issues/1302
.. _`#1296`: https://github.com/pyeve/eve/issues/1296
.. _`#1293`: https://github.com/pyeve/eve/issues/1293
Expand Down
15 changes: 9 additions & 6 deletions eve/methods/delete.py
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,10 @@
import copy


def all_done():
return {}, None, None, 204


@ratelimit()
@requires_auth("item")
@pre_event
Expand Down Expand Up @@ -102,7 +106,7 @@ def deleteitem_internal(
**lookup
)
if not original or (soft_delete_enabled and original.get(config.DELETED) is True):
abort(404)
return all_done()

# notify callbacks
if suppress_callbacks is not True:
Expand Down Expand Up @@ -182,7 +186,7 @@ def deleteitem_internal(
getattr(app, "on_deleted_item")(resource, original)
getattr(app, "on_deleted_item_%s" % resource)(original)

return {}, None, None, 204
return all_done()


@requires_auth("resource")
Expand Down Expand Up @@ -220,20 +224,19 @@ def delete(resource, **lookup):
result, _ = app.data.find(resource, default_request, lookup)
originals = list(result)
if not originals:
abort(404)
return all_done()
# I add new callback as I want the framework to be retro-compatible
getattr(app, "on_delete_resource_originals")(resource, originals, lookup)
getattr(app, "on_delete_resource_originals_%s" % resource)(originals, lookup)
id_field = resource_def["id_field"]

if resource_def["soft_delete"]:
# I need to check that I have at least some documents not soft_deleted
# Otherwise, I should abort 404
# I skip all the soft_deleted documents
originals = [x for x in originals if x.get(config.DELETED) is not True]
if not originals:
# Nothing to be deleted
abort(404)
return all_done()
for document in originals:
lookup[id_field] = document[id_field]
deleteitem_internal(
Expand All @@ -258,4 +261,4 @@ def delete(resource, **lookup):
getattr(app, "on_deleted_resource")(resource)
getattr(app, "on_deleted_resource_%s" % resource)()

return {}, None, None, 204
return all_done()
10 changes: 5 additions & 5 deletions eve/tests/methods/delete.py
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,7 @@ def test_delete(self):
def test_delete_non_existant(self):
url = self.item_id_url[:-5] + "00000"
r, status = self.delete(url, headers=self.etag_headers)
self.assert404(status)
self.assert204(status)

def test_delete_write_concern(self):
# should get a 500 since there's no replicaset on the mongod instance
Expand Down Expand Up @@ -371,7 +371,7 @@ def soft_delete_item(etag):

def test_multiple_softdelete(self):
"""After an item has been soft deleted, subsequent DELETEs should
return a 404 Not Found response.
return a 204 Not Found response.
"""
r, status = self.delete(self.item_id_url, headers=self.etag_headers)
self.assert204(status)
Expand All @@ -381,7 +381,7 @@ def test_multiple_softdelete(self):

# Second soft DELETE should return 404 Not Found
r, status = self.delete(self.item_id_url, headers=[("If-Match", new_etag)])
self.assert404(status)
self.assert204(status)

def test_softdelete_deleted_field(self):
"""The configured 'deleted' field should be added to all documents to indicate
Expand Down Expand Up @@ -738,9 +738,9 @@ def filter_this(resource, request, lookup):
lookup["_id"] = self.unknown_item_id

self.app.on_pre_DELETE += filter_this
# Would normally delete the known document; will return 404 instead.
# Would normally delete the known document; will return 204 instead.
r, s = self.parse_response(self.delete_item())
self.assert404(s)
self.assert204(s)

def test_on_post_DELETE_for_item(self):
devent = DummyEvent(self.after_delete)
Expand Down

0 comments on commit 398c78b

Please sign in to comment.