Skip to content

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

Open
@julianbrost

Description

@julianbrost

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    area/apiREST APIcore/qualityImprove code, libraries, algorithms, inline docs

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions