OBPIH-6021 Add an API endpoint to delete a product supplier + remove unnecessary url mapping #4442
Conversation
…nge the url on the frontend to be in plural form (productSuppliers, not productSupplier)
kchelstowski
requested review from
alannadolny,
awalkowiak,
drodzewicz and
jmiranda
| productSupplierService.delete(params.id) | ||
| render status: 204 |
There was a problem hiding this comment.
As I know, the delete method from @Service won't throw an exception if the entity we want to delete doesn't exist. Shouldn't we return another status in this case? Or we won't tell the frontend part whether this entity is deleted?
There was a problem hiding this comment.
@alannadolny I described the decision under the ticket. I apologize not for tagging everyone there.
There was a problem hiding this comment.
It's definitely an important decision in terms of coding conventions, so I suppose we'll discuss it in details.
There was a problem hiding this comment.
Yeah it's a good question. For what it's worth 204 = "No Content", so it's not saying "delete was successful".
It looks like the Grails docs imply that they always return 204.
https://docs.grails.org/5.2.4/guide/REST.html#_implementing_the_delete_action
I would be interested to see what a scaffolded controller looks like when generated in Grails 3.
As for the REST convention, it seems like 204 on the first attempt and then 404 on subsequent attempts is an accepted convention (based on the upvotes).
https://stackoverflow.com/questions/6439416/status-code-when-deleting-a-resource-using-http-delete-for-the-second-time
I need to read a little bit more, but I feel like returning 204 would be fine for now (and maybe the proper solution because it's not leaking implementation details i.e. 404 is telling me something that I wouldn't otherwise know). With that said, 204 is misleading.
So let's leave it as 204 for now. I'll add a tech debt label and we can come back to this when we've had some more time to investigate.
…unnecessary url mapping (#4442) * OBPIH-6021 Remove unnecessary url mapping for productSupplier and change the url on the frontend to be in plural form (productSuppliers, not productSupplier) * OBPIH-6021 Add an api endpoint to delete a product supplier
4 participants
It seems not to be necessary to add a url mapping for this (and it was not necessary to add it for the list method), as it's handled fine by the generic mappings:
By doing so, I've also applied @jmiranda 's suggestion from here to change the url to be in plural form (productSuppliers, not productSupplier).