Skip to content
This repository was archived by the owner on Aug 19, 2019. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/api_server.cc
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ MetadataApiServer::Dispatcher::Dispatcher(

void MetadataApiServer::Dispatcher::operator()(
const HttpServer::request& request,
std::shared_ptr<HttpServer::connection> conn) {
std::shared_ptr<HttpServer::connection> conn) const {
if (verbose_) {
LOG(INFO) << "Dispatcher called: " << request.method
<< " " << request.destination
Expand All @@ -40,7 +40,7 @@ void MetadataApiServer::Dispatcher::operator()(
}
// Look for the longest match first. This means going backwards through
// the map, since strings are sorted in lexicographical order.
for (auto it = handlers_.crbegin(); it != handlers_.crend(); --it) {
for (auto it = handlers_.crbegin(); it != handlers_.crend(); ++it) {
const std::string& method = it->first.first;
const std::string& prefix = it->first.second;
#ifdef VERBOSE
Expand All @@ -61,7 +61,7 @@ void MetadataApiServer::Dispatcher::operator()(
}
}

void MetadataApiServer::Dispatcher::log(const HttpServer::string_type& info) {
void MetadataApiServer::Dispatcher::log(const HttpServer::string_type& info) const {
LOG(ERROR) << info;
}

Expand Down
6 changes: 4 additions & 2 deletions src/api_server.h
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,8 @@ class MetadataApiServer {
~MetadataApiServer();

private:
friend class ApiServerTest;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is actually a bug in line 43 of api_server.cc — reverse iterators should still be incremented with ++. Can you please change --it to ++it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another thing we can clean up here is making both operator() and log() const in Dispatcher. Can you please do this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

class Dispatcher;
using HttpServer = http::server<Dispatcher>;
using Handler = std::function<void(const HttpServer::request&,
Expand All @@ -54,8 +56,8 @@ class MetadataApiServer {
using HandlerMap = std::map<std::pair<std::string, std::string>, Handler>;
Dispatcher(const HandlerMap& handlers, bool verbose);
void operator()(const HttpServer::request& request,
std::shared_ptr<HttpServer::connection> conn);
void log(const HttpServer::string_type& info);
std::shared_ptr<HttpServer::connection> conn) const;
void log(const HttpServer::string_type& info) const;
private:
// A mapping from a method/prefix pair to the handler function.
// Order matters: later entries override earlier ones.
Expand Down
24 changes: 16 additions & 8 deletions test/Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ YAML_CPP_LIBS=$(YAML_CPP_LIBDIR)/libyaml-cpp.a
CPPFLAGS+= \
-DENABLE_KUBERNETES_METADATA \
-isystem $(GTEST_DIR)/include -I$(GMOCK_DIR)/include \
-I$(YAML_CPP_DIR)/include
-I$(CPP_NETLIB_DIR) -I$(NETWORK_URI_DIR)/include -I$(YAML_CPP_DIR)/include
CXXFLAGS=\
-std=c++11 -g -pthread -Wno-write-strings -Wno-deprecated
LDFLAGS=-L$(CPP_NETLIB_LIBDIR) -L$(NETWORK_URI_LIBDIR) -L$(YAML_CPP_LIBDIR)
Expand All @@ -47,6 +47,7 @@ TEST_DIR=.
TEST_SOURCES=$(wildcard $(TEST_DIR)/*_unittest.cc)
TEST_OBJS=$(TEST_SOURCES:$(TEST_DIR)/%.cc=%.o)
TESTS=\
api_server_unittest \
base64_unittest \
configuration_unittest \
environment_unittest \
Expand All @@ -62,7 +63,7 @@ TESTS=\

GTEST_LIB=gtest_lib.a

all: $(TESTS)
all: $(SRC_DIR)/build-cpp-netlib $(SRC_DIR)/build-yaml-cpp $(TESTS)

# TODO: Running the test prints "Running main() from gtest_main.cc".
# Figure out how to fix this.
Expand All @@ -83,13 +84,17 @@ init-submodules:
git submodule update --init $(GTEST_MODULE)
touch init-submodules

$(CPP_NETLIB_LIBS):
cd $(SRC_DIR) && $(MAKE) $@
$(SRC_DIR)/init-submodules:
cd $(SRC_DIR) && $(MAKE) $(@:$(SRC_DIR)/%=%)

$(SRC_DIR)/build-cpp-netlib $(SRC_DIR)/build-yaml-cpp: $(SRC_DIR)/init-submodules
cd $(SRC_DIR) && $(MAKE) $(@:$(SRC_DIR)/%=%)

$(YAML_CPP_LIBS):
cd $(SRC_DIR) && $(MAKE) $@
$(CPP_NETLIB_LIBS): $(SRC_DIR)/build-cpp-netlib

$(SRC_DIR)/%.o: $(SRC_DIR)/%.cc
$(YAML_CPP_LIBS): $(SRC_DIR)/build-yaml-cpp

$(SRC_DIR)/%.o: $(SRC_DIR)/build-cpp-netlib $(SRC_DIR)/build-yaml-cpp $(SRC_DIR)/%.cc
cd $(SRC_DIR) && $(MAKE) $(@:$(SRC_DIR)/%=%)

$(GTEST_SOURCEDIR)/gtest-all.cc $(GTEST_SOURCEDIR)/gtest_main.cc: init-submodules
Expand All @@ -106,8 +111,11 @@ $(GTEST_LIB): gtest-all.o gtest_main.o
$(TESTS): $(GTEST_LIB) $(CPP_NETLIB_LIBS) $(YAML_CPP_LIBS)

# All unittest objects depend on GTEST_LIB.
$(TESTS:%=%.o): $(GTEST_LIB)
# Some headers need CPP_NETLIB_LIBS and YAML_CPP_LIBS.
$(TESTS:%=%.o): $(GTEST_LIB) $(CPP_NETLIB_LIBS) $(YAML_CPP_LIBS)

api_server_unittest: api_server_unittest.o $(SRC_DIR)/api_server.o $(SRC_DIR)/configuration.o $(SRC_DIR)/store.o $(SRC_DIR)/json.o $(SRC_DIR)/resource.o $(SRC_DIR)/logging.o $(SRC_DIR)/time.o
$(CXX) $(LDFLAGS) $^ $(LDLIBS) -o $@
base64_unittest: base64_unittest.o $(SRC_DIR)/base64.o
$(CXX) $(LDFLAGS) $^ $(LDLIBS) -o $@
configuration_unittest: configuration_unittest.o $(SRC_DIR)/configuration.o
Expand Down
82 changes: 82 additions & 0 deletions test/api_server_unittest.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
#include "../src/api_server.h"
#include "../src/configuration.h"
#include "../src/store.h"
#include "gtest/gtest.h"

namespace google {

class ApiServerTest : public ::testing::Test {
protected:
using Dispatcher = MetadataApiServer::Dispatcher;
std::unique_ptr<Dispatcher> CreateDispatcher(
const std::map<std::pair<std::string, std::string>,
std::function<void()>>& handlers) {
Dispatcher::HandlerMap handler_map;
for (const auto& element : handlers) {
std::function<void()> handler = element.second;
handler_map.emplace(element.first, [handler](
const MetadataApiServer::HttpServer::request& request,
std::shared_ptr<MetadataApiServer::HttpServer::connection> conn) {
handler();
});
}
return std::unique_ptr<Dispatcher>(new Dispatcher(handler_map, false));
}

void InvokeDispatcher(
const std::unique_ptr<Dispatcher>& dispatcher,
const std::string& method, const std::string& path) {
MetadataApiServer::HttpServer::request request;
request.method = method;
request.destination = path;
(*dispatcher)(request, nullptr);
}
};

TEST_F(ApiServerTest, DispatcherMatchesFullPath) {
bool handler_called = false;
bool bad_handler_called = false;
std::unique_ptr<Dispatcher> dispatcher = CreateDispatcher({
{{"GET", "/testPath/"}, [&handler_called]() {
handler_called = true;
}},
{{"GET", "/badPath/"}, [&bad_handler_called]() {
bad_handler_called = true;
}},
});

InvokeDispatcher(dispatcher, "GET", "/testPath/");
EXPECT_TRUE(handler_called);
EXPECT_FALSE(bad_handler_called);
}

TEST_F(ApiServerTest, DispatcherUnmatchedMethod) {
bool handler_called = false;
std::unique_ptr<Dispatcher> dispatcher = CreateDispatcher({
{{"GET", "/testPath/"}, [&handler_called]() {
handler_called = true;
}},
});

InvokeDispatcher(dispatcher, "POST", "/testPath/");
EXPECT_FALSE(handler_called);
}

TEST_F(ApiServerTest, DispatcherSubstringCheck) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this also check that an actual substring does match (e.g., /testPath/subPath/)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, added

bool handler_called = false;
std::unique_ptr<Dispatcher> dispatcher = CreateDispatcher({
{{"GET", "/testPath/"}, [&handler_called]() {
handler_called = true;
}},
});

InvokeDispatcher(dispatcher, "GET", "/testPathFoo/");
EXPECT_FALSE(handler_called);
InvokeDispatcher(dispatcher, "GET", "/test/");
EXPECT_FALSE(handler_called);
InvokeDispatcher(dispatcher, "GET", "/testFooPath/");
EXPECT_FALSE(handler_called);
InvokeDispatcher(dispatcher, "GET", "/testPath/subPath/");
EXPECT_TRUE(handler_called);
}
} // namespace google