From 1ed32e694bef33d5c1127d38d34597071b1bea72 Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Tue, 30 Jun 2026 09:37:52 +0100 Subject: [PATCH 01/10] docs(issues): add issue specs for #1964, #1965 (SI-34), and #1966 (SI-35) --- ...umber-of-downloads-btree-map-type-alias.md | 159 +++++++++++++ .../ISSUE.md | 197 ++++++++++++++++ .../manual-verification.md | 113 +++++++++ ...9-si-35-consolidate-duplicate-udp-types.md | 219 ++++++++++++++++++ 4 files changed, 688 insertions(+) create mode 100644 docs/issues/open/1964-rename-number-of-downloads-btree-map-type-alias.md create mode 100644 docs/issues/open/1965-1669-si-34-consolidate-duplicate-http-types/ISSUE.md create mode 100644 docs/issues/open/1965-1669-si-34-consolidate-duplicate-http-types/manual-verification.md create mode 100644 docs/issues/open/1966-1669-si-35-consolidate-duplicate-udp-types.md diff --git a/docs/issues/open/1964-rename-number-of-downloads-btree-map-type-alias.md b/docs/issues/open/1964-rename-number-of-downloads-btree-map-type-alias.md new file mode 100644 index 000000000..f7365233a --- /dev/null +++ b/docs/issues/open/1964-rename-number-of-downloads-btree-map-type-alias.md @@ -0,0 +1,159 @@ +--- +doc-type: issue +issue-type: task +status: planned +priority: p2 +github-issue: 1964 +spec-path: docs/issues/open/1964-rename-number-of-downloads-btree-map-type-alias.md +branch: "1964-rename-number-of-downloads-btree-map" +related-pr: null +last-updated-utc: 2026-06-30 12:00 +semantic-links: + skill-links: + - create-issue + related-artifacts: + - packages/primitives/src/lib.rs + - packages/tracker-core/src/databases/traits/torrent_metrics.rs + - packages/tracker-core/src/databases/driver/sqlite/torrent_metrics_store.rs + - packages/tracker-core/src/databases/driver/mysql/torrent_metrics_store.rs + - packages/tracker-core/src/databases/driver/postgres/torrent_metrics_store.rs + - packages/tracker-core/src/statistics/persisted/downloads.rs + - packages/tracker-core/src/torrent/repository/in_memory.rs + - packages/tracker-core/src/torrent/manager.rs + - packages/swarm-coordination-registry/src/swarm/registry.rs + - packages/torrent-repository-benchmarking/src/repository/mod.rs + - packages/torrent-repository-benchmarking/src/repository/ + - packages/torrent-repository-benchmarking/tests/ +--- + + + +# Issue #1964 - Rename `NumberOfDownloadsBTreeMap` to `NumberOfDownloadsPerInfoHash` + +## Goal + +Rename the type alias `NumberOfDownloadsBTreeMap` to `NumberOfDownloadsPerInfoHash` so the name +expresses the _intent_ of the type ("downloads per info-hash") rather than its internal +implementation (`BTreeMap`). + +## Background + +The type alias is defined in `packages/primitives/src/lib.rs`: + +```rust +pub type NumberOfDownloads = u32; +pub type NumberOfDownloadsBTreeMap = BTreeMap; +``` + +It represents the number of completed downloads per info-hash and serves as the persistence +boundary for torrent download counts — used by all three database drivers (SQLite, MySQL, +PostgreSQL) when loading torrent metrics from the database. + +The current name `NumberOfDownloadsBTreeMap` leaks the implementation detail (`BTreeMap`). If the +underlying collection were ever changed (e.g., to a `HashMap`), the name would become misleading +and need a follow-up rename. + +The sibling type `NumberOfDownloads` is named after _what_ it represents, not _how_ it's stored +(`u32`). The pair should follow the same convention. + +A workspace-wide search found 19 source files and 4 documentation files referencing this alias, +making this a low-risk but moderately broad rename. + +## Scope + +### In Scope + +- Rename `NumberOfDownloadsBTreeMap` to `NumberOfDownloadsPerInfoHash` in `packages/primitives/src/lib.rs` +- Update all references across the workspace (~19 source files + 4 doc files) +- Verify `linter all` and the full test suite pass + +### Out of Scope + +- Changing the underlying collection type (`BTreeMap` → something else) +- Renaming other type aliases in the codebase +- Changing the `NumberOfDownloads` alias (already well-named) + +## Implementation Plan + +Status values: `TODO`, `IN_PROGRESS`, `BLOCKED`, `DONE`. + +| ID | Status | Task | Notes / Expected Output | +| --- | ------ | ------------------------------------- | -------------------------------------------------------------------------------------------------------- | +| T1 | TODO | Rename definition in primitives crate | Change `NumberOfDownloadsBTreeMap` to `NumberOfDownloadsPerInfoHash` in `packages/primitives/src/lib.rs` | +| T2 | TODO | Update core domain references | Update imports/usages in `tracker-core`, `swarm-coordination-registry`, etc. | +| T3 | TODO | Update benchmarking references | Update imports/usages in `torrent-repository-benchmarking` crate and tests | +| T4 | TODO | Update documentation | Update the 4 doc files referencing the old name | +| T5 | TODO | Run full verification | `linter all`, `cargo test --workspace`, pre-commit checks | + +## Progress Tracking + +### Workflow Checkpoints + +- [x] Spec drafted in `docs/issues/drafts/` +- [ ] Spec reviewed and approved by user/maintainer +- [ ] GitHub issue created and issue number added to this spec +- [ ] (Optional, recommended for complex issues) Spec-only PR merged into `develop` before implementation +- [ ] Implementation completed +- [ ] Automatic verification completed (`linter all`, relevant tests, and any pre-push checks) +- [ ] Manual verification scenarios executed and recorded (status + evidence) +- [ ] Acceptance criteria reviewed after implementation and updated with evidence +- [ ] Reviewer validated acceptance criteria and updated checkboxes +- [ ] Committer verified spec progress is up to date before commit +- [ ] Issue closed and spec moved from `docs/issues/open/` to `docs/issues/closed/` + +### Progress Log + +- 2026-06-30 12:00 UTC - Copilot - Spec draft created + +## Acceptance Criteria + +- [ ] AC1: `NumberOfDownloadsBTreeMap` no longer appears anywhere in the codebase +- [ ] AC2: `NumberOfDownloadsPerInfoHash` is the sole name for the type alias +- [ ] AC3: All tests pass (`cargo test --workspace`) +- [ ] AC4: `linter all` exits with code `0` +- [ ] AC5: Pre-commit checks pass +- [ ] Manual verification scenarios are executed and documented (status + evidence) +- [ ] Acceptance criteria are re-reviewed after implementation and reflect actual behavior + +## Verification Plan + +### Automatic Checks + +- `linter all` +- `cargo test --workspace` +- Pre-commit checks (`./contrib/dev-tools/git/hooks/pre-commit.sh`) + +### Manual Verification Scenarios + +Status values: `TODO`, `IN_PROGRESS`, `DONE`, `FAILED`, `BLOCKED`. + +| ID | Scenario | Command/Steps | Expected Result | Status | Evidence | +| --- | --------------------------- | ----------------------------------------------------------------------- | ------------------------------------------ | ------ | ---------------------------- | +| M1 | Build succeeds after rename | `cargo build --workspace` | Zero errors, no warnings related to rename | TODO | {log/output/screenshot/path} | +| M2 | grep confirms no old name | `grep -r "NumberOfDownloadsBTreeMap" --include="*.rs" --include="*.md"` | No matches found | TODO | {log/output/screenshot/path} | + +### Acceptance Verification + +| AC ID | Status (`TODO`/`DONE`) | Evidence | +| ----- | ---------------------- | ------------------ | +| AC1 | TODO | {test/log/PR link} | +| AC2 | TODO | {test/log/PR link} | +| AC3 | TODO | {test/log/PR link} | +| AC4 | TODO | {test/log/PR link} | +| AC5 | TODO | {test/log/PR link} | + +## Risks and Trade-offs + +- **Risk**: Mass rename could miss a reference if a file uses a differently-formatted reference + (e.g., macro-generated code). **Mitigation**: grep for the old name after the rename to confirm + zero matches. +- **Risk**: External consumers of `torrust-tracker-primitives` (crates.io) could break if they + depend on the old name. **Mitigation**: Check if any published reverse-dependencies use this + type. The crate has minimal external consumers and the type is internal-facing. + +## References + +- Definition: `packages/primitives/src/lib.rs` (line 71) +- Usage sites: 19 source files across `tracker-core`, `swarm-coordination-registry`, + `torrent-repository-benchmarking`, and their tests +- Docs: 4 documentation files in `docs/issues/` referencing the type diff --git a/docs/issues/open/1965-1669-si-34-consolidate-duplicate-http-types/ISSUE.md b/docs/issues/open/1965-1669-si-34-consolidate-duplicate-http-types/ISSUE.md new file mode 100644 index 000000000..31fbfed86 --- /dev/null +++ b/docs/issues/open/1965-1669-si-34-consolidate-duplicate-http-types/ISSUE.md @@ -0,0 +1,197 @@ +--- +doc-type: issue +issue-type: task +status: planned +priority: p1 +github-issue: 1965 +spec-path: docs/issues/open/1965-1669-si-34-consolidate-duplicate-http-types/ISSUE.md +issue-folder: docs/issues/open/1965-1669-si-34-consolidate-duplicate-http-types/ +branch: "1965-1669-si-34-consolidate-duplicate-http-types" +related-pr: null +last-updated-utc: 2026-06-30 12:00 +semantic-links: + skill-links: + - create-issue + - run-tracker-locally + related-artifacts: + - docs/issues/open/1669-overhaul-packages/EPIC.md + - docs/issues/open/1669-overhaul-packages/DECISIONS.md + - packages/http-protocol/src/v1/requests/ + - packages/http-protocol/src/v1/responses/ + - packages/axum-http-server/tests/server/requests/ + - packages/axum-http-server/tests/server/responses/ + - packages/tracker-client/src/http/client/requests/ + - packages/tracker-client/src/http/client/responses/ + - .github/skills/dev/environment-setup/run-tracker-locally/SKILL.md +--- + + + +# Issue #1965 - EPIC 1669 SI-34: Consolidate Duplicate HTTP Types into `http-protocol` + +> **Parent EPIC**: [#1669 — Overhaul: Packages](https://github.com/torrust/torrust-tracker/issues/1669) +> **EPIC Reference**: `docs/issues/open/1669-overhaul-packages/EPIC.md` +> +> **Issue type**: Folder issue — manual verification evidence and command logs will be documented +> in a separate `manual-verification.md` file alongside this spec inside the issue folder at +> `docs/issues/open/1965-1669-si-34-consolidate-duplicate-http-types/`. + +## Goal + +Eliminate duplicate HTTP request/response type definitions across the workspace by consolidating +them into `packages/http-protocol`, and add the `http-protocol` dependency to `tracker-client` so +both consumers import from a single source of truth. + +## Background + +Three crate locations define overlapping HTTP request and response types: + +1. **`packages/http-protocol/src/v1/{requests,responses}/`** — server-side protocol parsing (production library) +2. **`packages/axum-http-server/tests/server/{requests,responses}/`** — test helpers (test-only code) +3. **`packages/tracker-client/src/http/client/{requests,responses}/`** — tracker client library (production library) + +Locations (2) and (3) define their own copies of types that semantically belong in (1): + +- `axum-http-server` **has** `http-protocol` as a dependency, but its tests define their own types instead of using it +- `tracker-client` does **not** depend on `http-protocol` at all + +The duplication creates maintenance burden: any change to these types must be replicated in two +or three places. Several types (especially `Error`, `Compact`, `CompactPeer`, `CompactPeerList`, +scrape `Query`/`QueryBuilder`/`QueryParams`, `ByteArray20`, `InfoHash`, `percent_encode_byte_array`) +are byte-for-byte identical between locations (2) and (3). + +The `http-protocol` crate is the canonical home for HTTP tracker protocol types. Client-side +parsing/serialization types are a natural extension of this crate, not a separate concern. + +## Scope + +### In Scope + +- Add client-side request construction and response deserialization types to `packages/http-protocol` + (e.g., query builders, response structs with `serde_bencode` derives) +- Replace duplicate types in `packages/axum-http-server/tests/server/` with imports from `http-protocol` +- Replace duplicate types in `packages/tracker-client/src/http/client/` with imports from `http-protocol` +- Add `http-protocol` as a dependency of `tracker-client` +- Create a `use-tracker-client` skill in `.github/skills/usage/` capturing the manual verification learnings +- Verify all tests pass and no functionality regresses + +### Out of Scope + +- Merging `packages/http-protocol` with other protocol crates +- Changing the public API of `http-protocol` beyond what's needed for consolidation +- Removing or refactoring the server-side types in `http-protocol` +- Changing how `axum-http-server` production code uses `http-protocol` + +## Implementation Plan + +Status values: `TODO`, `IN_PROGRESS`, `BLOCKED`, `DONE`. + +| ID | Status | Task | Notes / Expected Output | +| --- | ------ | --------------------------------------------------- | ----------------------------------------------------------------------------------------------- | +| T1 | TODO | Survey duplicate types and decide merge strategy | Catalog exact types to move; identify which location has the "best" version | +| T2 | TODO | Add client-side types to `http-protocol` | Move query builders, response deserialization structs, and shared helpers | +| T3 | TODO | Add `http-protocol` dependency to `tracker-client` | Update `Cargo.toml`, verify dependency tree | +| T4 | TODO | Replace duplicate types in `tracker-client` | Delete local copies, update imports to `http-protocol` | +| T5 | TODO | Replace duplicate types in `axum-http-server` tests | Delete local copies, update imports to `http-protocol` | +| T6 | TODO | Run full verification | `linter all`, `cargo test --workspace`, pre-commit, pre-push | +| T7 | TODO | Create `use-tracker-client` skill | New skill in `.github/skills/usage/use-tracker-client/` with learnings from manual verification | + +## Progress Tracking + +### Workflow Checkpoints + +- [x] Spec drafted in `docs/issues/drafts/` +- [ ] Spec reviewed and approved by user/maintainer +- [ ] GitHub issue created and issue number added to this spec +- [ ] (Optional, recommended for complex issues) Spec-only PR merged into `develop` before implementation +- [ ] Implementation completed +- [ ] Automatic verification completed (`linter all`, relevant tests, and any pre-push checks) +- [ ] Manual verification scenarios executed and recorded (status + evidence) +- [ ] Acceptance criteria reviewed after implementation and updated with evidence +- [ ] Reviewer validated acceptance criteria and updated checkboxes +- [ ] Committer verified spec progress is up to date before commit +- [ ] Issue closed and spec moved from `docs/issues/open/` to `docs/issues/closed/` + +### Progress Log + +- 2026-06-30 12:00 UTC - Copilot - Spec draft created + +## Acceptance Criteria + +- [ ] AC1: No HTTP request/response types are duplicated between `http-protocol`, `axum-http-server` tests, and `tracker-client` +- [ ] AC2: `tracker-client` depends on `http-protocol` and imports types from it instead of defining its own +- [ ] AC3: `axum-http-server` tests import types from `http-protocol` instead of defining their own +- [ ] AC4: All existing tests pass (`cargo test --workspace`) +- [ ] AC5: `linter all` exits with code `0` +- [ ] AC6: Pre-commit and pre-push checks pass +- [ ] AC7: No `deps.rs` or layer-violation regressions +- [ ] AC8: `use-tracker-client` skill is created in `.github/skills/usage/` with proper YAML frontmatter and instructions +- [ ] Manual verification scenarios are executed and documented (status + evidence) +- [ ] Acceptance criteria are re-reviewed after implementation and reflect actual behavior + +## Verification Plan + +### Automatic Checks + +- `linter all` +- `cargo test --workspace` +- Pre-commit checks (`./contrib/dev-tools/git/hooks/pre-commit.sh`) +- Pre-push checks (`./contrib/dev-tools/git/hooks/pre-push.sh`) +- `cargo machete` (no unused dependencies introduced) + +### Manual Verification Scenarios + +Status values: `TODO`, `IN_PROGRESS`, `DONE`, `FAILED`, `BLOCKED`. + +All manual verification evidence — including full command output, troubleshooting notes, and +step-by-step logs — will be recorded in a separate `manual-verification.md` file inside the +issue folder. The Evidence column below links to the relevant section of that file. + +**Skills used during manual verification**: + +- **Run tracker locally**: [`.github/skills/dev/environment-setup/run-tracker-locally/SKILL.md`](../../.github/skills/dev/environment-setup/run-tracker-locally/SKILL.md) — start the tracker with default development configuration +- **Tracker client**: No dedicated skill exists yet. A `use-tracker-client` skill will be created + in `.github/skills/usage/` as the final step of this issue, capturing the learnings from the + manual verification process. + +| ID | Scenario | Command/Steps | Expected Result | Status | Evidence | +| --- | ----------------------------------------------- | ---------------------------------------------------------------------------------------- | --------------------------------------------------- | ------ | ------------------------------------ | +| M1 | HTTP tracker announces work with tracker-client | Run `tracker_client http announce` against a local tracker; verify request/response flow | Same behavior as before the consolidation | TODO | `manual-verification.md#m1-announce` | +| M2 | HTTP scrape works with tracker-client | Run `tracker_client http scrape` against a local tracker | Same behavior as before | TODO | `manual-verification.md#m2-scrape` | +| M3 | axum-http-server integration tests pass | `cargo test -p torrust-tracker-axum-http-server --test integration` | All tests pass | TODO | `manual-verification.md#m3-tests` | +| M4 | No duplicate type definitions remain | `grep` for key struct names (e.g., `struct Query`, `struct CompactPeer`) in old paths | Only imports, no local definitions for merged types | TODO | `manual-verification.md#m4-grep` | + +### Acceptance Verification + +| AC ID | Status (`TODO`/`DONE`) | Evidence | +| ----- | ---------------------- | ------------------ | +| AC1 | TODO | {test/log/PR link} | +| AC2 | TODO | {test/log/PR link} | +| AC3 | TODO | {test/log/PR link} | +| AC4 | TODO | {test/log/PR link} | +| AC5 | TODO | {test/log/PR link} | +| AC6 | TODO | {test/log/PR link} | +| AC7 | TODO | {test/log/PR link} | +| AC8 | TODO | {test/log/PR link} | + +## Risks and Trade-offs + +- **Risk**: Client-side types differ subtly between `tracker-client` and `axum-http-server` tests + (e.g., `Event` default variant, `numwant` field presence). **Mitigation**: The implementer must + survey both versions and ensure the consolidated type in `http-protocol` accommodates both use + cases. Where differences are intentional, use configuration (e.g., builder methods, `Option` + fields) rather than separate types. +- **Risk**: Adding `http-protocol` as a dependency of `tracker-client` increases compile time for + the client. **Mitigation**: `http-protocol` is already a lightweight crate with few transitive + dependencies; the impact should be negligible. +- **Risk**: The consolidation might change the public API of `http-protocol`, potentially breaking + external consumers. **Mitigation**: Review all existing `pub` exports and ensure backward + compatibility, or bump the version appropriately with clear changelog entries. + +## References + +- Parent EPIC: [#1669](https://github.com/torrust/torrust-tracker/issues/1669) +- EPIC spec: `docs/issues/open/1669-overhaul-packages/EPIC.md` +- Decisions log: `docs/issues/open/1669-overhaul-packages/DECISIONS.md` +- Duplicate analysis: exploration performed 2026-06-30 by Copilot +- Related ADR: `docs/adrs/20260527175600_keep_protocol_and_domain_types_decoupled.md` diff --git a/docs/issues/open/1965-1669-si-34-consolidate-duplicate-http-types/manual-verification.md b/docs/issues/open/1965-1669-si-34-consolidate-duplicate-http-types/manual-verification.md new file mode 100644 index 000000000..22a7d4fd8 --- /dev/null +++ b/docs/issues/open/1965-1669-si-34-consolidate-duplicate-http-types/manual-verification.md @@ -0,0 +1,113 @@ +# Manual Verification — Issue #1965 (EPIC 1669 SI-34) + +> This file records manual verification evidence for the issue. +> It is populated during implementation. +> +> Skills used: +> +> - Run tracker locally: `.github/skills/dev/environment-setup/run-tracker-locally/SKILL.md` +> - Tracker client: skill to be created as part of this issue (T7) + +--- + +## M1: HTTP tracker announces work with tracker-client + +| Field | Value | +| ---------------- | ------ | +| **Status** | `TODO` | +| **Date** | | +| **Performed by** | | + +### Steps + +```text +(To be filled during verification) +``` + +### Output + +```text +(To be filled during verification) +``` + +### Result + +(To be filled: PASS / FAIL with notes) + +--- + +## M2: HTTP scrape works with tracker-client + +| Field | Value | +| ---------------- | ------ | +| **Status** | `TODO` | +| **Date** | | +| **Performed by** | | + +### Steps + +```text +(To be filled during verification) +``` + +### Output + +```text +(To be filled during verification) +``` + +### Result + +(To be filled: PASS / FAIL with notes) + +--- + +## M3: axum-http-server integration tests pass + +| Field | Value | +| ---------------- | ------ | +| **Status** | `TODO` | +| **Date** | | +| **Performed by** | | + +### Steps + +```text +(To be filled during verification) +``` + +### Output + +```text +(To be filled during verification) +``` + +### Result + +(To be filled: PASS / FAIL with notes) + +--- + +## M4: No duplicate type definitions remain + +| Field | Value | +| ---------------- | ------ | +| **Status** | `TODO` | +| **Date** | | +| **Performed by** | | + +### Steps + +```text +(To be filled during verification) +``` + +### Output + +```text +(To be filled during verification) +``` + +### Result + +(To be filled: PASS / FAIL with notes) diff --git a/docs/issues/open/1966-1669-si-35-consolidate-duplicate-udp-types.md b/docs/issues/open/1966-1669-si-35-consolidate-duplicate-udp-types.md new file mode 100644 index 000000000..2dda496b3 --- /dev/null +++ b/docs/issues/open/1966-1669-si-35-consolidate-duplicate-udp-types.md @@ -0,0 +1,219 @@ +--- +doc-type: issue +issue-type: task +status: planned +priority: p2 +github-issue: 1966 +spec-path: docs/issues/open/1966-1669-si-35-consolidate-duplicate-udp-types.md +branch: "1966-1669-si-35-consolidate-duplicate-udp-types" +related-pr: null +last-updated-utc: 2026-06-30 12:00 +semantic-links: + skill-links: + - create-issue + related-artifacts: + - docs/issues/open/1669-overhaul-packages/EPIC.md + - docs/issues/open/1669-overhaul-packages/DECISIONS.md + - packages/udp-protocol/src/ + - packages/udp-core/src/event.rs + - packages/udp-server/src/event.rs + - packages/udp-server/src/lib.rs + - packages/tracker-client/src/udp/mod.rs + - docs/adrs/20260527175600_keep_protocol_and_domain_types_decoupled.md + - packages/primitives/src/announce.rs + - packages/http-protocol/src/v1/requests/announce.rs + - packages/http-protocol/src/v1/responses/announce.rs + - packages/http-protocol/src/v1/responses/scrape.rs +--- + + + +# Issue #1966 - EPIC 1669 SI-35: Consolidate Duplicate UDP Types + +> **Parent EPIC**: [#1669 — Overhaul: Packages](https://github.com/torrust/torrust-tracker/issues/1669) +> **EPIC Reference**: `docs/issues/open/1669-overhaul-packages/EPIC.md` + +## Goal + +Eliminate duplicate type definitions and constants in the UDP tracker packages by consolidating +them into their canonical locations. + +## Background + +A workspace-wide audit of UDP-related packages found that the UDP layer is significantly cleaner +than the HTTP layer — the core protocol types (`ConnectRequest`, `ConnectResponse`, +`AnnounceRequest`, `AnnounceResponse`, `ScrapeRequest`, `ScrapeResponse`, `Request`, `Response`, +`ErrorResponse`, `ResponsePeer`, `TorrentScrapeStatistics`) are defined exclusively in +`packages/udp-protocol/src/` and imported everywhere else. This is the correct architecture. + +However, three duplications were found: + +### 🔴 `ConnectionContext` — full copy-paste + +The struct and its entire `impl` block are duplicated between: + +| | `packages/udp-core/src/event.rs` (line 26) | `packages/udp-server/src/event.rs` (line 85) | +| ------------------------------------------ | ----------------------------------------------------------------------------------------------------------------- | ----------------------------------------------------------------------------------------------------------------- | +| **Fields** | `pub client_socket_addr: SocketAddr`, `pub server_service_binding: ServiceBinding` | `client_socket_addr: SocketAddr` (private), `server_service_binding: ServiceBinding` (private) | +| **Methods** | `new()`, `client_socket_addr()`, `server_socket_addr()`, `client_address_ip_family()`, `client_address_ip_type()` | `new()`, `client_socket_addr()`, `server_socket_addr()`, `client_address_ip_family()`, `client_address_ip_type()` | +| **Derive** | `Debug, PartialEq, Eq, Clone` | `Debug, PartialEq, Eq, Clone` | +| **`From for LabelSet`** | Yes | Yes | + +The only difference is field visibility (`pub` in core, private in server). The impl blocks are +identical. One should be the canonical definition and the other should import it. + +### 🟡 `MAX_PACKET_SIZE` — same constant, two locations + +| Package | File | Value | +| -------------------------------------------------- | ------------------------------------------ | ------ | +| `packages/udp-server/src/lib.rs` (line 651) | `pub const MAX_PACKET_SIZE: usize = 1496;` | `1496` | +| `packages/tracker-client/src/udp/mod.rs` (line 11) | `pub const MAX_PACKET_SIZE: usize = 1496;` | `1496` | + +The `tracker-client` already depends on `udp-protocol`. This constant could live in +`udp-protocol` and be shared by both consumers. + +### 🟡 `PROTOCOL_ID` — dead code copy + +| Package | Symbol | Value | Visibility | +| -------------------------------------------------- | --------------------- | ------------------- | ------------ | +| `packages/udp-protocol/src/connect.rs` (line 15) | `PROTOCOL_IDENTIFIER` | `4_497_486_125_440` | `pub(crate)` | +| `packages/tracker-client/src/udp/mod.rs` (line 14) | `PROTOCOL_ID` | `0x0417_2710_1980` | `pub` | + +Same magic constant with different names. `PROTOCOL_ID` in `tracker-client` is **unused** — a +grep shows no references to it anywhere. It should be removed. + +### 🟢 Intentional duplications (not in scope) + +The following are kept separate per +[ADR 20260527175600](docs/adrs/20260527175600_keep_protocol_and_domain_types_decoupled.md) +and are **not** addressed by this issue: + +- `AnnounceEvent` — `udp-protocol` vs `primitives` (wire type vs domain type) +- `InfoHash` — `udp-protocol` vs `torrust_info_hash` (wire type vs domain type) +- `NumberOfBytes` — `udp-protocol` vs `primitives` vs `http-protocol` (wire type vs domain type) + +These types currently have comments like `// Intentionally kept in...` or `// Intentional boundary duplication` but +do not explicitly reference the ADR. As part of this issue, each location will gain a `// adr:` comment so +future contributors understand the architectural reasoning and do not accidentally re-couple the types. + +**Code locations to annotate**: + +- `packages/udp-protocol/src/common.rs` — `InfoHash` (line 20) and `NumberOfBytes` (line 46) +- `packages/http-protocol/src/v1/requests/announce.rs` — `NumberOfBytes` (line 28) +- `packages/http-protocol/src/v1/responses/announce.rs` — `Announce` DTO (line 11) +- `packages/http-protocol/src/v1/responses/scrape.rs` — scrape response DTOs (lines 10, 20) +- `packages/primitives/src/announce.rs` — `AnnounceEvent` (line 91) + +## Scope + +### In Scope + +- Consolidate `ConnectionContext` into a single canonical definition (likely in `udp-core`) +- Move `MAX_PACKET_SIZE` to `udp-protocol` and import it in both `udp-server` and `tracker-client` +- Remove the unused `PROTOCOL_ID` constant from `tracker-client` +- Add `adr:` comments to the code locations listed under "Intentional duplications" referencing + ADR `docs/adrs/20260527175600_keep_protocol_and_domain_types_decoupled.md`, so future + contributors understand why the duplication exists and do not accidentally re-couple the types +- Verify all tests pass and no functionality regresses + +### Out of Scope + +- Merging protocol-level types (`AnnounceEvent`, `InfoHash`, `NumberOfBytes`) — governed by ADR +- Changing the public API of `udp-protocol` beyond what's needed for consolidation +- Refactoring the UDP server architecture + +## Implementation Plan + +Status values: `TODO`, `IN_PROGRESS`, `BLOCKED`, `DONE`. + +| ID | Status | Task | Notes / Expected Output | +| --- | ------ | ------------------------------------------------ | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | +| T1 | TODO | Consolidate `ConnectionContext` into `udp-core` | Make `udp-server` import from `udp-core` instead of defining its own copy | +| T2 | TODO | Move `MAX_PACKET_SIZE` to `udp-protocol` | Add `pub const MAX_PACKET_SIZE: usize = 1496;` to `udp-protocol`; update imports | +| T3 | TODO | Remove dead `PROTOCOL_ID` from `tracker-client` | Delete the unused constant | +| T4 | TODO | Add `adr:` comments for intentional duplications | Annotate `udp-protocol/src/common.rs`, `http-protocol/src/v1/requests/announce.rs`, `http-protocol/src/v1/responses/announce.rs`, `http-protocol/src/v1/responses/scrape.rs`, and `primitives/src/announce.rs` with `// adr: docs/adrs/20260527175600...` comments | +| T5 | TODO | Run full verification | `linter all`, `cargo test --workspace`, pre-commit, pre-push | + +## Progress Tracking + +### Workflow Checkpoints + +- [x] Spec drafted in `docs/issues/drafts/` +- [ ] Spec reviewed and approved by user/maintainer +- [ ] GitHub issue created and issue number added to this spec +- [ ] (Optional, recommended for complex issues) Spec-only PR merged into `develop` before implementation +- [ ] Implementation completed +- [ ] Automatic verification completed (`linter all`, relevant tests, and any pre-push checks) +- [ ] Manual verification scenarios executed and recorded (status + evidence) +- [ ] Acceptance criteria reviewed after implementation and updated with evidence +- [ ] Reviewer validated acceptance criteria and updated checkboxes +- [ ] Committer verified spec progress is up to date before commit +- [ ] Issue closed and spec moved from `docs/issues/open/` to `docs/issues/closed/` + +### Progress Log + +- 2026-06-30 12:00 UTC - Copilot - Spec draft created + +## Acceptance Criteria + +- [ ] AC1: `ConnectionContext` is defined in exactly one location (imported by the other) +- [ ] AC2: `MAX_PACKET_SIZE` is defined in `udp-protocol` and imported by both `udp-server` and `tracker-client` +- [ ] AC3: `PROTOCOL_ID` no longer exists in `tracker-client` +- [ ] AC4: Each location listed in the "Intentional duplications" section has an `adr:` comment referencing the ADR +- [ ] AC5: All existing tests pass (`cargo test --workspace`) +- [ ] AC5: `linter all` exits with code `0` +- [ ] AC6: Pre-commit and pre-push checks pass +- [ ] Manual verification scenarios are executed and documented (status + evidence) +- [ ] Acceptance criteria are re-reviewed after implementation and reflect actual behavior + +## Verification Plan + +### Automatic Checks + +- `linter all` +- `cargo test --workspace` +- Pre-commit checks (`./contrib/dev-tools/git/hooks/pre-commit.sh`) +- Pre-push checks (`./contrib/dev-tools/git/hooks/pre-push.sh`) +- `cargo machete` (no unused dependencies introduced) + +### Manual Verification Scenarios + +Status values: `TODO`, `IN_PROGRESS`, `DONE`, `FAILED`, `BLOCKED`. + +| ID | Scenario | Command/Steps | Expected Result | Status | Evidence | +| --- | ---------------------------------------------- | --------------------------------------------------------------------------------------- | ----------------------------------------- | ------ | ---------------------------- | +| M1 | UDP tracker announces work with tracker-client | Run `tracker_client udp announce` against a local tracker; verify request/response flow | Same behavior as before the consolidation | TODO | {log/output/screenshot/path} | +| M2 | UDP scrape works with tracker-client | Run `tracker_client udp scrape` against a local tracker | Same behavior as before | TODO | {log/output/screenshot/path} | +| M3 | udp-server tests pass | `cargo test -p torrust-tracker-udp-server` | All tests pass | TODO | {log/output/screenshot/path} | +| M4 | No duplicate definitions remain | `grep` for `ConnectionContext` and `MAX_PACKET_SIZE` across workspace | Only one definition each | TODO | {log/output/screenshot/path} | + +### Acceptance Verification + +| AC ID | Status (`TODO`/`DONE`) | Evidence | +| ----- | ---------------------- | ------------------ | +| AC1 | TODO | {test/log/PR link} | +| AC2 | TODO | {test/log/PR link} | +| AC3 | TODO | {test/log/PR link} | +| AC4 | TODO | {test/log/PR link} | +| AC5 | TODO | {test/log/PR link} | +| AC6 | TODO | {test/log/PR link} | +| AC7 | TODO | {test/log/PR link} | + +## Risks and Trade-offs + +- **Risk**: `ConnectionContext` has different field visibility (`pub` in core, private in server). + **Mitigation**: The consolidated definition should use `pub` fields (or provide accessor methods) + so both consumers can use it without friction. +- **Risk**: Moving `MAX_PACKET_SIZE` to `udp-protocol` changes its visibility scope. + **Mitigation**: Make it `pub` in `udp-protocol`; both consumers already depend on it. +- **Risk**: Removing `PROTOCOL_ID` could break something if it's used via macro or build script. + **Mitigation**: The grep confirmed zero references; removal is safe. + +## References + +- Parent EPIC: [#1669](https://github.com/torrust/torrust-tracker/issues/1669) +- EPIC spec: `docs/issues/open/1669-overhaul-packages/EPIC.md` +- Decisions log: `docs/issues/open/1669-overhaul-packages/DECISIONS.md` +- Duplicate analysis: exploration performed 2026-06-30 by Copilot +- Related ADR: `docs/adrs/20260527175600_keep_protocol_and_domain_types_decoupled.md` +- Related HTTP consolidation issue: `docs/issues/drafts/1669-si-34-consolidate-duplicate-http-types-into-http-protocol.md` From 4862cb6f8933dfbe4ffa800f07f37e9051bfcadf Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Tue, 30 Jun 2026 10:12:50 +0100 Subject: [PATCH 02/10] chore(review): address Copilot PR suggestions for #1967 --- .../process-copilot-suggestions/SKILL.md | 2 +- .../ISSUE.md | 4 +- .../manual-verification.md | 10 +++++ ...9-si-35-consolidate-duplicate-udp-types.md | 4 +- ...ot-suggestions.md => EXAMPLE-COMPLETED.md} | 10 ++--- docs/pr-reviews/README.md | 4 +- .../pr-reviews/pr-1967-copilot-suggestions.md | 44 +++++++++++++++++++ 7 files changed, 66 insertions(+), 12 deletions(-) rename docs/pr-reviews/{pr-1733-copilot-suggestions.md => EXAMPLE-COMPLETED.md} (96%) create mode 100644 docs/pr-reviews/pr-1967-copilot-suggestions.md diff --git a/.github/skills/dev/pr-reviews/process-copilot-suggestions/SKILL.md b/.github/skills/dev/pr-reviews/process-copilot-suggestions/SKILL.md index 4f3ba5afc..1983e2839 100644 --- a/.github/skills/dev/pr-reviews/process-copilot-suggestions/SKILL.md +++ b/.github/skills/dev/pr-reviews/process-copilot-suggestions/SKILL.md @@ -174,7 +174,7 @@ Both are integrated into this workflow automatically. ## Example -See `docs/pr-reviews/pr-1733-copilot-suggestions.md` for a complete worked example +See `docs/pr-reviews/EXAMPLE-COMPLETED.md` for a complete worked example with all 26 Copilot suggestions processed, decided, and resolved. ## Completion Checklist diff --git a/docs/issues/open/1965-1669-si-34-consolidate-duplicate-http-types/ISSUE.md b/docs/issues/open/1965-1669-si-34-consolidate-duplicate-http-types/ISSUE.md index 31fbfed86..a1c1fe82c 100644 --- a/docs/issues/open/1965-1669-si-34-consolidate-duplicate-http-types/ISSUE.md +++ b/docs/issues/open/1965-1669-si-34-consolidate-duplicate-http-types/ISSUE.md @@ -149,9 +149,9 @@ issue folder. The Evidence column below links to the relevant section of that fi **Skills used during manual verification**: -- **Run tracker locally**: [`.github/skills/dev/environment-setup/run-tracker-locally/SKILL.md`](../../.github/skills/dev/environment-setup/run-tracker-locally/SKILL.md) — start the tracker with default development configuration +- **Run tracker locally**: [`../../../../.github/skills/dev/environment-setup/run-tracker-locally/SKILL.md`](../../../../.github/skills/dev/environment-setup/run-tracker-locally/SKILL.md) — start the tracker with default development configuration - **Tracker client**: No dedicated skill exists yet. A `use-tracker-client` skill will be created - in `.github/skills/usage/` as the final step of this issue, capturing the learnings from the + in `../../../../.github/skills/usage/` as the final step of this issue, capturing the learnings from the manual verification process. | ID | Scenario | Command/Steps | Expected Result | Status | Evidence | diff --git a/docs/issues/open/1965-1669-si-34-consolidate-duplicate-http-types/manual-verification.md b/docs/issues/open/1965-1669-si-34-consolidate-duplicate-http-types/manual-verification.md index 22a7d4fd8..cf13f977a 100644 --- a/docs/issues/open/1965-1669-si-34-consolidate-duplicate-http-types/manual-verification.md +++ b/docs/issues/open/1965-1669-si-34-consolidate-duplicate-http-types/manual-verification.md @@ -1,3 +1,13 @@ +--- +doc-type: issue +issue-type: task +status: planned +priority: p1 +github-issue: 1965 +spec-path: docs/issues/open/1965-1669-si-34-consolidate-duplicate-http-types/manual-verification.md +last-updated-utc: 2026-06-30 12:00 +--- + # Manual Verification — Issue #1965 (EPIC 1669 SI-34) > This file records manual verification evidence for the issue. diff --git a/docs/issues/open/1966-1669-si-35-consolidate-duplicate-udp-types.md b/docs/issues/open/1966-1669-si-35-consolidate-duplicate-udp-types.md index 2dda496b3..ac36075a5 100644 --- a/docs/issues/open/1966-1669-si-35-consolidate-duplicate-udp-types.md +++ b/docs/issues/open/1966-1669-si-35-consolidate-duplicate-udp-types.md @@ -161,8 +161,8 @@ Status values: `TODO`, `IN_PROGRESS`, `BLOCKED`, `DONE`. - [ ] AC3: `PROTOCOL_ID` no longer exists in `tracker-client` - [ ] AC4: Each location listed in the "Intentional duplications" section has an `adr:` comment referencing the ADR - [ ] AC5: All existing tests pass (`cargo test --workspace`) -- [ ] AC5: `linter all` exits with code `0` -- [ ] AC6: Pre-commit and pre-push checks pass +- [ ] AC6: `linter all` exits with code `0` +- [ ] AC7: Pre-commit and pre-push checks pass - [ ] Manual verification scenarios are executed and documented (status + evidence) - [ ] Acceptance criteria are re-reviewed after implementation and reflect actual behavior diff --git a/docs/pr-reviews/pr-1733-copilot-suggestions.md b/docs/pr-reviews/EXAMPLE-COMPLETED.md similarity index 96% rename from docs/pr-reviews/pr-1733-copilot-suggestions.md rename to docs/pr-reviews/EXAMPLE-COMPLETED.md index 90b06139d..eec879eff 100644 --- a/docs/pr-reviews/pr-1733-copilot-suggestions.md +++ b/docs/pr-reviews/EXAMPLE-COMPLETED.md @@ -6,9 +6,9 @@ semantic-links: - docs/pr-reviews/README.md --- -# PR #1733 Copilot Suggestions Tracking +# PR # Copilot Suggestions Tracking (EXAMPLE - COMPLETED) -Source: Copilot PR review threads for https://github.com/torrust/torrust-tracker/pull/1733 +Source: Copilot PR review threads for https://github.com/torrust/torrust-tracker/pull/ Status legend: @@ -18,9 +18,9 @@ Status legend: ## Processing Log -- 2026-05-06: Started processing suggestions (downloaded 26 threads from PR #1733) -- 2026-05-06: Applied code/doc fixes and committed changes -- 2026-05-06: Resolved all 26 threads in PR #1733 +- : Started processing suggestions (downloaded 26 threads from PR #) +- : Applied code/doc fixes and committed changes +- : Resolved all 26 threads in PR # All suggestions (action and no-action) have been processed and marked resolved. diff --git a/docs/pr-reviews/README.md b/docs/pr-reviews/README.md index bf70ec3c6..9bf99c41a 100644 --- a/docs/pr-reviews/README.md +++ b/docs/pr-reviews/README.md @@ -15,7 +15,7 @@ This directory contains tools and templates for managing GitHub Copilot code rev ## Files - [docs/templates/COPILOT-SUGGESTIONS-TEMPLATE.md](../templates/COPILOT-SUGGESTIONS-TEMPLATE.md) — Reusable template for tracking and processing Copilot suggestions on any PR. Copy and customize for each new PR. -- **pr-1733-copilot-suggestions.md** — Example of a completed suggestion review for PR #1733, showing how to document decisions, process suggestions, and track resolutions. +- **EXAMPLE-COMPLETED.md** — Example of a completed suggestion review, showing how to document decisions, process suggestions, and track resolutions. Use uppercase `EXAMPLE` files as reference; copy the template from `docs/templates/COPILOT-SUGGESTIONS-TEMPLATE.md` for new PRs. ## Workflow @@ -33,4 +33,4 @@ This directory contains tools and templates for managing GitHub Copilot code rev ## Example -See `pr-1733-copilot-suggestions.md` for a complete example where all 26 Copilot suggestions were reviewed, processed, and resolved. +See `EXAMPLE-COMPLETED.md` for a complete example where all 26 Copilot suggestions were reviewed, processed, and resolved. diff --git a/docs/pr-reviews/pr-1967-copilot-suggestions.md b/docs/pr-reviews/pr-1967-copilot-suggestions.md new file mode 100644 index 000000000..e382fd447 --- /dev/null +++ b/docs/pr-reviews/pr-1967-copilot-suggestions.md @@ -0,0 +1,44 @@ +--- +semantic-links: + skill-links: + - process-copilot-suggestions + related-artifacts: + - .github/skills/dev/pr-reviews/process-copilot-suggestions/SKILL.md +--- + + + + +# PR #1967 Copilot Suggestions Tracking + +Source: Copilot PR review threads for https://github.com/torrust/torrust-tracker/pull/1967 + +Status legend: + +- `action`: code/docs change applied +- `no-action`: suggestion reviewed; no code change needed +- `resolved`: thread resolved in PR + +## Workflow + +1. Download all review threads (including resolved/outdated state and thread IDs). +2. Add one row per thread in the Suggestions table. +3. Process suggestions one by one: + - decide `action` or `no-action` + - if `action`, apply change and validate + - if needed, commit changes + - resolve the PR thread +4. Set `Thread State` to `resolved` once resolved in PR. + +## Processing Log + +- 2026-06-30: Started processing suggestions. +- 2026-06-30: Completed processing suggestions. + +## Suggestions + +| # | Thread ID | Path | URL | Suggestion Summary | Decision | Status | Thread State | +| --- | ----------------------- | ------------------------------------------------------------------------------------------ | -------------------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------- | ------------------------------ | ------ | ------------ | +| 1 | `PRRT_kwDOGp2yqc6NNrmf` | `docs/issues/open/1965-1669-si-34-consolidate-duplicate-http-types/ISSUE.md` | [Comment](https://github.com/torrust/torrust-tracker/pull/1967#discussion_r3497403152) | Relative link `../../.github/skills/...` is broken — needs 4 `..` segments from the nested folder | action — fix the relative link | DONE | RESOLVED | +| 2 | `PRRT_kwDOGp2yqc6NNrnB` | `docs/issues/open/1965-1669-si-34-consolidate-duplicate-http-types/manual-verification.md` | [Comment](https://github.com/torrust/torrust-tracker/pull/1967#discussion_r3497403199) | Missing YAML frontmatter for docs metadata consistency | action — add YAML frontmatter | DONE | RESOLVED | +| 3 | `PRRT_kwDOGp2yqc6NNrnc` | `docs/issues/open/1966-1669-si-35-consolidate-duplicate-udp-types.md` | [Comment](https://github.com/torrust/torrust-tracker/pull/1967#discussion_r3497403232) | AC numbering duplicates AC5 and skips AC7 — renumber to align with table | action — fix AC numbering | DONE | RESOLVED | From 078dce7b223fb97fa40df9fdce4dee270e897bb5 Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Tue, 30 Jun 2026 09:27:46 +0100 Subject: [PATCH 03/10] refactor(rest-api-client): align client with rest-api-protocol and add typed ApiClient --- Cargo.lock | 1 + .../1944-1938-si-6-align-rest-api-client.md | 32 ++- .../server/v1/contract/authentication.rs | 28 +-- .../server/v1/contract/context/auth_key.rs | 56 ++--- .../tests/server/v1/contract/context/stats.rs | 8 +- .../server/v1/contract/context/torrent.rs | 32 +-- .../server/v1/contract/context/whitelist.rs | 34 +-- packages/rest-api-client/Cargo.toml | 1 + packages/rest-api-client/src/v1/client.rs | 211 +++++++++++++++++- packages/rest-api-client/src/v1/mod.rs | 2 + .../ci/qbittorrent_e2e/tracker/client.rs | 10 +- tests/servers/api/contract/stats/mod.rs | 2 +- 12 files changed, 309 insertions(+), 108 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index bad5a7a4a..7c027819e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5221,6 +5221,7 @@ dependencies = [ "reqwest", "serde", "thiserror 2.0.18", + "torrust-tracker-rest-api-protocol", "url", "uuid", ] diff --git a/docs/issues/open/1944-1938-si-6-align-rest-api-client.md b/docs/issues/open/1944-1938-si-6-align-rest-api-client.md index 3d4de21ce..e867c5d60 100644 --- a/docs/issues/open/1944-1938-si-6-align-rest-api-client.md +++ b/docs/issues/open/1944-1938-si-6-align-rest-api-client.md @@ -24,6 +24,16 @@ semantic-links: ## Subissue of REST API Contract-First Migration EPIC +## Clarifying Decisions (from AI agent Q&A with user) + +- **`AddKeyForm`**: Use the protocol package's `AddKeyForm` (with field `opt_seconds_valid`) and remove the local `AddKeyForm` from client. +- **`ClientError` enum variants**: + - `TransportError(reqwest::Error)` — network/connection failures + - `ApiError { status: StatusCode, body: String }` — non-2xx responses with the error body + - `DeserializationError(reqwest::Error)` — JSON parsing failures +- **Public `get()` function**: Keep as a public free function (used by health_check tests directly). +- **Re-export strategy**: Re-export both `ApiClient` and `ApiHttpClient` from the crate root for ergonomics. + ## Problem The REST API client package (`torrust-tracker-rest-api-client`) currently exposes only a **low-level** `Client` struct where all 10 methods return raw `reqwest::Response` values. Callers must manually deserialize responses and handle errors. Some internal methods use `.unwrap()`, panicking on transport errors. @@ -175,21 +185,21 @@ impl ApiClient { | ID | Status | Task | Notes | | --- | ------ | ------------------------------------------------------------ | ------------------------------------------------ | -| T1 | TODO | Rename `Client` → `ApiHttpClient` in `client.rs` | Compiler catches all references | -| T2 | TODO | Add `rest-api-protocol` to `rest-api-client/Cargo.toml` | | -| T3 | TODO | Define `ClientError` enum | Wraps reqwest error, deserialization, API errors | -| T4 | TODO | Add `ApiClient` struct before `ApiHttpClient` in `client.rs` | New high-level typed client | -| T5 | TODO | Implement typed methods on `ApiClient` for all endpoints | Returns `Result` | +| T1 | DONE | Rename `Client` → `ApiHttpClient` in `client.rs` | Compiler catches all references | +| T2 | DONE | Add `rest-api-protocol` to `rest-api-client/Cargo.toml` | | +| T3 | DONE | Define `ClientError` enum | Wraps reqwest error, deserialization, API errors | +| T4 | DONE | Add `ApiClient` struct before `ApiHttpClient` in `client.rs` | New high-level typed client | +| T5 | DONE | Implement typed methods on `ApiClient` for all endpoints | Returns `Result` | | T6 | TODO | Verify pre-commit and pre-push checks pass | | ## Verification / Progress -- [ ] `Client` renamed to `ApiHttpClient` across the codebase -- [ ] `rest-api-protocol` added as dependency -- [ ] `ClientError` enum defined -- [ ] `ApiClient` struct with typed methods for all endpoints added -- [ ] `ApiClient` appears before `ApiHttpClient` in `client.rs` -- [ ] All existing tests pass unchanged +- [x] `Client` renamed to `ApiHttpClient` across the codebase +- [x] `rest-api-protocol` added as dependency +- [x] `ClientError` enum defined +- [x] `ApiClient` struct with typed methods for all endpoints added +- [x] `ApiClient` appears before `ApiHttpClient` in `client.rs` +- [x] All existing tests pass unchanged - [ ] Pre-commit checks pass - [ ] Pre-push checks pass diff --git a/packages/axum-rest-api-server/tests/server/v1/contract/authentication.rs b/packages/axum-rest-api-server/tests/server/v1/contract/authentication.rs index 24cc4193e..e9fc8d396 100644 --- a/packages/axum-rest-api-server/tests/server/v1/contract/authentication.rs +++ b/packages/axum-rest-api-server/tests/server/v1/contract/authentication.rs @@ -4,7 +4,7 @@ mod given_that_the_token_is_only_provided_in_the_authentication_header { use torrust_tracker_rest_api_client::common::http::Query; use torrust_tracker_rest_api_client::connection_info::ConnectionInfo; use torrust_tracker_rest_api_client::v1::client::{ - AUTH_BEARER_TOKEN_HEADER_PREFIX, Client, headers_with_auth_token, headers_with_request_id, + AUTH_BEARER_TOKEN_HEADER_PREFIX, ApiHttpClient, headers_with_auth_token, headers_with_request_id, }; use torrust_tracker_test_helpers::logging::logs_contains_a_line_with; use torrust_tracker_test_helpers::{configuration, logging}; @@ -20,7 +20,7 @@ mod given_that_the_token_is_only_provided_in_the_authentication_header { let token = env.get_connection_info().api_token.unwrap(); - let response = Client::new(env.get_connection_info()) + let response = ApiHttpClient::new(env.get_connection_info()) .unwrap() .get_request_with_query("stats", Query::default(), Some(headers_with_auth_token(&token))) .await; @@ -48,7 +48,7 @@ mod given_that_the_token_is_only_provided_in_the_authentication_header { .expect("the auth token is not a valid header value"), ); - let response = Client::new(env.get_connection_info()) + let response = ApiHttpClient::new(env.get_connection_info()) .unwrap() .get_request_with_query("stats", Query::default(), Some(headers)) .await; @@ -83,7 +83,7 @@ mod given_that_the_token_is_only_provided_in_the_authentication_header { let connection_info = ConnectionInfo::anonymous(env.get_connection_info().origin); - let response = Client::new(connection_info) + let response = ApiHttpClient::new(connection_info) .unwrap() .get_request_with_query("stats", Query::default(), Some(headers)) .await; @@ -103,7 +103,7 @@ mod given_that_the_token_is_only_provided_in_the_query_param { use torrust_tracker_axum_rest_api_server::testing::environment::Started; use torrust_tracker_rest_api_client::common::http::{Query, QueryParam}; use torrust_tracker_rest_api_client::connection_info::ConnectionInfo; - use torrust_tracker_rest_api_client::v1::client::{Client, TOKEN_PARAM_NAME, headers_with_request_id}; + use torrust_tracker_rest_api_client::v1::client::{ApiHttpClient, TOKEN_PARAM_NAME, headers_with_request_id}; use torrust_tracker_test_helpers::logging::logs_contains_a_line_with; use torrust_tracker_test_helpers::{configuration, logging}; use uuid::Uuid; @@ -120,7 +120,7 @@ mod given_that_the_token_is_only_provided_in_the_query_param { let connection_info = ConnectionInfo::anonymous(env.get_connection_info().origin); - let response = Client::new(connection_info) + let response = ApiHttpClient::new(connection_info) .unwrap() .get_request_with_query( "stats", @@ -144,7 +144,7 @@ mod given_that_the_token_is_only_provided_in_the_query_param { let connection_info = ConnectionInfo::anonymous(env.get_connection_info().origin); - let response = Client::new(connection_info) + let response = ApiHttpClient::new(connection_info) .unwrap() .get_request_with_query( "stats", @@ -173,7 +173,7 @@ mod given_that_the_token_is_only_provided_in_the_query_param { let connection_info = ConnectionInfo::anonymous(env.get_connection_info().origin); - let response = Client::new(connection_info) + let response = ApiHttpClient::new(connection_info) .unwrap() .get_request_with_query( "stats", @@ -203,7 +203,7 @@ mod given_that_the_token_is_only_provided_in_the_query_param { let connection_info = ConnectionInfo::anonymous(env.get_connection_info().origin); // At the beginning of the query component - let response = Client::new(connection_info) + let response = ApiHttpClient::new(connection_info) .unwrap() .get_request(&format!("torrents?token={token}&limit=1")) .await; @@ -211,7 +211,7 @@ mod given_that_the_token_is_only_provided_in_the_query_param { assert_eq!(response.status(), 200); // At the end of the query component - let response = Client::new(env.get_connection_info()) + let response = ApiHttpClient::new(env.get_connection_info()) .unwrap() .get_request(&format!("torrents?limit=1&token={token}")) .await; @@ -227,7 +227,7 @@ mod given_that_not_token_is_provided { use torrust_tracker_axum_rest_api_server::testing::environment::Started; use torrust_tracker_rest_api_client::common::http::Query; use torrust_tracker_rest_api_client::connection_info::ConnectionInfo; - use torrust_tracker_rest_api_client::v1::client::{Client, headers_with_request_id}; + use torrust_tracker_rest_api_client::v1::client::{ApiHttpClient, headers_with_request_id}; use torrust_tracker_test_helpers::logging::logs_contains_a_line_with; use torrust_tracker_test_helpers::{configuration, logging}; use uuid::Uuid; @@ -244,7 +244,7 @@ mod given_that_not_token_is_provided { let connection_info = ConnectionInfo::anonymous(env.get_connection_info().origin); - let response = Client::new(connection_info) + let response = ApiHttpClient::new(connection_info) .unwrap() .get_request_with_query("stats", Query::default(), Some(headers_with_request_id(request_id))) .await; @@ -263,7 +263,7 @@ mod given_that_not_token_is_provided { mod given_that_token_is_provided_via_get_param_and_authentication_header { use torrust_tracker_axum_rest_api_server::testing::environment::Started; use torrust_tracker_rest_api_client::common::http::{Query, QueryParam}; - use torrust_tracker_rest_api_client::v1::client::{Client, TOKEN_PARAM_NAME, headers_with_auth_token}; + use torrust_tracker_rest_api_client::v1::client::{ApiHttpClient, TOKEN_PARAM_NAME, headers_with_auth_token}; use torrust_tracker_test_helpers::{configuration, logging}; #[tokio::test] @@ -276,7 +276,7 @@ mod given_that_token_is_provided_via_get_param_and_authentication_header { let non_authorized_token = "NonAuthorizedToken"; - let response = Client::new(env.get_connection_info()) + let response = ApiHttpClient::new(env.get_connection_info()) .unwrap() .get_request_with_query( "stats", diff --git a/packages/axum-rest-api-server/tests/server/v1/contract/context/auth_key.rs b/packages/axum-rest-api-server/tests/server/v1/contract/context/auth_key.rs index c9dba6bfb..36c1d7074 100644 --- a/packages/axum-rest-api-server/tests/server/v1/contract/context/auth_key.rs +++ b/packages/axum-rest-api-server/tests/server/v1/contract/context/auth_key.rs @@ -3,7 +3,7 @@ use std::time::Duration; use serde::Serialize; use torrust_tracker_axum_rest_api_server::testing::environment::Started; use torrust_tracker_core::authentication::Key; -use torrust_tracker_rest_api_client::v1::client::{AddKeyForm, Client, headers_with_request_id}; +use torrust_tracker_rest_api_client::v1::client::{AddKeyForm, ApiHttpClient, headers_with_request_id}; use torrust_tracker_test_helpers::logging::logs_contains_a_line_with; use torrust_tracker_test_helpers::{configuration, logging}; use uuid::Uuid; @@ -24,12 +24,12 @@ async fn should_allow_generating_a_new_random_auth_key() { let request_id = Uuid::new_v4(); - let response = Client::new(env.get_connection_info()) + let response = ApiHttpClient::new(env.get_connection_info()) .unwrap() .add_auth_key( AddKeyForm { opt_key: None, - seconds_valid: Some(60), + opt_seconds_valid: Some(60), }, Some(headers_with_request_id(request_id)), ) @@ -57,12 +57,12 @@ async fn should_allow_uploading_a_preexisting_auth_key() { let request_id = Uuid::new_v4(); - let response = Client::new(env.get_connection_info()) + let response = ApiHttpClient::new(env.get_connection_info()) .unwrap() .add_auth_key( AddKeyForm { opt_key: Some("Xc1L4PbQJSFGlrgSRZl8wxSFAuMa21z5".to_string()), - seconds_valid: Some(60), + opt_seconds_valid: Some(60), }, Some(headers_with_request_id(request_id)), ) @@ -90,12 +90,12 @@ async fn should_not_allow_generating_a_new_auth_key_for_unauthenticated_users() let request_id = Uuid::new_v4(); - let response = Client::new(connection_with_invalid_token(env.get_connection_info().origin)) + let response = ApiHttpClient::new(connection_with_invalid_token(env.get_connection_info().origin)) .unwrap() .add_auth_key( AddKeyForm { opt_key: None, - seconds_valid: Some(60), + opt_seconds_valid: Some(60), }, Some(headers_with_request_id(request_id)), ) @@ -110,12 +110,12 @@ async fn should_not_allow_generating_a_new_auth_key_for_unauthenticated_users() let request_id = Uuid::new_v4(); - let response = Client::new(connection_with_no_token(env.get_connection_info().origin)) + let response = ApiHttpClient::new(connection_with_no_token(env.get_connection_info().origin)) .unwrap() .add_auth_key( AddKeyForm { opt_key: None, - seconds_valid: Some(60), + opt_seconds_valid: Some(60), }, Some(headers_with_request_id(request_id)), ) @@ -141,12 +141,12 @@ async fn should_fail_when_the_auth_key_cannot_be_generated() { let request_id = Uuid::new_v4(); - let response = Client::new(env.get_connection_info()) + let response = ApiHttpClient::new(env.get_connection_info()) .unwrap() .add_auth_key( AddKeyForm { opt_key: None, - seconds_valid: Some(60), + opt_seconds_valid: Some(60), }, Some(headers_with_request_id(request_id)), ) @@ -179,7 +179,7 @@ async fn should_allow_deleting_an_auth_key() { let request_id = Uuid::new_v4(); - let response = Client::new(env.get_connection_info()) + let response = ApiHttpClient::new(env.get_connection_info()) .unwrap() .delete_auth_key(&auth_key.key.to_string(), Some(headers_with_request_id(request_id))) .await; @@ -214,7 +214,7 @@ async fn should_fail_generating_a_new_auth_key_when_the_provided_key_is_invalid( for invalid_key in invalid_keys { let request_id = Uuid::new_v4(); - let response = Client::new(env.get_connection_info()) + let response = ApiHttpClient::new(env.get_connection_info()) .unwrap() .post_form( "keys", @@ -254,7 +254,7 @@ async fn should_fail_generating_a_new_auth_key_when_the_key_duration_is_invalid( for invalid_key_duration in invalid_key_durations { let request_id = Uuid::new_v4(); - let response = Client::new(env.get_connection_info()) + let response = ApiHttpClient::new(env.get_connection_info()) .unwrap() .post_form( "keys", @@ -291,7 +291,7 @@ async fn should_fail_deleting_an_auth_key_when_the_key_id_is_invalid() { for invalid_auth_key in &invalid_auth_keys { let request_id = Uuid::new_v4(); - let response = Client::new(env.get_connection_info()) + let response = ApiHttpClient::new(env.get_connection_info()) .unwrap() .delete_auth_key(invalid_auth_key, Some(headers_with_request_id(request_id))) .await; @@ -321,7 +321,7 @@ async fn should_fail_when_the_auth_key_cannot_be_deleted() { let request_id = Uuid::new_v4(); - let response = Client::new(env.get_connection_info()) + let response = ApiHttpClient::new(env.get_connection_info()) .unwrap() .delete_auth_key(&auth_key.key.to_string(), Some(headers_with_request_id(request_id))) .await; @@ -355,7 +355,7 @@ async fn should_not_allow_deleting_an_auth_key_for_unauthenticated_users() { let request_id = Uuid::new_v4(); - let response = Client::new(connection_with_invalid_token(env.get_connection_info().origin)) + let response = ApiHttpClient::new(connection_with_invalid_token(env.get_connection_info().origin)) .unwrap() .delete_auth_key(&auth_key.key.to_string(), Some(headers_with_request_id(request_id))) .await; @@ -378,7 +378,7 @@ async fn should_not_allow_deleting_an_auth_key_for_unauthenticated_users() { let request_id = Uuid::new_v4(); - let response = Client::new(connection_with_no_token(env.get_connection_info().origin)) + let response = ApiHttpClient::new(connection_with_no_token(env.get_connection_info().origin)) .unwrap() .delete_auth_key(&auth_key.key.to_string(), Some(headers_with_request_id(request_id))) .await; @@ -409,7 +409,7 @@ async fn should_allow_reloading_keys() { let request_id = Uuid::new_v4(); - let response = Client::new(env.get_connection_info()) + let response = ApiHttpClient::new(env.get_connection_info()) .unwrap() .reload_keys(Some(headers_with_request_id(request_id))) .await; @@ -437,7 +437,7 @@ async fn should_fail_when_keys_cannot_be_reloaded() { force_database_error(&env.container.tracker_core_container.database_stores.schema_migrator).await; - let response = Client::new(env.get_connection_info()) + let response = ApiHttpClient::new(env.get_connection_info()) .unwrap() .reload_keys(Some(headers_with_request_id(request_id))) .await; @@ -468,7 +468,7 @@ async fn should_not_allow_reloading_keys_for_unauthenticated_users() { let request_id = Uuid::new_v4(); - let response = Client::new(connection_with_invalid_token(env.get_connection_info().origin)) + let response = ApiHttpClient::new(connection_with_invalid_token(env.get_connection_info().origin)) .unwrap() .reload_keys(Some(headers_with_request_id(request_id))) .await; @@ -482,7 +482,7 @@ async fn should_not_allow_reloading_keys_for_unauthenticated_users() { let request_id = Uuid::new_v4(); - let response = Client::new(connection_with_no_token(env.get_connection_info().origin)) + let response = ApiHttpClient::new(connection_with_no_token(env.get_connection_info().origin)) .unwrap() .reload_keys(Some(headers_with_request_id(request_id))) .await; @@ -501,7 +501,7 @@ mod deprecated_generate_key_endpoint { use torrust_tracker_axum_rest_api_server::testing::environment::Started; use torrust_tracker_core::authentication::Key; - use torrust_tracker_rest_api_client::v1::client::{Client, headers_with_request_id}; + use torrust_tracker_rest_api_client::v1::client::{ApiHttpClient, headers_with_request_id}; use torrust_tracker_test_helpers::logging::logs_contains_a_line_with; use torrust_tracker_test_helpers::{configuration, logging}; use uuid::Uuid; @@ -521,7 +521,7 @@ mod deprecated_generate_key_endpoint { let seconds_valid = 60; - let response = Client::new(env.get_connection_info()) + let response = ApiHttpClient::new(env.get_connection_info()) .unwrap() .generate_auth_key(seconds_valid, None) .await; @@ -549,14 +549,14 @@ mod deprecated_generate_key_endpoint { let request_id = Uuid::new_v4(); let seconds_valid = 60; - let response = Client::new(connection_with_invalid_token(env.get_connection_info().origin)) + let response = ApiHttpClient::new(connection_with_invalid_token(env.get_connection_info().origin)) .unwrap() .generate_auth_key(seconds_valid, Some(headers_with_request_id(request_id))) .await; assert_token_not_valid(response).await; - let response = Client::new(connection_with_no_token(env.get_connection_info().origin)) + let response = ApiHttpClient::new(connection_with_no_token(env.get_connection_info().origin)) .unwrap() .generate_auth_key(seconds_valid, None) .await; @@ -584,7 +584,7 @@ mod deprecated_generate_key_endpoint { ]; for invalid_key_duration in invalid_key_durations { - let response = Client::new(env.get_connection_info()) + let response = ApiHttpClient::new(env.get_connection_info()) .unwrap() .post_empty(&format!("key/{invalid_key_duration}"), None) .await; @@ -605,7 +605,7 @@ mod deprecated_generate_key_endpoint { let request_id = Uuid::new_v4(); let seconds_valid = 60; - let response = Client::new(env.get_connection_info()) + let response = ApiHttpClient::new(env.get_connection_info()) .unwrap() .generate_auth_key(seconds_valid, Some(headers_with_request_id(request_id))) .await; diff --git a/packages/axum-rest-api-server/tests/server/v1/contract/context/stats.rs b/packages/axum-rest-api-server/tests/server/v1/contract/context/stats.rs index 9d24087ee..610457b18 100644 --- a/packages/axum-rest-api-server/tests/server/v1/contract/context/stats.rs +++ b/packages/axum-rest-api-server/tests/server/v1/contract/context/stats.rs @@ -3,7 +3,7 @@ use std::str::FromStr; use torrust_info_hash::InfoHash; use torrust_tracker_axum_rest_api_server::testing::environment::Started; use torrust_tracker_primitives::peer::fixture::PeerBuilder; -use torrust_tracker_rest_api_client::v1::client::{Client, headers_with_request_id}; +use torrust_tracker_rest_api_client::v1::client::{ApiHttpClient, headers_with_request_id}; use torrust_tracker_rest_api_protocol::v1::context::stats::resources::stats::Stats; use torrust_tracker_test_helpers::logging::logs_contains_a_line_with; use torrust_tracker_test_helpers::{configuration, logging}; @@ -26,7 +26,7 @@ async fn should_allow_getting_tracker_statistics() { let request_id = Uuid::new_v4(); - let response = Client::new(env.get_connection_info()) + let response = ApiHttpClient::new(env.get_connection_info()) .unwrap() .get_tracker_statistics(Some(headers_with_request_id(request_id))) .await; @@ -81,7 +81,7 @@ async fn should_not_allow_getting_tracker_statistics_for_unauthenticated_users() let request_id = Uuid::new_v4(); - let response = Client::new(connection_with_invalid_token(env.get_connection_info().origin)) + let response = ApiHttpClient::new(connection_with_invalid_token(env.get_connection_info().origin)) .unwrap() .get_tracker_statistics(Some(headers_with_request_id(request_id))) .await; @@ -95,7 +95,7 @@ async fn should_not_allow_getting_tracker_statistics_for_unauthenticated_users() let request_id = Uuid::new_v4(); - let response = Client::new(connection_with_no_token(env.get_connection_info().origin)) + let response = ApiHttpClient::new(connection_with_no_token(env.get_connection_info().origin)) .unwrap() .get_tracker_statistics(Some(headers_with_request_id(request_id))) .await; diff --git a/packages/axum-rest-api-server/tests/server/v1/contract/context/torrent.rs b/packages/axum-rest-api-server/tests/server/v1/contract/context/torrent.rs index d46069c96..6d3166d00 100644 --- a/packages/axum-rest-api-server/tests/server/v1/contract/context/torrent.rs +++ b/packages/axum-rest-api-server/tests/server/v1/contract/context/torrent.rs @@ -4,7 +4,7 @@ use torrust_info_hash::InfoHash; use torrust_tracker_axum_rest_api_server::testing::environment::Started; use torrust_tracker_primitives::peer::fixture::PeerBuilder; use torrust_tracker_rest_api_client::common::http::{Query, QueryParam}; -use torrust_tracker_rest_api_client::v1::client::{Client, headers_with_request_id}; +use torrust_tracker_rest_api_client::v1::client::{ApiHttpClient, headers_with_request_id}; use torrust_tracker_rest_api_protocol::v1::context::torrent::resources::torrent::{self, Torrent}; use torrust_tracker_rest_api_runtime_adapter::v1::conversion; use torrust_tracker_test_helpers::logging::logs_contains_a_line_with; @@ -30,7 +30,7 @@ async fn should_allow_getting_all_torrents() { let request_id = Uuid::new_v4(); - let response = Client::new(env.get_connection_info()) + let response = ApiHttpClient::new(env.get_connection_info()) .unwrap() .get_torrents(Query::empty(), Some(headers_with_request_id(request_id))) .await; @@ -64,7 +64,7 @@ async fn should_allow_limiting_the_torrents_in_the_result() { let request_id = Uuid::new_v4(); - let response = Client::new(env.get_connection_info()) + let response = ApiHttpClient::new(env.get_connection_info()) .unwrap() .get_torrents( Query::params([QueryParam::new("limit", "1")].to_vec()), @@ -101,7 +101,7 @@ async fn should_allow_the_torrents_result_pagination() { let request_id = Uuid::new_v4(); - let response = Client::new(env.get_connection_info()) + let response = ApiHttpClient::new(env.get_connection_info()) .unwrap() .get_torrents( Query::params([QueryParam::new("offset", "1")].to_vec()), @@ -137,7 +137,7 @@ async fn should_allow_getting_a_list_of_torrents_providing_infohashes() { let request_id = Uuid::new_v4(); - let response = Client::new(env.get_connection_info()) + let response = ApiHttpClient::new(env.get_connection_info()) .unwrap() .get_torrents( Query::params( @@ -184,7 +184,7 @@ async fn should_fail_getting_torrents_when_the_offset_query_parameter_cannot_be_ for invalid_offset in &invalid_offsets { let request_id = Uuid::new_v4(); - let response = Client::new(env.get_connection_info()) + let response = ApiHttpClient::new(env.get_connection_info()) .unwrap() .get_torrents( Query::params([QueryParam::new("offset", invalid_offset)].to_vec()), @@ -213,7 +213,7 @@ async fn should_fail_getting_torrents_when_the_limit_query_parameter_cannot_be_p for invalid_limit in &invalid_limits { let request_id = Uuid::new_v4(); - let response = Client::new(env.get_connection_info()) + let response = ApiHttpClient::new(env.get_connection_info()) .unwrap() .get_torrents( Query::params([QueryParam::new("limit", invalid_limit)].to_vec()), @@ -242,7 +242,7 @@ async fn should_fail_getting_torrents_when_the_info_hash_parameter_is_invalid() for invalid_info_hash in &invalid_info_hashes { let request_id = Uuid::new_v4(); - let response = Client::new(env.get_connection_info()) + let response = ApiHttpClient::new(env.get_connection_info()) .unwrap() .get_torrents( Query::params([QueryParam::new("info_hash", invalid_info_hash)].to_vec()), @@ -268,7 +268,7 @@ async fn should_not_allow_getting_torrents_for_unauthenticated_users() { let request_id = Uuid::new_v4(); - let response = Client::new(connection_with_invalid_token(env.get_connection_info().origin)) + let response = ApiHttpClient::new(connection_with_invalid_token(env.get_connection_info().origin)) .unwrap() .get_torrents(Query::empty(), Some(headers_with_request_id(request_id))) .await; @@ -282,7 +282,7 @@ async fn should_not_allow_getting_torrents_for_unauthenticated_users() { let request_id = Uuid::new_v4(); - let response = Client::new(connection_with_no_token(env.get_connection_info().origin)) + let response = ApiHttpClient::new(connection_with_no_token(env.get_connection_info().origin)) .unwrap() .get_torrents(Query::default(), Some(headers_with_request_id(request_id))) .await; @@ -311,7 +311,7 @@ async fn should_allow_getting_a_torrent_info() { let request_id = Uuid::new_v4(); - let response = Client::new(env.get_connection_info()) + let response = ApiHttpClient::new(env.get_connection_info()) .unwrap() .get_torrent(&info_hash.to_string(), Some(headers_with_request_id(request_id))) .await; @@ -340,7 +340,7 @@ async fn should_fail_while_getting_a_torrent_info_when_the_torrent_does_not_exis let request_id = Uuid::new_v4(); let info_hash = InfoHash::from_str("9e0217d0fa71c87332cd8bf9dbeabcb2c2cf3c4d").unwrap(); // DevSkim: ignore DS173237 - let response = Client::new(env.get_connection_info()) + let response = ApiHttpClient::new(env.get_connection_info()) .unwrap() .get_torrent(&info_hash.to_string(), Some(headers_with_request_id(request_id))) .await; @@ -359,7 +359,7 @@ async fn should_fail_getting_a_torrent_info_when_the_provided_infohash_is_invali for invalid_infohash in &invalid_infohashes_returning_bad_request() { let request_id = Uuid::new_v4(); - let response = Client::new(env.get_connection_info()) + let response = ApiHttpClient::new(env.get_connection_info()) .unwrap() .get_torrent(invalid_infohash, Some(headers_with_request_id(request_id))) .await; @@ -370,7 +370,7 @@ async fn should_fail_getting_a_torrent_info_when_the_provided_infohash_is_invali for invalid_infohash in &invalid_infohashes_returning_not_found() { let request_id = Uuid::new_v4(); - let response = Client::new(env.get_connection_info()) + let response = ApiHttpClient::new(env.get_connection_info()) .unwrap() .get_torrent(invalid_infohash, Some(headers_with_request_id(request_id))) .await; @@ -393,7 +393,7 @@ async fn should_not_allow_getting_a_torrent_info_for_unauthenticated_users() { let request_id = Uuid::new_v4(); - let response = Client::new(connection_with_invalid_token(env.get_connection_info().origin)) + let response = ApiHttpClient::new(connection_with_invalid_token(env.get_connection_info().origin)) .unwrap() .get_torrent(&info_hash.to_string(), Some(headers_with_request_id(request_id))) .await; @@ -407,7 +407,7 @@ async fn should_not_allow_getting_a_torrent_info_for_unauthenticated_users() { let request_id = Uuid::new_v4(); - let response = Client::new(connection_with_no_token(env.get_connection_info().origin)) + let response = ApiHttpClient::new(connection_with_no_token(env.get_connection_info().origin)) .unwrap() .get_torrent(&info_hash.to_string(), Some(headers_with_request_id(request_id))) .await; diff --git a/packages/axum-rest-api-server/tests/server/v1/contract/context/whitelist.rs b/packages/axum-rest-api-server/tests/server/v1/contract/context/whitelist.rs index 7e5298deb..baa075ac9 100644 --- a/packages/axum-rest-api-server/tests/server/v1/contract/context/whitelist.rs +++ b/packages/axum-rest-api-server/tests/server/v1/contract/context/whitelist.rs @@ -2,7 +2,7 @@ use std::str::FromStr; use torrust_info_hash::InfoHash; use torrust_tracker_axum_rest_api_server::testing::environment::Started; -use torrust_tracker_rest_api_client::v1::client::{Client, headers_with_request_id}; +use torrust_tracker_rest_api_client::v1::client::{ApiHttpClient, headers_with_request_id}; use torrust_tracker_test_helpers::logging::logs_contains_a_line_with; use torrust_tracker_test_helpers::{configuration, logging}; use uuid::Uuid; @@ -24,7 +24,7 @@ async fn should_allow_whitelisting_a_torrent() { let request_id = Uuid::new_v4(); let info_hash = "9e0217d0fa71c87332cd8bf9dbeabcb2c2cf3c4d".to_owned(); // DevSkim: ignore DS173237 - let response = Client::new(env.get_connection_info()) + let response = ApiHttpClient::new(env.get_connection_info()) .unwrap() .whitelist_a_torrent(&info_hash, Some(headers_with_request_id(request_id))) .await; @@ -49,7 +49,7 @@ async fn should_allow_whitelisting_a_torrent_that_has_been_already_whitelisted() let info_hash = "9e0217d0fa71c87332cd8bf9dbeabcb2c2cf3c4d".to_owned(); // DevSkim: ignore DS173237 - let api_client = Client::new(env.get_connection_info()).unwrap(); + let api_client = ApiHttpClient::new(env.get_connection_info()).unwrap(); let request_id = Uuid::new_v4(); @@ -78,7 +78,7 @@ async fn should_not_allow_whitelisting_a_torrent_for_unauthenticated_users() { let request_id = Uuid::new_v4(); - let response = Client::new(connection_with_invalid_token(env.get_connection_info().origin)) + let response = ApiHttpClient::new(connection_with_invalid_token(env.get_connection_info().origin)) .unwrap() .whitelist_a_torrent(&info_hash, Some(headers_with_request_id(request_id))) .await; @@ -92,7 +92,7 @@ async fn should_not_allow_whitelisting_a_torrent_for_unauthenticated_users() { let request_id = Uuid::new_v4(); - let response = Client::new(connection_with_no_token(env.get_connection_info().origin)) + let response = ApiHttpClient::new(connection_with_no_token(env.get_connection_info().origin)) .unwrap() .whitelist_a_torrent(&info_hash, Some(headers_with_request_id(request_id))) .await; @@ -119,7 +119,7 @@ async fn should_fail_when_the_torrent_cannot_be_whitelisted() { let request_id = Uuid::new_v4(); - let response = Client::new(env.get_connection_info()) + let response = ApiHttpClient::new(env.get_connection_info()) .unwrap() .whitelist_a_torrent(&info_hash, Some(headers_with_request_id(request_id))) .await; @@ -143,7 +143,7 @@ async fn should_fail_whitelisting_a_torrent_when_the_provided_infohash_is_invali let request_id = Uuid::new_v4(); for invalid_infohash in &invalid_infohashes_returning_bad_request() { - let response = Client::new(env.get_connection_info()) + let response = ApiHttpClient::new(env.get_connection_info()) .unwrap() .whitelist_a_torrent(invalid_infohash, Some(headers_with_request_id(request_id))) .await; @@ -154,7 +154,7 @@ async fn should_fail_whitelisting_a_torrent_when_the_provided_infohash_is_invali let request_id = Uuid::new_v4(); for invalid_infohash in &invalid_infohashes_returning_not_found() { - let response = Client::new(env.get_connection_info()) + let response = ApiHttpClient::new(env.get_connection_info()) .unwrap() .whitelist_a_torrent(invalid_infohash, Some(headers_with_request_id(request_id))) .await; @@ -183,7 +183,7 @@ async fn should_allow_removing_a_torrent_from_the_whitelist() { let request_id = Uuid::new_v4(); - let response = Client::new(env.get_connection_info()) + let response = ApiHttpClient::new(env.get_connection_info()) .unwrap() .remove_torrent_from_whitelist(&hash, Some(headers_with_request_id(request_id))) .await; @@ -210,7 +210,7 @@ async fn should_not_fail_trying_to_remove_a_non_whitelisted_torrent_from_the_whi let request_id = Uuid::new_v4(); - let response = Client::new(env.get_connection_info()) + let response = ApiHttpClient::new(env.get_connection_info()) .unwrap() .remove_torrent_from_whitelist(&non_whitelisted_torrent_hash, Some(headers_with_request_id(request_id))) .await; @@ -229,7 +229,7 @@ async fn should_fail_removing_a_torrent_from_the_whitelist_when_the_provided_inf for invalid_infohash in &invalid_infohashes_returning_bad_request() { let request_id = Uuid::new_v4(); - let response = Client::new(env.get_connection_info()) + let response = ApiHttpClient::new(env.get_connection_info()) .unwrap() .remove_torrent_from_whitelist(invalid_infohash, Some(headers_with_request_id(request_id))) .await; @@ -240,7 +240,7 @@ async fn should_fail_removing_a_torrent_from_the_whitelist_when_the_provided_inf for invalid_infohash in &invalid_infohashes_returning_not_found() { let request_id = Uuid::new_v4(); - let response = Client::new(env.get_connection_info()) + let response = ApiHttpClient::new(env.get_connection_info()) .unwrap() .remove_torrent_from_whitelist(invalid_infohash, Some(headers_with_request_id(request_id))) .await; @@ -270,7 +270,7 @@ async fn should_fail_when_the_torrent_cannot_be_removed_from_the_whitelist() { let request_id = Uuid::new_v4(); - let response = Client::new(env.get_connection_info()) + let response = ApiHttpClient::new(env.get_connection_info()) .unwrap() .remove_torrent_from_whitelist(&hash, Some(headers_with_request_id(request_id))) .await; @@ -303,7 +303,7 @@ async fn should_not_allow_removing_a_torrent_from_the_whitelist_for_unauthentica let request_id = Uuid::new_v4(); - let response = Client::new(connection_with_invalid_token(env.get_connection_info().origin)) + let response = ApiHttpClient::new(connection_with_invalid_token(env.get_connection_info().origin)) .unwrap() .remove_torrent_from_whitelist(&hash, Some(headers_with_request_id(request_id))) .await; @@ -324,7 +324,7 @@ async fn should_not_allow_removing_a_torrent_from_the_whitelist_for_unauthentica let request_id = Uuid::new_v4(); - let response = Client::new(connection_with_no_token(env.get_connection_info().origin)) + let response = ApiHttpClient::new(connection_with_no_token(env.get_connection_info().origin)) .unwrap() .remove_torrent_from_whitelist(&hash, Some(headers_with_request_id(request_id))) .await; @@ -357,7 +357,7 @@ async fn should_allow_reload_the_whitelist_from_the_database() { let request_id = Uuid::new_v4(); - let response = Client::new(env.get_connection_info()) + let response = ApiHttpClient::new(env.get_connection_info()) .unwrap() .reload_whitelist(Some(headers_with_request_id(request_id))) .await; @@ -396,7 +396,7 @@ async fn should_fail_when_the_whitelist_cannot_be_reloaded_from_the_database() { let request_id = Uuid::new_v4(); - let response = Client::new(env.get_connection_info()) + let response = ApiHttpClient::new(env.get_connection_info()) .unwrap() .reload_whitelist(Some(headers_with_request_id(request_id))) .await; diff --git a/packages/rest-api-client/Cargo.toml b/packages/rest-api-client/Cargo.toml index f57aea95d..31e012f20 100644 --- a/packages/rest-api-client/Cargo.toml +++ b/packages/rest-api-client/Cargo.toml @@ -19,5 +19,6 @@ hyper = "1" reqwest = { version = "0", features = [ "json", "query" ] } serde = { version = "1", features = [ "derive" ] } thiserror = "2" +torrust-tracker-rest-api-protocol = { version = "3.0.0-develop", path = "../rest-api-protocol" } url = { version = "2", features = [ "serde" ] } uuid = { version = "1", features = [ "v4" ] } diff --git a/packages/rest-api-client/src/v1/client.rs b/packages/rest-api-client/src/v1/client.rs index fadef6bac..5021953a7 100644 --- a/packages/rest-api-client/src/v1/client.rs +++ b/packages/rest-api-client/src/v1/client.rs @@ -1,8 +1,15 @@ use std::time::Duration; use hyper::{HeaderMap, header}; -use reqwest::{Error, Response}; +use reqwest::{Response, StatusCode}; use serde::Serialize; +use serde::de::DeserializeOwned; +use thiserror::Error; +// Re-export AddKeyForm from the protocol package for backwards compatibility. +pub use torrust_tracker_rest_api_protocol::v1::context::auth_key::forms::add_key_form::AddKeyForm; +use torrust_tracker_rest_api_protocol::v1::context::auth_key::resources::auth_key::AuthKey; +use torrust_tracker_rest_api_protocol::v1::context::stats::resources::stats::Stats; +use torrust_tracker_rest_api_protocol::v1::context::torrent::resources::torrent::{ListItem, Torrent}; use url::Url; use uuid::Uuid; @@ -15,19 +22,206 @@ pub const AUTH_BEARER_TOKEN_HEADER_PREFIX: &str = "Bearer"; const API_PATH: &str = "api/v1/"; const DEFAULT_REQUEST_TIMEOUT_IN_SECS: u64 = 5; -/// API Client +/// Error type for [`ApiClient`] operations. +#[derive(Debug, Error)] +pub enum ClientError { + /// A transport-level error (connection refused, timeout, DNS failure, etc.). + #[error("transport error: {0}")] + TransportError(#[source] reqwest::Error), + + /// The API returned a non-2xx status code. + #[error("API error: {status} - {body}")] + ApiError { + /// The HTTP status code returned by the API. + status: StatusCode, + /// The response body (error message). + body: String, + }, + + /// Failed to deserialize the API response body into the expected type. + #[error("deserialization error: {0}")] + DeserializationError(#[source] reqwest::Error), +} + +/// High-level typed client for the Torrust Tracker REST API. +/// +/// Wraps [`ApiHttpClient`] and returns protocol DTOs from `rest-api-protocol`. +/// Never panics — all errors are returned as [`ClientError`]. +pub struct ApiClient { + inner: ApiHttpClient, +} + +impl ApiClient { + /// Creates a new `ApiClient`. + /// + /// # Errors + /// + /// Returns [`ClientError::TransportError`] if the HTTP client cannot be built. + pub fn new(connection_info: ConnectionInfo) -> Result { + Ok(Self { + inner: ApiHttpClient::new(connection_info).map_err(ClientError::TransportError)?, + }) + } + + /// Returns a reference to the inner [`ApiHttpClient`] for low-level operations. + #[must_use] + pub fn inner(&self) -> &ApiHttpClient { + &self.inner + } + + /// Generates a new random authentication key valid for `seconds_valid`. + /// + /// # Errors + /// + /// Returns [`ClientError::TransportError`] if the request fails. + /// Returns [`ClientError::ApiError`] if the API returns a non-2xx status. + /// Returns [`ClientError::DeserializationError`] if the response cannot be parsed. + pub async fn generate_auth_key(&self, seconds_valid: i32) -> Result { + let response = self.inner.generate_auth_key(seconds_valid, None).await; + Self::parse_response(response).await + } + + /// Adds a new authentication key using the provided form data. + /// + /// # Errors + /// + /// Returns [`ClientError::TransportError`] if the request fails. + /// Returns [`ClientError::ApiError`] if the API returns a non-2xx status. + /// Returns [`ClientError::DeserializationError`] if the response cannot be parsed. + pub async fn add_auth_key(&self, form: AddKeyForm) -> Result { + let response = self.inner.add_auth_key(form, None).await; + Self::parse_response(response).await + } + + /// Deletes an authentication key. + /// + /// # Errors + /// + /// Returns [`ClientError::TransportError`] if the request fails. + /// Returns [`ClientError::ApiError`] if the API returns a non-2xx status. + pub async fn delete_auth_key(&self, key: &str) -> Result<(), ClientError> { + let response = self.inner.delete_auth_key(key, None).await; + Self::check_success(response).await + } + + /// Reloads authentication keys from the database. + /// + /// # Errors + /// + /// Returns [`ClientError::TransportError`] if the request fails. + /// Returns [`ClientError::ApiError`] if the API returns a non-2xx status. + pub async fn reload_keys(&self) -> Result<(), ClientError> { + let response = self.inner.reload_keys(None).await; + Self::check_success(response).await + } + + /// Whitelists a torrent by info hash. + /// + /// # Errors + /// + /// Returns [`ClientError::TransportError`] if the request fails. + /// Returns [`ClientError::ApiError`] if the API returns a non-2xx status. + pub async fn whitelist_a_torrent(&self, info_hash: &str) -> Result<(), ClientError> { + let response = self.inner.whitelist_a_torrent(info_hash, None).await; + Self::check_success(response).await + } + + /// Removes a torrent from the whitelist. + /// + /// # Errors + /// + /// Returns [`ClientError::TransportError`] if the request fails. + /// Returns [`ClientError::ApiError`] if the API returns a non-2xx status. + pub async fn remove_torrent_from_whitelist(&self, info_hash: &str) -> Result<(), ClientError> { + let response = self.inner.remove_torrent_from_whitelist(info_hash, None).await; + Self::check_success(response).await + } + + /// Reloads the whitelist from the database. + /// + /// # Errors + /// + /// Returns [`ClientError::TransportError`] if the request fails. + /// Returns [`ClientError::ApiError`] if the API returns a non-2xx status. + pub async fn reload_whitelist(&self) -> Result<(), ClientError> { + let response = self.inner.reload_whitelist(None).await; + Self::check_success(response).await + } + + /// Gets a single torrent by info hash. + /// + /// # Errors + /// + /// Returns [`ClientError::TransportError`] if the request fails. + /// Returns [`ClientError::ApiError`] if the API returns a non-2xx status. + /// Returns [`ClientError::DeserializationError`] if the response cannot be parsed. + pub async fn get_torrent(&self, info_hash: &str) -> Result { + let response = self.inner.get_torrent(info_hash, None).await; + Self::parse_response(response).await + } + + /// Gets a list of torrents matching the query parameters. + /// + /// # Errors + /// + /// Returns [`ClientError::TransportError`] if the request fails. + /// Returns [`ClientError::ApiError`] if the API returns a non-2xx status. + /// Returns [`ClientError::DeserializationError`] if the response cannot be parsed. + pub async fn get_torrents(&self, params: Query) -> Result, ClientError> { + let response = self.inner.get_torrents(params, None).await; + Self::parse_response(response).await + } + + /// Gets tracker statistics. + /// + /// # Errors + /// + /// Returns [`ClientError::TransportError`] if the request fails. + /// Returns [`ClientError::ApiError`] if the API returns a non-2xx status. + /// Returns [`ClientError::DeserializationError`] if the response cannot be parsed. + pub async fn get_tracker_statistics(&self) -> Result { + let response = self.inner.get_tracker_statistics(None).await; + Self::parse_response(response).await + } + + /// Parses a successful response into the expected DTO type. + async fn parse_response(response: Response) -> Result { + let status = response.status(); + if !status.is_success() { + let body = response.text().await.map_err(ClientError::TransportError)?; + return Err(ClientError::ApiError { status, body }); + } + response.json::().await.map_err(ClientError::DeserializationError) + } + + /// Checks that the response has a 2xx status code, ignoring the body. + async fn check_success(response: Response) -> Result<(), ClientError> { + let status = response.status(); + if !status.is_success() { + let body = response.text().await.map_err(ClientError::TransportError)?; + return Err(ClientError::ApiError { status, body }); + } + Ok(()) + } +} + +/// Low-level HTTP transport for the Torrust Tracker REST API. +/// +/// Handles connection info, URL building, auth headers, and raw HTTP requests. +/// Returns [`reqwest::Response`] directly. For a typed high-level API, use +/// [`ApiClient`]. #[allow(clippy::struct_field_names)] -pub struct Client { +pub struct ApiHttpClient { connection_info: ConnectionInfo, base_path: String, http_client: reqwest::Client, } -impl Client { +impl ApiHttpClient { /// # Errors /// /// Will return an error if the HTTP client can't be created. - pub fn new(connection_info: ConnectionInfo) -> Result { + pub fn new(connection_info: ConnectionInfo) -> Result { let client = reqwest::Client::builder() .timeout(Duration::from_secs(DEFAULT_REQUEST_TIMEOUT_IN_SECS)) .build()?; @@ -256,10 +450,3 @@ pub fn headers_with_auth_token(token: &str) -> HeaderMap { ); headers } - -#[derive(Serialize, Debug)] -pub struct AddKeyForm { - #[serde(rename = "key")] - pub opt_key: Option, - pub seconds_valid: Option, -} diff --git a/packages/rest-api-client/src/v1/mod.rs b/packages/rest-api-client/src/v1/mod.rs index b9babe5bc..104437df8 100644 --- a/packages/rest-api-client/src/v1/mod.rs +++ b/packages/rest-api-client/src/v1/mod.rs @@ -1 +1,3 @@ pub mod client; + +pub use client::{ApiClient, ApiHttpClient}; diff --git a/src/console/ci/qbittorrent_e2e/tracker/client.rs b/src/console/ci/qbittorrent_e2e/tracker/client.rs index f517d0af5..afb802f4a 100644 --- a/src/console/ci/qbittorrent_e2e/tracker/client.rs +++ b/src/console/ci/qbittorrent_e2e/tracker/client.rs @@ -1,11 +1,11 @@ //! Tracker REST API client, scoped to E2E test needs. //! -//! Wraps the official [`torrust_tracker_rest_api_client::v1::Client`] so that +//! Wraps the official [`torrust_tracker_rest_api_client::v1::client::ApiHttpClient`] so that //! future scenario steps can call any REST API endpoint through the same client //! without having to reconstruct connection details each time. use anyhow::Context; use torrust_tracker_rest_api_client::connection_info::{ConnectionInfo, Origin}; -use torrust_tracker_rest_api_client::v1::client::Client; +use torrust_tracker_rest_api_client::v1::client::ApiHttpClient; use torrust_tracker_rest_api_protocol::v1::context::torrent::resources::torrent::Torrent; use super::super::types::InfoHash; @@ -14,9 +14,9 @@ use super::config_builder::TrackerConfig; /// Wrapper around the official Torrust Tracker REST API client. /// /// Provides typed, high-level helpers for the endpoints used in E2E test scenarios. -/// All other endpoints are still reachable through the inner [`Client`]. +/// All other endpoints are still reachable through the inner [`ApiHttpClient`]. pub(crate) struct TrackerApiClient { - inner: Client, + inner: ApiHttpClient, } impl TrackerApiClient { @@ -32,7 +32,7 @@ impl TrackerApiClient { let connection_info = ConnectionInfo::authenticated(origin, tracker_config.access_token()); - let inner = Client::new(connection_info).context("failed to build tracker REST API client")?; + let inner = ApiHttpClient::new(connection_info).context("failed to build tracker REST API client")?; Ok(Self { inner }) } diff --git a/tests/servers/api/contract/stats/mod.rs b/tests/servers/api/contract/stats/mod.rs index 6e1fe8c40..b77863d42 100644 --- a/tests/servers/api/contract/stats/mod.rs +++ b/tests/servers/api/contract/stats/mod.rs @@ -9,7 +9,7 @@ use torrust_tracker_client::http::client::Client as HttpTrackerClient; use torrust_tracker_client::http::client::requests::announce::QueryBuilder; use torrust_tracker_lib::app; use torrust_tracker_rest_api_client::connection_info::{ConnectionInfo, Origin}; -use torrust_tracker_rest_api_client::v1::client::Client as TrackerApiClient; +use torrust_tracker_rest_api_client::v1::client::ApiHttpClient as TrackerApiClient; #[tokio::test] async fn the_stats_api_endpoint_should_return_the_global_stats() { From aa0dc55864c47b2de4d868047e6727798cfd7da2 Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Tue, 30 Jun 2026 09:44:52 +0100 Subject: [PATCH 04/10] docs(rest-api-client): mark T6 complete and verification checklist finalised --- docs/issues/open/1944-1938-si-6-align-rest-api-client.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/issues/open/1944-1938-si-6-align-rest-api-client.md b/docs/issues/open/1944-1938-si-6-align-rest-api-client.md index e867c5d60..6373d395f 100644 --- a/docs/issues/open/1944-1938-si-6-align-rest-api-client.md +++ b/docs/issues/open/1944-1938-si-6-align-rest-api-client.md @@ -190,7 +190,7 @@ impl ApiClient { | T3 | DONE | Define `ClientError` enum | Wraps reqwest error, deserialization, API errors | | T4 | DONE | Add `ApiClient` struct before `ApiHttpClient` in `client.rs` | New high-level typed client | | T5 | DONE | Implement typed methods on `ApiClient` for all endpoints | Returns `Result` | -| T6 | TODO | Verify pre-commit and pre-push checks pass | | +| T6 | DONE | Verify pre-commit and pre-push checks pass | | ## Verification / Progress @@ -200,8 +200,8 @@ impl ApiClient { - [x] `ApiClient` struct with typed methods for all endpoints added - [x] `ApiClient` appears before `ApiHttpClient` in `client.rs` - [x] All existing tests pass unchanged -- [ ] Pre-commit checks pass -- [ ] Pre-push checks pass +- [x] Pre-commit checks pass +- [x] Pre-push checks pass ### Progress Log From 58e7ee0caaa1015b6d84603bdd899ea6e65ece5a Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Tue, 30 Jun 2026 10:16:30 +0100 Subject: [PATCH 05/10] fix(rest-api-client): ApiClient uses fallible transport methods to avoid panics ApiClient methods now call the fallible methods on ApiHttpClient and map failures to ClientError::TransportError, fulfilling the 'never panics' contract. Also made ClientError::ApiError variant fields implicitly public (since the enum itself is pub). --- packages/rest-api-client/src/v1/client.rs | 156 ++++++++++++++++++---- 1 file changed, 127 insertions(+), 29 deletions(-) diff --git a/packages/rest-api-client/src/v1/client.rs b/packages/rest-api-client/src/v1/client.rs index 5021953a7..6acf26a1f 100644 --- a/packages/rest-api-client/src/v1/client.rs +++ b/packages/rest-api-client/src/v1/client.rs @@ -77,7 +77,11 @@ impl ApiClient { /// Returns [`ClientError::ApiError`] if the API returns a non-2xx status. /// Returns [`ClientError::DeserializationError`] if the response cannot be parsed. pub async fn generate_auth_key(&self, seconds_valid: i32) -> Result { - let response = self.inner.generate_auth_key(seconds_valid, None).await; + let response = self + .inner + .post_empty_result(&format!("key/{seconds_valid}"), None) + .await + .map_err(ClientError::TransportError)?; Self::parse_response(response).await } @@ -89,7 +93,11 @@ impl ApiClient { /// Returns [`ClientError::ApiError`] if the API returns a non-2xx status. /// Returns [`ClientError::DeserializationError`] if the response cannot be parsed. pub async fn add_auth_key(&self, form: AddKeyForm) -> Result { - let response = self.inner.add_auth_key(form, None).await; + let response = self + .inner + .post_form_result("keys", &form, None) + .await + .map_err(ClientError::TransportError)?; Self::parse_response(response).await } @@ -100,7 +108,11 @@ impl ApiClient { /// Returns [`ClientError::TransportError`] if the request fails. /// Returns [`ClientError::ApiError`] if the API returns a non-2xx status. pub async fn delete_auth_key(&self, key: &str) -> Result<(), ClientError> { - let response = self.inner.delete_auth_key(key, None).await; + let response = self + .inner + .delete_result(&format!("key/{key}"), None) + .await + .map_err(ClientError::TransportError)?; Self::check_success(response).await } @@ -111,7 +123,11 @@ impl ApiClient { /// Returns [`ClientError::TransportError`] if the request fails. /// Returns [`ClientError::ApiError`] if the API returns a non-2xx status. pub async fn reload_keys(&self) -> Result<(), ClientError> { - let response = self.inner.reload_keys(None).await; + let response = self + .inner + .get_result("keys/reload", Query::default(), None) + .await + .map_err(ClientError::TransportError)?; Self::check_success(response).await } @@ -122,7 +138,11 @@ impl ApiClient { /// Returns [`ClientError::TransportError`] if the request fails. /// Returns [`ClientError::ApiError`] if the API returns a non-2xx status. pub async fn whitelist_a_torrent(&self, info_hash: &str) -> Result<(), ClientError> { - let response = self.inner.whitelist_a_torrent(info_hash, None).await; + let response = self + .inner + .post_empty_result(&format!("whitelist/{info_hash}"), None) + .await + .map_err(ClientError::TransportError)?; Self::check_success(response).await } @@ -133,7 +153,11 @@ impl ApiClient { /// Returns [`ClientError::TransportError`] if the request fails. /// Returns [`ClientError::ApiError`] if the API returns a non-2xx status. pub async fn remove_torrent_from_whitelist(&self, info_hash: &str) -> Result<(), ClientError> { - let response = self.inner.remove_torrent_from_whitelist(info_hash, None).await; + let response = self + .inner + .delete_result(&format!("whitelist/{info_hash}"), None) + .await + .map_err(ClientError::TransportError)?; Self::check_success(response).await } @@ -144,7 +168,11 @@ impl ApiClient { /// Returns [`ClientError::TransportError`] if the request fails. /// Returns [`ClientError::ApiError`] if the API returns a non-2xx status. pub async fn reload_whitelist(&self) -> Result<(), ClientError> { - let response = self.inner.reload_whitelist(None).await; + let response = self + .inner + .get_result("whitelist/reload", Query::default(), None) + .await + .map_err(ClientError::TransportError)?; Self::check_success(response).await } @@ -156,7 +184,11 @@ impl ApiClient { /// Returns [`ClientError::ApiError`] if the API returns a non-2xx status. /// Returns [`ClientError::DeserializationError`] if the response cannot be parsed. pub async fn get_torrent(&self, info_hash: &str) -> Result { - let response = self.inner.get_torrent(info_hash, None).await; + let response = self + .inner + .get_result(&format!("torrent/{info_hash}"), Query::default(), None) + .await + .map_err(ClientError::TransportError)?; Self::parse_response(response).await } @@ -168,7 +200,11 @@ impl ApiClient { /// Returns [`ClientError::ApiError`] if the API returns a non-2xx status. /// Returns [`ClientError::DeserializationError`] if the response cannot be parsed. pub async fn get_torrents(&self, params: Query) -> Result, ClientError> { - let response = self.inner.get_torrents(params, None).await; + let response = self + .inner + .get_result("torrents", params, None) + .await + .map_err(ClientError::TransportError)?; Self::parse_response(response).await } @@ -180,7 +216,11 @@ impl ApiClient { /// Returns [`ClientError::ApiError`] if the API returns a non-2xx status. /// Returns [`ClientError::DeserializationError`] if the response cannot be parsed. pub async fn get_tracker_statistics(&self) -> Result { - let response = self.inner.get_tracker_statistics(None).await; + let response = self + .inner + .get_result("stats", Query::default(), None) + .await + .map_err(ClientError::TransportError)?; Self::parse_response(response).await } @@ -234,59 +274,80 @@ impl ApiHttpClient { } pub async fn generate_auth_key(&self, seconds_valid: i32, headers: Option) -> Response { - self.post_empty(&format!("key/{seconds_valid}"), headers).await + self.post_empty_result(&format!("key/{seconds_valid}"), headers) + .await + .unwrap() } pub async fn add_auth_key(&self, add_key_form: AddKeyForm, headers: Option) -> Response { - self.post_form("keys", &add_key_form, headers).await + self.post_form_result("keys", &add_key_form, headers).await.unwrap() } pub async fn delete_auth_key(&self, key: &str, headers: Option) -> Response { - self.delete(&format!("key/{key}"), headers).await + self.delete_result(&format!("key/{key}"), headers).await.unwrap() } pub async fn reload_keys(&self, headers: Option) -> Response { - self.get("keys/reload", Query::default(), headers).await + self.get_result("keys/reload", Query::default(), headers).await.unwrap() } pub async fn whitelist_a_torrent(&self, info_hash: &str, headers: Option) -> Response { - self.post_empty(&format!("whitelist/{info_hash}"), headers).await + self.post_empty_result(&format!("whitelist/{info_hash}"), headers) + .await + .unwrap() } pub async fn remove_torrent_from_whitelist(&self, info_hash: &str, headers: Option) -> Response { - self.delete(&format!("whitelist/{info_hash}"), headers).await + self.delete_result(&format!("whitelist/{info_hash}"), headers).await.unwrap() } pub async fn reload_whitelist(&self, headers: Option) -> Response { - self.get("whitelist/reload", Query::default(), headers).await + self.get_result("whitelist/reload", Query::default(), headers).await.unwrap() } pub async fn get_torrent(&self, info_hash: &str, headers: Option) -> Response { - self.get(&format!("torrent/{info_hash}"), Query::default(), headers).await + self.get_result(&format!("torrent/{info_hash}"), Query::default(), headers) + .await + .unwrap() } pub async fn get_torrents(&self, params: Query, headers: Option) -> Response { - self.get("torrents", params, headers).await + self.get_result("torrents", params, headers).await.unwrap() } pub async fn get_tracker_statistics(&self, headers: Option) -> Response { - self.get("stats", Query::default(), headers).await + self.get_result("stats", Query::default(), headers).await.unwrap() } pub async fn get(&self, path: &str, params: Query, headers: Option) -> Response { + self.get_result(path, params, headers).await.unwrap() + } + + /// Fallible version of [`Self::get`] that returns a `Result` instead of panicking. + pub(crate) async fn get_result( + &self, + path: &str, + params: Query, + headers: Option, + ) -> Result { let mut query: Query = params; if let Some(token) = &self.connection_info.api_token { query.add_param(QueryParam::new(TOKEN_PARAM_NAME, token)); } - self.get_request_with_query(path, query, headers).await + self.get_request_with_query_result(path, query, headers).await } /// # Panics /// /// Will panic if the request can't be sent pub async fn post_empty(&self, path: &str, headers: Option) -> Response { + self.post_empty_result(path, headers).await.unwrap() + } + + /// Fallible version of [`Self::post_empty`] that returns a `Result` instead of panicking. + pub(crate) async fn post_empty_result(&self, path: &str, headers: Option) -> Result { let builder = self.http_client.post(self.base_url(path).clone()); let builder = match headers { @@ -299,13 +360,23 @@ impl ApiHttpClient { None => builder, }; - builder.send().await.unwrap() + builder.send().await } /// # Panics /// /// Will panic if the request can't be sent pub async fn post_form(&self, path: &str, form: &T, headers: Option) -> Response { + self.post_form_result(path, form, headers).await.unwrap() + } + + /// Fallible version of [`Self::post_form`] that returns a `Result` instead of panicking. + pub(crate) async fn post_form_result( + &self, + path: &str, + form: &T, + headers: Option, + ) -> Result { let builder = self.http_client.post(self.base_url(path).clone()).json(&form); let builder = match headers { @@ -318,13 +389,18 @@ impl ApiHttpClient { None => builder, }; - builder.send().await.unwrap() + builder.send().await } /// # Panics /// /// Will panic if the request can't be sent async fn delete(&self, path: &str, headers: Option) -> Response { + self.delete_result(path, headers).await.unwrap() + } + + /// Fallible version of [`Self::delete`] that returns a `Result` instead of panicking. + async fn delete_result(&self, path: &str, headers: Option) -> Result { let builder = self.http_client.delete(self.base_url(path).clone()); let builder = match headers { @@ -337,13 +413,23 @@ impl ApiHttpClient { None => builder, }; - builder.send().await.unwrap() + builder.send().await } /// # Panics /// /// Will panic if it can't convert the authentication token to a `HeaderValue`. pub async fn get_request_with_query(&self, path: &str, params: Query, headers: Option) -> Response { + self.get_request_with_query_result(path, params, headers).await.unwrap() + } + + /// Fallible version of [`Self::get_request_with_query`] that returns a `Result` instead of panicking. + pub(crate) async fn get_request_with_query_result( + &self, + path: &str, + params: Query, + headers: Option, + ) -> Result { match &self.connection_info.api_token { Some(token) => { let headers = if let Some(headers) = headers { @@ -379,16 +465,24 @@ impl ApiHttpClient { headers }; - get(self.base_url(path), Some(params), Some(headers)).await + get_result(self.base_url(path), Some(params), Some(headers)).await } - None => get(self.base_url(path), Some(params), headers).await, + None => get_result(self.base_url(path), Some(params), headers).await, } } + /// # Panics + /// + /// Will panic if the request can't be sent pub async fn get_request(&self, path: &str) -> Response { get(self.base_url(path), None, None).await } + /// Fallible version of [`Self::get_request`] that returns a `Result` instead of panicking. + pub(crate) async fn get_request_result(&self, path: &str) -> Result { + get_result(self.base_url(path), None, None).await + } + fn base_url(&self, path: &str) -> Url { Url::parse(&format!("{}{}{path}", self.connection_info.origin, self.base_path)).unwrap() } @@ -398,10 +492,14 @@ impl ApiHttpClient { /// /// Will panic if the request can't be sent pub async fn get(path: Url, query: Option, headers: Option) -> Response { + get_result(path, query, headers).await.unwrap() +} + +/// Fallible version of [`get`] that returns a `Result` instead of panicking. +pub(crate) async fn get_result(path: Url, query: Option, headers: Option) -> Result { let client = reqwest::Client::builder() .timeout(Duration::from_secs(DEFAULT_REQUEST_TIMEOUT_IN_SECS)) - .build() - .unwrap(); + .build()?; let mut request_builder = client.get(path); @@ -413,7 +511,7 @@ pub async fn get(path: Url, query: Option, headers: Option) -> request_builder = request_builder.headers(headers); } - request_builder.send().await.unwrap() + request_builder.send().await } /// Returns a `HeaderMap` with a request id header. From 876d0f597361c721aa13dc4bc504555565a526a6 Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Tue, 30 Jun 2026 10:56:50 +0100 Subject: [PATCH 06/10] docs(rest-api-client): add # Panics sections to ApiHttpClient methods Clippy requires # Panics documentation on all public methods that may panic. Added the missing doc sections to all 11 ApiHttpClient public methods that use .unwrap(). --- packages/rest-api-client/src/v1/client.rs | 55 +++++++++++++++++++++++ 1 file changed, 55 insertions(+) diff --git a/packages/rest-api-client/src/v1/client.rs b/packages/rest-api-client/src/v1/client.rs index 6acf26a1f..4d2be960a 100644 --- a/packages/rest-api-client/src/v1/client.rs +++ b/packages/rest-api-client/src/v1/client.rs @@ -273,52 +273,107 @@ impl ApiHttpClient { }) } + /// Generates a new random authentication key valid for `seconds_valid`. + /// + /// # Panics + /// + /// Will panic if the request can't be sent. pub async fn generate_auth_key(&self, seconds_valid: i32, headers: Option) -> Response { self.post_empty_result(&format!("key/{seconds_valid}"), headers) .await .unwrap() } + /// Adds a new authentication key using the provided form data. + /// + /// # Panics + /// + /// Will panic if the request can't be sent. pub async fn add_auth_key(&self, add_key_form: AddKeyForm, headers: Option) -> Response { self.post_form_result("keys", &add_key_form, headers).await.unwrap() } + /// Deletes an authentication key. + /// + /// # Panics + /// + /// Will panic if the request can't be sent. pub async fn delete_auth_key(&self, key: &str, headers: Option) -> Response { self.delete_result(&format!("key/{key}"), headers).await.unwrap() } + /// Reloads authentication keys from the database. + /// + /// # Panics + /// + /// Will panic if the request can't be sent. pub async fn reload_keys(&self, headers: Option) -> Response { self.get_result("keys/reload", Query::default(), headers).await.unwrap() } + /// Whitelists a torrent by info hash. + /// + /// # Panics + /// + /// Will panic if the request can't be sent. pub async fn whitelist_a_torrent(&self, info_hash: &str, headers: Option) -> Response { self.post_empty_result(&format!("whitelist/{info_hash}"), headers) .await .unwrap() } + /// Removes a torrent from the whitelist. + /// + /// # Panics + /// + /// Will panic if the request can't be sent. pub async fn remove_torrent_from_whitelist(&self, info_hash: &str, headers: Option) -> Response { self.delete_result(&format!("whitelist/{info_hash}"), headers).await.unwrap() } + /// Reloads the whitelist from the database. + /// + /// # Panics + /// + /// Will panic if the request can't be sent. pub async fn reload_whitelist(&self, headers: Option) -> Response { self.get_result("whitelist/reload", Query::default(), headers).await.unwrap() } + /// Gets a single torrent by info hash. + /// + /// # Panics + /// + /// Will panic if the request can't be sent. pub async fn get_torrent(&self, info_hash: &str, headers: Option) -> Response { self.get_result(&format!("torrent/{info_hash}"), Query::default(), headers) .await .unwrap() } + /// Gets a list of torrents matching the query parameters. + /// + /// # Panics + /// + /// Will panic if the request can't be sent. pub async fn get_torrents(&self, params: Query, headers: Option) -> Response { self.get_result("torrents", params, headers).await.unwrap() } + /// Gets tracker statistics. + /// + /// # Panics + /// + /// Will panic if the request can't be sent. pub async fn get_tracker_statistics(&self, headers: Option) -> Response { self.get_result("stats", Query::default(), headers).await.unwrap() } + /// Performs a GET request. + /// + /// # Panics + /// + /// Will panic if the request can't be sent. pub async fn get(&self, path: &str, params: Query, headers: Option) -> Response { self.get_result(path, params, headers).await.unwrap() } From ed0d4543a713755e520a6df8943fdf1335052d31 Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Tue, 30 Jun 2026 11:20:39 +0100 Subject: [PATCH 07/10] refactor(rest-api-client): make ApiClient fully panic-free - Added variant for URL construction failures. - Added so works for transport errors. - Changed all fallible methods to return instead of . - Made fallible (returns ) instead of panicking. - methods now use directly with no calls. --- packages/rest-api-client/src/v1/client.rs | 109 +++++++++------------- 1 file changed, 42 insertions(+), 67 deletions(-) diff --git a/packages/rest-api-client/src/v1/client.rs b/packages/rest-api-client/src/v1/client.rs index 4d2be960a..2f94a3b49 100644 --- a/packages/rest-api-client/src/v1/client.rs +++ b/packages/rest-api-client/src/v1/client.rs @@ -41,6 +41,16 @@ pub enum ClientError { /// Failed to deserialize the API response body into the expected type. #[error("deserialization error: {0}")] DeserializationError(#[source] reqwest::Error), + + /// An internal error (URL construction failure, etc.). + #[error("internal error: {0}")] + InternalError(String), +} + +impl From for ClientError { + fn from(err: reqwest::Error) -> Self { + Self::TransportError(err) + } } /// High-level typed client for the Torrust Tracker REST API. @@ -77,11 +87,7 @@ impl ApiClient { /// Returns [`ClientError::ApiError`] if the API returns a non-2xx status. /// Returns [`ClientError::DeserializationError`] if the response cannot be parsed. pub async fn generate_auth_key(&self, seconds_valid: i32) -> Result { - let response = self - .inner - .post_empty_result(&format!("key/{seconds_valid}"), None) - .await - .map_err(ClientError::TransportError)?; + let response = self.inner.post_empty_result(&format!("key/{seconds_valid}"), None).await?; Self::parse_response(response).await } @@ -93,11 +99,7 @@ impl ApiClient { /// Returns [`ClientError::ApiError`] if the API returns a non-2xx status. /// Returns [`ClientError::DeserializationError`] if the response cannot be parsed. pub async fn add_auth_key(&self, form: AddKeyForm) -> Result { - let response = self - .inner - .post_form_result("keys", &form, None) - .await - .map_err(ClientError::TransportError)?; + let response = self.inner.post_form_result("keys", &form, None).await?; Self::parse_response(response).await } @@ -108,11 +110,7 @@ impl ApiClient { /// Returns [`ClientError::TransportError`] if the request fails. /// Returns [`ClientError::ApiError`] if the API returns a non-2xx status. pub async fn delete_auth_key(&self, key: &str) -> Result<(), ClientError> { - let response = self - .inner - .delete_result(&format!("key/{key}"), None) - .await - .map_err(ClientError::TransportError)?; + let response = self.inner.delete_result(&format!("key/{key}"), None).await?; Self::check_success(response).await } @@ -123,11 +121,7 @@ impl ApiClient { /// Returns [`ClientError::TransportError`] if the request fails. /// Returns [`ClientError::ApiError`] if the API returns a non-2xx status. pub async fn reload_keys(&self) -> Result<(), ClientError> { - let response = self - .inner - .get_result("keys/reload", Query::default(), None) - .await - .map_err(ClientError::TransportError)?; + let response = self.inner.get_result("keys/reload", Query::default(), None).await?; Self::check_success(response).await } @@ -138,11 +132,7 @@ impl ApiClient { /// Returns [`ClientError::TransportError`] if the request fails. /// Returns [`ClientError::ApiError`] if the API returns a non-2xx status. pub async fn whitelist_a_torrent(&self, info_hash: &str) -> Result<(), ClientError> { - let response = self - .inner - .post_empty_result(&format!("whitelist/{info_hash}"), None) - .await - .map_err(ClientError::TransportError)?; + let response = self.inner.post_empty_result(&format!("whitelist/{info_hash}"), None).await?; Self::check_success(response).await } @@ -153,11 +143,7 @@ impl ApiClient { /// Returns [`ClientError::TransportError`] if the request fails. /// Returns [`ClientError::ApiError`] if the API returns a non-2xx status. pub async fn remove_torrent_from_whitelist(&self, info_hash: &str) -> Result<(), ClientError> { - let response = self - .inner - .delete_result(&format!("whitelist/{info_hash}"), None) - .await - .map_err(ClientError::TransportError)?; + let response = self.inner.delete_result(&format!("whitelist/{info_hash}"), None).await?; Self::check_success(response).await } @@ -168,11 +154,7 @@ impl ApiClient { /// Returns [`ClientError::TransportError`] if the request fails. /// Returns [`ClientError::ApiError`] if the API returns a non-2xx status. pub async fn reload_whitelist(&self) -> Result<(), ClientError> { - let response = self - .inner - .get_result("whitelist/reload", Query::default(), None) - .await - .map_err(ClientError::TransportError)?; + let response = self.inner.get_result("whitelist/reload", Query::default(), None).await?; Self::check_success(response).await } @@ -187,8 +169,7 @@ impl ApiClient { let response = self .inner .get_result(&format!("torrent/{info_hash}"), Query::default(), None) - .await - .map_err(ClientError::TransportError)?; + .await?; Self::parse_response(response).await } @@ -200,11 +181,7 @@ impl ApiClient { /// Returns [`ClientError::ApiError`] if the API returns a non-2xx status. /// Returns [`ClientError::DeserializationError`] if the response cannot be parsed. pub async fn get_torrents(&self, params: Query) -> Result, ClientError> { - let response = self - .inner - .get_result("torrents", params, None) - .await - .map_err(ClientError::TransportError)?; + let response = self.inner.get_result("torrents", params, None).await?; Self::parse_response(response).await } @@ -216,11 +193,7 @@ impl ApiClient { /// Returns [`ClientError::ApiError`] if the API returns a non-2xx status. /// Returns [`ClientError::DeserializationError`] if the response cannot be parsed. pub async fn get_tracker_statistics(&self) -> Result { - let response = self - .inner - .get_result("stats", Query::default(), None) - .await - .map_err(ClientError::TransportError)?; + let response = self.inner.get_result("stats", Query::default(), None).await?; Self::parse_response(response).await } @@ -384,7 +357,7 @@ impl ApiHttpClient { path: &str, params: Query, headers: Option, - ) -> Result { + ) -> Result { let mut query: Query = params; if let Some(token) = &self.connection_info.api_token { @@ -402,8 +375,8 @@ impl ApiHttpClient { } /// Fallible version of [`Self::post_empty`] that returns a `Result` instead of panicking. - pub(crate) async fn post_empty_result(&self, path: &str, headers: Option) -> Result { - let builder = self.http_client.post(self.base_url(path).clone()); + pub(crate) async fn post_empty_result(&self, path: &str, headers: Option) -> Result { + let builder = self.http_client.post(self.base_url(path)?.clone()); let builder = match headers { Some(headers) => builder.headers(headers), @@ -415,7 +388,7 @@ impl ApiHttpClient { None => builder, }; - builder.send().await + Ok(builder.send().await?) } /// # Panics @@ -431,8 +404,8 @@ impl ApiHttpClient { path: &str, form: &T, headers: Option, - ) -> Result { - let builder = self.http_client.post(self.base_url(path).clone()).json(&form); + ) -> Result { + let builder = self.http_client.post(self.base_url(path)?.clone()).json(&form); let builder = match headers { Some(headers) => builder.headers(headers), @@ -444,7 +417,7 @@ impl ApiHttpClient { None => builder, }; - builder.send().await + Ok(builder.send().await?) } /// # Panics @@ -455,8 +428,8 @@ impl ApiHttpClient { } /// Fallible version of [`Self::delete`] that returns a `Result` instead of panicking. - async fn delete_result(&self, path: &str, headers: Option) -> Result { - let builder = self.http_client.delete(self.base_url(path).clone()); + async fn delete_result(&self, path: &str, headers: Option) -> Result { + let builder = self.http_client.delete(self.base_url(path)?.clone()); let builder = match headers { Some(headers) => builder.headers(headers), @@ -468,7 +441,7 @@ impl ApiHttpClient { None => builder, }; - builder.send().await + Ok(builder.send().await?) } /// # Panics @@ -484,7 +457,8 @@ impl ApiHttpClient { path: &str, params: Query, headers: Option, - ) -> Result { + ) -> Result { + let url = self.base_url(path)?; match &self.connection_info.api_token { Some(token) => { let headers = if let Some(headers) = headers { @@ -520,9 +494,9 @@ impl ApiHttpClient { headers }; - get_result(self.base_url(path), Some(params), Some(headers)).await + get_result(url, Some(params), Some(headers)).await } - None => get_result(self.base_url(path), Some(params), headers).await, + None => get_result(url, Some(params), headers).await, } } @@ -530,16 +504,17 @@ impl ApiHttpClient { /// /// Will panic if the request can't be sent pub async fn get_request(&self, path: &str) -> Response { - get(self.base_url(path), None, None).await + get(self.base_url(path).unwrap(), None, None).await } /// Fallible version of [`Self::get_request`] that returns a `Result` instead of panicking. - pub(crate) async fn get_request_result(&self, path: &str) -> Result { - get_result(self.base_url(path), None, None).await + pub(crate) async fn get_request_result(&self, path: &str) -> Result { + get_result(self.base_url(path)?, None, None).await } - fn base_url(&self, path: &str) -> Url { - Url::parse(&format!("{}{}{path}", self.connection_info.origin, self.base_path)).unwrap() + fn base_url(&self, path: &str) -> Result { + Url::parse(&format!("{}{}{path}", self.connection_info.origin, self.base_path)) + .map_err(|e| ClientError::InternalError(format!("invalid URL: {e}"))) } } @@ -551,7 +526,7 @@ pub async fn get(path: Url, query: Option, headers: Option) -> } /// Fallible version of [`get`] that returns a `Result` instead of panicking. -pub(crate) async fn get_result(path: Url, query: Option, headers: Option) -> Result { +pub(crate) async fn get_result(path: Url, query: Option, headers: Option) -> Result { let client = reqwest::Client::builder() .timeout(Duration::from_secs(DEFAULT_REQUEST_TIMEOUT_IN_SECS)) .build()?; @@ -566,7 +541,7 @@ pub(crate) async fn get_result(path: Url, query: Option, headers: Option< request_builder = request_builder.headers(headers); } - request_builder.send().await + request_builder.send().await.map_err(ClientError::TransportError) } /// Returns a `HeaderMap` with a request id header. From 48dc77614c442f08153a435633e28201eb4f3a32 Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Tue, 30 Jun 2026 12:47:44 +0100 Subject: [PATCH 08/10] docs(rest-api-client): add SI-8 follow-up issue spec for eliminating unwraps --- ...-eliminate-unwraps-from-rest-api-client.md | 100 ++++++++++++++++++ 1 file changed, 100 insertions(+) create mode 100644 docs/issues/open/1969-1938-si-8-eliminate-unwraps-from-rest-api-client.md diff --git a/docs/issues/open/1969-1938-si-8-eliminate-unwraps-from-rest-api-client.md b/docs/issues/open/1969-1938-si-8-eliminate-unwraps-from-rest-api-client.md new file mode 100644 index 000000000..bd815f5aa --- /dev/null +++ b/docs/issues/open/1969-1938-si-8-eliminate-unwraps-from-rest-api-client.md @@ -0,0 +1,100 @@ +--- +doc-type: spec +issue-type: task +status: planned +priority: p2 +epic: 1938 +github-issue: 1969 +spec-path: docs/issues/open/1969-1938-si-8-eliminate-unwraps-from-rest-api-client.md +last-updated-utc: 2026-06-30 +semantic-links: + skill-links: + - create-issue + related-artifacts: + - docs/issues/open/1938-rest-api-contract-first-migration/EPIC.md + - packages/rest-api-client/ + - packages/rest-api-client/src/v1/client.rs +--- + + + +# SI-8: Eliminate all unwraps from the REST API client package + +## Subissue of REST API Contract-First Migration EPIC + +## Goal + +Eliminate all `.unwrap()` calls from the `torrust-tracker-rest-api-client` package. Every operation that can fail must return a `Result`. For operations that are provably infallible, replace bare `.unwrap()` with an explicit `.expect("infallible: ...")` that documents why the operation cannot fail. + +## Background + +The `ApiClient` was made fully panic-free in SI-6 (PR #1968). However, the low-level `ApiHttpClient` and several free functions/helpers in `client.rs` still contain `.unwrap()` and `.expect()` calls that can panic at runtime. + +The calls fall into two categories: + +### Transport unwraps (must return `Result`) + +These are real failure points — network errors, URL parsing failures, etc. They must return `Result`: + +1. **11 public `ApiHttpClient` methods** — thin wrappers that delegate to fallible `*_result()` counterparts but `.unwrap()` the result. +2. **`post_empty()`, `post_form()`, `delete()`** (private) — same wrapper-with-unwrap pattern. +3. **`get()` (pub method on `ApiHttpClient`)** — same pattern. +4. **`get()` (pub free function)** — thin wrapper around `get_result()`. +5. **`get_request()` (pub on `ApiHttpClient`)** — calls `base_url()` which already returns `Result`. + +### Infallible conversions (replace `unwrap` with `expect`) + +These are provably infallible operations where a descriptive `expect` message is the right pattern: + +1. **`headers_with_request_id()`** — `Uuid::to_string()` always produces a valid ASCII string, and `HeaderValue::from_str()` for ASCII strings never fails. +2. **`headers_with_auth_token()`** — same pattern, pre-formatted token string. +3. **`get_request_with_query_result()` auth token inserts** — 2 token-to-HeaderValue conversions, same provably-infallible pattern. + +## Scope + +### In Scope + +- Change all panicking public `ApiHttpClient` methods to return `Result` instead of `Response`. +- Update all caller sites across the repository (contract tests, E2E tests, integration tests) to handle the new `Result` return types. +- Change helper functions (`post_empty`, `post_form`, `delete`, `get`, `get_request`, `get()`) to return `Result`. +- Replace bare `.unwrap()` with `.expect("infallible: ...")` in `headers_with_request_id()`, `headers_with_auth_token()`, and `get_request_with_query_result()` auth token inserts. +- Update issue spec and documentation. + +### Out of Scope + +- Changing the `ApiHttpClient`'s HTTP transport or connection model. +- Adding retry/timeout policy (tracked separately). +- Removing the `ApiClient`/`ApiHttpClient` two-tier architecture. + +## Implementation Plan + +| ID | Status | Task | Notes | +| --- | ------ | ---------------------------------------------------------------------------------------------- | ------------------------------------------------------------------------ | +| T1 | TODO | Move `ApiHttpClient` public methods to return `Result` | 10 methods + `get()` method — return `ClientError` | +| T2 | TODO | Update all callers in contract tests (`packages/axum-rest-api-server/tests/`) | ~65 `ApiHttpClient::new(...)` call sites | +| T3 | TODO | Update callers in `src/console/ci/qbittorrent_e2e/tracker/client.rs` | E2E test runner wrapper | +| T4 | TODO | Update callers in `tests/servers/api/contract/stats/mod.rs` | Integration test | +| T5 | TODO | Replace bare `.unwrap()` with `.expect("infallible: ...")` for provably infallible conversions | `headers_with_request_id`, `headers_with_auth_token`, auth token inserts | +| T6 | TODO | Verify pre-commit and pre-push checks pass | | + +## Verification / Progress + +- [ ] All `ApiHttpClient` public methods return `Result` +- [ ] No bare `.unwrap()` calls remain (only `.expect("infallible: ...")` for provably infallible operations) +- [ ] All contract tests pass unchanged (except for updated `.unwrap()` calls on test side) +- [ ] E2E tests compile +- [ ] Pre-commit checks pass +- [ ] Pre-push checks pass + +## Acceptance Criteria + +- `ApiHttpClient` never panics on transport/URL failures; all errors are returned as `ClientError` +- Provably infallible conversions use `.expect("infallible: ...")` with a clear rationale +- No regressions in existing tests +- `linter all` passes + +### Progress Log + +| Date | Event | +| ---------- | ------------------ | +| 2026-06-30 | Draft spec created | From 7af17b38816ff5bda7959fe226946c5aaeae20a1 Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Tue, 30 Jun 2026 16:11:05 +0100 Subject: [PATCH 09/10] fix(rest-api-client): remove unused methods delete() and get_request_result() --- packages/rest-api-client/src/v1/client.rs | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/packages/rest-api-client/src/v1/client.rs b/packages/rest-api-client/src/v1/client.rs index 2f94a3b49..0385e09dc 100644 --- a/packages/rest-api-client/src/v1/client.rs +++ b/packages/rest-api-client/src/v1/client.rs @@ -420,13 +420,6 @@ impl ApiHttpClient { Ok(builder.send().await?) } - /// # Panics - /// - /// Will panic if the request can't be sent - async fn delete(&self, path: &str, headers: Option) -> Response { - self.delete_result(path, headers).await.unwrap() - } - /// Fallible version of [`Self::delete`] that returns a `Result` instead of panicking. async fn delete_result(&self, path: &str, headers: Option) -> Result { let builder = self.http_client.delete(self.base_url(path)?.clone()); @@ -507,11 +500,6 @@ impl ApiHttpClient { get(self.base_url(path).unwrap(), None, None).await } - /// Fallible version of [`Self::get_request`] that returns a `Result` instead of panicking. - pub(crate) async fn get_request_result(&self, path: &str) -> Result { - get_result(self.base_url(path)?, None, None).await - } - fn base_url(&self, path: &str) -> Result { Url::parse(&format!("{}{}{path}", self.connection_info.origin, self.base_path)) .map_err(|e| ClientError::InternalError(format!("invalid URL: {e}"))) From bbd29362bdbd64b36202dddeca8c255af566b82e Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Tue, 30 Jun 2026 16:13:03 +0100 Subject: [PATCH 10/10] docs(rest-api-client): update SI-8 spec to reflect removed delete() method --- ...969-1938-si-8-eliminate-unwraps-from-rest-api-client.md | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/docs/issues/open/1969-1938-si-8-eliminate-unwraps-from-rest-api-client.md b/docs/issues/open/1969-1938-si-8-eliminate-unwraps-from-rest-api-client.md index bd815f5aa..a63b868af 100644 --- a/docs/issues/open/1969-1938-si-8-eliminate-unwraps-from-rest-api-client.md +++ b/docs/issues/open/1969-1938-si-8-eliminate-unwraps-from-rest-api-client.md @@ -37,7 +37,7 @@ The calls fall into two categories: These are real failure points — network errors, URL parsing failures, etc. They must return `Result`: 1. **11 public `ApiHttpClient` methods** — thin wrappers that delegate to fallible `*_result()` counterparts but `.unwrap()` the result. -2. **`post_empty()`, `post_form()`, `delete()`** (private) — same wrapper-with-unwrap pattern. +2. **`post_empty()`, `post_form()`** (private) — same wrapper-with-unwrap pattern. 3. **`get()` (pub method on `ApiHttpClient`)** — same pattern. 4. **`get()` (pub free function)** — thin wrapper around `get_result()`. 5. **`get_request()` (pub on `ApiHttpClient`)** — calls `base_url()` which already returns `Result`. @@ -56,7 +56,7 @@ These are provably infallible operations where a descriptive `expect` message is - Change all panicking public `ApiHttpClient` methods to return `Result` instead of `Response`. - Update all caller sites across the repository (contract tests, E2E tests, integration tests) to handle the new `Result` return types. -- Change helper functions (`post_empty`, `post_form`, `delete`, `get`, `get_request`, `get()`) to return `Result`. +- Change helper functions (`post_empty`, `post_form`, `get`, `get_request`, `get()`) to return `Result`. - Replace bare `.unwrap()` with `.expect("infallible: ...")` in `headers_with_request_id()`, `headers_with_auth_token()`, and `get_request_with_query_result()` auth token inserts. - Update issue spec and documentation. @@ -97,4 +97,5 @@ These are provably infallible operations where a descriptive `expect` message is | Date | Event | | ---------- | ------------------ | -| 2026-06-30 | Draft spec created | +| 2026-06-30 | Spec drafted | +| 2026-06-30 | Spec moved to open |