HTTP routing with Mongoose

Question

I'm trying to extend the Mongoose RESTful server example with URL routing. This code relies on the Mongoose library available here.

This is heavily inspired by the routes class from this question but I've tried to use a multimap to store routes and make lookup more efficient, as well as support Mongoose's wildcard matches.

I've also added some unit tests (and intend to add more), but have had to rely on checking a side effect to determine test success. Is there a better way to do this?

CMakeLists.txt

cmake_minimum_required(VERSION 3.14)
project ("Mongoose_Routes")
set(CMAKE_CXX_STANDARD 17)
add_compile_options(-Werror -Wall -Wextra)
add_executable (Mongoose_Routes "main.cpp" "routes.cpp" "routes.h" "mongoose/mongoose.c")
include(FetchContent)
FetchContent_Declare(
 googletest
 URL https://github.com/google/googletest/archive/refs/tags/release-1.12.1.zip
)
# For Windows: Prevent overriding the parent project's compiler/linker settings
set(gtest_force_shared_crt ON CACHE BOOL "" FORCE)
FetchContent_MakeAvailable(googletest)
add_executable(UnitTests "tests/routes_test.cpp" "routes.cpp" "routes.h" "mongoose/mongoose.c")
target_link_libraries(UnitTests PUBLIC gtest_main)

Routes

#ifndef _ROUTES_H_INCLUDED_
#define _ROUTES_H_INCLUDED_
#include <functional>
#include <map>
#include <string>
extern "C"
{
 #include "mongoose/mongoose.h"
}
namespace Routes
{
 enum class HTTPMethod
 {
 NONE,
 GET,
 POST,
 PUT,
 FIRST = GET,
 LAST = PUT
 };
 enum class RouteStatus
 {
 SUCCESS,
 E_NOROUTE,
 E_NOMETHOD
 };
 typedef std::function<void(struct mg_connection*, struct mg_http_message*, void*)> RouteCallback;
 class RouteManager
 {
 public:
 RouteStatus GetRouteCallback(mg_str path, HTTPMethod method, RouteCallback& callback) const;
 bool SetRoute(std::string path, HTTPMethod method, std::function<void(struct mg_connection*, struct mg_http_message*, void*)> callback);
 private:
 typedef std::pair<std::string, HTTPMethod> RouteID;
 struct RouteCompare
 {
 bool operator()(const RouteID& lhs, const RouteID& rhs) const
 {
 if (lhs.second == rhs.second)
 {
 size_t i = 0, j = 0, ni = 0, nj = 0;
 std::string a = lhs.first, b = rhs.first;
 while (i < a.size() || j < b.size())
 {
 if (i < a.size() && j < b.size() && (a[i] == '?' || b[j] == a[i]))
 {
 i++; j++;
 }
 else if (i < a.size() && (a[i] == '*' || a[i] == '#'))
 {
 ni = i++; nj = j + 1;
 }
 else if (nj > 0 && nj <= b.size() && (a[ni] == '#' || b[j] != '/'))
 {
 i = ni; j = nj;
 }
 else
 {
 if (i == j) return a[i] < b[j];
 else return i < j;
 }
 }
 return false;
 }
 else return lhs.second < rhs.second;
 }
 };
 std::multimap<RouteID, RouteCallback, RouteCompare> routes;
 };
 HTTPMethod ParseHTTPMethod(mg_str method);
}
#endif // _ROUTES_H_INCLUDED_

