Skip to content

Commit

Permalink
test and reduce code duplication
Browse files Browse the repository at this point in the history
  • Loading branch information
@Qup42
Qup42 committed Oct 27, 2024
1 parent f2d9975 commit dadc2df
Show file tree
Hide file tree
Showing 3 changed files with 100 additions and 61 deletions.
54 changes: 24 additions & 30 deletions src/engine/Server.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -347,8 +347,17 @@ Awaitable<void> Server::process(
auto checkParameter = [&parameters](std::string_view key,
std::optional<std::string> value,
bool accessAllowed = true) {
return Server::checkParameter(parameters, key, std::move(value),
accessAllowed);
std::optional<string> param =
Server::checkParameter(parameters, key, std::move(value));

Check warning on line 351 in src/engine/Server.cpp

View check run for this annotation

Codecov / codecov/patch

src/engine/Server.cpp#L350-L351 

Added lines #L350 - L351 were not covered by tests
// Now that we have the value, check if there is a problem with the access.
// If yes, we abort the query processing at this point.
if (param && !accessAllowed) {
throw std::runtime_error(absl::StrCat("Access to \"", key, "=",
param.value(), "\" denied",
" (requires a valid access token)",
", processing of request aborted"));
}
return param;

Check warning on line 360 in src/engine/Server.cpp

View check run for this annotation

Codecov / codecov/patch

src/engine/Server.cpp#L355-L360 

Added lines #L355 - L360 were not covered by tests
};

// Check the access token. If an access token is provided and the check fails,
Expand Down Expand Up @@ -504,19 +513,13 @@ Awaitable<void> Server::process(
parsedHttpRequest.operation_);
}

// ____________________________________________________________________________
bool containsParam(const auto& params, const std::string& param,
const std::string& expected) {
auto parameterValue =
ad_utility::url_parser::getParameterCheckAtMostOnce(params, param);
return parameterValue == expected;
};

