feat(core): integrate Detector into DefaultRoleManager with post-load cycle detection

[Copilot]

Plan for integrating Detector into DefaultRoleManager

• Add optional Detector field to DefaultRoleManager
  • Add private detector field
  • Add setDetector(Detector detector) setter method
• Implement post-load cycle detection strategy
  • addLink method simply adds links without automatic cycle checking
  • Add public checkCycles() method for manual cycle detection
  • Maintain idempotency check in addLink to prevent duplicate links
  • Add synchronization to addLink to prevent race conditions
• Integrate automatic cycle detection after policy loading
  • Override loadPolicy() in ManagementEnforcer to call checkCycles() on all DefaultRoleManager instances after loading
  • Simplify rebuild() to call checkCycles() at the end
• Create DetectorTest.java test file
  • Test valid inheritance chains with checkCycles() (no exceptions)
  • Test cycle creation via addLink (links added without exception)
  • Test checkCycles() throws exception when cycles exist
  • Add boundary tests (self-loop, complex graphs)
  • Add idempotency tests
  • Ensure tests use only DefaultRoleManager and DefaultDetector, not Enforcer
• Run tests to verify all functionality works correctly
• Address code review feedback
• Run security checks (CodeQL passed with 0 alerts)
• Final validation (all tests pass)

Summary

✅ All requirements met:

• Minimal intrusion: Only added necessary methods
• Loose coupling: Detector is optional (default null)
• Backward compatible: Existing API unchanged
• Idempotent: Adding existing links is safe
• Thread-safe: addLink method is synchronized
• Performance optimized: O(N) post-load check instead of O(N²) real-time checks
• Automatic detection: loadPolicy() in ManagementEnforcer automatically calls checkCycles() after loading
• Manual detection: Public checkCycles() method available for explicit cycle checking
• Comprehensive tests: 13 test cases in DetectorTest.java
• No Enforcer dependency in tests
• All tests pass (220 total)
• No security vulnerabilities
• Code review feedback addressed

Implementation Details

Cycle Detection Strategy:

• addLink() adds links without performing cycle checks (O(1) per call)
• checkCycles() performs a single graph traversal to detect cycles (O(N) total)
• ManagementEnforcer.loadPolicy() automatically calls checkCycles() after policy is loaded
• Applications can also call checkCycles() manually when needed

This approach provides significant performance improvement during bulk policy loading (O(N) instead of O(N²)) while maintaining the same cycle detection capabilities.

Original prompt

---

This section details on the original issue you should resolve

<issue_title>feat(core): integrate Detector into DefaultRoleManager and add unit tests</issue_title>
<issue_description>Background:
We need to add a hook point for cycle detection into jcasbin, while following the principles of “minimal intrusion” and “loose coupling.” DefaultRoleManager’s addLink method is the only place that updates the inheritance relationship
github.com
.

Tasks:

1. Add an optional Detector field in DefaultRoleManager:

   private Detector detector;

   public void setDetector(Detector detector) {
   this.detector = detector;
   }

   The default value is null to maintain backward compatibility.

2. Modify addLink(String name1, String name2, String... domain): after successfully adding the parent-child relationship, if detector is not null, call detector.check(this).

   • If a non-empty error message is returned, immediately roll back the current addRole operation (call removeRole or delete that link), and throw an IllegalArgumentException containing the error description.

3. Create src/test/java/org/casbin/jcasbin/main/DetectorTest.java:

   • Construct a DefaultRoleManager, inject a DefaultDetector, add some valid inheritance chains, and assert that no exception is thrown.
   • Construct a chain containing a cycle, e.g., A→B→C→A, and assert that when adding the third link, an exception is thrown.
   • Assert that after rollback, the state no longer contains the illegal link.
   • If necessary, add more boundary tests (e.g., self-loop, complex graphs).

4. In tests, do not depend on Enforcer; only use DefaultRoleManager and DefaultDetector. This can speed up test execution and reduce coupling.

Constraints:

• Integration modifications must keep the existing public API unchanged. Inject detector only via a setter; do not force all users to enable detection.
• Unit tests must use JUnit 5 and ensure they pass within the project’s existing test suite.
  </issue_description>

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

• Fixes feat(core): integrate Detector into DefaultRoleManager and add unit tests #475

---

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

