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

Simplify HttpHandler::HandleRequest() signature by grouping arguments into structs #10142

Open
julianbrost opened this issue Aug 29, 2024 · 0 comments
Labels
area/api REST API core/quality Improve code, libraries, algorithms, inline docs

Comments

@julianbrost
Copy link
Contributor

HttpHandler::HandleRequest() currently takes quite a number of arguments, some of which also have quite complex types:

virtual bool HandleRequest(
AsioTlsStream& stream,
const ApiUser::Ptr& user,
boost::beast::http::request<boost::beast::http::string_body>& request,
const Url::Ptr& url,
boost::beast::http::response<boost::beast::http::string_body>& response,
const Dictionary::Ptr& params,
boost::asio::yield_context& yc,
HttpServerConnection& server
) = 0;

Given that this is a quite frequently overridden method, there are lots of copies of this parameter strings. Some of these are obviously related, like user, request (which actually is the request body), url, and params all describe attributes of the HTTP request being handled. So why not group them into a struct or class to simplify the signature?

Most handler implementations currently have unused parameters. For example, some only exist for the event stream handler, still, all implementations have to explicitly list them in their signature.

Related: when doing bigger changes to the interface there, one other improvement that comes to mind is how HttpServerConnection::StartStreaming() works: currently, to take control over the whole connection, this has to be called, but the underlying ASIO stream is still passed to every handler but it must not be used without calling StartStreaming(), otherwise, there's a good chance the connection ends up in a broken state. This could be improved by only exposing the underlying stream as a return value of the StartStreaming() method, similar to how it works in Go's net/http package.

The exact implementation is still up to discussion. For example: Should there be separate parameters for the request and response or a common parameter? Could that common parameter just be the existing HttpServerConnection object as it has to keep track of all that information already anyways?

The following HttpHandler::ProcessRequest() has a similar signature, however it's not overridden, so there aren't many copies of it, but maybe it could benefit from similar changes:

static void ProcessRequest(
AsioTlsStream& stream,
const ApiUser::Ptr& user,
boost::beast::http::request<boost::beast::http::string_body>& request,
boost::beast::http::response<boost::beast::http::string_body>& response,
boost::asio::yield_context& yc,
HttpServerConnection& server
);

@julianbrost julianbrost added area/api REST API core/quality Improve code, libraries, algorithms, inline docs labels Aug 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/api REST API core/quality Improve code, libraries, algorithms, inline docs
Projects
None yet
Development

No branches or pull requests

1 participant