diff options
50 files changed, 533 insertions, 231 deletions
@@ -237,7 +237,6 @@ gem 'webpack-rails', '~> 0.9.10' gem 'rack-proxy', '~> 0.6.0' gem 'sass-rails', '~> 5.0.6' -gem 'coffee-rails', '~> 4.1.0' gem 'uglifier', '~> 2.7.2' gem 'addressable', '~> 2.3.8' diff --git a/Gemfile.lock b/Gemfile.lock index e25a7d16461..a24636ad512 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -122,13 +122,6 @@ GEM coderay (1.1.1) coercible (1.0.0) descendants_tracker (~> 0.0.1) - coffee-rails (4.1.1) - coffee-script (>= 2.2.0) - railties (>= 4.0.0, < 5.1.x) - coffee-script (2.4.1) - coffee-script-source - execjs - coffee-script-source (1.10.0) colorize (0.7.7) concurrent-ruby (1.0.5) concurrent-ruby-ext (1.0.5) @@ -938,7 +931,6 @@ DEPENDENCIES charlock_holmes (~> 0.7.3) chronic (~> 0.10.2) chronic_duration (~> 0.10.6) - coffee-rails (~> 4.1.0) concurrent-ruby (~> 1.0.5) connection_pool (~> 2.0) creole (~> 0.5.0) diff --git a/app/assets/javascripts/dropzone_input.js b/app/assets/javascripts/dropzone_input.js index 73675d300be..9ebbb22e807 100644 --- a/app/assets/javascripts/dropzone_input.js +++ b/app/assets/javascripts/dropzone_input.js @@ -5,21 +5,28 @@ import './preview_markdown'; window.DropzoneInput = (function() { function DropzoneInput(form) { - var updateAttachingMessage, $attachingFileMessage, $mdArea, $attachButton, $cancelButton, $retryLink, $uploadingErrorContainer, $uploadingErrorMessage, $uploadProgress, $uploadingProgressContainer, appendToTextArea, btnAlert, child, closeAlertMessage, closeSpinner, divHover, divSpinner, dropzone, $formDropzone, formTextarea, getFilename, handlePaste, iconPaperclip, iconSpinner, insertToTextArea, isImage, maxFileSize, pasteText, uploadsPath, showError, showSpinner, uploadFile, addFileToForm; Dropzone.autoDiscover = false; - divHover = '<div class="div-dropzone-hover"></div>'; - iconPaperclip = '<i class="fa fa-paperclip div-dropzone-icon"></i>'; - $attachButton = form.find('.button-attach-file'); - $attachingFileMessage = form.find('.attaching-file-message'); - $cancelButton = form.find('.button-cancel-uploading-files'); - $retryLink = form.find('.retry-uploading-link'); - $uploadProgress = form.find('.uploading-progress'); - $uploadingErrorContainer = form.find('.uploading-error-container'); - $uploadingErrorMessage = form.find('.uploading-error-message'); - $uploadingProgressContainer = form.find('.uploading-progress-container'); - uploadsPath = window.uploads_path || null; - maxFileSize = gon.max_file_size || 10; - formTextarea = form.find('.js-gfm-input'); + const divHover = '<div class="div-dropzone-hover"></div>'; + const iconPaperclip = '<i class="fa fa-paperclip div-dropzone-icon"></i>'; + const $attachButton = form.find('.button-attach-file'); + const $attachingFileMessage = form.find('.attaching-file-message'); + const $cancelButton = form.find('.button-cancel-uploading-files'); + const $retryLink = form.find('.retry-uploading-link'); + const $uploadProgress = form.find('.uploading-progress'); + const $uploadingErrorContainer = form.find('.uploading-error-container'); + const $uploadingErrorMessage = form.find('.uploading-error-message'); + const $uploadingProgressContainer = form.find('.uploading-progress-container'); + const uploadsPath = window.uploads_path || null; + const maxFileSize = gon.max_file_size || 10; + const formTextarea = form.find('.js-gfm-input'); + let handlePaste; + let pasteText; + let addFileToForm; + let updateAttachingMessage; + let isImage; + let getFilename; + let uploadFile; + formTextarea.wrap('<div class="div-dropzone"></div>'); formTextarea.on('paste', (function(_this) { return function(event) { @@ -28,16 +35,16 @@ window.DropzoneInput = (function() { })(this)); // Add dropzone area to the form. - $mdArea = formTextarea.closest('.md-area'); + const $mdArea = formTextarea.closest('.md-area'); form.setupMarkdownPreview(); - $formDropzone = form.find('.div-dropzone'); + const $formDropzone = form.find('.div-dropzone'); $formDropzone.parent().addClass('div-dropzone-wrapper'); $formDropzone.append(divHover); $formDropzone.find('.div-dropzone-hover').append(iconPaperclip); if (!uploadsPath) return; - dropzone = $formDropzone.dropzone({ + const dropzone = $formDropzone.dropzone({ url: uploadsPath, dictDefaultMessage: '', clickable: true, @@ -117,7 +124,7 @@ window.DropzoneInput = (function() { } }); - child = $(dropzone[0]).children('textarea'); + const child = $(dropzone[0]).children('textarea'); // removeAllFiles(true) stops uploading files (if any) // and remove them from dropzone files queue. @@ -214,6 +221,35 @@ window.DropzoneInput = (function() { return value.first(); }; + const showSpinner = function(e) { + return $uploadingProgressContainer.removeClass('hide'); + }; + + const closeSpinner = function() { + return $uploadingProgressContainer.addClass('hide'); + }; + + const showError = function(message) { + $uploadingErrorContainer.removeClass('hide'); + $uploadingErrorMessage.html(message); + }; + + const closeAlertMessage = function() { + return form.find('.div-dropzone-alert').alert('close'); + }; + + const insertToTextArea = function(filename, url) { + return $(child).val(function(index, val) { + return val.replace(`{{${filename}}}`, url); + }); + }; + + const appendToTextArea = function(url) { + return $(child).val(function(index, val) { + return val + url + "\n"; + }); + }; + uploadFile = function(item, filename) { var formData; formData = new FormData(); @@ -262,35 +298,6 @@ window.DropzoneInput = (function() { messageContainer.text(attachingMessage); }; - insertToTextArea = function(filename, url) { - return $(child).val(function(index, val) { - return val.replace(`{{${filename}}}`, url); - }); - }; - - appendToTextArea = function(url) { - return $(child).val(function(index, val) { - return val + url + "\n"; - }); - }; - - showSpinner = function(e) { - return $uploadingProgressContainer.removeClass('hide'); - }; - - closeSpinner = function() { - return $uploadingProgressContainer.addClass('hide'); - }; - - showError = function(message) { - $uploadingErrorContainer.removeClass('hide'); - $uploadingErrorMessage.html(message); - }; - - closeAlertMessage = function() { - return form.find('.div-dropzone-alert').alert('close'); - }; - form.find('.markdown-selector').click(function(e) { e.preventDefault(); $(this).closest('.gfm-form').find('.div-dropzone').click(); diff --git a/app/assets/javascripts/lib/utils/http_status.js b/app/assets/javascripts/lib/utils/http_status.js index 415e50f32ae..625e53ee9de 100644 --- a/app/assets/javascripts/lib/utils/http_status.js +++ b/app/assets/javascripts/lib/utils/http_status.js @@ -3,6 +3,7 @@ */ export default { + ABORTED: 0, NO_CONTENT: 204, OK: 200, }; diff --git a/app/assets/javascripts/lib/utils/poll.js b/app/assets/javascripts/lib/utils/poll.js index e31cc5fbabe..97666e13ebe 100644 --- a/app/assets/javascripts/lib/utils/poll.js +++ b/app/assets/javascripts/lib/utils/poll.js @@ -81,6 +81,9 @@ export default class Poll { }) .catch((error) => { notificationCallback(false); + if (error.status === httpStatusCodes.ABORTED) { + return; + } errorCallback(error); }); } diff --git a/app/assets/stylesheets/framework/dropdowns.scss b/app/assets/stylesheets/framework/dropdowns.scss index f89f2f30443..fbf49f3a813 100644 --- a/app/assets/stylesheets/framework/dropdowns.scss +++ b/app/assets/stylesheets/framework/dropdowns.scss @@ -297,18 +297,12 @@ } .droplab-dropdown { - .description { - display: inline-block; - white-space: normal; - margin-left: 5px; - } - .dropdown-toggle > i { pointer-events: none; } - li { - padding: $gl-btn-padding $gl-btn-padding 2px; + .dropdown-menu li { + padding: $gl-btn-padding; cursor: pointer; > a, @@ -344,9 +338,25 @@ visibility: visible; } + &.divider { + margin: 0 8px; + padding: 0; + border-top: $gray-darkest; + } + .icon { visibility: hidden; } + + .description { + display: inline-block; + white-space: normal; + margin-left: 5px; + + p { + margin-bottom: 0; + } + } } .icon { @@ -354,12 +364,6 @@ vertical-align: top; padding-top: 2px; } - - .divider { - margin: 0 8px; - padding: 0; - border-top: $gray-darkest; - } } .droplab-dropdown .dropdown-menu, diff --git a/app/assets/stylesheets/framework/filters.scss b/app/assets/stylesheets/framework/filters.scss index 7e4e5fd7f1c..41184907abb 100644 --- a/app/assets/stylesheets/framework/filters.scss +++ b/app/assets/stylesheets/framework/filters.scss @@ -275,7 +275,7 @@ } .filtered-search-input-dropdown-menu { - max-height: 215px; + max-height: 225px; max-width: 280px; overflow: auto; @@ -382,10 +382,6 @@ } } -.dropdown-menu .filter-dropdown-item { - padding: 0; -} - @media (max-width: $screen-xs-max) { .issues-details-filters { padding: 0 0 10px; @@ -435,6 +431,7 @@ .fa { width: 15px; + line-height: $line-height-base; } .dropdown-label-box { diff --git a/app/assets/stylesheets/pages/issuable.scss b/app/assets/stylesheets/pages/issuable.scss index 56a4b53ed61..e858ef427b0 100644 --- a/app/assets/stylesheets/pages/issuable.scss +++ b/app/assets/stylesheets/pages/issuable.scss @@ -813,8 +813,6 @@ } .description { - margin-bottom: 10px; - .text { margin: 0; } diff --git a/app/models/commit.rb b/app/models/commit.rb index c7f62617c4c..1e19f00106a 100644 --- a/app/models/commit.rb +++ b/app/models/commit.rb @@ -1,5 +1,6 @@ class Commit extend ActiveModel::Naming + extend Gitlab::Cache::RequestCache include ActiveModel::Conversion include Noteable @@ -169,19 +170,9 @@ class Commit end def author - if RequestStore.active? - key = "commit_author:#{author_email.downcase}" - # nil is a valid value since no author may exist in the system - if RequestStore.store.key?(key) - @author = RequestStore.store[key] - else - @author = find_author_by_any_email - RequestStore.store[key] = @author - end - else - @author ||= find_author_by_any_email - end + User.find_by_any_email(author_email.downcase) end + request_cache(:author) { author_email.downcase } def committer @committer ||= User.find_by_any_email(committer_email.downcase) @@ -322,7 +313,7 @@ class Commit def raw_diffs(*args) if Gitlab::GitalyClient.feature_enabled?(:commit_raw_diffs) - Gitlab::GitalyClient::Commit.new(project.repository).diff_from_parent(self, *args) + Gitlab::GitalyClient::CommitService.new(project.repository).diff_from_parent(self, *args) else raw.diffs(*args) end @@ -331,7 +322,7 @@ class Commit def raw_deltas @deltas ||= Gitlab::GitalyClient.migrate(:commit_deltas) do |is_enabled| if is_enabled - Gitlab::GitalyClient::Commit.new(project.repository).commit_deltas(self) + Gitlab::GitalyClient::CommitService.new(project.repository).commit_deltas(self) else raw.deltas end @@ -368,10 +359,6 @@ class Commit end end - def find_author_by_any_email - User.find_by_any_email(author_email.downcase) - end - def repo_changes changes = { added: [], modified: [], removed: [] } diff --git a/app/models/note.rb b/app/models/note.rb index 3d39047d32f..d0e3bc0bfed 100644 --- a/app/models/note.rb +++ b/app/models/note.rb @@ -190,7 +190,7 @@ class Note < ActiveRecord::Base # override to return commits, which are not active record def noteable if for_commit? - project.commit(commit_id) + @commit ||= project.commit(commit_id) else super end diff --git a/app/policies/ci/build_policy.rb b/app/policies/ci/build_policy.rb index 129ed756477..00adb51e7de 100644 --- a/app/policies/ci/build_policy.rb +++ b/app/policies/ci/build_policy.rb @@ -1,9 +1,13 @@ module Ci class BuildPolicy < CommitStatusPolicy condition(:user_cannot_update) do - !::Gitlab::UserAccess - .new(@user, project: @subject.project) - .can_push_or_merge_to_branch?(@subject.ref) + access = ::Gitlab::UserAccess.new(@user, project: @subject.project) + + if @subject.tag? + !access.can_create_tag?(@subject.ref) + else + !access.can_push_or_merge_to_branch?(@subject.ref) + end end rule { user_cannot_update }.prevent :update_build diff --git a/app/views/layouts/nav/_new_group_sidebar.html.haml b/app/views/layouts/nav/_new_group_sidebar.html.haml index 7b68d11041e..c80308ed0de 100644 --- a/app/views/layouts/nav/_new_group_sidebar.html.haml +++ b/app/views/layouts/nav/_new_group_sidebar.html.haml @@ -55,7 +55,22 @@ %span Members - if current_user && can?(current_user, :admin_group, @group) - = nav_link(path: %w[groups#projects groups#edit]) do + = nav_link(path: %w[groups#projects groups#edit ci_cd#show]) do = link_to edit_group_path(@group), title: 'Settings' do %span Settings + %ul.sidebar-sub-level-items + = nav_link(path: 'groups#edit') do + = link_to edit_group_path(@group), title: 'General' do + %span + General + + = nav_link(path: 'groups#projects') do + = link_to projects_group_path(@group), title: 'Projects' do + %span + Projects + + = nav_link(controller: :ci_cd) do + = link_to group_settings_ci_cd_path(@group), title: 'Pipelines' do + %span + Pipelines diff --git a/app/views/layouts/nav/_new_project_sidebar.html.haml b/app/views/layouts/nav/_new_project_sidebar.html.haml index 8838852803b..7c9822c5a6a 100644 --- a/app/views/layouts/nav/_new_project_sidebar.html.haml +++ b/app/views/layouts/nav/_new_project_sidebar.html.haml @@ -165,7 +165,7 @@ Snippets - if project_nav_tab? :settings - = nav_link(path: %w[projects#edit members#show integrations#show services#edit repository#show ci_cd#show pages#show]) do + = nav_link(path: %w[projects#edit project_members#index integrations#show services#edit repository#show ci_cd#show pages#show]) do = link_to edit_project_path(@project), title: 'Settings', class: 'shortcuts-tree' do %span Settings @@ -177,8 +177,8 @@ = link_to edit_project_path(@project), title: 'General' do %span General - = nav_link(controller: :members) do - = link_to project_settings_members_path(@project), title: 'Members' do + = nav_link(controller: :project_members) do + = link_to project_project_members_path(@project), title: 'Members' do %span Members - if can_edit diff --git a/app/workers/project_service_worker.rb b/app/workers/project_service_worker.rb index fdfdeab7b41..4883d848c53 100644 --- a/app/workers/project_service_worker.rb +++ b/app/workers/project_service_worker.rb @@ -2,6 +2,8 @@ class ProjectServiceWorker include Sidekiq::Worker include DedicatedSidekiqQueue + sidekiq_options dead: false + def perform(hook_id, data) data = data.with_indifferent_access Service.find(hook_id).execute(data) diff --git a/app/workers/web_hook_worker.rb b/app/workers/web_hook_worker.rb index ad5ddf02a12..713c0228040 100644 --- a/app/workers/web_hook_worker.rb +++ b/app/workers/web_hook_worker.rb @@ -2,7 +2,7 @@ class WebHookWorker include Sidekiq::Worker include DedicatedSidekiqQueue - sidekiq_options retry: 4 + sidekiq_options retry: 4, dead: false def perform(hook_id, data, hook_name) hook = WebHook.find(hook_id) diff --git a/changelogs/unreleased/29901-refactor-initialization-dropzone_input-js.yml b/changelogs/unreleased/29901-refactor-initialization-dropzone_input-js.yml new file mode 100644 index 00000000000..8850422fc88 --- /dev/null +++ b/changelogs/unreleased/29901-refactor-initialization-dropzone_input-js.yml @@ -0,0 +1,4 @@ +--- +title: refactor initializations in dropzone_input.js +merge_request: 12768 +author: Brandon Everett diff --git a/changelogs/unreleased/31571-don-t-let-webhooks-jobs-go-to-the-dead-jobs-queue.yml b/changelogs/unreleased/31571-don-t-let-webhooks-jobs-go-to-the-dead-jobs-queue.yml new file mode 100644 index 00000000000..69900f0b314 --- /dev/null +++ b/changelogs/unreleased/31571-don-t-let-webhooks-jobs-go-to-the-dead-jobs-queue.yml @@ -0,0 +1,5 @@ +--- +title: Prevent web hook and project service background jobs from going to the dead + jobs queue +merge_request: +author: diff --git a/changelogs/unreleased/34831-remove-coffee-rails-gem.yml b/changelogs/unreleased/34831-remove-coffee-rails-gem.yml new file mode 100644 index 00000000000..b555f112b8d --- /dev/null +++ b/changelogs/unreleased/34831-remove-coffee-rails-gem.yml @@ -0,0 +1,4 @@ +--- +title: Remove coffee-rails gem +merge_request: +author: Takuya Noguchi diff --git a/changelogs/unreleased/34927-protect-manual-actions-on-tags.yml b/changelogs/unreleased/34927-protect-manual-actions-on-tags.yml new file mode 100644 index 00000000000..d996ae2826a --- /dev/null +++ b/changelogs/unreleased/34927-protect-manual-actions-on-tags.yml @@ -0,0 +1,4 @@ +--- +title: Protect manual actions against protected tag too +merge_request: 12908 +author: diff --git a/changelogs/unreleased/35225-transient-poll.yml b/changelogs/unreleased/35225-transient-poll.yml new file mode 100644 index 00000000000..59e2e738c7b --- /dev/null +++ b/changelogs/unreleased/35225-transient-poll.yml @@ -0,0 +1,4 @@ +--- +title: fix transient js error in rspec tests +merge_request: +author: diff --git a/changelogs/unreleased/request-store-wrap.yml b/changelogs/unreleased/request-store-wrap.yml new file mode 100644 index 00000000000..8017054b77b --- /dev/null +++ b/changelogs/unreleased/request-store-wrap.yml @@ -0,0 +1,4 @@ +--- +title: Add RequestCache which makes caching with RequestStore easier +merge_request: 12920 +author: diff --git a/config/initializers/gettext_rails_i18n_patch.rb b/config/initializers/gettext_rails_i18n_patch.rb index 69118f464ca..377e5104f9d 100644 --- a/config/initializers/gettext_rails_i18n_patch.rb +++ b/config/initializers/gettext_rails_i18n_patch.rb @@ -33,7 +33,6 @@ module GettextI18nRailsJs [ ".js", ".jsx", - ".coffee", ".vue" ].include? ::File.extname(file) end diff --git a/doc/update/9.3-to-9.4.md b/doc/update/9.3-to-9.4.md index 6962d124c80..9540c36e7d0 100644 --- a/doc/update/9.3-to-9.4.md +++ b/doc/update/9.3-to-9.4.md @@ -157,8 +157,7 @@ configuration file may contain syntax errors. The block name file, should be `[[storage]]` instead. ```shell -cd /home/git/gitaly -sudo -u git -H editor config.toml +sudo -u git -H sed -i.pre-9.4 's/\[\[storages\]\]/[[storage]]/' /home/git/gitaly/config.toml ``` #### Compile Gitaly diff --git a/lib/api/internal.rb b/lib/api/internal.rb index 465363da582..8b007869dc3 100644 --- a/lib/api/internal.rb +++ b/lib/api/internal.rb @@ -150,7 +150,7 @@ module API # # begin # repository = wiki? ? project.wiki.repository : project.repository - # Gitlab::GitalyClient::Notifications.new(repository.raw_repository).post_receive + # Gitlab::GitalyClient::NotificationService.new(repository.raw_repository).post_receive # rescue GRPC::Unavailable => e # render_api_error!(e, 500) # end diff --git a/lib/declarative_policy.rb b/lib/declarative_policy.rb index d9959bc1aff..b1eb1a6cef1 100644 --- a/lib/declarative_policy.rb +++ b/lib/declarative_policy.rb @@ -8,7 +8,12 @@ require_dependency 'declarative_policy/step' require_dependency 'declarative_policy/base' +require 'thread' + module DeclarativePolicy + CLASS_CACHE_MUTEX = Mutex.new + CLASS_CACHE_IVAR = :@__DeclarativePolicy_CLASS_CACHE + class << self def policy_for(user, subject, opts = {}) cache = opts[:cache] || {} @@ -23,7 +28,36 @@ module DeclarativePolicy subject = find_delegate(subject) - subject.class.ancestors.each do |klass| + class_for_class(subject.class) + end + + private + + # This method is heavily cached because there are a lot of anonymous + # modules in play in a typical rails app, and #name performs quite + # slowly for anonymous classes and modules. + # + # See https://bugs.ruby-lang.org/issues/11119 + # + # if the above bug is resolved, this caching could likely be removed. + def class_for_class(subject_class) + unless subject_class.instance_variable_defined?(CLASS_CACHE_IVAR) + CLASS_CACHE_MUTEX.synchronize do + # re-check in case of a race + break if subject_class.instance_variable_defined?(CLASS_CACHE_IVAR) + + policy_class = compute_class_for_class(subject_class) + subject_class.instance_variable_set(CLASS_CACHE_IVAR, policy_class) + end + end + + policy_class = subject_class.instance_variable_get(CLASS_CACHE_IVAR) + raise "no policy for #{subject.class.name}" if policy_class.nil? + policy_class + end + + def compute_class_for_class(subject_class) + subject_class.ancestors.each do |klass| next unless klass.name begin @@ -37,12 +71,8 @@ module DeclarativePolicy nil end end - - raise "no policy for #{subject.class.name}" end - private - def find_delegate(subject) seen = Set.new diff --git a/lib/declarative_policy/cache.rb b/lib/declarative_policy/cache.rb index b8cc60074c7..0804edba016 100644 --- a/lib/declarative_policy/cache.rb +++ b/lib/declarative_policy/cache.rb @@ -21,11 +21,14 @@ module DeclarativePolicy private def id_for(obj) - if obj.respond_to?(:id) && obj.id - obj.id.to_s - else - "##{obj.object_id}" - end + id = + begin + obj.id + rescue NoMethodError + nil + end + + id || "##{obj.object_id}" end end end diff --git a/lib/declarative_policy/condition.rb b/lib/declarative_policy/condition.rb index 9d7cf6b9726..51c4a8b2bbe 100644 --- a/lib/declarative_policy/condition.rb +++ b/lib/declarative_policy/condition.rb @@ -82,13 +82,14 @@ module DeclarativePolicy # depending on the scope, we may cache only by the user or only by # the subject, resulting in sharing across different policy objects. def cache_key - case @condition.scope - when :normal then "/dp/condition/#{@condition.key}/#{user_key},#{subject_key}" - when :user then "/dp/condition/#{@condition.key}/#{user_key}" - when :subject then "/dp/condition/#{@condition.key}/#{subject_key}" - when :global then "/dp/condition/#{@condition.key}" - else raise 'invalid scope' - end + @cache_key ||= + case @condition.scope + when :normal then "/dp/condition/#{@condition.key}/#{user_key},#{subject_key}" + when :user then "/dp/condition/#{@condition.key}/#{user_key}" + when :subject then "/dp/condition/#{@condition.key}/#{subject_key}" + when :global then "/dp/condition/#{@condition.key}" + else raise 'invalid scope' + end end def user_key diff --git a/lib/gitlab/cache/request_cache.rb b/lib/gitlab/cache/request_cache.rb new file mode 100644 index 00000000000..f1a04affd38 --- /dev/null +++ b/lib/gitlab/cache/request_cache.rb @@ -0,0 +1,94 @@ +module Gitlab + module Cache + # This module provides a simple way to cache values in RequestStore, + # and the cache key would be based on the class name, method name, + # optionally customized instance level values, optionally customized + # method level values, and optional method arguments. + # + # A simple example: + # + # class UserAccess + # extend Gitlab::Cache::RequestCache + # + # request_cache_key do + # [user&.id, project&.id] + # end + # + # request_cache def can_push_to_branch?(ref) + # # ... + # end + # end + # + # This way, the result of `can_push_to_branch?` would be cached in + # `RequestStore.store` based on the cache key. If RequestStore is not + # currently active, then it would be stored in a hash saved in an + # instance variable, so the cache logic would be the same. + # Here's another example using customized method level values: + # + # class Commit + # extend Gitlab::Cache::RequestCache + # + # def author + # User.find_by_any_email(author_email.downcase) + # end + # request_cache(:author) { author_email.downcase } + # end + # + # So that we could have different strategies for different methods + # + module RequestCache + def self.extended(klass) + return if klass < self + + extension = Module.new + klass.const_set(:RequestCacheExtension, extension) + klass.prepend(extension) + end + + def request_cache_key(&block) + if block_given? + @request_cache_key = block + else + @request_cache_key + end + end + + def request_cache(method_name, &method_key_block) + const_get(:RequestCacheExtension).module_eval do + cache_key_method_name = "#{method_name}_cache_key" + + define_method(method_name) do |*args| + store = + if RequestStore.active? + RequestStore.store + else + ivar_name = # ! and ? cannot be used as ivar name + "@cache_#{method_name.to_s.tr('!?', "\u2605\u2606")}" + + instance_variable_get(ivar_name) || + instance_variable_set(ivar_name, {}) + end + + key = __send__(cache_key_method_name, args) + + store.fetch(key) { store[key] = super(*args) } + end + + define_method(cache_key_method_name) do |args| + klass = self.class + + instance_key = instance_exec(&klass.request_cache_key) if + klass.request_cache_key + + method_key = instance_exec(&method_key_block) if method_key_block + + [klass.name, method_name, *instance_key, *method_key, *args] + .join(':') + end + + private cache_key_method_name + end + end + end + end +end diff --git a/lib/gitlab/git/blob.rb b/lib/gitlab/git/blob.rb index b6dd3cd20e0..db6cfc9671f 100644 --- a/lib/gitlab/git/blob.rb +++ b/lib/gitlab/git/blob.rb @@ -29,7 +29,7 @@ module Gitlab path = path.sub(/\A\/*/, '') path = '/' if path.empty? name = File.basename(path) - entry = Gitlab::GitalyClient::Commit.new(repository).tree_entry(sha, path, MAX_DATA_DISPLAY_SIZE) + entry = Gitlab::GitalyClient::CommitService.new(repository).tree_entry(sha, path, MAX_DATA_DISPLAY_SIZE) return unless entry case entry.type @@ -87,10 +87,10 @@ module Gitlab def raw(repository, sha) Gitlab::GitalyClient.migrate(:git_blob_raw) do |is_enabled| if is_enabled - Gitlab::GitalyClient::Blob.new(repository).get_blob(oid: sha, limit: MAX_DATA_DISPLAY_SIZE) + Gitlab::GitalyClient::BlobService.new(repository).get_blob(oid: sha, limit: MAX_DATA_DISPLAY_SIZE) else blob = repository.lookup(sha) - + new( id: blob.oid, size: blob.size, @@ -182,7 +182,7 @@ module Gitlab Gitlab::GitalyClient.migrate(:git_blob_load_all_data) do |is_enabled| @data = begin if is_enabled - Gitlab::GitalyClient::Blob.new(repository).get_blob(oid: id, limit: -1).data + Gitlab::GitalyClient::BlobService.new(repository).get_blob(oid: id, limit: -1).data else repository.lookup(id).content end diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb index 5aefa1404ad..c091bb9dcfe 100644 --- a/lib/gitlab/git/repository.rb +++ b/lib/gitlab/git/repository.rb @@ -1106,11 +1106,11 @@ module Gitlab end def gitaly_ref_client - @gitaly_ref_client ||= Gitlab::GitalyClient::Ref.new(self) + @gitaly_ref_client ||= Gitlab::GitalyClient::RefService.new(self) end def gitaly_commit_client - @gitaly_commit_client ||= Gitlab::GitalyClient::Commit.new(self) + @gitaly_commit_client ||= Gitlab::GitalyClient::CommitService.new(self) end def gitaly_migrate(method, &block) diff --git a/lib/gitlab/gitaly_client/blob.rb b/lib/gitlab/gitaly_client/blob_service.rb index 0c398b46a08..7ea8e8d0857 100644 --- a/lib/gitlab/gitaly_client/blob.rb +++ b/lib/gitlab/gitaly_client/blob_service.rb @@ -1,10 +1,10 @@ module Gitlab module GitalyClient - class Blob + class BlobService def initialize(repository) @gitaly_repo = repository.gitaly_repository end - + def get_blob(oid:, limit:) request = Gitaly::GetBlobRequest.new( repository: @gitaly_repo, diff --git a/lib/gitlab/gitaly_client/commit.rb b/lib/gitlab/gitaly_client/commit_service.rb index aafc0520664..470e3ac8779 100644 --- a/lib/gitlab/gitaly_client/commit.rb +++ b/lib/gitlab/gitaly_client/commit_service.rb @@ -1,6 +1,6 @@ module Gitlab module GitalyClient - class Commit + class CommitService # The ID of empty tree. # See http://stackoverflow.com/a/40884093/1856239 and https://github.com/git/git/blob/3ad8b5bf26362ac67c9020bf8c30eee54a84f56d/cache.h#L1011-L1012 EMPTY_TREE_ID = '4b825dc642cb6eb9a060e54bf8d69288fbee4904'.freeze @@ -17,20 +17,20 @@ module Gitlab child_id: child_id ) - GitalyClient.call(@repository.storage, :commit, :commit_is_ancestor, request).value + GitalyClient.call(@repository.storage, :commit_service, :commit_is_ancestor, request).value end def diff_from_parent(commit, options = {}) request_params = commit_diff_request_params(commit, options) request_params[:ignore_whitespace_change] = options.fetch(:ignore_whitespace_change, false) request = Gitaly::CommitDiffRequest.new(request_params) - response = GitalyClient.call(@repository.storage, :diff, :commit_diff, request) + response = GitalyClient.call(@repository.storage, :diff_service, :commit_diff, request) Gitlab::Git::DiffCollection.new(GitalyClient::DiffStitcher.new(response), options) end def commit_deltas(commit) request = Gitaly::CommitDeltaRequest.new(commit_diff_request_params(commit)) - response = GitalyClient.call(@repository.storage, :diff, :commit_delta, request) + response = GitalyClient.call(@repository.storage, :diff_service, :commit_delta, request) response.flat_map do |msg| msg.deltas.map { |d| Gitlab::Git::Diff.new(d) } end @@ -44,7 +44,7 @@ module Gitlab limit: limit.to_i ) - response = GitalyClient.call(@repository.storage, :commit, :tree_entry, request) + response = GitalyClient.call(@repository.storage, :commit_service, :tree_entry, request) entry = response.first return unless entry.oid.present? diff --git a/lib/gitlab/gitaly_client/notifications.rb b/lib/gitlab/gitaly_client/notification_service.rb index 78ed433e6b8..326e6f7dafc 100644 --- a/lib/gitlab/gitaly_client/notifications.rb +++ b/lib/gitlab/gitaly_client/notification_service.rb @@ -1,6 +1,6 @@ module Gitlab module GitalyClient - class Notifications + class NotificationService # 'repository' is a Gitlab::Git::Repository def initialize(repository) @gitaly_repo = repository.gitaly_repository @@ -10,7 +10,7 @@ module Gitlab def post_receive GitalyClient.call( @storage, - :notifications, + :notification_service, :post_receive, Gitaly::PostReceiveRequest.new(repository: @gitaly_repo) ) diff --git a/lib/gitlab/gitaly_client/ref.rb b/lib/gitlab/gitaly_client/ref_service.rb index 2c17b67d4b7..f541887843d 100644 --- a/lib/gitlab/gitaly_client/ref.rb +++ b/lib/gitlab/gitaly_client/ref_service.rb @@ -1,6 +1,6 @@ module Gitlab module GitalyClient - class Ref + class RefService include Gitlab::EncodingHelper # 'repository' is a Gitlab::Git::Repository @@ -12,19 +12,19 @@ module Gitlab def default_branch_name request = Gitaly::FindDefaultBranchNameRequest.new(repository: @gitaly_repo) - response = GitalyClient.call(@storage, :ref, :find_default_branch_name, request) + response = GitalyClient.call(@storage, :ref_service, :find_default_branch_name, request) Gitlab::Git.branch_name(response.name) end def branch_names request = Gitaly::FindAllBranchNamesRequest.new(repository: @gitaly_repo) - response = GitalyClient.call(@storage, :ref, :find_all_branch_names, request) + response = GitalyClient.call(@storage, :ref_service, :find_all_branch_names, request) consume_refs_response(response) { |name| Gitlab::Git.branch_name(name) } end def tag_names request = Gitaly::FindAllTagNamesRequest.new(repository: @gitaly_repo) - response = GitalyClient.call(@storage, :ref, :find_all_tag_names, request) + response = GitalyClient.call(@storage, :ref_service, :find_all_tag_names, request) consume_refs_response(response) { |name| Gitlab::Git.tag_name(name) } end @@ -34,7 +34,7 @@ module Gitlab commit_id: commit_id, prefix: ref_prefix ) - encode!(GitalyClient.call(@storage, :ref, :find_ref_name, request).name.dup) + encode!(GitalyClient.call(@storage, :ref_service, :find_ref_name, request).name.dup) end def count_tag_names @@ -48,7 +48,7 @@ module Gitlab def local_branches(sort_by: nil) request = Gitaly::FindLocalBranchesRequest.new(repository: @gitaly_repo) request.sort_by = sort_by_param(sort_by) if sort_by - response = GitalyClient.call(@storage, :ref, :find_local_branches, request) + response = GitalyClient.call(@storage, :ref_service, :find_local_branches, request) consume_branches_response(response) end diff --git a/lib/gitlab/performance_bar/peek_query_tracker.rb b/lib/gitlab/performance_bar/peek_query_tracker.rb index 574ae8731a5..f97e895dbd0 100644 --- a/lib/gitlab/performance_bar/peek_query_tracker.rb +++ b/lib/gitlab/performance_bar/peek_query_tracker.rb @@ -1,4 +1,5 @@ # Inspired by https://github.com/peek/peek-pg/blob/master/lib/peek/views/pg.rb +# PEEK_DB_CLIENT is a constant set in config/initializers/peek.rb module Gitlab module PerformanceBar module PeekQueryTracker @@ -23,7 +24,13 @@ module Gitlab subscribe('sql.active_record') do |_, start, finish, _, data| if RequestStore.active? && RequestStore.store[:peek_enabled] - track_query(data[:sql].strip, data[:binds], start, finish) + # data[:cached] is only available starting from Rails 5.1.0 + # https://github.com/rails/rails/blob/v5.1.0/activerecord/lib/active_record/connection_adapters/abstract/query_cache.rb#L113 + # Before that, data[:name] was set to 'CACHE' + # https://github.com/rails/rails/blob/v4.2.9/activerecord/lib/active_record/connection_adapters/abstract/query_cache.rb#L80 + unless data.fetch(:cached, data[:name] == 'CACHE') + track_query(data[:sql].strip, data[:binds], start, finish) + end end end end diff --git a/lib/gitlab/user_access.rb b/lib/gitlab/user_access.rb index d8b043f5021..c63b98500ee 100644 --- a/lib/gitlab/user_access.rb +++ b/lib/gitlab/user_access.rb @@ -1,8 +1,8 @@ module Gitlab class UserAccess - extend Gitlab::Cache::RequestStoreWrap + extend Gitlab::Cache::RequestCache - request_store_wrap_key do + request_cache_key do [user&.id, project&.id] end @@ -34,7 +34,7 @@ module Gitlab true end - def can_create_tag?(ref) + request_cache def can_create_tag?(ref) return false unless can_access_git? if ProtectedTag.protected?(project, ref) @@ -44,7 +44,7 @@ module Gitlab end end - def can_delete_branch?(ref) + request_cache def can_delete_branch?(ref) return false unless can_access_git? if ProtectedBranch.protected?(project, ref) @@ -58,7 +58,7 @@ module Gitlab can_push_to_branch?(ref) || can_merge_to_branch?(ref) end - request_store_wrap def can_push_to_branch?(ref) + request_cache def can_push_to_branch?(ref) return false unless can_access_git? if ProtectedBranch.protected?(project, ref) @@ -70,7 +70,7 @@ module Gitlab end end - request_store_wrap def can_merge_to_branch?(ref) + request_cache def can_merge_to_branch?(ref) return false unless can_access_git? if ProtectedBranch.protected?(project, ref) diff --git a/lib/tasks/gettext.rake b/lib/tasks/gettext.rake index b27f7475115..b48e4dce445 100644 --- a/lib/tasks/gettext.rake +++ b/lib/tasks/gettext.rake @@ -5,7 +5,7 @@ namespace :gettext do # See: https://github.com/grosser/gettext_i18n_rails#customizing-list-of-translatable-files def files_to_translate folders = %W(app lib config #{locale_path}).join(',') - exts = %w(rb erb haml slim rhtml js jsx vue coffee handlebars hbs mustache).join(',') + exts = %w(rb erb haml slim rhtml js jsx vue handlebars hbs mustache).join(',') Dir.glob( "{#{folders}}/**/*.{#{exts}}" diff --git a/spec/factories/commits.rb b/spec/factories/commits.rb index 36b9645438a..89e260cf65b 100644 --- a/spec/factories/commits.rb +++ b/spec/factories/commits.rb @@ -4,14 +4,19 @@ FactoryGirl.define do factory :commit do git_commit RepoHelpers.sample_commit project factory: :empty_project - author { build(:author) } initialize_with do new(git_commit, project) end + after(:build) do |commit| + allow(commit).to receive(:author).and_return build(:author) + end + trait :without_author do - author nil + after(:build) do |commit| + allow(commit).to receive(:author).and_return nil + end end end end diff --git a/spec/features/participants_autocomplete_spec.rb b/spec/features/participants_autocomplete_spec.rb index 382d83ca051..81b0a2f541b 100644 --- a/spec/features/participants_autocomplete_spec.rb +++ b/spec/features/participants_autocomplete_spec.rb @@ -54,7 +54,8 @@ feature 'Member autocomplete', :js do let(:note) { create(:note_on_commit, project: project, commit_id: project.commit.id) } before do - allow_any_instance_of(Commit).to receive(:author).and_return(author) + allow(User).to receive(:find_by_any_email) + .with(noteable.author_email.downcase).and_return(author) visit project_commit_path(project, noteable) end diff --git a/spec/javascripts/lib/utils/poll_spec.js b/spec/javascripts/lib/utils/poll_spec.js index 22f30191ab9..2aa7011ca51 100644 --- a/spec/javascripts/lib/utils/poll_spec.js +++ b/spec/javascripts/lib/utils/poll_spec.js @@ -25,23 +25,28 @@ function mockServiceCall(service, response, shouldFail = false) { describe('Poll', () => { const service = jasmine.createSpyObj('service', ['fetch']); - const callbacks = jasmine.createSpyObj('callbacks', ['success', 'error']); + const callbacks = jasmine.createSpyObj('callbacks', ['success', 'error', 'notification']); + + function setup() { + return new Poll({ + resource: service, + method: 'fetch', + successCallback: callbacks.success, + errorCallback: callbacks.error, + notificationCallback: callbacks.notification, + }).makeRequest(); + } afterEach(() => { callbacks.success.calls.reset(); callbacks.error.calls.reset(); + callbacks.notification.calls.reset(); service.fetch.calls.reset(); }); it('calls the success callback when no header for interval is provided', (done) => { mockServiceCall(service, { status: 200 }); - - new Poll({ - resource: service, - method: 'fetch', - successCallback: callbacks.success, - errorCallback: callbacks.error, - }).makeRequest(); + setup(); waitForAllCallsToFinish(service, 1, () => { expect(callbacks.success).toHaveBeenCalled(); @@ -51,15 +56,9 @@ describe('Poll', () => { }); }); - it('calls the error callback whe the http request returns an error', (done) => { + it('calls the error callback when the http request returns an error', (done) => { mockServiceCall(service, { status: 500 }, true); - - new Poll({ - resource: service, - method: 'fetch', - successCallback: callbacks.success, - errorCallback: callbacks.error, - }).makeRequest(); + setup(); waitForAllCallsToFinish(service, 1, () => { expect(callbacks.success).not.toHaveBeenCalled(); @@ -69,15 +68,22 @@ describe('Poll', () => { }); }); + it('skips the error callback when request is aborted', (done) => { + mockServiceCall(service, { status: 0 }, true); + setup(); + + waitForAllCallsToFinish(service, 1, () => { + expect(callbacks.success).not.toHaveBeenCalled(); + expect(callbacks.error).not.toHaveBeenCalled(); + expect(callbacks.notification).toHaveBeenCalled(); + + done(); + }); + }); + it('should call the success callback when the interval header is -1', (done) => { mockServiceCall(service, { status: 200, headers: { 'poll-interval': -1 } }); - - new Poll({ - resource: service, - method: 'fetch', - successCallback: callbacks.success, - errorCallback: callbacks.error, - }).makeRequest().then(() => { + setup().then(() => { expect(callbacks.success).toHaveBeenCalled(); expect(callbacks.error).not.toHaveBeenCalled(); diff --git a/spec/lib/gitlab/cache/request_cache_spec.rb b/spec/lib/gitlab/cache/request_cache_spec.rb new file mode 100644 index 00000000000..5b82c216a13 --- /dev/null +++ b/spec/lib/gitlab/cache/request_cache_spec.rb @@ -0,0 +1,133 @@ +require 'spec_helper' + +describe Gitlab::Cache::RequestCache do + let(:klass) do + Class.new do + extend Gitlab::Cache::RequestCache + + attr_accessor :id, :name, :result, :extra + + def self.name + 'ExpensiveAlgorithm' + end + + def initialize(id, name, result, extra = nil) + self.id = id + self.name = name + self.result = result + self.extra = nil + end + + request_cache def compute(arg) + result << arg + end + + request_cache def repute(arg) + result << arg + end + + def dispute(arg) + result << arg + end + request_cache(:dispute) { extra } + end + end + + let(:algorithm) { klass.new('id', 'name', []) } + + shared_examples 'cache for the same instance' do + it 'does not compute twice for the same argument' do + algorithm.compute(true) + result = algorithm.compute(true) + + expect(result).to eq([true]) + end + + it 'computes twice for the different argument' do + algorithm.compute(true) + result = algorithm.compute(false) + + expect(result).to eq([true, false]) + end + + it 'computes twice for the different class name' do + algorithm.compute(true) + allow(klass).to receive(:name).and_return('CheapAlgo') + result = algorithm.compute(true) + + expect(result).to eq([true, true]) + end + + it 'computes twice for the different method' do + algorithm.compute(true) + result = algorithm.repute(true) + + expect(result).to eq([true, true]) + end + + context 'when request_cache_key is provided' do + before do + klass.request_cache_key do + [id, name] + end + end + + it 'computes twice for the different keys, id' do + algorithm.compute(true) + algorithm.id = 'ad' + result = algorithm.compute(true) + + expect(result).to eq([true, true]) + end + + it 'computes twice for the different keys, name' do + algorithm.compute(true) + algorithm.name = 'same' + result = algorithm.compute(true) + + expect(result).to eq([true, true]) + end + + it 'uses extra method cache key if provided' do + algorithm.dispute(true) # miss + algorithm.extra = true + algorithm.dispute(true) # miss + result = algorithm.dispute(true) # hit + + expect(result).to eq([true, true]) + end + end + end + + context 'when RequestStore is active', :request_store do + it_behaves_like 'cache for the same instance' + + it 'computes once for different instances when keys are the same' do + algorithm.compute(true) + result = klass.new('id', 'name', algorithm.result).compute(true) + + expect(result).to eq([true]) + end + + it 'computes twice if RequestStore starts over' do + algorithm.compute(true) + RequestStore.end! + RequestStore.clear! + RequestStore.begin! + result = algorithm.compute(true) + + expect(result).to eq([true, true]) + end + end + + context 'when RequestStore is inactive' do + it_behaves_like 'cache for the same instance' + + it 'computes twice for different instances even if keys are the same' do + algorithm.compute(true) + result = klass.new('id', 'name', algorithm.result).compute(true) + + expect(result).to eq([true, true]) + end + end +end diff --git a/spec/lib/gitlab/git/repository_spec.rb b/spec/lib/gitlab/git/repository_spec.rb index 3eeed6126a0..83d067b2c31 100644 --- a/spec/lib/gitlab/git/repository_spec.rb +++ b/spec/lib/gitlab/git/repository_spec.rb @@ -45,11 +45,11 @@ describe Gitlab::Git::Repository, seed_helper: true do end it 'gets the branch name from GitalyClient' do - expect_any_instance_of(Gitlab::GitalyClient::Ref).to receive(:default_branch_name) + expect_any_instance_of(Gitlab::GitalyClient::RefService).to receive(:default_branch_name) repository.root_ref end - it_behaves_like 'wrapping gRPC errors', Gitlab::GitalyClient::Ref, :default_branch_name do + it_behaves_like 'wrapping gRPC errors', Gitlab::GitalyClient::RefService, :default_branch_name do subject { repository.root_ref } end end @@ -132,11 +132,11 @@ describe Gitlab::Git::Repository, seed_helper: true do it { is_expected.not_to include("branch-from-space") } it 'gets the branch names from GitalyClient' do - expect_any_instance_of(Gitlab::GitalyClient::Ref).to receive(:branch_names) + expect_any_instance_of(Gitlab::GitalyClient::RefService).to receive(:branch_names) subject end - it_behaves_like 'wrapping gRPC errors', Gitlab::GitalyClient::Ref, :branch_names + it_behaves_like 'wrapping gRPC errors', Gitlab::GitalyClient::RefService, :branch_names end describe '#tag_names' do @@ -160,11 +160,11 @@ describe Gitlab::Git::Repository, seed_helper: true do it { is_expected.not_to include("v5.0.0") } it 'gets the tag names from GitalyClient' do - expect_any_instance_of(Gitlab::GitalyClient::Ref).to receive(:tag_names) + expect_any_instance_of(Gitlab::GitalyClient::RefService).to receive(:tag_names) subject end - it_behaves_like 'wrapping gRPC errors', Gitlab::GitalyClient::Ref, :tag_names + it_behaves_like 'wrapping gRPC errors', Gitlab::GitalyClient::RefService, :tag_names end shared_examples 'archive check' do |extenstion| @@ -368,7 +368,7 @@ describe Gitlab::Git::Repository, seed_helper: true do context 'when Gitaly commit_count feature is enabled' do it_behaves_like 'counting commits' - it_behaves_like 'wrapping gRPC errors', Gitlab::GitalyClient::Commit, :commit_count do + it_behaves_like 'wrapping gRPC errors', Gitlab::GitalyClient::CommitService, :commit_count do subject { repository.commit_count('master') } end end @@ -1225,12 +1225,12 @@ describe Gitlab::Git::Repository, seed_helper: true do end it 'gets the branches from GitalyClient' do - expect_any_instance_of(Gitlab::GitalyClient::Ref).to receive(:local_branches) + expect_any_instance_of(Gitlab::GitalyClient::RefService).to receive(:local_branches) .and_return([]) @repo.local_branches end - it_behaves_like 'wrapping gRPC errors', Gitlab::GitalyClient::Ref, :local_branches do + it_behaves_like 'wrapping gRPC errors', Gitlab::GitalyClient::RefService, :local_branches do subject { @repo.local_branches } end end diff --git a/spec/lib/gitlab/gitaly_client/commit_spec.rb b/spec/lib/gitlab/gitaly_client/commit_service_spec.rb index dff5b25c712..fee5bb45fe5 100644 --- a/spec/lib/gitlab/gitaly_client/commit_spec.rb +++ b/spec/lib/gitlab/gitaly_client/commit_service_spec.rb @@ -1,7 +1,7 @@ require 'spec_helper' -describe Gitlab::GitalyClient::Commit do - let(:diff_stub) { double('Gitaly::Diff::Stub') } +describe Gitlab::GitalyClient::CommitService do + let(:diff_stub) { double('Gitaly::DiffService::Stub') } let(:project) { create(:project, :repository) } let(:repository) { project.repository } let(:repository_message) { repository.gitaly_repository } @@ -16,7 +16,7 @@ describe Gitlab::GitalyClient::Commit do right_commit_id: commit.id ) - expect_any_instance_of(Gitaly::Diff::Stub).to receive(:commit_diff).with(request, kind_of(Hash)) + expect_any_instance_of(Gitaly::DiffService::Stub).to receive(:commit_diff).with(request, kind_of(Hash)) described_class.new(repository).diff_from_parent(commit) end @@ -31,7 +31,7 @@ describe Gitlab::GitalyClient::Commit do right_commit_id: initial_commit.id ) - expect_any_instance_of(Gitaly::Diff::Stub).to receive(:commit_diff).with(request, kind_of(Hash)) + expect_any_instance_of(Gitaly::DiffService::Stub).to receive(:commit_diff).with(request, kind_of(Hash)) described_class.new(repository).diff_from_parent(initial_commit) end @@ -61,7 +61,7 @@ describe Gitlab::GitalyClient::Commit do right_commit_id: commit.id ) - expect_any_instance_of(Gitaly::Diff::Stub).to receive(:commit_delta).with(request, kind_of(Hash)).and_return([]) + expect_any_instance_of(Gitaly::DiffService::Stub).to receive(:commit_delta).with(request, kind_of(Hash)).and_return([]) described_class.new(repository).commit_deltas(commit) end @@ -76,7 +76,7 @@ describe Gitlab::GitalyClient::Commit do right_commit_id: initial_commit.id ) - expect_any_instance_of(Gitaly::Diff::Stub).to receive(:commit_delta).with(request, kind_of(Hash)).and_return([]) + expect_any_instance_of(Gitaly::DiffService::Stub).to receive(:commit_delta).with(request, kind_of(Hash)).and_return([]) described_class.new(repository).commit_deltas(initial_commit) end diff --git a/spec/lib/gitlab/gitaly_client/notifications_spec.rb b/spec/lib/gitlab/gitaly_client/notification_service_spec.rb index 7404ffe0f06..d9597c4aa78 100644 --- a/spec/lib/gitlab/gitaly_client/notifications_spec.rb +++ b/spec/lib/gitlab/gitaly_client/notification_service_spec.rb @@ -1,6 +1,6 @@ require 'spec_helper' -describe Gitlab::GitalyClient::Notifications do +describe Gitlab::GitalyClient::NotificationService do describe '#post_receive' do let(:project) { create(:empty_project) } let(:storage_name) { project.repository_storage } @@ -8,7 +8,7 @@ describe Gitlab::GitalyClient::Notifications do subject { described_class.new(project.repository) } it 'sends a post_receive message' do - expect_any_instance_of(Gitaly::Notifications::Stub) + expect_any_instance_of(Gitaly::NotificationService::Stub) .to receive(:post_receive).with(gitaly_request_with_path(storage_name, relative_path), kind_of(Hash)) subject.post_receive diff --git a/spec/lib/gitlab/gitaly_client/ref_spec.rb b/spec/lib/gitlab/gitaly_client/ref_service_spec.rb index 7c090460764..1e8ed9d645b 100644 --- a/spec/lib/gitlab/gitaly_client/ref_spec.rb +++ b/spec/lib/gitlab/gitaly_client/ref_service_spec.rb @@ -1,6 +1,6 @@ require 'spec_helper' -describe Gitlab::GitalyClient::Ref do +describe Gitlab::GitalyClient::RefService do let(:project) { create(:empty_project) } let(:storage_name) { project.repository_storage } let(:relative_path) { project.path_with_namespace + '.git' } @@ -8,7 +8,7 @@ describe Gitlab::GitalyClient::Ref do describe '#branch_names' do it 'sends a find_all_branch_names message' do - expect_any_instance_of(Gitaly::Ref::Stub) + expect_any_instance_of(Gitaly::RefService::Stub) .to receive(:find_all_branch_names) .with(gitaly_request_with_path(storage_name, relative_path), kind_of(Hash)) .and_return([]) @@ -19,7 +19,7 @@ describe Gitlab::GitalyClient::Ref do describe '#tag_names' do it 'sends a find_all_tag_names message' do - expect_any_instance_of(Gitaly::Ref::Stub) + expect_any_instance_of(Gitaly::RefService::Stub) .to receive(:find_all_tag_names) .with(gitaly_request_with_path(storage_name, relative_path), kind_of(Hash)) .and_return([]) @@ -30,7 +30,7 @@ describe Gitlab::GitalyClient::Ref do describe '#default_branch_name' do it 'sends a find_default_branch_name message' do - expect_any_instance_of(Gitaly::Ref::Stub) + expect_any_instance_of(Gitaly::RefService::Stub) .to receive(:find_default_branch_name) .with(gitaly_request_with_path(storage_name, relative_path), kind_of(Hash)) .and_return(double(name: 'foo')) @@ -41,7 +41,7 @@ describe Gitlab::GitalyClient::Ref do describe '#local_branches' do it 'sends a find_local_branches message' do - expect_any_instance_of(Gitaly::Ref::Stub) + expect_any_instance_of(Gitaly::RefService::Stub) .to receive(:find_local_branches) .with(gitaly_request_with_path(storage_name, relative_path), kind_of(Hash)) .and_return([]) @@ -50,7 +50,7 @@ describe Gitlab::GitalyClient::Ref do end it 'parses and sends the sort parameter' do - expect_any_instance_of(Gitaly::Ref::Stub) + expect_any_instance_of(Gitaly::RefService::Stub) .to receive(:find_local_branches) .with(gitaly_request_with_params(sort_by: :UPDATED_DESC), kind_of(Hash)) .and_return([]) @@ -59,7 +59,7 @@ describe Gitlab::GitalyClient::Ref do end it 'translates known mismatches on sort param values' do - expect_any_instance_of(Gitaly::Ref::Stub) + expect_any_instance_of(Gitaly::RefService::Stub) .to receive(:find_local_branches) .with(gitaly_request_with_params(sort_by: :NAME), kind_of(Hash)) .and_return([]) diff --git a/spec/lib/gitlab/gitaly_client_spec.rb b/spec/lib/gitlab/gitaly_client_spec.rb index ce7b18b784a..558ddb3fbd6 100644 --- a/spec/lib/gitlab/gitaly_client_spec.rb +++ b/spec/lib/gitlab/gitaly_client_spec.rb @@ -16,9 +16,9 @@ describe Gitlab::GitalyClient, lib: true, skip_gitaly_mock: true do 'default' => { 'gitaly_address' => address } }) - expect(Gitaly::Commit::Stub).to receive(:new).with(address, any_args) + expect(Gitaly::CommitService::Stub).to receive(:new).with(address, any_args) - described_class.stub(:commit, 'default') + described_class.stub(:commit_service, 'default') end end @@ -31,9 +31,9 @@ describe Gitlab::GitalyClient, lib: true, skip_gitaly_mock: true do 'default' => { 'gitaly_address' => prefixed_address } }) - expect(Gitaly::Commit::Stub).to receive(:new).with(address, any_args) + expect(Gitaly::CommitService::Stub).to receive(:new).with(address, any_args) - described_class.stub(:commit, 'default') + described_class.stub(:commit_service, 'default') end end end diff --git a/spec/models/commit_spec.rb b/spec/models/commit_spec.rb index 6056d78da4e..528b211c9d6 100644 --- a/spec/models/commit_spec.rb +++ b/spec/models/commit_spec.rb @@ -19,17 +19,15 @@ describe Commit, models: true do expect(commit.author).to eq(user) end - it 'caches the author' do - allow(RequestStore).to receive(:active?).and_return(true) + it 'caches the author', :request_store do user = create(:user, email: commit.author_email) - expect_any_instance_of(Commit).to receive(:find_author_by_any_email).and_call_original + expect(User).to receive(:find_by_any_email).and_call_original expect(commit.author).to eq(user) - key = "commit_author:#{commit.author_email}" + key = "Commit:author:#{commit.author_email.downcase}" expect(RequestStore.store[key]).to eq(user) expect(commit.author).to eq(user) - RequestStore.store.clear end end diff --git a/spec/policies/ci/build_policy_spec.rb b/spec/policies/ci/build_policy_spec.rb index 9e2b0506bf3..86e57fdf607 100644 --- a/spec/policies/ci/build_policy_spec.rb +++ b/spec/policies/ci/build_policy_spec.rb @@ -115,14 +115,6 @@ describe Ci::BuildPolicy, :models do end context 'when developers can push to the branch' do - let(:branch_policy) { :developers_can_push } - - it 'includes ability to update build' do - expect(policy).to be_allowed :update_build - end - end - - context 'when developers can push to the branch' do let(:branch_policy) { :developers_can_merge } it 'includes ability to update build' do diff --git a/spec/requests/api/internal_spec.rb b/spec/requests/api/internal_spec.rb index beaaf346283..cab3089c6b1 100644 --- a/spec/requests/api/internal_spec.rb +++ b/spec/requests/api/internal_spec.rb @@ -594,10 +594,10 @@ describe API::Internal do # end # # it "calls the Gitaly client with the project's repository" do - # expect(Gitlab::GitalyClient::Notifications). + # expect(Gitlab::GitalyClient::NotificationService). # to receive(:new).with(gitlab_git_repository_with(path: project.repository.path)). # and_call_original - # expect_any_instance_of(Gitlab::GitalyClient::Notifications). + # expect_any_instance_of(Gitlab::GitalyClient::NotificationService). # to receive(:post_receive) # # post api("/internal/notify_post_receive"), valid_params @@ -606,10 +606,10 @@ describe API::Internal do # end # # it "calls the Gitaly client with the wiki's repository if it's a wiki" do - # expect(Gitlab::GitalyClient::Notifications). + # expect(Gitlab::GitalyClient::NotificationService). # to receive(:new).with(gitlab_git_repository_with(path: project.wiki.repository.path)). # and_call_original - # expect_any_instance_of(Gitlab::GitalyClient::Notifications). + # expect_any_instance_of(Gitlab::GitalyClient::NotificationService). # to receive(:post_receive) # # post api("/internal/notify_post_receive"), valid_wiki_params @@ -618,7 +618,7 @@ describe API::Internal do # end # # it "returns 500 if the gitaly call fails" do - # expect_any_instance_of(Gitlab::GitalyClient::Notifications). + # expect_any_instance_of(Gitlab::GitalyClient::NotificationService). # to receive(:post_receive).and_raise(GRPC::Unavailable) # # post api("/internal/notify_post_receive"), valid_params @@ -636,10 +636,10 @@ describe API::Internal do # end # # it "calls the Gitaly client with the project's repository" do - # expect(Gitlab::GitalyClient::Notifications). + # expect(Gitlab::GitalyClient::NotificationService). # to receive(:new).with(gitlab_git_repository_with(path: project.repository.path)). # and_call_original - # expect_any_instance_of(Gitlab::GitalyClient::Notifications). + # expect_any_instance_of(Gitlab::GitalyClient::NotificationService). # to receive(:post_receive) # # post api("/internal/notify_post_receive"), valid_params @@ -648,10 +648,10 @@ describe API::Internal do # end # # it "calls the Gitaly client with the wiki's repository if it's a wiki" do - # expect(Gitlab::GitalyClient::Notifications). + # expect(Gitlab::GitalyClient::NotificationService). # to receive(:new).with(gitlab_git_repository_with(path: project.wiki.repository.path)). # and_call_original - # expect_any_instance_of(Gitlab::GitalyClient::Notifications). + # expect_any_instance_of(Gitlab::GitalyClient::NotificationService). # to receive(:post_receive) # # post api("/internal/notify_post_receive"), valid_wiki_params diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb index f1e00c1163b..4fc5eb0a527 100644 --- a/spec/services/notification_service_spec.rb +++ b/spec/services/notification_service_spec.rb @@ -383,7 +383,7 @@ describe NotificationService, services: true do before do build_team(note.project) reset_delivered_emails! - allow_any_instance_of(Commit).to receive(:author).and_return(@u_committer) + allow(note.noteable).to receive(:author).and_return(@u_committer) update_custom_notification(:new_note, @u_guest_custom, resource: project) update_custom_notification(:new_note, @u_custom_global) end |