summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
-rw-r--r--app/models/concerns/resolvable_discussion.rb19
-rw-r--r--app/models/project.rb11
-rw-r--r--app/models/repository.rb9
-rw-r--r--lib/gitlab/background_migration/prepare_untracked_uploads.rb10
-rw-r--r--lib/gitlab/bare_repository_import/repository.rb10
-rw-r--r--lib/gitlab/ci/pipeline/chain/skip.rb6
-rw-r--r--lib/gitlab/ci/stage/seed.rb6
-rw-r--r--lib/gitlab/github_import/client.rb6
-rw-r--r--lib/gitlab/user_access.rb10
-rw-r--r--rubocop/cop/gitlab/predicate_memoization.rb39
-rw-r--r--rubocop/rubocop.rb1
-rw-r--r--spec/rubocop/cop/gitlab/predicate_memoization_spec.rb86
12 files changed, 192 insertions, 21 deletions
diff --git a/app/models/concerns/resolvable_discussion.rb b/app/models/concerns/resolvable_discussion.rb
index b6c7b6735b9..6a2cb80b6d5 100644
--- a/app/models/concerns/resolvable_discussion.rb
+++ b/app/models/concerns/resolvable_discussion.rb
@@ -1,5 +1,6 @@
module ResolvableDiscussion
extend ActiveSupport::Concern
+ include ::Gitlab::Utils::StrongMemoize
included do
# A number of properties of this `Discussion`, like `first_note` and `resolvable?`, are memoized.
@@ -31,27 +32,35 @@ module ResolvableDiscussion
end
def resolvable?
- @resolvable ||= potentially_resolvable? && notes.any?(&:resolvable?)
+ strong_memoize(:resolvable) do
+ potentially_resolvable? && notes.any?(&:resolvable?)
+ end
end
def resolved?
- @resolved ||= resolvable? && notes.none?(&:to_be_resolved?)
+ strong_memoize(:resolved) do
+ resolvable? && notes.none?(&:to_be_resolved?)
+ end
end
def first_note
- @first_note ||= notes.first
+ notes.first
end
def first_note_to_resolve
return unless resolvable?
- @first_note_to_resolve ||= notes.find(&:to_be_resolved?) # rubocop:disable Gitlab/ModuleWithInstanceVariables
+ strong_memoize(:first_note_to_resolve) do
+ notes.find(&:to_be_resolved?)
+ end
end
def last_resolved_note
return unless resolved?
- @last_resolved_note ||= resolved_notes.sort_by(&:resolved_at).last # rubocop:disable Gitlab/ModuleWithInstanceVariables
+ strong_memoize(:last_resolved_note) do
+ resolved_notes.sort_by(&:resolved_at).last
+ end
end
def resolved_notes
diff --git a/app/models/project.rb b/app/models/project.rb
index 029f2da2e4e..7ab7df4fdcd 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -20,6 +20,7 @@ class Project < ActiveRecord::Base
include GroupDescendant
include Gitlab::SQL::Pattern
include DeploymentPlatform
+ include ::Gitlab::Utils::StrongMemoize
extend Gitlab::ConfigHelper
extend Gitlab::CurrentSettings
@@ -993,9 +994,13 @@ class Project < ActiveRecord::Base
end
def repo_exists?
- @repo_exists ||= repository.exists?
- rescue
- @repo_exists = false
+ strong_memoize(:repo_exists) do
+ begin
+ repository.exists?
+ rescue
+ false
+ end
+ end
end
def root_ref?(branch)
diff --git a/app/models/repository.rb b/app/models/repository.rb
index d27212b2058..8e9f33c174c 100644
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -895,15 +895,18 @@ class Repository
branch = Gitlab::Git::Branch.find(self, branch_or_name)
if branch
- @root_ref_sha ||= commit(root_ref).sha
- same_head = branch.target == @root_ref_sha
- merged = ancestor?(branch.target, @root_ref_sha)
+ same_head = branch.target == root_ref_sha
+ merged = ancestor?(branch.target, root_ref_sha)
!same_head && merged
else
nil
end
end
+ def root_ref_sha
+ @root_ref_sha ||= commit(root_ref).sha
+ end
+
delegate :merged_branch_names, to: :raw_repository
def merge_base(first_commit_id, second_commit_id)
diff --git a/lib/gitlab/background_migration/prepare_untracked_uploads.rb b/lib/gitlab/background_migration/prepare_untracked_uploads.rb
index 476c46341ae..4e0121ca34d 100644
--- a/lib/gitlab/background_migration/prepare_untracked_uploads.rb
+++ b/lib/gitlab/background_migration/prepare_untracked_uploads.rb
@@ -7,6 +7,7 @@ module Gitlab
class PrepareUntrackedUploads # rubocop:disable Metrics/ClassLength
# For bulk_queue_background_migration_jobs_by_range
include Database::MigrationHelpers
+ include ::Gitlab::Utils::StrongMemoize
FIND_BATCH_SIZE = 500
RELATIVE_UPLOAD_DIR = "uploads".freeze
@@ -142,7 +143,9 @@ module Gitlab
end
def postgresql?
- @postgresql ||= Gitlab::Database.postgresql?
+ strong_memoize(:postgresql) do
+ Gitlab::Database.postgresql?
+ end
end
def can_bulk_insert_and_ignore_duplicates?
@@ -150,8 +153,9 @@ module Gitlab
end
def postgresql_pre_9_5?
- @postgresql_pre_9_5 ||= postgresql? &&
- Gitlab::Database.version.to_f < 9.5
+ strong_memoize(:postgresql_pre_9_5) do
+ postgresql? && Gitlab::Database.version.to_f < 9.5
+ end
end
def schedule_populate_untracked_uploads_jobs
diff --git a/lib/gitlab/bare_repository_import/repository.rb b/lib/gitlab/bare_repository_import/repository.rb
index 85b79362196..c0c666dfb7b 100644
--- a/lib/gitlab/bare_repository_import/repository.rb
+++ b/lib/gitlab/bare_repository_import/repository.rb
@@ -1,6 +1,8 @@
module Gitlab
module BareRepositoryImport
class Repository
+ include ::Gitlab::Utils::StrongMemoize
+
attr_reader :group_path, :project_name, :repo_path
def initialize(root_path, repo_path)
@@ -41,11 +43,15 @@ module Gitlab
private
def wiki?
- @wiki ||= repo_path.end_with?('.wiki.git')
+ strong_memoize(:wiki) do
+ repo_path.end_with?('.wiki.git')
+ end
end
def hashed?
- @hashed ||= repo_relative_path.include?('@hashed')
+ strong_memoize(:hashed) do
+ repo_relative_path.include?('@hashed')
+ end
end
def repo_relative_path
diff --git a/lib/gitlab/ci/pipeline/chain/skip.rb b/lib/gitlab/ci/pipeline/chain/skip.rb
index 9a72de87bab..32cbb7ca6af 100644
--- a/lib/gitlab/ci/pipeline/chain/skip.rb
+++ b/lib/gitlab/ci/pipeline/chain/skip.rb
@@ -3,6 +3,8 @@ module Gitlab
module Pipeline
module Chain
class Skip < Chain::Base
+ include ::Gitlab::Utils::StrongMemoize
+
SKIP_PATTERN = /\[(ci[ _-]skip|skip[ _-]ci)\]/i
def perform!
@@ -24,7 +26,9 @@ module Gitlab
def commit_message_skips_ci?
return false unless @pipeline.git_commit_message
- @skipped ||= !!(@pipeline.git_commit_message =~ SKIP_PATTERN)
+ strong_memoize(:commit_message_skips_ci) do
+ !!(@pipeline.git_commit_message =~ SKIP_PATTERN)
+ end
end
end
end
diff --git a/lib/gitlab/ci/stage/seed.rb b/lib/gitlab/ci/stage/seed.rb
index bc97aa63b02..f33c87f554d 100644
--- a/lib/gitlab/ci/stage/seed.rb
+++ b/lib/gitlab/ci/stage/seed.rb
@@ -2,6 +2,8 @@ module Gitlab
module Ci
module Stage
class Seed
+ include ::Gitlab::Utils::StrongMemoize
+
attr_reader :pipeline
delegate :project, to: :pipeline
@@ -50,7 +52,9 @@ module Gitlab
private
def protected_ref?
- @protected_ref ||= project.protected_for?(pipeline.ref)
+ strong_memoize(:protected_ref) do
+ project.protected_for?(pipeline.ref)
+ end
end
end
end
diff --git a/lib/gitlab/github_import/client.rb b/lib/gitlab/github_import/client.rb
index 5da9befa08e..4f160e4a447 100644
--- a/lib/gitlab/github_import/client.rb
+++ b/lib/gitlab/github_import/client.rb
@@ -14,6 +14,8 @@ module Gitlab
# puts label.name
# end
class Client
+ include ::Gitlab::Utils::StrongMemoize
+
attr_reader :octokit
# A single page of data and the corresponding page number.
@@ -173,7 +175,9 @@ module Gitlab
end
def rate_limiting_enabled?
- @rate_limiting_enabled ||= api_endpoint.include?('.github.com')
+ strong_memoize(:rate_limiting_enabled) do
+ api_endpoint.include?('.github.com')
+ end
end
def api_endpoint
diff --git a/lib/gitlab/user_access.rb b/lib/gitlab/user_access.rb
index d9a5af09f08..f357488ac61 100644
--- a/lib/gitlab/user_access.rb
+++ b/lib/gitlab/user_access.rb
@@ -16,8 +16,10 @@ module Gitlab
def can_do_action?(action)
return false unless can_access_git?
- @permission_cache ||= {}
- @permission_cache[action] ||= user.can?(action, project)
+ permission_cache[action] =
+ permission_cache.fetch(action) do
+ user.can?(action, project)
+ end
end
def cannot_do_action?(action)
@@ -88,6 +90,10 @@ module Gitlab
private
+ def permission_cache
+ @permission_cache ||= {}
+ end
+
def can_access_git?
user && user.can?(:access_git)
end
diff --git a/rubocop/cop/gitlab/predicate_memoization.rb b/rubocop/cop/gitlab/predicate_memoization.rb
new file mode 100644
index 00000000000..3c25d61d087
--- /dev/null
+++ b/rubocop/cop/gitlab/predicate_memoization.rb
@@ -0,0 +1,39 @@
+module RuboCop
+ module Cop
+ module Gitlab
+ class PredicateMemoization < RuboCop::Cop::Cop
+ MSG = <<~EOL.freeze
+ Avoid using `@value ||= query` inside predicate methods in order to
+ properly memoize `false` or `nil` values.
+ https://docs.gitlab.com/ee/development/utilities.html#strongmemoize
+ EOL
+
+ def on_def(node)
+ return unless predicate_method?(node)
+
+ select_offenses(node).each do |offense|
+ add_offense(offense, location: :expression)
+ end
+ end
+
+ private
+
+ def predicate_method?(node)
+ node.method_name.to_s.end_with?('?')
+ end
+
+ def or_ivar_assignment?(or_assignment)
+ lhs = or_assignment.each_child_node.first
+
+ lhs.ivasgn_type?
+ end
+
+ def select_offenses(node)
+ node.each_descendant(:or_asgn).select do |or_assignment|
+ or_ivar_assignment?(or_assignment)
+ end
+ end
+ end
+ end
+ end
+end
diff --git a/rubocop/rubocop.rb b/rubocop/rubocop.rb
index 57af87f7fb9..9110237c538 100644
--- a/rubocop/rubocop.rb
+++ b/rubocop/rubocop.rb
@@ -1,4 +1,5 @@
require_relative 'cop/gitlab/module_with_instance_variables'
+require_relative 'cop/gitlab/predicate_memoization'
require_relative 'cop/include_sidekiq_worker'
require_relative 'cop/line_break_around_conditional_block'
require_relative 'cop/migration/add_column'
diff --git a/spec/rubocop/cop/gitlab/predicate_memoization_spec.rb b/spec/rubocop/cop/gitlab/predicate_memoization_spec.rb
new file mode 100644
index 00000000000..6c731139468
--- /dev/null
+++ b/spec/rubocop/cop/gitlab/predicate_memoization_spec.rb
@@ -0,0 +1,86 @@
+require 'spec_helper'
+require 'rubocop'
+require 'rubocop/rspec/support'
+require_relative '../../../../rubocop/cop/gitlab/predicate_memoization'
+
+describe RuboCop::Cop::Gitlab::PredicateMemoization do
+ include CopHelper
+
+ subject(:cop) { described_class.new }
+
+ shared_examples('registering offense') do |options|
+ let(:offending_lines) { options[:offending_lines] }
+
+ it 'registers an offense when a predicate method is memoizing via ivar' do
+ inspect_source(source)
+
+ aggregate_failures do
+ expect(cop.offenses.size).to eq(offending_lines.size)
+ expect(cop.offenses.map(&:line)).to eq(offending_lines)
+ end
+ end
+ end
+
+ shared_examples('not registering offense') do
+ it 'does not register offenses' do
+ inspect_source(source)
+
+ expect(cop.offenses).to be_empty
+ end
+ end
+
+ context 'when source is a predicate method memoizing via ivar' do
+ it_behaves_like 'registering offense', offending_lines: [3] do
+ let(:source) do
+ <<~RUBY
+ class C
+ def really?
+ @really ||= true
+ end
+ end
+ RUBY
+ end
+ end
+
+ it_behaves_like 'registering offense', offending_lines: [4] do
+ let(:source) do
+ <<~RUBY
+ class C
+ def really?
+ value = true
+ @really ||= value
+ end
+ end
+ RUBY
+ end
+ end
+ end
+
+ context 'when source is a predicate method using ivar with assignment' do
+ it_behaves_like 'not registering offense' do
+ let(:source) do
+ <<~RUBY
+ class C
+ def really?
+ @really = true
+ end
+ end
+ RUBY
+ end
+ end
+ end
+
+ context 'when source is a regular method memoizing via ivar' do
+ it_behaves_like 'not registering offense' do
+ let(:source) do
+ <<~RUBY
+ class C
+ def really
+ @really ||= true
+ end
+ end
+ RUBY
+ end
+ end
+ end
+end