Fixes #38930 - Make fact parsers use proper os

Make sure the fact parser searches by all unique constraint combinations before creating a new object.

Author: ShimShtein

app/models/concerns/operating_system_naming.rb

@@ -0,0 +1,61 @@
+module OperatingSystemNaming
+  extend ActiveSupport::Concern
+
+  module ClassMethods
+    # Find all operatingsystems that are duplicates of the given operating system according to all unique constraints
+    def find_by_attributes(name: nil, major: nil, minor: nil, description: nil)
+      where_attributes = {
+        name: name,
+        major: major,
+        minor: minor,
+      }.compact
+      scope = where(where_attributes)
+      scope = scope.or(where(description: description)) if description.present?
+      scope.or(where(title: generate_title(**where_attributes.merge(description: description))))
+    end
+
+    def title_name
+      "title".freeze
+    end
+
+    def generate_title(description:, name:, major:, minor:)
+      to_label(description: description, name: name, major: major, minor: minor).to_s[0..254]
+    end
+
+    def to_label(description:, name:, major:, minor:)
+      return description if description.present?
+      fullname(name: name, major: major, minor: minor)
+    end
+
+    def fullname(name:, major:, minor:)
+      "#{name} #{release(major: major, minor: minor)}"
+    end
+
+    def release(major:, minor:)
+      "#{major}#{('.' + minor.to_s) if minor.present?}"
+    end
+
+    def find_by_to_label(str)
+      os = find_by_description(str.to_s)
+      return os if os
+      name, version = str.split(" ")
+      cond = {:name => name}
+      if version
+        (major, minor) = os_major_minor_from_version_str(name, version)
+        cond[:major] = major if major
+        cond[:minor] = minor if minor
+      end
+      find_by(cond)
+    end
+
+    def os_major_minor_from_version_str(os_name, version_str)
+      if os_name == 'Ubuntu'
+        x, y, minor = version_str.split('.', 3)
+        major = "#{x}.#{y}"
+      else
+        major, minor = version_str.split('.')
+      end
+      [major, minor]
+    end
+  end
+end

app/models/operatingsystem.rb

@@ -6,6 +6,7 @@ class Operatingsystem < ApplicationRecord
   include Authorizable
   include ValidateOsFamily
   include PxeLoaderSupport
+  include OperatingSystemNaming
   extend FriendlyId
   friendly_id :title
 
@@ -95,10 +96,6 @@ class Jail < Safemode::Jail
     allow :id, :name, :major, :minor, :family, :to_s, :==, :release, :release_name, :kernel, :initrd, :pxe_type, :boot_files_uri, :password_hash, :mediumpath, :bootfile
   end
 
-  def self.title_name
-    "title".freeze
-  end
-
   def additional_media(medium_provider)
     medium_provider.additional_media.map(&:with_indifferent_access)
   end
@@ -141,8 +138,7 @@ def self.families_as_collection
 
   # The OS is usually represented as the concatenation of the OS and the revision
   def to_label
-    return description if description.present?
-    fullname
+    self.class.to_label(description: description, name: name, major: major, minor: minor)
   end
 
   # to_label setter updates description and does not try to parse and update major, minor attributes
@@ -155,40 +151,17 @@ def to_param
   end
 
   def release
-    "#{major}#{('.' + minor.to_s) if minor.present?}"
+    self.class.release(major: major, minor: minor)
   end
 
   def fullname
-    "#{name} #{release}"
+    self.class.fullname(name: name, major: major, minor: minor)
   end
 
   def to_s
     fullname
   end
 
-  def self.find_by_to_label(str)
-    os = find_by_description(str.to_s)
-    return os if os
-    name, version = str.split(" ")
-    cond = {:name => name}
-    if version
-      (major, minor) = os_major_minor_from_version_str(name, version)
-      cond[:major] = major if major
-      cond[:minor] = minor if minor
-    end
-    find_by(cond)
-  end
-
-  def self.os_major_minor_from_version_str(os_name, version_str)
-    if os_name == 'Ubuntu'
-      x, y, minor = version_str.split('.', 3)
-      major = "#{x}.#{y}"
-    else
-      major, minor = version_str.split('.')
-    end
-    [major, minor]
-  end
-
   # Implemented only in the OSs subclasses where it makes sense
   def available_loaders
     ["None", "PXELinux BIOS"]
@@ -384,7 +357,7 @@ def set_family
   end
 
   def set_title
-    self.title = to_label.to_s[0..254]
+    self.title = self.class.generate_title(description: description, name: name, major: major, minor: minor)
   end
 
   def stringify_major_and_minor

app/services/foreman_ansible/operating_system_parser.rb

