feat: Add my-clients library for external apis

martin commented

2026-08-09 12:12:01 +00:00

Owner

Client library for external dependencies using openapi-to-rust to generate the code

martin self-assigned this

2026-08-09 12:12:01 +00:00

martin added 1 commit

2026-08-09 12:12:01 +00:00

feat: Add my-clients library for external apis

ci/woodpecker/pr/pr-review Pipeline was successful

Details

ci/woodpecker/push/verify Pipeline was successful

Details

4f8b2333a9

martin added 1 commit

2026-08-09 17:25:31 +00:00

refactor: my-clients and move files

ci/woodpecker/push/verify Pipeline was successful

Details

ci/woodpecker/pr/pr-review Pipeline was successful

Details

36c086f01a

martin changed title from ~~WIP: feat: Add my-clients library for external apis~~ to feat: Add my-clients library for external apis

2026-08-09 17:27:53 +00:00

martin requested review from clank-bot

2026-08-09 17:27:53 +00:00

clank-bot commented

2026-08-09 17:34:05 +00:00

Member

Based on my review of the PR, here is my assessment:
{"summary": "# Review Summary\n\nThe PR adds a my-clients workspace crate that generates typed HTTP clients from OpenAPI specs using openapi-to-rust, starting with a Forgejo client. Overall this is a solid, well-structured addition: the codegen config (configs/forgejo.toml) is clean, the committed contract is pinned to Forgejo 16.0.2, the README documents the regeneration workflow, response bodies are read with a hard memory bound (__read_bounded_response_body), the RFC 9457 problem-details handling is a nice touch, and the integration test smoke-tests the client end-to-end. I also verified that the generated Authorization: Bearer <token> scheme is valid — Forgejo's docs explicitly support Bearer tokens, so with_api_key works as-is.\n\nThere are a few issues worth addressing before merge:\n\n1. Configured timeout is never applied. configs/forgejo.toml declares timeout_seconds = 30, but the generated HttpClient::new() builds reqwest::Client::new() with no timeout and no request ever calls .timeout(), so the setting is dead and requests can hang indefinitely. The HttpError::Timeout variant is never constructed anywhere.\n2. All response bodies are fully buffered in memory. Despite enabling the stream feature, every response (including binary ones like download_action_artifact/repo_get_archive) is buffered into a Vec capped at 8 MB. Downloads larger than 8 MB fail with ResponseTooLarge, and there is no true streaming.\n3. Contract regeneration depends on an unpinned third-party converter. convert-forgejo.sh fetches the spec through converter.swagger.io at regeneration time, so the committed contract can silently drift from what regeneration produces and reproducibility depends on an external service.\n4. Heavy native dependency tree in the workspace. my-clients pulls in aws-lc-sys (cmake/cc build), ring, rustls, etc. Since it's a workspace member, every workspace-wide build, clippy, and CI job now compiles all of this, noticeably increasing build times.\n\nMinor notes: the generated 61k-line files are committed (maintainable as long as the documented openapi-to-rust 0.12.1 is what people actually have installed — consider pinning it in the flake dev shell), and the dead if false || guards in generated code are cosmetic only.", "verdict": "changes", "comments": [{"file": "crates/my-clients/configs/forgejo.toml", "line": 18, "severity": "bug", "message": "timeout_seconds = 30 has no effect. The generated HttpClient::new() builds reqwest::Client::new() without a timeout (client.rs:235) and no generated request calls .timeout() before req.send(). reqwest's default is no timeout, so a non-responsive server causes the call to hang forever; the HttpError::Timeout variant is never constructed. Apply the timeout (e.g. via the reqwest ClientBuilder / a .timeout() on each request) so the configured value is honored, or remove the dead setting."}, {"file": "crates/my-clients/configs/forgejo.toml", "line": 19, "severity": "performance", "message": "max_response_body_bytes = 8388608 together with __read_bounded_response_body means every response is fully buffered in memory and the stream feature is never used. Binary/streaming endpoints (download_action_artifact, repo_get_archive, release attachments, raw file/LFS) will error out with ResponseTooLarge as soon as a payload exceeds 8 MB and cannot be streamed to disk. Consider exposing per-endpoint streaming or a larger/raising limit for the download-style operations."}, {"file": "crates/my-clients/scripts/convert-forgejo.sh", "line": 5, "severity": "style", "message": "The contract is regenerated live through the third-party converter.swagger.io service with no pinned source version, so the fetched spec (and thus the generated API surface) can change between runs and diverge from the committed contracts/forgejo.json. Since the contract is already committed with a fixed version (16.0.2), consider documenting/pinning the upstream Forgejo version the script targets and validating the output is a well-formed OpenAPI document before overwriting the file."}, {"file": "crates/my-clients/Cargo.toml", "line": 13, "severity": "performance", "message": "This crate's rustls/aws-lc dependency tree (aws-lc-sys, ring, cmake/cc, etc.) is heavy to compile. Because my-clients is a workspace member, cargo build, the strict cargo clippy --workspace, and all CI/Nix jobs for the whole workspace now build these native deps, noticeably increasing build time and CI wall-clock. Worth confirming this is acceptable or scoping the workspace build (e.g. excluding the client crate from default workspace builds)."}]}

