Merge branch 'extract-caching-from-github-importer' into 'master'

Move Caching module out of GithubImporter namespace to be reused

See merge request gitlab-org/gitlab!27096

lib/gitlab/cache/import/caching.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Cache
+    module Import
+      module Caching
+        # The default timeout of the cache keys.
+        TIMEOUT = 24.hours.to_i
+
+        WRITE_IF_GREATER_SCRIPT = <<-EOF.strip_heredoc.freeze
+        local key, value, ttl = KEYS[1], tonumber(ARGV[1]), ARGV[2]
+        local existing = tonumber(redis.call("get", key))
+
+        if existing == nil or value > existing then
+          redis.call("set", key, value)
+          redis.call("expire", key, ttl)
+          return true
+        else
+          return false
+        end
+        EOF
+
+        # Reads a cache key.
+        #
+        # If the key exists and has a non-empty value its TTL is refreshed
+        # automatically.
+        #
+        # raw_key - The cache key to read.
+        # timeout - The new timeout of the key if the key is to be refreshed.
+        def self.read(raw_key, timeout: TIMEOUT)
+          key = cache_key_for(raw_key)
+          value = Redis::Cache.with { |redis| redis.get(key) }
+
+          if value.present?
+            # We refresh the expiration time so frequently used keys stick
+            # around, removing the need for querying the database as much as
+            # possible.
+            #
+            # A key may be empty when we looked up a GitHub user (for example) but
+            # did not find a matching GitLab user. In that case we _don't_ want to
+            # refresh the TTL so we automatically pick up the right data when said
+            # user were to register themselves on the GitLab instance.
+            Redis::Cache.with { |redis| redis.expire(key, timeout) }
+          end
+
+          value
+        end
+
+        # Reads an integer from the cache, or returns nil if no value was found.
+        #
+        # See Caching.read for more information.
+        def self.read_integer(raw_key, timeout: TIMEOUT)
+          value = read(raw_key, timeout: timeout)
+
+          value.to_i if value.present?
+        end
+
+        # Sets a cache key to the given value.
+        #
+        # key - The cache key to write.
+        # value - The value to set.
+        # timeout - The time after which the cache key should expire.
+        def self.write(raw_key, value, timeout: TIMEOUT)
+          key = cache_key_for(raw_key)
+
+          Redis::Cache.with do |redis|
+            redis.set(key, value, ex: timeout)
+          end
+
+          value
+        end
+
+        # Adds a value to a set.
+        #
+        # raw_key - The key of the set to add the value to.
+        # value - The value to add to the set.
+        # timeout - The new timeout of the key.
+        def self.set_add(raw_key, value, timeout: TIMEOUT)
+          key = cache_key_for(raw_key)
+
+          Redis::Cache.with do |redis|
+            redis.multi do |m|
+              m.sadd(key, value)
+              m.expire(key, timeout)
+            end
+          end
+        end
+
+        # Returns true if the given value is present in the set.
+        #
+        # raw_key - The key of the set to check.
+        # value - The value to check for.
+        def self.set_includes?(raw_key, value)
+          key = cache_key_for(raw_key)
+
+          Redis::Cache.with do |redis|
+            redis.sismember(key, value)
+          end
+        end
+
+        # Sets multiple keys to a given value.
+        #
+        # mapping - A Hash mapping the cache keys to their values.
+        # timeout - The time after which the cache key should expire.
+        def self.write_multiple(mapping, timeout: TIMEOUT)
+          Redis::Cache.with do |redis|
+            redis.multi do |multi|
+              mapping.each do |raw_key, value|
+                multi.set(cache_key_for(raw_key), value, ex: timeout)
+              end
+            end
+          end
+        end
+
+        # Sets the expiration time of a key.
+        #
+        # raw_key - The key for which to change the timeout.
+        # timeout - The new timeout.
+        def self.expire(raw_key, timeout)
+          key = cache_key_for(raw_key)
+
+          Redis::Cache.with do |redis|
+            redis.expire(key, timeout)
+          end
+        end
+
+        # Sets a key to the given integer but only if the existing value is
+        # smaller than the given value.
+        #
+        # This method uses a Lua script to ensure the read and write are atomic.
+        #
+        # raw_key - The key to set.
+        # value - The new value for the key.
+        # timeout - The key timeout in seconds.
+        #
+        # Returns true when the key was overwritten, false otherwise.
+        def self.write_if_greater(raw_key, value, timeout: TIMEOUT)
+          key = cache_key_for(raw_key)
+          val = Redis::Cache.with do |redis|
+            redis
+              .eval(WRITE_IF_GREATER_SCRIPT, keys: [key], argv: [value, timeout])
+          end
+
+          val ? true : false
+        end
+
+        def self.cache_key_for(raw_key)
+          "#{Redis::Cache::CACHE_NAMESPACE}:#{raw_key}"
+        end
+      end
+    end
+  end
+end

