BigW Consortium Gitlab

Skip to content

G
gitlab-ce
Commit 34c083a1 by Robert Speicher
Options

Merge branch 'rubocop/enable-access-modifiers-cops' into 'master'

Enable Rubocop cops that check access modifiers ## What does this MR do? This MR enables Rubocop cops that detect methods that should be restricted but are the part of public API because of access modifiers used improperly. This also fixes existing offenses. ## Why was this MR needed? Some method in our codebase are public instead of being private because it is sometimes difficult to get it right without static analysis. ## What are the relevant issue numbers? See #17478 Closes #17372 See merge request !5014
parents 4284724d d6f66977
Showing with 184 additions and 177 deletions
.rubocop.yml
......@@ -510,6 +510,15 @@ Metrics/PerceivedComplexity:
#################### Lint ################################
# Checks for useless access modifiers.
Lint/UselessAccessModifier:
Enabled: true
# Checks for attempts to use `private` or `protected` to set the visibility
# of a class method, which does not work.
Lint/IneffectiveAccessModifier:
Enabled: false
# Checks for ambiguous operators in the first argument of a method invocation
# without parentheses.
Lint/AmbiguousOperator:
......
.rubocop_todo.yml
......@@ -19,10 +19,6 @@ Lint/AssignmentInCondition:
Lint/HandleExceptions:
Enabled: false
# Offense count: 21
Lint/IneffectiveAccessModifier:
Enabled: false
# Offense count: 2
Lint/Loop:
Enabled: false
......@@ -48,10 +44,6 @@ Lint/UnusedBlockArgument:
Lint/UnusedMethodArgument:
Enabled: false
# Offense count: 11
Lint/UselessAccessModifier:
Enabled: false
# Offense count: 12
# Cop supports --auto-correct.
Performance/PushSplat:
......
app/controllers/import/bitbucket_controller.rb
......@@ -82,8 +82,6 @@ class Import::BitbucketController < Import::BaseController
go_to_bitbucket_for_permissions
end
private
def access_params
{
bitbucket_access_token: session[:bitbucket_access_token],
......
app/controllers/import/gitlab_controller.rb
......@@ -61,8 +61,6 @@ class Import::GitlabController < Import::BaseController
go_to_gitlab_for_permissions
end
private
def access_params
{ gitlab_access_token: session[:gitlab_access_token] }
end
......
app/helpers/diff_helper.rb
......@@ -144,8 +144,6 @@ module DiffHelper
toggle_whitespace_link(url, options)
end
private
def hide_whitespace?
params[:w] == '1'
end
......
app/models/concerns/token_authenticatable.rb
module TokenAuthenticatable
extend ActiveSupport::Concern
private
def write_new_token(token_field)
new_token = generate_token(token_field)
write_attribute(token_field, new_token)
end
def generate_token(token_field)
loop do
token = Devise.friendly_token
break token unless self.class.unscoped.find_by(token_field => token)
end
end
class_methods do
def authentication_token_fields
@token_fields || []
end
private
private # rubocop:disable Lint/UselessAccessModifier
def add_authentication_token_field(token_field)
@token_fields = [] unless @token_fields
......@@ -32,18 +46,4 @@ module TokenAuthenticatable
end
end
end
private
def write_new_token(token_field)
new_token = generate_token(token_field)
write_attribute(token_field, new_token)
end
def generate_token(token_field)
loop do
token = Devise.friendly_token
break token unless self.class.unscoped.find_by(token_field => token)
end
end
end
app/services/auth/container_registry_authentication_service.rb
......@@ -24,10 +24,14 @@ module Auth
token[:access] = names.map do |name|
{ type: 'repository', name: name, actions: %w(*) }
end
token.encoded
end
def self.token_expire_at
Time.now + current_application_settings.container_registry_token_expire_delay.minutes
end
private
def authorized_token(*accesses)
......@@ -35,7 +39,7 @@ module Auth
token.issuer = registry.issuer
token.audience = params[:service]
token.subject = current_user.try(:username)
token.expire_time = ContainerRegistryAuthenticationService.token_expire_at
token.expire_time = self.class.token_expire_at
token[:access] = accesses.compact
token
end
......@@ -81,9 +85,5 @@ module Auth
def registry
Gitlab.config.registry
end
def self.token_expire_at
Time.now + current_application_settings.container_registry_token_expire_delay.minutes
end
end
end
app/services/system_note_service.rb
......@@ -2,7 +2,9 @@
#
# Used for creating system notes (e.g., when a user references a merge request
# from an issue, an issue's assignee changes, an issue is closed, etc.)
class SystemNoteService
module SystemNoteService
extend self
# Called when commits are added to a Merge Request
#
# noteable - Noteable object
......@@ -15,7 +17,7 @@ class SystemNoteService
# See new_commit_summary and existing_commit_summary.
#
# Returns the created Note object
def self.add_commits(noteable, project, author, new_commits, existing_commits = [], oldrev = nil)
def add_commits(noteable, project, author, new_commits, existing_commits = [], oldrev = nil)
total_count = new_commits.length + existing_commits.length
commits_text = "#{total_count} commit".pluralize(total_count)
......@@ -40,7 +42,7 @@ class SystemNoteService
# "Reassigned to @rspeicher"
#
# Returns the created Note object
def self.change_assignee(noteable, project, author, assignee)
def change_assignee(noteable, project, author, assignee)
body = assignee.nil? ? 'Assignee removed' : "Reassigned to #{assignee.to_reference}"
create_note(noteable: noteable, project: project, author: author, note: body)
......@@ -63,7 +65,7 @@ class SystemNoteService
# "Removed ~5 label"
#
# Returns the created Note object
def self.change_label(noteable, project, author, added_labels, removed_labels)
def change_label(noteable, project, author, added_labels, removed_labels)
labels_count = added_labels.count + removed_labels.count
references = ->(label) { label.to_reference(format: :id) }
......@@ -101,7 +103,7 @@ class SystemNoteService
# "Miletone changed to 7.11"
#
# Returns the created Note object
def self.change_milestone(noteable, project, author, milestone)
def change_milestone(noteable, project, author, milestone)
body = 'Milestone '
body += milestone.nil? ? 'removed' : "changed to #{milestone.to_reference(project)}"
......@@ -123,7 +125,7 @@ class SystemNoteService
# "Status changed to closed by bc17db76"
#
# Returns the created Note object
def self.change_status(noteable, project, author, status, source)
def change_status(noteable, project, author, status, source)
body = "Status changed to #{status}"
body << " by #{source.gfm_reference(project)}" if source
......@@ -131,26 +133,26 @@ class SystemNoteService
end
# Called when 'merge when build succeeds' is executed
def self.merge_when_build_succeeds(noteable, project, author, last_commit)
def merge_when_build_succeeds(noteable, project, author, last_commit)
body = "Enabled an automatic merge when the build for #{last_commit.to_reference(project)} succeeds"
create_note(noteable: noteable, project: project, author: author, note: body)
end
# Called when 'merge when build succeeds' is canceled
def self.cancel_merge_when_build_succeeds(noteable, project, author)
def cancel_merge_when_build_succeeds(noteable, project, author)
body = 'Canceled the automatic merge'
create_note(noteable: noteable, project: project, author: author, note: body)
end
def self.remove_merge_request_wip(noteable, project, author)
def remove_merge_request_wip(noteable, project, author)
body = 'Unmarked this merge request as a Work In Progress'
create_note(noteable: noteable, project: project, author: author, note: body)
end
def self.add_merge_request_wip(noteable, project, author)
def add_merge_request_wip(noteable, project, author)
body = 'Marked this merge request as a **Work In Progress**'
create_note(noteable: noteable, project: project, author: author, note: body)
......@@ -168,7 +170,7 @@ class SystemNoteService
# "Title changed from **Old** to **New**"
#
# Returns the created Note object
def self.change_title(noteable, project, author, old_title)
def change_title(noteable, project, author, old_title)
new_title = noteable.title.dup
old_diffs, new_diffs = Gitlab::Diff::InlineDiff.new(old_title, new_title).inline_diffs
......@@ -191,7 +193,7 @@ class SystemNoteService
# "Made the issue confidential"
#
# Returns the created Note object
def self.change_issue_confidentiality(issue, project, author)
def change_issue_confidentiality(issue, project, author)
body = issue.confidential ? 'Made the issue confidential' : 'Made the issue visible'
create_note(noteable: issue, project: project, author: author, note: body)
end
......@@ -210,7 +212,7 @@ class SystemNoteService
# "Target branch changed from `Old` to `New`"
#
# Returns the created Note object
def self.change_branch(noteable, project, author, branch_type, old_branch, new_branch)
def change_branch(noteable, project, author, branch_type, old_branch, new_branch)
body = "#{branch_type} branch changed from `#{old_branch}` to `#{new_branch}`".capitalize
create_note(noteable: noteable, project: project, author: author, note: body)
end
......@@ -229,7 +231,7 @@ class SystemNoteService
# "Restored target branch `feature`"
#
# Returns the created Note object
def self.change_branch_presence(noteable, project, author, branch_type, branch, presence)
def change_branch_presence(noteable, project, author, branch_type, branch, presence)
verb =
if presence == :add
'restored'
......@@ -245,7 +247,7 @@ class SystemNoteService
# Example note text:
#
# "Started branch `201-issue-branch-button`"
def self.new_issue_branch(issue, project, author, branch)
def new_issue_branch(issue, project, author, branch)
h = Gitlab::Routing.url_helpers
link = h.namespace_project_compare_url(project.namespace, project, from: project.default_branch, to: branch)
......@@ -270,7 +272,7 @@ class SystemNoteService
# See cross_reference_note_content.
#
# Returns the created Note object
def self.cross_reference(noteable, mentioner, author)
def cross_reference(noteable, mentioner, author)
return if cross_reference_disallowed?(noteable, mentioner)
gfm_reference = mentioner.gfm_reference(noteable.project)
......@@ -294,7 +296,7 @@ class SystemNoteService
end
end
def self.cross_reference?(note_text)
def cross_reference?(note_text)
note_text.start_with?(cross_reference_note_prefix)
end
......@@ -308,7 +310,7 @@ class SystemNoteService
# mentioner - Mentionable object
#
# Returns Boolean
def self.cross_reference_disallowed?(noteable, mentioner)
def cross_reference_disallowed?(noteable, mentioner)
return true if noteable.is_a?(ExternalIssue) && !noteable.project.jira_tracker_active?
return false unless mentioner.is_a?(MergeRequest)
return false unless noteable.is_a?(Commit)
......@@ -328,7 +330,7 @@ class SystemNoteService
#
# Returns Boolean
def self.cross_reference_exists?(noteable, mentioner)
def cross_reference_exists?(noteable, mentioner)
# Initial scope should be system notes of this noteable type
notes = Note.system.where(noteable_type: noteable.class)
......@@ -342,9 +344,60 @@ class SystemNoteService
notes_for_mentioner(mentioner, noteable, notes).count > 0
end
# Build an Array of lines detailing each commit added in a merge request
#
# new_commits - Array of new Commit objects
#
# Returns an Array of Strings
def new_commit_summary(new_commits)
new_commits.collect do |commit|
"* #{commit.short_id} - #{escape_html(commit.title)}"
end
end
# Called when the status of a Task has changed
#
# noteable - Noteable object.
# project - Project owning noteable
# author - User performing the change
# new_task - TaskList::Item object.
#
# Example Note text:
#
# "Soandso marked the task Whatever as completed."
#
# Returns the created Note object
def change_task_status(noteable, project, author, new_task)
status_label = new_task.complete? ? Taskable::COMPLETED : Taskable::INCOMPLETE
body = "Marked the task **#{new_task.source}** as #{status_label}"
create_note(noteable: noteable, project: project, author: author, note: body)
end
# Called when noteable has been moved to another project
#
# direction - symbol, :to or :from
# noteable - Noteable object
# noteable_ref - Referenced noteable
# author - User performing the move
#
# Example Note text:
#
# "Moved to some_namespace/project_new#11"
#
# Returns the created Note object
def noteable_moved(noteable, project, noteable_ref, author, direction:)
unless [:to, :from].include?(direction)
raise ArgumentError, "Invalid direction `#{direction}`"
end
cross_reference = noteable_ref.to_reference(project)
body = "Moved #{direction} #{cross_reference}"
create_note(noteable: noteable, project: project, author: author, note: body)
end
private
def self.notes_for_mentioner(mentioner, noteable, notes)
def notes_for_mentioner(mentioner, noteable, notes)
if mentioner.is_a?(Commit)
notes.where('note LIKE ?', "#{cross_reference_note_prefix}%#{mentioner.to_reference(nil)}")
else
......@@ -353,29 +406,18 @@ class SystemNoteService
end
end
def self.create_note(args = {})
def create_note(args = {})
Note.create(args.merge(system: true))
end
def self.cross_reference_note_prefix
def cross_reference_note_prefix
'mentioned in '
end
def self.cross_reference_note_content(gfm_reference)
def cross_reference_note_content(gfm_reference)
"#{cross_reference_note_prefix}#{gfm_reference}"
end
# Build an Array of lines detailing each commit added in a merge request
#
# new_commits - Array of new Commit objects
#
# Returns an Array of Strings
def self.new_commit_summary(new_commits)
new_commits.collect do |commit|
"* #{commit.short_id} - #{escape_html(commit.title)}"
end
end
# Build a single line summarizing existing commits being added in a merge
# request
#
......@@ -392,7 +434,7 @@ class SystemNoteService
# "* ea0f8418 - 1 commit from branch `feature`"
#
# Returns a newline-terminated String
def self.existing_commit_summary(noteable, existing_commits, oldrev = nil)
def existing_commit_summary(noteable, existing_commits, oldrev = nil)
return '' if existing_commits.empty?
count = existing_commits.size
......@@ -415,47 +457,7 @@ class SystemNoteService
"* #{commit_ids} - #{commits_text} from branch `#{branch}`\n"
end
# Called when the status of a Task has changed
#
# noteable - Noteable object.
# project - Project owning noteable
# author - User performing the change
# new_task - TaskList::Item object.
#
# Example Note text:
#
# "Soandso marked the task Whatever as completed."
#
# Returns the created Note object
def self.change_task_status(noteable, project, author, new_task)
status_label = new_task.complete? ? Taskable::COMPLETED : Taskable::INCOMPLETE
body = "Marked the task **#{new_task.source}** as #{status_label}"
create_note(noteable: noteable, project: project, author: author, note: body)
end
# Called when noteable has been moved to another project
#
# direction - symbol, :to or :from
# noteable - Noteable object
# noteable_ref - Referenced noteable
# author - User performing the move
#
# Example Note text:
#
# "Moved to some_namespace/project_new#11"
#
# Returns the created Note object
def self.noteable_moved(noteable, project, noteable_ref, author, direction:)
unless [:to, :from].include?(direction)
raise ArgumentError, "Invalid direction `#{direction}`"
end
cross_reference = noteable_ref.to_reference(project)
body = "Moved #{direction} #{cross_reference}"
create_note(noteable: noteable, project: project, author: author, note: body)
end
def self.escape_html(text)
def escape_html(text)
Rack::Utils.escape_html(text)
end
end
lib/banzai/filter/emoji_filter.rb
......@@ -38,6 +38,11 @@ module Banzai
end
end
# Build a regexp that matches all valid :emoji: names.
def self.emoji_pattern
@emoji_pattern ||= /:(#{Gitlab::Emoji.emojis_names.map { |name| Regexp.escape(name) }.join('|')}):/
end
private
def emoji_url(name)
......@@ -59,11 +64,6 @@ module Banzai
ActionController::Base.helpers.url_to_image(image)
end
# Build a regexp that matches all valid :emoji: names.
def self.emoji_pattern
@emoji_pattern ||= /:(#{Gitlab::Emoji.emojis_names.map { |name| Regexp.escape(name) }.join('|')}):/
end
def emoji_pattern
self.class.emoji_pattern
end
......
lib/banzai/filter/markdown_filter.rb
......@@ -12,7 +12,12 @@ module Banzai
html
end
private
def self.renderer
@renderer ||= begin
renderer = Redcarpet::Render::HTML.new
Redcarpet::Markdown.new(renderer, redcarpet_options)
end
end
def self.redcarpet_options
# https://github.com/vmg/redcarpet#and-its-like-really-simple-to-use
......@@ -28,12 +33,7 @@ module Banzai
}.freeze
end
def self.renderer
@renderer ||= begin
renderer = Redcarpet::Render::HTML.new
Redcarpet::Markdown.new(renderer, redcarpet_options)
end
end
private_class_method :redcarpet_options
end
end
end
lib/banzai/renderer.rb
module Banzai
module Renderer
extend self
# Convert a Markdown String into an HTML-safe String of HTML
#
# Note that while the returned HTML will have been sanitized of dangerous
......@@ -14,7 +16,7 @@ module Banzai
# context - Hash of context options passed to our HTML Pipeline
#
# Returns an HTML-safe String
def self.render(text, context = {})
def render(text, context = {})
cache_key = context.delete(:cache_key)
cache_key = full_cache_key(cache_key, context[:pipeline])
......@@ -52,7 +54,7 @@ module Banzai
# texts_and_contexts
# => [{ text: '### Hello',
# context: { cache_key: [note, :note] } }]
def self.cache_collection_render(texts_and_contexts)
def cache_collection_render(texts_and_contexts)
items_collection = texts_and_contexts.each_with_index do |item, index|
context = item[:context]
cache_key = full_cache_multi_key(context.delete(:cache_key), context[:pipeline])
......@@ -81,7 +83,7 @@ module Banzai
items_collection.map { |item| item[:rendered] }
end
def self.render_result(text, context = {})
def render_result(text, context = {})
text = Pipeline[:pre_process].to_html(text, context) if text
Pipeline[context[:pipeline]].call(text, context)
......@@ -100,7 +102,7 @@ module Banzai
# :user - User object
#
# Returns an HTML-safe String
def self.post_process(html, context)
def post_process(html, context)
context = Pipeline[context[:pipeline]].transform_context(context)
pipeline = Pipeline[:post_process]
......@@ -113,7 +115,7 @@ module Banzai
private
def self.cacheless_render(text, context = {})
def cacheless_render(text, context = {})
Gitlab::Metrics.measure(:banzai_cacheless_render) do
result = render_result(text, context)
......@@ -126,7 +128,7 @@ module Banzai
end
end
def self.full_cache_key(cache_key, pipeline_name)
def full_cache_key(cache_key, pipeline_name)
return unless cache_key
["banzai", *cache_key, pipeline_name || :full]
end
......@@ -134,7 +136,7 @@ module Banzai
# To map Rails.cache.read_multi results we need to know the Rails.cache.expanded_key.
# Other option will be to generate stringified keys on our side and don't delegate to Rails.cache.expanded_key
# method.
def self.full_cache_multi_key(cache_key, pipeline_name)
def full_cache_multi_key(cache_key, pipeline_name)
return unless cache_key
Rails.cache.send(:expanded_key, full_cache_key(cache_key, pipeline_name))
end
......
lib/gitlab/ci/config/node/configurable.rb
......@@ -40,7 +40,7 @@ module Gitlab
Hash[(@nodes || {}).map { |key, factory| [key, factory.dup] }]
end
private
private # rubocop:disable Lint/UselessAccessModifier
def node(key, node, metadata)
factory = Node::Factory.new(node)
......
lib/gitlab/database.rb
......@@ -55,12 +55,12 @@ module Gitlab
end
end
private
def self.connection
ActiveRecord::Base.connection
end
private_class_method :connection
def self.database_version
row = connection.execute("SELECT VERSION()").first
......@@ -70,5 +70,7 @@ module Gitlab
row.first
end
end
private_class_method :database_version
end
end
lib/gitlab/diff/inline_diff.rb
......@@ -19,24 +19,6 @@ module Gitlab
attr_accessor :old_line, :new_line, :offset
def self.for_lines(lines)
changed_line_pairs = self.find_changed_line_pairs(lines)
inline_diffs = []
changed_line_pairs.each do |old_index, new_index|
old_line = lines[old_index]
new_line = lines[new_index]
old_diffs, new_diffs = new(old_line, new_line, offset: 1).inline_diffs
inline_diffs[old_index] = old_diffs
inline_diffs[new_index] = new_diffs
end
inline_diffs
end
def initialize(old_line, new_line, offset: 0)
@old_line = old_line[offset..-1]
@new_line = new_line[offset..-1]
......@@ -63,32 +45,54 @@ module Gitlab
[old_diffs, new_diffs]
end
private
class << self
def for_lines(lines)
changed_line_pairs = find_changed_line_pairs(lines)
# Finds pairs of old/new line pairs that represent the same line that changed
def self.find_changed_line_pairs(lines)
# Prefixes of all diff lines, indicating their types
# For example: `" - + -+ ---+++ --+ -++"`
line_prefixes = lines.each_with_object("") { |line, s| s << line[0] }.gsub(/[^ +-]/, ' ')
inline_diffs = []
changed_line_pairs = []
line_prefixes.scan(LINE_PAIRS_PATTERN) do
# For `"---+++"`, `begin_index == 0`, `end_index == 6`
begin_index, end_index = Regexp.last_match.offset(:del_ins)
changed_line_pairs.each do |old_index, new_index|
old_line = lines[old_index]
new_line = lines[new_index]
# For `"---+++"`, `changed_line_count == 3`
changed_line_count = (end_index - begin_index) / 2
old_diffs, new_diffs = new(old_line, new_line, offset: 1).inline_diffs
halfway_index = begin_index + changed_line_count
(begin_index...halfway_index).each do |i|
# For `"---+++"`, index 1 maps to 1 + 3 = 4
changed_line_pairs << [i, i + changed_line_count]
inline_diffs[old_index] = old_diffs
inline_diffs[new_index] = new_diffs
end
inline_diffs
end
changed_line_pairs
private
# Finds pairs of old/new line pairs that represent the same line that changed
def find_changed_line_pairs(lines)
# Prefixes of all diff lines, indicating their types
# For example: `" - + -+ ---+++ --+ -++"`
line_prefixes = lines.each_with_object("") { |line, s| s << line[0] }.gsub(/[^ +-]/, ' ')
changed_line_pairs = []
line_prefixes.scan(LINE_PAIRS_PATTERN) do
# For `"---+++"`, `begin_index == 0`, `end_index == 6`
begin_index, end_index = Regexp.last_match.offset(:del_ins)
# For `"---+++"`, `changed_line_count == 3`
changed_line_count = (end_index - begin_index) / 2
halfway_index = begin_index + changed_line_count
(begin_index...halfway_index).each do |i|
# For `"---+++"`, index 1 maps to 1 + 3 = 4
changed_line_pairs << [i, i + changed_line_count]
end
end
changed_line_pairs
end
end
private
def longest_common_prefix(a, b)
max_length = [a.length, b.length].max
......
lib/gitlab/metrics.rb
......@@ -141,10 +141,10 @@ module Gitlab
end
end
private
def self.current_transaction
Transaction.current
end
private_class_method :current_transaction
end
end
lib/gitlab/themes.rb
......@@ -2,6 +2,8 @@ module Gitlab
# Module containing GitLab's application theme definitions and helper methods
# for accessing them.
module Themes
extend self
# Theme ID used when no `default_theme` configuration setting is provided.
APPLICATION_DEFAULT = 2
......@@ -22,7 +24,7 @@ module Gitlab
# classes that might be applied to the `body` element
#
# Returns a String
def self.body_classes
def body_classes
THEMES.collect(&:css_class).uniq.join(' ')
end
......@@ -33,26 +35,26 @@ module Gitlab
# id - Integer ID
#
# Returns a Theme
def self.by_id(id)
def by_id(id)
THEMES.detect { |t| t.id == id } || default
end
# Returns the number of defined Themes
def self.count
def count
THEMES.size
end
# Get the default Theme
#
# Returns a Theme
def self.default
def default
by_id(default_id)
end
# Iterate through each Theme
#
# Yields the Theme object
def self.each(&block)
def each(&block)
THEMES.each(&block)
end
......@@ -61,7 +63,7 @@ module Gitlab
# user - User record
#
# Returns a Theme
def self.for_user(user)
def for_user(user)
if user
by_id(user.theme_id)
else
......@@ -71,7 +73,7 @@ module Gitlab
private
def self.default_id
def default_id
id = Gitlab.config.gitlab.default_theme.to_i
# Prevent an invalid configuration setting from causing an infinite loop
......
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