// ____________________________________________________________________________
std::pair<bool, bool> Server::determineResultPinning(
const ad_utility::url_parser::ParamValueMap& params) const {
const bool pinSubtrees = containsParam(params, "pinsubtrees", "true");
const bool pinResult = containsParam(params, "pinresult", "true");
const ad_utility::url_parser::ParamValueMap& params) {
const bool pinSubtrees =
checkParameter(params, "pinsubtrees", "true").has_value();
const bool pinResult =
checkParameter(params, "pinresult", "true").has_value();
return {pinSubtrees, pinResult};
}
// ____________________________________________________________________________
Expand Down Expand Up @@ -716,7 +719,7 @@ class NoSupportedMediatypeError : public std::runtime_error {
// ____________________________________________________________________________
MediaType Server::determineMediaType(
const ad_utility::url_parser::ParamValueMap& params,
const ad_utility::httpUtils::HttpRequest auto& request) const {
const ad_utility::httpUtils::HttpRequest auto& request) {
// The following code block determines the media type to be used for the
// result. The media type is either determined by the "Accept:" header of
// the request or by the URL parameter "action=..." (for TSV and CSV export,
Expand All @@ -725,17 +728,17 @@ MediaType Server::determineMediaType(

// The explicit `action=..._export` parameter have precedence over the
// `Accept:...` header field
if (containsParam(params, "action", "csv_export")) {
if (checkParameter(params, "action", "csv_export")) {
mediaType = MediaType::csv;
} else if (containsParam(params, "action", "tsv_export")) {
} else if (checkParameter(params, "action", "tsv_export")) {
mediaType = MediaType::tsv;
} else if (containsParam(params, "action", "qlever_json_export")) {
} else if (checkParameter(params, "action", "qlever_json_export")) {
mediaType = MediaType::qleverJson;
} else if (containsParam(params, "action", "sparql_json_export")) {
} else if (checkParameter(params, "action", "sparql_json_export")) {
mediaType = MediaType::sparqlJson;
} else if (containsParam(params, "action", "turtle_export")) {
} else if (checkParameter(params, "action", "turtle_export")) {
mediaType = MediaType::turtle;
} else if (containsParam(params, "action", "binary_export")) {
} else if (checkParameter(params, "action", "binary_export")) {
mediaType = MediaType::octetStream;
}

Expand Down Expand Up @@ -1018,11 +1021,9 @@ bool Server::checkAccessToken(
}

// _____________________________________________________________________________

std::optional<std::string> Server::checkParameter(
const ad_utility::url_parser::ParamValueMap& parameters,
std::string_view key, std::optional<std::string> value,
bool accessAllowed) {
std::string_view key, std::optional<std::string> value) {
auto param =
ad_utility::url_parser::getParameterCheckAtMostOnce(parameters, key);
if (!param.has_value()) {
Expand All @@ -1037,12 +1038,5 @@ std::optional<std::string> Server::checkParameter(
} else if (value != parameterValue) {
return std::nullopt;
}
// Now that we have the value, check if there is a problem with the access.
// If yes, we abort the query processing at this point.
if (!accessAllowed) {
throw std::runtime_error(absl::StrCat(
"Access to \"", key, "=", value.value(), "\" denied",
" (requires a valid access token)", ", processing of request aborted"));
}
return value;
}
31 changes: 15 additions & 16 deletions src/engine/Server.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -151,12 +151,14 @@ class Server {
// Determine the media type to be used for the result. The media type is
// determined (in this order) by the current action (e.g.,
// "action=csv_export") and by the "Accept" header of the request.
ad_utility::MediaType determineMediaType(
static ad_utility::MediaType determineMediaType(
const ad_utility::url_parser::ParamValueMap& params,
const ad_utility::httpUtils::HttpRequest auto& request) const;
// Determine whether the subtress and the result should be pinned.
std::pair<bool, bool> determineResultPinning(
const ad_utility::url_parser::ParamValueMap& params) const;
const ad_utility::httpUtils::HttpRequest auto& request);
FRIEND_TEST(ServerTest, determineMediaType);
// Determine whether the subtrees and the result should be pinned.
static std::pair<bool, bool> determineResultPinning(
const ad_utility::url_parser::ParamValueMap& params);
FRIEND_TEST(ServerTest, determineResultPinning);
// Sets up the PlannedQuery s.t. it is ready to be executed.
Awaitable<PlannedQuery> setupPlannedQuery(
const ad_utility::url_parser::ParamValueMap& params,
Expand Down Expand Up @@ -234,19 +236,16 @@ class Server {
/// HTTP error response.
bool checkAccessToken(std::optional<std::string_view> accessToken) const;

/// Checks if a URL parameter exists in the request, if we are allowed to
/// access it and it matches the expected `value`. If yes, return the value,
/// otherwise return `std::nullopt`. If `value` is `std::nullopt`, only check
/// if the key exists. We need this because we have parameters like
/// "cmd=stats", where a fixed combination of the key and value determines the
/// kind of action, as well as parameters like "index-decription=...", where
/// the key determines the kind of action. If the key is not found, always
/// return `std::nullopt`. If `accessAllowed` is false and a value is present,
/// throw an exception.
/// Checks if a URL parameter exists in the request, and it matches the
/// expected `value`. If yes, return the value, otherwise return
/// `std::nullopt`. If `value` is `std::nullopt`, only check if the key
/// exists. We need this because we have parameters like "cmd=stats", where a
/// fixed combination of the key and value determines the kind of action, as
/// well as parameters like "index-decription=...", where the key determines
/// the kind of action. If the key is not found, always return `std::nullopt`.
static std::optional<std::string> checkParameter(
const ad_utility::url_parser::ParamValueMap& parameters,
std::string_view key, std::optional<std::string> value,
bool accessAllowed);
std::string_view key, std::optional<std::string> value);
FRIEND_TEST(ServerTest, checkParameter);

/// Check if user-provided timeout is authorized with a valid access-token or
Expand Down
76 changes: 61 additions & 15 deletions test/ServerTest.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -146,29 +146,75 @@ TEST(ServerTest, checkParameter) {
const ParamValueMap exampleParams = {{"foo", {"bar"}},
{"baz", {"qux", "quux"}}};

EXPECT_THAT(Server::checkParameter(exampleParams, "doesNotExist", "", false),
EXPECT_THAT(Server::checkParameter(exampleParams, "doesNotExist", ""),
testing::Eq(std::nullopt));
EXPECT_THAT(Server::checkParameter(exampleParams, "foo", "baz", false),
EXPECT_THAT(Server::checkParameter(exampleParams, "foo", "baz"),
testing::Eq(std::nullopt));
AD_EXPECT_THROW_WITH_MESSAGE(
Server::checkParameter(exampleParams, "foo", "bar", false),
testing::StrEq("Access to \"foo=bar\" denied (requires a valid access "
"token), processing of request aborted"));
EXPECT_THAT(Server::checkParameter(exampleParams, "foo", "bar", true),
EXPECT_THAT(Server::checkParameter(exampleParams, "foo", "bar"),
testing::Optional(testing::StrEq("bar")));
AD_EXPECT_THROW_WITH_MESSAGE(
Server::checkParameter(exampleParams, "baz", "qux", false),
Server::checkParameter(exampleParams, "baz", "qux"),
testing::StrEq("Parameter \"baz\" must be given exactly once. Is: 2"));
EXPECT_THAT(Server::checkParameter(exampleParams, "foo", std::nullopt, true),
EXPECT_THAT(Server::checkParameter(exampleParams, "foo", std::nullopt),
testing::Optional(testing::StrEq("bar")));
AD_EXPECT_THROW_WITH_MESSAGE(
Server::checkParameter(exampleParams, "foo", std::nullopt, false),
testing::StrEq("Access to \"foo=bar\" denied (requires a valid access "
"token), processing of request aborted"));
AD_EXPECT_THROW_WITH_MESSAGE(
Server::checkParameter(exampleParams, "baz", std::nullopt, false),
Server::checkParameter(exampleParams, "baz", std::nullopt),
testing::StrEq("Parameter \"baz\" must be given exactly once. Is: 2"));
AD_EXPECT_THROW_WITH_MESSAGE(
Server::checkParameter(exampleParams, "baz", std::nullopt, true),
Server::checkParameter(exampleParams, "baz", std::nullopt),
testing::StrEq("Parameter \"baz\" must be given exactly once. Is: 2"));
}

TEST(ServerTest, determineResultPinning) {
EXPECT_THAT(Server::determineResultPinning(
{{"pinsubtrees", {"true"}}, {"pinresult", {"true"}}}),
testing::Pair(true, true));
EXPECT_THAT(Server::determineResultPinning({{"pinresult", {"true"}}}),
testing::Pair(false, true));
EXPECT_THAT(Server::determineResultPinning({{"pinsubtrees", {"otherValue"}}}),
testing::Pair(false, false));
}

TEST(ServerTest, determineMediaType) {
auto MakeRequest = [](const std::optional<std::string>& accept,
const http::verb method = http::verb::get,
const std::string& target = "/",
const std::string& body = "") {
auto req = http::request<http::string_body>{method, target, 11};
if (accept.has_value()) {
req.set(http::field::accept, accept.value());
}
req.body() = body;
req.prepare_payload();
return req;
};
auto checkActionMediatype = [&](const std::string& actionName,
ad_utility::MediaType expectedMediaType) {
EXPECT_THAT(Server::determineMediaType({{"action", {actionName}}},
MakeRequest(std::nullopt)),
testing::Eq(expectedMediaType));
};
// The media type associated with the action overrides the `Accept` header.
EXPECT_THAT(Server::determineMediaType(
{{"action", {"csv_export"}}},
MakeRequest("application/sparql-results+json")),
testing::Eq(ad_utility::MediaType::csv));
checkActionMediatype("csv_export", ad_utility::MediaType::csv);
checkActionMediatype("tsv_export", ad_utility::MediaType::tsv);
checkActionMediatype("qlever_json_export", ad_utility::MediaType::qleverJson);
checkActionMediatype("sparql_json_export", ad_utility::MediaType::sparqlJson);
checkActionMediatype("turtle_export", ad_utility::MediaType::turtle);
checkActionMediatype("binary_export", ad_utility::MediaType::octetStream);
EXPECT_THAT(Server::determineMediaType(
{}, MakeRequest("application/sparql-results+json")),
testing::Eq(ad_utility::MediaType::sparqlJson));
// No supported media type.
EXPECT_THROW(Server::determineMediaType({}, MakeRequest("text/css")),
std::runtime_error);
// No `Accept` header means that any content type is allowed.
EXPECT_THAT(Server::determineMediaType({}, MakeRequest(std::nullopt)),
testing::Eq(ad_utility::MediaType::sparqlJson));
// No `Accept` header and an empty `Accept` header are not distinguished.
EXPECT_THAT(Server::determineMediaType({}, MakeRequest("")),
testing::Eq(ad_utility::MediaType::sparqlJson));
}

0 comments on commit dadc2df

Please sign in to comment.