andypike / rectify

Build maintainable Rails apps
MIT License
596 stars 48 forks source link

Initial implementation of Query Objects #8

Closed andypike closed 8 years ago

andypike commented 8 years ago

Covers https://github.com/andypike/rectify/issues/7

Also, as additional integration testing this branch is being used by the sample app and here is the PR for that: https://github.com/andypike/smarty_pants/pull/3

The readme has been updated with details of how these work but the TLDR is that you can now encapsulate your database queries in an object which reduces the code that ends up in your Rails models. Here's an example:

class UsersOlderThan < Rectify::Query
  def initialize(age)
    @age = age
  end

  def query
    User.where("age > ?", @age)
  end
end

You derive off of Rectify::Query and add a method called #query that returns an ActiveRecord::Relation. We supply some basic methods for operating on this:

UsersOlderThan.new(20).each do |user|
  puts user.name
end

Developers can compose Query Objects using the | operator:

active_users_over_20 = ActiveUsers.new | UsersOlderThan.new(20)

active_users_over_20.count # => Returns number of active users over 20 years old

There is also some provision for using raw SQL which enables the developer to use special features of their database that ActiveRecord::Relation does not support:

class UsersOverUsingSql < Rectify::Query
  include Rectify::SqlQuery

  def initialize(age)
    @age = age
  end

  def model
    User
  end

  def sql
    <<-SQL.strip_heredoc
      SELECT *
      FROM users
      WHERE age > :age
      ORDER BY age ASC
    SQL
  end

  def params
    { :age => @age }
  end
end

There is also an experimental RSpec reporter that the developer can enable which will tell them how many SQL queries were made and how long they took per target (class). To enable this feature do the following in your spec/rails_helper.rb:

require "rectify/rspec"

Rectify::RSpec::DatabaseReporter.enable

I'd like to add some configuration here to allow tracking of certain queries (only SELECT for example) and also to exclude queries that are in feature specs and also Query Object specs. Here is the output with this reporter enabled on the Rectify specs:

Target         | Type       | Queries | Time (s)
------------------------------------------------
Rectify::Query | query      |      56 | 0.00672

Database Queries: 56 in 0.00672s
andypike commented 8 years ago

@kimberger Here you go, what do you think of this? Lots of details in the readme so might be a good idea to read that first. Interested in hearing what you think 😄

andypike commented 8 years ago

Ping @tombeynon and @efexen. Would be interested in your views on this. Seem legit?

tombeynon commented 8 years ago

Hey @andypike - this looks really nice. I have to admit query objects isn't something we generally use too much, since our data layer isn't usually complex enough to warrant moving queries out of the model. That's basically all we use models for.

Having saids that, I can definitely see use cases. One thing I'd like to be able to do with this is create a query using multiple method calls to build it. I assume your structure isn't too bothered about how we define the query, so long as #query returns an AR assoc?

I like your method of merging queries using the single pipe operator, but does this also respond to #merge in the same way standard AR assocs would?

Generally a big thumbs up from me though.

andypike commented 8 years ago

@tombeynon Thanks for the feedback! I agree with you that if your queries are pretty simple then using Query Objects will probably be overkill and you won't get a lot of benefit from them, so that's cool.

In terms of #query, yeah, Rectify doesn't care how you create the query as long as you return an ActiveRecord::Relation object. So if you want to split that up into multiple methods or use some scopes from your models then that's cool too.

For merging queries, currently only the pipe operator is supported but we could alias #merge to that if we wanted to. In that case the usage would look like this:

users = ActiveUsers.new.merge(UsersOlderThan.new(20))

# vs

users = ActiveUsers.new | UsersOlderThan.new(20)

It would work but isn't as nice IMO. Is there a use case where having #merge would be beneficial?

Thanks again ❤️

tombeynon commented 8 years ago

@andypike only reason I mention #merge is to mimic the AR API since the behaviour appears to be much the same. Agreed it isn't as nice though, so I don't really have a problem if it isn't included.

kimberger commented 8 years ago

My only concern with the | is that people could easily read it as ||?

andypike commented 8 years ago

@tombeynon Cool, thanks for the clarification ❤️

@kimberger I thought about using >> after being inspired by the pipeline operator |> in Elixir (I couldn't get |> to work in Ruby). But I was reminded that in Ruby, the way to do set union is with a single pipe (see http://ruby-doc.org/core-2.3.0/Array.html#method-i-7C) so I thought it best to follow the operator that is already used in Ruby for this same operation.

Do you think we should use something else that is less likely to be misread? Maybe we should add #merge as @tombeynon suggested as an alias to | which will allow people to use the method that feels right to them?

I guess the other option would be to provide a method that combines a list of queries, maybe something like:

Rectify::Query.merge(ActiveUsers.new, UsersOlderThan.new(20))

Would be interested in your thoughts? ❤️

kimberger commented 8 years ago

@andypike ah fair enough. Hadn't actually seen the single pipe used before :smile:

I think the advantage in Elixir is that their operator can go at the beginning of a new line like so:

  model
  |> cast(params, @required_fields, @optional_fields)
  |> validate_length(:bio, min: 2)

But you lose that advantage in ruby because the equivalent would be:

ActiveUsers.new |
  UsersOlderThan.new(20) |
  MaleUsers.new

#merge might be nicest for handling multiple lines?

tombeynon commented 8 years ago

I think including #merge as an alias for the pipe is a good idea - it allows people to use both, and if i remember correctly, AR relation used to allow both | and #merge for combining scopes.

I toyed with the idea of a class method .merge on a query subclass to simplify, but handling other arguments will end up messy. I actually really like Rectify::Query.merge as a convenience method.

andypike commented 8 years ago

@tombeynon @kimberger OK cool thanks chaps, I'll get that added. Great feedback 💖

andypike commented 8 years ago

@tombeynon @kimberger I've added Query.merge and Query#merge as discussed. See https://github.com/andypike/rectify/pull/8/commits/64388c80e748590595ee69e4b01dfa9d51eb0134 for changes and the commit message for how to use them.

Thanks again for the help 😄