Skip to content
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

Merged
merged 3 commits into from Dec 6, 2021
Merged

Add CORS header #519

merged 3 commits into from Dec 6, 2021

Conversation

hannahbast
Copy link
Member

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 .

Copy link
Member

@joka921 joka921 left a 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).

@@ -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, "*");
Copy link
Member

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).

Copy link
Collaborator

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

Copy link
Member Author

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.

Copy link
Member

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.
Copy link
Member

@joka921 joka921 left a 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.

@@ -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);
Copy link
Member

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).

@@ -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, "*");
Copy link
Member

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.

@joka921 joka921 merged commit 1d5503c into ad-freiburg:master Dec 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants