Skip to content

Commit

Permalink
Merge pull request pistacheio#400 from dennisjenkins75/router_notfoun…
Browse files Browse the repository at this point in the history
…d_fix

Resolves bug pistacheio#323: double invocation of router's notFound handler.
  • Loading branch information
dennisjenkins75 authored Dec 31, 2018
2 parents 5f62bfd + 6e432b3 commit 7fe7bb5
Show file tree
Hide file tree
Showing 2 changed files with 66 additions and 14 deletions.
20 changes: 6 additions & 14 deletions src/server/router.cc
Original file line number Diff line number Diff line change
Expand Up @@ -321,19 +321,7 @@ RouterHandler::onRequest(
Http::ResponseWriter response)
{
auto resp = response.clone();
auto result = router->route(req, std::move(resp));

/* @Feature: add support for a custom NotFound handler */
if (result == Route::Status::NotFound)
{
if (router->hasNotFoundHandler())
{
auto resp2 = response.clone();
router->invokeNotFoundHandler(req, std::move(resp2));
}
else
response.send(Http::Code::Not_Found, "Could not find a matching route");
}
router->route(req, std::move(resp));
}

} // namespace Private
Expand Down Expand Up @@ -453,7 +441,11 @@ Router::route(const Http::Request& req, Http::ResponseWriter response) {
if (handler1 == Route::Result::Ok) return Route::Status::Match;
}

if (hasNotFoundHandler()) invokeNotFoundHandler(req, std::move(response));
if (hasNotFoundHandler()) {
invokeNotFoundHandler(req, std::move(response));
} else {
response.send(Http::Code::Not_Found, "Could not find a matching route");
}
return Route::Status::NotFound;
}

Expand Down
60 changes: 60 additions & 0 deletions tests/router_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,13 @@
#include "gtest/gtest.h"
#include <algorithm>

#include <pistache/endpoint.h>
#include <pistache/http.h>
#include <pistache/router.h>

#include "httplib.h"

using namespace Pistache;
using namespace Pistache::Rest;

bool match(const SegmentTreeNode& routes, const std::string& req) {
Expand Down Expand Up @@ -144,3 +149,58 @@ TEST(router_test, test_mixed) {
ASSERT_FALSE(matchSplat(routes, "/hello", { "hello" }));
ASSERT_TRUE(matchSplat(routes, "/hi", { "hi" }));
}

TEST(router_test, test_notfound_exactly_once) {
Address addr(Ipv4::any(), 0);
auto endpoint = std::make_shared<Http::Endpoint>(addr);

auto opts = Http::Endpoint::options().threads(1).maxPayload(4096);
endpoint->init(opts);

int count_found = 0;
int count_not_found = 0;

Rest::Router router;
Routes::NotFound(router, [&count_not_found](
const Pistache::Rest::Request& request,
Pistache::Http::ResponseWriter response) {
count_not_found++;
std::string err{"Couldn't find route: \"" + request.resource() +
"\"\n"};
response.send(Pistache::Http::Code::Not_Found, err);
return Pistache::Rest::Route::Result::Ok;
});
Routes::Get(router, "/moogle", [&count_found](
const Pistache::Rest::Request&,
Pistache::Http::ResponseWriter response) {
count_found++;
response.send(Pistache::Http::Code::Ok, "kupo!\n");
return Pistache::Rest::Route::Result::Ok;
});

endpoint->setHandler(router.handler());
endpoint->serveThreaded();
const auto bound_port = endpoint->getPort();
httplib::Client client("localhost", bound_port);

// Verify that the notFound handler is NOT called when route is found.
count_not_found = count_found = 0;
client.Get("/moogle");
ASSERT_EQ(count_found, 1);
ASSERT_EQ(count_not_found, 0);

// Verify simple solution to bug #323 (one bad url triggered 2 routes).
count_not_found = count_found = 0;
client.Get("/kefka");
ASSERT_EQ(count_found, 0);
ASSERT_EQ(count_not_found, 1);

// Anal test, 2 calls = 2 route hits.
count_not_found = count_found = 0;
client.Get("/vicks");
client.Get("/wedge");
ASSERT_EQ(count_found, 0);
ASSERT_EQ(count_not_found, 2);

endpoint->shutdown();
}

0 comments on commit 7fe7bb5

Please sign in to comment.