@@ -8,6 +8,7 @@ def operatingsystem
       args[:release_name] = os_release_name if os_name == 'Debian' || os_name == 'Ubuntu'
       # for Ansible, the CentOS Stream can be identified by missing minor version only
       args[:name] = "CentOS_Stream" if os_name == 'CentOS' && os_minor.blank?
+      args[:description] = os_description
       return @local_os if local_os(args).present?
       return @new_os if new_os(args).present?
       logger.debug do
@@ -20,15 +21,19 @@ def operatingsystem
     end
 
     def local_os(args)
-      @local_os = Operatingsystem.where(args).first
+      @local_os = Operatingsystem.find_by_attributes(**args.slice(:name, :major, :minor, :description)).first
     end
 
     def new_os(args)
       return @new_os if @new_os.present?
-      @new_os = Operatingsystem.new(args.merge(:description => os_description))
+      @new_os = generate_new_os(args)
       @new_os if @new_os.valid? && @new_os.save
     end
 
+    def generate_new_os(args)
+      Operatingsystem.new(args.merge(:description => os_description))
+    end
+
     def debian_os_major_sid
       release = if facts[:ansible_distribution_major_version] == 'n/a'
                   facts[:ansible_distribution_release]

app/services/foreman_chef/fact_parser.rb

@@ -51,7 +51,7 @@ def operatingsystem
       end
 
       args = { :name => os_name, :major => major.to_s, :minor => minor.to_s }
-      klass.where(args).first || klass.new(args.merge(:release_name => release_name)).tap(&:save!)
+      Operatingsystem.find_by_attributes(**args).first || klass.new(args.merge(:release_name => release_name)).tap(&:save!)
     end
 
     # we don't want to parse puppet environment, foreman_chef already has its own environment

app/services/foreman_salt/fact_parser.rb

@@ -3,7 +3,7 @@ class FactParser < ::FactParser
     attr_reader :facts
 
     def operatingsystem
-      os = Operatingsystem.where(os_hash).first_or_initialize
+      os = Operatingsystem.find_by_attributes(**os_hash).first || Operatingsystem.new(os_hash)
       if os.new_record?
         os.deduce_family
         os.release_name = facts[:oscodename] || facts[:lsb_distrib_codename]

app/services/katello/rhsm_fact_parser.rb

@@ -73,9 +73,9 @@ def operatingsystem
           os_attributes[:name] = "CentOS"
         end
 
-        os = ::Operatingsystem.find_by(os_attributes)
+        os = ::Operatingsystem.find_by_attributes(**os_attributes.slice(:name, :major, :minor, :description)).first
         if os.blank?
-          created = ::Operatingsystem.create_or_find_by(os_attributes)
+          created = ::Operatingsystem.create(os_attributes)
           created.errors.any? ? ::Operatingsystem.find_by(os_attributes) : created
         else
           os

app/services/puppet_fact_parser.rb

@@ -4,6 +4,8 @@ class PuppetFactParser < FactParser
   def operatingsystem
     orel = os_release.dup
 
+    args = { :name => os_name }
+
     if orel.present?
       if os_name =~ /ubuntu/i
         major = os_major_version.to_s
@@ -13,29 +15,24 @@ def operatingsystem
         major = major.to_s.gsub(/\D/, '')
         minor = minor.to_s.gsub(/[^\d\.]/, '')
       end
-      args = {:name => os_name, :major => major, :minor => minor}
-      os = Operatingsystem.find_or_initialize_by(args)
-      if os_name[/debian|ubuntu/i] || os.family == 'Debian'
-        if distro_codename
-          os.release_name = distro_codename
-        elsif os.release_name.blank?
-          os.release_name = 'unknown'
-        end
+      args[:major] = major
+      args[:minor] = minor
+      if os_name[/debian|ubuntu/i]
+        args[:release_name] = distro_codename if distro_codename
+        args[:release_name] = 'unknown' if args[:release_name].blank?
       end
-    else
-      os = Operatingsystem.find_or_initialize_by(:name => os_name)
     end
 
-    if os.description.blank?
-      if os_name == 'SLES'
-        os.description = os_name + ' ' + orel.gsub('.', ' SP')
-      elsif distro_description
-        family = os.deduce_family || 'Operatingsystem'
-        os = os.becomes(family.constantize)
-        os.description = os.shorten_description(distro_description)
-      end
+    if os_name == 'SLES'
+      args[:description] = "#{os_name} #{orel.gsub('.', ' SP')}"
+    elsif distro_description
+      family = Operatingsystem.deduce_family(os_name) || 'Operatingsystem'
+      os_class = family.constantize
+      args[:description] = os_class.new(args).shorten_description(distro_description)
     end
 
+    os = Operatingsystem.find_by_attributes(**args.slice(:name, :major, :minor, :description)).first || Operatingsystem.new(args)
+
     if os.new_record?
       os.save!
       Operatingsystem.find_by_id(os.id) # complete reload to be an instance of the STI subclass