
How to use Query Objects to refactor Rails SQL-queries - mkdev_me
https://mkdev.me/en/posts/how-to-use-query-objects-to-refactor-rails-sql-queries
======
projectileboy
My rant is not entirely appropriate in the context of Rails, but generally
speaking, I cannot for the life of me understand why so many programmers take
something very simple - a SQL query - and decide the best way to deal with it
is to hide the SQL in some kind of wrapper, thus “simplifying” it. Not once
have I used one of these libraries where within hours you run across a non-
arcane scenario that the wrapper can’t handle (just yesterday: Postgres type
cast to insert an enum value).

~~~
pcarolan
For 80% of use cases all you're doing is Person.find(id), Items.top(10),
user.email, etc. ORMs are great for productivity, then eventually you run into
a complex query where you want to join a bunch of tables, sort, filter, etc.
In these cases, the best practice is to do this in SQL in my opinion. All ORMs
give you the ability to get an object by executing raw sql. It's not an either
or, it's a both and for general productivity.

------
glennericksen
A "when to" should be added to this "how to". This might be cohesive in a
smaller code-base, but it is not pleasant as "query objects" multiply. There
are other ways of handling complex queries closer to the model without this
type of abstraction. If you reach for this pattern, be sure it's worth the
added cost in complexity.

------
jonathanhefner
I created a gem which wraps all this up in a nice API:
[https://github.com/jonathanhefner/talent_scout](https://github.com/jonathanhefner/talent_scout)

So the final code from the article would look like:

    
    
      class ProductSearch < TalentScout::ModelSearch
        criteria(:search) { |search| where("title ILIKE '%?%'", search) }
    
        criteria(:from_price) { |from_price| where("price > ?", from_price) }
    
        criteria(:to_price) { |to_price| where("price < ?", to_price) }
    
        criteria(:properties) { |properties| joins(:product_properties).where(property_id: properties) }
    
        criteria(:category_id) { |category_id| where(category_id: category_id) }
    
        order :price, default: :desc
      end
    

Brevity is an obvious benefit, but reliability even more so. For example, the
code in the article has at least two major mistakes:

1) `from_price` will silently never be applied

2) `sort_type` and `sort_direction` defaults are mixed up and always nil,
either of which will raise an exception, but only when the corresponding
request params are omitted

Another benefit of using the gem: instances of the `ProductSearch` class from
above can be used with the stock Rails form builder (i.e. `form_for` and
`form_with model:`).

------
victorbojica
I'm trying to figure out what is of such interest. The fact that you can do
all the querying very neat or hiding all the logic behind a class ? Am i
missing something ?

~~~
the_gastropod
I’m with you. Indirection for indirection-sake is not a great idea. If this
code should be moved anywhere, it’s to a scope on the model.

~~~
jp_sc
It's a way to avoid the "fat models" problem (models files with too many lines
on them).

~~~
the_gastropod
I feel like this is akin to putting your sick mother outside in the snow to
avoid the problem of her having a fever.

If you have a model with hundreds of lines, there may be a larger design
problem that could be addressed. By extracting lines into other files to
arbitrarily keep line counts low, you just increase the effort it takes future
developers to understand the code.

~~~
jp_sc
You need the code somewhere. "Extracting lines into other files" is organizing
it. And you do it to keep concerns separated and thus _decreasing_ the effort
for future developers to understand it.

------
revskill
In real world, most of our "pattern" become technical debt as codebase grows
very fast. My solution is just simply using raw sql query. Hide complexity of
query behind another complex "ActiveRecord" is not the solution.

The benefit is separation of concern when your team has a SQL specialist. And
no more code technical debt as your project scales.

~~~
aantix
Most of the relationships are belongs_to and has_many.

How is a raw SQL more readable and expressive than that?

Sure, there may be an occasional AR expression that could be optimized with a
raw SQL query?

But for most associations for information based systems, AR is incredibly
descriptive and performant.

~~~
ratww
ActiveRecord is far from straightforward when it comes to joins,
unfortunately.

Even if you use belongs_to/has_many you still have to manually perform the
join, otherwise you'll get an N+1 issue which will affect your performance. On
top of that there are four ways to perform that join: joins(:table),
include(:table), eager_load(:table) or preload(:table).

In SQL you can basically achieve the same level of expressiveness by using
NATURAL JOIN.

------
gingerlime
2018 ?

I share similar doubts to other commenters.

One semi-positive side effect of moving this into a class however, was testing
(kinda?).

I'm never sure how to test certain bits of my code that might do something as
simple as `Model.where(:x => "y").where(:y => "z").limit(3)` ... usually
ending up with a bunch of `expect(...).to receive_message_chain` which feels
extremely clunky (and doesn't even test all where params).

The other day, I even struggled to test an even simpler `User.update(:x =>
"y")`, because there was another `update` in a before filter on
ApplicationController... and it easily becomes a mock-the-world situation.

Am I doing it all wrong?

~~~
DougBTX
I like to think about what a test of `Model.where(:x => "y").where(:y =>
"z").limit(3)` is supposed to be testing. If the test is that the correct
query gets generated, then a simple check to ensure that the `where` calls
have been made is fine. If the test is that the query returns the correct
data, then either a real database or something lighter weight but which
handles queries in the same way needs to be used, since it is really a test of
how the query is interpreted rather than what the query is.

~~~
gingerlime
I’m testing the message. So that the (in this case) `.where` was called and
with the right parameters... I trust ActiveRecord with the rest, and I don’t
want to hit the DB.

I can’t simply use `expect(...).to receive(:where)`... even if I only cared
that some `.where` was called...

------
unnu
It's just the concept of interactors used for the specific case of search.

------
jeremycw
This is the type of code/suggestion that I would have eaten up 10 years ago as
a junior. Now this type of code just makes me angry and I spend most of my
time turning stuff like this back into what it was before applying this query
object abstraction.

There's nothing wrong with the code before he applies this abstraction. It's
twenty lines, it's exactly in the first place I would go looking if I need to
debug, it directly uses active record so I know there's no hidden BS.

