File response

[LeSpocky]

Requirements for Contributing a Bug Fix

Identify the Bug

See: #255

Description of the Change

Adds a unit test highlighting the bug, no real fix so far.

Alternate Designs

This snippet at least fixes the webserver to return a 500 Internal Server Error:

diff --git a/src/webserver.cpp b/src/webserver.cpp
index a1392bd..4c4a034 100644
--- a/src/webserver.cpp
+++ b/src/webserver.cpp
@@ -623,6 +623,10 @@ MHD_Result webserver::finalize_answer(MHD_Connection* connection, struct details
     try {
         try {
             raw_response = mr->dhrs->get_raw_response();
+            if (raw_response == nullptr) {
+                mr->dhrs = internal_error_page(mr);
+                raw_response = mr->dhrs->get_raw_response();
+            }
         } catch(const std::invalid_argument& iae) {
             mr->dhrs = not_found_page(mr);
             raw_response = mr->dhrs->get_raw_response();

The unit test just fails different then, but that confirms the unit test itself is ok:

Running test (25): file_serving_resource_missing
[ASSERT FAILURE] (../../test/integ/basic.cpp:923) - error in "file_serving_resource_missing": 500!=404
- Time spent during "file_serving_resource_missing": 0.668945 ms

Possible Drawbacks

n.a.

Verification Process

Tested against multiple different version of libmicrohttpd.

Release Notes

n.a.

[LeSpocky]

As discussed in #255 that diff could be a solution already. The unit test could test on HTTP status NOT equal to 200 instead? Or test equal to 500?

[LeSpocky]

I force pushed a possible solution as discussed in #255, but did not change description etc. of this PR, yet. Should I?

[LeSpocky]

I just discovered another problem: if that filename points to a directory instead of a file … 🙄

[etr]

I just discovered another problem: if that filename points to a directory instead of a file …

We should just manage this in the same way (direction = invalid) and express it in the documentation making it clear that only existing files (not directories) are acceptable.

Type of size taking return value of lseek() should be off_t. That was introduced wrong with f9b7691. Could be fixed in a separate commit or just fixed here en passant.

Given the broad changes that are going to happen in this method, I think we can just do it in this change.

[LeSpocky]

Those unit test are very repetitive. Maybe that can be improved later?

[etr]

Those unit test are very repetitive. Maybe that can be improved later?

Yeah, I think it would be worth keeping a change to those in an isolated change.

[LeSpocky]

I updated the API doc to the constructor of the file_response class as suggested. However on that other remark of yours …

Can we also add information about the method new failing path in the README file?

Not sure in what direction that should go? There is this section Building responses to requests which describes the different flavors of http_response classes. The improved failure handling was in the get_raw_response() method and over where it is called in finalize_answer(). Instead of crashing or sending back potentially nonsense responses, libhttpserver returns 500 now, and explicitly expects the user to make sure a file used for the file_response must exist and be a regular file. It was a good idea to check that on user side before already. What do you have in mind should be added to the README and where? 🤔

[etr]

In the section called "Building responses to requests" there is piece dedicated to the file_response.

file_response(const std::string& filename, int response_code = 200, const std::string& content_type = "text/plain"): Uses the filename passed in construction as pointer to a file on disk. The body of the HTTP response will be set using the content of the file. The other two optional parameters are the response_code and the content_type. You can find constant definition for the various response codes within the http_utils library file.

Don't need to make it too complicated. I'd just change it as below (the diff is in bold):

file_response(const std::string& filename, int response_code = 200, const std::string& content_type = "text/plain"): Uses the filename passed in construction as pointer to a file on disk. The body of the HTTP response will be set using the content of the file. The file must be a regular file and exist on disk. Otherwise libhttpserver will return an error 500 (Internal Server Error). The other two optional parameters are the response_code and the content_type. You can find constant definition for the various response codes within the http_utils library file.

[etr]

Thanks! This was a great piece of improvement for the library.