diff options
author | Shinya Maeda <shinya@gitlab.com> | 2018-05-30 14:50:09 +0900 |
---|---|---|
committer | Shinya Maeda <shinya@gitlab.com> | 2018-05-30 14:50:09 +0900 |
commit | 09122f93c34b15cb827aabdbdf35fc33b08f93af (patch) | |
tree | 57c137ef57621a7a2ed4940c56c7f5cbe6ec1c80 /app/models | |
parent | 1d20679e9c8b1ba16bebaf982255946e7207b4d4 (diff) | |
parent | 5b1416aa74c4fa80e0c324fd2907166af5ca479b (diff) | |
download | gitlab-ce-09122f93c34b15cb827aabdbdf35fc33b08f93af.tar.gz |
Merge branch 'master' into per-project-pipeline-iid
Diffstat (limited to 'app/models')
-rw-r--r-- | app/models/application_setting.rb | 39 | ||||
-rw-r--r-- | app/models/concerns/batch_destroy_dependent_associations.rb | 28 | ||||
-rw-r--r-- | app/models/concerns/diff_file.rb | 9 | ||||
-rw-r--r-- | app/models/diff_note.rb | 67 | ||||
-rw-r--r-- | app/models/event.rb | 2 | ||||
-rw-r--r-- | app/models/internal_id.rb | 24 | ||||
-rw-r--r-- | app/models/merge_request_diff_file.rb | 7 | ||||
-rw-r--r-- | app/models/note.rb | 4 | ||||
-rw-r--r-- | app/models/note_diff_file.rb | 7 | ||||
-rw-r--r-- | app/models/project.rb | 1 |
10 files changed, 112 insertions, 76 deletions
diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb index e8ccb320fae..b12f7a2c83f 100644 --- a/app/models/application_setting.rb +++ b/app/models/application_setting.rb @@ -230,6 +230,7 @@ class ApplicationSetting < ActiveRecord::Base after_commit do reset_memoized_terms end + after_commit :expire_performance_bar_allowed_user_ids_cache, if: -> { previous_changes.key?('performance_bar_allowed_group_id') } def self.defaults { @@ -386,31 +387,6 @@ class ApplicationSetting < ActiveRecord::Base super(levels.map { |level| Gitlab::VisibilityLevel.level_value(level) }) end - def performance_bar_allowed_group_id=(group_full_path) - group_full_path = nil if group_full_path.blank? - - if group_full_path.nil? - if group_full_path != performance_bar_allowed_group_id - super(group_full_path) - Gitlab::PerformanceBar.expire_allowed_user_ids_cache - end - - return - end - - group = Group.find_by_full_path(group_full_path) - - if group - if group.id != performance_bar_allowed_group_id - super(group.id) - Gitlab::PerformanceBar.expire_allowed_user_ids_cache - end - else - super(nil) - Gitlab::PerformanceBar.expire_allowed_user_ids_cache - end - end - def performance_bar_allowed_group Group.find_by_id(performance_bar_allowed_group_id) end @@ -420,15 +396,6 @@ class ApplicationSetting < ActiveRecord::Base performance_bar_allowed_group_id.present? end - # - If `enable` is true, we early return since the actual attribute that holds - # the enabling/disabling is `performance_bar_allowed_group_id` - # - If `enable` is false, we set `performance_bar_allowed_group_id` to `nil` - def performance_bar_enabled=(enable) - return if Gitlab::Utils.to_boolean(enable) - - self.performance_bar_allowed_group_id = nil - end - # Choose one of the available repository storage options. Currently all have # equal weighting. def pick_repository_storage @@ -506,4 +473,8 @@ class ApplicationSetting < ActiveRecord::Base errors.add(:terms, "You need to set terms to be enforced") unless terms.present? end + + def expire_performance_bar_allowed_user_ids_cache + Gitlab::PerformanceBar.expire_allowed_user_ids_cache + end end diff --git a/app/models/concerns/batch_destroy_dependent_associations.rb b/app/models/concerns/batch_destroy_dependent_associations.rb new file mode 100644 index 00000000000..353ee2e73d0 --- /dev/null +++ b/app/models/concerns/batch_destroy_dependent_associations.rb @@ -0,0 +1,28 @@ +# Provides a way to work around Rails issue where dependent objects are all +# loaded into memory before destroyed: https://github.com/rails/rails/issues/22510. +# +# This concern allows an ActiveRecord module to destroy all its dependent +# associations in batches. The idea is borrowed from https://github.com/thisismydesign/batch_dependent_associations. +# +# The differences here with that gem: +# +# 1. We allow excluding certain associations. +# 2. We don't need to support delete_all since we can use the EachBatch concern. +module BatchDestroyDependentAssociations + extend ActiveSupport::Concern + + DEPENDENT_ASSOCIATIONS_BATCH_SIZE = 1000 + + def dependent_associations_to_destroy + self.class.reflect_on_all_associations(:has_many).select { |assoc| assoc.options[:dependent] == :destroy } + end + + def destroy_dependent_associations_in_batches(exclude: []) + dependent_associations_to_destroy.each do |association| + next if exclude.include?(association.name) + + # rubocop:disable GitlabSecurity/PublicSend + public_send(association.name).find_each(batch_size: DEPENDENT_ASSOCIATIONS_BATCH_SIZE, &:destroy) + end + end +end diff --git a/app/models/concerns/diff_file.rb b/app/models/concerns/diff_file.rb new file mode 100644 index 00000000000..72332072012 --- /dev/null +++ b/app/models/concerns/diff_file.rb @@ -0,0 +1,9 @@ +module DiffFile + extend ActiveSupport::Concern + + def to_hash + keys = Gitlab::Git::Diff::SERIALIZE_KEYS - [:diff] + + as_json(only: keys).merge(diff: diff).with_indifferent_access + end +end diff --git a/app/models/diff_note.rb b/app/models/diff_note.rb index 616a626419b..d752d5bcdee 100644 --- a/app/models/diff_note.rb +++ b/app/models/diff_note.rb @@ -3,6 +3,7 @@ # A note of this type can be resolvable. class DiffNote < Note include NoteOnDiff + include Gitlab::Utils::StrongMemoize NOTEABLE_TYPES = %w(MergeRequest Commit).freeze @@ -12,7 +13,6 @@ class DiffNote < Note validates :original_position, presence: true validates :position, presence: true - validates :diff_line, presence: true, if: :on_text? validates :line_code, presence: true, line_code: true, if: :on_text? validates :noteable_type, inclusion: { in: NOTEABLE_TYPES } validate :positions_complete @@ -23,6 +23,7 @@ class DiffNote < Note before_validation :update_position, on: :create, if: :on_text? before_validation :set_line_code, if: :on_text? after_save :keep_around_commits + after_commit :create_diff_file, on: :create def discussion_class(*) DiffDiscussion @@ -53,21 +54,25 @@ class DiffNote < Note position.position_type == "image" end + def create_diff_file + return unless should_create_diff_file? + + diff_file = fetch_diff_file + diff_line = diff_file.line_for_position(self.original_position) + + creation_params = diff_file.diff.to_hash + .except(:too_large) + .merge(diff: diff_file.diff_hunk(diff_line)) + + create_note_diff_file(creation_params) + end + def diff_file - @diff_file ||= - begin - if created_at_diff?(noteable.diff_refs) - # We're able to use the already persisted diffs (Postgres) if we're - # presenting a "current version" of the MR discussion diff. - # So no need to make an extra Gitaly diff request for it. - # As an extra benefit, the returned `diff_file` already - # has `highlighted_diff_lines` data set from Redis on - # `Diff::FileCollection::MergeRequestDiff`. - noteable.diffs(paths: original_position.paths, expanded: true).diff_files.first - else - original_position.diff_file(self.project.repository) - end - end + strong_memoize(:diff_file) do + enqueue_diff_file_creation_job if should_create_diff_file? + + fetch_diff_file + end end def diff_line @@ -98,6 +103,38 @@ class DiffNote < Note private + def enqueue_diff_file_creation_job + # Avoid enqueuing multiple file creation jobs at once for a note (i.e. + # parallel calls to `DiffNote#diff_file`). + lease = Gitlab::ExclusiveLease.new("note_diff_file_creation:#{id}", timeout: 1.hour.to_i) + return unless lease.try_obtain + + CreateNoteDiffFileWorker.perform_async(id) + end + + def should_create_diff_file? + on_text? && note_diff_file.nil? && self == discussion.first_note + end + + def fetch_diff_file + if note_diff_file + diff = Gitlab::Git::Diff.new(note_diff_file.to_hash) + Gitlab::Diff::File.new(diff, + repository: project.repository, + diff_refs: original_position.diff_refs) + elsif created_at_diff?(noteable.diff_refs) + # We're able to use the already persisted diffs (Postgres) if we're + # presenting a "current version" of the MR discussion diff. + # So no need to make an extra Gitaly diff request for it. + # As an extra benefit, the returned `diff_file` already + # has `highlighted_diff_lines` data set from Redis on + # `Diff::FileCollection::MergeRequestDiff`. + noteable.diffs(paths: original_position.paths, expanded: true).diff_files.first + else + original_position.diff_file(self.project.repository) + end + end + def supported? for_commit? || self.noteable.has_complete_diff_refs? end diff --git a/app/models/event.rb b/app/models/event.rb index 741a84194e2..ac0b1c7b27c 100644 --- a/app/models/event.rb +++ b/app/models/event.rb @@ -40,6 +40,7 @@ class Event < ActiveRecord::Base ).freeze RESET_PROJECT_ACTIVITY_INTERVAL = 1.hour + REPOSITORY_UPDATED_AT_INTERVAL = 5.minutes delegate :name, :email, :public_email, :username, to: :author, prefix: true, allow_nil: true delegate :title, to: :issue, prefix: true, allow_nil: true @@ -391,6 +392,7 @@ class Event < ActiveRecord::Base def set_last_repository_updated_at Project.unscoped.where(id: project_id) + .where("last_repository_updated_at < ? OR last_repository_updated_at IS NULL", REPOSITORY_UPDATED_AT_INTERVAL.ago) .update_all(last_repository_updated_at: created_at) end diff --git a/app/models/internal_id.rb b/app/models/internal_id.rb index dbd82dda06e..f50f28deffe 100644 --- a/app/models/internal_id.rb +++ b/app/models/internal_id.rb @@ -24,12 +24,9 @@ class InternalId < ActiveRecord::Base # # The operation locks the record and gathers a `ROW SHARE` lock (in PostgreSQL). # As such, the increment is atomic and safe to be called concurrently. - # - # If a `maximum_iid` is passed in, this overrides the incremented value if it's - # greater than that. This can be used to correct the increment value if necessary. - def increment_and_save!(maximum_iid) + def increment_and_save! lock! - self.last_value = [(last_value || 0) + 1, (maximum_iid || 0) + 1].max + self.last_value = (last_value || 0) + 1 save! last_value end @@ -93,16 +90,7 @@ class InternalId < ActiveRecord::Base # and increment its last value # # Note this will acquire a ROW SHARE lock on the InternalId record - - # Note we always calculate the maximum iid present here and - # pass it in to correct the InternalId entry if it's last_value is off. - # - # This can happen in a transition phase where both `AtomicInternalId` and - # `NonatomicInternalId` code runs (e.g. during a deploy). - # - # This is subject to be cleaned up with the 10.8 release: - # https://gitlab.com/gitlab-org/gitlab-ce/issues/45389. - (lookup || create_record).increment_and_save!(maximum_iid) + (lookup || create_record).increment_and_save! end end @@ -128,15 +116,11 @@ class InternalId < ActiveRecord::Base InternalId.create!( **scope, usage: usage_value, - last_value: maximum_iid + last_value: init.call(subject) || 0 ) end rescue ActiveRecord::RecordNotUnique lookup end - - def maximum_iid - @maximum_iid ||= init.call(subject) || 0 - end end end diff --git a/app/models/merge_request_diff_file.rb b/app/models/merge_request_diff_file.rb index 1199ff5af22..cd8ba6b904d 100644 --- a/app/models/merge_request_diff_file.rb +++ b/app/models/merge_request_diff_file.rb @@ -1,5 +1,6 @@ class MergeRequestDiffFile < ActiveRecord::Base include Gitlab::EncodingHelper + include DiffFile belongs_to :merge_request_diff @@ -12,10 +13,4 @@ class MergeRequestDiffFile < ActiveRecord::Base def diff binary? ? super.unpack('m0').first : super end - - def to_hash - keys = Gitlab::Git::Diff::SERIALIZE_KEYS - [:diff] - - as_json(only: keys).merge(diff: diff).with_indifferent_access - end end diff --git a/app/models/note.rb b/app/models/note.rb index 109405d3f17..02f7a9b1e4f 100644 --- a/app/models/note.rb +++ b/app/models/note.rb @@ -63,6 +63,7 @@ class Note < ActiveRecord::Base has_many :todos has_many :events, as: :target, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent has_one :system_note_metadata + has_one :note_diff_file, inverse_of: :diff_note, foreign_key: :diff_note_id delegate :gfm_reference, :local_reference, to: :noteable delegate :name, to: :project, prefix: true @@ -100,7 +101,8 @@ class Note < ActiveRecord::Base scope :inc_author_project, -> { includes(:project, :author) } scope :inc_author, -> { includes(:author) } scope :inc_relations_for_view, -> do - includes(:project, :author, :updated_by, :resolved_by, :award_emoji, :system_note_metadata) + includes(:project, :author, :updated_by, :resolved_by, :award_emoji, + :system_note_metadata, :note_diff_file) end scope :diff_notes, -> { where(type: %w(LegacyDiffNote DiffNote)) } diff --git a/app/models/note_diff_file.rb b/app/models/note_diff_file.rb new file mode 100644 index 00000000000..e688018a6d9 --- /dev/null +++ b/app/models/note_diff_file.rb @@ -0,0 +1,7 @@ +class NoteDiffFile < ActiveRecord::Base + include DiffFile + + belongs_to :diff_note, inverse_of: :note_diff_file + + validates :diff_note, presence: true +end diff --git a/app/models/project.rb b/app/models/project.rb index 0fe9f8880b4..e275ac4dc6f 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -24,6 +24,7 @@ class Project < ActiveRecord::Base include ChronicDurationAttribute include FastDestroyAll::Helpers include WithUploads + include BatchDestroyDependentAssociations extend Gitlab::ConfigHelper |