#include "routes.h"
//----------------------------------------------------------------
//
//----------------------------------------------------------------
Routes::RouteStatus Routes::RouteManager::GetRouteCallback(mg_str path, HTTPMethod method, RouteCallback& callback) const
{
 auto route = RouteID{ std::string(path.ptr, path.len), method };
 auto lower = routes.lower_bound(route);
 if (lower == routes.end() || !mg_match(path, mg_str(lower->first.first.c_str()), NULL)) return RouteStatus::E_NOROUTE;
 else if (lower->first.second != method) return RouteStatus::E_NOMETHOD;
 // Want the longest (and therefore lexicographically largest) path which matches
 auto upper = routes.upper_bound(route);
 while (std::next(lower) != upper) { lower = std::next(lower); }
 callback = lower->second;
 return RouteStatus::SUCCESS;
}
//----------------------------------------------------------------
//
//----------------------------------------------------------------
bool Routes::RouteManager::SetRoute(std::string path, HTTPMethod method,
 std::function<void(struct mg_connection*, struct mg_http_message*, void*)> callback)
{
 if (path.empty()) return false;
 if (method < HTTPMethod::FIRST || method > HTTPMethod::LAST) return false;
 if (callback == nullptr) return false;
 routes.emplace(RouteID{ path, method }, callback);
 return true;
}
//----------------------------------------------------------------
//
//----------------------------------------------------------------
Routes::HTTPMethod Routes::ParseHTTPMethod(mg_str m)
{
 Routes::HTTPMethod method = Routes::HTTPMethod::NONE;
 if (mg_vcmp(&m, "GET") == 0) method = Routes::HTTPMethod::GET;
 else if (mg_vcmp(&m, "POST") == 0) method = Routes::HTTPMethod::POST;
 else if (mg_vcmp(&m, "PUT") == 0) method = Routes::HTTPMethod::PUT;
 return method;
}

Main

#include <memory>
#include "routes.h"
//----------------------------------------------------------------
//
//----------------------------------------------------------------
void fn(struct mg_connection* c, int ev, void* ev_data, void* fn_data)
{
 if (ev == MG_EV_HTTP_MSG)
 {
 Routes::RouteManager* pRouteManager = static_cast<Routes::RouteManager*>(fn_data);
 struct mg_http_message* hm = (struct mg_http_message*)ev_data;
 
 Routes::RouteCallback callback;
 auto status = pRouteManager->GetRouteCallback(hm->uri, Routes::ParseHTTPMethod(hm->method), callback);
 if (status == Routes::RouteStatus::SUCCESS)
 {
 (callback)(c, hm, fn_data);
 }
 else if (status == Routes::RouteStatus::E_NOMETHOD) mg_http_reply(c, 405, NULL, "");
 else if (status == Routes::RouteStatus::E_NOROUTE) mg_http_reply(c, 404, NULL, "");
 }
}
//----------------------------------------------------------------
//
//----------------------------------------------------------------
int main()
{
 struct mg_mgr mgr;
 mg_mgr_init(&mgr);
 auto pRouteManager = std::make_unique<Routes::RouteManager>();
 pRouteManager->SetRoute("/foo/#", Routes::HTTPMethod::GET,
 [](struct mg_connection* c, struct mg_http_message*, void*)
 {
 mg_http_reply(c, 200, NULL, "GET");
 });
 pRouteManager->SetRoute("/foo/#", Routes::HTTPMethod::PUT,
 [](struct mg_connection* c, struct mg_http_message*, void*)
 {
 mg_http_reply(c, 200, NULL, "PUT");
 });
 pRouteManager->SetRoute("/foo/bar", Routes::HTTPMethod::GET,
 [](struct mg_connection* c, struct mg_http_message*, void*)
 {
 mg_http_reply(c, 200, NULL, "BAR");
 });
 if (mg_http_listen(&mgr, "localhost:8000", fn, pRouteManager.get()) == NULL)
 {
 perror("Failed to bind Webserver");
 return EXIT_FAILURE;
 }
 while (true)
 {
 mg_mgr_poll(&mgr, 100);
 }
 mg_mgr_free(&mgr);
 exit(EXIT_SUCCESS);
}

Tests

