Repository navigation
Conversation
added 3 commits
September 24, 2026 10:12
RoleManager.add_link only walks all_roles to apply pattern-derived links when a matching function is set, but delete_link walked it unconditionally. With matching_func = None, every iteration called match_error_handler(None, ...), which raised and swallowed a TypeError, so the result was correct but each delete was O(roles) wasted work. The cost multiplies with a domain matching function on g (e.g. `g, user, group, *` with key_match), because DomainManager forwards the delete to every cached domain's role manager. Guard the loop with the same check add_link uses. ConditionalRoleManager, DomainManager and ConditionalDomainManager all delegate to this method, so they are covered too. Add tests that check delete_link behavior without a matching function, both on its own and across cached domains.
RoleManager.add_link only walks all_roles to apply pattern-derived links when a matching function is set, but delete_link walked it unconditionally. With matching_func = None, every iteration called match_error_handler(None, ...), which raised and swallowed a TypeError, so the result was correct but each delete was O(roles) wasted work. The cost multiplies with a domain matching function on g (e.g. `g, user, group, *` with key_match), because DomainManager forwards the delete to every cached domain's role manager. Guard the loop with the same check add_link uses. ConditionalRoleManager, DomainManager and ConditionalDomainManager all delegate to this method, so they are covered too. Add tests that check delete_link behavior without a matching function, both on its own and across cached domains.
Member
|
replaced by: b7c36d9 |
Coverage Report for CI Build 35975753290Warning Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes. Coverage decreased (-0.05%) to 84.172%Details
Uncovered Changes
Coverage Regressions2 previously-covered lines in 1 file lost coverage.
Coverage Stats
💛 - Coveralls |
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.
RoleManager.add_linkonly walksall_rolesto apply pattern-derived links when a matching function is set, butdelete_linkwalked it unconditionally. Withmatching_func = None, every iteration calledmatch_error_handler(None, ...), which raised and swallowed a TypeError, so the result was correct but each delete was O(roles) wasted work.The cost multiplies with a domain matching function on g (e.g.
g, user, group, *withkey_match), becauseDomainManagerforwards the delete to every cached domain's role manager.Guard the loop with the same check add_link uses.
ConditionalRoleManager,DomainManagerandConditionalDomainManagerall delegate to this method, so they are covered too.Add tests that check
delete_linkbehavior without a matching function, both on its own and across cached domains.Also correct
!= Nonetois not None(PEP8)