fixes #10832 - separating lookup keys into puppet and variable

app/controllers/api/v1/lookup_keys_controller.rb

@@ -11,9 +11,10 @@ class LookupKeysController < V1::BaseController
       param :per_page, String, :desc => "number of entries per request"
 
       def index
-        @lookup_keys = LookupKey.
-          authorized(:view_external_variables).
-          search_for(*search_options).paginate(paginate_options)
+        @lookup_keys = PuppetclassLookupKey.authorized(:view_external_variables).
+          search_for(*search_options).paginate(paginate_options).to_a +
+            VariableLookupKey.authorized(:view_external_variables).
+            search_for(*search_options).paginate(paginate_options).to_a
       end
 
       api :GET, "/lookup_keys/:id/", "Show a lookup key."

app/controllers/concerns/api/v2/lookup_keys_common_controller.rb

@@ -69,9 +69,9 @@ def find_hostgroup
 
   def find_smart_variable
     id = params.keys.include?('smart_variable_id') ? params['smart_variable_id'] : params['id']
-    @smart_variable   = LookupKey.authorized(:view_external_variables).smart_variables.find_by_id(id.to_i) if id.to_i > 0
+    @smart_variable   = VariableLookupKey.authorized(:view_external_variables).smart_variables.find_by_id(id.to_i) if id.to_i > 0
     @smart_variable ||= (puppet_cond = { :puppetclass_id => @puppetclass.id } if @puppetclass
-                         LookupKey.authorized(:view_external_variables).smart_variables.where(puppet_cond).find_by_key(id)
+                         VariableLookupKey.authorized(:view_external_variables).smart_variables.where(puppet_cond).find_by_key(id)
                         )
     @smart_variable
   end
@@ -81,19 +81,19 @@ def find_smart_variables
   end
 
   def smart_variables_resource_scope
-    return LookupKey.authorized(:view_external_variables).smart_variables unless (@puppetclass || @host || @hostgroup)
+    return VariableLookupKey.authorized(:view_external_variables).smart_variables unless (@puppetclass || @host || @hostgroup)
     puppetclass_ids   = @puppetclass.id if @puppetclass
     puppetclass_ids ||= @hostgroup.all_puppetclasses.map(&:id) if @hostgroup
     puppetclass_ids ||= @host.all_puppetclasses.map(&:id) if @host
-    LookupKey.authorized(:view_external_variables).global_parameters_for_class(puppetclass_ids)
+    VariableLookupKey.authorized(:view_external_variables).global_parameters_for_class(puppetclass_ids)
   end
 
   def find_smart_class_parameter
     id = params.keys.include?('smart_class_parameter_id') ? params['smart_class_parameter_id'] : params['id']
-    @smart_class_parameter = LookupKey.authorized(:view_external_variables).smart_class_parameters.find_by_id(id.to_i) if id.to_i > 0
+    @smart_class_parameter = PuppetclassLookupKey.authorized(:view_external_variables).smart_class_parameters.find_by_id(id.to_i) if id.to_i > 0
     @smart_class_parameter ||= (puppet_cond = { 'environment_classes.puppetclass_id'=> @puppetclass.id } if @puppetclass
                                 env_cond = { 'environment_classes.environment_id' => @environment.id } if @environment
-                                LookupKey.authorized(:view_external_variables).smart_class_parameters.where(puppet_cond).where(env_cond).where(:key => id).first
+                                PuppetclassLookupKey.authorized(:view_external_variables).smart_class_parameters.where(puppet_cond).where(env_cond).where(:key => id).first
                                )
     @smart_class_parameter
   end
@@ -103,7 +103,7 @@ def find_smart_class_parameters
   end
 
   def smart_class_parameters_resource_scope
-    base = LookupKey.authorized(:view_external_variables)
+    base = PuppetclassLookupKey.authorized(:view_external_variables)
     return base.smart_class_parameters unless (@puppetclass || @environment || @host || @hostgroup)
     if @puppetclass && @environment
       base.smart_class_parameters_for_class(@puppetclass.id, @environment.id)

