Post-merge cleanup for #37: zero-alloc leak, doc honesty, prune aliases + redundant example#38
Merged
Merged
Conversation
Three follow-ups from a first-principles review of what landed: - Fix residual zero-alloc leak at checker.rs:544 in evaluate_batch_in_session_by. The batch grant path was building `Cow::Owned(policy_type.to_string())` from a `Cow<'static, str>` that was already in scope. Replace with `policy_type.clone()` — free for `Cow::Borrowed`, single clone for `Cow::Owned`, never the unconditional `String` allocation the previous code forced. - Correct the EvalCtx::policy_type rustdoc on dynamic-name allocation cost. The previous doc claimed dynamic names paid "one allocation" and that `ctx.grant`/`deny` cloning was "the same per-call cost as today." Tracing the helper path: the checker calls `policy.policy_type()` (alloc 1), clones the Cow into the EvalCtx (alloc 2, because cloning Cow::Owned clones the String), and `ctx.grant`/`deny` clones it again into the result (alloc 3). Static names stay zero-alloc; dynamic names pay 3. Rewrite the doc to match reality. - Delete the deprecated `*_with_context_in_session_by` aliases. Deprecation grace cycles are a v1+ accommodation. Pre-1.0, the rename is a clean break — keeping aliases adds doc/clippy noise for callers we shouldn't be optimizing for. - Delete `examples/list_scope_two_checkers.rs`. The two-checker recipe lives in `PermissionChecker`'s "Modeling list/scope endpoints" rustdoc section, where adopters encounter it organically. The separate example file is duplication — twice the maintenance, twice the drift risk, no net adopter win. CHANGELOG `[Unreleased]` entries adjusted accordingly.
![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: fa73df1019
ℹ️ 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".
| - `Policy::policy_type` return type changed from `&str` to `Cow<'static, str>`. Built-in policies return `Cow::Borrowed("Name")` and pay zero allocations. Migrate downstream policies with one line per impl: `fn policy_type(&self) -> Cow<'static, str> { Cow::Borrowed("MyPolicy") }`. Dynamic-name policies return `Cow::Owned(self.name.clone())` — the same per-call allocation cost as before. | ||
| - `EvalCtx` and `BatchEvalCtx` gain a `policy_type: Cow<'static, str>` field, captured once per evaluation by the checker (and by combinators when they fan out). Custom `Policy` impls and tests that build these directly need to populate it. | ||
| - `PermissionChecker::evaluate_batch_with_context_in_session_by` renamed to `evaluate_batch_in_session_by_resource`; `filter_authorized_with_context_in_session_by` renamed to `filter_authorized_in_session_by_resource`. The new `_by_resource` suffix mirrors the existing `_by` (per-item `(R, C)`) and makes the distinguishing axis explicit. The old names remain as `#[deprecated(since = "0.3.0-alpha.3")]` thin delegates for one alpha cycle. | ||
| - `PermissionChecker::evaluate_batch_with_context_in_session_by` renamed to `evaluate_batch_in_session_by_resource`; `filter_authorized_with_context_in_session_by` renamed to `filter_authorized_in_session_by_resource`. The new `_by_resource` suffix mirrors the existing `_by` (per-item `(R, C)`) and makes the distinguishing axis explicit. No deprecation aliases — pre-1.0, the rename is a clean break. |
There was a problem hiding this comment.
After this change the *_with_context_in_session_by aliases no longer exist, but the Batch List Endpoints section in MIGRATION.md still tells users to call filter_authorized_with_context_in_session_by. Anyone following the migration guide against this version will hit a missing-method compile error; update that snippet to filter_authorized_in_session_by_resource (and any analogous evaluate_* references) or keep the alias until the docs are migrated.
Useful? React with 👍 / 👎.
Contributor
There was a problem hiding this comment.
Pull request overview
Post-merge cleanup following #37 to tighten allocation behavior, align documentation with actual runtime costs, and reduce API/example surface area ahead of 1.0.
Changes:
- Fix a forced
Stringallocation in the batch-grant path by cloning an existingCow<'static, str>instead of callingto_string(). - Update
EvalCtx::policy_typerustdoc to accurately describe dynamic policy-type allocation behavior. - Remove pre-1.0 deprecated method aliases and delete a redundant example file; reflect the breaking change in the changelog.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/policy.rs |
Updates EvalCtx::policy_type rustdoc to match actual allocation behavior for Cow::Owned. |
src/checker.rs |
Avoids unconditional allocation in batch grant (policy_type.clone()), and removes deprecated alias methods. |
examples/list_scope_two_checkers.rs |
Removes a duplicated example now covered by rustdoc. |
CHANGELOG.md |
Updates breaking-change notes to reflect removal of deprecation aliases. |
Comments suppressed due to low confidence (1)
src/checker.rs:662
- With the deprecated alias removed, the migration guide still references
filter_authorized_with_context_in_session_by(seeMIGRATION.md:164-174). That doc snippet should be updated to the newfilter_authorized_in_session_by_resourcename so downstream users don’t copy/paste an API that no longer exists.
/// Returns only authorized items with a shared context.
///
/// Filter analogue of [`Self::evaluate_batch_in_session_by_resource`].
pub async fn filter_authorized_in_session_by_resource<I, F>(
&self,
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+8
to
+10
| - `Policy::policy_type` return type changed from `&str` to `Cow<'static, str>`. Built-in policies return `Cow::Borrowed("Name")` and pay zero allocations. Migrate downstream policies with one line per impl: `fn policy_type(&self) -> Cow<'static, str> { Cow::Borrowed("MyPolicy") }`. Dynamic-name policies return `Cow::Owned(self.name.clone())` — the same per-call allocation cost as before. | ||
| - `EvalCtx` and `BatchEvalCtx` gain a `policy_type: Cow<'static, str>` field, captured once per evaluation by the checker (and by combinators when they fan out). Custom `Policy` impls and tests that build these directly need to populate it. | ||
| - `PermissionChecker::evaluate_batch_with_context_in_session_by` renamed to `evaluate_batch_in_session_by_resource`; `filter_authorized_with_context_in_session_by` renamed to `filter_authorized_in_session_by_resource`. The new `_by_resource` suffix mirrors the existing `_by` (per-item `(R, C)`) and makes the distinguishing axis explicit. The old names remain as `#[deprecated(since = "0.3.0-alpha.3")]` thin delegates for one alpha cycle. | ||
| - `PermissionChecker::evaluate_batch_with_context_in_session_by` renamed to `evaluate_batch_in_session_by_resource`; `filter_authorized_with_context_in_session_by` renamed to `filter_authorized_in_session_by_resource`. The new `_by_resource` suffix mirrors the existing `_by` (per-item `(R, C)`) and makes the distinguishing axis explicit. No deprecation aliases — pre-1.0, the rename is a clean break. |
Post-merge review on #38 caught three drift points: - MIGRATION.md still pointed at `filter_authorized_with_context_in_session_by`, which #38 removed. Updated to `filter_authorized_in_session_by_resource` (the post-rename name) and noted the rename in passing so readers coming from an early 0.3-alpha can map old → new. Added pointers to the sibling `_by` / `evaluate_batch_*` entrypoints. - MIGRATION.md's 0.3 examples still showed `fn policy_type(&self) -> &str` and direct `PolicyEvalResult::{Granted, Denied}` struct literals with `String` policy types. Neither compiles against current 0.3 (the trait returns `Cow<'static, str>` and the variants store `Cow<'static, str>`). Rewrote both the static-name and dynamic-name examples to use the current trait shape plus the `ctx.grant` / `ctx.deny` shortcuts that replace the manual struct-literal pattern. - MIGRATION.md's Checker Calls section now leads with `PermissionChecker::check` for the RBAC/ABAC-only case (the simplest migration target) and points fact-backed callers at `evaluate_in_session` with a real session-builder example. - `EvalCtx::policy_type` rustdoc previously suggested an escape hatch: "hand-build the PolicyEvalResult and move the name into it without going through the shortcut." That isn't actually achievable from a `&EvalCtx` — the cloned `Cow` is behind a shared reference and cannot be moved out, so hand-building either requires another `policy_type()` call or another clone. Rewrote to recommend the only actually-zero-alloc path: return a `Cow::Borrowed` from a `'static` name table.
Both the `Policy::policy_type` trait rustdoc and the CHANGELOG entry for the `&str` → `Cow<'static, str>` return-type change claimed dynamic-name policies pay "the same per-call cost as before." That isn't true: under the pre-`Cow` trait, a dynamic policy could return `&self.name` from `policy_type()` with zero allocation. Now it has to return `Cow::Owned(self.name.clone())`, which is one allocation just at the trait call — plus the two downstream helper-path clones we already document on `EvalCtx::policy_type`. Both spots now say so plainly, cross-link the `EvalCtx::policy_type` accounting, and recommend a `'static` name table when allocation cost matters.
hardbyte
added a commit
that referenced
this pull request
Jun 1, 2026
Four follow-ups continuing the 0.3 pre-release sweep. **MSRV pin (1.82).** `rust-version` was unset in Cargo.toml, so clippy's incompatible_msrv lint never fired. Pin to 1.82 (the floor needed by `Option::is_none_or`, which the new per-axis builder uses). The pin immediately caught three `u128::is_multiple_of` calls (1.87 stdlib) in test code, replaced with idiomatic `% 2 == 0`. Without the pin, an unrelated bump in MSRV could land silently. **Post-merge docs audit.** Sweep across README, MIGRATION.md, lib.rs crate docs, and CHANGELOG for stale references after #37/#38/#39. The only `_with_context_` and `evaluate_access` references that survive are the legitimate ones in MIGRATION.md (0.2 → 0.3 mapping). No stale example file pointers, no broken cross-references. **EvalCtx::policy_type rustdoc precision.** The doc said "captured once per evaluation by the checker" — literally true for the single-item path after the move-into-ctx fix in #39, but the batch path captures once *per chunk* into each BatchEvalCtx. Tightened the opening sentence to name the single-item vs batch distinction explicitly, with method-link references on both sides. Also updated the `Policy::policy_type` trait doc's allocation count to match (was "plus two more on the helper path", now "plus one more on the single-item helper path; the batch path also clones into each BatchEvalCtx chunk"). **Bench-backed numbers for the builder batch shortcut.** Added a Criterion bench `policy_builder_subject_only_batch` that compares three shapes on the same N-item batch: (1) PolicyBuilder with the new evaluate_batch override, (2) a hand-written dynamic-name Policy using the serial-loop default (apples-to-apples — same allocation shape), (3) a hand-written static-name Policy (the floor). Numbers under criterion --quick on this machine: N=1: 486 ns vs 558 ns vs 483 ns → builder 13% faster N=10: 1.65 µs vs 2.01 µs vs 1.33 µs → builder 18% faster N=25: 3.49 µs vs 3.70 µs vs 2.64 µs → builder 6% faster N=100: 11.30 µs vs 16.55 µs vs 9.69 µs → builder 32% faster The optimization is genuinely measurable in throughput for dynamic-name policies — not just a trace-volume win. The gain grows with batch size, as expected: the broadcast skips N-1 closure calls and N-1 ctx.grant/deny constructions. Static-name hand-written policies remain fastest; the CHANGELOG now nudges adopters toward `'static` name tables explicitly.
hardbyte
added a commit
that referenced
this pull request
Jun 1, 2026
…coverability docs (#39) * alpha.3 follow-ups: builder batch shortcut, alloc fix, FactSource docs Four substantive improvements aimed at tightening alpha.3 before the 0.3 release. **PolicyBuilder per-axis batch shortcut.** InternalPolicy now retains the per-axis predicates separately rather than collapsing them into one closure at build time. The new `evaluate_batch` impl tests the batch-shared axes (`.subjects()` and `.actions()`) once for the whole batch — if either rejects, broadcasts the denial to every item rather than re-running N closures and emitting N trace leaves. Per-item axes (`.resources()`, `.context()`, `.when()`) still run per item. The substantive win is reduced `gatehouse.batch_policy` trace volume for policies whose discriminator is subject- or action-only (Partly-staff checks, role-gated list policies). Four new tests cover the short-circuit-on-grant, short-circuit-on-deny, mixed-axis, and empty- batch shapes, asserting predicate call counts via AtomicUsize so the "once not per-item" claim is regression-tested. **Move-into-EvalCtx allocation fix (single-item path).** The single- item `evaluate_in_session` was paying an extra `String` clone per evaluation for dynamic-name policies — `policy.policy_type()` produced a `Cow::Owned`, the checker cloned it into the `EvalCtx`, and the local was kept alive only so it could be moved into the eventual `AccessEvaluation::Granted`. Now: move `policy.policy_type()` straight into `EvalCtx`, read `&str` for tracing through `ctx.policy_type.as_ref()`, and destructure `let EvalCtx { policy_type, .. } = ctx;` on the grant branch to extract it back out without a second clone. Static-name (`Cow::Borrowed`) policies are unchanged; dynamic-name policies drop from 3 helper-path allocations to 2. The batch path is structurally different (the local outlives multiple per-chunk `BatchEvalCtx`s) and keeps its clones — accounted for in the rustdoc. **FactSource discoverability docs.** Two changes targeting the failure mode where downstream authors call `Arc<dyn HierarchyService>` directly from `evaluate`, paying N redundant lookups per list batch instead of deduping via the session: (1) FactSource rustdoc gains a `(subject, scope) → resolved-id` example showing the trait is not relationship-shaped — any per-request lookup whose answer is fixed for the request can be a fact key, and the session guarantees one round trip per unique key per request regardless of how many policies or items consult it; (2) `Policy::evaluate` rustdoc gains a one-liner signposting "if your body does I/O whose result depends on subject- derived data but not resource, register a FactSource and consume via `ctx.session.get(key).await`." **`evaluate_batch` design-intent docs.** One paragraph in the trait rustdoc making the serial default explicit ("the trait cannot know your concurrency budget; N policies × M items through `join_all` can easily exhaust connection pools") and pointing at the actual override shapes — `join_all` for small fixed fan-outs, `FuturesUnordered` for streaming, semaphore-bounded for pool-aware throughput. No new sibling method; documentation is the right answer here. CHANGELOG `[Unreleased]` entries adjusted for all four changes, including the corrected dynamic-name allocation accounting (was "two more on the helper path", now "one more single-item, more on batch"). * alpha.3 polish: MSRV pin, doc precision, bench-backed numbers Four follow-ups continuing the 0.3 pre-release sweep. **MSRV pin (1.82).** `rust-version` was unset in Cargo.toml, so clippy's incompatible_msrv lint never fired. Pin to 1.82 (the floor needed by `Option::is_none_or`, which the new per-axis builder uses). The pin immediately caught three `u128::is_multiple_of` calls (1.87 stdlib) in test code, replaced with idiomatic `% 2 == 0`. Without the pin, an unrelated bump in MSRV could land silently. **Post-merge docs audit.** Sweep across README, MIGRATION.md, lib.rs crate docs, and CHANGELOG for stale references after #37/#38/#39. The only `_with_context_` and `evaluate_access` references that survive are the legitimate ones in MIGRATION.md (0.2 → 0.3 mapping). No stale example file pointers, no broken cross-references. **EvalCtx::policy_type rustdoc precision.** The doc said "captured once per evaluation by the checker" — literally true for the single-item path after the move-into-ctx fix in #39, but the batch path captures once *per chunk* into each BatchEvalCtx. Tightened the opening sentence to name the single-item vs batch distinction explicitly, with method-link references on both sides. Also updated the `Policy::policy_type` trait doc's allocation count to match (was "plus two more on the helper path", now "plus one more on the single-item helper path; the batch path also clones into each BatchEvalCtx chunk"). **Bench-backed numbers for the builder batch shortcut.** Added a Criterion bench `policy_builder_subject_only_batch` that compares three shapes on the same N-item batch: (1) PolicyBuilder with the new evaluate_batch override, (2) a hand-written dynamic-name Policy using the serial-loop default (apples-to-apples — same allocation shape), (3) a hand-written static-name Policy (the floor). Numbers under criterion --quick on this machine: N=1: 486 ns vs 558 ns vs 483 ns → builder 13% faster N=10: 1.65 µs vs 2.01 µs vs 1.33 µs → builder 18% faster N=25: 3.49 µs vs 3.70 µs vs 2.64 µs → builder 6% faster N=100: 11.30 µs vs 16.55 µs vs 9.69 µs → builder 32% faster The optimization is genuinely measurable in throughput for dynamic-name policies — not just a trace-volume win. The gain grows with batch size, as expected: the broadcast skips N-1 closure calls and N-1 ctx.grant/deny constructions. Static-name hand-written policies remain fastest; the CHANGELOG now nudges adopters toward `'static` name tables explicitly.
Merged
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.
Fast follow-up to #37, from a first-principles review of what landed.
Fixes
checker.rs:544 zero-alloc leak. The batch grant path in
evaluate_batch_in_session_bywas buildingCow::Owned(policy_type.to_string())from aCow<'static, str>already in scope. Replaced withpolicy_type.clone()— free forCow::Borrowed, single clone forCow::Owned, never the unconditionalStringallocation the previous code forced. Caught by post-merge review.EvalCtx::policy_type rustdoc undercounted dynamic-name allocations. Claimed "one allocation" + "ctx.grant/deny ... the same per-call cost as today." Actual count on the helper path:
policy.policy_type()(alloc 1) → clone into EvalCtx (alloc 2 — cloningCow::Ownedclones theString) → clone into result viactx.grant/deny(alloc 3). Static names stay zero-alloc; dynamic names pay 3. Rewrote the rustdoc to match reality.Removals
Drop
#[deprecated]aliases forevaluate_batch_with_context_in_session_by/filter_authorized_with_context_in_session_by. Deprecation grace cycles are a v1+ accommodation — they exist to give stable-API users time to migrate without breakage. Pre-1.0, the rename is a clean break. Carrying 4 method names where we need 2 just adds clippy and doc noise.Delete
examples/list_scope_two_checkers.rs(~300 lines). The two-checker recipe is already inPermissionChecker's "Modeling list/scope endpoints" rustdoc section, where adopters encounter it organically. The separate example file is duplication — twice the maintenance, twice the drift risk, no net win.Kept
The
Cow<'static, str>trait change onPolicy::policy_typestays. It's the most invasive change in #37, but pre-1.0 is the right time to do it if we're going to do it at all, and the static-name zero-alloc win is real for the built-ins and any consumer policy with a fixed name.Validation
cargo fmt --all --check✅cargo clippy --all-targets --all-features -- -D warnings✅cargo test --all-targets --all-features✅cargo test --doc --all-features✅