Fix can? for ActiveRecord relation-based conditions#890
Open
olistik wants to merge 1 commit intoCanCanCommunity:developfrom
Open
Fix can? for ActiveRecord relation-based conditions#890olistik wants to merge 1 commit intoCanCanCommunity:developfrom
olistik wants to merge 1 commit intoCanCanCommunity:developfrom
Conversation
Normalize ActiveRecord relation values in ConditionsMatcher When an ability condition uses an ActiveRecord relation such as: can :read, Article, id: Article.where(user_id: user.id).select(:id) `accessible_by` works correctly because the relation is translated to SQL, but `can?` previously failed during in-memory matching. This change converts relation values to their selected scalar values before matching so `can?` behaves consistently with `accessible_by`. Add a spec for relation-backed `id` conditions.
Author
|
Fixes #889 |
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.
This PR fixes an inconsistency between
accessible_byandcan?when an ability condition uses anActiveRecord::Relationas the condition value.Example:
Before this change:
Article.accessible_by(ability)correctly returns the expected recordsability.can?(:read, article)incorrectly returnsfalseAfter this change:
accessible_bycontinues to behave correctlycan?now returns the expected result for matching recordsProblem
accessible_byworks because ActiveRecord conditions are translated into SQL.However,
can?performs an in-memory condition match, and when the condition value is anActiveRecord::Relation, it was comparing the record attribute against relation objects rather than against the selected scalar values.This caused behavior like:
Change
This PR updates
CanCan::ConditionsMatcher#condition_match?so that when a condition value is anActiveRecord::Relation, it is normalized into the selected record values before performing the in-memory match.This makes
can?consistent withaccessible_byfor relation-backed conditions such as:Test coverage
Added a spec in:
The example verifies that:
accessible_byreturns the expected matching recordcan?returnstruefor the matching recordcan?returnsfalsefor a non-matching recordWhy this matters
Using subqueries / relation-backed conditions is already a valid and useful pattern in ability definitions, especially when trying to avoid eager-loading IDs into Ruby arrays.
This change ensures that:
accessible_by)can?)follow the same logic and produce consistent results.
Notes for reviewers
This change is intentionally narrow and only affects in-memory condition matching when a condition value is an
ActiveRecord::Relation.It does not change SQL generation or
accessible_bybehavior, which was already correct.