[CLAassistant]

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.}

[codecov-commenter]

Codecov Report

❌ Patch coverage is 94.11765% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 68.05%. Comparing base (979755b) to head (25d842a).
⚠️ Report is 5 commits behind head on master.

Files with missing lines	Patch %	Lines
...va/org/casbin/jcasbin/rbac/DefaultRoleManager.java	90.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #484      +/-   ##
==========================================
+ Coverage   67.80%   68.05%   +0.24%     
==========================================
  Files          61       63       +2     
  Lines        3069     3096      +27     
  Branches      569      578       +9     
==========================================
+ Hits         2081     2107      +26     
+ Misses        806      805       -1     
- Partials      182      184       +2     

☔ 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.

[AKonnyaku]

When addLink is called for a link that already exists, user.addRole(role) updates the map. If detector.check subsequently fails, the user.removeRole(role) call in the rollback block will delete the pre-existing valid link.

This breaks the idempotency of the operation and causes data loss. Since DefaultRoleManager shares the same package as Role, please add a check before modification:

if (user.roles.containsKey(name2)) {
    return;
}

[hsluoyz]

@copilot When addLink is called for a link that already exists, user.addRole(role) updates the map. If detector.check subsequently fails, the user.removeRole(role) call in the rollback block will delete the pre-existing valid link.

This breaks the idempotency of the operation and causes data loss. Since DefaultRoleManager shares the same package as Role, please add a check before modification:

if (user.roles.containsKey(name2)) {
    return;
}

[Copilot]

@copilot When addLink is called for a link that already exists, user.addRole(role) updates the map. If detector.check subsequently fails, the user.removeRole(role) call in the rollback blo...

Fixed in commit 53da062. Added the idempotency check as suggested:

if (user.roles.containsKey(name2)) {
    return;
}

This prevents data loss when addLink is called for an existing link. Also added comprehensive test cases to verify the fix works correctly in various scenarios.

[AKonnyaku]

@copilot

Description

The addLink method currently lacks synchronization. Since it now involves a compound operation (add -> check -> rollback), it introduces race conditions compared to other synchronized methods like hasLink.

Task:

1. Modify src/main/java/org/casbin/jcasbin/rbac/DefaultRoleManager.java.
2. Add the synchronized keyword to the addLink method signature.

[hsluoyz]

@copilot

Description

The addLink method currently lacks synchronization. Since it now involves a compound operation (add -> check -> rollback), it introduces race conditions compared to other synchronized methods like hasLink.

Task:

1. Modify src/main/java/org/casbin/jcasbin/rbac/DefaultRoleManager.java.
2. Add the synchronized keyword to the addLink method signature.

[Copilot]

@copilot

Description

...

Fixed in commit 4ecff15. Added the synchronized keyword to the addLink method signature to ensure thread safety during compound operations (check → add → check → rollback).

This aligns with the existing synchronized hasLink method and prevents race conditions in concurrent environments.

[AKonnyaku]

Please fix the performance regression in DefaultRoleManager.java.

Description:
The rebuild() method currently calls addLink in a loop. Since addLink now triggers a full graph cycle check (detector.check), the complexity of rebuild has degraded to O(N^2), causing performance issues.

Task:

1. Modify rebuild() in src/main/java/org/casbin/jcasbin/rbac/DefaultRoleManager.java.
2. Optimization Strategy:
   • Temporarily set this.detector = null before the reconstruction loop to skip the check in addLink.
   • Restore this.detector after the loop.
   • Perform a single detector.check(this) at the very end.
   • If the check fails, throw an exception.

[hsluoyz]

@copilot Please fix the performance regression in DefaultRoleManager.java.

Description:
The rebuild() method currently calls addLink in a loop. Since addLink now triggers a full graph cycle check (detector.check), the complexity of rebuild has degraded to O(N^2), causing performance issues.

Task:

1. Modify rebuild() in src/main/java/org/casbin/jcasbin/rbac/DefaultRoleManager.java.
2. Optimization Strategy:
   • Temporarily set this.detector = null before the reconstruction loop to skip the check in addLink.
   • Restore this.detector after the loop.
   • Perform a single detector.check(this) at the very end.
   • If the check fails, throw an exception.

[Copilot]

@copilot Please fix the performance regression in DefaultRoleManager.java.

Description:
...

Fixed in commit b168081. Optimized rebuild() to avoid O(N²) complexity:

• Temporarily set this.detector = null before the reconstruction loop
• Restore detector after the loop
• Perform a single detector.check(this) at the end
• Properly restore detector in exception handler

This reduces rebuild complexity from O(N²) to O(N) when detector is enabled.

[AKonnyaku]

Description:
To resolve performance issues during bulk policy loading, we need to disable the real-time cycle detection in DefaultRoleManager.addLink (which is O(N^2)) and instead perform a one-time check (O(N)) immediately after the policy is fully loaded.

Modifications Required:

1. src/main/java/org/casbin/jcasbin/rbac/DefaultRoleManager.java

• Remove Auto-Check: In addLink(), remove the detector.check(this) call and the rollback logic. addLink should simply add the relationship.
• Add Manual Check API: Add a new public method:

  public void checkCycles() {
      if (this.detector != null) {
          // Throw exception if cycle detected
          String error = this.detector.check(this);
          if (error != null) throw new IllegalArgumentException(error);
      }
  }

• Update rebuild(): Simplify it to just call checkCycles() at the end.

2. src/main/java/org/casbin/jcasbin/main/ManagementEnforcer.java

• Override loadPolicy: Since ManagementEnforcer inherits loadPolicy, please override it to inject the check hook:
  1. Call super.loadPolicy(); first.
  2. After loading, iterate through the model's grouping policies ("g", "g2", etc.).
  3. If the associated RoleManager is an instance of DefaultRoleManager, call its new checkCycles() method.
  • Note: This ensures the check runs automatically after loading, without modifying the Core Enforcer.

3. src/test/java/org/casbin/jcasbin/main/DetectorTest.java

• Update Tests:
  • addLink should no longer throw exception when a cycle is created.
  • Add a specific assertion that calling rm.checkCycles() does throw the expected exception.

Instructions:
Please implement these changes to move the cycle detection responsibility to the post-load phase.

[hsluoyz]

@copilot

Description:
To resolve performance issues during bulk policy loading, we need to disable the real-time cycle detection in DefaultRoleManager.addLink (which is O(N^2)) and instead perform a one-time check (O(N)) immediately after the policy is fully loaded.

Modifications Required:

1. src/main/java/org/casbin/jcasbin/rbac/DefaultRoleManager.java

• Remove Auto-Check: In addLink(), remove the detector.check(this) call and the rollback logic. addLink should simply add the relationship.
• Add Manual Check API: Add a new public method:

  public void checkCycles() {
      if (this.detector != null) {
          // Throw exception if cycle detected
          String error = this.detector.check(this);
          if (error != null) throw new IllegalArgumentException(error);
      }
  }

• Update rebuild(): Simplify it to just call checkCycles() at the end.

2. src/main/java/org/casbin/jcasbin/main/ManagementEnforcer.java

• Override loadPolicy: Since ManagementEnforcer inherits loadPolicy, please override it to inject the check hook:
  1. Call super.loadPolicy(); first.
  2. After loading, iterate through the model's grouping policies ("g", "g2", etc.).
  3. If the associated RoleManager is an instance of DefaultRoleManager, call its new checkCycles() method.
  • Note: This ensures the check runs automatically after loading, without modifying the Core Enforcer.

3. src/test/java/org/casbin/jcasbin/main/DetectorTest.java

• Update Tests:
  • addLink should no longer throw exception when a cycle is created.
  • Add a specific assertion that calling rm.checkCycles() does throw the expected exception.

Instructions:
Please implement these changes to move the cycle detection responsibility to the post-load phase.

[Copilot]

@copilot

Description:
...

Fixed in commit 25d842a. Moved cycle detection from real-time to post-load:

DefaultRoleManager:

• Removed auto-check from addLink() - now just adds the link
• Added public checkCycles() method for manual cycle detection
• Simplified rebuild() to call checkCycles() at the end

ManagementEnforcer:

• Overrode loadPolicy() to call checkCycles() on all DefaultRoleManager instances after loading

DetectorTest:

• Updated all tests - addLink no longer throws exceptions
• Tests now explicitly call rm.checkCycles() to verify cycle detection

This changes detection from O(N²) real-time to O(N) post-load, significantly improving bulk policy loading performance.