Skip to content

feat: remove reflection dependency and support generic RoleManager in DefaultDetector#488

Merged
hsluoyz merged 2 commits intomasterfrom
copilot/remove-reflection-dependency
Dec 30, 2025
Merged

feat: remove reflection dependency and support generic RoleManager in DefaultDetector#488
hsluoyz merged 2 commits intomasterfrom
copilot/remove-reflection-dependency

Conversation

Copy link
Contributor

Copilot AI commented Dec 29, 2025

DefaultDetector used reflection to access private fields in DefaultRoleManager and rejected non-DefaultRoleManager implementations, preventing cycle detection from working with custom RoleManager implementations (e.g., Redis, JDBC adapters).

Changes

Added getRoleGraph() to RoleManager interface

  • Returns Map<String, List<String>> adjacency list of role inheritance
  • Default implementation returns empty map for backward compatibility

Implemented getRoleGraph() in DefaultRoleManager

  • Returns defensive copy of internal role graph
  • Prevents external modification of internal state

Refactored DefaultDetector

  • Removed reflection-based buildGraph() method (39 lines)
  • Removed instanceof DefaultRoleManager check
  • Directly calls rm.getRoleGraph() to obtain graph

Updated test expectations

  • testUnsupportedRoleManager now verifies generic RoleManager support instead of expecting IllegalArgumentException

Usage

// Custom RoleManager implementations can now support cycle detection
public class RedisRoleManager implements RoleManager {
    @Override
    public Map<String, List<String>> getRoleGraph() {
        // Return role graph from Redis
        Map<String, List<String>> graph = new HashMap<>();
        // ... populate from Redis
        return graph;
    }
    // ... other methods
}

Detector detector = new DefaultDetector();
detector.check(new RedisRoleManager()); // Now works without reflection
Original prompt

This section details on the original issue you should resolve

<issue_title>[feature] Remove reflection dependency and support generic RoleManager</issue_title>
<issue_description>### Motivation

Currently, the DefaultDetector implementation relies on Java Reflection to access the private allRoles field within DefaultRoleManager. This approach is fragile and breaks encapsulation.

Additionally, the detector explicitly checks if (!(rm instanceof DefaultRoleManager)). This tight coupling prevents the cycle detection feature from being used with other RoleManager implementations (e.g., Redis, JDBC adapters), restricting the module's extensibility.

Proposed Changes

I propose refactoring the interaction between Detector and RoleManager to use a defined interface contract instead of reflection.

Tasks

1. Modify RoleManager Interface

  • File: src/main/java/org/casbin/jcasbin/rbac/RoleManager.java
  • Action: Add a new method signature to retrieve the relationship graph.
// Returns the dependency graph (adjacency list) of roles.
Map<String, List<String>> getRoleGraph();
  • Note: Provide a default implementation (e.g., throwing UnsupportedOperationException or returning an empty map) to ensure backward compatibility for other existing implementations.

2. Update DefaultRoleManager

  • File: src/main/java/org/casbin/jcasbin/rbac/DefaultRoleManager.java
  • Action: Implement the getRoleGraph() method.
  • Constraint: It MUST return a defensive copy (deep or shallow copy) of the internal allRoles map. Do not return the reference to the internal map directly to ensure thread safety and encapsulation.

3. Refactor DefaultDetector

  • File: src/main/java/org/casbin/jcasbin/detector/DefaultDetector.java
  • Action:
  • Remove the buildGraph method that contains the reflection logic.
  • Remove the instanceof DefaultRoleManager check.
  • Update the check method to call rm.getRoleGraph() directly to build the graph.

Verification

  • The existing test suite DefaultDetectorTest must pass without any modifications.
  • No reflection code should remain in the detector module.
    </issue_description>

Comments on the Issue (you are @copilot in this section)


✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

Copilot AI self-assigned this Dec 29, 2025
@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

- Added getRoleGraph() method to RoleManager interface with default implementation
- Implemented getRoleGraph() in DefaultRoleManager with defensive copy
- Removed reflection-based buildGraph() method from DefaultDetector
- Removed instanceof check to support any RoleManager implementation
- Updated test to reflect new behavior (now accepts any RoleManager)

Co-authored-by: hsluoyz <3787410+hsluoyz@users.noreply.github.com>
Copilot AI changed the title [WIP] Remove reflection dependency and support generic RoleManager Remove reflection dependency and support generic RoleManager in DefaultDetector Dec 29, 2025
Copilot AI requested a review from hsluoyz December 29, 2025 12:22
@hsluoyz hsluoyz marked this pull request as ready for review December 29, 2025 12:34
@codecov-commenter
Copy link

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 67.82%. Comparing base (979755b) to head (b090f2a).

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #488      +/-   ##
==========================================
+ Coverage   67.80%   67.82%   +0.01%     
==========================================
  Files          61       61              
  Lines        3069     3058      -11     
  Branches      569      568       -1     
==========================================
- Hits         2081     2074       -7     
+ Misses        806      802       -4     
  Partials      182      182              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@hsluoyz hsluoyz changed the title Remove reflection dependency and support generic RoleManager in DefaultDetector feat: remove reflection dependency and support generic RoleManager in DefaultDetector Dec 30, 2025
@hsluoyz hsluoyz merged commit 92b6832 into master Dec 30, 2025
1 of 3 checks passed
hsluoyz pushed a commit that referenced this pull request Dec 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[feature] Remove reflection dependency and support generic RoleManager

4 participants