docs(prd): add PRD 0031 — split _merge_provider_route into named case helpers
This commit is contained in:
@@ -0,0 +1,173 @@
|
||||
# PRD 0031: Split `_merge_provider_route` into named case helpers
|
||||
|
||||
- **Status:** Draft
|
||||
- **Author:** didericis-claude
|
||||
- **Created:** 2026-06-02
|
||||
- **Issue:** #120
|
||||
|
||||
## Summary
|
||||
|
||||
Refactor `_merge_provider_route` in `bot_bottle/egress.py` to replace its
|
||||
five-outcome nested conditional with a top-level host-lookup dispatch and
|
||||
one named private function per outcome. No behaviour change.
|
||||
|
||||
## Problem
|
||||
|
||||
`_merge_provider_route` handles five distinct cases in a single function:
|
||||
|
||||
1. **append-new** — host not in manifest; append a fresh route.
|
||||
2. **upgrade-bare** — host found, no existing auth; adopt provider auth.
|
||||
3. **no-op** — host found, existing auth matches provider auth exactly;
|
||||
return unchanged.
|
||||
4. **tls-passthrough upgrade** — same as no-op but provider sets
|
||||
`tls_passthrough=True` and the existing route doesn't; flip the flag.
|
||||
5. **conflict-die** — host found, existing auth differs from provider;
|
||||
hard error.
|
||||
|
||||
These cases are currently identified by interleaved `if`/`continue`
|
||||
conditions rather than named dispatch. The control flow is:
|
||||
|
||||
```
|
||||
for idx, route in enumerate(routes): # host lookup
|
||||
if route.host != pr.host: continue
|
||||
if route.auth_scheme or route.token_ref: # already-authed branch
|
||||
if same auth:
|
||||
if tls upgrade needed: replace # case 4
|
||||
return # case 3
|
||||
die(...) # case 5
|
||||
[token_env alloc]
|
||||
routes[idx] = ... # case 2
|
||||
return routes
|
||||
[token_env alloc]
|
||||
routes.append(...) # case 1
|
||||
return routes
|
||||
```
|
||||
|
||||
Specific problems:
|
||||
|
||||
- Cases 3 and 4 share the same `if same auth:` block; case 3 is the
|
||||
implicit fall-through after the inner `if`, making it invisible by
|
||||
name.
|
||||
- `_find_or_alloc_token_env` is called twice with identical arguments in
|
||||
the upgrade-bare path (case 2) and the append-new path (case 1).
|
||||
- Adding a sixth case requires reading the whole function to find the
|
||||
right insertion point, with no structural hint about case boundaries.
|
||||
- The in-place index write (`routes[idx] = EgressRoute(...)`) spells out
|
||||
every field explicitly; a new field added to `EgressRoute` silently
|
||||
drops its value on any in-place replacement that wasn't updated.
|
||||
|
||||
## Goals / Success Criteria
|
||||
|
||||
- Each of the five outcomes is implemented in its own named private
|
||||
function with a docstring stating the precondition.
|
||||
- `_merge_provider_route` reads as a dispatch table: find the host, then
|
||||
call the right helper.
|
||||
- In-place replacements use `dataclasses.replace` instead of full
|
||||
constructor calls.
|
||||
- All existing `TestProviderRouteMerge` tests pass without modification.
|
||||
- No behaviour change.
|
||||
|
||||
## Non-goals
|
||||
|
||||
- Changing the public API of `egress_routes_for_bottle` or
|
||||
`egress_manifest_routes`.
|
||||
- Changing merge semantics (what counts as a conflict, what upgrade rules
|
||||
apply).
|
||||
- Consolidating any other complexity in `egress.py`.
|
||||
|
||||
## Design
|
||||
|
||||
### Dispatch structure
|
||||
|
||||
```python
|
||||
def _merge_provider_route(
|
||||
routes: list[EgressRoute], pr: EgressRoute,
|
||||
) -> list[EgressRoute]:
|
||||
for idx, route in enumerate(routes):
|
||||
if route.host.lower() == pr.host.lower():
|
||||
return _merge_at_index(routes, idx, route, pr)
|
||||
return _append_provider_route(routes, pr)
|
||||
```
|
||||
|
||||
### Per-case helpers
|
||||
|
||||
**`_merge_at_index`** — dispatches on whether the existing route already
|
||||
carries auth:
|
||||
|
||||
```python
|
||||
def _merge_at_index(
|
||||
routes: list[EgressRoute], idx: int, route: EgressRoute, pr: EgressRoute,
|
||||
) -> list[EgressRoute]:
|
||||
if route.auth_scheme or route.token_ref:
|
||||
return _merge_authed(routes, idx, route, pr)
|
||||
return _upgrade_bare(routes, idx, route, pr)
|
||||
```
|
||||
|
||||
**`_merge_authed`** — existing route has auth; either no-op/tls-upgrade
|
||||
or conflict-die:
|
||||
|
||||
```python
|
||||
def _merge_authed(
|
||||
routes: list[EgressRoute], idx: int, route: EgressRoute, pr: EgressRoute,
|
||||
) -> list[EgressRoute]:
|
||||
"""Precondition: route.auth_scheme or route.token_ref is set."""
|
||||
if route.auth_scheme != pr.auth_scheme or route.token_ref != pr.token_ref:
|
||||
die(...) # case 5
|
||||
if pr.tls_passthrough and not route.tls_passthrough:
|
||||
routes[idx] = dataclasses.replace(route, tls_passthrough=True) # case 4
|
||||
return routes # case 3 (implicit no-op)
|
||||
```
|
||||
|
||||
**`_upgrade_bare`** — existing route has no auth; adopt provider auth
|
||||
and preserve `path_allowlist` + `roles`:
|
||||
|
||||
```python
|
||||
def _upgrade_bare(
|
||||
routes: list[EgressRoute], idx: int, route: EgressRoute, pr: EgressRoute,
|
||||
) -> list[EgressRoute]:
|
||||
"""Precondition: route has no auth_scheme or token_ref."""
|
||||
token_env = _find_or_alloc_token_env(routes, pr.token_ref) if pr.auth_scheme else ""
|
||||
routes[idx] = dataclasses.replace(
|
||||
route,
|
||||
auth_scheme=pr.auth_scheme,
|
||||
token_env=token_env,
|
||||
token_ref=pr.token_ref,
|
||||
tls_passthrough=pr.tls_passthrough,
|
||||
)
|
||||
return routes
|
||||
```
|
||||
|
||||
**`_append_provider_route`** — host not in manifest; append a new route
|
||||
(no `path_allowlist` or `roles` to preserve):
|
||||
|
||||
```python
|
||||
def _append_provider_route(
|
||||
routes: list[EgressRoute], pr: EgressRoute,
|
||||
) -> list[EgressRoute]:
|
||||
"""Precondition: no existing route matches pr.host."""
|
||||
token_env = _find_or_alloc_token_env(routes, pr.token_ref) if pr.auth_scheme else ""
|
||||
routes.append(dataclasses.replace(pr, token_env=token_env))
|
||||
return routes
|
||||
```
|
||||
|
||||
Note: `dataclasses.replace(pr, token_env=token_env)` preserves any
|
||||
future fields added to `EgressRoute` without requiring updates here.
|
||||
|
||||
### Import
|
||||
|
||||
Add `import dataclasses` to `egress.py` (currently uses the `dataclass`
|
||||
decorator from `dataclasses` but not `dataclasses.replace`).
|
||||
|
||||
## Implementation chunks
|
||||
|
||||
1. **PRD (this commit).** Sets the design.
|
||||
2. **Refactor.** Apply the dispatch + four helpers in `egress.py`; add
|
||||
`dataclasses` import; delete the old `_merge_provider_route` body.
|
||||
3. **Tests.** Existing `TestProviderRouteMerge` suite is the acceptance
|
||||
gate — no new tests needed beyond confirming all pass.
|
||||
|
||||
## References
|
||||
|
||||
- Issue #120: Refactor `_merge_provider_route`.
|
||||
- Issue #117: Complexity hotspots — source of the finding.
|
||||
- PRD 0030: Deduplicate egress token resolution (prior egress cleanup).
|
||||
Reference in New Issue
Block a user