*🤖 Review by opencode (opencode/deepseek-v4-flash-free) — verdict: comment

Based on my review of the PR, here is my assessment: {"summary": "# Review Summary\n\nThe PR adds a `my-clients` workspace crate that generates typed HTTP clients from OpenAPI specs using `openapi-to-rust`, starting with a Forgejo client. Overall this is a solid, well-structured addition: the codegen config (`configs/forgejo.toml`) is clean, the committed contract is pinned to Forgejo 16.0.2, the README documents the regeneration workflow, response bodies are read with a hard memory bound (`__read_bounded_response_body`), the RFC 9457 problem-details handling is a nice touch, and the integration test smoke-tests the client end-to-end. I also verified that the generated `Authorization: Bearer <token>` scheme is valid — Forgejo's docs explicitly support `Bearer` tokens, so `with_api_key` works as-is.\n\nThere are a few issues worth addressing before merge:\n\n1. **Configured timeout is never applied.** `configs/forgejo.toml` declares `timeout_seconds = 30`, but the generated `HttpClient::new()` builds `reqwest::Client::new()` with no timeout and no request ever calls `.timeout()`, so the setting is dead and requests can hang indefinitely. The `HttpError::Timeout` variant is never constructed anywhere.\n2. **All response bodies are fully buffered in memory.** Despite enabling the `stream` feature, every response (including binary ones like `download_action_artifact`/`repo_get_archive`) is buffered into a `Vec` capped at 8 MB. Downloads larger than 8 MB fail with `ResponseTooLarge`, and there is no true streaming.\n3. **Contract regeneration depends on an unpinned third-party converter.** `convert-forgejo.sh` fetches the spec through `converter.swagger.io` at regeneration time, so the committed contract can silently drift from what regeneration produces and reproducibility depends on an external service.\n4. **Heavy native dependency tree in the workspace.** `my-clients` pulls in `aws-lc-sys` (cmake/cc build), `ring`, `rustls`, etc. Since it's a workspace member, every workspace-wide build, clippy, and CI job now compiles all of this, noticeably increasing build times.\n\nMinor notes: the generated 61k-line files are committed (maintainable as long as the documented `openapi-to-rust 0.12.1` is what people actually have installed — consider pinning it in the flake dev shell), and the dead `if false ||` guards in generated code are cosmetic only.", "verdict": "changes", "comments": [{"file": "crates/my-clients/configs/forgejo.toml", "line": 18, "severity": "bug", "message": "`timeout_seconds = 30` has no effect. The generated `HttpClient::new()` builds `reqwest::Client::new()` without a timeout (client.rs:235) and no generated request calls `.timeout()` before `req.send()`. reqwest's default is no timeout, so a non-responsive server causes the call to hang forever; the `HttpError::Timeout` variant is never constructed. Apply the timeout (e.g. via the reqwest `ClientBuilder` / a `.timeout()` on each request) so the configured value is honored, or remove the dead setting."}, {"file": "crates/my-clients/configs/forgejo.toml", "line": 19, "severity": "performance", "message": "`max_response_body_bytes = 8388608` together with `__read_bounded_response_body` means every response is fully buffered in memory and the `stream` feature is never used. Binary/streaming endpoints (`download_action_artifact`, `repo_get_archive`, release attachments, raw file/LFS) will error out with `ResponseTooLarge` as soon as a payload exceeds 8 MB and cannot be streamed to disk. Consider exposing per-endpoint streaming or a larger/raising limit for the download-style operations."}, {"file": "crates/my-clients/scripts/convert-forgejo.sh", "line": 5, "severity": "style", "message": "The contract is regenerated live through the third-party `converter.swagger.io` service with no pinned source version, so the fetched spec (and thus the generated API surface) can change between runs and diverge from the committed `contracts/forgejo.json`. Since the contract is already committed with a fixed version (16.0.2), consider documenting/pinning the upstream Forgejo version the script targets and validating the output is a well-formed OpenAPI document before overwriting the file."}, {"file": "crates/my-clients/Cargo.toml", "line": 13, "severity": "performance", "message": "This crate's rustls/aws-lc dependency tree (aws-lc-sys, ring, cmake/cc, etc.) is heavy to compile. Because `my-clients` is a workspace member, `cargo build`, the strict `cargo clippy --workspace`, and all CI/Nix jobs for the whole workspace now build these native deps, noticeably increasing build time and CI wall-clock. Worth confirming this is acceptable or scoping the workspace build (e.g. excluding the client crate from default workspace builds)."}]} --- *🤖 Review by opencode (opencode/deepseek-v4-flash-free) — verdict: **comment**

martin added 1 commit

2026-08-09 19:46:10 +00:00

Remove some features

ci/woodpecker/pr/pr-review Pipeline was canceled

Details

ci/woodpecker/push/verify Pipeline was successful

Details

abe369596c