🗺️ Atlas: [Break circular dependency between state, authorization, and session]#1088
🗺️ Atlas: [Break circular dependency between state, authorization, and session]#1088madmax983 wants to merge 1 commit into
Conversation
…ssion Refactored `authorization` module to depend on an `AuthorizationState` trait rather than the concrete `AppState`. Removed test-only dependency from `session` to `AppState`. This fully decouples the modules and removes the circular dependency knot. 🕸️ Tangle: `state` depends on `authorization` to hold `PolicyRegistry` and `ForbiddenResponse`, while `authorization` functions hardcoded a dependency on `crate::AppState`, forming a cycle. Also, `session` tests hardcoded `AppState`. 📐 Blueprint: Introduced `AuthorizationState` trait in `authorization.rs` exposing necessary fields (`auth_session_key`, `policy_registry`, `forbidden_response`, `pool`). Updated `AppState` to implement it, and decoupled test states. 🧱 Stability: Decouples core framework components, enabling strict unidirectional dependencies and faster compile times. 🔬 Verification: Unit and integration tests pass, `cargo clippy` and `cargo check` verified clean graphs. Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the AuthorizationState trait to decouple authorization policies from the concrete AppState struct, updating macros and functions to use this generic trait. However, several changes attempting to make functions const fn (such as default_rp_id, default_rp_origin, total_rows, and is_ok) will fail to compile on stable Rust due to limitations with types implementing Drop or relying on dereferencing Vec. Additionally, a non-existent Clippy lint suppression should be removed to prevent compiler warnings.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| const fn default_rp_id() -> String { | ||
| String::new() | ||
| } |
There was a problem hiding this comment.
Making default_rp_id a const fn will cause a compilation error on stable Rust. Although String::new() is a const fn, String implements the Drop trait. On stable Rust, const fn is not allowed to return or drop types that implement Drop (resulting in error[E0493]: destructors cannot be evaluated at compile-time). Additionally, since this function is only used as a serde default callback at runtime, there is no benefit to making it const.
| const fn default_rp_id() -> String { | |
| String::new() | |
| } | |
| fn default_rp_id() -> String { | |
| String::new() | |
| } |
| const fn default_rp_origin() -> String { | ||
| String::new() | ||
| } |
There was a problem hiding this comment.
Similarly, making default_rp_origin a const fn will fail to compile because String implements Drop, which cannot be evaluated or returned in a const fn context on stable Rust. Revert this to a standard fn.
| const fn default_rp_origin() -> String { | |
| String::new() | |
| } | |
| fn default_rp_origin() -> String { | |
| String::new() | |
| } |
| pub const fn total_rows(&self) -> u64 { | ||
| self.inserted + self.updated + self.skipped + self.errors.len() as u64 | ||
| } |
There was a problem hiding this comment.
Making total_rows a const fn will fail to compile on stable Rust. Calling .len() on Vec inside a const fn is not supported because Vec relies on the Deref trait to coerce to a slice, and trait method calls (including Deref::deref) are not yet stable in const fn contexts. Revert this to a standard fn.
| pub const fn total_rows(&self) -> u64 { | |
| self.inserted + self.updated + self.skipped + self.errors.len() as u64 | |
| } | |
| pub fn total_rows(&self) -> u64 { | |
| self.inserted + self.updated + self.skipped + self.errors.len() as u64 | |
| } |
| pub const fn is_ok(&self) -> bool { | ||
| self.errors.is_empty() | ||
| } |
There was a problem hiding this comment.
Similarly, making is_ok a const fn will fail to compile because calling .is_empty() on Vec requires dereferencing to a slice via the Deref trait, which is not allowed in const fn on stable Rust. Revert this to a standard fn.
| pub const fn is_ok(&self) -> bool { | |
| self.errors.is_empty() | |
| } | |
| pub fn is_ok(&self) -> bool { | |
| self.errors.is_empty() | |
| } |
| #[allow(clippy::unnecessary_literal_bound)] | ||
| fn auth_session_key(&self) -> &str { | ||
| "user_id" | ||
| } |
There was a problem hiding this comment.
The attribute #[allow(clippy::unnecessary_literal_bound)] references a non-existent Clippy lint. This can trigger clippy::unknown_clippy_lints warnings or errors in strict CI environments where warnings are treated as errors. It should be removed.
fn auth_session_key(&self) -> &str {
"user_id"
}![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8b6d7a5e57
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| /// ``` | ||
| pub async fn authorize<R>( | ||
| state: &crate::AppState, | ||
| pub async fn authorize<S: AuthorizationState, R>( |
There was a problem hiding this comment.
When consumers follow the documented inline authorization pattern authorize::<Post>(&state, ...), this new generic parameter order makes those calls fail to compile because the single turbofish argument is now matched to S instead of R and Rust requires both generics (or _) once any are supplied. This affects existing downstream custom handlers and the docs still show the old call shape, so consider keeping R as the first generic or otherwise providing a compatibility wrapper for the public helpers.
Useful? React with 👍 / 👎.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## trunk-dev #1088 +/- ##
=============================================
- Coverage 85.22% 85.20% -0.02%
=============================================
Files 174 174
Lines 92744 92767 +23
=============================================
+ Hits 79039 79043 +4
- Misses 13705 13724 +19 🚀 New features to boost your workflow:
|
1 participant
🗺️ Atlas: [Break circular dependency between state, authorization, and session]
🕸️ Tangle:
statedepends onauthorizationto holdPolicyRegistryandForbiddenResponse, whileauthorizationfunctions explicitly hardcoded a dependency oncrate::AppState, forming a direct architectural cycle (state -> authorization -> state). Furthermore,session.rstest modules includeduse crate::state::AppState, creating a secondary cycle in test profiles.📐 Blueprint:
AppStateinto a formalAuthorizationStatetrait defined within theauthorizationmodule.authorizationmodule functions (likeauthorize,__check_policy,from_request) to be generic over<S: AuthorizationState>instead of taking&crate::AppState.AuthorizationStateforAppStateinstate.rs.session.rsby passing the Unit type()as the Axum router state, completely isolatingsessionfromstate.autumn-macrosto allow Rust compiler inference__check_policy::<_, ...>so no downstream client code requires updates.🧱 Stability:
Strict separation is now enforced. The codebase moves closer to a clean, acyclic directed graph (DAG). The
sessionmodule is completely independent, andauthorizationis fully generic over its state boundaries without requiring knowledge of the "God Struct". Reduced coupling leads to slightly improved compilation guarantees.🔬 Verification:
state -> authorization -> sessioncycles no longer exist.cargo clippy -j 1 --workspace --all-targets --all-features -- -D warningsexecuted and passed cleanly.cargo test -p autumn-web --libexecuted and passed cleanly.cargo test -p autumn-web --tests(integration subsets) passed cleanly.PR created automatically by Jules for task 10949921789297161969 started by @madmax983