Switch ActsAsArModel to be QueryRelation based by default

Author: Fryguy

app/models/pglogical_subscription.rb

@@ -16,22 +16,20 @@ class PglogicalSubscription < ActsAsArModel
     :provider_region_name => :string
   )
 
-  def self.find(*args)
-    case args.first
+  def self.search(mode, args)
+    case mode
     when :all then find_all
-    when :first, :last then find_one(args.first)
-    else find_id(args.first)
+    when :first, :last then find_one(mode)
+    else find_id(mode)
     end
   end
 
   def self.lookup_by_id(to_find)
-    find(to_find)
+    find_id(to_find)
   rescue ActiveRecord::RecordNotFound
     nil
   end
-
   singleton_class.send(:alias_method, :find_by_id, :lookup_by_id)
-  Vmdb::Deprecation.deprecate_methods(singleton_class, :find_by_id => :lookup_by_id)
 
   def save!(reload_failover_monitor = true)
     assert_different_region!
@@ -73,7 +71,7 @@ def delete(reload_failover_monitor_service = true)
   end
 
   def self.delete_all(list = nil)
-    (list.nil? ? find(:all) : list)&.each { |sub| sub.delete(false) }
+    (list.nil? ? all : list)&.each { |sub| sub.delete(false) }
     EvmDatabase.restart_failover_monitor_service_queue
     nil
   end
@@ -189,7 +187,7 @@ def self.find_id(to_find)
     subscriptions.each do |s|
       return new(subscription_to_columns(s)) if s.symbolize_keys[:subscription_name] == to_find
     end
-    raise ActiveRecord::RecordNotFound, "Coundn't find subscription with id #{to_find}"
+    raise ActiveRecord::RecordNotFound, "Couldn't find subscription with id #{to_find}"
   end
   private_class_method :find_id
 

lib/acts_as_ar_model.rb

@@ -1,3 +1,5 @@
+require 'query_relation'
+
 class ActsAsArModel
   include Vmdb::Logging
   include ArVisibleAttribute
@@ -121,6 +123,7 @@ def self.set_columns_hash(hash)
   include ActiveRecord::VirtualAttributes::VirtualFields
 
   include AttributeBag
+  extend QueryRelation::Queryable
 
   def self.instances_are_derived?
     true
@@ -146,59 +149,49 @@ def self.reflections
   #
   # Find routines
   #
-  def self.find(*_args)
-    raise NotImplementedError, _("find must be implemented in a subclass")
+  def self.find(mode_or_id, *args)
+    if %i[all first last].include?(mode_or_id)
+      Vmdb::Deprecation.warn("find(:#{mode_or_id}) is deprecated (use #{mode_or_id} instead)")
+      search(mode_or_id, from_legacy_options(args.extract_options!))
+    else
+      lookup_by_id(mode_or_id, *args).tap do |record|
+        raise ActiveRecord::RecordNotFound, "Couldn't find #{self} with id=#{mode_or_id}" if record.nil?
+      end
+    end
   end
 
-  # This method is called by QueryRelation upon executing the query.
-  #   Since it will receive non-legacy search options, we need to convert
-  #   them to handle the legacy find.
-  def self.search(mode, options = {})
-    find(mode, to_legacy_options(options))
+  def self.search(mode, options)
+    raise NotImplementedError, "must be defined in a subclass"
   end
-  private_class_method :search
 
-  def self.to_legacy_options(options)
+  def self.from_legacy_options(options)
     {
-      :conditions => options[:where],
-      :include    => options[:includes],
-      :limit      => options[:limit],
-      :order      => options[:order],
-      :offset     => options[:offset],
-      :select     => options[:select],
-      :group      => options[:group],
+      :where    => options[:conditions],
+      :includes => options[:include],
+      :limit    => options[:limit],
+      :order    => options[:order],
+      :offset   => options[:offset],
+      :select   => options[:select],
+      :group    => options[:group],
     }.delete_blanks
   end
-  private_class_method :to_legacy_options
-
-  def self.all(*args)
-    require 'query_relation'
-    QueryRelation.new(self, *args)
-  end
-
-  def self.first(*args)
-    find(:first, *args)
-  end
-
-  def self.last(*args)
-    find(:last, *args)
-  end
-
-  def self.count(*args)
-    all(*args).count
-  end
+  private_class_method :from_legacy_options
 
   def self.find_by_id(*id)
     options = id.extract_options!
-    options[:conditions] = {:id => id.first}
-    first(options)
+    options[:where] = {:id => id.first}
+    search(:first, options)
   end
 
+  singleton_class.send(:alias_method, :lookup_by_id, :find_by_id)
+  Vmdb::Deprecation.deprecate_methods(singleton_class, :find_by_id => :lookup_by_id)
+
   def self.find_all_by_id(*ids)
     options = ids.extract_options!
-    options[:conditions] = {:id => ids.flatten}
-    all(options)
+    options[:where] = {:id => ids.flatten}
+    search(:all, options)
   end
+  Vmdb::Deprecation.deprecate_methods(singleton_class, :find_all_by_id => "use where(:id => ids) instead")
 
   #
   # Methods pulled from ActiveRecord 2.3.8

spec/lib/acts_as_ar_model_spec.rb

@@ -55,35 +55,133 @@
   end
 
   describe ".all" do
-    it "chains through active query" do
-      expect(base_class).to receive(:find).with(:all, {}).and_return([])
+    it "comes from QueryRelation" do
+      expect(base_class).to receive(:search).with(:all, {}).and_return([])
       expect(base_class.all.to_a).to eq([])
     end
 
-    it "supports where (as an example)" do
-      expect(base_class).to receive(:find).with(:all, {:conditions => {:id => 5}}).and_return([])
+    it "supports where from QueryRelation (as an example)" do
+      expect(base_class).to receive(:search).with(:all, {:where => {:id => 5}}).and_return([])
       expect(base_class.all.where(:id => 5).to_a).to eq([])
     end
   end
 
   describe ".first" do
-    it "chains through active query" do
-      expect(base_class).to receive(:find).with(:first).and_return(nil)
+    it "comes from QueryRelation" do
+      expect(base_class).to receive(:search).with(:first, {}).and_return(nil)
       expect(base_class.first).to eq(nil)
     end
   end
 
   describe ".last" do
-    it "chains through active query" do
-      expect(base_class).to receive(:find).with(:last).and_return(nil)
+    it "comes from QueryRelation" do
+      expect(base_class).to receive(:search).with(:last, {}).and_return(nil)
       expect(base_class.last).to eq(nil)
     end
   end
 
   describe ".count" do
-    it "chains through active query" do
-      expect(base_class).to receive(:find).with(:all, {}).and_return([])
+    it "comes from QueryRelation" do
+      expect(base_class).to receive(:search).with(:all, {}).and_return([])
       expect(base_class.count).to eq(0)
     end
   end
+
+  describe ".find (deprecated)" do
+    around do |example|
+      Vmdb::Deprecation.silence do
+        example.run
+      end
+    end
+
+    describe "find(:all)" do
+      it "chains through QueryRelation" do
+        expect(base_class).to receive(:search).with(:all, {}).and_return([])
+        expect(base_class.find(:all).to_a).to eq([])
+      end
+
+      it "supports :conditions legacy option (as an example)" do
+        expect(base_class).to receive(:search).with(:all, {:where => {:id => 5}}).and_return([])
+        expect(base_class.find(:all, :conditions => {:id => 5}).to_a).to eq([])
+      end
+    end
+
+    describe "find(:first)" do
+      it "chains through QueryRelation" do
+        expect(base_class).to receive(:search).with(:first, {}).and_return(nil)
+        expect(base_class.find(:first)).to eq(nil)
+      end
+    end
+
+    describe ".find(:last)" do
+      it "chains through QueryRelation" do
+        expect(base_class).to receive(:search).with(:last, {}).and_return(nil)
+        expect(base_class.find(:last)).to eq(nil)
+      end
+    end
+  end
+
+  describe ".find" do
+    it "finds a record by id" do
+      expected = Object.new
+      expect(base_class).to receive(:search).with(:first, {:where => {:id => 5}}).and_return(expected)
+      expect(base_class.find(5)).to eq(expected)
+    end
+
+    it "raises when not found" do
+      expect(base_class).to receive(:search).with(:first, {:where => {:id => 5}}).and_return(nil)
+      expect { base_class.find(5) }.to raise_error(ActiveRecord::RecordNotFound)
+    end
+  end
+
+  describe ".lookup_by_id" do
+    it "finds a record by id" do
+      expected = Object.new
+      expect(base_class).to receive(:search).with(:first, {:where => {:id => 5}}).and_return(expected)
+      expect(base_class.lookup_by_id(5)).to eq(expected)
+    end
+
+    it "returns nil when not found" do
+      expect(base_class).to receive(:search).with(:first, {:where => {:id => 5}}).and_return(nil)
+      expect(base_class.lookup_by_id(5)).to be_nil
+    end
+  end
+
+  describe ".find_by_id (deprecated)" do
+    around do |example|
+      Vmdb::Deprecation.silence do
+        example.run
+      end
+    end
+
+    it "finds a record by id" do
+      expected = Object.new
+      expect(base_class).to receive(:search).with(:first, {:where => {:id => 5}}).and_return(expected)
+      expect(base_class.find_by_id(5)).to eq(expected)
+    end
+
+    it "returns nil when not found" do
+      expect(base_class).to receive(:search).with(:first, {:where => {:id => 5}}).and_return(nil)
+      expect(base_class.find_by_id(5)).to be_nil
+    end
+  end
+
+  describe ".find_all_by_id (deprecated)" do
+    around do |example|
+      Vmdb::Deprecation.silence do
+        example.run
+      end
+    end
+
+    it "finds record by ids" do
+      expected = [Object.new, Object.new]
+      expect(base_class).to receive(:search).with(:all, {:where => {:id => [5, 6]}}).and_return(expected)
+      expect(base_class.find_all_by_id(5, 6)).to eq(expected)
+    end
+
+    it "returns empty Array when none found" do
+      expect(base_class).to receive(:search).with(:all, {:where => {:id => [5, 6]}}).and_return([])
+      expect(base_class.find_all_by_id(5, 6)).to eq([])
+    end
+  end
 end

