refactor(server): validate the namespace once and split the sweep out of the store - #35
Merged
Merged
Conversation
BryanFRD
enabled auto-merge (squash)
August 14, 2026 12:17
There was a problem hiding this comment.
Reviewed the diff end to end. Clean mechanical refactor, matches the description:
authorizemiddleware now inserts the validatedNamespaceinto request extensions; every handler that needs it (batch,upload,download,verify,retain) sits behindroute_layer(authorize)on theobjectsrouter, soExtension<Namespace>can never be missing at runtime.Namespacedropping its lifetime and gainingDisplayis applied consistently — checked all formerNamespace<'_>call sites (auth.rs, cache.rs, github.rs, config.rs, storage, routes.rs) and none were missed.upload/download'sPath((.., oid)): Path<(String, String, String)>is valid and still extracts the right positional segment now that org/repo come from the extension instead.storage/sweep.rsreachingLocalStore's private fields from a child module is legitimate (module-tree privacy), so the split costs no encapsulation as claimed.
Nothing blocking. No nits either — this is a tidy, low-risk refactor.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No behaviour change. Same 30 tests, same e2e, unchanged — which is the point: if any of this altered what the server does, they would say so.
The namespace was validated twice
The middleware parsed
{org}/{repo}, validated it, and threw it away. Every handler then re-read the path parameters and calledNamespace::newagain, so the same validation ran twice per request and each new handler had to remember to do it — a rule enforced by discipline rather than by the compiler.The middleware now puts the validated
Namespacein the request extensions and handlers take it asExtension<Namespace>. A handler cannot run without one, because the only thing that constructs it is the middleware that guards those routes.retainloses three lines,downloadloses its whole body prologue.Namespacealso drops its lifetime parameter and owns its two strings. The borrow existed to avoid two allocations per request, which is not a real saving next to streaming multi-gigabyte objects, and it was threaded through five modules. It gained aDisplay(org/repo), which is whatconfig,githubandcachewere each formatting by hand — the GitHub URL is now{api_url}/repos/{ns}.The store was doing two jobs
storage.rshad grown to 230 lines covering both the transfer path (write, verify, read) and collection (walk, age, delete, prune). Those change for different reasons and will keep diverging — #8, #26 and #32 all land on the maintenance side.Split into
storage/mod.rs(146 lines) andstorage/sweep.rs(100), withSweepReportmoving next to the code that produces it. A child module reaches the parent's private fields, soLocalStorekeeps its encapsulation and the split costs nothing at the call site.Left alone deliberately
Moving the signal handling out of
main.rswould have collided with #33, which is open and touches exactly those lines. Not worth a conflict for a 30-line file.