|
| 1 | +## Context |
| 2 | + |
| 3 | +`tools_cache` still uses the legacy `db/schema` + `db/crud` persistence pattern. `ToolsCacheCRUD` accepts `ToolsCacheSave`, returns `ToolsCacheDB`, and consumers convert rows with `ToolsCache.model_validate(...)`. The legacy schema also contains cache-entry behavior (`is_expired`) and uses `created_at: datetime = datetime.now()`, which evaluates once when the module is imported rather than when each entry is created. |
| 4 | + |
| 5 | +The cache is shared infrastructure used by chat attachment processing, currency exchange-rate fetching, web fetching, HTML cleanup, Twitter fetching, and scheduled cleanup. Cache entries are internal full-state objects rather than partial external snapshots: |
| 6 | + |
| 7 | +``` |
| 8 | +caller |
| 9 | + │ |
| 10 | + ▼ |
| 11 | +repository ─────▶ ToolsCacheDB |
| 12 | + │ ▲ |
| 13 | + ▼ │ |
| 14 | +domain dataclass ◀──── mapper |
| 15 | +``` |
| 16 | + |
| 17 | +``` |
| 18 | +key |
| 19 | + required deterministic identity, never DB-generated |
| 20 | +
|
| 21 | +value |
| 22 | + required full replacement value |
| 23 | +
|
| 24 | +created_at |
| 25 | + required timestamp, defaulted per domain instance |
| 26 | +
|
| 27 | +expires_at |
| 28 | + nullable expiration state |
| 29 | + None means "never expires" |
| 30 | +``` |
| 31 | + |
| 32 | +## Goals / Non-Goals |
| 33 | + |
| 34 | +**Goals:** |
| 35 | + |
| 36 | +- Introduce a tools cache domain dataclass, mapper, and repository matching newer persistence units. |
| 37 | +- Keep `ToolsCacheDB` as the only SQLAlchemy model for the existing table. |
| 38 | +- Preserve deterministic cache keys, cache hit/miss behavior, explicit expiration checks, full upsert behavior, deletion, and expired-entry cleanup. |
| 39 | +- Correct `created_at` defaulting so each new domain object receives its own creation timestamp. |
| 40 | +- Add domain, mapper, and repository tests beside legacy tests, then migrate production callers in reviewable feature groups. |
| 41 | +- Remove legacy CRUD/schema access only after production and test references are gone. |
| 42 | + |
| 43 | +**Non-Goals:** |
| 44 | + |
| 45 | +- No database migration or schema change. |
| 46 | +- No external API or OpenAPI change. |
| 47 | +- No cache backend replacement, distributed cache, in-memory cache, eviction policy, or serialization redesign. |
| 48 | +- No automatic deletion of an expired entry during `get`; callers retain the current explicit expiration check behavior. |
| 49 | +- No clock abstraction or timezone migration in this change. |
| 50 | + |
| 51 | +## Decisions |
| 52 | + |
| 53 | +### 1. Keep One SQLAlchemy Model |
| 54 | + |
| 55 | +`ToolsCacheDB` remains the only DB model and table definition for `tools_cache`. The new model is a feature-level dataclass mapped to and from `ToolsCacheDB`. |
| 56 | + |
| 57 | +**Rationale**: The table already has the required shape. A second SQLAlchemy representation would duplicate ownership and create migration risk without improving the domain boundary. |
| 58 | + |
| 59 | +**Alternative considered**: Create a parallel SQLAlchemy model. Rejected because it adds competing metadata for the same table. |
| 60 | + |
| 61 | +### 2. Use One Full-State Domain Model |
| 62 | + |
| 63 | +The domain model should represent both new and persisted entries: |
| 64 | + |
| 65 | +``` |
| 66 | +ToolsCache( |
| 67 | + key: str, |
| 68 | + value: str, |
| 69 | + created_at: datetime = field(default_factory=datetime.now), |
| 70 | + expires_at: datetime | None = None, |
| 71 | +) |
| 72 | +``` |
| 73 | + |
| 74 | +It retains `is_expired()` behavior. `expires_at = None` is meaningful state and means the entry never expires. |
| 75 | + |
| 76 | +**Rationale**: Cache writes are internal and always contain the complete entry state. A separate save/draft model would reproduce the legacy split without adding safety. |
| 77 | + |
| 78 | +**Alternative considered**: Keep `ToolsCacheSave` beside the domain model. Rejected because there is no distinct create-only or partial-update shape. |
| 79 | + |
| 80 | +### 3. Correct `created_at` to a Per-Instance Default |
| 81 | + |
| 82 | +The domain dataclass uses `field(default_factory = datetime.now)`. Repository conversion always writes the domain timestamp explicitly. |
| 83 | + |
| 84 | +On insert, `created_at` records construction time. On update, `save` writes the supplied full object, including `created_at`; callers currently construct a fresh entry when refreshing cache content, so a refresh receives a fresh timestamp. |
| 85 | + |
| 86 | +**Rationale**: The existing Pydantic declaration evaluates `datetime.now()` at import time and can reuse that stale timestamp for every omitted `created_at`. No production consumer reads `created_at`, and expiration cleanup depends only on `expires_at`, so correcting this is isolated and testable. |
| 87 | + |
| 88 | +**Alternative considered**: Preserve the existing row timestamp during upsert. Rejected because `save` is full replacement and callers are replacing the cache entry, not patching individual fields. |
| 89 | + |
| 90 | +### 4. Keep Deterministic Key Creation at the Domain Boundary |
| 91 | + |
| 92 | +The domain model exposes the existing deterministic key creation behavior using the same algorithm: |
| 93 | + |
| 94 | +1. Base64-encode prefix and identifier separately. |
| 95 | +2. Join them with `~`. |
| 96 | +3. Return the MD5 digest through the existing `digest_md5` helper. |
| 97 | + |
| 98 | +Callers use the domain helper rather than a repository method. |
| 99 | + |
| 100 | +**Rationale**: Key derivation defines cache identity but does not access persistence. Keeping it with the domain model avoids making callers resolve a database dependency for a pure function while preserving the exact stored-key format. |
| 101 | + |
| 102 | +**Alternative considered**: Keep `create_key` on the repository for a one-for-one CRUD replacement. Rejected because it keeps unrelated domain behavior on the persistence boundary. |
| 103 | + |
| 104 | +### 5. Expose a Narrow Repository Surface |
| 105 | + |
| 106 | +The repository provides: |
| 107 | + |
| 108 | +- `get(key) -> ToolsCache | None` |
| 109 | +- `get_all(skip, limit) -> list[ToolsCache]` |
| 110 | +- `save(entry) -> ToolsCache` |
| 111 | +- `delete(key) -> ToolsCache | None` |
| 112 | +- `delete_expired() -> int` |
| 113 | + |
| 114 | +`save` performs an exact full-object upsert: |
| 115 | + |
| 116 | +``` |
| 117 | + ┌──────────────────────┐ |
| 118 | + │ ToolsCache domain │ |
| 119 | + │ complete entry state │ |
| 120 | + └──────────┬───────────┘ |
| 121 | + │ |
| 122 | + ┌──────────────┴──────────────┐ |
| 123 | + ▼ ▼ |
| 124 | + no existing key existing key found |
| 125 | + │ │ |
| 126 | + ▼ ▼ |
| 127 | + insert all fields replace all fields |
| 128 | +``` |
| 129 | + |
| 130 | +There are no separate public `create` and `update` methods because production callers use upsert semantics and do not need partial updates. |
| 131 | + |
| 132 | +**Rationale**: A smaller repository API makes the intended cache behavior explicit and avoids carrying CRUD mechanics that only legacy persistence tests use. |
| 133 | + |
| 134 | +**Alternative considered**: Mirror every CRUD method. Rejected because `create` and `update` have no production caller and are both covered by `save` behavior. |
| 135 | + |
| 136 | +### 6. Migrate Consumers by Feature Group |
| 137 | + |
| 138 | +The repository is added to DI while legacy CRUD remains available. Callers then migrate in bounded groups: |
| 139 | + |
| 140 | +1. Domain, mapper, repository, DI, SQL test helper, and focused tests. |
| 141 | +2. Currency exchange-rate caching. |
| 142 | +3. Web browsing, HTML cleanup, and Twitter caching. |
| 143 | +4. Chat attachment caching and related responder/support mocks. |
| 144 | +5. Scheduled expired-entry cleanup. |
| 145 | +6. Remove legacy DI, CRUD, schema, and tests after all references are gone. |
| 146 | + |
| 147 | +**Rationale**: Cache persistence has many consumers but no single service owner. Feature-group migration keeps failures attributable and preserves manual review checkpoints. |
| 148 | + |
| 149 | +**Alternative considered**: Replace every cache caller in one pass. Rejected because the web-browsing and chat test surfaces are broad enough to warrant separate checkpoints. |
| 150 | + |
| 151 | +## Risks / Trade-offs |
| 152 | + |
| 153 | +- **Deterministic key output changes and existing cache entries become unreachable** -> Keep the exact algorithm and add a golden-output test for the existing prefix/identifier example. |
| 154 | +- **`created_at` correction changes stored timestamps on refreshed entries** -> Specify full replacement explicitly and test insert/update timestamps; no current caller reads this field. |
| 155 | +- **`expires_at = None` is mistaken for an absent update** -> Mapper/repository tests must cover clearing expiration and never-expiring entries. |
| 156 | +- **Expired entries begin disappearing during reads** -> Keep `get` as a raw lookup and retain explicit domain expiration checks in consumers. |
| 157 | +- **Mixed DB/Pydantic/domain cache shapes during migration** -> Migrate one feature group at a time and keep focused tests green before removing legacy access. |
| 158 | +- **Tests currently return dictionaries or Pydantic cache models from mocks** -> Update mocks to return the domain dataclass as each caller migrates. |
| 159 | + |
| 160 | +## Migration Plan |
| 161 | + |
| 162 | +1. Add the domain model, mapper, repository, DI property, and SQL test helper beside the existing CRUD/schema. |
| 163 | +2. Add domain, mapper, and repository tests covering identity, timestamp, expiration, upsert, deletion, and cleanup semantics. |
| 164 | +3. Migrate currency cache consumers and tests. |
| 165 | +4. Migrate web browsing, HTML cleanup, Twitter cache consumers, and tests. |
| 166 | +5. Migrate chat attachment cache consumers and related support mocks/tests. |
| 167 | +6. Migrate scheduled cleanup to the repository. |
| 168 | +7. Remove legacy DI access, CRUD/schema tests, CRUD/schema files, and SQL test helper after reference searches are clean. |
| 169 | +8. Run the full offline test suite, all-files pre-commit, and strict OpenSpec validation. |
| 170 | + |
| 171 | +Rollback remains straightforward because the database schema and deterministic key format do not change, and legacy CRUD remains available until the final cleanup milestone. |
| 172 | + |
| 173 | +## Open Questions |
| 174 | + |
| 175 | +None. Cache writes are internal full-state replacements, and the current caller/test surface defines the required behavior. |
0 commit comments