From 6fec8d6914bff958d34a5fdad1cee5ff5e9e9355 Mon Sep 17 00:00:00 2001 From: Hodlinator <172445034+hodlinator@users.noreply.github.com> Date: Mon, 24 Aug 2026 11:25:31 +0200 Subject: [PATCH] refactor: Make HTTPRequest fields private Makes sense since they are only set by methods in the class itself, and already had accessors for most fields. --- src/httpserver.cpp | 4 +- src/httpserver.h | 31 ++++++++------- src/test/httpserver_tests.cpp | 73 +++++++++++++++++------------------ 3 files changed, 55 insertions(+), 53 deletions(-) diff --git a/src/httpserver.cpp b/src/httpserver.cpp index 22dde8a696a..c94863cae33 100644 --- a/src/httpserver.cpp +++ b/src/httpserver.cpp @@ -1064,8 +1064,8 @@ void HTTPServer::MaybeDispatchRequestsFromClient(const std::shared_ptrm_req->m_method), - client->m_req->m_target, + RequestMethodString(client->m_req->GetRequestMethod()), + client->m_req->GetURI(), client->m_origin, client->m_id); diff --git a/src/httpserver.h b/src/httpserver.h index 42d5b155dc0..ce2bd4eaac2 100644 --- a/src/httpserver.h +++ b/src/httpserver.h @@ -150,18 +150,6 @@ class HTTPRemoteClient; class HTTPRequest { public: - HTTPRequestMethod m_method; - std::string m_target; - HTTPVersion m_version; - HTTPHeaders m_headers; - std::string m_body; - - //! Pointer to the client that made the request so we know who to respond to. - std::weak_ptr m_client; - - //! Response headers may be set in advance before response body is known - HTTPHeaders m_response_headers; - explicit HTTPRequest(const std::shared_ptr& client) : m_client{client} {} //! Construct with a null client for unit tests explicit HTTPRequest() : m_client{} {} @@ -186,6 +174,9 @@ public: WriteReply(status, std::as_bytes(std::span{reply_body_view})); } + const HTTPVersion& GetVersion() const { return m_version; } + std::shared_ptr GetClient() const { return m_client.lock(); } + // These methods reimplement the API from http_libevent::HTTPRequest // for downstream JSONRPC and REST modules. std::string GetURI() const { return m_target; } @@ -195,6 +186,8 @@ public: std::optional GetHeader(std::string_view hdr) const; std::string ReadBody() const { return m_body; } void WriteHeader(std::string&& hdr, std::string&& value); + std::optional GetChunkSize() const { return m_chunk_size; } + uint64_t GetChunkProgress() const { return m_chunk_read; } enum class State { Init, @@ -206,6 +199,19 @@ public: State GetState() const { return m_state; } void SetState(State state) { m_state = state; } +private: + HTTPRequestMethod m_method; + std::string m_target; + HTTPVersion m_version; + HTTPHeaders m_headers; + std::string m_body; + + //! Pointer to the client that made the request so we know who to respond to. + std::weak_ptr m_client; + + //! Response headers may be set in advance before response body is known + HTTPHeaders m_response_headers; + // If a large request is sent with "Transfer-encoding: chunked" we may // read the chunk size in a separate I/O loop iteration than the chunk // of data itself. Store the chunk size value here until the chunk is read. @@ -214,7 +220,6 @@ public: // Track the progress of the chunk here. uint64_t m_chunk_read{0}; -private: State m_state = State::Init; }; diff --git a/src/test/httpserver_tests.cpp b/src/test/httpserver_tests.cpp index 94f9920dd87..5459fb49e29 100644 --- a/src/test/httpserver_tests.cpp +++ b/src/test/httpserver_tests.cpp @@ -206,19 +206,16 @@ BOOST_AUTO_TEST_CASE(http_request_tests) BOOST_CHECK(req.LoadControlData(reader)); BOOST_CHECK(req.LoadHeaders(reader)); BOOST_CHECK(req.LoadBody(reader)); - BOOST_CHECK_EQUAL(req.m_method, HTTPRequestMethod::POST); BOOST_CHECK_EQUAL(req.GetRequestMethod(), HTTPRequestMethod::POST); - BOOST_CHECK_EQUAL(req.m_target, "/"); BOOST_CHECK_EQUAL(req.GetURI(), "/"); - BOOST_CHECK_EQUAL(req.m_version.major, 1); - BOOST_CHECK_EQUAL(req.m_version.minor, 1); + BOOST_CHECK_EQUAL(req.GetVersion().major, 1); + BOOST_CHECK_EQUAL(req.GetVersion().minor, 1); BOOST_CHECK_EQUAL(req.GetHeader("Host"), "127.0.0.1"); BOOST_CHECK_EQUAL(req.GetHeader("Connection"), "close"); BOOST_CHECK_EQUAL(req.GetHeader("Content-Type"), "application/json"); BOOST_CHECK_EQUAL(req.GetHeader("Authorization"), "Basic X19jb29raWVfXzo5OGQ5ODQ3MWNmNjg0NzAzYTkzN2EzNzk0ZDFlODQ1NjZmYTRkZjJiMzFkYjhhODI4ZGY4MjVjOTg5ZGI4OTVl"); BOOST_CHECK_EQUAL(req.GetHeader("Content-Length"), "46"); - BOOST_CHECK_EQUAL(req.m_body.size(), 46); - BOOST_CHECK_EQUAL(req.m_body, R"({"method":"getblockcount","params":[],"id":1})""\n"); + BOOST_CHECK_EQUAL(req.ReadBody(), R"({"method":"getblockcount","params":[],"id":1})""\n"); } { // Malformed: no spaces between data @@ -313,13 +310,13 @@ BOOST_AUTO_TEST_CASE(http_request_tests) BOOST_CHECK(req.LoadControlData(reader)); BOOST_CHECK(req.LoadHeaders(reader)); BOOST_CHECK(req.LoadBody(reader)); - BOOST_CHECK_EQUAL(req.m_method, HTTPRequestMethod::GET); - BOOST_CHECK_EQUAL(req.m_target, "/"); - BOOST_CHECK_EQUAL(req.m_version.major, 1); - BOOST_CHECK_EQUAL(req.m_version.minor, 0); + BOOST_CHECK_EQUAL(req.GetRequestMethod(), HTTPRequestMethod::GET); + BOOST_CHECK_EQUAL(req.GetURI(), "/"); + BOOST_CHECK_EQUAL(req.GetVersion().major, 1); + BOOST_CHECK_EQUAL(req.GetVersion().minor, 0); BOOST_CHECK_EQUAL(req.GetHeader("Host"), "127.0.0.1"); // no body is OK - BOOST_CHECK_EQUAL(req.m_body.size(), 0); + BOOST_CHECK_EQUAL(req.ReadBody(), ""); } { // Malformed: missing colon @@ -345,7 +342,7 @@ BOOST_AUTO_TEST_CASE(http_request_tests) BOOST_CHECK(req.LoadHeaders(reader)); BOOST_CHECK(req.LoadBody(reader)); // Don't try to read request body if Content-Length is missing - BOOST_CHECK_EQUAL(req.m_body.size(), 0); + BOOST_CHECK_EQUAL(req.ReadBody(), ""); } { // Malformed: Content-Length is not a number @@ -409,7 +406,7 @@ BOOST_AUTO_TEST_CASE(http_request_tests) BOOST_CHECK(req.LoadControlData(reader)); BOOST_CHECK(req.LoadHeaders(reader)); BOOST_CHECK(req.LoadBody(reader)); - BOOST_CHECK_EQUAL(req.m_body, R"({"method":"getblockcount"})"); + BOOST_CHECK_EQUAL(req.ReadBody(), R"({"method":"getblockcount"})"); } { // Prevent "chunked" transfer from exceeding size limit @@ -445,7 +442,7 @@ BOOST_AUTO_TEST_CASE(http_request_tests) BOOST_CHECK(req.LoadControlData(reader)); BOOST_CHECK(req.LoadHeaders(reader)); BOOST_CHECK(req.LoadBody(reader)); - BOOST_CHECK_EQUAL(req.m_body, R"({"method":"getblockcount"})"); + BOOST_CHECK_EQUAL(req.ReadBody(), R"({"method":"getblockcount"})"); // Chunk Trailer was parsed, but ignored BOOST_CHECK_EQUAL(reader.Remaining(), 0); BOOST_CHECK(!req.GetHeader("Expires")); @@ -541,7 +538,7 @@ BOOST_AUTO_TEST_CASE(http_request_state_tests) "GET /endpoint HTTP/1.0\n\n"); client->ReadRequest(*client->m_req); BOOST_CHECK_EQUAL(client->m_req->GetState(), HTTPRequest::State::Complete); - BOOST_CHECK_EQUAL(client->m_req->m_body, "I miss you"); + BOOST_CHECK_EQUAL(client->m_req->ReadBody(), "I miss you"); // Next request sitting in buffer BOOST_CHECK_EQUAL(client->m_recv_buffer.size(), 24); // Complete first request hasn't been moved yet, expect no-op @@ -555,8 +552,8 @@ BOOST_AUTO_TEST_CASE(http_request_state_tests) // Read second request client->ReadRequest(*client->m_req); BOOST_CHECK_EQUAL(client->m_req->GetState(), HTTPRequest::State::Complete); - BOOST_CHECK_EQUAL(client->m_req->m_target, "/endpoint"); - BOOST_CHECK_EQUAL(client->m_req->m_body.size(), 0); + BOOST_CHECK_EQUAL(client->m_req->GetURI(), "/endpoint"); + BOOST_CHECK_EQUAL(client->m_req->ReadBody().size(), 0); // Buffer is cleared BOOST_CHECK_EQUAL(client->m_recv_buffer.size(), 0); } @@ -579,7 +576,7 @@ BOOST_AUTO_TEST_CASE(http_request_state_tests) client->receive(std::string(10000, 'x')); BOOST_CHECK_EQUAL(client->m_recv_buffer.size(), 10000); client->ReadRequest(*client->m_req); - BOOST_CHECK_EQUAL(client->m_req->m_body.size(), 10000 * i); + BOOST_CHECK_EQUAL(client->m_req->ReadBody().size(), 10000 * i); BOOST_CHECK_EQUAL(client->m_recv_buffer.size(), 0); } BOOST_CHECK_EQUAL(client->m_req->GetState(), HTTPRequest::State::Complete); @@ -596,7 +593,7 @@ BOOST_AUTO_TEST_CASE(http_request_state_tests) "GET /next HTTP/1.0\n\n"); client->ReadRequest(*client->m_req); BOOST_CHECK_EQUAL(client->m_req->GetState(), HTTPRequest::State::Complete); - BOOST_CHECK_EQUAL(client->m_req->m_body, "body"); + BOOST_CHECK_EQUAL(client->m_req->ReadBody(), "body"); // Only the second request is left over BOOST_CHECK_EQUAL(client->m_recv_buffer.size(), 20); } @@ -605,9 +602,9 @@ BOOST_AUTO_TEST_CASE(http_request_state_tests) std::shared_ptr client{std::make_shared()}; client->m_req = std::make_unique(client); - BOOST_CHECK(!client->m_req->m_chunk_size); - BOOST_CHECK_EQUAL(client->m_req->m_chunk_read, 0); - BOOST_CHECK_EQUAL(client->m_req->m_body.size(), 0); + BOOST_CHECK(!client->m_req->GetChunkSize()); + BOOST_CHECK_EQUAL(client->m_req->GetChunkProgress(), 0); + BOOST_CHECK_EQUAL(client->m_req->ReadBody().size(), 0); BOOST_CHECK_EQUAL(client->m_req->GetState(), HTTPRequest::State::Init); // First chunk is incomplete @@ -617,40 +614,40 @@ BOOST_AUTO_TEST_CASE(http_request_state_tests) "10\n" R"({"method)"); client->ReadRequest(*client->m_req); - BOOST_CHECK(client->m_req->m_chunk_size); - BOOST_CHECK_EQUAL(*client->m_req->m_chunk_size, 16); - BOOST_CHECK_EQUAL(client->m_req->m_chunk_read, 8); - BOOST_CHECK_EQUAL(client->m_req->m_body.size(), 8); + BOOST_CHECK(client->m_req->GetChunkSize()); + BOOST_CHECK_EQUAL(*client->m_req->GetChunkSize(), 16); + BOOST_CHECK_EQUAL(client->m_req->GetChunkProgress(), 8); + BOOST_CHECK_EQUAL(client->m_req->ReadBody().size(), 8); BOOST_CHECK_EQUAL(client->m_req->GetState(), HTTPRequest::State::NeedsBody); // More data arrives, chunk is completed. client->receive(R"(":"getbl)""\n"); client->ReadRequest(*client->m_req); // State is reset - BOOST_CHECK(!client->m_req->m_chunk_size); - BOOST_CHECK_EQUAL(client->m_req->m_chunk_read, 0); + BOOST_CHECK(!client->m_req->GetChunkSize()); + BOOST_CHECK_EQUAL(client->m_req->GetChunkProgress(), 0); // New data is added to body but body is still incomplete - BOOST_CHECK_EQUAL(client->m_req->m_body.size(), 16); + BOOST_CHECK_EQUAL(client->m_req->ReadBody().size(), 16); BOOST_CHECK_EQUAL(client->m_req->GetState(), HTTPRequest::State::NeedsBody); // Next chunk arrives without terminal CRLF client->receive("a\n" R"(ockcount"})"); client->ReadRequest(*client->m_req); - BOOST_CHECK(client->m_req->m_chunk_size); - BOOST_CHECK_EQUAL(*client->m_req->m_chunk_size, 10); - BOOST_CHECK_EQUAL(client->m_req->m_chunk_read, 10); - BOOST_CHECK_EQUAL(client->m_req->m_body.size(), 26); + BOOST_CHECK(client->m_req->GetChunkSize()); + BOOST_CHECK_EQUAL(*client->m_req->GetChunkSize(), 10); + BOOST_CHECK_EQUAL(client->m_req->GetChunkProgress(), 10); + BOOST_CHECK_EQUAL(client->m_req->ReadBody().size(), 26); BOOST_CHECK_EQUAL(client->m_req->GetState(), HTTPRequest::State::NeedsBody); // Chunk terminal CRLF arrives with final (size 0) chunk client->receive("\n0\n\n"); client->ReadRequest(*client->m_req); // Body size hasn't changed - BOOST_CHECK_EQUAL(client->m_req->m_body.size(), 26); + BOOST_CHECK_EQUAL(client->m_req->ReadBody().size(), 26); // We're done BOOST_CHECK_EQUAL(client->m_req->GetState(), HTTPRequest::State::Complete); - BOOST_CHECK_EQUAL(client->m_req->m_body, R"({"method":"getblockcount"})"); + BOOST_CHECK_EQUAL(client->m_req->ReadBody(), R"({"method":"getblockcount"})"); } { // Invalid headers: error state stops reading @@ -772,7 +769,7 @@ BOOST_AUTO_TEST_CASE(http_request_state_tests) client->receive("\n"); client->ReadRequest(*client->m_req); BOOST_CHECK_EQUAL(client->m_req->GetState(), HTTPRequest::State::Complete); - BOOST_CHECK_EQUAL(client->m_req->m_body, "x"); + BOOST_CHECK_EQUAL(client->m_req->ReadBody(), "x"); } { // Ensure chunk trailer counts towards the headers size limit @@ -868,11 +865,11 @@ BOOST_AUTO_TEST_CASE(http_server_socket_tests) // Connected client should have one request already from the static content. if (requests.size() == 1) { // Check the received request - BOOST_CHECK_EQUAL(requests.front()->m_body, R"({"method":"getblockcount","params":[],"id":1})""\n"); + BOOST_CHECK_EQUAL(requests.front()->ReadBody(), R"({"method":"getblockcount","params":[],"id":1})""\n"); BOOST_CHECK_EQUAL(requests.front()->GetPeer().ToStringAddrPort(), "5.5.5.5:6789"); // Inspect the connection pointed to from the request - client = requests.front()->m_client.lock(); + client = requests.front()->GetClient(); BOOST_REQUIRE(client); BOOST_CHECK_EQUAL(client->m_origin, "5.5.5.5:6789");