kvokka / pp_sql

Rails ActiveRecord SQL queries log beautifier
MIT License
265 stars 9 forks source link

Make pp_sql work even if to_sql is not rewritten #17

Closed dputtick closed 4 years ago

dputtick commented 4 years ago

@kvokka this gem is great, thanks for maintaining it! For our use case, we'd prefer not to modify to_sql everywhere, but we do want to be able to use pp_sql in the Rails console. This PR makes that possible.

I didn't have any great ideas for how to handle the case where to_sql returns something other than a sql query, so I figured maybe just calling to_s before formatting it would be ok. Open to other suggestions, though. We could check if to_sql is defined on the parent class in pp_sql, but that didn't feel great.

kvokka commented 4 years ago

Thank you for your contribution, @dputtick . I like the idea, but want to ask first, why the solution like

# config/initializers/pp_sql.rb
Rails.application.console do
  require 'pp_sql'
end

# Gemfile
...
gem 'pp_sql', require: false

does not fit your needs?

dputtick commented 4 years ago

@kvokka I tried that, and it seems like pp_sql doesn't load properly when required this way. PpSql is defined in the global scope, but I get NoMethodError: undefined methodpp_sql'` when I try to call it on a Relation in the console (with Rails 5.1), and to_sql looks unchanged. I haven't taken the time to dig further into why that is.

kvokka commented 4 years ago

It looks like ActiveSupport does not hit the hooks, so it'll be required to do it manually. Can you please try this?

# config/initializers/pp_sql.rb
Rails.application.console do
  require 'pp_sql'
  ActiveRecord::Relation.send(:prepend, ToSqlBeautify)
  ActiveRecord::LogSubscriber.send(:prepend, LogSubscriberPrettyPrint)
end

Sorry, i'm not writing Ruby for a while, so this aspect was missed in the first message

dputtick commented 4 years ago

Ah, right, ActiveRecord::Relation.send(:prepend, PpSql::ToSqlBeautify) worked. Thanks for helping to figure that out.

I do think I would still prefer the approach taken in this PR. I'm typically using .to_sql to embed a Rails scope inside of raw sql, and I prefer to avoid parsing + prettifying that SQL unnecessarily (even if only in the console). I'd prefer to avoid that and only parse + prettify with .pp_sql. But, feel free to close this if you disagree or don't think it's worth the effort!

kvokka commented 4 years ago

to_sql method is overwritten anyway, so it is more the question of code organisation. This code helped you, maybe it'll be useful for someone else. Thank you for the contribution!