#include <gtest/gtest.h>
#include <memory>
#include "../routes.h"
class RouteTest : public ::testing::Test
{
protected:
 std::unique_ptr<Routes::RouteManager> pRouteManager;
 void SetUp() override
 {
 pRouteManager = std::make_unique<Routes::RouteManager>();
 }
};
TEST_F(RouteTest, Empty)
{
 Routes::RouteCallback callback;
 auto status = pRouteManager->GetRouteCallback(mg_str("/foo/bar"), Routes::HTTPMethod::GET, callback);
 EXPECT_EQ(status, Routes::RouteStatus::E_NOROUTE);
}
TEST_F(RouteTest, GETSimple)
{
 bool called = false;
 pRouteManager->SetRoute("/foo/bar", Routes::HTTPMethod::GET,
 [&called](struct mg_connection*, struct mg_http_message*, void*)
 {
 called = true;
 });
 pRouteManager->SetRoute("/foo/baz", Routes::HTTPMethod::GET,
 [&called](struct mg_connection*, struct mg_http_message*, void*)
 {
 called = false;
 });
 Routes::RouteCallback callback;
 auto status = pRouteManager->GetRouteCallback(mg_str("/foo/bar"), Routes::HTTPMethod::GET, callback);
 if (status == Routes::RouteStatus::SUCCESS)
 {
 (callback)(nullptr, nullptr, nullptr);
 EXPECT_TRUE(called);
 }
 EXPECT_EQ(status, Routes::RouteStatus::SUCCESS);
}

Edit: This implementation has bugs ("/foo/bar" vs "/foo/#" depends on which route is added first). A second test is required to determine which of the matching routes is largest.

Answer 1

Unnecessary use of smart pointers

You use std::unique_ptr twice, but both times it is unnecessary. In main(), you can just allocate a Route::RoutesManager on the stack:

int main() {
 ...
 Routes::RouteManager routeManager;
 routeManager.setRoute(...);
 ...
 if (mg_http_listen(&mgr, "localhost:8000", fn, &routeManager) == NULL) {
 ...
 }
 ...
}

And in the test fixture you also can store it by value:

class RouteTest : public ::testing::Test
{
protected:
 Routes::RouteManager routeManager;
};
TEST_F(RouteTest, Empty)
{
 ...
 auto status = routeManager.GetRouteCallback(...);
 ...
}

Make RouteCompare::operator() readable and test it

The function RouteCompare::operator() is completely unreadable. Lots of one or two-letter variable names and no comments. I would also add lots of tests to ensure the comparisons work as expected, and especially test edge cases and weird uses of wildcards.

Create a class RouteID

Instead of using std::pair, I would make RouteID a proper class. That way, you can give names to the two fields, so you no longer have to use .first and .second, which require you to remember the correct order. Also, you can then move the comparison operator into class RouteID, so you no longer have to explicitly specify it when declaring the std::multimap.

Simplify GetRouteCallback()

You can use equal_range() instead of using both lower_bound() upper_bound(), and make use of the fact that you can call std::prev() the end() iterator, if you know that the lower bound was not equal to end():

auto [lower, upper] = routes.equal_range(route);
if (lower == routes.end() || ...)
 return RouteStatus::E_...;
callback = std::prev(upper)->second;

Using side-effects during testing

I've also added some unit tests (and intend to add more), but have had to rely on checking a side effect to determine test success. Is there a better way to do this?

You can make the callback return a value. Of course, just because your tests needed a bool called is not a good reason to have your callbacks return a bool. But I don't think it's necessary; it's perfectly fine to capture a reference to some variable in the callback function. No global variables were used, everything was local to the unit test.

• S Meredith: When using just lower_bound() (first element not less than the key) I was testing mg_match again to ensure I have equality. Can this test be simplified after using equal_range()? I'm considering if (lower == routes.end() || lower == upper) ... but I'm not certain if that is correct.
• G. Sliepen: Maybe, dpeending how hte comparator function works? You could have done that with your use of lower_bound() and upper_bound() then as well.
• S Meredith: I think before I was deliberately delaying upper_bound() until I had checked that lower_bound() wasn't end(), so I didn't think about it. I'll test the comparator as you suggest, will probably clear up whether that check works.
• G. Sliepen: equal_range() is faster than calling both lower_bound() and upper_bound() separately. Also think about the most likely case: will people request URIs that your HTTP server can serve, or will they request non-existing URIs?
• S Meredith: I completely agree equal_range() is better here, I simply didn't know about it. I was really just querying the 'did I find any matching keys' check.