[WIP] Switch ActsAsArModel to be QueryRelation based by default#23618
[WIP] Switch ActsAsArModel to be QueryRelation based by default#23618Fryguy wants to merge 1 commit intoManageIQ:masterfrom
Conversation
| end | ||
| | ||
| singleton_class.send(:alias_method, :find_by_id, :lookup_by_id) | ||
| Vmdb::Deprecation.deprecate_methods(singleton_class, :find_by_id => :lookup_by_id) |
There was a problem hiding this comment.
This deprecation was moved into the base class.
There was a problem hiding this comment.
I don't think we use find_by_id anymore. I did find in a few spots, but they look rare.
Maybe your search shows more callers (to any class)
Fryguy force-pushed the aarm_is_query_relation branch from df9c3b9 to 084b6ec Compare Fryguy force-pushed the aarm_is_query_relation branch from 084b6ec to 1471303 Compare | Interestingly no deprecations showed up in the tests...let's see what cross repo shows @miq-bot cross-repo-test /all |
From Pull Request: ManageIQ/manageiq#23618
| 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 |
There was a problem hiding this comment.
yea, think we can drop support for find(:all).
We have no callers and only class implements this interface
There was a problem hiding this comment.
Think we can be more yolo here.
The interface for QueryRelation is search. We decided to convert that to legacy find, for all of our AAArM implementations, but since PglogicalSubscription is the only class that implemented find(), lets just fix it in the one place and not keep around the legacy find support and all.
I am a little concerned that Chargebacks and VimPerformanceTrends do not properly implement the AAArM interface, but it looks like reports call a custom method to run those reports. (i.e.: trend.rb )
Don't feel we should keep dead weight in our interface when we don't have anything using that interface (except for pglogical)
| Yeah I was concerned maybe something was using that old interface that's why I added the deprecations but nothing showing up as deprecations so I don't think it's used |
| This pull request is not mergeable. Please rebase and repush. |
kbrock left a comment
kbrock left a comment There was a problem hiding this comment.
This is a great change. It would be a shame not get it in.
| # | ||
| def self.find(*_args) | ||
| raise NotImplementedError, _("find must be implemented in a subclass") | ||
| def self.find(mode_or_id, *args) |
There was a problem hiding this comment.
Really think we can drop this. The only place we use it is in PgLogical (as mentioned in the previous list). This will remove legacy_options and a lot of other stuff. (Not sure the legacy options even work in current versions of Rails.)
| end | ||
| | ||
| singleton_class.send(:alias_method, :find_by_id, :lookup_by_id) | ||
| Vmdb::Deprecation.deprecate_methods(singleton_class, :find_by_id => :lookup_by_id) |
There was a problem hiding this comment.
I don't think we use find_by_id anymore. I did find in a few spots, but they look rare.
Maybe your search shows more callers (to any class)
3 participants
This flips ActsAsArModel to be search-first (i.e. via QueryRelation) and deprecates the old-style find method that takes :all, :first, or :last (However, find with an id is still supported just like in Rails). This also deprecates the old-style find_by_id and find_all_by_id.
WIP: I think I should change the default implementation of
PglogicalSubscription#searchto handlewhere :id- right now it can't handle it.cc @kbrock