diff options
46 files changed, 1061 insertions, 489 deletions
diff --git a/app/models/concerns/counter_attribute.rb b/app/models/concerns/counter_attribute.rb index 4dfaae674bf..f1efbba67e1 100644 --- a/app/models/concerns/counter_attribute.rb +++ b/app/models/concerns/counter_attribute.rb @@ -30,13 +30,16 @@ # end # # To increment the counter we can use the method: -# delayed_increment_counter(:commit_count, 3) +# increment_counter(:commit_count, 3) +# +# This method would determine whether it would increment the counter using Redis, +# or fallback to legacy increment on ActiveRecord counters. # # It is possible to register callbacks to be executed after increments have # been flushed to the database. Callbacks are not executed if there are no increments # to flush. # -# counter_attribute_after_flush do |statistic| +# counter_attribute_after_commit do |statistic| # Namespaces::ScheduleAggregationWorker.perform_async(statistic.namespace_id) # end # @@ -44,21 +47,7 @@ module CounterAttribute extend ActiveSupport::Concern extend AfterCommitQueue include Gitlab::ExclusiveLeaseHelpers - - LUA_STEAL_INCREMENT_SCRIPT = <<~EOS - local increment_key, flushed_key = KEYS[1], KEYS[2] - local increment_value = redis.call("get", increment_key) or 0 - local flushed_value = redis.call("incrby", flushed_key, increment_value) - if flushed_value == 0 then - redis.call("del", increment_key, flushed_key) - else - redis.call("del", increment_key) - end - return flushed_value - EOS - - WORKER_DELAY = 10.minutes - WORKER_LOCK_TTL = 10.minutes + include Gitlab::Utils::StrongMemoize class_methods do def counter_attribute(attribute, if: nil) @@ -72,13 +61,13 @@ module CounterAttribute @counter_attributes ||= [] end - def after_flush_callbacks - @after_flush_callbacks ||= [] + def after_commit_callbacks + @after_commit_callbacks ||= [] end - # perform registered callbacks after increments have been flushed to the database - def counter_attribute_after_flush(&callback) - after_flush_callbacks << callback + # perform registered callbacks after increments have been committed to the database + def counter_attribute_after_commit(&callback) + after_commit_callbacks << callback end end @@ -90,60 +79,19 @@ module CounterAttribute counter_attribute[:if_proc].call(self) end - # This method must only be called by FlushCounterIncrementsWorker - # because it should run asynchronously and with exclusive lease. - # This will - # 1. temporarily move the pending increment for a given attribute - # to a relative "flushed" Redis key, delete the increment key and return - # the value. If new increments are performed at this point, the increment - # key is recreated as part of `delayed_increment_counter`. - # The "flushed" key is used to ensure that we can keep incrementing - # counters in Redis while flushing existing values. - # 2. then the value is used to update the counter in the database. - # 3. finally the "flushed" key is deleted. - def flush_increments_to_database!(attribute) - lock_key = counter_lock_key(attribute) - - with_exclusive_lease(lock_key) do - previous_db_value = read_attribute(attribute) - increment_key = counter_key(attribute) - flushed_key = counter_flushed_key(attribute) - increment_value = steal_increments(increment_key, flushed_key) - new_db_value = nil - - next if increment_value == 0 - - transaction do - update_counters_with_lease({ attribute => increment_value }) - redis_state { |redis| redis.del(flushed_key) } - new_db_value = reset.read_attribute(attribute) - end - - execute_after_flush_callbacks - - log_flush_counter(attribute, increment_value, previous_db_value, new_db_value) + def counter(attribute) + strong_memoize_with(:counter, attribute) do + # This needs #to_sym because attribute could come from a Sidekiq param, + # which would be a string. + build_counter_for(attribute.to_sym) end end - def delayed_increment_counter(attribute, increment) - raise ArgumentError, "#{attribute} is not a counter attribute" unless counter_attribute_enabled?(attribute) - + def increment_counter(attribute, increment) return if increment == 0 run_after_commit_or_now do - increment_counter(attribute, increment) - - FlushCounterIncrementsWorker.perform_in(WORKER_DELAY, self.class.name, self.id, attribute) - end - - true - end - - def increment_counter(attribute, increment) - if counter_attribute_enabled?(attribute) - new_value = redis_state do |redis| - redis.incrby(counter_key(attribute), increment) - end + new_value = counter(attribute).increment(increment) log_increment_counter(attribute, increment, new_value) end @@ -156,70 +104,33 @@ module CounterAttribute end def reset_counter!(attribute) - if counter_attribute_enabled?(attribute) - detect_race_on_record(log_fields: { caller: __method__, attributes: attribute }) do - update!(attribute => 0) - clear_counter!(attribute) - end - - log_clear_counter(attribute) + detect_race_on_record(log_fields: { caller: __method__, attributes: attribute }) do + counter(attribute).reset! end - end - - def get_counter_value(attribute) - if counter_attribute_enabled?(attribute) - redis_state do |redis| - redis.get(counter_key(attribute)).to_i - end - end - end - - def counter_key(attribute) - "project:{#{project_id}}:counters:#{self.class}:#{id}:#{attribute}" - end - def counter_flushed_key(attribute) - counter_key(attribute) + ':flushed' + log_clear_counter(attribute) end - def counter_lock_key(attribute) - counter_key(attribute) + ':lock' + def execute_after_commit_callbacks + self.class.after_commit_callbacks.each do |callback| + callback.call(self.reset) + end end private - def database_lock_key - "project:{#{project_id}}:#{self.class}:#{id}" - end - - def steal_increments(increment_key, flushed_key) - redis_state do |redis| - redis.eval(LUA_STEAL_INCREMENT_SCRIPT, keys: [increment_key, flushed_key]) - end - end - - def clear_counter!(attribute) - redis_state do |redis| - redis.del(counter_key(attribute)) - end - end + def build_counter_for(attribute) + raise ArgumentError, %(attribute "#{attribute}" does not exist) unless has_attribute?(attribute) - def execute_after_flush_callbacks - self.class.after_flush_callbacks.each do |callback| - callback.call(self) + if counter_attribute_enabled?(attribute) + Gitlab::Counters::BufferedCounter.new(self, attribute) + else + Gitlab::Counters::LegacyCounter.new(self, attribute) end end - def redis_state(&block) - Gitlab::Redis::SharedState.with(&block) - end - - def with_exclusive_lease(lock_key) - in_lock(lock_key, ttl: WORKER_LOCK_TTL) do - yield - end - rescue Gitlab::ExclusiveLeaseHelpers::FailedToObtainLockError - # a worker is already updating the counters + def database_lock_key + "project:{#{project_id}}:#{self.class}:#{id}" end # detect_race_on_record uses a lease to monitor access @@ -273,19 +184,6 @@ module CounterAttribute Gitlab::AppLogger.info(payload) end - def log_flush_counter(attribute, increment, previous_db_value, new_db_value) - payload = Gitlab::ApplicationContext.current.merge( - message: 'Flush counter attribute to database', - attribute: attribute, - project_id: project_id, - increment: increment, - previous_db_value: previous_db_value, - new_db_value: new_db_value - ) - - Gitlab::AppLogger.info(payload) - end - def log_clear_counter(attribute) payload = Gitlab::ApplicationContext.current.merge( message: 'Clear counter attribute', diff --git a/app/models/project_statistics.rb b/app/models/project_statistics.rb index a56738e1cee..506f6305791 100644 --- a/app/models/project_statistics.rb +++ b/app/models/project_statistics.rb @@ -13,19 +13,19 @@ class ProjectStatistics < ApplicationRecord counter_attribute :build_artifacts_size counter_attribute :packages_size - counter_attribute_after_flush do |project_statistic| - project_statistic.refresh_storage_size! + counter_attribute_after_commit do |project_statistics| + project_statistics.refresh_storage_size! - Namespaces::ScheduleAggregationWorker.perform_async(project_statistic.namespace_id) + Namespaces::ScheduleAggregationWorker.perform_async(project_statistics.namespace_id) end before_save :update_storage_size COLUMNS_TO_REFRESH = [:repository_size, :wiki_size, :lfs_objects_size, :commit_count, :snippets_size, :uploads_size, :container_registry_size].freeze - INCREMENTABLE_COLUMNS = { - pipeline_artifacts_size: %i[storage_size], - snippets_size: %i[storage_size] - }.freeze + INCREMENTABLE_COLUMNS = [ + :pipeline_artifacts_size, + :snippets_size + ].freeze NAMESPACE_RELATABLE_COLUMNS = [:repository_size, :wiki_size, :lfs_objects_size, :uploads_size, :container_registry_size].freeze STORAGE_SIZE_COMPONENTS = [ :repository_size, @@ -120,7 +120,7 @@ class ProjectStatistics < ApplicationRecord # we have to update the storage_size separately. # # For counter attributes, storage_size will be refreshed after the counter is flushed, - # through counter_attribute_after_flush + # through counter_attribute_after_commit # # For non-counter attributes, storage_size is updated depending on key => [columns] in INCREMENTABLE_COLUMNS def self.increment_statistic(project, key, amount) @@ -131,26 +131,14 @@ class ProjectStatistics < ApplicationRecord def increment_statistic(key, amount) raise ArgumentError, "Cannot increment attribute: #{key}" unless incrementable_attribute?(key) - return if amount == 0 - - if counter_attribute_enabled?(key) - delayed_increment_counter(key, amount) - else - legacy_increment_statistic(key, amount) - end - end - def legacy_increment_statistic(key, amount) - increment_columns!(key, amount) - - Namespaces::ScheduleAggregationWorker.perform_async( # rubocop: disable CodeReuse/Worker - project.namespace_id) + increment_counter(key, amount) end private def incrementable_attribute?(key) - INCREMENTABLE_COLUMNS.key?(key) || counter_attribute_enabled?(key) + INCREMENTABLE_COLUMNS.include?(key) || counter_attribute_enabled?(key) end def storage_size_components @@ -161,16 +149,6 @@ class ProjectStatistics < ApplicationRecord storage_size_components.map { |component| "COALESCE (#{component}, 0)" }.join(' + ').freeze end - def increment_columns!(key, amount) - increments = { key => amount } - additional = INCREMENTABLE_COLUMNS.fetch(key, []) - additional.each do |column| - increments[column] = amount - end - - update_counters_with_lease(increments) - end - def schedule_namespace_aggregation_worker run_after_commit do Namespaces::ScheduleAggregationWorker.perform_async(project.namespace_id) diff --git a/app/policies/issue_policy.rb b/app/policies/issue_policy.rb index 2bc535adf41..491eebe9daf 100644 --- a/app/policies/issue_policy.rb +++ b/app/policies/issue_policy.rb @@ -27,6 +27,23 @@ class IssuePolicy < IssuablePolicy desc "Issue is persisted" condition(:persisted, scope: :subject) { @subject.persisted? } + # accessing notes requires the notes widget to be available for work items(or issue) + condition(:notes_widget_enabled, scope: :subject) do + @subject.work_item_type.widgets.include?(::WorkItems::Widgets::Notes) + end + + rule { ~notes_widget_enabled }.policy do + prevent :create_note + prevent :read_note + prevent :read_internal_note + prevent :set_note_created_at + prevent :mark_note_as_confidential + # these actions on notes are not available on issues/work items yet, + # but preventing any action on work item notes as long as there is no notes widget seems reasonable + prevent :resolve_note + prevent :reposition_note + end + rule { confidential & ~can_read_confidential }.policy do prevent(*create_read_update_admin_destroy(:issue)) prevent :read_issue_iid diff --git a/app/policies/note_policy.rb b/app/policies/note_policy.rb index 67b57595beb..9fd95bbe42d 100644 --- a/app/policies/note_policy.rb +++ b/app/policies/note_policy.rb @@ -20,12 +20,20 @@ class NotePolicy < BasePolicy condition(:confidential, scope: :subject) { @subject.confidential? } + # if noteable is a work item it needs to check the notes widget availability + condition(:notes_widget_enabled, scope: :subject) do + !@subject.noteable.respond_to?(:work_item_type) || + @subject.noteable.work_item_type.widgets.include?(::WorkItems::Widgets::Notes) + end + # Should be matched with IssuablePolicy#read_internal_note # and EpicPolicy#read_internal_note condition(:can_read_confidential) do access_level >= Gitlab::Access::REPORTER || admin? end + rule { ~notes_widget_enabled }.prevent_all + rule { ~editable }.prevent :admin_note # If user can't read the issue/MR/etc then they should not be allowed to do anything to their own notes diff --git a/app/services/issuable/discussions_list_service.rb b/app/services/issuable/discussions_list_service.rb index d11ba6c6adf..1e5e37e4e1b 100644 --- a/app/services/issuable/discussions_list_service.rb +++ b/app/services/issuable/discussions_list_service.rb @@ -16,7 +16,7 @@ module Issuable end def execute - return Note.none unless can_read_issuable? + return Note.none unless can_read_issuable_notes? notes = NotesFinder.new(current_user, params.merge({ target: issuable, project: issuable.project })) .execute.with_web_entity_associations.inc_relations_for_view.fresh @@ -58,10 +58,11 @@ module Issuable end end - def can_read_issuable? + def can_read_issuable_notes? return Ability.allowed?(current_user, :read_security_resource, issuable) if issuable.is_a?(Vulnerability) - Ability.allowed?(current_user, :"read_#{issuable.to_ability_name}", issuable) + Ability.allowed?(current_user, :"read_#{issuable.to_ability_name}", issuable) && + Ability.allowed?(current_user, :read_note, issuable) end end end diff --git a/app/services/projects/refresh_build_artifacts_size_statistics_service.rb b/app/services/projects/refresh_build_artifacts_size_statistics_service.rb index 1f86e5f4ba9..8e006dc8c34 100644 --- a/app/services/projects/refresh_build_artifacts_size_statistics_service.rb +++ b/app/services/projects/refresh_build_artifacts_size_statistics_service.rb @@ -18,7 +18,7 @@ module Projects # Mark the refresh ready for another worker to pick up and process the next batch refresh.requeue!(batch.last.id) - refresh.project.statistics.delayed_increment_counter(:build_artifacts_size, total_artifacts_size) + refresh.project.statistics.increment_counter(:build_artifacts_size, total_artifacts_size) end else # Remove the refresh job from the table if there are no more diff --git a/app/views/projects/_merge_request_merge_checks_settings.html.haml b/app/views/projects/_merge_request_merge_checks_settings.html.haml index 8c12399fdbb..bb7a7731067 100644 --- a/app/views/projects/_merge_request_merge_checks_settings.html.haml +++ b/app/views/projects/_merge_request_merge_checks_settings.html.haml @@ -3,17 +3,6 @@ .form-group %b= s_('ProjectSettings|Merge checks') %p.text-secondary= s_('ProjectSettings|These checks must pass before merge requests can be merged.') - .builds-feature - = form.gitlab_ui_checkbox_component :only_allow_merge_if_pipeline_succeeds, - s_('ProjectSettings|Pipelines must succeed'), - help_text: s_("ProjectSettings|Merge requests can't be merged if the latest pipeline did not succeed or is still running.") - .gl-pl-6 - = form.gitlab_ui_checkbox_component :allow_merge_on_skipped_pipeline, - s_('ProjectSettings|Skipped pipelines are considered successful'), - help_text: s_('ProjectSettings|Introduces the risk of merging changes that do not pass the pipeline.'), - checkbox_options: { class: 'gl-pl-6' } + = render 'projects/merge_request_pipelines_and_threads_options', form: form, project: @project = render_if_exists 'projects/merge_request_merge_checks_status_checks', form: form, project: @project - = form.gitlab_ui_checkbox_component :only_allow_merge_if_all_discussions_are_resolved, - s_('ProjectSettings|All threads must be resolved'), - checkbox_options: { data: { qa_selector: 'allow_merge_if_all_discussions_are_resolved_checkbox' } } = render_if_exists 'projects/merge_request_merge_checks_jira_enforcement', form: form, project: @project diff --git a/app/views/projects/_merge_request_pipelines_and_threads_options.html.haml b/app/views/projects/_merge_request_pipelines_and_threads_options.html.haml new file mode 100644 index 00000000000..94f8d3cc4a3 --- /dev/null +++ b/app/views/projects/_merge_request_pipelines_and_threads_options.html.haml @@ -0,0 +1,13 @@ +- form = local_assigns.fetch(:form) + += form.gitlab_ui_checkbox_component :only_allow_merge_if_pipeline_succeeds, + s_('ProjectSettings|Pipelines must succeed'), + help_text: s_("ProjectSettings|Merge requests can't be merged if the latest pipeline did not succeed or is still running.") +.gl-pl-6 + = form.gitlab_ui_checkbox_component :allow_merge_on_skipped_pipeline, + s_('ProjectSettings|Skipped pipelines are considered successful'), + help_text: s_('ProjectSettings|Introduces the risk of merging changes that do not pass the pipeline.'), + checkbox_options: { class: 'gl-pl-6' } += form.gitlab_ui_checkbox_component :only_allow_merge_if_all_discussions_are_resolved, + s_('ProjectSettings|All threads must be resolved'), + checkbox_options: { data: { qa_selector: 'allow_merge_if_all_discussions_are_resolved_checkbox' } } diff --git a/app/views/projects/merge_requests/show.html.haml b/app/views/projects/merge_requests/show.html.haml index e42501ea278..e84b0232353 100644 --- a/app/views/projects/merge_requests/show.html.haml +++ b/app/views/projects/merge_requests/show.html.haml @@ -48,7 +48,7 @@ = _("Changes") = gl_badge_tag @diffs_count, { size: :sm } .d-flex.flex-wrap.align-items-center.justify-content-lg-end - #js-vue-discussion-counter{ data: { blocks_merge: @project.only_allow_merge_if_all_discussions_are_resolved?.to_s } } + #js-vue-discussion-counter{ data: { blocks_merge: @project.only_allow_merge_if_all_discussions_are_resolved?(inherit_group_setting: true).to_s } } - if moved_mr_sidebar_enabled? - if !!@issuable_sidebar.dig(:current_user, :id) .js-sidebar-todo-widget-root{ data: { project_path: @issuable_sidebar[:project_full_path], iid: @issuable_sidebar[:iid], id: @issuable_sidebar[:id] } } diff --git a/app/workers/flush_counter_increments_worker.rb b/app/workers/flush_counter_increments_worker.rb index 8c7e17587d0..e92e1a9b7b5 100644 --- a/app/workers/flush_counter_increments_worker.rb +++ b/app/workers/flush_counter_increments_worker.rb @@ -29,6 +29,6 @@ class FlushCounterIncrementsWorker model = model_class.find_by_id(model_id) return unless model - model.flush_increments_to_database!(attribute) + Gitlab::Counters::BufferedCounter.new(model, attribute).commit_increment! end end diff --git a/data/deprecations/15-7-deprecate-gitlab-runner-exec-cmd.yml b/data/deprecations/15-7-deprecate-gitlab-runner-exec-cmd.yml new file mode 100644 index 00000000000..d535e58fc36 --- /dev/null +++ b/data/deprecations/15-7-deprecate-gitlab-runner-exec-cmd.yml @@ -0,0 +1,21 @@ +- title: "The `gitlab-runner exec` command is deprecated" # (required) The name of the feature to be deprecated + announcement_milestone: "15.7" # (required) The milestone when this feature was first announced as deprecated. + announcement_date: "2022-12-22" # (required) The date of the milestone release when this feature was first announced as deprecated. This should almost always be the 22nd of a month (YYYY-MM-22), unless you did an out of band blog post. + removal_milestone: "16.0" # (required) The milestone when this feature is planned to be removed + removal_date: "2023-05-22" # (required) The date of the milestone release when this feature is planned to be removed. This should almost always be the 22nd of a month (YYYY-MM-22), unless you did an out of band blog post. + breaking_change: true # (required) If this deprecation is a breaking change, set this value to true + reporter: DarrenEastman # (required) GitLab username of the person reporting the deprecation + stage: Verify # (required) String value of the stage that the feature was created in. e.g., Growth + issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/385235 # (required) Link to the deprecation issue in GitLab + body: | # (required) Do not modify this line, instead modify the lines below. + The [`gitlab-runner exec`](https://docs.gitlab.com/runner/commands/#gitlab-runner-exec) command is deprecated and will be fully removed from GitLab Runner in 16.0. The `gitlab-runner exec` feature was initially developed to provide the ability to validate a GitLab CI pipeline on a local system without needing to commit the updates to a GitLab instance. However, with the continued evolution of GitLab CI, replicating all GitLab CI features into `gitlab-runner exec` was no longer viable. Pipeline syntax and validation [simulation](https://docs.gitlab.com/ee/ci/pipeline_editor/#simulate-a-cicd-pipeline) are available in the GitLab pipeline editor. + end_of_support_milestone: "16.0" # (optional) Use "XX.YY" format. The milestone when support for this feature will end. + end_of_support_date: "2023-05-22" # (optional) The date of the milestone release when support for this feature will end. + + +# OTHER OPTIONAL FIELDS +# + tiers: # (optional - may be required in the future) An array of tiers that the feature is available in currently. e.g., [Free, Silver, Gold, Core, Premium, Ultimate] + documentation_url: https://docs.gitlab.com/runner/commands/#gitlab-runner-exec # (optional) This is a link to the current documentation page + image_url: # (optional) This is a link to a thumbnail image depicting the feature + video_url: # (optional) Use the youtube thumbnail URL with the structure of https://img.youtube.com/vi/UNIQUEID/hqdefault.jpg diff --git a/db/docs/ci_pipeline_metadata.yml b/db/docs/ci_pipeline_metadata.yml index d246253e64a..8a255916261 100644 --- a/db/docs/ci_pipeline_metadata.yml +++ b/db/docs/ci_pipeline_metadata.yml @@ -1,5 +1,5 @@ --- -table_name: ci_pipelines_metadata +table_name: ci_pipeline_metadata classes: - Ci::PipelineMetadata feature_categories: diff --git a/doc/ci/testing/code_quality.md b/doc/ci/testing/code_quality.md index d1ed28b79c0..6b736f5c487 100644 --- a/doc/ci/testing/code_quality.md +++ b/doc/ci/testing/code_quality.md @@ -75,7 +75,7 @@ See also the Code Climate list of [Supported Languages for Maintainability](http Changes to files in merge requests can cause Code Quality to fall if merged. In these cases, the merge request's diff view displays an indicator next to lines with new Code Quality violations. For example: - + ## Example configuration diff --git a/doc/ci/testing/img/code_quality_mr_diff_report_v14_2.png b/doc/ci/testing/img/code_quality_mr_diff_report_v14_2.png Binary files differdeleted file mode 100644 index c1e9aad24ac..00000000000 --- a/doc/ci/testing/img/code_quality_mr_diff_report_v14_2.png +++ /dev/null diff --git a/doc/ci/testing/img/code_quality_mr_diff_report_v15_7.png b/doc/ci/testing/img/code_quality_mr_diff_report_v15_7.png Binary files differnew file mode 100644 index 00000000000..b45124e0e5d --- /dev/null +++ b/doc/ci/testing/img/code_quality_mr_diff_report_v15_7.png diff --git a/doc/update/deprecations.md b/doc/update/deprecations.md index f117baa849c..1920e97bb15 100644 --- a/doc/update/deprecations.md +++ b/doc/update/deprecations.md @@ -205,6 +205,21 @@ The [Phabricator task importer](https://docs.gitlab.com/ee/user/project/import/p <div class="deprecation removal-160 breaking-change"> +### The `gitlab-runner exec` command is deprecated + +End of Support: GitLab <span class="removal-milestone">16.0</span> (2023-05-22)<br /> +Planned removal: GitLab <span class="removal-milestone">16.0</span> (2023-05-22) + +WARNING: +This is a [breaking change](https://docs.gitlab.com/ee/development/deprecation_guidelines/). +Review the details carefully before upgrading. + +The [`gitlab-runner exec`](https://docs.gitlab.com/runner/commands/#gitlab-runner-exec) command is deprecated and will be fully removed from GitLab Runner in 16.0. The `gitlab-runner exec` feature was initially developed to provide the ability to validate a GitLab CI pipeline on a local system without needing to commit the updates to a GitLab instance. However, with the continued evolution of GitLab CI, replicating all GitLab CI features into `gitlab-runner exec` was no longer viable. Pipeline syntax and validation [simulation](https://docs.gitlab.com/ee/ci/pipeline_editor/#simulate-a-cicd-pipeline) are available in the GitLab pipeline editor. + +</div> + +<div class="deprecation removal-160 breaking-change"> + ### ZenTao integration End of Support: GitLab <span class="removal-milestone">16.0</span> (2023-05-22)<br /> diff --git a/lib/api/helpers/notes_helpers.rb b/lib/api/helpers/notes_helpers.rb index 9c4128509c8..302dac4abf7 100644 --- a/lib/api/helpers/notes_helpers.rb +++ b/lib/api/helpers/notes_helpers.rb @@ -90,7 +90,12 @@ module API params = finder_params_by_noteable_type_and_id(noteable_type, noteable_id) noteable = NotesFinder.new(current_user, params).target - noteable = nil unless can?(current_user, noteable_read_ability_name(noteable), noteable) + + # Checking `read_note` permission here, because API code does not seem to use NoteFinder to find notes, + # but rather pulls notes directly through notes association, so there is no chance to check read_note + # permission at service level. With WorkItem model we need to make sure that it has WorkItem::Widgets::Note + # available in order to access notes. + noteable = nil unless can_read_notes?(noteable) noteable || not_found!(noteable_type) end @@ -147,6 +152,13 @@ module API def disable_query_limiting Gitlab::QueryLimiting.disable!('https://gitlab.com/gitlab-org/gitlab/-/issues/211538') end + + private + + def can_read_notes?(noteable) + Ability.allowed?(current_user, noteable_read_ability_name(noteable), noteable) && + Ability.allowed?(current_user, :read_note, noteable) + end end end end diff --git a/lib/gitlab/counters/buffered_counter.rb b/lib/gitlab/counters/buffered_counter.rb new file mode 100644 index 00000000000..56593b642a9 --- /dev/null +++ b/lib/gitlab/counters/buffered_counter.rb @@ -0,0 +1,113 @@ +# frozen_string_literal: true + +module Gitlab + module Counters + class BufferedCounter + include Gitlab::ExclusiveLeaseHelpers + + WORKER_DELAY = 10.minutes + WORKER_LOCK_TTL = 10.minutes + + LUA_FLUSH_INCREMENT_SCRIPT = <<~LUA + local increment_key, flushed_key = KEYS[1], KEYS[2] + local increment_value = redis.call("get", increment_key) or 0 + local flushed_value = redis.call("incrby", flushed_key, increment_value) + if flushed_value == 0 then + redis.call("del", increment_key, flushed_key) + else + redis.call("del", increment_key) + end + return flushed_value + LUA + + def initialize(counter_record, attribute) + @counter_record = counter_record + @attribute = attribute + end + + def get + redis_state do |redis| + redis.get(key).to_i + end + end + + def increment(amount) + result = redis_state do |redis| + redis.incrby(key, amount) + end + + FlushCounterIncrementsWorker.perform_in(WORKER_DELAY, counter_record.class.name, counter_record.id, attribute) + + result + end + + def reset! + counter_record.update!(attribute => 0) + + redis_state do |redis| + redis.del(key) + end + end + + def commit_increment! + with_exclusive_lease do + flush_amount = amount_to_be_flushed + next if flush_amount == 0 + + counter_record.transaction do + counter_record.update_counters_with_lease({ attribute => flush_amount }) + remove_flushed_key + end + + counter_record.execute_after_commit_callbacks + end + + counter_record.reset.read_attribute(attribute) + end + + # amount_to_be_flushed returns the total value to be flushed. + # The total value is the sum of the following: + # - current value in the increment_key + # - any existing value in the flushed_key that has not been flushed + def amount_to_be_flushed + redis_state do |redis| + redis.eval(LUA_FLUSH_INCREMENT_SCRIPT, keys: [key, flushed_key]) + end + end + + def key + project_id = counter_record.project.id + record_name = counter_record.class + record_id = counter_record.id + + "project:{#{project_id}}:counters:#{record_name}:#{record_id}:#{attribute}" + end + + def flushed_key + "#{key}:flushed" + end + + private + + attr_reader :counter_record, :attribute + + def remove_flushed_key + redis_state do |redis| + redis.del(flushed_key) + end + end + + def redis_state(&block) + Gitlab::Redis::SharedState.with(&block) + end + + def with_exclusive_lease(&block) + lock_key = "#{key}:locked" + + in_lock(lock_key, ttl: WORKER_LOCK_TTL, &block) + rescue Gitlab::ExclusiveLeaseHelpers::FailedToObtainLockError + # a worker is already updating the counters + end + end + end +end diff --git a/lib/gitlab/counters/legacy_counter.rb b/lib/gitlab/counters/legacy_counter.rb new file mode 100644 index 00000000000..06951514ec3 --- /dev/null +++ b/lib/gitlab/counters/legacy_counter.rb @@ -0,0 +1,34 @@ +# frozen_string_literal: true + +module Gitlab + module Counters + # This class is a wrapper over ActiveRecord counter + # for attributes that have not adopted Redis-backed BufferedCounter. + class LegacyCounter + def initialize(counter_record, attribute) + @counter_record = counter_record + @attribute = attribute + @current_value = counter_record.method(attribute).call + end + + def increment(amount) + updated = counter_record.class.update_counters(counter_record.id, { attribute => amount }) + + if updated == 1 + counter_record.execute_after_commit_callbacks + @current_value += amount + end + + @current_value + end + + def reset! + counter_record.update!(attribute => 0) + end + + private + + attr_reader :counter_record, :attribute + end + end +end diff --git a/lib/gitlab/database/gitlab_schema.rb b/lib/gitlab/database/gitlab_schema.rb index b1918415919..0f848ed40fb 100644 --- a/lib/gitlab/database/gitlab_schema.rb +++ b/lib/gitlab/database/gitlab_schema.rb @@ -15,7 +15,7 @@ module Gitlab module Database module GitlabSchema - GITLAB_SCHEMAS_FILE = 'lib/gitlab/database/gitlab_schemas.yml' + DICTIONARY_PATH = 'db/docs/' # These tables are deleted/renamed, but still referenced by migrations. # This is needed for now, but should be removed in the future @@ -70,7 +70,7 @@ module Gitlab table_name.gsub!(/_[0-9]+$/, '') # Tables that are properly mapped - if gitlab_schema = tables_to_schema[table_name] + if gitlab_schema = views_and_tables_to_schema[table_name] return gitlab_schema end @@ -98,12 +98,36 @@ module Gitlab undefined ? :"undefined_#{table_name}" : nil end + def self.dictionary_path_globs + [Rails.root.join(DICTIONARY_PATH, '*.yml')] + end + + def self.view_path_globs + [Rails.root.join(DICTIONARY_PATH, 'views', '*.yml')] + end + + def self.views_and_tables_to_schema + @views_and_tables_to_schema ||= self.tables_to_schema.merge(self.views_to_schema) + end + def self.tables_to_schema - @tables_to_schema ||= YAML.load_file(Rails.root.join(GITLAB_SCHEMAS_FILE)) + @tables_to_schema ||= Dir.glob(self.dictionary_path_globs).each_with_object({}) do |file_path, dic| + data = YAML.load_file(file_path) + + dic[data['table_name']] = data['gitlab_schema'].to_sym + end + end + + def self.views_to_schema + @views_to_schema ||= Dir.glob(self.view_path_globs).each_with_object({}) do |file_path, dic| + data = YAML.load_file(file_path) + + dic[data['view_name']] = data['gitlab_schema'].to_sym + end end def self.schema_names - @schema_names ||= self.tables_to_schema.values.to_set + @schema_names ||= self.views_and_tables_to_schema.values.to_set end end end diff --git a/qa/qa/page/project/settings/merge_request.rb b/qa/qa/page/project/settings/merge_request.rb index d862979aeec..ae6a04028c9 100644 --- a/qa/qa/page/project/settings/merge_request.rb +++ b/qa/qa/page/project/settings/merge_request.rb @@ -15,7 +15,7 @@ module QA element :merge_ff_radio end - view 'app/views/projects/_merge_request_merge_checks_settings.html.haml' do + view 'app/views/projects/_merge_request_pipelines_and_threads_options.html.haml' do element :allow_merge_if_all_discussions_are_resolved_checkbox end diff --git a/rubocop/rubocop-code_reuse.yml b/rubocop/rubocop-code_reuse.yml index a3b75117621..6d54f880759 100644 --- a/rubocop/rubocop-code_reuse.yml +++ b/rubocop/rubocop-code_reuse.yml @@ -26,6 +26,7 @@ CodeReuse/ActiveRecord: - lib/banzai/**/*.rb - lib/gitlab/background_migration/**/*.rb - lib/gitlab/cycle_analytics/**/*.rb + - lib/gitlab/counters/**/*.rb - lib/gitlab/database/**/*.rb - lib/gitlab/database_importers/common_metrics/importer.rb - lib/gitlab/import_export/**/*.rb diff --git a/spec/features/projects/settings/visibility_settings_spec.rb b/spec/features/projects/settings/visibility_settings_spec.rb index 105c52b7107..5246eda976b 100644 --- a/spec/features/projects/settings/visibility_settings_spec.rb +++ b/spec/features/projects/settings/visibility_settings_spec.rb @@ -28,26 +28,6 @@ RSpec.describe 'Projects > Settings > Visibility settings', :js, feature_categor expect(visibility_select_container).to have_content 'Only accessible by project members. Membership must be explicitly granted to each user.' end - context 'builds select' do - it 'hides builds select section' do - find('.project-feature-controls[data-for="project[project_feature_attributes][builds_access_level]"] .gl-toggle').click - - visit project_settings_merge_requests_path(project) - - expect(page).to have_selector('.builds-feature', visible: false) - end - - context 'given project with builds_disabled access level' do - let(:project) { create(:project, :builds_disabled, namespace: user.namespace) } - - it 'hides builds select section' do - visit project_settings_merge_requests_path(project) - - expect(page).to have_selector('.builds-feature', visible: false) - end - end - end - context 'disable email notifications' do it 'is visible' do expect(page).to have_selector('.js-emails-disabled', visible: true) diff --git a/spec/graphql/resolvers/users/participants_resolver_spec.rb b/spec/graphql/resolvers/users/participants_resolver_spec.rb index eb2418b63f4..27c3b9643ce 100644 --- a/spec/graphql/resolvers/users/participants_resolver_spec.rb +++ b/spec/graphql/resolvers/users/participants_resolver_spec.rb @@ -115,7 +115,8 @@ RSpec.describe Resolvers::Users::ParticipantsResolver do create(:award_emoji, name: 'thumbsup', awardable: public_note) # 1 extra query per source (3 emojis + 2 notes) to fetch participables collection - expect { query.call }.not_to exceed_query_limit(control_count).with_threshold(5) + # 1 extra query to load work item widgets collection + expect { query.call }.not_to exceed_query_limit(control_count).with_threshold(6) end it 'does not execute N+1 for system note metadata relation' do diff --git a/spec/lib/gitlab/counters/buffered_counter_spec.rb b/spec/lib/gitlab/counters/buffered_counter_spec.rb new file mode 100644 index 00000000000..a1fd97768ea --- /dev/null +++ b/spec/lib/gitlab/counters/buffered_counter_spec.rb @@ -0,0 +1,233 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Counters::BufferedCounter, :clean_gitlab_redis_shared_state do + using RSpec::Parameterized::TableSyntax + + subject(:counter) { described_class.new(counter_record, attribute) } + + let(:counter_record) { create(:project_statistics) } + let(:attribute) { :build_artifacts_size } + + describe '#get' do + it 'returns the value when there is an existing value stored in the counter' do + Gitlab::Redis::SharedState.with do |redis| + redis.set(counter.key, 456) + end + + expect(counter.get).to eq(456) + end + + it 'returns 0 when there is no existing value' do + expect(counter.get).to eq(0) + end + end + + describe '#increment' do + it 'sets a new key by the given value' do + counter.increment(123) + + expect(counter.get).to eq(123) + end + + it 'increments an existing key by the given value' do + counter.increment(100) + counter.increment(123) + + expect(counter.get).to eq(100 + 123) + end + + it 'returns the new value' do + counter.increment(123) + + expect(counter.increment(23)).to eq(146) + end + + it 'schedules a worker to commit the counter into database' do + expect(FlushCounterIncrementsWorker).to receive(:perform_in) + .with(described_class::WORKER_DELAY, counter_record.class.to_s, counter_record.id, attribute) + + counter.increment(123) + end + end + + describe '#reset!' do + before do + allow(counter_record).to receive(:update!) + + counter.increment(123) + end + + it 'removes the key from Redis' do + counter.reset! + + Gitlab::Redis::SharedState.with do |redis| + expect(redis.exists?(counter.key)).to eq(false) + end + end + + it 'resets the counter to 0' do + counter.reset! + + expect(counter.get).to eq(0) + end + + it 'resets the record to 0' do + expect(counter_record).to receive(:update!).with(attribute => 0) + + counter.reset! + end + end + + describe '#commit_increment!' do + it 'obtains an exclusive lease during processing' do + expect(counter).to receive(:with_exclusive_lease).and_call_original + + counter.commit_increment! + end + + context 'when there is an amount to commit' do + let(:increments) { [10, -3] } + + before do + increments.each { |i| counter.increment(i) } + end + + it 'commits the increment into the database' do + expect { counter.commit_increment! } + .to change { counter_record.reset.read_attribute(attribute) }.by(increments.sum) + end + + it 'removes the increment entry from Redis' do + Gitlab::Redis::SharedState.with do |redis| + key_exists = redis.exists?(counter.key) + expect(key_exists).to be_truthy + end + + counter.commit_increment! + + Gitlab::Redis::SharedState.with do |redis| + key_exists = redis.exists?(counter.key) + expect(key_exists).to be_falsey + end + end + end + + context 'when there are no counters to flush' do + context 'when there are no counters in the relative :flushed key' do + it 'does not change the record' do + expect { counter.commit_increment! }.not_to change { counter_record.reset.attributes } + end + end + + # This can be the case where updating counters in the database fails with error + # and retrying the worker will retry flushing the counters but the main key has + # disappeared and the increment has been moved to the "<...>:flushed" key. + context 'when there are counters in the relative :flushed key' do + let(:flushed_amount) { 10 } + + before do + Gitlab::Redis::SharedState.with do |redis| + redis.incrby(counter.flushed_key, flushed_amount) + end + end + + it 'updates the record' do + expect { counter.commit_increment! } + .to change { counter_record.reset.read_attribute(attribute) }.by(flushed_amount) + end + + it 'deletes the relative :flushed key' do + counter.commit_increment! + + Gitlab::Redis::SharedState.with do |redis| + key_exists = redis.exists?(counter.flushed_key) + expect(key_exists).to be_falsey + end + end + end + + context 'when deleting :flushed key fails' do + before do + Gitlab::Redis::SharedState.with do |redis| + redis.incrby(counter.flushed_key, 10) + + allow(redis).to receive(:del).and_raise('could not delete key') + end + end + + it 'does a rollback of the counter update' do + expect { counter.commit_increment! }.to raise_error('could not delete key') + + expect(counter_record.reset.read_attribute(attribute)).to eq(0) + end + end + + context 'when the counter record has after_commit callbacks' do + it 'has registered callbacks' do + expect(counter_record.class.after_commit_callbacks.size).to eq(1) + end + + context 'when there are increments to flush' do + before do + counter.increment(10) + end + + it 'executes the callbacks' do + expect(counter_record).to receive(:execute_after_commit_callbacks).and_call_original + + counter.commit_increment! + end + end + + context 'when there are no increments to flush' do + it 'does not execute the callbacks' do + expect(counter_record).not_to receive(:execute_after_commit_callbacks).and_call_original + + counter.commit_increment! + end + end + end + end + end + + describe '#amount_to_be_flushed' do + let(:increment_key) { counter.key } + let(:flushed_key) { counter.flushed_key } + + where(:increment, :flushed, :result, :flushed_key_present) do + nil | nil | 0 | false + nil | 0 | 0 | false + 0 | 0 | 0 | false + 1 | 0 | 1 | true + 1 | nil | 1 | true + 1 | 1 | 2 | true + 1 | -2 | -1 | true + -1 | 1 | 0 | false + end + + with_them do + before do + Gitlab::Redis::SharedState.with do |redis| + redis.set(increment_key, increment) if increment + redis.set(flushed_key, flushed) if flushed + end + end + + it 'returns the current value to be flushed' do + value = counter.amount_to_be_flushed + expect(value).to eq(result) + end + + it 'drops the increment key and creates the flushed key if it does not exist' do + counter.amount_to_be_flushed + + Gitlab::Redis::SharedState.with do |redis| + expect(redis.exists?(increment_key)).to eq(false) + expect(redis.exists?(flushed_key)).to eq(flushed_key_present) + end + end + end + end +end diff --git a/spec/lib/gitlab/counters/legacy_counter_spec.rb b/spec/lib/gitlab/counters/legacy_counter_spec.rb new file mode 100644 index 00000000000..e66b1ce08c4 --- /dev/null +++ b/spec/lib/gitlab/counters/legacy_counter_spec.rb @@ -0,0 +1,41 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Counters::LegacyCounter do + subject(:counter) { described_class.new(counter_record, attribute) } + + let(:counter_record) { create(:project_statistics) } + let(:attribute) { :snippets_size } + let(:amount) { 123 } + + describe '#increment' do + it 'increments the attribute in the counter record' do + expect { counter.increment(amount) }.to change { counter_record.reload.method(attribute).call }.by(amount) + end + + it 'returns the value after the increment' do + counter.increment(100) + + expect(counter.increment(amount)).to eq(100 + amount) + end + + it 'executes after counter_record after commit callback' do + expect(counter_record).to receive(:execute_after_commit_callbacks).and_call_original + + counter.increment(amount) + end + end + + describe '#reset!' do + before do + allow(counter_record).to receive(:update!) + end + + it 'resets the record to 0' do + expect(counter_record).to receive(:update!).with(attribute => 0) + + counter.reset! + end + end +end diff --git a/spec/lib/gitlab/database/gitlab_schema_spec.rb b/spec/lib/gitlab/database/gitlab_schema_spec.rb index 72950895022..4b37cbda047 100644 --- a/spec/lib/gitlab/database/gitlab_schema_spec.rb +++ b/spec/lib/gitlab/database/gitlab_schema_spec.rb @@ -1,42 +1,86 @@ # frozen_string_literal: true require 'spec_helper' +RSpec.shared_examples 'validate path globs' do |path_globs| + it 'returns an array of path globs' do + expect(path_globs).to be_an(Array) + expect(path_globs).to all(be_an(Pathname)) + end +end + RSpec.describe Gitlab::Database::GitlabSchema do - describe '.tables_to_schema' do - it 'all tables have assigned a known gitlab_schema' do - expect(described_class.tables_to_schema).to all( + describe '.views_and_tables_to_schema' do + it 'all tables and views have assigned a known gitlab_schema' do + expect(described_class.views_and_tables_to_schema).to all( match([be_a(String), be_in(Gitlab::Database.schemas_to_base_models.keys.map(&:to_sym))]) ) end # This being run across different databases indirectly also tests # a general consistency of structure across databases - Gitlab::Database.database_base_models.select { |k, _| k != 'geo' }.each do |db_config_name, db_class| + Gitlab::Database.database_base_models.except(:geo).each do |db_config_name, db_class| context "for #{db_config_name} using #{db_class}" do let(:db_data_sources) { db_class.connection.data_sources } # The Geo database does not share the same structure as all decomposed databases - subject { described_class.tables_to_schema.select { |_, v| v != :gitlab_geo } } + subject { described_class.views_and_tables_to_schema.select { |_, v| v != :gitlab_geo } } it 'new data sources are added' do - missing_tables = db_data_sources.to_set - subject.keys + missing_data_sources = db_data_sources.to_set - subject.keys - expect(missing_tables).to be_empty, \ - "Missing table(s) #{missing_tables.to_a} not found in #{described_class}.tables_to_schema. " \ - "Any new tables must be added to #{described_class::GITLAB_SCHEMAS_FILE}." + expect(missing_data_sources).to be_empty, \ + "Missing table/view(s) #{missing_data_sources.to_a} not found in " \ + "#{described_class}.views_and_tables_to_schema. " \ + "Any new tables or views must be added to the database dictionary. " \ + "More info: https://docs.gitlab.com/ee/development/database/database_dictionary.html" end it 'non-existing data sources are removed' do - extra_tables = subject.keys.to_set - db_data_sources + extra_data_sources = subject.keys.to_set - db_data_sources - expect(extra_tables).to be_empty, \ - "Extra table(s) #{extra_tables.to_a} found in #{described_class}.tables_to_schema. " \ - "Any removed or renamed tables must be removed from #{described_class::GITLAB_SCHEMAS_FILE}." + expect(extra_data_sources).to be_empty, \ + "Extra table/view(s) #{extra_data_sources.to_a} found in #{described_class}.views_and_tables_to_schema. " \ + "Any removed or renamed tables or views must be removed from the database dictionary. " \ + "More info: https://docs.gitlab.com/ee/development/database/database_dictionary.html" end end end end + describe '.dictionary_path_globs' do + include_examples 'validate path globs', described_class.dictionary_path_globs + end + + describe '.view_path_globs' do + include_examples 'validate path globs', described_class.view_path_globs + end + + describe '.tables_to_schema' do + let(:database_models) { Gitlab::Database.database_base_models.except(:geo) } + let(:views) { database_models.flat_map { |_, m| m.connection.views }.sort.uniq } + + subject { described_class.tables_to_schema } + + it 'returns only tables' do + tables = subject.keys + + expect(tables).not_to include(views.to_set) + end + end + + describe '.views_to_schema' do + let(:database_models) { Gitlab::Database.database_base_models.except(:geo) } + let(:tables) { database_models.flat_map { |_, m| m.connection.tables }.sort.uniq } + + subject { described_class.views_to_schema } + + it 'returns only views' do + views = subject.keys + + expect(views).not_to include(tables.to_set) + end + end + describe '.table_schema' do using RSpec::Parameterized::TableSyntax diff --git a/spec/lib/gitlab/database/tables_truncate_spec.rb b/spec/lib/gitlab/database/tables_truncate_spec.rb index fa366a6e32f..4d04bd67a1e 100644 --- a/spec/lib/gitlab/database/tables_truncate_spec.rb +++ b/spec/lib/gitlab/database/tables_truncate_spec.rb @@ -148,6 +148,18 @@ RSpec.describe Gitlab::Database::TablesTruncate, :reestablished_active_record_ba } ) + allow(Gitlab::Database::GitlabSchema).to receive(:views_and_tables_to_schema).and_return( + { + "_test_gitlab_main_items" => :gitlab_main, + "_test_gitlab_main_references" => :gitlab_main, + "_test_gitlab_hook_logs" => :gitlab_main, + "_test_gitlab_ci_items" => :gitlab_ci, + "_test_gitlab_ci_references" => :gitlab_ci, + "_test_gitlab_shared_items" => :gitlab_shared, + "_test_gitlab_geo_items" => :gitlab_geo + } + ) + allow(logger).to receive(:info).with(any_args) end diff --git a/spec/models/concerns/counter_attribute_spec.rb b/spec/models/concerns/counter_attribute_spec.rb index fd1f0d3e836..1dd9b78d642 100644 --- a/spec/models/concerns/counter_attribute_spec.rb +++ b/spec/models/concerns/counter_attribute_spec.rb @@ -12,74 +12,6 @@ RSpec.describe CounterAttribute, :counter_attribute, :clean_gitlab_redis_shared_ let(:model) { CounterAttributeModel.find(project_statistics.id) } end - describe 'after_flush callbacks' do - let(:attribute) { model.class.counter_attributes.first[:attribute] } - - subject { model.flush_increments_to_database!(attribute) } - - it 'has registered callbacks' do # defined in :counter_attribute RSpec tag - expect(model.class.after_flush_callbacks.size).to eq(1) - end - - context 'when there are increments to flush' do - before do - model.delayed_increment_counter(attribute, 10) - end - - it 'executes the callbacks' do - subject - - expect(model.flushed).to be_truthy - end - end - - context 'when there are no increments to flush' do - it 'does not execute the callbacks' do - subject - - expect(model.flushed).to be_nil - end - end - end - - describe '.steal_increments' do - let(:increment_key) { 'counters:Model:123:attribute' } - let(:flushed_key) { 'counter:Model:123:attribute:flushed' } - - subject { model.send(:steal_increments, increment_key, flushed_key) } - - where(:increment, :flushed, :result, :flushed_key_present) do - nil | nil | 0 | false - nil | 0 | 0 | false - 0 | 0 | 0 | false - 1 | 0 | 1 | true - 1 | nil | 1 | true - 1 | 1 | 2 | true - 1 | -2 | -1 | true - -1 | 1 | 0 | false - end - - with_them do - before do - Gitlab::Redis::SharedState.with do |redis| - redis.set(increment_key, increment) if increment - redis.set(flushed_key, flushed) if flushed - end - end - - it { is_expected.to eq(result) } - - it 'drops the increment key and creates the flushed key if it does not exist' do - subject - - Gitlab::Redis::SharedState.with do |redis| - expect(redis.exists?(increment_key)).to eq(false) - expect(redis.exists?(flushed_key)).to eq(flushed_key_present) - end - end - end - end - describe '#counter_attribute_enabled?' do it 'is true when counter attribute is defined' do expect(project_statistics.counter_attribute_enabled?(:build_artifacts_size)) @@ -104,4 +36,42 @@ RSpec.describe CounterAttribute, :counter_attribute, :clean_gitlab_redis_shared_ end end end + + describe '#counter' do + using RSpec::Parameterized::TableSyntax + + it 'returns the counter for the respective attribute' do + expect(model.counter(:build_artifacts_size).send(:attribute)).to eq(:build_artifacts_size) + expect(model.counter(:commit_count).send(:attribute)).to eq(:commit_count) + end + + it 'returns the appropriate counter for the attribute' do + expect(model.counter(:build_artifacts_size).class).to eq(Gitlab::Counters::BufferedCounter) + expect(model.counter(:packages_size).class).to eq(Gitlab::Counters::BufferedCounter) + expect(model.counter(:wiki_size).class).to eq(Gitlab::Counters::LegacyCounter) + end + + context 'with a conditional counter attribute' do + where(:enabled, :expected_counter_class) do + [ + [true, Gitlab::Counters::BufferedCounter], + [false, Gitlab::Counters::LegacyCounter] + ] + end + + with_them do + before do + model.allow_package_size_counter = enabled + end + + it 'returns the appropriate counter based on the condition' do + expect(model.counter(:packages_size).class).to eq(expected_counter_class) + end + end + end + + it 'raises error for unknown attribute' do + expect { model.counter(:unknown) }.to raise_error(ArgumentError, 'attribute "unknown" does not exist') + end + end end diff --git a/spec/models/packages/package_spec.rb b/spec/models/packages/package_spec.rb index 9638e7e3fa8..d6f71f2401c 100644 --- a/spec/models/packages/package_spec.rb +++ b/spec/models/packages/package_spec.rb @@ -713,7 +713,7 @@ RSpec.describe Packages::Package, type: :model do subject(:destroy!) { package.destroy! } it 'updates the project statistics' do - expect(project_statistics).to receive(:delayed_increment_counter).with(:packages_size, -package_file.size) + expect(project_statistics).to receive(:increment_counter).with(:packages_size, -package_file.size) destroy! end diff --git a/spec/models/project_statistics_spec.rb b/spec/models/project_statistics_spec.rb index 95da9a7521b..a6e2bcf1525 100644 --- a/spec/models/project_statistics_spec.rb +++ b/spec/models/project_statistics_spec.rb @@ -467,6 +467,13 @@ RSpec.describe ProjectStatistics do .to change { statistics.reload.storage_size } .by(20) end + + it 'schedules a namespace aggregation worker' do + expect(Namespaces::ScheduleAggregationWorker).to receive(:perform_async) + .with(statistics.project.namespace.id) + + described_class.increment_statistic(project, stat, 20) + end end shared_examples 'a statistic that increases storage_size asynchronously' do @@ -474,7 +481,8 @@ RSpec.describe ProjectStatistics do described_class.increment_statistic(project, stat, 13) Gitlab::Redis::SharedState.with do |redis| - increment = redis.get(statistics.counter_key(stat)) + key = statistics.counter(stat).key + increment = redis.get(key) expect(increment.to_i).to eq(13) end end @@ -482,7 +490,7 @@ RSpec.describe ProjectStatistics do it 'schedules a worker to update the statistic and storage_size async', :sidekiq_inline do expect(FlushCounterIncrementsWorker) .to receive(:perform_in) - .with(CounterAttribute::WORKER_DELAY, described_class.name, statistics.id, stat) + .with(Gitlab::Counters::BufferedCounter::WORKER_DELAY, described_class.name, statistics.id, stat) .and_call_original expect { described_class.increment_statistic(project, stat, 20) } diff --git a/spec/models/projects/build_artifacts_size_refresh_spec.rb b/spec/models/projects/build_artifacts_size_refresh_spec.rb index 21cd8e0b9d4..caff06262d9 100644 --- a/spec/models/projects/build_artifacts_size_refresh_spec.rb +++ b/spec/models/projects/build_artifacts_size_refresh_spec.rb @@ -67,6 +67,8 @@ RSpec.describe Projects::BuildArtifactsSizeRefresh, type: :model do let!(:last_job_artifact_id_on_refresh_start) { create(:ci_job_artifact, project: refresh.project) } + let(:statistics) { refresh.project.statistics } + before do stats = create(:project_statistics, project: refresh.project, build_artifacts_size: 120) stats.increment_counter(:build_artifacts_size, 30) @@ -89,11 +91,11 @@ RSpec.describe Projects::BuildArtifactsSizeRefresh, type: :model do end it 'resets the build artifacts size stats' do - expect { refresh.process! }.to change { refresh.project.statistics.build_artifacts_size }.to(0) + expect { refresh.process! }.to change { statistics.build_artifacts_size }.to(0) end it 'resets the counter attribute to zero' do - expect { refresh.process! }.to change { refresh.project.statistics.get_counter_value(:build_artifacts_size) }.to(0) + expect { refresh.process! }.to change { statistics.counter(:build_artifacts_size).get }.to(0) end end diff --git a/spec/policies/issue_policy_spec.rb b/spec/policies/issue_policy_spec.rb index 8371b5685ed..905ef591b53 100644 --- a/spec/policies/issue_policy_spec.rb +++ b/spec/policies/issue_policy_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe IssuePolicy do +RSpec.describe IssuePolicy, feature_category: :team_planning do include_context 'ProjectPolicyTable context' include ExternalAuthorizationServiceHelpers include ProjectHelpers @@ -13,6 +13,8 @@ RSpec.describe IssuePolicy do let(:author) { create(:user) } let(:assignee) { create(:user) } let(:reporter) { create(:user) } + let(:maintainer) { create(:user) } + let(:owner) { create(:user) } let(:group) { create(:group, :public) } let(:reporter_from_group_link) { create(:user) } let(:non_member) { create(:user) } @@ -198,6 +200,8 @@ RSpec.describe IssuePolicy do before do project.add_guest(guest) project.add_reporter(reporter) + project.add_maintainer(maintainer) + project.add_owner(owner) group.add_reporter(reporter_from_group_link) @@ -413,6 +417,37 @@ RSpec.describe IssuePolicy do expect(permissions(admin, hidden_issue)).to be_allowed(:read_issue) end end + + context 'when accounting for notes widget' do + let(:policy) { described_class.new(reporter, note) } + + before do + widgets_per_type = WorkItems::Type::WIDGETS_FOR_TYPE.dup + widgets_per_type[:task] = [::WorkItems::Widgets::Description] + stub_const('WorkItems::Type::WIDGETS_FOR_TYPE', widgets_per_type) + end + + context 'and notes widget is disabled for task' do + let(:task) { create(:work_item, :task, project: project) } + + it 'does not allow accessing notes' do + # if notes widget is disabled not even maintainer can access notes + expect(permissions(maintainer, task)).to be_disallowed(:create_note, :read_note, :mark_note_as_confidential, :read_internal_note) + expect(permissions(admin, task)).to be_disallowed(:create_note, :read_note, :read_internal_note, :mark_note_as_confidential, :set_note_created_at) + end + end + + context 'and notes widget is enabled for issue' do + it 'allows accessing notes' do + # with notes widget enabled, even guests can access notes + expect(permissions(guest, issue)).to be_allowed(:create_note, :read_note) + expect(permissions(guest, issue)).to be_disallowed(:read_internal_note, :mark_note_as_confidential, :set_note_created_at) + expect(permissions(reporter, issue)).to be_allowed(:create_note, :read_note, :read_internal_note, :mark_note_as_confidential) + expect(permissions(maintainer, issue)).to be_allowed(:create_note, :read_note, :read_internal_note, :mark_note_as_confidential) + expect(permissions(owner, issue)).to be_allowed(:create_note, :read_note, :read_internal_note, :mark_note_as_confidential, :set_note_created_at) + end + end + end end context 'with external authorization enabled' do diff --git a/spec/policies/note_policy_spec.rb b/spec/policies/note_policy_spec.rb index 6a261b4ff5b..dcfc398806a 100644 --- a/spec/policies/note_policy_spec.rb +++ b/spec/policies/note_policy_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe NotePolicy do +RSpec.describe NotePolicy, feature_category: :team_planning do describe '#rules', :aggregate_failures do let(:user) { create(:user) } let(:project) { create(:project, :public) } @@ -255,6 +255,31 @@ RSpec.describe NotePolicy do it_behaves_like 'user can read the note' end + + context 'when notes widget is disabled for task' do + let(:policy) { described_class.new(developer, note) } + + before do + widgets_per_type = WorkItems::Type::WIDGETS_FOR_TYPE.dup + widgets_per_type[:task] = [::WorkItems::Widgets::Description] + stub_const('WorkItems::Type::WIDGETS_FOR_TYPE', widgets_per_type) + end + + context 'when noteable is task' do + let(:noteable) { create(:work_item, :task, project: project) } + let(:note) { create(:note, system: true, noteable: noteable, author: user, project: project) } + + it_behaves_like 'user cannot read or act on the note' + end + + context 'when noteable is issue' do + let(:noteable) { create(:work_item, :issue, project: project) } + let(:note) { create(:note, system: true, noteable: noteable, author: user, project: project) } + + it_behaves_like 'user can read the note' + it_behaves_like 'user can act on the note' + end + end end context 'when it is a system note referencing a confidential issue' do @@ -313,7 +338,7 @@ RSpec.describe NotePolicy do end it 'does not allow guests to read confidential notes and replies' do - expect(permissions(guest, confidential_note)).to be_disallowed(:read_note, :admin_note, :reposition_note, :resolve_note, :award_emoji, :mark_note_as_confidential) + expect(permissions(guest, confidential_note)).to be_disallowed(:read_note, :read_internal_note, :admin_note, :reposition_note, :resolve_note, :award_emoji, :mark_note_as_confidential) end it 'allows reporter to read all notes but not resolve and admin them' do diff --git a/spec/requests/api/discussions_spec.rb b/spec/requests/api/discussions_spec.rb index 89769314daa..38016375b8f 100644 --- a/spec/requests/api/discussions_spec.rb +++ b/spec/requests/api/discussions_spec.rb @@ -29,6 +29,73 @@ RSpec.describe API::Discussions, feature_category: :team_planning do end end + context 'when noteable is a WorkItem' do + let!(:work_item) { create(:work_item, :issue, project: project, author: user) } + let!(:work_item_note) { create(:discussion_note_on_issue, noteable: work_item, project: project, author: user) } + + let(:parent) { project } + let(:noteable) { work_item } + let(:note) { work_item_note } + let(:url) { "/projects/#{parent.id}/issues/#{noteable[:iid]}/discussions" } + + it_behaves_like 'discussions API', 'projects', 'issues', 'iid', can_reply_to_individual_notes: true + + context 'with work item without notes widget' do + before do + stub_const('WorkItems::Type::BASE_TYPES', { issue: { name: 'NoNotesWidget', enum_value: 0 } }) + stub_const('WorkItems::Type::WIDGETS_FOR_TYPE', { issue: [::WorkItems::Widgets::Description] }) + end + + context 'when fetching discussions' do + it "returns 404" do + get api(url, user) + + expect(response).to have_gitlab_http_status(:not_found) + end + end + + context 'when single fetching discussion by discussion_id' do + it "returns 404" do + get api("#{url}/#{work_item_note.discussion_id}", user) + + expect(response).to have_gitlab_http_status(:not_found) + end + end + + context 'when trying to create a new discussion' do + it "returns 404" do + post api(url, user), params: { body: 'hi!' } + + expect(response).to have_gitlab_http_status(:not_found) + end + end + + context 'when trying to create a new comment on a discussion' do + it 'returns 404' do + post api("#{url}/#{note.discussion_id}/notes", user), params: { body: 'Hello!' } + + expect(response).to have_gitlab_http_status(:not_found) + end + end + + context 'when trying to update a new comment on a discussion' do + it 'returns 404' do + put api("#{url}/notes/#{note.id}", user), params: { body: 'Update Hello!' } + + expect(response).to have_gitlab_http_status(:not_found) + end + end + + context 'when deleting a note' do + it 'returns 404' do + delete api("#{url}/#{note.discussion_id}/notes/#{note.id}", user) + + expect(response).to have_gitlab_http_status(:not_found) + end + end + end + end + context 'when noteable is a Snippet' do let!(:snippet) { create(:project_snippet, project: project, author: user) } let!(:snippet_note) { create(:discussion_note_on_project_snippet, noteable: snippet, project: project, author: user) } diff --git a/spec/requests/api/graphql/mutations/notes/create/note_spec.rb b/spec/requests/api/graphql/mutations/notes/create/note_spec.rb index f263c11a5f0..00e25909746 100644 --- a/spec/requests/api/graphql/mutations/notes/create/note_spec.rb +++ b/spec/requests/api/graphql/mutations/notes/create/note_spec.rb @@ -102,6 +102,35 @@ RSpec.describe 'Adding a Note', feature_category: :team_planning do it_behaves_like 'a Note mutation with confidential notes' end + + context 'as work item' do + let(:noteable) { create(:work_item, :issue, project: project) } + + context 'when using internal param' do + let(:variables_extra) { { internal: true } } + + it_behaves_like 'a Note mutation with confidential notes' + end + + context 'when using deprecated confidential param' do + let(:variables_extra) { { confidential: true } } + + it_behaves_like 'a Note mutation with confidential notes' + end + + context 'without notes widget' do + let(:variables_extra) { {} } + + before do + stub_const('WorkItems::Type::BASE_TYPES', { issue: { name: 'NoNotesWidget', enum_value: 0 } }) + stub_const('WorkItems::Type::WIDGETS_FOR_TYPE', { issue: [::WorkItems::Widgets::Description] }) + end + + it_behaves_like 'a Note mutation that does not create a Note' + it_behaves_like 'a mutation that returns top-level errors', + errors: [Gitlab::Graphql::Authorize::AuthorizeResource::RESOURCE_ACCESS_ERROR] + end + end end context 'when body only contains quick actions' do diff --git a/spec/requests/api/graphql/mutations/notes/destroy_spec.rb b/spec/requests/api/graphql/mutations/notes/destroy_spec.rb index 3ee76b5ca03..eb45e2aa033 100644 --- a/spec/requests/api/graphql/mutations/notes/destroy_spec.rb +++ b/spec/requests/api/graphql/mutations/notes/destroy_spec.rb @@ -5,14 +5,11 @@ require 'spec_helper' RSpec.describe 'Destroying a Note', feature_category: :team_planning do include GraphqlHelpers - let!(:note) { create(:note) } - let(:mutation) do - variables = { - id: GitlabSchema.id_from_object(note).to_s - } - - graphql_mutation(:destroy_note, variables) - end + let(:noteable) { create(:work_item, :issue) } + let!(:note) { create(:note, noteable: noteable, project: noteable.project) } + let(:global_note_id) { GitlabSchema.id_from_object(note).to_s } + let(:variables) { { id: global_note_id } } + let(:mutation) { graphql_mutation(:destroy_note, variables) } def mutation_response graphql_mutation_response(:destroy_note) @@ -47,5 +44,31 @@ RSpec.describe 'Destroying a Note', feature_category: :team_planning do expect(mutation_response).to have_key('note') expect(mutation_response['note']).to be_nil end + + context 'when note is system' do + let!(:note) { create(:note, :system) } + + it 'does not destroy system note' do + expect do + post_graphql_mutation(mutation, current_user: current_user) + end.not_to change { Note.count } + end + end + + context 'without notes widget' do + before do + stub_const('WorkItems::Type::BASE_TYPES', { issue: { name: 'NoNotesWidget', enum_value: 0 } }) + stub_const('WorkItems::Type::WIDGETS_FOR_TYPE', { issue: [::WorkItems::Widgets::Description] }) + end + + it 'does not update the Note' do + expect do + post_graphql_mutation(mutation, current_user: current_user) + end.to not_change { Note.count } + end + + it_behaves_like 'a mutation that returns top-level errors', + errors: [Gitlab::Graphql::Authorize::AuthorizeResource::RESOURCE_ACCESS_ERROR] + end end end diff --git a/spec/requests/api/graphql/mutations/notes/update/note_spec.rb b/spec/requests/api/graphql/mutations/notes/update/note_spec.rb index 6b9dbbbfab9..dff8a87314b 100644 --- a/spec/requests/api/graphql/mutations/notes/update/note_spec.rb +++ b/spec/requests/api/graphql/mutations/notes/update/note_spec.rb @@ -36,49 +36,32 @@ RSpec.describe 'Updating a Note', feature_category: :team_planning do it_behaves_like 'a Note mutation when the given resource id is not for a Note' - it 'updates the Note' do - post_graphql_mutation(mutation, current_user: current_user) - - expect(note.reload.note).to eq(updated_body) - end - - it 'returns the updated Note' do - post_graphql_mutation(mutation, current_user: current_user) - - expect(mutation_response['note']['body']).to eq(updated_body) - end + it_behaves_like 'a Note mutation updates a note successfully' + it_behaves_like 'a Note mutation update with errors' + it_behaves_like 'a Note mutation update only with quick actions' - context 'when there are ActiveRecord validation errors' do - let(:params) { { body: '', confidential: true } } + context 'for work item' do + let(:noteable) { create(:work_item, :issue) } + let(:note) { create(:note, noteable: noteable, project: noteable.project, note: original_body) } - it_behaves_like 'a mutation that returns errors in the response', - errors: ["Note can't be blank", 'Confidential can not be changed for existing notes'] + it_behaves_like 'a Note mutation updates a note successfully' + it_behaves_like 'a Note mutation update with errors' + it_behaves_like 'a Note mutation update only with quick actions' - it 'does not update the Note' do - post_graphql_mutation(mutation, current_user: current_user) - - expect(note.reload.note).to eq(original_body) - expect(note.confidential).to be_falsey - end - - it 'returns the original Note' do - post_graphql_mutation(mutation, current_user: current_user) - - expect(mutation_response['note']['body']).to eq(original_body) - expect(mutation_response['note']['confidential']).to be_falsey - end - end + context 'without notes widget' do + before do + stub_const('WorkItems::Type::BASE_TYPES', { issue: { name: 'NoNotesWidget', enum_value: 0 } }) + stub_const('WorkItems::Type::WIDGETS_FOR_TYPE', { issue: [::WorkItems::Widgets::Description] }) + end - context 'when body only contains quick actions' do - let(:updated_body) { '/close' } + it 'does not update the Note' do + post_graphql_mutation(mutation, current_user: current_user) - it 'returns a nil note and empty errors' do - post_graphql_mutation(mutation, current_user: current_user) + expect(note.reload.note).to eq(original_body) + end - expect(mutation_response).to include( - 'errors' => [], - 'note' => nil - ) + it_behaves_like 'a mutation that returns top-level errors', + errors: [Gitlab::Graphql::Authorize::AuthorizeResource::RESOURCE_ACCESS_ERROR] end end end diff --git a/spec/requests/api/notes_spec.rb b/spec/requests/api/notes_spec.rb index 339927e86a6..c2d9db1e6fb 100644 --- a/spec/requests/api/notes_spec.rb +++ b/spec/requests/api/notes_spec.rb @@ -203,6 +203,51 @@ RSpec.describe API::Notes, feature_category: :team_planning do end end end + + context 'without notes widget' do + let(:request_body) { 'Hi!' } + let(:params) { { body: request_body } } + let(:request_path) { "/projects/#{ext_proj.id}/issues/#{ext_issue.iid}/notes" } + + before do + stub_const('WorkItems::Type::BASE_TYPES', { issue: { name: 'NoNotesWidget', enum_value: 0 } }) + stub_const('WorkItems::Type::WIDGETS_FOR_TYPE', { issue: [::WorkItems::Widgets::Description] }) + end + + it 'does not fetch notes' do + get api(request_path, private_user) + + expect(response).to have_gitlab_http_status(:not_found) + end + + it 'does not fetch specific note' do + get api("#{request_path}/#{cross_reference_note.id}", private_user) + + expect(response).to have_gitlab_http_status(:not_found) + end + + it 'does not create note' do + post api(request_path, private_user), params: params + + expect(response).to have_gitlab_http_status(:not_found) + end + + it 'does not update note' do + put api("#{request_path}/#{cross_reference_note.id}", private_user), params: params + + expect(response).to have_gitlab_http_status(:not_found) + end + + it 'does not run quick actions' do + params[:body] = "/spend 1h" + + expect do + post api("#{request_path}/#{cross_reference_note.id}", private_user), params: params + end.to not_change { Note.system.count }.and(not_change { Note.where(system: false).count }) + + expect(response).to have_gitlab_http_status(:not_found) + end + end end end diff --git a/spec/services/issuable/discussions_list_service_spec.rb b/spec/services/issuable/discussions_list_service_spec.rb index 2ce47f42a72..ecdd8d031c9 100644 --- a/spec/services/issuable/discussions_list_service_spec.rb +++ b/spec/services/issuable/discussions_list_service_spec.rb @@ -17,6 +17,19 @@ RSpec.describe Issuable::DiscussionsListService do let_it_be(:issuable) { create(:issue, project: project) } it_behaves_like 'listing issuable discussions', :guest, 1, 7 + + context 'without notes widget' do + let_it_be(:issuable) { create(:work_item, :issue, project: project) } + + before do + stub_const('WorkItems::Type::BASE_TYPES', { issue: { name: 'NoNotesWidget', enum_value: 0 } }) + stub_const('WorkItems::Type::WIDGETS_FOR_TYPE', { issue: [::WorkItems::Widgets::Description] }) + end + + it "returns no notes" do + expect(discussions_service.execute).to be_empty + end + end end describe 'fetching notes for merge requests' do diff --git a/spec/services/projects/refresh_build_artifacts_size_statistics_service_spec.rb b/spec/services/projects/refresh_build_artifacts_size_statistics_service_spec.rb index 6a715312097..a3cff345f68 100644 --- a/spec/services/projects/refresh_build_artifacts_size_statistics_service_spec.rb +++ b/spec/services/projects/refresh_build_artifacts_size_statistics_service_spec.rb @@ -28,6 +28,7 @@ RSpec.describe Projects::RefreshBuildArtifactsSizeStatisticsService, :clean_gitl end let(:now) { Time.zone.now } + let(:statistics) { project.statistics } around do |example| freeze_time { example.run } @@ -45,7 +46,7 @@ RSpec.describe Projects::RefreshBuildArtifactsSizeStatisticsService, :clean_gitl end it 'increments the counter attribute by the total size of the current batch of artifacts' do - expect { service.execute }.to change { project.statistics.get_counter_value(:build_artifacts_size) }.to(3) + expect { service.execute }.to change { statistics.counter(:build_artifacts_size).get }.to(3) end it 'updates the last_job_artifact_id to the ID of the last artifact from the batch' do diff --git a/spec/support/counter_attribute.rb b/spec/support/counter_attribute.rb index 18db5c92644..44df2df0ea5 100644 --- a/spec/support/counter_attribute.rb +++ b/spec/support/counter_attribute.rb @@ -15,7 +15,7 @@ RSpec.configure do |config| attr_accessor :flushed, :allow_package_size_counter - counter_attribute_after_flush do |subject| + counter_attribute_after_commit do |subject| subject.flushed = true end end diff --git a/spec/support/shared_examples/graphql/notes_creation_shared_examples.rb b/spec/support/shared_examples/graphql/notes_creation_shared_examples.rb index bdd4dbfe209..3ff93371c19 100644 --- a/spec/support/shared_examples/graphql/notes_creation_shared_examples.rb +++ b/spec/support/shared_examples/graphql/notes_creation_shared_examples.rb @@ -20,7 +20,7 @@ RSpec.shared_examples 'a Note mutation when the user does not have permission' d it_behaves_like 'a Note mutation that does not create a Note' it_behaves_like 'a mutation that returns top-level errors', - errors: ['The resource that you are attempting to access does not exist or you don\'t have permission to perform this action'] + errors: ['The resource that you are attempting to access does not exist or you don\'t have permission to perform this action'] end RSpec.shared_examples 'a Note mutation when there are active record validation errors' do |model: Note| @@ -74,7 +74,7 @@ RSpec.shared_examples 'a Note mutation when there are rate limit validation erro it_behaves_like 'a Note mutation that does not create a Note' it_behaves_like 'a mutation that returns top-level errors', - errors: ['This endpoint has been requested too many times. Try again later.'] + errors: ['This endpoint has been requested too many times. Try again later.'] context 'when the user is in the allowlist' do before do @@ -97,3 +97,55 @@ RSpec.shared_examples 'a Note mutation with confidential notes' do expect(mutation_response['note']['internal']).to eq(true) end end + +RSpec.shared_examples 'a Note mutation updates a note successfully' do + it 'updates the Note' do + post_graphql_mutation(mutation, current_user: current_user) + + expect(note.reload.note).to eq(updated_body) + end + + it 'returns the updated Note' do + post_graphql_mutation(mutation, current_user: current_user) + + expect(mutation_response['note']['body']).to eq(updated_body) + end +end + +RSpec.shared_examples 'a Note mutation update with errors' do + context 'when there are ActiveRecord validation errors' do + let(:params) { { body: '', confidential: true } } + + it_behaves_like 'a mutation that returns errors in the response', + errors: ["Note can't be blank", 'Confidential can not be changed for existing notes'] + + it 'does not update the Note' do + post_graphql_mutation(mutation, current_user: current_user) + + expect(note.reload.note).to eq(original_body) + expect(note.confidential).to be_falsey + end + + it 'returns the original Note' do + post_graphql_mutation(mutation, current_user: current_user) + + expect(mutation_response['note']['body']).to eq(original_body) + expect(mutation_response['note']['confidential']).to be_falsey + end + end +end + +RSpec.shared_examples 'a Note mutation update only with quick actions' do + context 'when body only contains quick actions' do + let(:updated_body) { '/close' } + + it 'returns a nil note and empty errors' do + post_graphql_mutation(mutation, current_user: current_user) + + expect(mutation_response).to include( + 'errors' => [], + 'note' => nil + ) + end + end +end diff --git a/spec/support/shared_examples/models/concerns/counter_attribute_shared_examples.rb b/spec/support/shared_examples/models/concerns/counter_attribute_shared_examples.rb index 81dbbe7758e..a20bb794095 100644 --- a/spec/support/shared_examples/models/concerns/counter_attribute_shared_examples.rb +++ b/spec/support/shared_examples/models/concerns/counter_attribute_shared_examples.rb @@ -6,11 +6,6 @@ RSpec.shared_examples_for CounterAttribute do |counter_attributes| Gitlab::ApplicationContext.push(feature_category: 'test', caller_id: 'caller') end - it 'defines a Redis counter_key' do - expect(model.counter_key(:counter_name)) - .to eq("project:{#{model.project_id}}:counters:CounterAttributeModel:#{model.id}:counter_name") - end - it 'defines a method to store counters' do registered_attributes = model.class.counter_attributes.map { |e| e[:attribute] } # rubocop:disable Rails/Pluck expect(registered_attributes).to contain_exactly(*counter_attributes) @@ -18,10 +13,11 @@ RSpec.shared_examples_for CounterAttribute do |counter_attributes| counter_attributes.each do |attribute| describe attribute do - describe '#delayed_increment_counter', :redis do + describe '#increment_counter', :redis do let(:increment) { 10 } + let(:counter_key) { model.counter(attribute).key } - subject { model.delayed_increment_counter(attribute, increment) } + subject { model.increment_counter(attribute, increment) } context 'when attribute is a counter attribute' do where(:increment) { [10, -3] } @@ -45,7 +41,7 @@ RSpec.shared_examples_for CounterAttribute do |counter_attributes| subject Gitlab::Redis::SharedState.with do |redis| - counter = redis.get(model.counter_key(attribute)) + counter = redis.get(counter_key) expect(counter).to eq(increment.to_s) end end @@ -56,7 +52,7 @@ RSpec.shared_examples_for CounterAttribute do |counter_attributes| it 'schedules a worker to flush counter increments asynchronously' do expect(FlushCounterIncrementsWorker).to receive(:perform_in) - .with(CounterAttribute::WORKER_DELAY, model.class.name, model.id, attribute) + .with(Gitlab::Counters::BufferedCounter::WORKER_DELAY, model.class.name, model.id, attribute) .and_call_original subject @@ -74,128 +70,13 @@ RSpec.shared_examples_for CounterAttribute do |counter_attributes| end end end - - context 'when attribute is not a counter attribute' do - it 'raises ArgumentError' do - expect { model.delayed_increment_counter(:unknown_attribute, 10) } - .to raise_error(ArgumentError, 'unknown_attribute is not a counter attribute') - end - end - end - end - end - - describe '.flush_increments_to_database!', :redis do - let(:incremented_attribute) { counter_attributes.first } - - subject { model.flush_increments_to_database!(incremented_attribute) } - - it 'obtains an exclusive lease during processing' do - expect(model) - .to receive(:in_lock) - .with(model.counter_lock_key(incremented_attribute), ttl: described_class::WORKER_LOCK_TTL) - .and_call_original - - subject - end - - context 'when there is a counter to flush' do - before do - model.delayed_increment_counter(incremented_attribute, 10) - model.delayed_increment_counter(incremented_attribute, -3) - end - - it 'updates the record and logs it', :aggregate_failures do - expect(Gitlab::AppLogger).to receive(:info).with( - hash_including( - message: 'Acquiring lease for project statistics update', - attributes: [incremented_attribute] - ) - ) - - expect(Gitlab::AppLogger).to receive(:info).with( - hash_including( - message: 'Flush counter attribute to database', - attribute: incremented_attribute, - project_id: model.project_id, - increment: 7, - previous_db_value: 0, - new_db_value: 7, - 'correlation_id' => an_instance_of(String), - 'meta.feature_category' => 'test', - 'meta.caller_id' => 'caller' - ) - ) - - expect { subject }.to change { model.reset.read_attribute(incremented_attribute) }.by(7) - end - - it 'removes the increment entry from Redis' do - Gitlab::Redis::SharedState.with do |redis| - key_exists = redis.exists?(model.counter_key(incremented_attribute)) - expect(key_exists).to be_truthy - end - - subject - - Gitlab::Redis::SharedState.with do |redis| - key_exists = redis.exists?(model.counter_key(incremented_attribute)) - expect(key_exists).to be_falsey - end - end - end - - context 'when there are no counters to flush' do - context 'when there are no counters in the relative :flushed key' do - it 'does not change the record' do - expect { subject }.not_to change { model.reset.attributes } - end - end - - # This can be the case where updating counters in the database fails with error - # and retrying the worker will retry flushing the counters but the main key has - # disappeared and the increment has been moved to the "<...>:flushed" key. - context 'when there are counters in the relative :flushed key' do - before do - Gitlab::Redis::SharedState.with do |redis| - redis.incrby(model.counter_flushed_key(incremented_attribute), 10) - end - end - - it 'updates the record' do - expect { subject }.to change { model.reset.read_attribute(incremented_attribute) }.by(10) - end - - it 'deletes the relative :flushed key' do - subject - - Gitlab::Redis::SharedState.with do |redis| - key_exists = redis.exists?(model.counter_flushed_key(incremented_attribute)) - expect(key_exists).to be_falsey - end - end - end - end - - context 'when deleting :flushed key fails' do - before do - Gitlab::Redis::SharedState.with do |redis| - redis.incrby(model.counter_flushed_key(incremented_attribute), 10) - - expect(redis).to receive(:del).and_raise('could not delete key') - end - end - - it 'does a rollback of the counter update' do - expect { subject }.to raise_error('could not delete key') - - expect(model.reset.read_attribute(incremented_attribute)).to eq(0) end end end describe '#reset_counter!' do let(:attribute) { counter_attributes.first } + let(:counter_key) { model.counter(attribute).key } before do model.update!(attribute => 123) @@ -208,7 +89,7 @@ RSpec.shared_examples_for CounterAttribute do |counter_attributes| expect { subject }.to change { model.reload.send(attribute) }.from(123).to(0) Gitlab::Redis::SharedState.with do |redis| - key_exists = redis.exists?(model.counter_key(attribute)) + key_exists = redis.exists?(counter_key) expect(key_exists).to be_falsey end end diff --git a/spec/support/shared_examples/models/update_project_statistics_shared_examples.rb b/spec/support/shared_examples/models/update_project_statistics_shared_examples.rb index b81bd514d0a..eb742921d35 100644 --- a/spec/support/shared_examples/models/update_project_statistics_shared_examples.rb +++ b/spec/support/shared_examples/models/update_project_statistics_shared_examples.rb @@ -15,7 +15,7 @@ RSpec.shared_examples 'UpdateProjectStatistics' do |with_counter_attribute| def read_pending_increment Gitlab::Redis::SharedState.with do |redis| - key = project.statistics.counter_key(project_statistics_name) + key = project.statistics.counter(project_statistics_name).key redis.get(key).to_i end end @@ -25,7 +25,7 @@ RSpec.shared_examples 'UpdateProjectStatistics' do |with_counter_attribute| def expect_flush_counter_increments_worker_performed expect(FlushCounterIncrementsWorker) .to receive(:perform_in) - .with(CounterAttribute::WORKER_DELAY, project.statistics.class.name, project.statistics.id, project_statistics_name) + .with(Gitlab::Counters::BufferedCounter::WORKER_DELAY, project.statistics.class.name, project.statistics.id, project_statistics_name) yield diff --git a/spec/workers/flush_counter_increments_worker_spec.rb b/spec/workers/flush_counter_increments_worker_spec.rb index 14b49b97ac3..83670acf4b6 100644 --- a/spec/workers/flush_counter_increments_worker_spec.rb +++ b/spec/workers/flush_counter_increments_worker_spec.rb @@ -12,29 +12,32 @@ RSpec.describe FlushCounterIncrementsWorker, :counter_attribute do subject { worker.perform(model.class.name, model.id, attribute) } - it 'flushes increments to database' do + it 'commits increments to database' do expect(model.class).to receive(:find_by_id).and_return(model) - expect(model) - .to receive(:flush_increments_to_database!) - .with(attribute) - .and_call_original + expect_next_instance_of(Gitlab::Counters::BufferedCounter, model, attribute) do |service| + expect(service).to receive(:commit_increment!) + end subject end context 'when model class does not exist' do - subject { worker.perform('non-existend-model') } + subject { worker.perform('NonExistentModel', 1, attribute) } it 'does nothing' do - expect(worker).not_to receive(:in_lock) + expect(Gitlab::Counters::BufferedCounter).not_to receive(:new) + + subject end end context 'when record does not exist' do - subject { worker.perform(model.class.name, model.id + 100, attribute) } + subject { worker.perform(model.class.name, non_existing_record_id, attribute) } it 'does nothing' do - expect(worker).not_to receive(:in_lock) + expect(Gitlab::Counters::BufferedCounter).not_to receive(:new) + + subject end end end |
