railtie: Add relation extension

[ethan-readyset]

This commit adds two methods to ActiveRecord::Relation:

• Relation#create_readyset_cache!, which creates a new cache based on the query represented by the relation; and
• Relation#drop_cache!, which drops the cache associated with the query represented by the relation (if one exists)

[helpotters]

Regarding the extension, I'm looking into how we can confirm that we're handling it safely, especially if we're aiming for good encapsulation of behaviors.

Like we're assuming that users won't just make a web interface and allow users to pass in whatever they want to this module.

I'll provide the actual feedback soon!

[ethan-readyset]

@helpotters What kinds of safety concerns do you have? If you're worried about SQL injections, I think it'd be up to users to ensure that they are properly escaping their queries before create/drop_readyset_cache! is invoked

[ethan-readyset]

In terms of whether we should be extending ActiveRecord::Relation, we could always have users include the Readyset::RelationExtension module into their ApplicationRecord base class instead. The methods we defined here seemed pretty low risk though -- we're not changing any of the relation's internal state, and the method names are very unlikely to ever conflict with any methods defined internally by Rails. But curious to hear what you think!

[helpotters]

@helpotters What kinds of safety concerns do you have? If you're worried about SQL injections, I think it'd be up to users to ensure that they are properly escaping their queries before create/drop_readyset_cache! is invoked

It's that and also the query builder (string concat vs adding to an array). I personally just don't know enough to be certain in either direction. So, I'm just going to look over some examples.

[helpotters]

Overall, this is good to go. The only requested changes are to the query builder (which could maybe be extracted to a method if we're going to use it anywhere else).

This is fantastic work, Ethan! Good test coverage.

[helpotters]

Also, after reviewing this gem, I decided to finish up PR #32 (Logger).

Methinks this PR could be made more robust for the developer by providing more feedback about what's going on.

For example, within #create_cache:

  Readyset::Logger.info("Creating cache with name: #{name}")

  begin
    # Existing code for cache creation
  rescue ArgumentError => e
    Readyset::Logger.error("ArgumentError in create_cache!: #{e.message}")
    raise
  rescue => e
    Readyset::Logger.error("Unexpected error in create_cache!: #{e.message}")
    raise
  end

  Readyset::Logger.info("Cache successfully created with name: #{name}")

Here are the specs for Readyset::Logger as reference:

it_behaves_like 'a logger method', :debug, 'This is a debug message'
it_behaves_like 'a logger method', :info, 'This is an info message'
it_behaves_like 'a logger method', :warn, 'This is a warn message'
it_behaves_like 'a logger method', :error, 'This is an error message'
it_behaves_like 'a logger method', :fatal, 'This is an fatal message'
it_behaves_like 'a logger method', :unknown, 'This is an unknown message'

context 'with invalid log level' do
  it 'raises an error' do
    expect { Readyset::Logger.log(:invalid_level, 'message') }.to raise_error(ArgumentError)
  end
end

[ethan-readyset]

@helpotters Ready for another pass -- thanks for the great comments!

[helpotters]

After taking a second look at the spec, the query let introduces Mystery Guests to the specs, mainly due to it being outside of the spec itself and also it being redefined later.

Everything else is solid, but I left some commentary on the topic of subject that I don't think warrants the effort of a change due to the time crunch.

[helpotters]

I've given some thought to my review process, and I think it has been both delayed and causing blocks in our progress.

The feature itself is solid, I only had minor nitpicks about specs. While that does reflect on the overall codebase, given the dependencies of other PRs on this one, and the fact that the feature itself wouldn't be changed by the remaining feedback, it would be best to simply merge it.

Granted, I think we should consider a better git workflow, mainly by working with a dev branch, and allowing dependent PRs to work off feature branches (rebasing and merging when appropriate). I'm experimenting with this by creating a dependent feature branch (origin/feature-query-annotation off of this PR's branch origin/add-route-and-exclude.