Optimize entitlement calculation hot paths

lib/entitlements/data/groups/calculated.rb

@@ -97,11 +97,12 @@ def self.read_all(ou_key, cfg_obj, skip_broken_references: false)
               options = { skip_broken_references: skip_broken_references }
 
               Entitlements.cache[:file_objects][filename] ||= ruleset(filename: filename, config: cfg_obj, options: options)
+              file_object = Entitlements.cache[:file_objects][filename]
               @groups_cache[group_dn] = Entitlements::Models::Group.new(
                 dn: group_dn,
-                members: Entitlements.cache[:file_objects][filename].modified_filtered_members,
-                description: Entitlements.cache[:file_objects][filename].description,
-                metadata: Entitlements.cache[:file_objects][filename].metadata.merge("_filename" => filename)
+                members: file_object.modified_filtered_members,
+                description: file_object.description,
+                metadata: file_object.metadata.merge("_filename" => filename)
               )
               result.add group_dn
             end

lib/entitlements/data/groups/calculated/base.rb

@@ -117,28 +117,11 @@ def filtered_members
               result = members.dup
               filters.reject { |_, filter_val| filter_val == :all }.each do |filter_name, filter_val|
                 filter_cfg = Entitlements::Data::Groups::Calculated.filters_index[filter_name]
+                next unless filter_applies?(filter_cfg.fetch(:config, {}))
+
                 clazz = filter_cfg.fetch(:class)
                 obj = clazz.new(filter: filter_val, config: filter_cfg.fetch(:config, {}))
-                # If excluded_paths is set, ignore any of those excluded paths
-                unless filter_cfg[:config]["excluded_paths"].nil?
-                  # if the filename is not in any of the excluded paths, filter it
-                  unless filter_cfg[:config]["excluded_paths"].any? { |excluded_path| filename.include?(excluded_path) }
-                    result.reject! { |member| obj.filtered?(member) }
-                  end
-                end
-
-                # if included_paths is set, filter only files at those included paths
-                unless filter_cfg[:config]["included_paths"].nil?
-                  # if the filename is in any of the included paths, filter it
-                  if filter_cfg[:config]["included_paths"].any? { |included_path| filename.include?(included_path) }
-                    result.reject! { |member| obj.filtered?(member) }
-                  end
-                end
-
-                # if neither included_paths nor excluded_paths are set, filter normally
-                if filter_cfg[:config]["included_paths"].nil? and filter_cfg[:config]["excluded_paths"].nil?
-                  result.reject! { |member| obj.filtered?(member) }
-                end
+                result.reject! { |member| obj.filtered?(member) }
               end
               result
             end
@@ -170,6 +153,17 @@ def modified_filtered_members
 
           attr_reader :config, :options
 
+          def filter_applies?(filter_config)
+            included_paths = filter_config["included_paths"]
+            excluded_paths = filter_config["excluded_paths"]
+
+            return true if included_paths.nil? && excluded_paths.nil?
+            return true if included_paths&.any? { |included_path| filename.include?(included_path) }
+            return true if excluded_paths&.none? { |excluded_path| filename.include?(excluded_path) }
+
+            false
+          end
+
           # Common method that takes a given list of members and applies the modifiers.
           # Used to calculated `modified_members` and `modified_filtered_members`.
           #

lib/entitlements/data/groups/calculated/filters/base.rb

@@ -49,20 +49,13 @@ def initialize(filter:, config: {})
             # member - Entitlements::Models::Person object
             #
             # Returns true if a member of the filter conditions, false otherwise.
-            Contract Entitlements::Models::Person => C::Bool
             def member_of_filter?(member)
-              # First handle all username entries, regardless of order, because we do not
-              # have to mess around with reading groups for those.
-              filter.reject { |filter_val| filter_val =~ /\// }.each do |filter_val|
-                return true if filter_val.downcase == member.uid.downcase
-              end
+              return true if filter_usernames.include?(member.uid.downcase)
 
-              # Now handle all group entries.
-              filter.select { |filter_val| filter_val =~ /\// }.each do |filter_val|
+              filter_groups.each do |filter_val|
                 return true if member_of_named_group?(member, filter_val)
               end
 
-              # If we get here there was no match.
               false
             end
 
@@ -73,18 +66,27 @@ def member_of_filter?(member)
             # group_ref - Optionally a string with a reference to a group to look up
             #
             # Returns true if a member of the group, false otherwise.
-            Contract Entitlements::Models::Person, String => C::Bool
             def member_of_named_group?(member, group_ref)
               Entitlements.cache[:member_of_named_group] ||= {}
               Entitlements.cache[:member_of_named_group][group_ref] ||= begin
                 member_set = Entitlements::Data::Groups::Calculated::Rules::Group.matches(
                   value: group_ref,
                 )
-                member_set.map { |person| person.uid.downcase }
+                member_set.each_with_object(Set.new) { |person, result| result.add(person.uid.downcase) }
               end
 
               Entitlements.cache[:member_of_named_group][group_ref].include?(member.uid.downcase)
             end
+
+            def filter_usernames
+              @filter_usernames ||= filter.each_with_object(Set.new) do |filter_val, result|
+                result.add(filter_val.downcase) unless filter_val.include?("/")
+              end
+            end
+
+            def filter_groups
+              @filter_groups ||= filter.select { |filter_val| filter_val.include?("/") }
+            end
           end
         end
       end

lib/entitlements/data/groups/calculated/filters/member_of_group.rb

@@ -17,7 +17,6 @@ class MemberOfGroup < Entitlements::Data::Groups::Calculated::Filters::Base
             # member - Entitlements::Models::Person object
             #
             # Returns true if the person is to be filtered out, false otherwise.
-            Contract Entitlements::Models::Person => C::Bool
             def filtered?(member)
               return false if filter == :all
               return false unless member_of_named_group?(member, config.fetch("group"))

lib/entitlements/data/groups/calculated/rules/group.rb

@@ -41,15 +41,6 @@ def self.matches(value:, filename: nil, options: {})
                 raise "Error: Circular dependency #{Entitlements.cache[:dependencies].join(' -> ')}"
               end
 
-              # If we have calculated this before, then apply modifiers and return the result. `current_value` here
-              # is a set with the correct answers but that does not take into effect any modifiers. Therefore we
-              # reference back to the object so we can have the modifiers applied. There's a cache in the object that
-              # remembers the value of `.modified_members` each time it's calculated, so this is inexpensive.
-              filebase_with_path = File.join(Entitlements::Util::Util.path_for_group(ou), cn)
-              if Entitlements.cache[:file_objects].key?(filebase_with_path)
-                return Entitlements.cache[:file_objects][filebase_with_path].modified_members
-              end
-
               # We actually need to calculate this group. Find the file based on the ou and cn in the directory.
               files = files_for(ou, options: options)
               match_regex = Regexp.new("\\A" + Regexp.escape(cn.gsub("*", "\f")).gsub("\\f", ".*") + "\\z")
@@ -58,23 +49,26 @@ def self.matches(value:, filename: nil, options: {})
                 result = Set.new
                 matching_files.each do |filebase, ext|
                   filebase_with_path = File.join(Entitlements::Util::Util.path_for_group(ou), filebase)
+                  target_filename = "#{filebase_with_path}.#{ext}"
 
                   # If the object has already been calculated then we can just merge the value from
                   # the cache without going any further. Otherwise, create a new object for the group
                   # reference and calculate them.
-                  unless Entitlements.cache[:file_objects][filebase_with_path]
-                    clazz = Kernel.const_get(FILE_EXTENSIONS[ext])
-                    Entitlements.cache[:file_objects][filebase_with_path] = clazz.new(
-                      filename: "#{filebase_with_path}.#{ext}",
+                  unless Entitlements.cache[:file_objects][target_filename]
+                    target_config = Entitlements.config.fetch("groups", {})[ou] || {}
+                    Entitlements.cache[:file_objects][target_filename] = Entitlements::Data::Groups::Calculated.ruleset(
+                      filename: target_filename,
+                      config: target_config,
+                      options: options,
                     )
-                    if Entitlements.cache[:file_objects][filebase_with_path].members == :calculating
+                    if Entitlements.cache[:file_objects][target_filename].members == :calculating
                       next if matching_files.size > 1
                       raise "Error: Invalid self-referencing wildcard in #{ou}/#{filebase}.#{ext}"
                     end
                   end
 
-                  unless Entitlements.cache[:file_objects][filebase_with_path].modified_members == :calculating
-                    result.merge Entitlements.cache[:file_objects][filebase_with_path].modified_members
+                  unless Entitlements.cache[:file_objects][target_filename].modified_members == :calculating
+                    result.merge Entitlements.cache[:file_objects][target_filename].modified_members
                   end
                 end
                 return result

lib/entitlements/extras/ldap_group/filters/member_of_ldap_group.rb

@@ -16,7 +16,6 @@ class MemberOfLDAPGroup < Entitlements::Data::Groups::Calculated::Filters::Base
           # member - Entitlements::Models::Person object
           #
           # Returns true if the person is to be filtered out, false otherwise.
-          Contract Entitlements::Models::Person => C::Bool
           def filtered?(member)
             return false if filter == :all
             return false unless member_of_ldap_group?(member, config.fetch("ldap_group"))
@@ -31,14 +30,13 @@ def filtered?(member)
           # group_dn - LDAP distinguished name of the group
           #
           # Returns true if a member of the group, false otherwise.
-          Contract Entitlements::Models::Person, String => C::Bool
           def member_of_ldap_group?(member, group_dn)
             Entitlements.cache[:member_of_ldap_group] ||= {}
             Entitlements.cache[:member_of_ldap_group][group_dn] ||= begin
               member_set = Entitlements::Extras::LDAPGroup::Rules::LDAPGroup.matches(value: group_dn)
-              member_set.map { |person| person.uid.downcase }
+              member_set.each_with_object(Set.new) { |person, result| result.add(person.uid.downcase) }
             rescue Entitlements::Data::Groups::GroupNotFoundError
-              []
+              Set.new
             end
 
             Entitlements.cache[:member_of_ldap_group][group_dn].include?(member.uid.downcase)

spec/unit/entitlements/data/groups/calculated/base_spec.rb

@@ -109,6 +109,33 @@
     end
   end
 
+  describe "#advanced_filters - included and excluded paths" do
+    let(:file) { fixture("ldap-config/filters/included-path-filters.yaml") }
+    let(:obj) { Entitlements::Data::Groups::Calculated::YAML.new(filename: file, config: config) }
+    let(:config) { { "base" => "ou=Felines,ou=Groups,dc=kittens,dc=net" } }
+
+    it "checks each member once when both path rules select the file" do
+      filter_cfg = {
+        class: Entitlements::Data::Groups::Calculated::Filters::MemberOfGroup,
+        config: {
+          "group" => "internal/workday/on-leave",
+          "included_paths" => ["ldap-config/filters"],
+          "excluded_paths" => ["fake-path/is-fake"]
+        }
+      }
+      Entitlements::Data::Groups::Calculated.register_filter("included-paths", filter_cfg)
+      russianblue = people_obj.read["russianblue"]
+      blackmanx = people_obj.read["blackmanx"]
+
+      filter = instance_double(Entitlements::Data::Groups::Calculated::Filters::MemberOfGroup)
+      expect(Entitlements::Data::Groups::Calculated::Filters::MemberOfGroup).to receive(:new).and_return(filter)
+      expect(filter).to receive(:filtered?).with(russianblue).once.and_return(false)
+      expect(filter).to receive(:filtered?).with(blackmanx).once.and_return(false)
+
+      expect(obj.filtered_members).to eq(Set.new([blackmanx, russianblue]))
+    end
+  end
+
   describe "#modified_members" do
     before(:each) do
       allow_any_instance_of(described_class).to receive(:modifiers_constant).and_return(%w[expiration])

spec/unit/entitlements/data/groups/calculated/filters/member_of_group_spec.rb

@@ -8,7 +8,7 @@
   let(:cache) do
     {
       people_obj: people_obj,
-      file_objects: { fixture("ldap-config/internal/mygroup") => mygroup_obj },
+      file_objects: { fixture("ldap-config/internal/mygroup.txt") => mygroup_obj },
       calculated: { "internal" => { "mygroup" => groupdef } }
     }
   end
@@ -22,6 +22,8 @@
 
   before(:each) do
     setup_default_filters
+    allow(Entitlements::Data::Groups::Calculated::Rules::Group)
+      .to receive(:files_for).with("internal", options: {}).and_return("mygroup" => "txt")
   end
 
   context "with a :none filter" do

spec/unit/entitlements/data/groups/calculated/rules/group_spec.rb

@@ -95,6 +95,24 @@
         answer_set = Set.new(result.map { |name| people_obj.read[name] })
         expect(obj.members).to eq(answer_set)
       end
+
+      it "reuses recursively created file objects during top-level calculation" do
+        Entitlements.config_file = fixture("config.yaml")
+        path = fixture("ldap-config/nested_groups")
+        allow(Entitlements::Util::Util).to receive(:path_for_group).with("nested_groups").and_return(path)
+
+        obj.members
+        recursively_created = cache[:file_objects].dup
+        Entitlements::Data::Groups::Calculated.read_all(
+          "nested_groups",
+          { "base" => "ou=Nested,ou=Groups,dc=kittens,dc=net" }
+        )
+
+        recursively_created.each do |key, value|
+          expect(cache[:file_objects][key]).to equal(value)
+        end
+        expect(cache[:file_objects].keys).to all(satisfy { |filename| !File.extname(filename).empty? })
+      end
     end
 
     context "for a wildcard group that does not self-reference" do

spec/unit/entitlements/data/groups/calculated_spec.rb

@@ -50,6 +50,18 @@
       expect(simple1.members.map { |i| i.uid }).to include(ragamuffin)
     end
 
+    it "caches file objects by their full filename" do
+      allow(Entitlements::Util::Util).to receive(:path_for_group).with(ou_key)
+        .and_return(fixture("ldap-config/#{ou_key}"))
+
+      described_class.read_all(ou_key, cfg_obj)
+
+      expect(cache[:file_objects].keys).to contain_exactly(
+        fixture("ldap-config/simple/simple1.yaml"),
+        fixture("ldap-config/simple/simple2.yaml")
+      )
+    end
+
     it "skips over a subdirectory in the main OU" do
       allow(Entitlements::Util::Util).to receive(:path_for_group).with("nested_ou")
         .and_return(fixture("ldap-config/nested_ou"))