SI-8: Eliminate all unwraps from the REST API client package#1973
Merged
josecelano merged 2 commits intoJul 13, 2026
Merged
Conversation
josecelano
force-pushed
the
1969-si-8-eliminate-unwraps-from-rest-api-client
branch
from
July 13, 2026 10:21
1b9a3fa to
b4a00c8
Compare
There was a problem hiding this comment.
Pull request overview
This PR implements SI-8 by making the low-level REST API HTTP client (torrust-tracker-rest-api-client) return Result instead of panicking, and updates downstream callers (contract tests, integration tests, and the qBittorrent E2E wrapper) to compile against the new signatures.
Changes:
- Updated
ApiHttpClientpublic request methods (and the freegethelper) to returnResult<Response, ClientError>rather than unwrapping. - Updated REST API server contract tests and integration tests to explicitly unwrap/expect the new
Resultin test code. - Propagated errors in the qBittorrent E2E tracker client wrapper with
?, and applied a small clippy-driven cleanup inhttp_health_check.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/servers/api/contract/stats/mod.rs | Adapts integration helper to the new fallible REST call by using expect(...). |
| src/console/ci/qbittorrent_e2e/tracker/client.rs | Propagates REST API client errors with ? in E2E runner wrapper. |
| src/bin/http_health_check.rs | Removes redundant_else by flattening the non-success branch. |
| packages/rest-api-client/src/v1/client.rs | Changes ApiHttpClient public methods + free get to return Result and adjusts internal wiring. |
| packages/axum-rest-api-server/tests/server/v1/contract/context/whitelist.rs | Updates contract tests to unwrap the new Result<Response, ClientError> return values. |
| packages/axum-rest-api-server/tests/server/v1/contract/context/torrent.rs | Updates contract tests to unwrap the new Result<Response, ClientError> return values. |
| packages/axum-rest-api-server/tests/server/v1/contract/context/stats.rs | Updates contract tests to unwrap the new Result<Response, ClientError> return values. |
| packages/axum-rest-api-server/tests/server/v1/contract/context/health_check.rs | Adjusts to get(...) -> Result by unwrapping in the contract test. |
| packages/axum-rest-api-server/tests/server/v1/contract/context/auth_key.rs | Updates contract tests to unwrap the new Result<Response, ClientError> return values. |
| packages/axum-rest-api-server/tests/server/v1/contract/authentication.rs | Updates contract tests to unwrap the new Result<Response, ClientError> return values. |
| docs/issues/open/1969-1938-si-8-eliminate-unwraps-from-rest-api-client.md | Updates issue spec status/progress and documents the two-phase caller strategy. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…ient package
Change all panicking `ApiHttpClient` public methods to return
`Result<Response, ClientError>`. Also changed the free function `get()`,
`get_request()`, and `get_request_with_query()` to return `Result`.
Updated all callers:
- Contract test callers use `.unwrap()` / `.expect()` (test code)
- E2E `TrackerApiClient` propagates errors with `?` (production code)
- Integration test uses `.expect()` (test code)
The 4 remaining `.expect("infallible: ...")` calls in `headers_with_request_id`,
`headers_with_auth_token`, and `get_request_with_query_result` auth token
inserts are provably infallible conversions as documented.
Also fixed a pre-existing clippy `redundant_else` warning in
`http_health_check.rs`.
Closes torrust#1969
josecelano
force-pushed
the
1969-si-8-eliminate-unwraps-from-rest-api-client
branch
from
July 13, 2026 10:27
b4a00c8 to
dbb84e2
Compare
The methods are no longer 'fallible versions that return Result instead of panicking' since the public wrappers now also return Result. Update comments to describe what each method actually adds (token/query injection). Addresses Copilot PR review feedback.
Member
Author
|
ACK bb0e3d2 |
josecelano
added a commit
that referenced
this pull request
Jul 15, 2026
…ssues/open to docs/issues/closed 174904d chore(ci): point to ADR instead of closed issue spec in deployment-packages.yaml (Jose Celano) 2e2178f docs(skill): document branch-already-exists edge case in cleanup-completed-issues skill (Jose Celano) 1bfd306 chore(issues): archive closed issue specs #1447, #1459, #1505, #1926, #1938, #1944, #1959, #1964, #1965, #1969 to docs/issues/closed (Jose Celano) Pull request description: Archives the following 10 issue specs that are closed on GitHub, moving them from `docs/issues/open/` to `docs/issues/closed/`: | Issue | Title | PR | |-------|-------|-----| | #1447 | Change logging threshold for connection ID error to WARNING | #1975 | | #1459 | Docker Security Overhaul | #1958 | | #1505 | Optimize peer IP list from swarm (implementation rejected) | #1949 | | #1926 | Define package versioning strategy | #1961 | | #1938 | REST API Contract-First Migration EPIC | #1960 (SI-1→5) | | #1944 | SI-6: Align REST API client | #1968 | | #1959 | SI-7: Review tests, align v1 namespace | #1963 | | #1964 | Rename number-of-downloads BTreeMap type alias | #1972 | | #1965 | SI-34: Consolidate duplicate HTTP types | #1974 | | #1969 | SI-8: Eliminate unwraps from REST API client | #1973 | ### Changes Made - All 10 specs verified `CLOSED` on GitHub with merged PRs - Files/directories moved via `git mv` to `docs/issues/closed/` - Frontmatter updated: `status: done`, `spec-path` → closed path, `last-updated-utc: 2026-07-15` - Workflow checkpoints updated and progress log entries added - Supplementary files in multi-file directories updated with new paths - Parent EPIC specs (#1669, #1938) updated: subissue status, spec paths, and table references - Fixed stale references in `.github/workflows/deployment-packages.yaml`, `docs/adrs/`, and `docs/issues/closed/` files - Pre-commit and pre-push hooks passed (linter, tests, doc-tests, nightly checks) ACKs for top commit: josecelano: ACK 174904d Tree-SHA512: c7e38354bb5898fc5125a0b68fd61a02ca273689593638eb3b955b9176038ef82c40fc98b0a2f722eb18d8d429d2f2dbb6f205eee3061211ab9906c5be46942b
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Eliminates all
.unwrap()calls from thetorrust-tracker-rest-api-clientpackage as specified in issue #1969.Changes
ApiHttpClientmethods now returnResult<Response, ClientError>instead of panickingpost_empty(),post_form(),get(),get_request(),get_request_with_query()— all returnResultget()— returnsResult.expect("infallible: ...")with rationale (headers_with_request_id,headers_with_auth_token, auth token inserts inget_request_with_query_result).unwrap()(test code), E2E runner propagates with?(production code)redundant_elseinhttp_health_check.rsDesign Decisions
Per discussion (2026-07-13):
.unwrap(),.expect(), or?.Result: No panicking public API surface remains.Verification
ApiHttpClientpublic methods returnResult<Response, ClientError>.unwrap()calls remain (only.expect("infallible: ..."))cargo check --tests --workspacepassescargo clippy --lib -p torrust-tracker-rest-api-clientpassesCloses #1969