Skip to content

Commit ed0d168

Browse files
josecelanojosecelano
committed
Merge #1714: feat(tracker-core): split Database trait into four narrow persistence traits
bb98322 docs(tracker-core): fix broken intra-doc links in sqlite and mysql drivers (Jose Celano) 757acbe docs(issues): add follow-up subissue spec for consumer migration to narrow traits (Jose Celano) 4aea234 docs(adrs): add ADR for keeping Database as aggregate supertrait (Jose Celano) dd4eaf6 feat(tracker-core): split Database trait into four narrow persistence traits (Jose Celano) 81a1472 docs(1713): update spec with implementation notes and refined acceptance criteria (Jose Celano) 9a91691 docs(1525-04): rename spec and link to GitHub issue #1713 (Jose Celano) Pull request description: ## Summary Implements GitHub issue #1713 — part of the persistence overhaul EPIC #1525. Splits the monolithic `Database` trait in `packages/tracker-core` into four focused narrow traits, each covering a single concern: | Trait | Concern | |---|---| | `AuthKeyStore` | Authentication key persistence | | `SchemaMigrator` | Schema creation / teardown | | `TorrentMetricsStore` | Per-torrent and global download counters | | `WhitelistStore` | Info-hash whitelist management | The `Database` aggregate supertrait is kept as an **internal-only compile-time guard** via blanket impl — no consumer call sites change in this PR. ## Changes ### New: `packages/tracker-core/src/databases/traits/` Four narrow trait files plus a `mod.rs` with re-exports: - `auth_keys.rs` — `AuthKeyStore` (+ `MockAuthKeyStore`) - `schema.rs` — `SchemaMigrator` (+ `MockSchemaMigrator`) - `torrent_metrics.rs` — `TorrentMetricsStore` (+ `MockTorrentMetricsStore`) - `whitelist.rs` — `WhitelistStore` (+ `MockWhitelistStore`) - `database.rs` — `Database` blanket impl (internal guard only) ### Updated: `packages/tracker-core/src/databases/` - `mod.rs` — routes through `pub mod traits` and re-exports all trait names at the `databases` level for backward compatibility - `driver/sqlite.rs`, `driver/mysql.rs` — implement four narrow traits via separate `impl` blocks; blanket impl satisfies `Database` automatically - `authentication/handler.rs` — tests now use `MockAuthKeyStore` instead of `MockDatabase` ### Docs - ADR: `docs/adrs/20260429000000_keep_database_as_aggregate_supertrait.md` - Spec (updated): `docs/issues/1713-1525-04-split-persistence-traits.md` - Follow-up subissue spec (new): `docs/issues/1525-04b-migrate-consumers-to-narrow-traits.md` ## Design Decision The `Database` supertrait is kept (not removed) because consumer wiring (`Arc<Box<dyn Database>>` in `TrackerCoreContainer`) still references it. Consumer migration is deferred to subissue #1525-04b, where `DatabaseStores` (a plain struct with one `Arc<dyn XxxStore>` field per concern) will replace `Arc<Box<dyn Database>>` in the container. See the ADR for full rationale. ## Related - Closes #1713 - Part of EPIC #1525 - Follow-up: subissue #1525-04b (consumer migration to narrow traits) - Predecessor: `docs/issues/1525-overhaul-persistence.md` ACKs for top commit: josecelano: ACK bb98322 Tree-SHA512: 7d49cd27cb1387b7f3e87988f98ad871d24c680ca654e18fe86d7ae0f0fd1d2470745c4ce2d2b6f72f1f1ae886cc1f7b7c3af855d56701f2349aa25ddb594547
2 parents 899fc82 + bb98322 commit ed0d168

16 files changed

Lines changed: 771 additions & 338 deletions

‎docs/adrs/20260429000000_keep_database_as_aggregate_supertrait.md‎

Lines changed: 94 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,94 @@
1+
# Keep `Database` as an Aggregate Supertrait
2+
3+
## Description
4+
5+
The persistence layer used a single monolithic `Database` trait with 18 methods
6+
spanning four distinct concerns: schema lifecycle, torrent metrics, whitelist
7+
management, and authentication keys. Consumers that only needed one concern
8+
(e.g. `DatabaseKeyRepository`) were forced to depend on the full 18-method
9+
interface, making tests harder to write and clouding the intent of each service.
10+
11+
The question was how to split the trait while preserving a single, discoverable
12+
contract that all database drivers must satisfy.
13+
14+
## Agreement
15+
16+
Split `Database` into four narrow context traits:
17+
18+
- `SchemaMigrator` — `create_database_tables`, `drop_database_tables`
19+
- `TorrentMetricsStore` — load/save/increase per-torrent and global download counters (7 methods)
20+
- `WhitelistStore` — load/get/add/remove infohash whitelist entries (4 required + 1 default method)
21+
- `AuthKeyStore` — load/get/add/remove authentication keys (4 methods)
22+
23+
Keep `Database` as an **empty aggregate supertrait** with a blanket implementation:
24+
25+
```rust
26+
pub trait Database: Sync + Send + SchemaMigrator + TorrentMetricsStore + WhitelistStore + AuthKeyStore {}
27+
28+
impl<T> Database for T where T: Sync + Send + SchemaMigrator + TorrentMetricsStore + WhitelistStore + AuthKeyStore {}
29+
```
30+
31+
`Database` is a **private, internal compile-time contract** for driver
32+
completeness only. External consumers (services, repositories, tests) will
33+
progress toward using only the narrow traits they actually need. That migration
34+
happens in future subissues and does not require changing any consumer in this
35+
step.
36+
37+
### Alternatives Considered
38+
39+
**Independent traits only (no `Database` supertrait)** — Each driver would
40+
implement four separate traits; consumers would receive `Arc<Box<dyn AuthKeyStore>>`
41+
etc. instead of `Arc<Box<dyn Database>>`.
42+
43+
Rejected because:
44+
45+
1. There would be no single place to verify that a driver implements the
46+
complete persistence contract — the compiler can no longer catch a partially
47+
implemented driver as one unit.
48+
2. Changing every call site (container wiring, factory, tests) all at once
49+
would turn this structural step into a much larger, riskier diff. The
50+
aggregate supertrait lets the split land cleanly first; consumer migration
51+
follows in subsequent subissues.
52+
53+
Note on trait-object upcasting: migrating consumers to narrow traits does **not**
54+
require upcasting (`dyn Database` → `dyn WhitelistStore`). The factory will
55+
construct the concrete driver type (e.g. `Arc<Sqlite>`) and coerce it directly
56+
into each narrow trait object (`Arc<dyn WhitelistStore>`, etc.). Coercion from
57+
a sized type to a trait object is available on all Rust versions; upcasting
58+
between two trait objects would be a different story, but is not needed here.
59+
60+
### Consequences
61+
62+
#### Positive
63+
64+
- Each narrow trait expresses a single context; services and tests can depend
65+
only on the interface they actually need.
66+
- `#[automock]` on each narrow trait generates focused mocks (`MockAuthKeyStore`
67+
etc.) instead of one 18-method mega-mock.
68+
- The blanket impl makes it impossible to partially implement `Database`:
69+
the compiler enforces completeness of all four narrow traits together.
70+
71+
### Negative
72+
73+
- Tests that previously used `MockDatabase` must be updated to use the
74+
appropriate narrow mock (`MockWhitelistStore`, `MockAuthKeyStore`, etc.).
75+
This is actually simpler — each mock covers only the methods the test cares
76+
about — but it is a mechanical change across test files.
77+
- `Database` will persist as long as `Arc<Box<dyn Database>>` wiring exists.
78+
That wiring will be replaced in subissue #1525-04b
79+
([docs/issues/1525-04b-migrate-consumers-to-narrow-traits.md](../issues/1525-04b-migrate-consumers-to-narrow-traits.md))
80+
by a plain `DatabaseStores` struct (one `Arc<dyn XxxStore>` field per
81+
context). `TrackerCoreContainer` will hold `DatabaseStores` instead of
82+
`Arc<Box<dyn Database>>`; each service is wired at construction time by
83+
passing only the narrow store it needs. At that point `Database` can be
84+
made fully private or removed.
85+
86+
## Date
87+
88+
2026-04-29
89+
90+
## References
91+
92+
- Issue spec: [docs/issues/1713-1525-04-split-persistence-traits.md](../issues/1713-1525-04-split-persistence-traits.md)
93+
- GitHub issue: <https://github.com/torrust/torrust-tracker/issues/1713>
94+
- EPIC: [docs/issues/1525-overhaul-persistence.md](../issues/1525-overhaul-persistence.md)

