Skip to content
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

Support combining scopes when loading records, use Arel instead of SQL #717

Open
wants to merge 12 commits into
base: develop
Choose a base branch
from

Conversation

mamhoff
Copy link
Contributor

@mamhoff mamhoff commented Jun 28, 2021

This refactors the ActiveRecordAdapter to use Arel/ActiveRecord to merge conditions. The advantage is that we can finally merge scopes from different abilities!

This only works with Rails versions 5.2+, so this PR would drop support for prior versions. Given that Rails 7 is about to come out, that's something at least worth considering.

I think this PR makes the ActiveRecord Adapter quite a bit more readable, which should make future contributions easier.

I know it's a big change. Thank you for your hard work on this gem.

coorasse and others added 10 commits June 21, 2021 10:45
This breaks all kinds of things for now, but will open up using the
`accessible_by` on scopes.
Rails 5.1 has been EOL for a long while, and the code hoops we'd have to
jump through to keep supporting it are really quite large.
This passes all the conditions we have into ActiveRecord's `where`
method and joins them using `or` and leaves the SQL generation to
ActiveRecord/Arel.
The conditions array is really an artefact of the old implementation and
should IMO not be tested. Let's instead test where the rubber hits the
road: Which database records are returned.
Now that we're using ActiveRecord to generate and merge conditions, we
do not need a lot of code any longer.
This extracts a lot of stuff into methods. I sure am hopeful this makes
the code more readable!
@mamhoff mamhoff force-pushed the refactor-activerecord-adapter branch from f1c244e to 4f8bc1c Compare June 28, 2021 14:36
IDs have primary key constraints, and we cannot know when our test runs
respective to other tests.
Ok, so Postgres can't order by a column that's not on the select list.
Let's put the column on the select list if that's what we want to order
by!
Comment on lines +24 to 27
if @model_class.respond_to?(:where) && @model_class.respond_to?(:joins)
build_relation
else
@model_class.all(conditions: conditions, joins: joins)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like support for old AR

Comment on lines 14 to +19
@compressed_rules = RulesCompressor.new(@rules.reverse).rules_collapsed.reverse
StiNormalizer.normalize(@compressed_rules)
ConditionsNormalizer.normalize(model_class, @compressed_rules)
end

# Returns conditions intended to be used inside a database query. Normally you will not call this
# method directly, but instead go through ModelAdditions#accessible_by.
#
# If there is only one "can" definition, a hash of conditions will be returned matching the one defined.
#
# can :manage, User, :id => 1
# query(:manage, User).conditions # => { :id => 1 }
#
# If there are multiple "can" definitions, a SQL string will be returned to handle complex cases.
#
# can :manage, User, :id => 1
# can :manage, User, :manager_id => 1
# cannot :manage, User, :self_managed => true
# query(:manage, User).conditions # => "not (self_managed = 't') AND ((manager_id = 1) OR (id = 1))"
#
def conditions
conditions_extractor = ConditionsExtractor.new(@model_class)
if @compressed_rules.size == 1 && @compressed_rules.first.base_behavior
# Return the conditions directly if there's just one definition
conditions_extractor.tableize_conditions(@compressed_rules.first.conditions).dup
else
extract_multiple_conditions(conditions_extractor, @compressed_rules)
end
end

def extract_multiple_conditions(conditions_extractor, rules)
rules.reverse.inject(false_sql) do |sql, rule|
merge_conditions(sql, conditions_extractor.tableize_conditions(rule.conditions).dup, rule.base_behavior)
ConditionsNormalizer.normalize(@model_class, @compressed_rules)
# run the extractor on the reversed set of rules to fill the cache properly
@conditions_extractor = ConditionsExtractor.new(@model_class).tap do |extractor|
@compressed_rules.reverse_each { |rule| extractor.tableize_conditions(rule.conditions) }
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if these reverse's are necessary (line 14, then re-reversed on line 19)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They are, because the RulesCompressor has an implicit requirement on the ordering of the conditions, unfortunately.

@@ -22,7 +22,7 @@ module ClassMethods
# Here only the articles which the user can update are returned.
def accessible_by(ability, action = :index, strategy: CanCan.accessible_by_strategy)
CanCan.with_accessible_by_strategy(strategy) do
ability.model_adapter(self, action).database_records
ability.model_adapter(all, action).database_records
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if this is already being called from a scope?

User.active.accessible_by(ability, :read)

I think that switching this to all causes accessible_by to return a new scope ignore all previous filters

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The opposite is true: current behavior is to ignore all previous filters, and this change will take them into account.

Try in any project you have:

User.active.all.count vs
User.active.klass.all.count - using klass (or self in the above code) will unscope the relation, whereas all will not.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha, I thought self in that context was a AR relation.

@23tux
Copy link

23tux commented Sep 26, 2022

Is there any news on this?

@coorasse
Copy link
Member

I am ready to reconsider this when Rails 8 is out Septemeber this year. I will then drop support for Rails 5 and this PR can be simplified. Thanks for the amazing work you did here!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants