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

Pitfalls of using QueryObject pattern #41

Open
Galathius opened this issue Oct 24, 2023 · 3 comments
Open

Pitfalls of using QueryObject pattern #41

Galathius opened this issue Oct 24, 2023 · 3 comments

Comments

@Galathius
Copy link

Hi folks! Thanks for your work on this gem.

QueryObject is a commonly used pattern in RoR community, and I noticed that everyone is re-implementing it similarly.
All realizations I saw were mostly designed to be used in ActiveRecord scopes definition:

class User < ApplicationRecord
  scope :active, Users::ActiveQuery
end

Using query objects in this way has quite unpleasant and hidden behavior – if you try to create some subqueries on User model inside of query object – it will be scoped to the "current" scope.

I've briefly described the problem itself here.

Also there are some instructions how to reproduce the issue with this gem:

# rails new pattern-gem-test && cd pattern-gem-test
# bundle add rails-patterns

# rails g model User name:string terminated:boolean manager_id:integer

# rails db:migrate

# app/models/user.rb
class User < ApplicationRecord
  belongs_to :manager, class_name: 'User', optional: true

  scope :with_terminated_manager, Users::WithTerminatedManagerQuery
end

# app/models/users/with_terminated_manager_query.rb
class Users::WithTerminatedManagerQuery < Patterns::Query
  queries User

  def query
    relation.where(manager: User.where(terminated: true))
  end
end

# Then by executing the scope this query:
User.where(name: 'Galalthius').with_terminated_manager
# SELECT "users".*
# FROM "users"
# WHERE "users"."name" = 'Galathius'       <---------- name condition
#   AND "users"."manager_id" IN
#     (SELECT "users"."id"
#      FROM "users"
#      WHERE "users"."name" = 'Galathius'  <---------- name condition
#        AND "users"."terminated" = TRUE)

So far I found the only workaround for this, is simply define scopes using lambdas, although this reduces readability:

  scope :with_terminated_manager, -> { Users::WithTerminatedManagerQuery.new(self).call }
@stevo
Copy link
Member

stevo commented Oct 24, 2023

Personally I dislike using it this way altogether. I tend to always use those objects directly (without being triggered by scope) just to encapsulate specific filtering settings for a specific use-case(s).

@Galathius
Copy link
Author

@stevo I personally agree, but I can also imagine that some people may want to use it exactly in this way.
I would propose to add one more class method to Query pattern, something like ar_scope which will return lambda.
Something like this:

class << self
    def ar_scope
      klass = self
      ->(*args) { klass.new(self, *args).resolve }
    end
  end

And then you can use it in your scope definition:

scope :with_terminated_manager, Users::WithTerminatedManagerQuery.ar_scope

If you don't mind I can create PR for this change.

@wojtodzio
Copy link
Member

Just quickly looking at it, I think you could also undo whatever Rails' scoping does quite easily

> relation = Event.paid; relation.scoping { Event.all.count }
=> 4
> relation = Event.paid; relation.scoping { relation.unscoped.scoping { Event.all.count }}
=> 31

I think you could use it to do something like this:

class CoolQuery < Patterns::Query
  def call
    relation.unscoped.scoping { query }
  end

  def query
    # Your query goes here
  end

  # The rest of the class was omitted for brevity
end

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

No branches or pull requests

3 participants