Accessible by strategy: Exists subquery #691

kaspernj · 2021-01-26T20:33:49Z

The new subquery strategy is great, but doing a "WHERE id IN (...)" can be slow, because Postgres will resolve the ID's on the right first, so if you have a big table that is going to take a lot of juice.

"WHERE EXISTS (...)" should be a lot faster, because it will already know the related ID's and only sub query those instead of resolving them first through the given sub query.

There is a problem however. It's difficult to alias the table properly in ActiveRecord. I have gotten around that by doing two exist-queries just to make it possible to reference the original table in the exists-subquery. It's still much faster than doing a "WHERE id IN (...)" however and it solves my own performance issues.

When I try to run specs with the following command I get an error: uninitialized constant CanCan::ModelAdapters::ActiveRecord5Adapter

Do I need to do something special to get the specs up and running?

ghiculescu · 2021-04-28T16:02:43Z

Do I need to do something special to get the specs up and running?

Rebase against master, now that Travis has been replaced by Github Actions #707

kaspernj · 2021-05-13T08:20:26Z

Rebase against master, now that Travis has been replaced by Github Actions #707

I still get the same error when trying to run the specs locally. I have tried running bundle exec rake and bundle exec rspec but both result in the same error unfortunately :'-(

kaspernj · 2021-05-13T09:08:50Z

lib/cancan/model_adapters/active_record_5_adapter.rb

-          end
-          @model_class.where(@model_class.primary_key => inner)
-
+          build_joins_relation_subquery(where_conditions)


This refactor was needed to deal with Rubocop complexity offences.

we should probably, at some point, extract these strategies

@coorasse I have extracted all strategies as separate classes (also to solve Rubocop complexity issues). What do you think?

kaspernj · 2021-05-13T09:09:51Z

lib/cancan/model_adapters/active_record_5_adapter.rb

+        @model_class.where("EXISTS (#{double_exists_query_sql(where_conditions)})")
+      end
+
+      def double_exists_inner_query(where_conditions)


I am not sure the code has gotten less abstract having to use this many helper methods, but I couldn't otherwise find a way around the Rubocop complexity offences :'-(

I honestly might get rid of rubocop and switch to standard soon :)

Personally I feel that Rubocop has forced me to make the code more abstract than it had to be here :-)

coorasse · 2021-06-18T11:11:59Z

I really like this PR, and I really like the fact that we start working on different strategies (SQL) to fetch the records. cancancan cannot always know what's the best strategy, so the only way we have is to allow users to use custom strategies.

Being said that. I'd like to include this new strategy. I want to first pull the branch and see if I can add more tests for more cases

kaspernj · 2021-06-22T10:04:08Z

@coorasse That sounds great. We cannot live without this strategy in the project I am working on, which involves a lot of really big tables with tons of data :-)

…tegy/double_exist_subquery

…ting code out into separate strategy classes

kaspernj · 2021-06-24T12:58:51Z

I have refactored to two different strategies:

Joined alias exists subquery

Joins the same table with an alias, which is then used to make an exists query, which is faster than "WHERE id IN (...)". You will need to use the full table name for columns, since the alias will have the same columns - but you should always do that in ActiveRecord anyway ;-)

Joined alias each rule as exists

Joins the same table with an alias, iterates through all relevant rules, and uses them as separate or-exists queries.

This can be very useful to avoid ActiveRecord messing up the join names, if the same tables are used in multiple other different rules, which is something that has plagued us a lot. It is sometimes faster than the other strategies, but it might be your only way out, if ActiveRecord decides to mess you up, which is currently the case for us in Rails 6.1 (but works in 6.0.4 but not in 6.0.3).

It only left joins if nil values are detected, which can also affect the query speed.

Here you must also use the table names for columns to not mess with the alias.

xjunior · 2021-07-15T19:48:24Z

lib/cancan/model_adapters/active_record_5_adapter.rb

-          relation.left_joins(joins).distinct
-        end
+      def strategy_class
+        require_relative "strategies/#{CanCan.accessible_by_strategy}"


Why not just require them at the top of the file?

That would require code that the user in most cases would never use. I assume that most users are just going to set a single strategy and use that for everything :-)

coorasse · 2021-11-10T16:01:48Z

Thanks! I went through the PR and looks solid. I'll probably extract some parts of it and merge them separately.

coorasse · 2021-11-10T16:10:33Z

@kaspernj regarding the tests, you can run them using appraisal (see README). For example:

DB='sqlite' bundle exec appraisal rake

coorasse · 2021-11-11T14:57:21Z

I am now looking at the first of the two stretegies, and I don't understand why you need to join the table with itaself....:🤔

https://github.com/CanCanCommunity/cancancan/pull/691/files#diff-cf63e8e34aa3f5976e07653a98befdb9ae3cabef3996551d81352afdaa9e0f71R9

kaspernj · 2021-11-16T08:58:45Z

@coorasse The short version is to not dynamically join tables directly in the query, which can cause issues when using other libraries that work with dynamic queries like Ransack (and others) :-)

Here is an example.

Multiple different roles that has different types of access.
Based on which role the current user has a number of tables are joined.
Same table is joined by different queries.
Because the same table is joined multiple times the aliases changes based on which role the user has.

To get around this a sub query is used where CanCan joins the table. Manually joined tables (or tables joined through other libraries like Ransack) are unaffected by which role the user has.

This can also result in bugs, because joining tables with aliases can mess up in ActiveRecord. I have had multiple issues about this in both Rails and Ransack already. By joining the tables from CanCan in a sub query, we can minimise those issues greatly, which directly fixed a number of issues in two projects I was working :-)

…trategy/double_exist_subquery

kaspernj · 2021-11-16T10:50:34Z

@coorasse So I see the two new strategies as tools to get around those issues. Maybe you wouldn't use these strategies by default, but they would be options to get around the mentioned issues if you run into them like I did :-)

kaspernj · 2021-11-16T19:23:31Z

I am now looking at the first of the two stretegies, and I don't understand why you need to join the table with itaself....:🤔

https://github.com/CanCanCommunity/cancancan/pull/691/files#diff-cf63e8e34aa3f5976e07653a98befdb9ae3cabef3996551d81352afdaa9e0f71R9

@coorasse In order to do a sub query, I cannot use the original table name, so I have to alias the same table to use its ID in the sub query. If you know a better way of doing this then please enlighten me? :-)

coorasse · 2022-03-23T07:39:12Z

I tried a lot of different things, but I am not able to generate an alias 😞 . I think your solution is the only working one, although I still really don't like that it performs a join that is not necessary. Pity. I'll think this through a bit more today, but if I don't find an alternative, I'd rather have this merged than not. So expect to have this available in the next release of cancancan anyway.

kaspernj · 2022-03-23T07:47:31Z

@coorasse I feel the exact same way about the alias, and I have also sunk a considerable amount of time into trying to get around it :'-(

kaspernj · 2022-03-23T07:47:55Z

The performance gains of this has been huge however :-)

kaspernj marked this pull request as ready for review May 13, 2021 07:59

kaspernj force-pushed the accessible_by_strategy/double_exist_subquery branch 9 times, most recently from 5d11842 to 58c7204 Compare May 13, 2021 09:06

Added the new strategy: double_exist_subquery

bd4fb38

kaspernj force-pushed the accessible_by_strategy/double_exist_subquery branch from 58c7204 to bd4fb38 Compare May 13, 2021 09:08

kaspernj commented May 13, 2021

View reviewed changes

coorasse self-assigned this Jun 18, 2021

coorasse added the new feature label Jun 18, 2021

kaspernj added 8 commits June 22, 2021 21:11

Merge remote-tracking branch 'github/develop' into accessible_by_stra…

f181dc5

…tegy/double_exist_subquery

Accessible by strategy: Each rule as exists subquery

f77a096

Use aliased joined table instead of double sub query

704176a

Solved offences and specifically solved complexity offences by extrac…

fc3c88d

…ting code out into separate strategy classes

Extracted existing strategies out into separate classes as well

efb09cd

Fixed more offences and cleaned up

b4536fd

Removed unused file

64f37c3

Fixed spec

be79997

kaspernj changed the title ~~Accessible by strategy: Double exist subquery~~ Accessible by strategy: Exist subquery Jun 24, 2021

kaspernj changed the title ~~Accessible by strategy: Exist subquery~~ Accessible by strategy: Exists subquery Jun 24, 2021

kaspernj added 10 commits June 24, 2021 11:33

Fixed specs

e70c734

Select column through table name

279fdb5

Fixed more specs

f18c5aa

Fixed offences

493af4c

Fixed SQL in spec

70b983d

Unscoped might remove existing settings on the query

f101181

Limit results to 1

8bb0a9b

Added limit in spec as well

bf86cd6

Fixed SQL

7373d0b

Just use JOIN both places

dd26042

xjunior reviewed Jul 15, 2021

View reviewed changes

coorasse mentioned this pull request Nov 10, 2021

Extract strategies in separate files #754

Merged

coorasse mentioned this pull request Nov 12, 2021

Debug exists subquery #757

Closed

kaspernj added 2 commits November 16, 2021 09:05

Merge remote-tracking branch 'cancancan/develop' into accessible_by_s…

ec37dbd

…trategy/double_exist_subquery

Fixed several issues after merging with latest version of develop

45a4416

coorasse merged commit 24ed063 into CanCanCommunity:develop Mar 23, 2022

kaspernj deleted the accessible_by_strategy/double_exist_subquery branch May 1, 2022 07:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Accessible by strategy: Exists subquery #691

Accessible by strategy: Exists subquery #691

kaspernj commented Jan 26, 2021

ghiculescu commented Apr 28, 2021

kaspernj commented May 13, 2021

kaspernj May 13, 2021

coorasse Jun 18, 2021

kaspernj Jun 24, 2021

kaspernj May 13, 2021

coorasse Jun 18, 2021

kaspernj Jun 24, 2021

coorasse commented Jun 18, 2021

kaspernj commented Jun 22, 2021

kaspernj commented Jun 24, 2021 •

edited

Loading

xjunior Jul 15, 2021

kaspernj Jul 15, 2021

coorasse commented Nov 10, 2021

coorasse commented Nov 10, 2021

coorasse commented Nov 11, 2021

kaspernj commented Nov 16, 2021

kaspernj commented Nov 16, 2021

kaspernj commented Nov 16, 2021

coorasse commented Mar 23, 2022

kaspernj commented Mar 23, 2022

kaspernj commented Mar 23, 2022

Accessible by strategy: Exists subquery #691

Accessible by strategy: Exists subquery #691

Conversation

kaspernj commented Jan 26, 2021

ghiculescu commented Apr 28, 2021

kaspernj commented May 13, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coorasse commented Jun 18, 2021

kaspernj commented Jun 22, 2021

kaspernj commented Jun 24, 2021 • edited Loading

Joined alias exists subquery

Joined alias each rule as exists

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coorasse commented Nov 10, 2021

coorasse commented Nov 10, 2021

coorasse commented Nov 11, 2021

kaspernj commented Nov 16, 2021

kaspernj commented Nov 16, 2021

kaspernj commented Nov 16, 2021

coorasse commented Mar 23, 2022

kaspernj commented Mar 23, 2022

kaspernj commented Mar 23, 2022

kaspernj commented Jun 24, 2021 •

edited

Loading