Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

De-couple DescendantLoader from Rails #15460

Closed
wants to merge 3 commits into from
Closed

De-couple DescendantLoader from Rails #15460

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2ff8f43
Adds DescendantLoader.setup
NickLaMuro Jun 23, 2017
84cb87e
Require ActiveRecord in DescendantLoader
NickLaMuro Jun 27, 2017
cc17416
Allow DescendantLoader methods to work w/o Rails
NickLaMuro Jun 27, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion lib/acts_as_ar_model.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
require_relative "extensions/descendant_loader.rb"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove the .rb

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm concerned that this breaks Rails autoloading, but that concern is probably minor/unwarranted.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will look into this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following the precedent from my other recent comment:

  • Will remove the .rb (for some reason I thought it was required... or helped in some way, but nope)
  • Not sure how I will test the autoloading concern to be honest. Might take me more time to investigate then I probably should at this time, mostly because I only so deep of knowledge on the internal workings of Rails' autoloading that I probably would need to do some spelunking to properly give a "yay" or "nay". I will try and update regardless.


class ActsAsArModel
include Vmdb::Logging

Expand All @@ -21,7 +23,11 @@ def self.base_class
superclass == ActsAsArModel ? self : superclass.base_class
end

class << self; alias_method :base_model, :base_class; end
class << self
alias base_model base_class

prepend DescendantLoader::ArDescendantsWithLoader
end

#
# Column methods
Expand Down
19 changes: 14 additions & 5 deletions lib/extensions/descendant_loader.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,10 @@
# When Active Record calls `Bbb.descendants` to construct the `type`
# condition, `Ccc` is automatically loaded.
#

require "active_record"
require_relative "../manageiq"

class DescendantLoader
CACHE_VERSION = 2

Expand All @@ -63,6 +67,13 @@ def self.status(io = $stdout)
io.puts
end

def self.setup
return if @setup_complete
ActiveRecord::Base.singleton_class.send(:prepend, DescendantLoader::ArDescendantsWithLoader)
ActiveSupport::Dependencies.send(:prepend, DescendantLoader::AsDependenciesClearWithLoader)
@setup_complete = true
end

# Extract class definitions (namely: a list of which scopes it might
# be defined in [depending on runtime details], the name of the class,
# and the name of its superclass), given a path to a ruby script file.
Expand Down Expand Up @@ -159,7 +170,7 @@ def name_combinations(names)
# RubyParser is slow, so wrap it in a simple mtime-based cache.
module Cache
def cache_path
Rails.root.join('tmp/cache/sti_loader.yml')
ManageIQ.root.join('tmp/cache/sti_loader.yml')
end

def load_cache
Expand Down Expand Up @@ -199,7 +210,7 @@ def classes_in(filename)

module Mapper
def descendants_paths
@descendants_paths ||= [Rails.root.join("app/models")]
@descendants_paths ||= [ManageIQ.root.join("app/models")]
end

def class_inheritance_relationships
Expand Down Expand Up @@ -266,9 +277,7 @@ def clear
end
end

ActiveRecord::Base.singleton_class.send(:prepend, DescendantLoader::ArDescendantsWithLoader)
ActiveSupport::Dependencies.send(:prepend, DescendantLoader::AsDependenciesClearWithLoader)
ActsAsArModel.singleton_class.send(:prepend, DescendantLoader::ArDescendantsWithLoader)
DescendantLoader.setup

at_exit do
Copy link
Member

@Fryguy Fryguy Jul 5, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically, this should be part of the setup as well. (The at_exit I mean, if my comment here was not clear )

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe address this comment?

Copy link
Member Author

@NickLaMuro NickLaMuro Jan 11, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. Will do. Should have confirmed that I intended to do that originally. Will look into doing this tomorrow.

DescendantLoader.instance.save_cache!
Expand Down