Graph Store HTTP Protocol (GET, POST) back end #1668
Conversation
# Conflicts: # test/ServerTest.cpp
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1668 +/- ##
==========================================
+ Coverage 89.86% 89.89% +0.03%
==========================================
Files 389 393 +4
Lines 37189 37320 +131
Branches 4196 4213 +17
==========================================
+ Hits 33419 33550 +131
- Misses 2473 2474 +1
+ Partials 1297 1296 -1 ☔ View full report in Codecov by Sentry. |
# Conflicts: # src/engine/CMakeLists.txt
Conformance check passed ✅No test result changes. |
|
| if (rawRequest.find(field::content_type) != rawRequest.end()) { | ||
| contentTypeString = rawRequest.at(field::content_type); | ||
| } | ||
| if (contentTypeString.empty()) { | ||
| // ContentType not set or empty; we don't try to guess -> 400 Bad Request | ||
| } | ||
| const auto contentType = | ||
| ad_utility::getMediaTypeFromAcceptHeader(contentTypeString); |
There was a problem hiding this comment.
Extract this as a function.
And there is an empty if (probably misssing TODO, do you want this to throw?
| std::vector<TurtleTriple> triples; | ||
| switch (contentType.value()) { | ||
| case ad_utility::MediaType::turtle: | ||
| case ad_utility::MediaType::ntriples: { | ||
| auto parser = Re2Parser(); | ||
| parser.setInputStream(rawRequest.body()); | ||
| triples = parser.parseAndReturnAllTriples(); | ||
| break; | ||
| } | ||
| default: { | ||
| // Unsupported media type -> 415 Unsupported Media Type | ||
| throw std::runtime_error(absl::StrCat( | ||
| "Mediatype \"", ad_utility::toString(contentType.value()), | ||
| "\" is not supported for SPARQL Graph Store HTTP " | ||
| "Protocol in QLever.")); | ||
| } | ||
| } |
There was a problem hiding this comment.
Can also be a small function (which can be defined in the .cpp file.
Also report in the error message, which types ARE supported, s.t. the user can fix this.
And for the handling of 415 you probably need a dedicated exception type.
| if (std::holds_alternative<GraphRef>(graph)) { | ||
| g = Iri(std::get<GraphRef>(graph).toStringRepresentation()); | ||
| } |
There was a problem hiding this comment.
This IRI can be computed in advance. And it is somewhat wasteful to repeat the Graph IRI over and over (but maybe that is not super relevant, as the subject, predicate, and object are also strings here.
| if (std::holds_alternative<GraphRef>(graph)) { | ||
| g = Iri(std::get<GraphRef>(graph).toStringRepresentation()); | ||
| } | ||
| return SparqlTripleSimpleWithGraph(triple.subject_, triple.predicate_, |
There was a problem hiding this comment.
You should be able to std::move here (I hope that ad_utility:transform supports this, otherwise do something else or let's think about something.
| triple.object_, g); | ||
| }; | ||
| updateClause::GraphUpdate up{ | ||
| ad_utility::transform(triples, transformTurtleTriple), {}}; | ||
| res._clause = parsedQuery::UpdateClause{up}; | ||
| return res; |
There was a problem hiding this comment.
The actual triple parsing can also be a separate function. Everything that doesn't directly depend on the templated request can and should be extracted to the .cpp file.
| [[nodiscard]] const Id& getId() const { return std::get<Id>(_variant); } | ||
| [[nodiscard]] Id& getId() { return std::get<Id>(_variant); } |
There was a problem hiding this comment.
We don't use nodiscard by default.
Only when it is really semantically subtle (e.g. RAII types like Lock-Guard).
That clang-tidy rule of "every const method should be nodiscard" doesn't really add much value.
| inline auto MakeRequest( | ||
| const http::verb method = http::verb::get, const std::string& target = "/", | ||
| const ad_utility::HashMap<http::field, std::string>& headers = {}, | ||
| const std::optional<std::string>& body = std::nullopt) { | ||
| // version 11 stands for HTTP/1.1 | ||
| auto req = http::request<http::string_body>{method, target, 11}; | ||
| for (const auto& [key, value] : headers) { | ||
| req.set(key, value); | ||
| } | ||
| if (body.has_value()) { | ||
| req.body() = body.value(); | ||
| req.prepare_payload(); | ||
| } | ||
| return req; | ||
| } | ||
|
|
||
| inline auto MakeGetRequest(const std::string& target) { | ||
| return MakeRequest(http::verb::get, target); | ||
| } | ||
|
|
||
| inline auto MakePostRequest(const std::string& target, | ||
| const std::string& contentType, | ||
| const std::string& body) { | ||
| return MakeRequest(http::verb::post, target, | ||
| {{http::field::content_type, contentType}}, body); |
There was a problem hiding this comment.
Thos are functions, so their names should be lowercase.
| EXPECT_THAT(GraphStoreProtocol::extractTargetGraph({{"graph", {"foo"}}}), | ||
| t::iri("<foo>")); |
There was a problem hiding this comment.
The case {"graph", {"foo", "bar"}} can also be tested (multiple graphs throw in this domain.
| AD_EXPECT_THROW_WITH_MESSAGE( | ||
| GraphStoreProtocol::transformGraphStoreProtocol( | ||
| ad_utility::testing::MakeRequest(boost::beast::http::verb::put, | ||
| "/?default")), | ||
| testing::HasSubstr("PUT in the SPARQL Graph Store HTTP Protocol")); | ||
| AD_EXPECT_THROW_WITH_MESSAGE( | ||
| GraphStoreProtocol::transformGraphStoreProtocol( | ||
| ad_utility::testing::MakeRequest(boost::beast::http::verb::delete_, | ||
| "/?default")), | ||
| testing::HasSubstr("DELETE in the SPARQL Graph Store HTTP Protocol")); | ||
| AD_EXPECT_THROW_WITH_MESSAGE( | ||
| GraphStoreProtocol::transformGraphStoreProtocol( | ||
| ad_utility::testing::MakeRequest(boost::beast::http::verb::head, | ||
| "/?default")), | ||
| testing::HasSubstr("HEAD in the SPARQL Graph Store HTTP Protocol")); | ||
| AD_EXPECT_THROW_WITH_MESSAGE( | ||
| GraphStoreProtocol::transformGraphStoreProtocol( | ||
| ad_utility::testing::MakeRequest(boost::beast::http::verb::patch, | ||
| "/?default")), | ||
| testing::HasSubstr("PATCH in the SPARQL Graph Store HTTP Protocol")); | ||
| AD_EXPECT_THROW_WITH_MESSAGE( | ||
| GraphStoreProtocol::transformGraphStoreProtocol( | ||
| ad_utility::testing::MakeRequest(boost::beast::http::verb::connect, | ||
| "/?default")), |
There was a problem hiding this comment.
A lot of code duplication here, can be mitigated.
| TC(Var{"?s"}), "?p", TC(Var{"?o"}))})))); | ||
| EXPECT_THAT( | ||
| GraphStoreProtocol::transformGraphStoreProtocol( | ||
| ad_utility::testing::MakePostRequest( |
There was a problem hiding this comment.
A using namespace ad_utiility::testing inside this function would make the code a little shorter (not too important though).
Implement a function
transformGraphStoreProtocolthat does the back end of transforming a SPARQL Graph Store HTTP Protocol request to it's equivalent SPARQL Query or Update. The integration will be a separate step.TODO:
Additional smaller changes that were required or sensible and which could be extracted into separate PRs if needed:
checkParameterfromServertoad_utility::url_parserHttpRequestHelperIdfromTripleComponent