lib/gitlab/github_import.rb

@@ -16,7 +16,7 @@ module Gitlab
     def self.ghost_user_id
       key = 'github-import/ghost-user-id'
 
-      Caching.read_integer(key) || Caching.write(key, User.select(:id).ghost.id)
+      Gitlab::Cache::Import::Caching.read_integer(key) || Gitlab::Cache::Import::Caching.write(key, User.select(:id).ghost.id)
     end
   end
 end

lib/gitlab/github_import/caching.rb

-# frozen_string_literal: true
-
-module Gitlab
-  module GithubImport
-    module Caching
-      # The default timeout of the cache keys.
-      TIMEOUT = 24.hours.to_i
-
-      WRITE_IF_GREATER_SCRIPT = <<-EOF.strip_heredoc.freeze
-      local key, value, ttl = KEYS[1], tonumber(ARGV[1]), ARGV[2]
-      local existing = tonumber(redis.call("get", key))
-
-      if existing == nil or value > existing then
-        redis.call("set", key, value)
-        redis.call("expire", key, ttl)
-        return true
-      else
-        return false
-      end
-      EOF
-
-      # Reads a cache key.
-      #
-      # If the key exists and has a non-empty value its TTL is refreshed
-      # automatically.
-      #
-      # raw_key - The cache key to read.
-      # timeout - The new timeout of the key if the key is to be refreshed.
-      def self.read(raw_key, timeout: TIMEOUT)
-        key = cache_key_for(raw_key)
-        value = Redis::Cache.with { |redis| redis.get(key) }
-
-        if value.present?
-          # We refresh the expiration time so frequently used keys stick
-          # around, removing the need for querying the database as much as
-          # possible.
-          #
-          # A key may be empty when we looked up a GitHub user (for example) but
-          # did not find a matching GitLab user. In that case we _don't_ want to
-          # refresh the TTL so we automatically pick up the right data when said
-          # user were to register themselves on the GitLab instance.
-          Redis::Cache.with { |redis| redis.expire(key, timeout) }
-        end
-
-        value
-      end
-
-      # Reads an integer from the cache, or returns nil if no value was found.
-      #
-      # See Caching.read for more information.
-      def self.read_integer(raw_key, timeout: TIMEOUT)
-        value = read(raw_key, timeout: timeout)
-
-        value.to_i if value.present?
-      end
-
-      # Sets a cache key to the given value.
-      #
-      # key - The cache key to write.
-      # value - The value to set.
-      # timeout - The time after which the cache key should expire.
-      def self.write(raw_key, value, timeout: TIMEOUT)
-        key = cache_key_for(raw_key)
-
-        Redis::Cache.with do |redis|
-          redis.set(key, value, ex: timeout)
-        end
-
-        value
-      end
-
-      # Adds a value to a set.
-      #
-      # raw_key - The key of the set to add the value to.
-      # value - The value to add to the set.
-      # timeout - The new timeout of the key.
-      def self.set_add(raw_key, value, timeout: TIMEOUT)
-        key = cache_key_for(raw_key)
-
-        Redis::Cache.with do |redis|
-          redis.multi do |m|
-            m.sadd(key, value)
-            m.expire(key, timeout)
-          end
-        end
-      end
-
-      # Returns true if the given value is present in the set.
-      #
-      # raw_key - The key of the set to check.
-      # value - The value to check for.
-      def self.set_includes?(raw_key, value)
-        key = cache_key_for(raw_key)
-
-        Redis::Cache.with do |redis|
-          redis.sismember(key, value)
-        end
-      end
-
-      # Sets multiple keys to a given value.
-      #
-      # mapping - A Hash mapping the cache keys to their values.
-      # timeout - The time after which the cache key should expire.
-      def self.write_multiple(mapping, timeout: TIMEOUT)
-        Redis::Cache.with do |redis|
-          redis.multi do |multi|
-            mapping.each do |raw_key, value|
-              multi.set(cache_key_for(raw_key), value, ex: timeout)
-            end
-          end
-        end
-      end
-
-      # Sets the expiration time of a key.
-      #
-      # raw_key - The key for which to change the timeout.
-      # timeout - The new timeout.
-      def self.expire(raw_key, timeout)
-        key = cache_key_for(raw_key)
-
-        Redis::Cache.with do |redis|
-          redis.expire(key, timeout)
-        end
-      end
-
-      # Sets a key to the given integer but only if the existing value is
-      # smaller than the given value.
-      #
-      # This method uses a Lua script to ensure the read and write are atomic.
-      #
-      # raw_key - The key to set.
-      # value - The new value for the key.
-      # timeout - The key timeout in seconds.
-      #
-      # Returns true when the key was overwritten, false otherwise.
-      def self.write_if_greater(raw_key, value, timeout: TIMEOUT)
-        key = cache_key_for(raw_key)
-        val = Redis::Cache.with do |redis|
-          redis
-            .eval(WRITE_IF_GREATER_SCRIPT, keys: [key], argv: [value, timeout])
-        end
-
-        val ? true : false
-      end
-
-      def self.cache_key_for(raw_key)
-        "#{Redis::Cache::CACHE_NAMESPACE}:#{raw_key}"
-      end
-    end
-  end
-end

lib/gitlab/github_import/issuable_finder.rb

@@ -23,7 +23,7 @@ module Gitlab
       #
       # This method will return `nil` if no ID could be found.
       def database_id
-        val = Caching.read(cache_key)
+        val = Gitlab::Cache::Import::Caching.read(cache_key)
 
         val.to_i if val.present?
       end
@@ -32,7 +32,7 @@ module Gitlab
       #
       # database_id - The ID of the corresponding database row.
       def cache_database_id(database_id)
-        Caching.write(cache_key, database_id)
+        Gitlab::Cache::Import::Caching.write(cache_key, database_id)
       end
 
       private

lib/gitlab/github_import/label_finder.rb

@@ -15,7 +15,7 @@ module Gitlab
 
       # Returns the label ID for the given name.
       def id_for(name)
-        Caching.read_integer(cache_key_for(name))
+        Gitlab::Cache::Import::Caching.read_integer(cache_key_for(name))
       end
 
       # rubocop: disable CodeReuse/ActiveRecord
@@ -27,7 +27,7 @@ module Gitlab
             hash[cache_key_for(name)] = id
           end
 
-        Caching.write_multiple(mapping)
+        Gitlab::Cache::Import::Caching.write_multiple(mapping)
       end
       # rubocop: enable CodeReuse/ActiveRecord
 

lib/gitlab/github_import/milestone_finder.rb

@@ -18,7 +18,7 @@ module Gitlab
       def id_for(issuable)
         return unless issuable.milestone_number
 
-        Caching.read_integer(cache_key_for(issuable.milestone_number))
+        Gitlab::Cache::Import::Caching.read_integer(cache_key_for(issuable.milestone_number))
       end
 
       # rubocop: disable CodeReuse/ActiveRecord
@@ -30,7 +30,7 @@ module Gitlab
             hash[cache_key_for(iid)] = id
           end
 
-        Caching.write_multiple(mapping)
+        Gitlab::Cache::Import::Caching.write_multiple(mapping)
       end
       # rubocop: enable CodeReuse/ActiveRecord
 

lib/gitlab/github_import/page_counter.rb

@@ -19,12 +19,12 @@ module Gitlab
       #
       # Returns true if the page number was overwritten, false otherwise.
       def set(page)
