search

innocampus / videbo

Distributed video hosting for Moodle and other LMS
https://innocampus.github.io/videbo/
0 stars 0 forks source link

Handle file name error in `DELETE` API route #20

Open daniil-berg opened 1 week ago

daniil-berg commented 1 week ago

Requesting DELETE from the /api/file/<hash><ext> endpoint currently provides no feedback, as to whether the storage node even controls a file with the provided hash/extension at all. If it does not, a warning is logged, but the client receives the same 200 response either way.

https://github.com/innocampus/videbo/blob/e7b53e0d6668e2b47b8a9a5342e8123a98845f12/src/videbo/storage/file_controller.py#L410-L417

No feedback/guarantees about successful deletion makes sense, but if the client application provided an invalid/unknown hash, it would be polite to respond accordingly. In this case, a 404 seems appropriate. The client is still free to ignore such a status code, but it may also be valuable information, in case there is a bug on the other end.

@larsbonczek What do you think? The way I see it, there are no implications for mod_videoservice because it already ignores any non-200 response to a deletion request anyway. If anything, this is an opportunity to log this specific 404 response on that end, too. That way we could more easily identify problems in setups with multiple LMS connected to one Videbo server. We always want to make sure that a file is only deleted, if it is no longer needed on any registered LMS. Better logging in the deletion process just seems like a good idea. (That is a topic for mod_videoservice, I just wanted to share my motivation for this issue here.)

larsbonczek commented 6 days ago

I agree that it makes sense to return a 404 status code if the specified file was not found. Although it would probably be even more interesting to receive a 500 status code if the deletion fails, because in that case the LMS may want to try again or log an error.