feat(cpp): gaia::HttpClient — a general HTTP client abstraction - #2809
feat(cpp): gaia::HttpClient — a general HTTP client abstraction#2809kovtcharov wants to merge 1 commit into
Conversation
C++ tool authors had no way to call an HTTP service: the only client in the framework was LemonadeClient's private httpGet/httpPost/httpPostStreaming, and cpp-httplib is a PRIVATE CMake dependency consumers cannot see. gaia::HttpClient is a pimpl over cpp-httplib, so the transport stays private — including gaia/http_client.h pulls in no 10k-line header. It exposes get/post/postStreaming with header maps, per-request timeouts and TLS (reusing the existing OpenSSL auto-detection). Every failure raises HttpError naming the URL and the failure mode, carrying status() and body(); there is no empty-response fallback. LemonadeClient's private HTTP methods are now thin forwards. Timeouts, URL normalization (/v1 and /api/v1 preservation) and the SSE [DONE] → normal-completion path are preserved; the streaming path no longer feeds a non-2xx error body to SseParser, and an https:// URL on an HTTP-only build now raises an actionable error instead of "SSL not supported." Header names merge case-insensitively, CRLF in header fields or request paths is rejected, ports are validated, and moved-from clients stay usable. Unblocks the embeddings API (#2791) and MCP HTTP transport (#2802).
|
Verdict: Approve This PR extracts a general-purpose The one thing worth knowing: this is a pure transport refactor, so the risk is behavioral drift in the Lemonade path. The tests cover that well — a dedicated Note: the review environment blocked shell access, so I could not pull the PR description; this verdict rests on static review of the diff and the included tests. Real-world evidenceN/A — C++ library change under 🔍 Technical detailsStrengths
🟢 Minor (optional, non-blocking)
|
The failing
|
…xe (amd#2818) Every `cpp/**` PR has been failing the `C++ Integration Tests (STX)` check before it compiles anything — amd#2807, amd#2809 and amd#2816 are all red for a reason unrelated to their diffs. The self-hosted runner cached CMake under `$env:TEMP` and re-downloaded it only when `bin\cmake.exe` was missing; Windows Temp cleanup deleted `share\cmake-3.31\Modules` and left `bin\`, so the job kept trusting a CMake that cannot resolve `CMAKE_ROOT` and every run died the same way until someone cleared Temp by hand. Now each candidate toolchain is probed for the thing the build actually depends on, a failed probe falls through to a clean re-download, and tools live in the runner tool cache instead of a directory the OS sweeps. One measurement drove the design and is worth flagging for review: **exit codes cannot detect this failure.** A CMake missing its Modules tree prints `Could not find CMAKE_ROOT` to stderr and still exits 0 — for `--version` and for `--help-module-list` (measured on 4.4.2). So the issue's suggested `cmake --version` exit-0 check would not have caught it on its own; validity requires the Modules tree on disk *and* a probe that does not report a broken root. The stale `%TEMP%\cmake` tree on the runner is now inert — nothing reads it — so no manual cleanup is needed to make this work; deleting it just reclaims disk. Closes amd#2817 ## Test plan - [ ] `pwsh -File .github/scripts/tests/CppBuildTools.Tests.ps1` passes (21/21). It asserts the exact regression: `bin/cmake.exe` present + `share/` absent reports **invalid**, both present reports **valid**, and includes negative controls showing the old `Test-Path cmake.exe` check and an exit-code-only check would both have accepted the broken install. - [ ] New `C++ toolchain script tests` job is green (parse-checks every `.github/scripts/*.ps1`, then runs the unit tests). - [ ] `C++ Integration Tests (STX)` on this PR gets past `Ensure C++ build tools are available` and reaches compilation. The step log should name which CMake it accepted and, if it rejected one, why. - [ ] Reproduce the root cause on any machine: copy a `cmake` binary alone into an empty directory and run `--version` — it prints the `CMAKE_ROOT` error and exits 0. - [ ] After merge, re-run CI on amd#2807, amd#2809 and amd#2816 with no changes to their diffs and confirm the STX check goes green.
1 participant
Before: a C++ tool author who needed to call any HTTP service had to vendor their own client — the framework's only client was
LemonadeClient's private, Lemonade-shapedhttpGet/httpPost/httpPostStreaming, and cpp-httplib is a PRIVATE CMake dependency consumers cannot see. After:gaia::HttpClientgives themget/post/postStreamingwith header maps, per-request timeouts and TLS, and every failure raisesHttpErrornaming the URL and the failure mode (withstatus()/body()for programmatic handling) instead of a silent empty response. The transport stays private — includinggaia/http_client.hpulls in no 10k-line header, enforced by a compile-time#errorin the test file.LemonadeClientnow sits on top of it, with its timeouts, URL normalization and SSE[DONE]handling preserved.Two user-visible improvements fall out of the refactor: a non-2xx streaming body is no longer fed to
SseParser(it is reported throughHttpErrorinstead), and anhttps://URL on an HTTP-only build now says how to fix it rather than "SSL not supported."Closes #2790.
Test plan
cmake -S cpp -B cpp/build -DGAIA_BUILD_TESTS=ON && cmake --build cpp/build && ctest --test-dir cpp/build --output-on-failure→ 491/491 pass (463 before + 28 new)test_lemonade_clientcases pass unmodifiedcpp/tests/test_http_client.cpp: GET/POST/streaming, header passthrough and case-insensitive merge, response headers, read timeout, connection failure, non-2xx status, malformed port, CRLF injection, path joining, moved-from clientLemonadeOverHttpClientTestcases drive the refactored transport end-to-end against the mock server — including a real SSE stream, which pins the[DONE]→ normal-completion path that the offline unit tests never coveredcpp/tests/test_http_client.cpp#errors ifgaia/http_client.hleaks httplib; separately verified a consumer TU compiles with no httplib include path at all-DCMAKE_DISABLE_FIND_PACKAGE_OpenSSL=ON) compiles the no-TLS branches, warning-clean under-Wall -Wextra -Wpedantic