-        Caching.write_if_greater(cache_key, page)
+        Gitlab::Cache::Import::Caching.write_if_greater(cache_key, page)
       end
 
       # Returns the current value from the cache.
       def current
-        Caching.read_integer(cache_key) || 1
+        Gitlab::Cache::Import::Caching.read_integer(cache_key) || 1
       end
     end
   end

lib/gitlab/github_import/parallel_scheduling.rb

@@ -42,7 +42,7 @@ module Gitlab
         # still scheduling duplicates while. Since all work has already been
         # completed those jobs will just cycle through any remaining pages while
         # not scheduling anything.
-        Caching.expire(already_imported_cache_key, 15.minutes.to_i)
+        Gitlab::Cache::Import::Caching.expire(already_imported_cache_key, 15.minutes.to_i)
 
         retval
       end
@@ -112,14 +112,14 @@ module Gitlab
       def already_imported?(object)
         id = id_for_already_imported_cache(object)
 
-        Caching.set_includes?(already_imported_cache_key, id)
+        Gitlab::Cache::Import::Caching.set_includes?(already_imported_cache_key, id)
       end
 
       # Marks the given object as "already imported".
       def mark_as_imported(object)
         id = id_for_already_imported_cache(object)
 
-        Caching.set_add(already_imported_cache_key, id)
+        Gitlab::Cache::Import::Caching.set_add(already_imported_cache_key, id)
       end
 
       # Returns the ID to use for the cache used for checking if an object has

lib/gitlab/github_import/user_finder.rb

@@ -102,11 +102,11 @@ module Gitlab
 
       def email_for_github_username(username)
         cache_key = EMAIL_FOR_USERNAME_CACHE_KEY % username
-        email = Caching.read(cache_key)
+        email = Gitlab::Cache::Import::Caching.read(cache_key)
 
         unless email
           user = client.user(username)
-          email = Caching.write(cache_key, user.email) if user
+          email = Gitlab::Cache::Import::Caching.write(cache_key, user.email) if user
         end
 
         email
@@ -125,7 +125,7 @@ module Gitlab
       def id_for_github_id(id)
         gitlab_id = query_id_for_github_id(id) || nil
 
-        Caching.write(ID_CACHE_KEY % id, gitlab_id)
+        Gitlab::Cache::Import::Caching.write(ID_CACHE_KEY % id, gitlab_id)
       end
 
       # Queries and caches the GitLab user ID for a GitHub email, if one was
@@ -133,7 +133,7 @@ module Gitlab
       def id_for_github_email(email)
         gitlab_id = query_id_for_github_email(email) || nil
 
-        Caching.write(ID_FOR_EMAIL_CACHE_KEY % email, gitlab_id)
+        Gitlab::Cache::Import::Caching.write(ID_FOR_EMAIL_CACHE_KEY % email, gitlab_id)
       end
 
       # rubocop: disable CodeReuse/ActiveRecord
@@ -155,7 +155,7 @@ module Gitlab
       # 1. A boolean indicating if the key was present or not.
       # 2. The ID as an Integer, or nil in case no ID could be found.
       def read_id_from_cache(key)
-        value = Caching.read(key)
+        value = Gitlab::Cache::Import::Caching.read(key)
         exists = !value.nil?
         number = value.to_i
 

spec/lib/gitlab/github_import/caching_spec.rb → spec/lib/gitlab/cache/import/caching_spec.rb

@@ -2,7 +2,7 @@
 
 require 'spec_helper'
 
-describe Gitlab::GithubImport::Caching, :clean_gitlab_redis_cache do
+describe Gitlab::Cache::Import::Caching, :clean_gitlab_redis_cache do
   describe '.read' do
     it 'reads a value from the cache' do
       described_class.write('foo', 'bar')

spec/lib/gitlab/github_import/issuable_finder_spec.rb

@@ -30,7 +30,7 @@ describe Gitlab::GithubImport::IssuableFinder, :clean_gitlab_redis_cache do
 
   describe '#cache_database_id' do
     it 'caches the ID of a database row' do
-      expect(Gitlab::GithubImport::Caching)
+      expect(Gitlab::Cache::Import::Caching)
         .to receive(:write)
         .with('github-import/issuable-finder/4/MergeRequest/1', 10)
 

spec/lib/gitlab/github_import/label_finder_spec.rb

@@ -21,7 +21,7 @@ describe Gitlab::GithubImport::LabelFinder, :clean_gitlab_redis_cache do
       it 'returns nil for an empty cache key' do
         key = finder.cache_key_for(bug.name)
 
-        Gitlab::GithubImport::Caching.write(key, '')
+        Gitlab::Cache::Import::Caching.write(key, '')
 
         expect(finder.id_for(bug.name)).to be_nil
       end
@@ -40,7 +40,7 @@ describe Gitlab::GithubImport::LabelFinder, :clean_gitlab_redis_cache do
 
   describe '#build_cache' do
     it 'builds the cache of all project labels' do
-      expect(Gitlab::GithubImport::Caching)
+      expect(Gitlab::Cache::Import::Caching)
         .to receive(:write_multiple)
         .with(
           {

spec/lib/gitlab/github_import/milestone_finder_spec.rb

@@ -22,7 +22,7 @@ describe Gitlab::GithubImport::MilestoneFinder, :clean_gitlab_redis_cache do
       it 'returns nil for an empty cache key' do
         key = finder.cache_key_for(milestone.iid)
 
-        Gitlab::GithubImport::Caching.write(key, '')
+        Gitlab::Cache::Import::Caching.write(key, '')
 
         expect(finder.id_for(issuable)).to be_nil
       end
@@ -41,7 +41,7 @@ describe Gitlab::GithubImport::MilestoneFinder, :clean_gitlab_redis_cache do
 
   describe '#build_cache' do
     it 'builds the cache of all project milestones' do
-      expect(Gitlab::GithubImport::Caching)
+      expect(Gitlab::Cache::Import::Caching)
         .to receive(:write_multiple)
         .with("github-import/milestone-finder/#{project.id}/1" => milestone.id)
         .and_call_original

spec/lib/gitlab/github_import/page_counter_spec.rb

@@ -12,7 +12,7 @@ describe Gitlab::GithubImport::PageCounter, :clean_gitlab_redis_cache do
     end
 
     it 'sets the initial page number to the cached value when one is present' do
-      Gitlab::GithubImport::Caching.write(counter.cache_key, 2)
+      Gitlab::Cache::Import::Caching.write(counter.cache_key, 2)
 
       expect(described_class.new(project, :issues).current).to eq(2)
     end

spec/lib/gitlab/github_import/parallel_scheduling_spec.rb

@@ -57,7 +57,7 @@ describe Gitlab::GithubImport::ParallelScheduling do
 
       expect(importer).to receive(:parallel_import)
 
-      expect(Gitlab::GithubImport::Caching)
+      expect(Gitlab::Cache::Import::Caching)
         .to receive(:expire)
         .with(importer.already_imported_cache_key, a_kind_of(Numeric))
 
@@ -287,7 +287,7 @@ describe Gitlab::GithubImport::ParallelScheduling do
         .with(object)
         .and_return(object.id)
 
-      expect(Gitlab::GithubImport::Caching)
+      expect(Gitlab::Cache::Import::Caching)
         .to receive(:set_add)
         .with(importer.already_imported_cache_key, object.id)
         .and_call_original

spec/lib/gitlab/github_import/user_finder_spec.rb

@@ -162,7 +162,7 @@ describe Gitlab::GithubImport::UserFinder, :clean_gitlab_redis_cache do
 
     context 'when an Email address is cached' do
       it 'reads the Email address from the cache' do
-        expect(Gitlab::GithubImport::Caching)
+        expect(Gitlab::Cache::Import::Caching)
           .to receive(:read)
           .and_return(email)
 
@@ -182,7 +182,7 @@ describe Gitlab::GithubImport::UserFinder, :clean_gitlab_redis_cache do
       it 'caches the Email address when an Email address is available' do
         expect(client).to receive(:user).with('kittens').and_return(user)
 
-        expect(Gitlab::GithubImport::Caching)
+        expect(Gitlab::Cache::Import::Caching)
           .to receive(:write)
           .with(an_instance_of(String), email)
 
@@ -195,7 +195,7 @@ describe Gitlab::GithubImport::UserFinder, :clean_gitlab_redis_cache do
           .with('kittens')
           .and_return(nil)
 
-        expect(Gitlab::GithubImport::Caching)
+        expect(Gitlab::Cache::Import::Caching)
           .not_to receive(:write)
 
         expect(finder.email_for_github_username('kittens')).to be_nil
@@ -207,7 +207,7 @@ describe Gitlab::GithubImport::UserFinder, :clean_gitlab_redis_cache do
     let(:id) { 4 }
 
     it 'reads a user ID from the cache' do
-      Gitlab::GithubImport::Caching
+      Gitlab::Cache::Import::Caching
         .write(described_class::ID_CACHE_KEY % id, 4)
 
       expect(finder.cached_id_for_github_id(id)).to eq([true, 4])
@@ -222,7 +222,7 @@ describe Gitlab::GithubImport::UserFinder, :clean_gitlab_redis_cache do
     let(:email) { 'kittens@example.com' }
 
     it 'reads a user ID from the cache' do
-      Gitlab::GithubImport::Caching
+      Gitlab::Cache::Import::Caching
         .write(described_class::ID_FOR_EMAIL_CACHE_KEY % email, 4)
 
       expect(finder.cached_id_for_github_email(email)).to eq([true, 4])
@@ -241,7 +241,7 @@ describe Gitlab::GithubImport::UserFinder, :clean_gitlab_redis_cache do
         .with(id)
         .and_return(42)
 
-      expect(Gitlab::GithubImport::Caching)
+      expect(Gitlab::Cache::Import::Caching)
         .to receive(:write)
         .with(described_class::ID_CACHE_KEY % id, 42)
 
@@ -253,7 +253,7 @@ describe Gitlab::GithubImport::UserFinder, :clean_gitlab_redis_cache do
         .with(id)
         .and_return(nil)
 
-      expect(Gitlab::GithubImport::Caching)
+      expect(Gitlab::Cache::Import::Caching)
         .to receive(:write)
         .with(described_class::ID_CACHE_KEY % id, nil)
 
@@ -269,7 +269,7 @@ describe Gitlab::GithubImport::UserFinder, :clean_gitlab_redis_cache do
         .with(email)
         .and_return(42)
 
-      expect(Gitlab::GithubImport::Caching)
+      expect(Gitlab::Cache::Import::Caching)
         .to receive(:write)
         .with(described_class::ID_FOR_EMAIL_CACHE_KEY % email, 42)
 
@@ -281,7 +281,7 @@ describe Gitlab::GithubImport::UserFinder, :clean_gitlab_redis_cache do
         .with(email)
         .and_return(nil)
 
-      expect(Gitlab::GithubImport::Caching)
+      expect(Gitlab::Cache::Import::Caching)
         .to receive(:write)
         .with(described_class::ID_FOR_EMAIL_CACHE_KEY % email, nil)
 
@@ -317,13 +317,13 @@ describe Gitlab::GithubImport::UserFinder, :clean_gitlab_redis_cache do
 
   describe '#read_id_from_cache' do
     it 'reads an ID from the cache' do
-      Gitlab::GithubImport::Caching.write('foo', 10)
+      Gitlab::Cache::Import::Caching.write('foo', 10)
 
       expect(finder.read_id_from_cache('foo')).to eq([true, 10])
     end
 
     it 'reads a cache key with an empty value' do
-      Gitlab::GithubImport::Caching.write('foo', nil)
+      Gitlab::Cache::Import::Caching.write('foo', nil)
 
       expect(finder.read_id_from_cache('foo')).to eq([true, nil])
     end

spec/lib/gitlab/github_import_spec.rb

@@ -35,7 +35,7 @@ describe Gitlab::GithubImport do
     end
 
     it 'caches the ghost user ID' do
-      expect(Gitlab::GithubImport::Caching)
+      expect(Gitlab::Cache::Import::Caching)
         .to receive(:write)
         .once
         .and_call_original