Graph store integration Preparations #1743
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1743 +/- ##
==========================================
+ Coverage 89.95% 90.05% +0.10%
==========================================
Files 395 396 +1
Lines 37651 37910 +259
Branches 4234 4261 +27
==========================================
+ Hits 33869 34141 +272
+ Misses 2487 2476 -11
+ Partials 1295 1293 -2 ☔ View full report in Codecov by Sentry. |
src/engine/Server.cpp
Outdated
|
|
||
| if (!plannedQuery.parsedQuery_.hasUpdateClause()) { | ||
| if (!plannedUpdate.parsedQuery_.hasUpdateClause()) { |
There was a problem hiding this comment.
Can this check with the well-defined error message be performed earlier (e.g. right after the parsing, and in a single function for the query and update? This could also then be moved into the SPARQLProtocol file.
Then we can here get away with a simple AD_CORRECTNESS_CHECK
There was a problem hiding this comment.
Moved the check into the visit function. This could get even cleaner if we'd separate the parsing functions for and have separate types for parsed Queries/Updates.
There was a problem hiding this comment.
I'll wait until the very end with moving the function, to keep the review more manageable.
| #include "util/http/UrlParser.h" | ||
| #include "util/http/beast.h" | ||
|
|
||
| class SPARQLProtocol { |
There was a problem hiding this comment.
I'll have a look at this file another time, but I assume, that there is not much change other than moving the functionality out of the Server.cpp file into this file, is that right?
There was a problem hiding this comment.
As of now there's no change here at all, yes. Some small changes will happen in the follow-up PR.
| // TODO: the storing of the PlannedQuery outside the try-catch block in case | ||
| // of an error has been broken for some time |
There was a problem hiding this comment.
Can you elaborate which case exactly is broken here? Then we can decide if that is an easy or a hard problem to fix.
There was a problem hiding this comment.
When all the query execution was in this function, it was possible to access the planned query when doing the exception handling. This was used to add the planned query to the error response for debugging. The planned query is not available here and not all places were exceptions could be thrown have access to the planned query.
Conformance check passed ✅No test result changes. |
|
parseHttpRequestandextractAccessTokeninto separate ClassSPARQLProtocol.processQueryOrUpdate(and methods below it) to take aParsedQuery. Consolidate query preparation intoparseQuery,planQueryandsetupPlannedQuery.Both these changes will make integrating the Graph Store Protocol much easier