‎docs/adrs/index.md‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
# ADR Index
22

3-
| ADR | Date | Title | Short Description |
4-
| --------------------------------------------------------------------------------------- | ---------- | ------------------------------------------------------ | ------------------------------------------------------------------------------------------------------------------------------ |
5-
| [20240227164834](20240227164834_use_plural_for_modules_containing_collections.md) | 2024-02-27 | Use plural for modules containing collections | Module names should use plural when they contain multiple types with the same responsibility (e.g. `requests/`, `responses/`). |
6-
| [20260420200013](20260420200013_adopt_custom_github_copilot_aligned_agent_framework.md) | 2026-04-20 | Adopt a custom, GitHub-Copilot-aligned agent framework | Use AGENTS.md, Agent Skills, and Custom Agent profiles instead of third-party agent frameworks. |
3+
| ADR | Date | Title | Short Description |
4+
| --------------------------------------------------------------------------------------- | ---------- | ------------------------------------------------------ | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ |
5+
| [20240227164834](20240227164834_use_plural_for_modules_containing_collections.md) | 2024-02-27 | Use plural for modules containing collections | Module names should use plural when they contain multiple types with the same responsibility (e.g. `requests/`, `responses/`). |
6+
| [20260420200013](20260420200013_adopt_custom_github_copilot_aligned_agent_framework.md) | 2026-04-20 | Adopt a custom, GitHub-Copilot-aligned agent framework | Use AGENTS.md, Agent Skills, and Custom Agent profiles instead of third-party agent frameworks. |
7+
| [20260429000000](20260429000000_keep_database_as_aggregate_supertrait.md) | 2026-04-29 | Keep `Database` as an aggregate supertrait | Split the 18-method monolithic `Database` trait into four narrow context traits (`SchemaMigrator`, `TorrentMetricsStore`, `WhitelistStore`, `AuthKeyStore`) while keeping `Database` as an empty aggregate supertrait with a blanket impl. |

‎docs/issues/1525-04b-migrate-consumers-to-narrow-traits.md‎

Lines changed: 170 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,170 @@
1+
# Subissue Draft for #1525-04b: Migrate Consumers to Narrow Persistence Traits
2+
3+
## Goal
4+
5+
Replace every use of `Arc<Box<dyn Database>>` in production and test code with
6+
the specific narrow trait the consumer actually needs (`AuthKeyStore`,
7+
`TorrentMetricsStore`, `WhitelistStore`, or `SchemaMigrator`). After this
8+
subissue the `Database` aggregate supertrait becomes a purely internal
9+
compile-time guard that is no longer part of the public surface of
10+
`tracker-core`.
11+
12+
## Background
13+
14+
Subissue #1525-04 (GitHub [#1713](https://github.com/torrust/torrust-tracker/issues/1713))
15+
introduced the four narrow traits and kept `Database` as an aggregate supertrait
16+
so that consumer call sites did not need to change.
17+
18+
Now that the structural split is in place, this subissue wires consumers to the
19+
narrow traits they actually need. No upcasting is required: the factory will
20+
construct the concrete driver (`Sqlite`, `Mysql`) and coerce it directly into
21+
each narrow `Arc<dyn XxxStore>`. Coercion from a sized type to a trait object is
22+
available on all Rust versions.
23+
24+
## Proposed Branch
25+
26+
- `1525-04b-migrate-consumers-to-narrow-traits`
27+
28+
## Current State
29+
30+
All consumers depend on `Arc<Box<dyn Database>>` for everything, regardless of
31+
which methods they actually call:
32+
33+
| Consumer | Methods actually used |
34+
| -------------------------------------------------- | ----------------------------------------------------------- |
35+
| `DatabaseKeyRepository` | `AuthKeyStore` methods only |
36+
| `DatabaseDownloadsMetricRepository` | `TorrentMetricsStore` methods only |
37+
| `whitelist::setup::initialize_whitelist_manager` | `WhitelistStore` methods only |
38+
| `databases::driver::build` / `initialize_database` | `SchemaMigrator::create_database_tables` only |
39+
| `bin/persistence_benchmark` | All four concerns — uses `Database` as a convenience bundle |
40+
| `container::TrackerCoreContainer` | Holds the database and fans it out to the above |
41+
42+
## Target State
43+
44+
```text
45+
TrackerCoreContainer
46+
database_stores: DatabaseStores ← replaces Arc<Box<dyn Database>>
47+
...rest of fields unchanged...
48+
```
49+
50+
`DatabaseStores` is a plain struct holding one `Arc<dyn XxxStore>` per context.
51+
The container stores it as one named field; individual services are wired at
52+
construction time by passing the relevant field (e.g.
53+
`database_stores.auth_key_store.clone()`) to each service constructor. Services
54+
themselves never see `DatabaseStores` — they receive only the narrow trait they
55+
need.
56+
57+
The factory (`databases::driver::build` / `initialize_database`) constructs the
58+
concrete driver once and produces four `Arc<dyn XxxStore>` coercions from it:
59+
60+
```rust
61+
pub struct DatabaseStores {
62+
pub schema_migrator: Arc<dyn SchemaMigrator>,
63+
pub torrent_metrics_store: Arc<dyn TorrentMetricsStore>,
64+
pub whitelist_store: Arc<dyn WhitelistStore>,
65+
pub auth_key_store: Arc<dyn AuthKeyStore>,
66+
}
67+
68+
pub fn initialize_database(config: &Core) -> DatabaseStores {
69+
match config.database.driver {
70+
Driver::Sqlite3 => {
71+
let db = Arc::new(Sqlite::new(&config.database.path).expect("..."));
72+
db.create_database_tables().expect("...");
73+
DatabaseStores {
74+
schema_migrator: db.clone(),
75+
torrent_metrics_store: db.clone(),
76+
whitelist_store: db.clone(),
77+
auth_key_store: db,
78+
}
79+
}
80+
Driver::MySQL => { /* same pattern */ }
81+
}
82+
}
83+
```
84+
85+
## Tasks
86+
87+
### 1) Introduce `DatabaseStores`
88+
89+
Add a plain struct `databases::setup::DatabaseStores` holding one `Arc<dyn XxxStore>`
90+
per narrow trait. No `Arc<Box<dyn Database>>`.
91+
92+
### 2) Update `initialize_database`
93+
94+
Change the return type from `Arc<Box<dyn Database>>` to `DatabaseStores`.
95+
Build the concrete driver, call `create_database_tables`, then produce the four
96+
coercions.
97+
98+
### 3) Update `TrackerCoreContainer`
99+
100+
- Replace `pub database: Arc<Box<dyn Database>>` with `pub database_stores: DatabaseStores`.
101+
- Update `initialize_from` to call `initialize_database` (which now returns
102+
`DatabaseStores`) and fan the narrow stores out to each service constructor:
103+
104+
```rust
105+
let db = initialize_database(core_config);
106+
let whitelist_manager = initialize_whitelist_manager(db.whitelist_store.clone(), ...);
107+
let db_key_repository = Arc::new(DatabaseKeyRepository::new(db.auth_key_store.clone()));
108+
let db_downloads = Arc::new(DatabaseDownloadsMetricRepository::new(db.torrent_metrics_store.clone()));
109+
// ... store the struct itself so callers can still access it if needed
110+
Self { database_stores: db, ... }
111+
```
112+
113+
### 4) Update individual consumers
114+
115+
- `DatabaseKeyRepository::new` — accept `Arc<dyn AuthKeyStore>` instead of
116+
`Arc<Box<dyn Database>>`.
117+
- `DatabaseDownloadsMetricRepository::new` — accept `Arc<dyn TorrentMetricsStore>`.
118+
- `whitelist::setup::initialize_whitelist_manager` — accept `Arc<dyn WhitelistStore>`.
119+
120+
### 5) Update tests in `authentication/handler.rs`
121+
122+
Replace `Arc<Box<dyn Database>>` wiring with `MockAuthKeyStore` injected
123+
directly as `Arc<dyn AuthKeyStore>`.
124+
125+
### 6) Update `axum-rest-tracker-api-server` test helper
126+
127+
`packages/axum-rest-tracker-api-server/tests/server/mod.rs::force_database_error`
128+
currently receives `&Arc<Box<dyn Database>>`. Update to the narrow trait(s) it
129+
actually exercises.
130+
131+
### 7) Update benchmark binary
132+
133+
`bin/persistence_benchmark/driver_bench/` passes `&dyn Database` to operations
134+
that each touch only one concern. Update each operation function to accept the
135+
narrow trait it needs:
136+
137+
- `operations/torrent.rs` → `&dyn TorrentMetricsStore`
138+
- `operations/whitelist.rs` → `&dyn WhitelistStore`
139+
- `operations/keys.rs` → `&dyn AuthKeyStore`
140+
- `database/mod.rs::reset_database` → `&dyn SchemaMigrator`
141+
142+
### 8) Make `Database` private
143+
144+
Once no production or test code outside `databases/` uses `Database`, stop
145+
re-exporting it from `databases/mod.rs`. Keep it accessible inside
146+
`databases/traits/database.rs` for driver authors.
147+
148+
## Out of Scope
149+
150+
- Async trait methods. That is subissue #1525-05.
151+
- Schema migrations. That is subissue #1525-06.
152+
- PostgreSQL support. That is subissue #1525-08.
153+
154+
## Acceptance Criteria
155+
156+
- [ ] `Arc<Box<dyn Database>>` appears only inside `databases/` (driver + traits).
157+
- [ ] Each consumer holds only the narrow trait(s) it uses.
158+
- [ ] `Database` is no longer re-exported from `databases/mod.rs`.
159+
- [ ] Tests in `authentication/handler.rs` use `MockAuthKeyStore` directly.
160+
- [ ] `force_database_error` helper in `axum-rest-tracker-api-server` is updated.
161+
- [ ] Benchmark operations accept narrow traits.
162+
- [ ] `cargo test --workspace --all-targets` passes.
163+
- [ ] `linter all` exits with code `0`.
164+
165+
## References
166+
167+
- EPIC: #1525
168+
- Predecessor: [docs/issues/1713-1525-04-split-persistence-traits.md](1713-1525-04-split-persistence-traits.md)
169+
- ADR: [docs/adrs/20260429000000_keep_database_as_aggregate_supertrait.md](../adrs/20260429000000_keep_database_as_aggregate_supertrait.md)
170+
- Successor: [docs/issues/1525-05-migrate-sqlite-and-mysql-to-sqlx.md](1525-05-migrate-sqlite-and-mysql-to-sqlx.md)

‎docs/issues/1525-05-migrate-sqlite-and-mysql-to-sqlx.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -268,7 +268,7 @@ and all `r2d2`/`rusqlite`/`mysql` dependencies are gone.
268268
## References
269269

270270
- EPIC: `#1525`
271-
- Subissue `1525-04`: `docs/issues/1525-04-split-persistence-traits.md` — must be completed first
271+
- Subissue `1525-04`: `docs/issues/1713-1525-04-split-persistence-traits.md` — must be completed first
272272
- Subissue `1525-03`: `docs/issues/1525-03-persistence-benchmarking.md` — benchmark baseline
273273
- Reference PR: `#1695`
274274
- Reference implementation branch: `josecelano:pr-1684-review` — see EPIC for checkout

‎docs/issues/1525-overhaul-persistence.md‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -103,9 +103,15 @@ You can then browse or search it while working in the main repository.
103103

104104
### 4) Split the persistence traits by context
105105

106-
- Spec file: `docs/issues/1525-04-split-persistence-traits.md`
106+
- Spec file: `docs/issues/1713-1525-04-split-persistence-traits.md`
107107
- Outcome: smaller interfaces with lower coupling and clearer responsibilities
108108

109+
### 4b) Migrate consumers to narrow persistence traits
110+
111+
- Spec file: `docs/issues/1525-04b-migrate-consumers-to-narrow-traits.md`
112+
- Outcome: every consumer holds only the narrow trait(s) it uses; `Database`
113+
becomes a private compile-time guard inside `databases/`
114+
109115
### 5) Migrate SQLite and MySQL drivers to async `sqlx`
110116

111117
- Spec file: `docs/issues/1525-05-migrate-sqlite-and-mysql-to-sqlx.md`

docs/issues/1525-04-split-persistence-traits.md renamed to docs/issues/1713-1525-04-split-persistence-traits.md

Lines changed: 20 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
# Subissue Draft for #1525-04: Split Persistence Traits by Context
1+
# Issue #1713 (Subissue of #1525-04): Split Persistence Traits by Context
22

33
## Goal
44

@@ -43,7 +43,7 @@ This preserves both goals:
4343

4444
## Proposed Branch
4545

46-
- `1525-04-split-persistence-traits`
46+
- `1713-1525-04-split-persistence-traits`
4747

4848
## Current State
4949

@@ -246,6 +246,22 @@ pub use torrent_metrics::{MockTorrentMetricsStore, TorrentMetricsStore};
246246
pub use whitelist::{MockWhitelistStore, WhitelistStore};
247247
```
248248

249+
## Implementation Notes
250+
251+
- **`mockall` dependency**: Already present in `[dependencies]` of `tracker-core/Cargo.toml`.
252+
No change needed.
253+
254+
- **ADR timestamp**: Use the date the ADR is authored (`YYYYMMDDHHMMSS` format, today's date).
255+
256+
- **Consumer file changes**: The spirit of this subissue is not to mix refactorings — keep the
257+
focus on the structural split. However, if test-only code (e.g. `MockDatabase` usage in
258+
`handler.rs`) must be updated to compile after `MockDatabase` is removed, that change is
259+
acceptable. Production consumer files (`persisted.rs`, `downloads.rs`, etc.) must not change.
260+
261+
- **Method signatures**: Follow the actual code in `mod.rs` — the spec snippets are suggestions
262+
and may have drifted. In particular, `save_torrent_downloads` takes `completed: u32` (not
263+
`NumberOfDownloads`) in the current code.
264+
249265
## Out of Scope
250266

251267
- Changing consumer wiring from `Arc<Box<dyn Database>>` to narrow trait objects.
@@ -261,7 +277,8 @@ pub use whitelist::{MockWhitelistStore, WhitelistStore};
261277
- [ ] `Database` is an empty aggregate supertrait with a blanket impl.
262278
- [ ] Both drivers (`Sqlite`, `Mysql`) compile through the blanket impl with no manual
263279
`impl Database for <Driver>` block.
264-
- [ ] No existing consumer file (`persisted.rs`, `downloads.rs`, etc.) is changed.
280+
- [ ] Production consumer files (`persisted.rs`, `downloads.rs`, etc.) are not changed.
281+
- [ ] Test code that used `MockDatabase` is updated to use the appropriate narrow mock type.
265282
- [ ] `#[automock]` is on the four narrow traits; `MockDatabase` is removed.
266283
- [ ] No behavior change — existing tests pass without modification.
267284
- [ ] Persistence benchmarking (see subissue #1525-03) shows no regression against the

0 commit comments

Comments
 (0)