Extract ActsAsArModel to QueryRelation gem

[Fryguy]

Marking this PR as WIP until I release a "real" gem. For now, I wanted to get the repo out there and start iterating on it with some of the needs from @abellotti . Having this WIP PR in place allows us to verify that ManageIQ still works after any changes there. EDIT: Will make the gem release in a separate PR.

@kbrock @abellotti Please review.

[abellotti]

👍 on the query_relation gem update for this, just verified with updated PR (with local mixin) ManageIQ/manageiq-api-client#24

[kbrock]

👍 lgtm

[kbrock]

its green!

[kbrock]

This is getting there.

I agree that getting away from find looks like a good idea.
I just don't want to introduce a gem that has a magical callback method in our code.

[kbrock]

The original code just called standard active record api find.

If we want to deviate from that, would it make sense to just call QueryRelation with a block?

QueryRelation.new(self, *args) do |mode, options|
  find(mode, to_legacy_options(options))
end

not sure if QueryRelation would needself other than looking up klass. Maybe passing that in would be better

[Fryguy]

So what I see you proposing is essentially creating an anonymous proc for the callback instead of a method / telling QueryRelation to use that method.

The only thing I don't like about the anonymous proc is that it's not testable in isolation.

[kbrock]

# old
class MyModel
  def self.search(*_)
  end
  def self.all
    QueryRelation.new(self)
  end
end
model = MyModel.all.where(...).order(...)
expect(model).to receive(:search).with(...)

# new

class MyModel
  def self.search(*_)
  end
  def self.all
    QueryRelation.new(self) { |mode, options| search(mode, options) }
  end
end
model = MyModel.all.where(...).order(...)
expect(model).to receive(:search).with(...)

# MIQ is unchanged:
expect(model).to receive(:find).with(...)

[kbrock]

the anonymous proc is private. and does not need to be tested. Maybe for this I propose an integration test. But I feel this is ok since the "integration" test is only testing the public interfaces of a single class.

QueryRelation tests ensure that calling these methods populate the hash.
MyModel test can ignore QueryRelation and just focuses that if you call it, it will call find correctly.

describe QueryRelation do
  describe "#all" do
    it "yields" do
      model = double("Model", :klass => "Model")
      expect(model).to receive("rslt=").with(:select => %w(abc)).and_return([1])
      expect(QueryRelation.new(model).select("abc").to_a).to eq([1])
    end
  end
end

Yea, I think changing the self going in would be better. Possibly passing in :klass. It would simplify the proc and the double.

[Fryguy]

The original code just called standard active record api find.

Was thinking about this more and although the original code did this it was against the "old" find interface, which, moving forward, I don't want QueryRelation to abide by. The "current" find interface for ActiveRecord only accepts ids. As such, QueryRelation expects a different method, intentionally not find so the caller (or perhaps the QueryRelation::Queryable mixin) can implement an id-based find. The method name I chose was search after some negotiation with @abellotti, with the purpose being to take the options from QueryRelation and use them to do the search against the source.

In this case of ActsAsArModel, it takes the options, converts them to legacy options and calls the legacy find. Ideally, I'd like to kill the legacy find method of ActsAsArModel, and then kill the to_legacy_options method as well, moving towards the "current" ActiveRecord like interface.

Note that the method search is just the default, the caller can override that to whatever they want.

QueryRelation.new(self, :query_method => :my_magic_method)

[Fryguy]

I think what I don't like about QueryRelation.new(self) { |mode, options| search(mode, options) } is that the block is a pointless forwarding method. Declaring that search is the default method, and providing an override (see the query_method comment above) felt more Rubyish.

I see your point about it being semi-magical that QueryRelation is going to, by default, call search on self, and it appears you want to make that more explicit. I was sort of taking the documentation-driven approach that you see with things like Mixins. For example, it is documented for Enumerable that if you implement the .each method, then everything will just work. Similarly, I was thinking that for QueryRelation if you implement the .search method, then everything will just work.

Note that if the caller wants explicit, they can also do QueryRelation.new(self, :query_method => :search). Does that work for you?

[abellotti]

I tend to agree with @Fryguy here. Nice thing about current approach is less setup for caller for default/documented implementation (e.g. requiring self.search), other can be registered via :query_method. Also, as noted earlier, with the find() implemented in the caller, it can be fine-tuned/performant as needed which I've done in the API Client making the queries to the Server more efficient and levering bulk queries for example.

[kbrock]

good news: you won me over on using search
bad news: now I'm not sure if I want to link klass and search together.

oh, and did I say how much I liked how you pulled out to_legacy_options? very cool stuff

[kbrock]

I like moving this here. thanks

[kbrock]

I'm ok with this. The gist of my thought was: ManageIQ/query_relation#5

[Fryguy]

@kbrock Sounds good. I'm thinking lets get this in as is to unblock manageiq_api_client. Then I'll release a new query_relation gem and then have a follow up PR to switch from git to a real gem. Don't quite want to release query_relation yet as we might have to add a couple of things for manageiq_api_client.

Additionally, we can debate the proc interface vs klass vs method in ManageIQ/query_relation#5

[Fryguy]

Rebased

[miq-bot]

<pr_mergeability_checker />This pull request is not mergeable. Please rebase and repush.

[Fryguy]

HAHA! I beat you @miq-bot !

[miq-bot]

Checked commit Fryguy@381ca40 with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1
2 files checked, 0 offenses detected
Everything looks good. 🏆

[abellotti]

@Fryguy not sure about replication travis failure.

[chessbyte]

Euwe Backport details:

$ git log
commit a6b797815996974b94c7eddfb0a5b226a77f7e75
Author: Keenan Brock <keenan@thebrocks.net>
Date:   Thu Sep 29 19:01:39 2016 -0400

    Merge pull request #11120 from Fryguy/extract_acts_as_ar_relation

    Extract ActsAsArModel to QueryRelation gem
    (cherry picked from commit a88ea045413938cbe0970eaa0a52476f7803df90)