spec/models/pglogical_subscription_spec.rb

@@ -118,42 +118,35 @@
       expect(actual_attrs).to match_array(expected_attrs)
     end
 
-    it "supports find(:all) with records" do
-      with_records
-      actual_attrs = described_class.find(:all).map(&:attributes)
-      expect(actual_attrs).to match_array(expected_attrs)
-    end
-
     it "retrieves no records with no records" do
       with_no_records
       expect(described_class.all).to be_empty
-      expect(described_class.find(:all)).to be_empty
     end
   end
 
   describe ".first" do
     it "retrieves the first record with records" do
       with_records
-      rec = described_class.find(:first)
+      rec = described_class.first
       expect(rec.attributes).to eq(expected_attrs.first)
     end
 
     it "returns nil with no records" do
       with_no_records
-      expect(described_class.find(:first)).to be_nil
+      expect(described_class.first).to be_nil
     end
   end
 
   describe ".last" do
     it "retrieves the last record with :last" do
       with_records
-      rec = described_class.find(:last)
+      rec = described_class.last
       expect(rec.attributes).to eq(expected_attrs.last)
     end
 
     it "returns nil with :last" do
       with_no_records
-      expect(described_class.find(:last)).to be_nil
+      expect(described_class.last).to be_nil
     end
   end
 
@@ -264,7 +257,7 @@
       with_records
       allow(MiqRegionRemote).to receive(:with_remote_connection).and_yield(double(:connection))
 
-      sub = described_class.find(:first)
+      sub = described_class.first
       sub.host = "other-host.example.com"
       allow(sub).to receive(:assert_different_region!)
 
@@ -289,7 +282,7 @@
     end
 
     it "deletes all subscriptions if no parameter passed" do
-      allow(described_class).to receive(:find).with(:all).and_return([subscription1, subscription2])
+      allow(described_class).to receive(:search).with(:all, {}).and_return([subscription1, subscription2])
       expect(subscription1).to receive(:delete)
       expect(subscription2).to receive(:delete)
       described_class.delete_all
@@ -368,7 +361,7 @@
   end
 
   describe "#delete" do
-    let(:sub) { described_class.find(:first) }
+    let(:sub) { described_class.first }
 
     it "drops the subscription" do
       allow(pglogical).to receive(:subscriptions).and_return([subscriptions.first], [])
@@ -444,7 +437,7 @@
     it "validates existing subscriptions with new parameters" do
       allow(pglogical).to receive(:subscriptions).and_return([subscriptions.first])
 
-      sub = described_class.find(:first)
+      sub = described_class.first
       expect(sub.host).to eq "example.com"
       expect(sub.port).to be_blank
       expect(sub.user).to eq "root"