app/controllers/lookup_keys_controller.rb

@@ -4,25 +4,24 @@ class LookupKeysController < ApplicationController
   before_filter :find_resource, :only => [:edit, :update, :destroy], :if => Proc.new { params[:id] }
 
   def index
-    @lookup_keys = resource_base.search_for(params[:search], :order => params[:order])
-                                .includes(:param_classes)
-                                .paginate(:page => params[:page])
+    @lookup_keys = resource_base.search_for(params[:search], :order => params[:order]).includes(:puppetclass)
+                       .paginate(:page => params[:page])
     @puppetclass_authorizer = Authorizer.new(User.current, :collection => @lookup_keys.map(&:puppetclass_id).compact.uniq)
   end
 
   def edit
   end
 
   def update
-    if @lookup_key.update_attributes(params[:lookup_key])
+    if resource.update_attributes(params[resource_name])
       process_success
     else
       process_error
     end
   end
 
   def destroy
-    if @lookup_key.destroy
+    if resource.destroy
       process_success
     else
       process_error
@@ -34,4 +33,8 @@ def destroy
   def controller_permission
     'external_variables'
   end
+
+  def resource
+    instance_variable_get("@#{resource_name}")
+  end
 end

app/controllers/puppetclass_lookup_keys_controller.rb

@@ -0,0 +1,10 @@
+class PuppetclassLookupKeysController < LookupKeysController
+  before_filter :setup_search_options, :only => :index
+
+  def index
+    @lookup_keys = resource_base.search_for(params[:search], :order => params[:order])
+                                .paginate(:page => params[:page])
+                                .includes(:param_classes)
+    @puppetclass_authorizer = Authorizer.new(User.current, :collection => @lookup_keys.map(&:puppetclass_id).compact.uniq)
+  end
+end

app/controllers/variable_lookup_keys_controller.rb

@@ -0,0 +1,2 @@
+class VariableLookupKeysController < LookupKeysController
+end

app/helpers/hosts_and_hostgroups_helper.rb

@@ -103,8 +103,8 @@ def realm_field(f, can_override = false, override = false)
   def interesting_klasses(obj)
     classes    = obj.all_puppetclasses
     classes_ids = classes.reorder('').pluck('puppetclasses.id')
-    smart_vars = LookupKey.reorder('').where(:puppetclass_id => classes_ids).uniq.pluck(:puppetclass_id)
-    class_vars = LookupKey.reorder('').joins(:environment_classes).where(:environment_classes => { :puppetclass_id => classes_ids }).uniq.pluck('environment_classes.puppetclass_id')
+    smart_vars = VariableLookupKey.reorder('').where(:puppetclass_id => classes_ids).uniq.pluck(:puppetclass_id)
+    class_vars = PuppetclassLookupKey.reorder('').joins(:environment_classes).where(:environment_classes => { :puppetclass_id => classes_ids }).uniq.pluck('environment_classes.puppetclass_id')
     klasses    = (smart_vars + class_vars).uniq
 
     classes.where(:id => klasses)

app/helpers/lookup_keys_helper.rb

@@ -35,7 +35,7 @@ def show_puppet_class(f)
     # Show the parent puppetclass as a context, but permit no change.
     if params["puppetclass_id"]
       select_f f, :puppetclass_id, [Puppetclass.find(params["puppetclass_id"])], :id, :to_label, {}, {:label => _("Puppet class"), :disabled => true}
-    elsif f.object.is_param && f.object.param_class
+    elsif f.object.puppet? && f.object.param_class
       field(f, :puppetclass_id, :label => _('Puppet Class')) do
         content_tag(:input, nil, :value => f.object.param_class, :type => 'text', :disabled => true)
       end
@@ -47,7 +47,7 @@ def show_puppet_class(f)
 
   def param_type_selector(f, options = {})
     selectable_f f, :key_type, options_for_select(LookupKey::KEY_TYPES.map { |e| [_(e),e] }, f.object.key_type),{},
