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.

Sign up for GitHub

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

Jump to bottom

Add CORS header #519

Merged
merged 3 commits into from
Dec 6, 2021
Merged

Add CORS header #519

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5278261
Add CORS header (for starters, allow all origins)
Dec 6, 2021
87854cf
Set CORS header in Server.cpp, not in HttpUtils.h
Dec 6, 2021
6be263e
Implemented Johannes' suggestion
Dec 6, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions src/util/HttpServer/HttpUtils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
}

response.keep_alive(request.keep_alive());
response.body() = std::move(body);
// Set Content-Length and Transfer-Encoding.
Expand Down