-
-
Notifications
You must be signed in to change notification settings - Fork 228
fix(enforcer): add useAdapter param to avoid infinite loop in watchers #536
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Pull Request Test Coverage Report for Build 20748193976Details
💛 - Coveralls |
f08c0a4 to
e68147f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes an infinite loop issue (#506) in watchers by adding a useAdapter parameter to internal policy management methods. When set to false, this parameter prevents adapter calls, which is crucial for the self* methods that are called from watchers to avoid triggering recursive adapter updates.
Key Changes
- Added optional
useAdapterparameter (defaults totrue) to seven internal methods inInternalEnforcer - Updated all
self*methods inManagementEnforcerto passfalsefor bothuseWatcheranduseAdapterparameters - Modified adapter conditional checks to include the new
useAdapterflag
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 15 comments.
| File | Description |
|---|---|
| src/internalEnforcer.ts | Added useAdapter parameter with default value true to seven internal methods and updated adapter conditional checks to respect this flag |
| src/managementEnforcer.ts | Updated six self* methods to pass false for the new useAdapter parameter |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/internalEnforcer.ts
Outdated
| } | ||
|
|
||
| if (this.adapter && this.autoSave) { | ||
| // if (this.adapter && this.autoSave) { |
Copilot
AI
Jan 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove commented-out code. The old condition is no longer needed since the new condition with useAdapter is now in place.
src/managementEnforcer.ts
Outdated
| this.fm.addFunction(name, func); | ||
| } | ||
|
|
||
| // add fifth argument |
Copilot
AI
Jan 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment is vague and unhelpful. Consider replacing it with a more descriptive comment explaining why the useAdapter parameter is needed, such as "Pass useAdapter=false to prevent infinite loops when called from watchers".
| // add fifth argument | |
| // Wrapper around addPolicyInternal that passes useAdapter=false (final argument) | |
| // to prevent infinite update loops when invoked from watchers. |
src/internalEnforcer.ts
Outdated
| protected async addPoliciesInternal( | ||
| sec: string, | ||
| ptype: string, | ||
| rules: string[][], | ||
| useWatcher: boolean, | ||
| useAdapter = true | ||
| ): Promise<boolean> { |
Copilot
AI
Jan 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add JSDoc parameter documentation for the new useAdapter parameter to explain its purpose. For example: "@param useAdapter whether to use the adapter for persistence (set to false to avoid infinite loops in watchers)".
src/internalEnforcer.ts
Outdated
| protected async addPoliciesInternalEx( | ||
| sec: string, | ||
| ptype: string, | ||
| rules: string[][], | ||
| useWatcher: boolean, | ||
| useAdapter = true | ||
| ): Promise<boolean> { |
Copilot
AI
Jan 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add JSDoc parameter documentation for the new useAdapter parameter to explain its purpose. For example: "@param useAdapter whether to use the adapter for persistence (set to false to avoid infinite loops in watchers)".
src/internalEnforcer.ts
Outdated
| protected async removePoliciesInternal( | ||
| sec: string, | ||
| ptype: string, | ||
| rules: string[][], | ||
| useWatcher: boolean, | ||
| useAdapter = true | ||
| ): Promise<boolean> { |
Copilot
AI
Jan 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add JSDoc parameter documentation for the new useAdapter parameter to explain its purpose. For example: "@param useAdapter whether to use the adapter for persistence (set to false to avoid infinite loops in watchers)".
| */ | ||
| export class InternalEnforcer extends CoreEnforcer { | ||
| /** | ||
| * addPolicyInternal adds a rule to the current policy. |
Copilot
AI
Jan 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add JSDoc parameter documentation for the new useAdapter parameter to explain its purpose. For example: "@param useAdapter whether to use the adapter for persistence (set to false to avoid infinite loops in watchers)".
| * addPolicyInternal adds a rule to the current policy. | |
| * addPolicyInternal adds a rule to the current policy. | |
| * | |
| * @param useAdapter whether to use the adapter for persistence (set to false to avoid infinite loops in watchers) |
src/internalEnforcer.ts
Outdated
| protected async removePolicyInternal( | ||
| sec: string, | ||
| ptype: string, | ||
| rule: string[], | ||
| useWatcher: boolean, | ||
| useAdapter = true | ||
| ): Promise<boolean> { |
Copilot
AI
Jan 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add JSDoc parameter documentation for the new useAdapter parameter to explain its purpose. For example: "@param useAdapter whether to use the adapter for persistence (set to false to avoid infinite loops in watchers)".
src/internalEnforcer.ts
Outdated
| } | ||
|
|
||
| if (this.adapter && this.autoSave) { | ||
| // if (this.adapter && this.autoSave) { |
Copilot
AI
Jan 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove commented-out code. The old condition is no longer needed since the new condition with useAdapter is now in place.
| // if (this.adapter && this.autoSave) { |
src/internalEnforcer.ts
Outdated
| } | ||
|
|
||
| if (this.autoSave) { | ||
| // if (this.autoSave) { |
Copilot
AI
Jan 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove commented-out code. The old condition is no longer needed since the new condition with useAdapter is now in place.
| // if (this.autoSave) { |
src/internalEnforcer.ts
Outdated
| useAdapter = true | ||
| ): Promise<boolean> { | ||
| if (this.adapter && this.autoSave) { | ||
| // if (this.adapter && this.autoSave) { |
Copilot
AI
Jan 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove commented-out code. The old condition is no longer needed since the new condition with useAdapter is now in place.
e68147f to
75165bd
Compare
75165bd to
c64e7a6
Compare
|
replaced by: #539 |
fix #506