-
Notifications
You must be signed in to change notification settings - Fork 37
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
Add CORS header #519
Add CORS header #519
Conversation
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.
I would suggest putting this change somewhere else (see below).
src/util/HttpServer/HttpUtils.h
Outdated
@@ -56,6 +56,7 @@ auto createHttpResponseFromString(std::string body, http::status status, | |||
MediaType mediaType = MediaType::html) { | |||
http::response<http::string_body> response{status, request.version()}; | |||
response.set(http::field::content_type, toString(mediaType)); | |||
response.set(http::field::access_control_allow_origin, "*"); |
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.
In general I am fine with sending this field,
But I am wondering whether this should be the default behavior in the (very generic and reusable) HttpUtils.h
module.
My suggestion would be to go to Server::process (in Server.cpp, this is the actual QLever server),
and take the send
parameter (an awaitable that takes the message and sets it),
and modify it by setting the response there.
auto sendActual = [&send] (auto message) ->boost::asio::awaitable<void> {
message.set(....) // set the field
co_await send(std::move(message)); };
(And then always use sendActual
instead of send
(or rather rename the parameter send).
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.
Also it's worth noting that #513 introduced an overload of this function that would need similar code
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 + very true + I fixed it!
As an aside: I was and am confused about the use of co_return in this code. It seems to be used (or not) in an inconsistent fashion OR I have not fully understood when it must be used and when not.
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.
Currently, our Server awaitables
all co_return void
. As in "ordinary" void functions, you do not need this statement
if your function naturally "falls off the cliff" at the closing }
.
Additionally, co_await
of an awaitable<void>
also is a void statement, so consider the analogy between these examples:
void a(); // defined somewhere else
void b() { a();} // call a, fall off the cliff, which is fine (because void is returned);
void c() { return a();} // the same as b(), return <something that is void> is valid C++
// generally `return a();` and `a(); return;` are equivalent, iff a() returns void.
Similarly, in coroutine land
awaitable<void> a() ; // Something that can be co_awaited, and that co_await returns void.
awaitable<void> b() {co_await a();} // implicit co_return void at the end.
awaitable<void> c() {co_return co_await a();} // same as b(), b.c. everything returns void.
Only exception: Every coroutine needs at least one co_await, co_return
or co_yield
statement,
so the following co_return is needed:
awaitable<void> f() {
computeSomething(); // an "ordinary" synchronous function call.
co_return; // "redundant", b.c. void at the end of function, but needed to deduce that this is supposed to be a coroutine.
}
HttpUtils is general-purpose code, that is not the place to always set a header that is optional.
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.
A small addition to my previous comment.
src/engine/Server.cpp
Outdated
@@ -113,14 +117,14 @@ boost::asio::awaitable<void> Server::process( | |||
|
|||
if (params.contains("query")) { | |||
if (params.at("query").empty()) { | |||
co_return co_await send(createBadRequestResponse( | |||
co_return co_await sendWithCors(createBadRequestResponse( | |||
"Parameter \"query\" must not have an empty value", request)); | |||
} | |||
|
|||
co_return co_await processQuery(params, requestTimer, std::move(request), | |||
send); |
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.
Also use sendWithCors
here, then you don't need anything in the processQuery below.
(Currently, the TSV and CSV responses don't have their header fields changed,
those would also be included by this change).
src/engine/Server.cpp
Outdated
@@ -244,6 +248,7 @@ boost::asio::awaitable<void> Server::processQuery( | |||
auto sendJson = [&request, &send]( | |||
const json& jsonString) -> boost::asio::awaitable<void> { | |||
auto response = createJsonResponse(jsonString, request); | |||
response.set(http::field::access_control_allow_origin, "*"); |
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.
Probably becomes unnecessary (see my comment above.
The QLever backend currently does not send an Access-Control-Allow-Origin header. This works fine when the QLever UI and the QLever backend operate under the same domain, like https://qlever.cs.uni-freiburg.de .
However, this is not always the case. In particular, when someone installs QLever for the first time, they typically run QLever and the QLever UI on different ports of the same machine, for example localhost:7000 (Backend) and localhost:8000 (UI).
Then the UI will block the results from localhost:7000 because of the same-origin policy (different ports on the same machine count as different origins).
A simple fix is to let QLever always send the header
Access-Control-Allow-Origin: *
. Then the results from the QLever backend can be used on any website. Right now, I don't see a problem with that. I have checked the Wikidata Query Service and they also do this: https://query.wikidata.org .