-                 options.merge({ :disabled => (f.object.is_param && !f.object.override), :size => "col-md-8", :class=> "without_select2",
+                 options.merge({ :disabled => (f.object.puppet? && !f.object.override), :size => "col-md-8", :class=> "without_select2",
                  :help_inline => popover("",_("<dl>" +
                "<dt>String</dt> <dd>Everything is taken as a string.</dd>" +
                "<dt>Boolean</dt> <dd>Common representation of boolean values are accepted.</dd>" +
@@ -62,7 +62,7 @@ def param_type_selector(f, options = {})
 
   def validator_type_selector(f)
     selectable_f f, :validator_type, options_for_select(LookupKey::VALIDATOR_TYPES.map { |e| [_(e),e]  }, f.object.validator_type),{:include_blank => _("None")},
-               { :disabled => (f.object.is_param && !f.object.override), :size => "col-md-8", :class=> "without_select2",
+               { :disabled => (f.object.puppet? && !f.object.override), :size => "col-md-8", :class=> "without_select2",
                  :onchange => 'validatorTypeSelected(this)',
                  :help_inline => popover("",_("<dl>" +
                "<dt>List</dt> <dd>A list of the allowed values, specified in the Validator rule field.</dd>" +

app/models/concerns/counter_cache_fix.rb

@@ -14,9 +14,19 @@ def update_counter_caches
         if key =~ /_id/
           association = self.association(key.sub(/_id$/, '').to_sym)
           if association.options[ :counter_cache ]
-            counter_name = self.class.name.underscore.split("/")[0].pluralize.to_sym
-            association.klass.reset_counters(old_value, counter_name) if old_value
-            association.klass.reset_counters(new_value, counter_name) if new_value
+
+# in case of counter cache on STI, specify the :inverse_of option, otherwise, we might throw an error.
+# i.e. VariableLookupKey belongs to puppetclass, but the association name on the puppetclass is lookup_keys, not puppetclass_lookup_keys
+# thus, we define the correct name for the association to be derived.
+            if association.options[:inverse_of].is_a?(Symbol)
+              counter_name = association.options[:inverse_of]
+            else
+              counter_name = self.class.name.underscore.split("/")[0].pluralize.to_sym
+            end
+
+            association_name = counter_name.to_s.sub(/_count$/, "").to_sym
+            association.klass.reset_counters(old_value, association_name) if old_value
+            association.klass.reset_counters(new_value, association_name) if new_value
           end
         end
       end

app/models/environment_class.rb

@@ -1,42 +1,52 @@
 class EnvironmentClass < ActiveRecord::Base
   belongs_to :environment
   belongs_to :puppetclass
-  belongs_to :lookup_key
-  validates :lookup_key_id, :uniqueness => {:scope => [:environment_id, :puppetclass_id]}
+  belongs_to :puppetclass_lookup_key
+  validates :puppetclass_lookup_key_id, :uniqueness => {:scope => [:environment_id, :puppetclass_id]}
   validates :puppetclass_id, :environment_id, :presence => true
 
   scope :parameters_for_class, lambda {|puppetclasses_ids, environment_id|
-    all_parameters_for_class(puppetclasses_ids, environment_id).where(:lookup_keys => {:override => true})
+    all_parameters_for_class(puppetclasses_ids, environment_id).where(:puppetclass_lookup_keys => {:override => true})
   }
 
   scope :all_parameters_for_class, lambda {|puppetclasses_ids, environment_id|
     where(:puppetclass_id => puppetclasses_ids, :environment_id => environment_id).
-      where('lookup_key_id is NOT NULL').
-      includes(:lookup_key)
+      where('puppetclass_lookup_key_id is NOT NULL').
+      includes(:puppetclass_lookup_key)
   }
 
-  scope :used_by_other_environment_classes, lambda{|lookup_key_id, this_environment_class_id|
-    where(:lookup_key_id => lookup_key_id).
+  scope :used_by_other_environment_classes, lambda{|puppetclass_lookup_key_id, this_environment_class_id|
+    where(:puppetclass_lookup_key_id => puppetclass_lookup_key_id).
       where("id != #{this_environment_class_id}")
   }
 
   # These counters key track of unique puppet class keys (parameters) across environments
   after_create do |record|
-    Puppetclass.increment_counter(:global_class_params_count, self.puppetclass.id) unless self.lookup_key.blank? ||
-      EnvironmentClass.used_by_other_environment_classes(self.lookup_key, self.id).count > 0
+    Puppetclass.increment_counter(:global_class_params_count, self.puppetclass.id) unless self.puppetclass_lookup_key.blank? ||
+      EnvironmentClass.used_by_other_environment_classes(self.puppetclass_lookup_key, self.id).count > 0
   end
 
   after_destroy do |record|
-    Puppetclass.decrement_counter(:global_class_params_count, self.puppetclass.id) unless self.lookup_key.blank? ||
-      EnvironmentClass.used_by_other_environment_classes(self.lookup_key, self.id).count > 0
+    Puppetclass.decrement_counter(:global_class_params_count, self.puppetclass.id) unless self.puppetclass_lookup_key.blank? ||
+      EnvironmentClass.used_by_other_environment_classes(self.puppetclass_lookup_key, self.id).count > 0
+  end
+
+  def lookup_key_id=(val)
+    Foreman::Deprecation.deprecation_warning("1.12", "lookup_key_id= is deprecated, please use puppetclass_lookup_key_id= instead.")
+    self.puppetclass_lookup_key_id=val
+  end
+
+  def lookup_key=(val)
+    Foreman::Deprecation.deprecation_warning("1.12", "lookup_key= is deprecated, please use puppetclass_lookup_key= instead.")
+    self.puppetclass_lookup_key=val
   end
 
   #TODO move these into scopes?
-  def self.is_in_any_environment(puppetclass, lookup_key)
-    EnvironmentClass.where(:puppetclass_id => puppetclass, :lookup_key_id => lookup_key ).count > 0
+  def self.is_in_any_environment(puppetclass, puppetclass_lookup_key)
+    EnvironmentClass.where(:puppetclass_id => puppetclass, :puppetclass_lookup_key_id => puppetclass_lookup_key ).count > 0
   end
 
-  def self.key_in_environment(env, puppetclass,  lookup_key)
-    EnvironmentClass.where(:environment_id => env, :puppetclass_id => puppetclass, :lookup_key_id => lookup_key ).first
+  def self.key_in_environment(env, puppetclass,  puppetclass_lookup_key)
+    EnvironmentClass.where(:environment_id => env, :puppetclass_id => puppetclass, :puppetclass_lookup_key_id => puppetclass_lookup_key ).first
   end
 end

app/models/lookup_key.rb → app/models/lookup_keys/lookup_key.rb

@@ -14,65 +14,39 @@ class LookupKey < ActiveRecord::Base
 
   serialize :default_value
 
-  belongs_to :puppetclass, :inverse_of => :lookup_keys, :counter_cache => true
-  has_many :environment_classes, :dependent => :destroy
-  has_many :environments, :through => :environment_classes, :uniq => true
-  has_many :param_classes, :through => :environment_classes, :source => :puppetclass
-  def param_class
-    param_classes.first
-  end
-
-  def audit_class
-    param_class || puppetclass
-  end
-
   has_many :lookup_values, :dependent => :destroy, :inverse_of => :lookup_key
   accepts_nested_attributes_for :lookup_values,
                                 :reject_if => :reject_invalid_lookup_values,
                                 :allow_destroy => true
 
-  before_validation :cast_default_value, :unless => Proc.new{|p| p.use_puppet_default }
-  validates :key, :uniqueness => {:scope => :is_param }, :unless => Proc.new{|p| p.is_param?}
+  before_validation :cast_default_value
 
   validates :key, :presence => true
-  validates :puppetclass, :presence => true, :unless => Proc.new {|k| k.is_param?}
   validates :validator_type, :inclusion => { :in => VALIDATOR_TYPES, :message => N_("invalid")}, :allow_blank => true, :allow_nil => true
   validates :key_type, :inclusion => {:in => KEY_TYPES, :message => N_("invalid")}, :allow_blank => true, :allow_nil => true
   validate :validate_default_value
   validates_associated :lookup_values
-  validate :ensure_type, :disable_merge_overrides, :disable_avoid_duplicates, :disable_merge_default
+  validate :disable_merge_overrides, :disable_avoid_duplicates, :disable_merge_default
 
   before_save :sanitize_path
   attr_name :key
 
-  scoped_search :on => :key, :complete_value => true, :default_order => true
-  scoped_search :on => :lookup_values_count, :rename => :values_count
-  scoped_search :on => :override, :complete_value => {:true => true, :false => false}
-  scoped_search :on => :merge_overrides, :complete_value => {:true => true, :false => false}
-  scoped_search :on => :merge_default, :complete_value => {:true => true, :false => false}
-  scoped_search :on => :avoid_duplicates, :complete_value => {:true => true, :false => false}
-  scoped_search :in => :param_classes, :on => :name, :rename => :puppetclass, :complete_value => true
-  scoped_search :in => :lookup_values, :on => :value, :rename => :value, :complete_value => true
+  def self.inherited(child)
+    child.instance_eval do
+      scoped_search :on => :key, :complete_value => true, :default_order => true
+      scoped_search :on => :lookup_values_count, :rename => :values_count
+      scoped_search :on => :override, :complete_value => {:true => true, :false => false}
+      scoped_search :on => :merge_overrides, :complete_value => {:true => true, :false => false}
+      scoped_search :on => :merge_default, :complete_value => {:true => true, :false => false}
+      scoped_search :on => :avoid_duplicates, :complete_value => {:true => true, :false => false}
+      scoped_search :in => :lookup_values, :on => :value, :rename => :value, :complete_value => true
+    end
+  end
 
   default_scope lambda { order('lookup_keys.key') }
 
   scope :override, lambda { where(:override => true) }
 
-  scope :smart_class_parameters_for_class, lambda {|puppetclass_ids, environment_id|
-    joins(:environment_classes).where(:environment_classes => {:puppetclass_id => puppetclass_ids, :environment_id => environment_id})
-  }
-
-  scope :parameters_for_class, lambda {|puppetclass_ids, environment_id|
-    override.smart_class_parameters_for_class(puppetclass_ids,environment_id)
-  }
-
-  scope :global_parameters_for_class, lambda {|puppetclass_ids|
-    where(:puppetclass_id => puppetclass_ids)
-  }
-
-  scope :smart_variables, lambda { where('lookup_keys.puppetclass_id > 0').readonly(false) }
-  scope :smart_class_parameters, lambda { where(:is_param => true).joins(:environment_classes).readonly(false) }
-
   # new methods for API instead of revealing db names
   alias_attribute :parameter, :key
   alias_attribute :variable, :key
@@ -88,16 +62,12 @@ def self.find_by_name(str)
     nil
   end
 
-  def to_label
-    "#{audit_class}::#{key}"
-  end
-
-  def is_smart_variable?
-    puppetclass_id.to_i > 0
+  def audit_class
+    self
   end
 
-  def is_smart_class_parameter?
-    is_param? && environment_classes.any?
+  def to_label
+    "#{audit_class}::#{key}"
   end
 
   def supports_merge?
@@ -172,6 +142,10 @@ def overridden?(host)
     lookup_values.find_by_match(host.send(:lookup_value_match)).present?
   end
 
+  def puppet?
+    false
+  end
+
   private
 
   # Generate possible lookup values type matches to a given host
@@ -232,12 +206,6 @@ def load_yaml_or_json(value)
     end
   end
 
-  def ensure_type
-    if puppetclass_id.present? and is_param?
-      self.errors.add(:base, _('Global variable or class Parameter, not both'))
-    end
-  end
-
   def validate_default_value
     Foreman::Parameters::Validator.new(self,
       :type => validator_type,