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

DELETE response by default #314

Closed
mingard opened this Issue Jul 6, 2017 · 14 comments

Comments

Projects
3 participants
@mingard
Copy link
Member

mingard commented Jul 6, 2017

Open for discussion:

Current behaviour

I believe there is a flag to enable a return body (I don't know what it is, or if it's documented).

Proposed behaviour

I would like to make a response for a DELETE call default, giving some stats on the number of documents deleted and the number remaining.

@jimlambie

This comment has been minimized.

Copy link
Member

jimlambie commented Jul 7, 2017

@mingard the config option is "feedback": true.

  • when false it returns HTTP 204 No Content
  • when true it does return the number of documents deleted
@mingard

This comment has been minimized.

Copy link
Member Author

mingard commented Jul 10, 2017

@jimlambie I was actually hoping that we could change the default behaviour to true inline with other method requests. Also for it to return full meta including total documents (obviously pagination figures aren't relevant here).

Thoughts?

@jimlambie

This comment has been minimized.

Copy link
Member

jimlambie commented Jul 12, 2017

@mingard @eduardoboucas

Have reviewed the options here, and with the completion of the ticket that added deletedDocs to hooks, this has become much simpler.

I'm going to leave the default for feedback, so that by default it returns 204 No Content.

However, if set to true, the following will be returned. Do note, however, that the totalCount property reflects only the count of the documents in the collection that match the query specified in the DELETE call.

{
  status: 'success',
  message: 'Documents deleted successfully',
  deleted: 1, // documents deleted
  totalCount: 0 // remaining documents THAT MATCH THE QUERY
}

Thoughts?

@eduardoboucas

This comment has been minimized.

Copy link
Member

eduardoboucas commented Jul 12, 2017

Looks good to me! Adds more info and avoids breaking changes. 🎉

@mingard

This comment has been minimized.

Copy link
Member Author

mingard commented Jul 12, 2017

Would changing the default (where no feedback param is set, default to true) be considered a breaking change?

I'd still rather a returned body was the default, not just a 204 No Content header.

@eduardoboucas

This comment has been minimized.

Copy link
Member

eduardoboucas commented Jul 12, 2017

Would changing the default (where no feedback param is set, default to true) be considered a breaking change?

It would, since we'd have to change the response code from 204 to 200.

@mingard

This comment has been minimized.

Copy link
Member Author

mingard commented Jul 12, 2017

Gotcha. Would still prefer a meta response body by default.

@mingard

This comment has been minimized.

Copy link
Member Author

mingard commented Jul 12, 2017

What is the justification for feedback-by-default for GET, PUT, POST but not DELETE?

I've just been testing and it appears that a failed delete (invalid query) returns the same response header. To differentiate between an accepted and unaccepted filter, it would also be useful to return either

  • 200 (Accepted with response)
  • 200 (Not accepted with response)
  • 204 (No Content) when feedback is false and accepted
  • 406 (Not Accepted) when feedback is false and query is not accepted
@jimlambie

This comment has been minimized.

Copy link
Member

jimlambie commented Jul 12, 2017

Sending a 204 as the response for a DELETE is (or was) considered enough feedback. GET obviously needs to return a response body, and PUT & POST are either updating or inserting data so have something to report on. A DELETE removes data, and therefore there is nothing left to say other than "done".

@jimlambie

This comment has been minimized.

Copy link
Member

jimlambie commented Jul 12, 2017

We send a 400 if the query is malformed

@mingard

This comment has been minimized.

Copy link
Member Author

mingard commented Jul 12, 2017

therefore there is nothing left to say other than "done".

Feedback as to what has been done is just as useful for a DELETE as a PUT, therefore should be default. If there is no extra step to know how many documents have been updated, why should there be an extra step to know how many documents have been deleted?

@jimlambie

This comment has been minimized.

Copy link
Member

jimlambie commented Jul 12, 2017

Ok, I get where you're coming from. Unfortunately, this won't be included in the next release (Version 3.0) as a default - until it is you'll have to run with "feedback": true.

@mingard

This comment has been minimized.

Copy link
Member Author

mingard commented Jul 12, 2017

Okay, it's not urgent so let's leave this open until then.

@jimlambie

This comment has been minimized.

Copy link
Member

jimlambie commented Jul 12, 2017

I'm actually modifying this.

The response (with feedback: true) will return a totalCount property that is equal to the number of documents remaining in the collection.

{
  status: 'success',
  message: 'Documents deleted successfully',
  deleted: 1, // documents deleted
  totalCount: 100 // remaining documents in the collection
}

@jimlambie jimlambie removed the help wanted label Jul 13, 2017

@jimlambie jimlambie added this to Backlog in API Version 3.0 Sep 24, 2017

@jimlambie jimlambie moved this from Backlog to Ready for Release in API Version 3.0 Sep 24, 2017

@jimlambie jimlambie closed this Dec 7, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment