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

With_role cannot be combined into larger queries #579

Open
Taeir opened this issue Feb 23, 2022 · 2 comments · May be fixed by #580
Open

With_role cannot be combined into larger queries #579

Taeir opened this issue Feb 23, 2022 · 2 comments · May be fixed by #580

Comments

@Taeir
Copy link

Taeir commented Feb 23, 2022

The issue

The definition of resources_find in ResourceAdapter uses (simplified) resources.select("table_name.*"). This hinders the ability of ActiveRecord to combine queries together, because the select is already supplied to select all fields, even when only some of them are needed.

The fix

As far as I understand, removing the complete line that supplies the select does not alter any behavior. It leaves it up to ActiveRecord to determine which fields are necessary for the query. It will default to * but can be limited to id when useful for combining queries. I will try to make a pull request (#580) to see if this is indeed the case in all scenarios. I have tested so far with MySQL and SQLite where it works, and I expect other implementations to behave the same.

A detailed example

Setup
Showing with an example, consider models User, Blog and Article:

class User < ActiveRecord::Base
  rolify
end

class Blog < ActiveRecord::Base
  ROLES = %i[editor].freeze
  resourcify

  has_many :articles
end

class Article < ActiveRecord::Base
  belongs_to :blog
end

Normal Behavior
In active record, the following works

Article.where(blog_id: Blog.where(<some conditions>))

These are (usually) combined into a single query. This is because ActiveRecord recognizes that we want only the ids of the blogs and effectively applies .select(:id). Contrast that with for example:

Article.where(blog_id: Blog.where(<some conditions>).ids)

Now we get 2 separate queries, once for all the ids of the blogs and then another query with the condition blog_id IN (... those ids ...).

So for efficiency sake (less requests to the database), the top way would be preferable.

Rolify
Unfortunatly, the rolify with_role has already specified a select * (in resources_find), and this breaks this functionality. For example:

Article.where(blog_id: Blog.with_role(:editor, user))

Gives an invalid SQL query. Similarly, applying .select(:id) has no effect and does not affect the (invalid) query (the former select seems to take precedence). The only resort is to use .ids, which splits it into 2 separate queries.

Versions

Rolify 6.0.0
ActiveRecord 6.1.4.6
All versions of Ruby

Taeir added a commit to Taeir/rolify that referenced this issue Feb 23, 2022
@Taeir Taeir linked a pull request Feb 23, 2022 that will close this issue
@Taeir
Copy link
Author

Taeir commented Feb 23, 2022

Actually, #175 explains why this select was added in the first place: as a way to mark the query as not read-only. However, the select prevents the user from specifying their own select (and prevents ActiveRecord from inferring it). Instead, a .readonly(false) might be a cleaner solution.

Not sure if that is necessary though with newer versions of ActiveRecord since that issue is already 8 years old.

@wheatevo
Copy link

Issue #584 is caused by this select as well.

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

Successfully merging a pull request may close this issue.

2 participants