Skip to content

Conversation

@dagregi
Copy link
Contributor

@dagregi dagregi commented Dec 30, 2025

Prepare


Description

Target issue

closes #12900

Implementation Details


Test and Document the changes

  • Static code analysis has been run locally and issues have been fixed
  • Relevant unit and integration tests have been added/updated
  • Relevant documentation has been updated if any (i.e. user guides, installation and configuration guides, technical design docs etc)

Please check the below before submitting your PR. The PR will not be merged if there are no commits that start with docs: to indicate documentation changes or if the below checklist is not selected.

  • I confirm that there is no impact on the docs due to the code changes in this PR.

Summary by CodeRabbit

  • Tests

    • Enhanced authorization benchmark validation with improved request handling.
    • Updated test token issuer configuration in benchmark tests.
  • Refactor

    • Simplified request preparation and error handling in benchmark utilities.
    • Streamlined benchmark workflow for clearer validation flow.

✏️ Tip: You can customize this high-level summary in your review settings.

@dagregi dagregi self-assigned this Dec 30, 2025
@dagregi dagregi added the comp-jans-cedarling Touching folder /jans-cedarling label Dec 30, 2025
@mo-auto
Copy link
Member

mo-auto commented Dec 30, 2025

Snyk checks have passed. No issues have been found so far.

Status Scanner Critical High Medium Low Total (0)
Open Source Security 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 30, 2025

📝 Walkthrough

Walkthrough

This PR modifies benchmark functions in the Cedarling authorization library to fix failing benchmarks. It refactors request builder functions from returning Results to returning concrete Request objects, updates issuer URLs for correct JWT validation, introduces pre-benchmark authorization validation, and adjusts function signatures to accept references instead of owned values.

Changes

Cohort / File(s) Summary
Benchmark request builders and validation
cedarling/benches/authz_authorize_benchmark.rs
Refactored prepare_cedarling_request_for_without_jwt_validation() to return Request directly instead of Result; refactored prepare_cedarling_request_for_with_jwt_validation() to accept &KeyPair and issuer_url: &str, returning Request directly; introduced new async helper validate_cedarling_works(); updated issuer URLs from admin-ui-test.gluu.org to test.jans.org; adjusted all call sites to work with non-fallible constructors and added pre-benchmark validation checks

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely summarizes the main change: fixing benchmarks in jans-cedarling to work correctly, which aligns with the primary objective of addressing benchmark issues.
Description check ✅ Passed The PR description follows the template with required sections completed: target issue linked (#12900), preparation checklist checked, and documentation impact confirmed. However, implementation details section is empty despite the fix being non-trivial.
Linked Issues check ✅ Passed The changes directly address issue #12900 by fixing validator initialization issues in the benchmark by adding validation checks and correcting token issuer configuration from admin-ui-test.gluu.org to test.jans.org.
Out of Scope Changes check ✅ Passed All changes are scoped to fixing the authz_authorize_benchmark.rs file's validator and token configuration issues. The updates to function signatures and the addition of validation checks are directly necessary to address the benchmark problems identified in issue #12900.
✨ Finishing touches
  • 📝 Generate docstrings

📜 Recent review details

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 76a1d9a and 32bfc51.

📒 Files selected for processing (1)
  • jans-cedarling/cedarling/benches/authz_authorize_benchmark.rs
🧰 Additional context used
📓 Path-based instructions (1)
jans-cedarling/**/*.rs

📄 CodeRabbit inference engine (jans-cedarling/AGENTS.md)

jans-cedarling/**/*.rs: Check formatting compliance with cargo fmt --check and follow rustfmt settings in rustfmt.toml
Review clippy.toml for project-specific lint rules
Use rustfmt with project's rustfmt.toml settings for all Rust code
Enforce maximum line width of 100 characters in Rust code
Use HorizontalVertical imports layout with StdExternalCrate grouping in rustfmt.toml
Use 4-space indentation (no tabs) in all Rust code
Group imports with std/external crates first, then internal modules, using imports_granularity = "Module"
Follow existing import patterns in the codebase
Use snake_case for variables, functions, and modules in Rust
Use PascalCase for types, traits, and enums in Rust
Use SCREAMING_SNAKE_CASE for constants in Rust
Follow Rust naming conventions as established by the language
Use thiserror crate for custom error types
Prefer Result<T, E> over panics for error handling
Use derive_more crate for error derivation when needed
Include context in error messages for better debugging
Use typed-builder crate for complex struct construction
Leverage serde for serialization/deserialization in Rust
Use smol_str for string optimization where appropriate in Rust
Prefer strong typing over stringly-typed APIs in Rust
Use standard Rust docstrings without Python-style sections (avoid # Arguments, # Returns)
Document public API items with docstrings focusing on 'why' not 'what'
Include examples in docstrings for complex functionality
Keep documentation concise, focusing on explanatory content rather than obvious details
Each Rust file must contain the Apache 2.0 license header with copyright attribution to Gluu, Inc.

Files:

  • jans-cedarling/cedarling/benches/authz_authorize_benchmark.rs
🧠 Learnings (8)
📓 Common learnings
Learnt from: CR
Repo: JanssenProject/jans PR: 0
File: jans-cedarling/AGENTS.md:0-0
Timestamp: 2025-12-10T08:24:27.240Z
Learning: Run benchmarks using `cargo bench -p cedarling`
Learnt from: CR
Repo: JanssenProject/jans PR: 0
File: jans-cedarling/AGENTS.md:0-0
Timestamp: 2025-12-10T08:24:27.240Z
Learning: Applies to jans-cedarling/**/authorize_*.rs : Follow existing test patterns in authorize_*.rs files
Learnt from: CR
Repo: JanssenProject/jans PR: 0
File: jans-cedarling/AGENTS.md:0-0
Timestamp: 2025-12-10T08:24:27.240Z
Learning: Run specific test files using `cargo test -p cedarling --test authorize_unsigned` format
📚 Learning: 2025-12-10T08:24:27.240Z
Learnt from: CR
Repo: JanssenProject/jans PR: 0
File: jans-cedarling/AGENTS.md:0-0
Timestamp: 2025-12-10T08:24:27.240Z
Learning: Applies to jans-cedarling/**/authorize_*.rs : Follow existing test patterns in authorize_*.rs files

Applied to files:

  • jans-cedarling/cedarling/benches/authz_authorize_benchmark.rs
📚 Learning: 2025-12-10T08:24:27.240Z
Learnt from: CR
Repo: JanssenProject/jans PR: 0
File: jans-cedarling/AGENTS.md:0-0
Timestamp: 2025-12-10T08:24:27.240Z
Learning: Run specific test files using `cargo test -p cedarling --test authorize_unsigned` format

Applied to files:

  • jans-cedarling/cedarling/benches/authz_authorize_benchmark.rs
📚 Learning: 2025-12-10T08:24:27.240Z
Learnt from: CR
Repo: JanssenProject/jans PR: 0
File: jans-cedarling/AGENTS.md:0-0
Timestamp: 2025-12-10T08:24:27.240Z
Learning: Run benchmarks using `cargo bench -p cedarling`

Applied to files:

  • jans-cedarling/cedarling/benches/authz_authorize_benchmark.rs
📚 Learning: 2025-11-28T05:59:26.842Z
Learnt from: haileyesus2433
Repo: JanssenProject/jans PR: 12455
File: jans-cedarling/cedarling/src/jwt/mod.rs:143-145
Timestamp: 2025-11-28T05:59:26.842Z
Learning: In jans-cedarling JWT service, the local-JWKS-only configuration (no trusted issuers) loads keys but does not initialize validators. This means signed tokens will hit ValidateJwtError::MissingValidator and be skipped. This is acceptable as the intended use case is authorize_unsigned. Validator setup for JWKS-only signed authorization is planned for future implementation.

Applied to files:

  • jans-cedarling/cedarling/benches/authz_authorize_benchmark.rs
📚 Learning: 2025-12-10T08:24:27.240Z
Learnt from: CR
Repo: JanssenProject/jans PR: 0
File: jans-cedarling/AGENTS.md:0-0
Timestamp: 2025-12-10T08:24:27.240Z
Learning: Applies to jans-cedarling/**/*.rs : Leverage serde for serialization/deserialization in Rust

Applied to files:

  • jans-cedarling/cedarling/benches/authz_authorize_benchmark.rs
📚 Learning: 2025-12-10T08:24:27.240Z
Learnt from: CR
Repo: JanssenProject/jans PR: 0
File: jans-cedarling/AGENTS.md:0-0
Timestamp: 2025-12-10T08:24:27.240Z
Learning: Run main crate tests with `cargo test -p cedarling`

Applied to files:

  • jans-cedarling/cedarling/benches/authz_authorize_benchmark.rs
📚 Learning: 2025-12-10T14:10:48.131Z
Learnt from: olehbozhok
Repo: JanssenProject/jans PR: 12819
File: jans-cedarling/cedarling/src/common/policy_store/manager.rs:101-107
Timestamp: 2025-12-10T14:10:48.131Z
Learning: In all Rust source files under the jans-cedarling directory, avoid using println! or eprintln! because they do not work in WASM builds. Use the provided Logger API, e.g. logger.log(&System::log_msg(...)) to log messages. Ensure the logger is properly initialized in the WASM context and replace direct prints with the logger pattern in relevant code paths (especially in WASM targets).

Applied to files:

  • jans-cedarling/cedarling/benches/authz_authorize_benchmark.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
  • GitHub Check: python_tests (3.11)
  • GitHub Check: python_tests (3.10)
  • GitHub Check: wasm_tests
  • GitHub Check: cleanup
  • GitHub Check: rust_tests
  • GitHub Check: rust_benchmarks
  • GitHub Check: golang_tests
🔇 Additional comments (5)
jans-cedarling/cedarling/benches/authz_authorize_benchmark.rs (5)

21-34: LGTM! Validation helper addresses the reported issue.

The new validate_cedarling_works function ensures that cedarling validators are properly initialized before benchmarking, directly addressing issue #12900 where benchmarks produced "no validator was initialized" warnings.


43-45: LGTM! Pre-benchmark validation ensures accuracy.

The changes correctly integrate with the updated request builder signature and add validation before benchmarking to ensure measurements reflect actual authorization performance.


72-75: LGTM! Correctly uses mock server issuer.

The function now passes references to keys and the mock server's issuer URL, ensuring JWT validation works correctly during benchmarks.


184-272: LGTM! Simplified signature and consistent issuer URLs.

The function signature change removes unnecessary Result wrapping, simplifying call sites. The issuer URL update to test.jans.org provides consistency across the test suite.


274-365: LGTM! Improved signature enables dynamic issuer configuration.

The function now accepts references and a dynamic issuer URL, which is essential for JWT validation to work correctly with mock servers. All three tokens (access, id, userinfo) consistently use the provided issuer URL, ensuring validators can be initialized properly.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@mo-auto mo-auto added the kind-bug Issue or PR is a bug in existing functionality label Dec 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp-jans-cedarling Touching folder /jans-cedarling kind-bug Issue or PR is a bug in existing functionality

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(jans-cedarling): fix benchmarks to actually work

3 participants