BigW Consortium Gitlab

Skip to content

G
gitlab-ce
Commit b540b987 by Robert Speicher
Options

Merge branch 'no-ivar-in-modules' into 'master'

Add cop to make sure we don't use ivar in a module See merge request gitlab-org/gitlab-ce!12800
parents 95f5a525 02994fbe
Showing with 766 additions and 188 deletions
.rubocop.yml
......@@ -1185,7 +1185,20 @@ RSpec/SubjectStub:
RSpec/VerifiedDoubles:
Enabled: false
# GitlabSecurity ##############################################################
# Gitlab ###################################################################
Gitlab/ModuleWithInstanceVariables:
Enable: true
Exclude:
# We ignore Rails helpers right now because it's hard to workaround it
- app/helpers/**/*_helper.rb
# We ignore Rails mailers right now because it's hard to workaround it
- app/mailers/emails/**/*.rb
# We ignore spec helpers because it usually doesn't matter
- spec/support/**/*.rb
- features/steps/**/*.rb
# GitlabSecurity ###########################################################
GitlabSecurity/DeepMunge:
Enabled: true
......
app/controllers/concerns/boards_responses.rb
......@@ -24,11 +24,11 @@ module BoardsResponses
end
def respond_with_boards
respond_with(@boards)
respond_with(@boards) # rubocop:disable Gitlab/ModuleWithInstanceVariables
end
def respond_with_board
respond_with(@board)
respond_with(@board) # rubocop:disable Gitlab/ModuleWithInstanceVariables
end
def respond_with(resource)
......
app/controllers/concerns/creates_commit.rb
module CreatesCommit
extend ActiveSupport::Concern
include Gitlab::Utils::StrongMemoize
# rubocop:disable Gitlab/ModuleWithInstanceVariables
def create_commit(service, success_path:, failure_path:, failure_view: nil, success_notice: nil)
if can?(current_user, :push_code, @project)
@project_to_commit_into = @project
......@@ -45,6 +47,7 @@ module CreatesCommit
end
end
end
# rubocop:enable Gitlab/ModuleWithInstanceVariables
def authorize_edit_tree!
return if can_collaborate_with_project?
......@@ -77,6 +80,7 @@ module CreatesCommit
end
end
# rubocop:disable Gitlab/ModuleWithInstanceVariables
def new_merge_request_path
project_new_merge_request_path(
@project_to_commit_into,
......@@ -88,20 +92,28 @@ module CreatesCommit
}
)
end
# rubocop:enable Gitlab/ModuleWithInstanceVariables
def existing_merge_request_path
project_merge_request_path(@project, @merge_request)
project_merge_request_path(@project, @merge_request) # rubocop:disable Gitlab/ModuleWithInstanceVariables
end
# rubocop:disable Gitlab/ModuleWithInstanceVariables
def merge_request_exists?
return @merge_request if defined?(@merge_request)
@merge_request = MergeRequestsFinder.new(current_user, project_id: @project.id).execute.opened
.find_by(source_project_id: @project_to_commit_into, source_branch: @branch_name, target_branch: @start_branch)
strong_memoize(:merge_request) do
MergeRequestsFinder.new(current_user, project_id: @project.id)
.execute
.opened
.find_by(
source_project_id: @project_to_commit_into,
source_branch: @branch_name,
target_branch: @start_branch)
end
end
# rubocop:enable Gitlab/ModuleWithInstanceVariables
def different_project?
@project_to_commit_into != @project
@project_to_commit_into != @project # rubocop:disable Gitlab/ModuleWithInstanceVariables
end
def create_merge_request?
......@@ -109,6 +121,6 @@ module CreatesCommit
# as the target branch in the same project,
# we don't want to create a merge request.
params[:create_merge_request].present? &&
(different_project? || @start_branch != @branch_name)
(different_project? || @start_branch != @branch_name) # rubocop:disable Gitlab/ModuleWithInstanceVariables
end
end
app/controllers/concerns/group_tree.rb
module GroupTree
# rubocop:disable Gitlab/ModuleWithInstanceVariables
def render_group_tree(groups)
@groups = if params[:filter].present?
Gitlab::GroupHierarchy.new(groups.search(params[:filter]))
......@@ -20,5 +21,6 @@ module GroupTree
render json: serializer.represent(@groups)
end
end
# rubocop:enable Gitlab/ModuleWithInstanceVariables
end
end
app/controllers/concerns/issuable_actions.rb
......@@ -17,7 +17,7 @@ module IssuableActions
end
def update
@issuable = update_service.execute(issuable)
@issuable = update_service.execute(issuable) # rubocop:disable Gitlab/ModuleWithInstanceVariables
respond_to do |format|
format.html do
......@@ -81,7 +81,7 @@ module IssuableActions
private
def recaptcha_check_if_spammable(should_redirect = true, &block)
return yield unless @issuable.is_a? Spammable
return yield unless issuable.is_a? Spammable
recaptcha_check_with_fallback(should_redirect, &block)
end
......@@ -89,7 +89,7 @@ module IssuableActions
def render_conflict_response
respond_to do |format|
format.html do
@conflict = true
@conflict = true # rubocop:disable Gitlab/ModuleWithInstanceVariables
render :edit
end
......@@ -104,7 +104,7 @@ module IssuableActions
end
def labels
@labels ||= LabelsFinder.new(current_user, project_id: @project.id).execute
@labels ||= LabelsFinder.new(current_user, project_id: @project.id).execute # rubocop:disable Gitlab/ModuleWithInstanceVariables
end
def authorize_destroy_issuable!
......@@ -114,7 +114,7 @@ module IssuableActions
end
def authorize_admin_issuable!
unless can?(current_user, :"admin_#{resource_name}", @project)
unless can?(current_user, :"admin_#{resource_name}", @project) # rubocop:disable Gitlab/ModuleWithInstanceVariables
return access_denied!
end
end
......@@ -148,6 +148,7 @@ module IssuableActions
@resource_name ||= controller_name.singularize
end
# rubocop:disable Gitlab/ModuleWithInstanceVariables
def render_entity_json
if @issuable.valid?
render json: serializer.represent(@issuable)
......@@ -155,6 +156,7 @@ module IssuableActions
render json: { errors: @issuable.errors.full_messages }, status: :unprocessable_entity
end
end
# rubocop:enable Gitlab/ModuleWithInstanceVariables
def serializer
raise NotImplementedError
......@@ -165,6 +167,6 @@ module IssuableActions
end
def parent
@project || @group
@project || @group # rubocop:disable Gitlab/ModuleWithInstanceVariables
end
end
app/controllers/concerns/issuable_collections.rb
......@@ -2,6 +2,7 @@ module IssuableCollections
extend ActiveSupport::Concern
include SortingHelper
include Gitlab::IssuableMetadata
include Gitlab::Utils::StrongMemoize
included do
helper_method :finder
......@@ -9,6 +10,7 @@ module IssuableCollections
private
# rubocop:disable Gitlab/ModuleWithInstanceVariables
def set_issuables_index
@issuables = issuables_collection
@issuables = @issuables.page(params[:page])
......@@ -33,6 +35,7 @@ module IssuableCollections
@users.push(author) if author
end
end
# rubocop:enable Gitlab/ModuleWithInstanceVariables
def issuables_collection
finder.execute.preload(preload_for_collection)
......@@ -41,7 +44,7 @@ module IssuableCollections
def redirect_out_of_range(total_pages)
return false if total_pages.zero?
out_of_range = @issuables.current_page > total_pages
out_of_range = @issuables.current_page > total_pages # rubocop:disable Gitlab/ModuleWithInstanceVariables
if out_of_range
redirect_to(url_for(params.merge(page: total_pages, only_path: true)))
......@@ -51,7 +54,7 @@ module IssuableCollections
end
def issuable_page_count
page_count_for_relation(@issuables, finder.row_count)
page_count_for_relation(@issuables, finder.row_count) # rubocop:disable Gitlab/ModuleWithInstanceVariables
end
def page_count_for_relation(relation, row_count)
......@@ -66,6 +69,7 @@ module IssuableCollections
finder_class.new(current_user, filter_params)
end
# rubocop:disable Gitlab/ModuleWithInstanceVariables
def filter_params
set_sort_order_from_cookie
set_default_state
......@@ -90,6 +94,7 @@ module IssuableCollections
@filter_params.permit(IssuableFinder::VALID_PARAMS)
end
# rubocop:enable Gitlab/ModuleWithInstanceVariables
def set_default_state
params[:state] = 'opened' if params[:state].blank?
......@@ -129,9 +134,9 @@ module IssuableCollections
end
def finder
return @finder if defined?(@finder)
@finder = issuable_finder_for(@finder_type)
strong_memoize(:finder) do
issuable_finder_for(@finder_type) # rubocop:disable Gitlab/ModuleWithInstanceVariables
end
end
def collection_type
......
app/controllers/concerns/issues_action.rb
......@@ -2,6 +2,7 @@ module IssuesAction
extend ActiveSupport::Concern
include IssuableCollections
# rubocop:disable Gitlab/ModuleWithInstanceVariables
def issues
@finder_type = IssuesFinder
@label = finder.labels.first
......@@ -17,4 +18,5 @@ module IssuesAction
format.atom { render layout: 'xml.atom' }
end
end
# rubocop:enable Gitlab/ModuleWithInstanceVariables
end
app/controllers/concerns/merge_requests_action.rb
......@@ -2,6 +2,7 @@ module MergeRequestsAction
extend ActiveSupport::Concern
include IssuableCollections
# rubocop:disable Gitlab/ModuleWithInstanceVariables
def merge_requests
@finder_type = MergeRequestsFinder
@label = finder.labels.first
......@@ -10,6 +11,7 @@ module MergeRequestsAction
@issuable_meta_data = issuable_meta_data(@merge_requests, collection_type)
end
# rubocop:enable Gitlab/ModuleWithInstanceVariables
private
......
app/controllers/concerns/milestone_actions.rb
......@@ -6,7 +6,7 @@ module MilestoneActions
format.html { redirect_to milestone_redirect_path }
format.json do
render json: tabs_json("shared/milestones/_merge_requests_tab", {
merge_requests: @milestone.sorted_merge_requests,
merge_requests: @milestone.sorted_merge_requests, # rubocop:disable Gitlab/ModuleWithInstanceVariables
show_project_name: true
})
end
......@@ -18,7 +18,7 @@ module MilestoneActions
format.html { redirect_to milestone_redirect_path }
format.json do
render json: tabs_json("shared/milestones/_participants_tab", {
users: @milestone.participants
users: @milestone.participants # rubocop:disable Gitlab/ModuleWithInstanceVariables
})
end
end
......@@ -29,7 +29,7 @@ module MilestoneActions
format.html { redirect_to milestone_redirect_path }
format.json do
render json: tabs_json("shared/milestones/_labels_tab", {
labels: @milestone.labels
labels: @milestone.labels # rubocop:disable Gitlab/ModuleWithInstanceVariables
})
end
end
......@@ -43,6 +43,7 @@ module MilestoneActions
}
end
# rubocop:disable Gitlab/ModuleWithInstanceVariables
def milestone_redirect_path
if @project
project_milestone_path(@project, @milestone)
......@@ -52,4 +53,5 @@ module MilestoneActions
dashboard_milestone_path(@milestone.safe_title, title: @milestone.title)
end
end
# rubocop:enable Gitlab/ModuleWithInstanceVariables
end
app/controllers/concerns/notes_actions.rb
module NotesActions
include RendersNotes
include Gitlab::Utils::StrongMemoize
extend ActiveSupport::Concern
included do
......@@ -30,6 +31,7 @@ module NotesActions
render json: notes_json
end
# rubocop:disable Gitlab/ModuleWithInstanceVariables
def create
create_params = note_params.merge(
merge_request_diff_head_sha: params[:merge_request_diff_head_sha],
......@@ -47,7 +49,9 @@ module NotesActions
format.html { redirect_back_or_default }
end
end
# rubocop:enable Gitlab/ModuleWithInstanceVariables
# rubocop:disable Gitlab/ModuleWithInstanceVariables
def update
@note = Notes::UpdateService.new(project, current_user, note_params).execute(note)
......@@ -60,6 +64,7 @@ module NotesActions
format.html { redirect_back_or_default }
end
end
# rubocop:enable Gitlab/ModuleWithInstanceVariables
def destroy
if note.editable?
......@@ -138,7 +143,7 @@ module NotesActions
end
else
template = "discussions/_diff_discussion"
@fresh_discussion = true
@fresh_discussion = true # rubocop:disable Gitlab/ModuleWithInstanceVariables
locals = { discussions: [discussion], on_image: on_image }
end
......@@ -191,7 +196,7 @@ module NotesActions
end
def noteable
@noteable ||= notes_finder.target || @note&.noteable
@noteable ||= notes_finder.target || @note&.noteable # rubocop:disable Gitlab/ModuleWithInstanceVariables
end
def require_noteable!
......@@ -211,20 +216,21 @@ module NotesActions
end
def note_project
return @note_project if defined?(@note_project)
return nil unless project
strong_memoize(:note_project) do
return nil unless project
note_project_id = params[:note_project_id]
note_project_id = params[:note_project_id]
@note_project =
if note_project_id.present?
Project.find(note_project_id)
else
project
end
the_project =
if note_project_id.present?
Project.find(note_project_id)
else
project
end
return access_denied! unless can?(current_user, :create_note, @note_project)
return access_denied! unless can?(current_user, :create_note, the_project)
@note_project
the_project
end
end
end
app/controllers/concerns/oauth_applications.rb
......@@ -14,6 +14,6 @@ module OauthApplications
end
def load_scopes
@scopes = Doorkeeper.configuration.scopes
@scopes ||= Doorkeeper.configuration.scopes
end
end
app/controllers/concerns/preview_markdown.rb
module PreviewMarkdown
extend ActiveSupport::Concern
# rubocop:disable Gitlab/ModuleWithInstanceVariables
def preview_markdown
result = PreviewMarkdownService.new(@project, current_user, params).execute
......@@ -20,4 +21,5 @@ module PreviewMarkdown
}
}
end
# rubocop:enable Gitlab/ModuleWithInstanceVariables
end
app/controllers/concerns/renders_commits.rb
module RendersCommits
def prepare_commits_for_rendering(commits)
Banzai::CommitRenderer.render(commits, @project, current_user)
Banzai::CommitRenderer.render(commits, @project, current_user) # rubocop:disable Gitlab/ModuleWithInstanceVariables
commits
end
......
app/controllers/concerns/renders_notes.rb
module RendersNotes
# rubocop:disable Gitlab/ModuleWithInstanceVariables
def prepare_notes_for_rendering(notes, noteable = nil)
preload_noteable_for_regular_notes(notes)
preload_max_access_for_authors(notes, @project)
......@@ -7,6 +8,7 @@ module RendersNotes
notes
end
# rubocop:enable Gitlab/ModuleWithInstanceVariables
private
......
app/controllers/concerns/service_params.rb
......@@ -66,7 +66,7 @@ module ServiceParams
FILTER_BLANK_PARAMS = [:password].freeze
def service_params
dynamic_params = @service.event_channel_names + @service.event_names
dynamic_params = @service.event_channel_names + @service.event_names # rubocop:disable Gitlab/ModuleWithInstanceVariables
service_params = params.permit(:id, service: ALLOWED_PARAMS_CE + dynamic_params)
if service_params[:service].is_a?(Hash)
......
app/controllers/concerns/snippets_actions.rb
......@@ -4,6 +4,7 @@ module SnippetsActions
def edit
end
# rubocop:disable Gitlab/ModuleWithInstanceVariables
def raw
disposition = params[:inline] == 'false' ? 'attachment' : 'inline'
......@@ -14,6 +15,7 @@ module SnippetsActions
filename: @snippet.sanitized_file_name
)
end
# rubocop:enable Gitlab/ModuleWithInstanceVariables
private
......
app/controllers/concerns/spammable_actions.rb
......@@ -2,6 +2,7 @@ module SpammableActions
extend ActiveSupport::Concern
include Recaptcha::Verify
include Gitlab::Utils::StrongMemoize
included do
before_action :authorize_submit_spammable!, only: :mark_as_spam
......@@ -18,9 +19,9 @@ module SpammableActions
private
def ensure_spam_config_loaded!
return @spam_config_loaded if defined?(@spam_config_loaded)
@spam_config_loaded = Gitlab::Recaptcha.load_configurations!
strong_memoize(:spam_config_loaded) do
Gitlab::Recaptcha.load_configurations!
end
end
def recaptcha_check_with_fallback(should_redirect = true, &fallback)
......
app/controllers/concerns/toggle_subscription_action.rb
......@@ -12,7 +12,7 @@ module ToggleSubscriptionAction
private
def subscribable_project
@project || raise(NotImplementedError)
@project ||= raise(NotImplementedError)
end
def subscribable_resource
......
app/models/concerns/mentionable.rb
......@@ -44,13 +44,11 @@ module Mentionable
end
def all_references(current_user = nil, extractor: nil)
@extractors ||= {}
# Use custom extractor if it's passed in the function parameters.
if extractor
@extractors[current_user] = extractor
extractors[current_user] = extractor
else
extractor = @extractors[current_user] ||= Gitlab::ReferenceExtractor.new(project, current_user)
extractor = extractors[current_user] ||= Gitlab::ReferenceExtractor.new(project, current_user)
extractor.reset_memoized_values
end
......@@ -69,6 +67,10 @@ module Mentionable
extractor
end
def extractors
@extractors ||= {}
end
def mentioned_users(current_user = nil)
all_references(current_user).users
end
......
app/models/concerns/milestoneish.rb
......@@ -103,9 +103,11 @@ module Milestoneish
end
def memoize_per_user(user, method_name)
@memoized ||= {}
@memoized[method_name] ||= {}
@memoized[method_name][user&.id] ||= yield
memoized_users[method_name][user&.id] ||= yield
end
def memoized_users
@memoized_users ||= Hash.new { |h, k| h[k] = {} }
end
# override in a class that includes this module to get a faster query
......
app/models/concerns/noteable.rb
......@@ -46,6 +46,7 @@ module Noteable
notes.inc_relations_for_view.grouped_diff_discussions(*args)
end
# rubocop:disable Gitlab/ModuleWithInstanceVariables
def resolvable_discussions
@resolvable_discussions ||=
if defined?(@discussions)
......@@ -54,6 +55,7 @@ module Noteable
discussion_notes.resolvable.discussions(self)
end
end
# rubocop:enable Gitlab/ModuleWithInstanceVariables
def discussions_resolvable?
resolvable_discussions.any?(&:resolvable?)
......
app/models/concerns/participable.rb
......@@ -56,15 +56,17 @@ module Participable
#
# Returns an Array of User instances.
def participants(current_user = nil)
@participants ||= Hash.new do |hash, user|
hash[user] = raw_participants(user)
end
@participants[current_user]
all_participants[current_user]
end
private
def all_participants
@all_participants ||= Hash.new do |hash, user|
hash[user] = raw_participants(user)
end
end
def raw_participants(current_user = nil)
current_user ||= author
ext = Gitlab::ReferenceExtractor.new(project, current_user)
......
app/models/concerns/relative_positioning.rb
......@@ -52,7 +52,7 @@ module RelativePositioning
# to its predecessor. This process will recursively move all the predecessors until we have a place
if (after.relative_position - before.relative_position) < 2
before.move_before
@positionable_neighbours = [before]
@positionable_neighbours = [before] # rubocop:disable Gitlab/ModuleWithInstanceVariables
end
self.relative_position = position_between(before.relative_position, after.relative_position)
......@@ -65,7 +65,7 @@ module RelativePositioning
if before.shift_after?
issue_to_move = self.class.in_projects(project_ids).find_by!(relative_position: pos_after)
issue_to_move.move_after
@positionable_neighbours = [issue_to_move]
@positionable_neighbours = [issue_to_move] # rubocop:disable Gitlab/ModuleWithInstanceVariables
pos_after = issue_to_move.relative_position
end
......@@ -80,7 +80,7 @@ module RelativePositioning
if after.shift_before?
issue_to_move = self.class.in_projects(project_ids).find_by!(relative_position: pos_before)
issue_to_move.move_before
@positionable_neighbours = [issue_to_move]
@positionable_neighbours = [issue_to_move] # rubocop:disable Gitlab/ModuleWithInstanceVariables
pos_before = issue_to_move.relative_position
end
......@@ -132,6 +132,7 @@ module RelativePositioning
end
end
# rubocop:disable Gitlab/ModuleWithInstanceVariables
def save_positionable_neighbours
return unless @positionable_neighbours
......@@ -140,4 +141,5 @@ module RelativePositioning
status
end
# rubocop:enable Gitlab/ModuleWithInstanceVariables
end
app/models/concerns/resolvable_discussion.rb
......@@ -31,15 +31,11 @@ module ResolvableDiscussion
end
def resolvable?
return @resolvable if @resolvable.present?
@resolvable = potentially_resolvable? && notes.any?(&:resolvable?)
@resolvable ||= potentially_resolvable? && notes.any?(&:resolvable?)
end
def resolved?
return @resolved if @resolved.present?
@resolved = resolvable? && notes.none?(&:to_be_resolved?)
@resolved ||= resolvable? && notes.none?(&:to_be_resolved?)
end
def first_note
......@@ -49,13 +45,13 @@ module ResolvableDiscussion
def first_note_to_resolve
return unless resolvable?
@first_note_to_resolve ||= notes.find(&:to_be_resolved?)
@first_note_to_resolve ||= notes.find(&:to_be_resolved?) # rubocop:disable Gitlab/ModuleWithInstanceVariables
end
def last_resolved_note
return unless resolved?
@last_resolved_note ||= resolved_notes.sort_by(&:resolved_at).last
@last_resolved_note ||= resolved_notes.sort_by(&:resolved_at).last # rubocop:disable Gitlab/ModuleWithInstanceVariables
end
def resolved_notes
......@@ -95,7 +91,7 @@ module ResolvableDiscussion
yield(notes_relation)
# Set the notes array to the updated notes
@notes = notes_relation.fresh.to_a
@notes = notes_relation.fresh.to_a # rubocop:disable Gitlab/ModuleWithInstanceVariables
self.class.memoized_values.each do |var|
instance_variable_set(:"@#{var}", nil)
......
app/models/concerns/routable.rb
......@@ -88,7 +88,7 @@ module Routable
def full_name
if route && route.name.present?
@full_name ||= route.name
@full_name ||= route.name # rubocop:disable Gitlab/ModuleWithInstanceVariables
else
update_route if persisted?
......@@ -112,7 +112,7 @@ module Routable
def expires_full_path_cache
RequestStore.delete(full_path_key) if RequestStore.active?
@full_path = nil
@full_path = nil # rubocop:disable Gitlab/ModuleWithInstanceVariables
end
def build_full_path
......@@ -127,7 +127,7 @@ module Routable
def uncached_full_path
if route && route.path.present?
@full_path ||= route.path
@full_path ||= route.path # rubocop:disable Gitlab/ModuleWithInstanceVariables
else
update_route if persisted?
......@@ -166,7 +166,7 @@ module Routable
route || build_route(source: self)
route.path = build_full_path
route.name = build_full_name
@full_path = nil
@full_name = nil
@full_path = nil # rubocop:disable Gitlab/ModuleWithInstanceVariables
@full_name = nil # rubocop:disable Gitlab/ModuleWithInstanceVariables
end
end
app/models/concerns/spammable.rb
......@@ -12,6 +12,7 @@ module Spammable
attr_accessor :spam
attr_accessor :spam_log
alias_method :spam?, :spam
after_validation :check_for_spam, on: [:create, :update]
......@@ -34,10 +35,6 @@ module Spammable
end
end
def spam?
@spam
end
def check_for_spam
error_msg = if Gitlab::Recaptcha.enabled?
"Your #{spammable_entity_type} has been recognized as spam. "\
......
app/models/concerns/taskable.rb
......@@ -39,7 +39,7 @@ module Taskable
def task_list_items
return [] if description.blank?
@task_list_items ||= Taskable.get_tasks(description)
@task_list_items ||= Taskable.get_tasks(description) # rubocop:disable Gitlab/ModuleWithInstanceVariables
end
def tasks
......
app/models/concerns/time_trackable.rb
......@@ -21,6 +21,7 @@ module TimeTrackable
has_many :timelogs, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
end
# rubocop:disable Gitlab/ModuleWithInstanceVariables
def spend_time(options)
@time_spent = options[:duration]
@time_spent_user = options[:user]
......@@ -36,6 +37,7 @@ module TimeTrackable
end
end
alias_method :spend_time=, :spend_time
# rubocop:enable Gitlab/ModuleWithInstanceVariables
def total_time_spent
timelogs.sum(:time_spent)
......@@ -52,9 +54,10 @@ module TimeTrackable
private
def reset_spent_time
timelogs.new(time_spent: total_time_spent * -1, user: @time_spent_user)
timelogs.new(time_spent: total_time_spent * -1, user: @time_spent_user) # rubocop:disable Gitlab/ModuleWithInstanceVariables
end
# rubocop:disable Gitlab/ModuleWithInstanceVariables
def add_or_subtract_spent_time
timelogs.new(
time_spent: time_spent,
......@@ -62,16 +65,19 @@ module TimeTrackable
spent_at: @spent_at
)
end
# rubocop:enable Gitlab/ModuleWithInstanceVariables
def check_negative_time_spent
return if time_spent.nil? || time_spent == :reset
# we need to cache the total time spent so multiple calls to #valid?
# doesn't give a false error
@original_total_time_spent ||= total_time_spent
if time_spent < 0 && (time_spent.abs > @original_total_time_spent)
if time_spent < 0 && (time_spent.abs > original_total_time_spent)
errors.add(:time_spent, 'Time to subtract exceeds the total time spent')
end
end
# we need to cache the total time spent so multiple calls to #valid?
# doesn't give a false error
def original_total_time_spent
@original_total_time_spent ||= total_time_spent
end
end
app/serializers/concerns/with_pagination.rb
......@@ -14,7 +14,7 @@ module WithPagination
# we shouldn't try to paginate single resources
def represent(resource, opts = {})
if paginated? && resource.respond_to?(:page)
super(@paginator.paginate(resource), opts)
super(paginator.paginate(resource), opts)
else
super(resource, opts)
end
......
app/services/concerns/issues/resolve_discussions.rb
module Issues
module ResolveDiscussions
include Gitlab::Utils::StrongMemoize
attr_reader :merge_request_to_resolve_discussions_of_iid, :discussion_to_resolve_id
# rubocop:disable Gitlab/ModuleWithInstanceVariables
def filter_resolve_discussion_params
@merge_request_to_resolve_discussions_of_iid ||= params.delete(:merge_request_to_resolve_discussions_of)
@discussion_to_resolve_id ||= params.delete(:discussion_to_resolve)
end
# rubocop:enable Gitlab/ModuleWithInstanceVariables
def merge_request_to_resolve_discussions_of
return @merge_request_to_resolve_discussions_of if defined?(@merge_request_to_resolve_discussions_of)
@merge_request_to_resolve_discussions_of = MergeRequestsFinder.new(current_user, project_id: project.id)
.execute
.find_by(iid: merge_request_to_resolve_discussions_of_iid)
strong_memoize(:merge_request_to_resolve_discussions_of) do
MergeRequestsFinder.new(current_user, project_id: project.id)
.execute
.find_by(iid: merge_request_to_resolve_discussions_of_iid)
end
end
def discussions_to_resolve
return [] unless merge_request_to_resolve_discussions_of
@discussions_to_resolve ||=
@discussions_to_resolve ||= # rubocop:disable Gitlab/ModuleWithInstanceVariables
if discussion_to_resolve_id
discussion_or_nil = merge_request_to_resolve_discussions_of
.find_discussion(discussion_to_resolve_id)
......
app/services/spam_check_service.rb
......@@ -7,16 +7,19 @@
# - params with :request
#
module SpamCheckService
# rubocop:disable Gitlab/ModuleWithInstanceVariables
def filter_spam_check_params
@request = params.delete(:request)
@api = params.delete(:api)
@recaptcha_verified = params.delete(:recaptcha_verified)
@spam_log_id = params.delete(:spam_log_id)
end
# rubocop:enable Gitlab/ModuleWithInstanceVariables
# In order to be proceed to the spam check process, @spammable has to be
# a dirty instance, which means it should be already assigned with the new
# attribute values.
# rubocop:disable Gitlab/ModuleWithInstanceVariables
def spam_check(spammable, user)
spam_service = SpamService.new(spammable, @request)
......@@ -24,4 +27,5 @@ module SpamCheckService
user.spam_logs.find_by(id: @spam_log_id)&.update!(recaptcha_verified: true)
end
end
# rubocop:enable Gitlab/ModuleWithInstanceVariables
end
app/workers/concerns/new_issuable.rb
......@@ -9,15 +9,15 @@ module NewIssuable
end
def set_user(user_id)
@user = User.find_by(id: user_id)
@user = User.find_by(id: user_id) # rubocop:disable Gitlab/ModuleWithInstanceVariables
log_error(User, user_id) unless @user
log_error(User, user_id) unless @user # rubocop:disable Gitlab/ModuleWithInstanceVariables
end
def set_issuable(issuable_id)
@issuable = issuable_class.find_by(id: issuable_id)
@issuable = issuable_class.find_by(id: issuable_id) # rubocop:disable Gitlab/ModuleWithInstanceVariables
log_error(issuable_class, issuable_id) unless @issuable
log_error(issuable_class, issuable_id) unless @issuable # rubocop:disable Gitlab/ModuleWithInstanceVariables
end
def log_error(record_class, record_id)
......
config/initializers/fix_local_cache_middleware.rb
......@@ -6,7 +6,7 @@ module LocalCacheRegistryCleanupWithEnsure
def call(env)
LocalCacheRegistry.set_cache_for(local_cache_key, LocalStore.new)
response = @app.call(env)
response = @app.call(env) # rubocop:disable Gitlab/ModuleWithInstanceVariables
response[2] = ::Rack::BodyProxy.new(response[2]) do
LocalCacheRegistry.set_cache_for(local_cache_key, nil)
end
......
config/initializers/rspec_profiling.rb
......@@ -19,10 +19,10 @@ module RspecProfilingExt
def example_finished(*args)
super
rescue => err
return if @already_logged_example_finished_error
return if @already_logged_example_finished_error # rubocop:disable Gitlab/ModuleWithInstanceVariables
$stderr.puts "rspec_profiling couldn't collect an example: #{err}. Further warnings suppressed."
@already_logged_example_finished_error = true
@already_logged_example_finished_error = true # rubocop:disable Gitlab/ModuleWithInstanceVariables
end
alias_method :example_passed, :example_finished
......
config/initializers/rugged_use_gitlab_git_attributes.rb
......@@ -15,8 +15,11 @@ module Rugged
class Repository
module UseGitlabGitAttributes
def fetch_attributes(name, *)
attributes.attributes(name)
end
def attributes
@attributes ||= Gitlab::Git::Attributes.new(path)
@attributes.attributes(name)
end
end
......
doc/development/README.md
......@@ -37,6 +37,7 @@ comments: false
- [`Gemfile` guidelines](gemfile.md)
- [Sidekiq debugging](sidekiq_debugging.md)
- [Gotchas](gotchas.md) to avoid
- [Avoid modules with instance variables](module_with_instance_variables.md) if possible
- [Issue and merge requests state models](object_state_models.md)
- [How to dump production data to staging](db_dump.md)
- [Working with the GitHub importer](github_importer.md)
......
doc/development/module_with_instance_variables.md 0 → 100644
## Modules with instance variables could be considered harmful
### Background
Rails somehow encourages people using modules and instance variables
everywhere. For example, using instance variables in the controllers,
helpers, and views. They're also encouraging the use of
`ActiveSupport::Concern`, which further strengthens the idea of
saving everything in a giant, single object, and people could access
everything in that one giant object.
### The problems
Of course this is convenient to develop, because we just have everything
within reach. However this has a number of downsides when that chosen object
is growing, it would later become out of control for the same reason.
There are just too many things in the same context, and we don't know if
those things are tightly coupled or not, depending on each others or not.
It's very hard to tell when the complexity grows to a point, and it makes
tracking the code also extremely hard. For example, a class could be using
3 different instance variables, and all of them could be initialized and
manipulated from 3 different modules. It's hard to track when those variables
start giving us troubles. We don't know which module would suddenly change
one of the variables. Everything could touch anything.
### Similar concerns
People are saying multiple inheritance is bad. Mixing multiple modules with
multiple instance variables scattering everywhere suffer from the same issue.
The same applies to `ActiveSupport::Concern`. See:
[Consider replacing concerns with dedicated classes & composition](
https://gitlab.com/gitlab-org/gitlab-ce/issues/23786)
There's also a similar idea:
[Use decorators and interface segregation to solve overgrowing models problem](
https://gitlab.com/gitlab-org/gitlab-ce/issues/13484)
Note that `included` doesn't solve the whole issue. They define the
dependencies, but they still allow each modules to talk implicitly via the
instance variables in the final giant object, and that's where the problem is.
### Solutions
We should split the giant object into multiple objects, and they communicate
with each other with the API, i.e. public methods. In short, composition over
inheritance. This way, each smaller objects would have their own respective
limited states, i.e. instance variables. If one instance variable goes wrong,
we would be very clear that it's from that single small object, because
no one else could be touching it.
With clearly defined API, this would make things less coupled and much easier
to debug and track, and much more extensible for other objects to use, because
they communicate in a clear way, rather than implicit dependencies.
### Acceptable use
However, it's not always bad to use instance variables in a module,
as long as it's contained in the same module; that is, no other modules or
objects are touching them, then it would be an acceptable use.
We especially allow the case where a single instance variable is used with
`||=` to setup the value. This would look like:
``` ruby
module M
def f
@f ||= true
end
end
```
Unfortunately it's not easy to code more complex rules into the cop, so
we rely on people's best judgement. If we could find another good pattern
we could easily add to the cop, we should do it.
### How to rewrite and avoid disabling this cop
Even if we could just disable the cop, we should avoid doing so. Some code
could be easily rewritten in simple form. Consider this acceptable method:
``` ruby
module Gitlab
module Emoji
def emoji_unicode_version(name)
@emoji_unicode_versions_by_name ||=
JSON.parse(File.read(Rails.root.join('fixtures', 'emojis', 'emoji-unicode-version-map.json')))
@emoji_unicode_versions_by_name[name]
end
end
end
```
This method is totally fine because it's already self-contained. No other
methods should be using `@emoji_unicode_versions_by_name` and we're good.
However it's still offending the cop because it's not just `||=`, and the
cop is not smart enough to judge that this is fine.
On the other hand, we could split this method into two:
``` ruby
module Gitlab
module Emoji
def emoji_unicode_version(name)
emoji_unicode_versions_by_name[name]
end
private
def emoji_unicode_versions_by_name
@emoji_unicode_versions_by_name ||=
JSON.parse(File.read(Rails.root.join('fixtures', 'emojis', 'emoji-unicode-version-map.json')))
end
end
end
```
Now the cop won't complain. Here's a bad example which we could rewrite:
``` ruby
module SpamCheckService
def filter_spam_check_params
@request = params.delete(:request)
@api = params.delete(:api)
@recaptcha_verified = params.delete(:recaptcha_verified)
@spam_log_id = params.delete(:spam_log_id)
end
def spam_check(spammable, user)
spam_service = SpamService.new(spammable, @request)
spam_service.when_recaptcha_verified(@recaptcha_verified, @api) do
user.spam_logs.find_by(id: @spam_log_id)&.update!(recaptcha_verified: true)
end
end
end
```
There are several implicit dependencies here. First, `params` should be
defined before use. Second, `filter_spam_check_params` should be called
before `spam_check`. These are all implicit and the includer could be using
those instance variables without awareness.
This should be rewritten like:
``` ruby
class SpamCheckService
def initialize(request:, api:, recaptcha_verified:, spam_log_id:)
@request = request
@api = api
@recaptcha_verified = recaptcha_verified
@spam_log_id = spam_log_id
end
def spam_check(spammable, user)
spam_service = SpamService.new(spammable, @request)
spam_service.when_recaptcha_verified(@recaptcha_verified, @api) do
user.spam_logs.find_by(id: @spam_log_id)&.update!(recaptcha_verified: true)
end
end
end
```
And use it like:
``` ruby
class UpdateSnippetService < BaseService
def execute
# ...
spam = SpamCheckService.new(params.slice!(:request, :api, :recaptcha_verified, :spam_log_id))
spam.check(snippet, current_user)
# ...
end
end
```
This way, all those instance variables are isolated in `SpamCheckService`
rather than whatever includes the module, and those modules which were also
included, making it much easier to track down any issues,
and reducing the chance of having name conflicts.
### How to disable this cop
Put the disabling comment right after your code in the same line:
``` ruby
module M
def violating_method
@f + @g # rubocop:disable Gitlab/ModuleWithInstanceVariables
end
end
```
If there are multiple lines, you could also enable and disable for a section:
``` ruby
module M
# rubocop:disable Gitlab/ModuleWithInstanceVariables
def violating_method
@f = 0
@g = 1
@h = 2
end
# rubocop:enable Gitlab/ModuleWithInstanceVariables
end
```
Note that you need to enable it at some point, otherwise everything below
won't be checked.
### Things we might need to ignore right now
Because of the way Rails helpers and mailers work, we might not be able to
avoid the use of instance variables there. For those cases, we could ignore
them at the moment. At least we're not going to share those modules with
other random objects, so they're still somewhat isolated.
### Instance variables in views
They're bad because we can't easily tell who's using the instance variables
(from controller's point of view) and where we set them up (from partials'
point of view), making it extremely hard to track data dependency.
We're trying to use something like this instead:
``` haml
= render 'projects/commits/commit', commit: commit, ref: ref, project: project
```
And in the partial:
``` haml
- ref = local_assigns.fetch(:ref)
- commit = local_assigns.fetch(:commit)
- project = local_assigns.fetch(:project)
```
This way it's clearer where those values were coming from, and we gain the
benefit to have typo check over using instance variables. In the future,
we should also forbid the use of instance variables in partials.
features/support/env.rb
......@@ -42,11 +42,11 @@ module StdoutReporterWithScenarioLocation
# Override the standard reporter to show filename and line number next to each
# scenario for easy, focused re-runs
def before_scenario_run(scenario, step_definitions = nil)
@max_step_name_length = scenario.steps.map(&:name).map(&:length).max if scenario.steps.any?
@max_step_name_length = scenario.steps.map(&:name).map(&:length).max if scenario.steps.any? # rubocop:disable Gitlab/ModuleWithInstanceVariables
name = scenario.name
# This number has no significance, it's just to line things up
max_length = @max_step_name_length + 19
max_length = @max_step_name_length + 19 # rubocop:disable Gitlab/ModuleWithInstanceVariables
out.puts "\n #{'Scenario:'.green} #{name.light_green.ljust(max_length)}" \
" # #{scenario.feature.filename}:#{scenario.line}"
end
......
lib/api/helpers.rb
......@@ -32,6 +32,11 @@ module API
end
end
# rubocop:disable Gitlab/ModuleWithInstanceVariables
# We can't rewrite this with StrongMemoize because `sudo!` would
# actually write to `@current_user`, and `sudo?` would immediately
# call `current_user` again which reads from `@current_user`.
# We should rewrite this in a way that using StrongMemoize is possible
def current_user
return @current_user if defined?(@current_user)
......@@ -45,6 +50,7 @@ module API
@current_user
end
# rubocop:enable Gitlab/ModuleWithInstanceVariables
def sudo?
initial_current_user != current_user
......@@ -415,6 +421,7 @@ module API
private
# rubocop:disable Gitlab/ModuleWithInstanceVariables
def initial_current_user
return @initial_current_user if defined?(@initial_current_user)
......@@ -424,6 +431,7 @@ module API
unauthorized!
end
end
# rubocop:enable Gitlab/ModuleWithInstanceVariables
def sudo!
return unless sudo_identifier
......@@ -443,7 +451,7 @@ module API
sudoed_user = find_user(sudo_identifier)
not_found!("User with ID or username '#{sudo_identifier}'") unless sudoed_user
@current_user = sudoed_user
@current_user = sudoed_user # rubocop:disable Gitlab/ModuleWithInstanceVariables
end
def sudo_identifier
......
lib/api/helpers/internal_helpers.rb
......@@ -6,18 +6,16 @@ module API
'git-upload-pack' => [:ssh_upload_pack, Gitlab::GitalyClient::MigrationStatus::OPT_OUT]
}.freeze
attr_reader :redirected_path
def wiki?
set_project unless defined?(@wiki)
@wiki
set_project unless defined?(@wiki) # rubocop:disable Gitlab/ModuleWithInstanceVariables
@wiki # rubocop:disable Gitlab/ModuleWithInstanceVariables
end
def project
set_project unless defined?(@project)
@project
end
def redirected_path
@redirected_path
set_project unless defined?(@project) # rubocop:disable Gitlab/ModuleWithInstanceVariables
@project # rubocop:disable Gitlab/ModuleWithInstanceVariables
end
def ssh_authentication_abilities
......@@ -69,6 +67,7 @@ module API
private
# rubocop:disable Gitlab/ModuleWithInstanceVariables
def set_project
if params[:gl_repository]
@project, @wiki = Gitlab::GlRepository.parse(params[:gl_repository])
......@@ -77,6 +76,7 @@ module API
@project, @wiki, @redirected_path = Gitlab::RepoPath.parse(params[:project])
end
end
# rubocop:enable Gitlab/ModuleWithInstanceVariables
# Project id to pass between components that don't share/don't have
# access to the same filesystem mounts
......
lib/extracts_path.rb
......@@ -40,7 +40,7 @@ module ExtractsPath
def extract_ref(id)
pair = ['', '']
return pair unless @project
return pair unless @project # rubocop:disable Gitlab/ModuleWithInstanceVariables
if id =~ /^(\h{40})(.+)/
# If the ref appears to be a SHA, we're done, just split the string
......@@ -104,6 +104,7 @@ module ExtractsPath
#
# Automatically renders `not_found!` if a valid tree path could not be
# resolved (e.g., when a user inserts an invalid path or ref).
# rubocop:disable Gitlab/ModuleWithInstanceVariables
def assign_ref_vars
# assign allowed options
allowed_options = ["filter_ref"]
......@@ -130,14 +131,15 @@ module ExtractsPath
rescue RuntimeError, NoMethodError, InvalidPathError
render_404
end
# rubocop:enable Gitlab/ModuleWithInstanceVariables
def tree
@tree ||= @repo.tree(@commit.id, @path)
@tree ||= @repo.tree(@commit.id, @path) # rubocop:disable Gitlab/ModuleWithInstanceVariables
end
def lfs_blob_ids
blob_ids = tree.blobs.map(&:id)
@lfs_blob_ids = Gitlab::Git::Blob.batch_lfs_pointers(@project.repository, blob_ids).map(&:id)
@lfs_blob_ids = Gitlab::Git::Blob.batch_lfs_pointers(@project.repository, blob_ids).map(&:id) # rubocop:disable Gitlab/ModuleWithInstanceVariables
end
private
......@@ -150,8 +152,8 @@ module ExtractsPath
end
def ref_names
return [] unless @project
return [] unless @project # rubocop:disable Gitlab/ModuleWithInstanceVariables
@ref_names ||= @project.repository.ref_names
@ref_names ||= @project.repository.ref_names # rubocop:disable Gitlab/ModuleWithInstanceVariables
end
end
lib/gitlab/cache/request_cache.rb
......@@ -45,11 +45,13 @@ module Gitlab
klass.prepend(extension)
end
attr_accessor :request_cache_key_block
def request_cache_key(&block)
if block_given?
@request_cache_key = block
self.request_cache_key_block = block
else
@request_cache_key
request_cache_key_block
end
end
......
lib/gitlab/ci/charts.rb
......@@ -6,7 +6,7 @@ module Gitlab
query
.group("DATE(#{::Ci::Pipeline.table_name}.created_at)")
.count(:created_at)
.transform_keys { |date| date.strftime(@format) }
.transform_keys { |date| date.strftime(@format) } # rubocop:disable Gitlab/ModuleWithInstanceVariables
end
def interval_step
......
lib/gitlab/ci/config/entry/configurable.rb
......@@ -29,15 +29,15 @@ module Gitlab
self.class.nodes.each do |key, factory|
factory
.value(@config[key])
.value(config[key])
.with(key: key, parent: self)
@entries[key] = factory.create!
entries[key] = factory.create!
end
yield if block_given?
@entries.each_value do |entry|
entries.each_value do |entry|
entry.compose!(deps)
end
end
......@@ -59,13 +59,13 @@ module Gitlab
def helpers(*nodes)
nodes.each do |symbol|
define_method("#{symbol}_defined?") do
@entries[symbol]&.specified?
entries[symbol]&.specified?
end
define_method("#{symbol}_value") do
return unless @entries[symbol] && @entries[symbol].valid?
return unless entries[symbol] && entries[symbol].valid?
@entries[symbol].value
entries[symbol].value
end
end
end
......
lib/gitlab/ci/config/entry/node.rb
......@@ -90,6 +90,12 @@ module Gitlab
def self.aspects
@aspects ||= []
end
private
def entries
@entries
end
end
end
end
......
lib/gitlab/ci/config/entry/validatable.rb
......@@ -13,7 +13,7 @@ module Gitlab
end
def errors
@validator.messages + descendants.flat_map(&:errors)
@validator.messages + descendants.flat_map(&:errors) # rubocop:disable Gitlab/ModuleWithInstanceVariables
end
class_methods do
......
lib/gitlab/current_settings.rb
......@@ -53,7 +53,7 @@ module Gitlab
end
def in_memory_application_settings
@in_memory_application_settings ||= ::ApplicationSetting.new(::ApplicationSetting.defaults)
@in_memory_application_settings ||= ::ApplicationSetting.new(::ApplicationSetting.defaults) # rubocop:disable Gitlab/ModuleWithInstanceVariables
rescue ActiveRecord::StatementInvalid, ActiveRecord::UnknownAttributeError
# In case migrations the application_settings table is not created yet,
# we fallback to a simple OpenStruct
......
lib/gitlab/cycle_analytics/base_event_fetcher.rb
......@@ -56,7 +56,9 @@ module Gitlab
end
def allowed_ids
nil
@allowed_ids ||= allowed_ids_finder_class
.new(@options[:current_user], project_id: @project.id)
.execute.where(id: event_result_ids).pluck(:id)
end
def event_result_ids
......
lib/gitlab/cycle_analytics/base_query.rb
......@@ -14,9 +14,9 @@ module Gitlab
def stage_query
query = mr_closing_issues_table.join(issue_table).on(issue_table[:id].eq(mr_closing_issues_table[:issue_id]))
.join(issue_metrics_table).on(issue_table[:id].eq(issue_metrics_table[:issue_id]))
.where(issue_table[:project_id].eq(@project.id))
.where(issue_table[:project_id].eq(@project.id)) # rubocop:disable Gitlab/ModuleWithInstanceVariables
.where(issue_table[:deleted_at].eq(nil))
.where(issue_table[:created_at].gteq(@options[:from]))
.where(issue_table[:created_at].gteq(@options[:from])) # rubocop:disable Gitlab/ModuleWithInstanceVariables
# Load merge_requests
query = query.join(mr_table, Arel::Nodes::OuterJoin)
......
lib/gitlab/cycle_analytics/code_event_fetcher.rb
module Gitlab
module CycleAnalytics
class CodeEventFetcher < BaseEventFetcher
include MergeRequestAllowed
def initialize(*args)
@projections = [mr_table[:title],
mr_table[:iid],
......@@ -20,6 +18,10 @@ module Gitlab
def serialize(event)
AnalyticsMergeRequestSerializer.new(project: @project).represent(event)
end
def allowed_ids_finder_class
MergeRequestsFinder
end
end
end
end
lib/gitlab/cycle_analytics/issue_allowed.rb deleted 100644 → 0
module Gitlab
module CycleAnalytics
module IssueAllowed
def allowed_ids
@allowed_ids ||= IssuesFinder.new(@options[:current_user], project_id: @project.id).execute.where(id: event_result_ids).pluck(:id)
end
end
end
end
lib/gitlab/cycle_analytics/issue_event_fetcher.rb
module Gitlab
module CycleAnalytics
class IssueEventFetcher < BaseEventFetcher
include IssueAllowed
def initialize(*args)
@projections = [issue_table[:title],
issue_table[:iid],
......@@ -18,6 +16,10 @@ module Gitlab
def serialize(event)
AnalyticsIssueSerializer.new(project: @project).represent(event)
end
def allowed_ids_finder_class
IssuesFinder
end
end
end
end
lib/gitlab/cycle_analytics/merge_request_allowed.rb deleted 100644 → 0
module Gitlab
module CycleAnalytics
module MergeRequestAllowed
def allowed_ids
@allowed_ids ||= MergeRequestsFinder.new(@options[:current_user], project_id: @project.id).execute.where(id: event_result_ids).pluck(:id)
end
end
end
end
lib/gitlab/cycle_analytics/plan_event_fetcher.rb
......@@ -18,6 +18,10 @@ module Gitlab
private
def allowed_ids
nil
end
def merge_request_diff_commits
@merge_request_diff_commits ||=
MergeRequestDiffCommit
......
lib/gitlab/cycle_analytics/production_helper.rb
......@@ -2,7 +2,9 @@ module Gitlab
module CycleAnalytics
module ProductionHelper
def stage_query
super.where(mr_metrics_table[:first_deployed_to_production_at].gteq(@options[:from]))
super
.where(mr_metrics_table[:first_deployed_to_production_at]
.gteq(@options[:from])) # rubocop:disable Gitlab/ModuleWithInstanceVariables
end
end
end
......
lib/gitlab/cycle_analytics/review_event_fetcher.rb
module Gitlab
module CycleAnalytics
class ReviewEventFetcher < BaseEventFetcher
include MergeRequestAllowed
def initialize(*args)
@projections = [mr_table[:title],
mr_table[:iid],
......@@ -14,9 +12,15 @@ module Gitlab
super(*args)
end
private
def serialize(event)
AnalyticsMergeRequestSerializer.new(project: @project).represent(event)
end
def allowed_ids_finder_class
MergeRequestsFinder
end
end
end
end
lib/gitlab/cycle_analytics/staging_event_fetcher.rb
......@@ -22,6 +22,10 @@ module Gitlab
private
def allowed_ids
nil
end
def serialize(event)
AnalyticsBuildSerializer.new.represent(event['build'])
end
......
lib/gitlab/database/rename_reserved_paths_migration/v1/migration_classes.rb
......@@ -6,7 +6,7 @@ module Gitlab
module Routable
def full_path
if route && route.path.present?
@full_path ||= route.path
@full_path ||= route.path # rubocop:disable Gitlab/ModuleWithInstanceVariables
else
update_route if persisted?
......@@ -30,7 +30,7 @@ module Gitlab
def prepare_route
route || build_route(source: self)
route.path = build_full_path
@full_path = nil
@full_path = nil # rubocop:disable Gitlab/ModuleWithInstanceVariables
end
end
......
lib/gitlab/emoji.rb
......@@ -31,8 +31,7 @@ module Gitlab
end
def emoji_unicode_version(name)
@emoji_unicode_versions_by_name ||= JSON.parse(File.read(Rails.root.join('fixtures', 'emojis', 'emoji-unicode-version-map.json')))
@emoji_unicode_versions_by_name[name]
emoji_unicode_versions_by_name[name]
end
def normalize_emoji_name(name)
......@@ -56,5 +55,12 @@ module Gitlab
ActionController::Base.helpers.content_tag('gl-emoji', emoji_info['moji'], title: emoji_info['description'], data: data)
end
private
def emoji_unicode_versions_by_name
@emoji_unicode_versions_by_name ||=
JSON.parse(File.read(Rails.root.join('fixtures', 'emojis', 'emoji-unicode-version-map.json')))
end
end
end
lib/gitlab/git/popen.rb
......@@ -16,8 +16,8 @@ module Gitlab
vars['PWD'] = path
options = { chdir: path }
@cmd_output = ""
@cmd_status = 0
cmd_output = ""
cmd_status = 0
Open3.popen3(vars, *cmd, options) do |stdin, stdout, stderr, wait_thr|
yield(stdin) if block_given?
stdin.close
......@@ -25,14 +25,14 @@ module Gitlab
if lazy_block
return lazy_block.call(stdout.lazy)
else
@cmd_output << stdout.read
cmd_output << stdout.read
end
@cmd_output << stderr.read
@cmd_status = wait_thr.value.exitstatus
cmd_output << stderr.read
cmd_status = wait_thr.value.exitstatus
end
[@cmd_output, @cmd_status]
[cmd_output, cmd_status]
end
def popen_with_timeout(cmd, timeout, path, vars = {})
......
lib/gitlab/import_export/command_line_util.rb
......@@ -32,7 +32,7 @@ module Gitlab
def execute(cmd)
output, status = Gitlab::Popen.popen(cmd)
@shared.error(Gitlab::ImportExport::Error.new(output.to_s)) unless status.zero?
@shared.error(Gitlab::ImportExport::Error.new(output.to_s)) unless status.zero? # rubocop:disable Gitlab/ModuleWithInstanceVariables
status.zero?
end
......
lib/gitlab/metrics/influx_db.rb
......@@ -154,6 +154,7 @@ module Gitlab
# When enabled this should be set before being used as the usual pattern
# "@foo ||= bar" is _not_ thread-safe.
# rubocop:disable Gitlab/ModuleWithInstanceVariables
def pool
if influx_metrics_enabled?
if @pool.nil?
......@@ -170,6 +171,7 @@ module Gitlab
@pool
end
end
# rubocop:enable Gitlab/ModuleWithInstanceVariables
end
end
end
lib/gitlab/metrics/prometheus.rb
......@@ -4,6 +4,7 @@ module Gitlab
module Metrics
module Prometheus
include Gitlab::CurrentSettings
include Gitlab::Utils::StrongMemoize
REGISTRY_MUTEX = Mutex.new
PROVIDER_MUTEX = Mutex.new
......@@ -17,16 +18,18 @@ module Gitlab
end
def prometheus_metrics_enabled?
return @prometheus_metrics_enabled if defined?(@prometheus_metrics_enabled)
@prometheus_metrics_enabled = prometheus_metrics_enabled_unmemoized
strong_memoize(:prometheus_metrics_enabled) do
prometheus_metrics_enabled_unmemoized
end
end
def registry
return @registry if @registry
REGISTRY_MUTEX.synchronize do
@registry ||= ::Prometheus::Client.registry
strong_memoize(:registry) do
REGISTRY_MUTEX.synchronize do
strong_memoize(:registry) do
::Prometheus::Client.registry
end
end
end
end
......
lib/gitlab/slash_commands/presenters/issue_base.rb
......@@ -11,32 +11,36 @@ module Gitlab
end
def project
@resource.project
resource.project
end
def author
@resource.author
resource.author
end
def fields
[
{
title: "Assignee",
value: @resource.assignees.any? ? @resource.assignees.first.name : "_None_",
value: resource.assignees.any? ? resource.assignees.first.name : "_None_",
short: true
},
{
title: "Milestone",
value: @resource.milestone ? @resource.milestone.title : "_None_",
value: resource.milestone ? resource.milestone.title : "_None_",
short: true
},
{
title: "Labels",
value: @resource.labels.any? ? @resource.label_names.join(', ') : "_None_",
value: resource.labels.any? ? resource.label_names.join(', ') : "_None_",
short: true
}
]
end
private
attr_reader :resource
end
end
end
......
lib/tasks/gettext.rake
......@@ -23,6 +23,7 @@ namespace :gettext do
desc 'Lint all po files in `locale/'
task lint: :environment do
require 'simple_po_parser'
require 'gitlab/utils'
FastGettext.silence_errors
files = Dir.glob(Rails.root.join('locale/*/gitlab.po'))
......
lib/tasks/gitlab/task_helpers.rb
require 'rainbow/ext/string'
require 'gitlab/utils/strong_memoize'
module Gitlab
TaskFailedError = Class.new(StandardError)
TaskAbortedByUserError = Class.new(StandardError)
module TaskHelpers
include Gitlab::Utils::StrongMemoize
extend self
# Ask if the user wants to continue
......@@ -105,16 +108,16 @@ module Gitlab
end
def gitlab_user?
return @is_gitlab_user unless @is_gitlab_user.nil?
current_user = run_command(%w(whoami)).chomp
@is_gitlab_user = current_user == gitlab_user
strong_memoize(:is_gitlab_user) do
current_user = run_command(%w(whoami)).chomp
current_user == gitlab_user
end
end
def warn_user_is_not_gitlab
return if @warned_user_not_gitlab
return if gitlab_user?
unless gitlab_user?
strong_memoize(:warned_user_not_gitlab) do
current_user = run_command(%w(whoami)).chomp
puts " Warning ".color(:black).background(:yellow)
......@@ -122,8 +125,6 @@ module Gitlab
puts " Things may work\/fail for the wrong reasons."
puts " For correct results you should run this as user #{gitlab_user.color(:magenta)}."
puts ""
@warned_user_not_gitlab = true
end
end
......
qa/qa/runtime/scenario.rb
......@@ -6,13 +6,15 @@ module QA
module Scenario
extend self
attr_reader :attributes
def attributes
@attributes ||= {}
end
def define(attribute, value)
(@attributes ||= {}).store(attribute.to_sym, value)
attributes.store(attribute.to_sym, value)
define_singleton_method(attribute) do
@attributes[attribute.to_sym].tap do |value|
attributes[attribute.to_sym].tap do |value|
if value.to_s.empty?
raise ArgumentError, "Empty `#{attribute}` attribute!"
end
......
rubocop/cop/gitlab/module_with_instance_variables.rb 0 → 100644
module RuboCop
module Cop
module Gitlab
class ModuleWithInstanceVariables < RuboCop::Cop::Cop
MSG = <<~EOL.freeze
Do not use instance variables in a module. Please read this
for the rationale behind it:
https://docs.gitlab.com/ee/development/module_with_instance_variables.html
EOL
def on_module(node)
check_method_definition(node)
# Not sure why some module would have an extra begin wrapping around
node.each_child_node(:begin) do |begin_node|
check_method_definition(begin_node)
end
end
private
def check_method_definition(node)
node.each_child_node(:def) do |definition|
# We allow this pattern:
#
# def f
# @f ||= true
# end
if only_ivar_or_assignment?(definition)
# We don't allow if any other ivar is used
definition.each_descendant(:ivar) do |offense|
add_offense(offense, :expression)
end
# We allow initialize method and single ivar
elsif !initialize_method?(definition) && !single_ivar?(definition)
definition.each_descendant(:ivar, :ivasgn) do |offense|
add_offense(offense, :expression)
end
end
end
end
def only_ivar_or_assignment?(definition)
node = definition.child_nodes.last
definition.child_nodes.size == 2 &&
node.or_asgn_type? && node.child_nodes.first.ivasgn_type?
end
def single_ivar?(definition)
node = definition.child_nodes.last
definition.child_nodes.size == 2 && node.ivar_type?
end
def initialize_method?(definition)
definition.children.first == :initialize
end
end
end
end
end
rubocop/rubocop.rb
......@@ -8,6 +8,7 @@ require_relative 'cop/line_break_after_guard_clauses'
require_relative 'cop/polymorphic_associations'
require_relative 'cop/project_path_helper'
require_relative 'cop/redirect_with_status'
require_relative 'cop/gitlab/module_with_instance_variables'
require_relative 'cop/sidekiq_options_queue'
require_relative 'cop/migration/add_column'
require_relative 'cop/migration/add_concurrent_foreign_key'
......
spec/lib/gitlab/cycle_analytics/base_event_fetcher_spec.rb
......@@ -23,6 +23,8 @@ describe Gitlab::CycleAnalytics::BaseEventFetcher do
allow_any_instance_of(described_class).to receive(:serialize) do |event|
event
end
allow_any_instance_of(described_class)
.to receive(:allowed_ids).and_return(nil)
stub_const('Gitlab::CycleAnalytics::BaseEventFetcher::MAX_EVENTS', max_events)
......
spec/rubocop/cop/gitlab/module_with_instance_variables_spec.rb 0 → 100644
require 'spec_helper'
require 'rubocop'
require 'rubocop/rspec/support'
require_relative '../../../../rubocop/cop/gitlab/module_with_instance_variables'
describe RuboCop::Cop::Gitlab::ModuleWithInstanceVariables 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 instance variable is used in a module' do
inspect_source(cop, 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(cop, source)
expect(cop.offenses).to be_empty
end
end
context 'when source is a regular module' do
it_behaves_like 'registering offense', offending_lines: [3] do
let(:source) do
<<~RUBY
module M
def f
@f = true
end
end
RUBY
end
end
end
context 'when source is a nested module' do
it_behaves_like 'registering offense', offending_lines: [4] do
let(:source) do
<<~RUBY
module N
module M
def f
@f = true
end
end
end
RUBY
end
end
end
context 'when source is a nested module with multiple offenses' do
it_behaves_like 'registering offense', offending_lines: [4, 12] do
let(:source) do
<<~RUBY
module N
module M
def f
@f = true
end
def g
true
end
def h
@h = true
end
end
end
RUBY
end
end
end
context 'when source is using simple or ivar assignment' do
it_behaves_like 'not registering offense' do
let(:source) do
<<~RUBY
module M
def f
@f ||= true
end
end
RUBY
end
end
end
context 'when source is using simple ivar' do
it_behaves_like 'not registering offense' do
let(:source) do
<<~RUBY
module M
def f?
@f
end
end
RUBY
end
end
end
context 'when source is defining initialize' do
it_behaves_like 'not registering offense' do
let(:source) do
<<~RUBY
module M
def initialize
@a = 1
@b = 2
end
end
RUBY
end
end
end
context 'when source is using simple or ivar assignment with other ivar' do
it_behaves_like 'registering offense', offending_lines: [3] do
let(:source) do
<<~RUBY
module M
def f
@f ||= g(@g)
end
end
RUBY
end
end
end
context 'when source is using or ivar assignment with something else' do
it_behaves_like 'registering offense', offending_lines: [3, 4] do
let(:source) do
<<~RUBY
module M
def f
@f ||= true
@f.to_s
end
end
RUBY
end
end
end
end
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment