-
Notifications
You must be signed in to change notification settings - Fork 50
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Parse URLs using boost::Url
#1417
Parse URLs using boost::Url
#1417
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1417 +/- ##
==========================================
- Coverage 94.13% 92.51% -1.62%
==========================================
Files 347 353 +6
Lines 25636 26487 +851
Branches 3450 3551 +101
==========================================
+ Hits 24132 24504 +372
- Misses 1462 1941 +479
Partials 42 42 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An opportunity to also fix another bug.
The real solution is to completely handle the URL parsing using Boos::Url which wasn't yet released at the time we implemented this rather hacky URL parser.
Quality Gate passedIssues Measures |
boost::Url
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this should fix some longstanding bugs and makes the code much easier to read.
One major thing:
We should have unit tests for this code
(for the UrlParser, as well as for the (is it static or can it be static) method in Server.cpp
that does the dispatch between GET and the various types of POST.
Some basic tests are now present. They will be extended a little bit more.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is starting to look really nice.
I still have some suggestions and some questions, but we are definitely getting there.
test/ServerTest.cpp
Outdated
// TODO `default-graph-uri` and `named-graph-uri` can appear multiple times. | ||
// This currently throws an exception and is not considered. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, So this means that in HTTP there can be duplicate parameters in general, and the semantics depend on whatever the service does.
So probably, the value type of the parameters should not be string
but HashSet<string>
or something like this.
Then the consuming code (in this case the Server.cpp
has to check, whiich of these can and cannot appear multiple times. At least add a comment in the respective code in the Parser, s.t. we can revisit it at a later time (I currently am working on the named graph support).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The specs (RFC and similar) actually don't specify semantics for multiple values for a query parameter.
Probably a topic for the followup PR, but anyways: What would be the reason against simply using a multiset
instead of a set
here? Is it that abseil does not provide a multiset and we'd have to resort to std::multiset
?
This comment is just a temporary note for me. For now I'd add some TODOs in appropriate places and warnings to the result if these parameters are used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only a small single comment left, address it and we are ready to merge.
Quality Gate passedIssues Measures |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much for improving the URL parser.
This PR cleans up the parsing of HTTP Requests and switches to
boost::Url
for URL parsing.This should fix issues with the following types of HTTP requests:
The only (known) limitation that remains for now is that duplicate HTTP parameters throw an error, because we are not yet sure about their semantics.