diff options
62 files changed, 1529 insertions, 169 deletions
diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 99fab68d24c..379da90827a 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -463,7 +463,7 @@ class ApplicationController < ActionController::Base feature_category: feature_category) do yield ensure - @current_context = Gitlab::ApplicationContext.current + @current_context = Labkit::Context.current.to_h end end diff --git a/app/graphql/mutations/admin/sidekiq_queues/delete_jobs.rb b/app/graphql/mutations/admin/sidekiq_queues/delete_jobs.rb index ea1502d4b62..32ca6de9b96 100644 --- a/app/graphql/mutations/admin/sidekiq_queues/delete_jobs.rb +++ b/app/graphql/mutations/admin/sidekiq_queues/delete_jobs.rb @@ -8,7 +8,7 @@ module Mutations ADMIN_MESSAGE = 'You must be an admin to use this mutation' - Gitlab::ApplicationContext::KNOWN_KEYS.each do |key| + Labkit::Context::KNOWN_KEYS.each do |key| argument key, GraphQL::STRING_TYPE, required: false, diff --git a/app/models/ci/group_variable.rb b/app/models/ci/group_variable.rb index 1e1dd68ee6c..2928ce801ad 100644 --- a/app/models/ci/group_variable.rb +++ b/app/models/ci/group_variable.rb @@ -6,16 +6,18 @@ module Ci include Ci::HasVariable include Presentable include Ci::Maskable + prepend HasEnvironmentScope belongs_to :group, class_name: "::Group" alias_attribute :secret_value, :value validates :key, uniqueness: { - scope: :group_id, + scope: [:group_id, :environment_scope], message: "(%{value}) has already been taken" } scope :unprotected, -> { where(protected: false) } + scope :by_environment_scope, -> (environment_scope) { where(environment_scope: environment_scope) } end end diff --git a/app/models/ci/variable.rb b/app/models/ci/variable.rb index 13358b95a47..84505befc5c 100644 --- a/app/models/ci/variable.rb +++ b/app/models/ci/variable.rb @@ -18,7 +18,6 @@ module Ci } scope :unprotected, -> { where(protected: false) } - scope :by_key, -> (key) { where(key: key) } scope :by_environment_scope, -> (environment_scope) { where(environment_scope: environment_scope) } end end diff --git a/app/models/concerns/ci/contextable.rb b/app/models/concerns/ci/contextable.rb index ddf4c607804..bdba2d3e251 100644 --- a/app/models/concerns/ci/contextable.rb +++ b/app/models/concerns/ci/contextable.rb @@ -20,7 +20,7 @@ module Ci variables.concat(user_variables) variables.concat(dependency_variables) if dependencies variables.concat(secret_instance_variables) - variables.concat(secret_group_variables) + variables.concat(secret_group_variables(environment: environment)) variables.concat(secret_project_variables(environment: environment)) variables.concat(trigger_request.user_variables) if trigger_request variables.concat(pipeline.variables) @@ -85,13 +85,13 @@ module Ci project.ci_instance_variables_for(ref: git_ref) end - def secret_group_variables + def secret_group_variables(environment: expanded_environment_name) return [] unless project.group - project.group.ci_variables_for(git_ref, project) + project.group.ci_variables_for(git_ref, project, environment: environment) end - def secret_project_variables(environment: persisted_environment) + def secret_project_variables(environment: expanded_environment_name) project.ci_variables_for(ref: git_ref, environment: environment) end diff --git a/app/models/concerns/ci/has_variable.rb b/app/models/concerns/ci/has_variable.rb index 9bf2b409080..7309469c77e 100644 --- a/app/models/concerns/ci/has_variable.rb +++ b/app/models/concerns/ci/has_variable.rb @@ -16,6 +16,7 @@ module Ci format: { with: /\A[a-zA-Z0-9_]+\z/, message: "can contain only letters, digits and '_'." } + scope :by_key, -> (key) { where(key: key) } scope :order_key_asc, -> { reorder(key: :asc) } attr_encrypted :value, diff --git a/app/models/group.rb b/app/models/group.rb index ff51865a118..ba0d70b9c13 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -525,15 +525,11 @@ class Group < Namespace } end - def ci_variables_for(ref, project) - cache_key = "ci_variables_for:group:#{self&.id}:project:#{project&.id}:ref:#{ref}" + def ci_variables_for(ref, project, environment: nil) + cache_key = "ci_variables_for:group:#{self&.id}:project:#{project&.id}:ref:#{ref}:environment:#{environment}" ::Gitlab::SafeRequestStore.fetch(cache_key) do - list_of_ids = [self] + ancestors - variables = Ci::GroupVariable.where(group: list_of_ids) - variables = variables.unprotected unless project.protected_for?(ref) - variables = variables.group_by(&:group_id) - list_of_ids.reverse.flat_map { |group| variables[group.id] }.compact + uncached_ci_variables_for(ref, project, environment: environment) end end @@ -775,6 +771,23 @@ class Group < Namespace def enable_shared_runners! update!(shared_runners_enabled: true) end + + def uncached_ci_variables_for(ref, project, environment: nil) + list_of_ids = [self] + ancestors + variables = Ci::GroupVariable.where(group: list_of_ids) + variables = variables.unprotected unless project.protected_for?(ref) + + if Feature.enabled?(:scoped_group_variables, self, default_enabled: :yaml) + variables = if environment + variables.on_environment(environment) + else + variables.where(environment_scope: '*') + end + end + + variables = variables.group_by(&:group_id) + list_of_ids.reverse.flat_map { |group| variables[group.id] }.compact + end end Group.prepend_if_ee('EE::Group') diff --git a/app/models/packages/package.rb b/app/models/packages/package.rb index 84cfa94dbfb..36971dc04c5 100644 --- a/app/models/packages/package.rb +++ b/app/models/packages/package.rb @@ -224,6 +224,12 @@ class Packages::Package < ApplicationRecord end end + def sync_maven_metadata(user) + return unless maven? && version? && user + + ::Packages::Maven::Metadata::SyncWorker.perform_async(user.id, project.id, name) + end + private def composer_tag_version? diff --git a/app/services/packages/maven/find_or_create_package_service.rb b/app/services/packages/maven/find_or_create_package_service.rb index 4c916d264a7..401e52f7e51 100644 --- a/app/services/packages/maven/find_or_create_package_service.rb +++ b/app/services/packages/maven/find_or_create_package_service.rb @@ -2,7 +2,6 @@ module Packages module Maven class FindOrCreatePackageService < BaseService - MAVEN_METADATA_FILE = 'maven-metadata.xml' SNAPSHOT_TERM = '-SNAPSHOT' def execute @@ -33,7 +32,7 @@ module Packages # - my-company/my-app/maven-metadata.xml # # The first upload has to create the proper package (the one with the version set). - if params[:file_name] == MAVEN_METADATA_FILE && !params[:path]&.ends_with?(SNAPSHOT_TERM) + if params[:file_name] == Packages::Maven::Metadata.filename && !params[:path]&.ends_with?(SNAPSHOT_TERM) package_name, version = params[:path], nil else package_name, _, version = params[:path].rpartition('/') diff --git a/app/services/packages/maven/metadata.rb b/app/services/packages/maven/metadata.rb new file mode 100644 index 00000000000..437e18e3138 --- /dev/null +++ b/app/services/packages/maven/metadata.rb @@ -0,0 +1,13 @@ +# frozen_string_literal: true + +module Packages + module Maven + module Metadata + FILENAME = 'maven-metadata.xml' + + def self.filename + FILENAME + end + end + end +end diff --git a/app/services/packages/maven/metadata/append_package_file_service.rb b/app/services/packages/maven/metadata/append_package_file_service.rb new file mode 100644 index 00000000000..e991576ebc6 --- /dev/null +++ b/app/services/packages/maven/metadata/append_package_file_service.rb @@ -0,0 +1,88 @@ +# frozen_string_literal: true + +module Packages + module Maven + module Metadata + class AppendPackageFileService + XML_CONTENT_TYPE = 'application/xml' + DEFAULT_CONTENT_TYPE = 'application/octet-stream' + + MD5_FILE_NAME = "#{Metadata.filename}.md5" + SHA1_FILE_NAME = "#{Metadata.filename}.sha1" + SHA256_FILE_NAME = "#{Metadata.filename}.sha256" + SHA512_FILE_NAME = "#{Metadata.filename}.sha512" + + def initialize(package:, metadata_content:) + @package = package + @metadata_content = metadata_content + end + + def execute + return ServiceResponse.error(message: 'package is not set') unless @package + return ServiceResponse.error(message: 'metadata content is not set') unless @metadata_content + + file_md5 = digest_from(@metadata_content, :md5) + file_sha1 = digest_from(@metadata_content, :sha1) + file_sha256 = digest_from(@metadata_content, :sha256) + file_sha512 = digest_from(@metadata_content, :sha512) + + @package.transaction do + append_metadata_file( + content: @metadata_content, + file_name: Metadata.filename, + content_type: XML_CONTENT_TYPE, + sha1: file_sha1, + md5: file_md5, + sha256: file_sha256 + ) + + append_metadata_file(content: file_md5, file_name: MD5_FILE_NAME) + append_metadata_file(content: file_sha1, file_name: SHA1_FILE_NAME) + append_metadata_file(content: file_sha256, file_name: SHA256_FILE_NAME) + append_metadata_file(content: file_sha512, file_name: SHA512_FILE_NAME) + end + + ServiceResponse.success(message: 'New metadata package file created') + end + + private + + def append_metadata_file(content:, file_name:, content_type: DEFAULT_CONTENT_TYPE, sha1: nil, md5: nil, sha256: nil) + file_md5 = md5 || digest_from(content, :md5) + file_sha1 = sha1 || digest_from(content, :sha1) + file_sha256 = sha256 || digest_from(content, :sha256) + + file = CarrierWaveStringFile.new_file( + file_content: content, + filename: file_name, + content_type: content_type + ) + + ::Packages::CreatePackageFileService.new( + @package, + file: file, + size: file.size, + file_name: file_name, + file_sha1: file_sha1, + file_md5: file_md5, + file_sha256: file_sha256 + ).execute + end + + def digest_from(content, type) + digest_class = case type + when :md5 + Digest::MD5 + when :sha1 + Digest::SHA1 + when :sha256 + Digest::SHA256 + when :sha512 + Digest::SHA512 + end + digest_class.hexdigest(content) + end + end + end + end +end diff --git a/app/services/packages/maven/metadata/create_versions_xml_service.rb b/app/services/packages/maven/metadata/create_versions_xml_service.rb new file mode 100644 index 00000000000..6aebc1560c6 --- /dev/null +++ b/app/services/packages/maven/metadata/create_versions_xml_service.rb @@ -0,0 +1,186 @@ +# frozen_string_literal: true + +module Packages + module Maven + module Metadata + class CreateVersionsXmlService + include Gitlab::Utils::StrongMemoize + + XPATH_VERSIONING = '//metadata/versioning' + XPATH_VERSIONS = '//versions' + XPATH_VERSION = '//version' + XPATH_LATEST = '//latest' + XPATH_RELEASE = '//release' + XPATH_LAST_UPDATED = '//lastUpdated' + + INDENT_SPACE = 2 + + EMPTY_VERSIONS_PAYLOAD = { + changes_exist: true, + empty_versions: true + }.freeze + + def initialize(metadata_content:, package:) + @metadata_content = metadata_content + @package = package + end + + def execute + return ServiceResponse.error(message: 'package not set') unless @package + return ServiceResponse.error(message: 'metadata_content not set') unless @metadata_content + return ServiceResponse.error(message: 'metadata_content is invalid') unless valid_metadata_content? + return ServiceResponse.success(payload: EMPTY_VERSIONS_PAYLOAD) if versions_from_database.empty? + + changes_exist = false + changes_exist = true if update_versions_list + changes_exist = true if update_latest + changes_exist = true if update_release + update_last_updated_timestamp if changes_exist + + payload = { changes_exist: changes_exist, empty_versions: false } + payload[:metadata_content] = xml_doc.to_xml(indent: INDENT_SPACE) if changes_exist + + ServiceResponse.success(payload: payload) + end + + private + + def valid_metadata_content? + versioning_xml_node.present? && + versions_xml_node.present? && + last_updated_xml_node.present? + end + + def update_versions_list + return false if versions_from_xml == versions_from_database + + version_xml_nodes.remove + + versions_from_database.each do |version| + versions_xml_node.add_child(version_node_for(version)) + end + true + end + + def update_latest + return false if latest_coherent? + + latest_xml_node.content = latest_from_database + true + end + + def latest_coherent? + latest_from_xml.nil? || latest_from_xml == latest_from_database + end + + def update_release + return false if release_coherent? + + if release_from_database + release_xml_node.content = release_from_database + else + release_xml_node.remove + end + + true + end + + def release_coherent? + release_from_xml == release_from_database + end + + def update_last_updated_timestamp + last_updated_xml_node.content = Time.zone.now.strftime('%Y%m%d%H%M%S') + end + + def versioning_xml_node + strong_memoize(:versioning_xml_node) do + xml_doc.xpath(XPATH_VERSIONING).first + end + end + + def versions_xml_node + strong_memoize(:versions_xml_node) do + versioning_xml_node&.xpath(XPATH_VERSIONS) + &.first + end + end + + def version_xml_nodes + versions_xml_node&.xpath(XPATH_VERSION) + end + + def latest_xml_node + strong_memoize(:latest_xml_node) do + versioning_xml_node&.xpath(XPATH_LATEST) + &.first + end + end + + def release_xml_node + strong_memoize(:release_xml_node) do + versioning_xml_node&.xpath(XPATH_RELEASE) + &.first + end + end + + def last_updated_xml_node + strong_memoize(:last_updated_xml_mode) do + versioning_xml_node.xpath(XPATH_LAST_UPDATED) + .first + end + end + + def version_node_for(version) + Nokogiri::XML::Node.new('version', xml_doc).tap { |node| node.content = version } + end + + def versions_from_xml + strong_memoize(:versions_from_xml) do + versions_xml_node.xpath(XPATH_VERSION) + .map(&:text) + end + end + + def latest_from_xml + latest_xml_node&.text + end + + def release_from_xml + release_xml_node&.text + end + + def versions_from_database + strong_memoize(:versions_from_database) do + @package.project.packages + .maven + .displayable + .with_name(@package.name) + .has_version + .order_created + .pluck_versions + end + end + + def latest_from_database + versions_from_database.last + end + + def release_from_database + strong_memoize(:release_from_database) do + non_snapshot_versions_from_database = versions_from_database.reject { |v| v.ends_with?('SNAPSHOT') } + non_snapshot_versions_from_database.last + end + end + + def xml_doc + strong_memoize(:xml_doc) do + Nokogiri::XML(@metadata_content) do |config| + config.default_xml.noblanks + end + end + end + end + end + end +end diff --git a/app/services/packages/maven/metadata/sync_service.rb b/app/services/packages/maven/metadata/sync_service.rb new file mode 100644 index 00000000000..ab45e30c4f7 --- /dev/null +++ b/app/services/packages/maven/metadata/sync_service.rb @@ -0,0 +1,78 @@ +# frozen_string_literal: true + +module Packages + module Maven + module Metadata + class SyncService < BaseContainerService + include Gitlab::Utils::StrongMemoize + + alias_method :project, :container + + MAX_FILE_SIZE = 10.megabytes.freeze + + def execute + return error('Blank package name') unless package_name + return error('Not allowed') unless Ability.allowed?(current_user, :destroy_package, project) + return error('Non existing versionless package') unless versionless_package_for_versions + return error('Non existing metadata file for versions') unless metadata_package_file_for_versions + + update_versions_xml + end + + private + + def update_versions_xml + return error('Metadata file for versions is too big') if metadata_package_file_for_versions.size > MAX_FILE_SIZE + + metadata_package_file_for_versions.file.use_open_file do |file| + result = CreateVersionsXmlService.new(metadata_content: file, package: versionless_package_for_versions) + .execute + + next result unless result.success? + next success('No changes for versions xml') unless result.payload[:changes_exist] + + if result.payload[:empty_versions] + versionless_package_for_versions.destroy! + success('Versionless package for versions destroyed') + else + AppendPackageFileService.new(metadata_content: result.payload[:metadata_content], package: versionless_package_for_versions) + .execute + end + end + end + + def metadata_package_file_for_versions + strong_memoize(:metadata_file_for_versions) do + versionless_package_for_versions.package_files + .with_file_name(Metadata.filename) + .recent + .first + end + end + + def versionless_package_for_versions + strong_memoize(:versionless_package_for_versions) do + project.packages + .maven + .displayable + .with_name(package_name) + .with_version(nil) + .first + end + end + + def package_name + params[:package_name] + end + + def error(message) + ServiceResponse.error(message: message) + end + + def success(message) + ServiceResponse.success(message: message) + end + end + end + end +end diff --git a/app/workers/all_queues.yml b/app/workers/all_queues.yml index 51df9a9d891..20e1636b2fa 100644 --- a/app/workers/all_queues.yml +++ b/app/workers/all_queues.yml @@ -1083,6 +1083,14 @@ :weight: 1 :idempotent: :tags: [] +- :name: package_repositories:packages_maven_metadata_sync + :feature_category: :package_registry + :has_external_dependencies: + :urgency: :low + :resource_boundary: :unknown + :weight: 1 + :idempotent: true + :tags: [] - :name: package_repositories:packages_nuget_extraction :feature_category: :package_registry :has_external_dependencies: diff --git a/app/workers/concerns/application_worker.rb b/app/workers/concerns/application_worker.rb index 0de26e27631..d101ef100d8 100644 --- a/app/workers/concerns/application_worker.rb +++ b/app/workers/concerns/application_worker.rb @@ -18,7 +18,7 @@ module ApplicationWorker set_queue def structured_payload(payload = {}) - context = Gitlab::ApplicationContext.current.merge( + context = Labkit::Context.current.to_h.merge( 'class' => self.class.name, 'job_status' => 'running', 'queue' => self.class.queue, diff --git a/app/workers/concerns/cronjob_queue.rb b/app/workers/concerns/cronjob_queue.rb index b89d6bba72c..955387b5ad4 100644 --- a/app/workers/concerns/cronjob_queue.rb +++ b/app/workers/concerns/cronjob_queue.rb @@ -15,7 +15,7 @@ module CronjobQueue # Cronjobs never get scheduled with arguments, so this is safe to # override def context_for_arguments(_args) - return if Gitlab::ApplicationContext.current_context_include?(:caller_id) + return if Gitlab::ApplicationContext.current_context_include?('meta.caller_id') Gitlab::ApplicationContext.new(caller_id: "Cronjob") end diff --git a/app/workers/packages/maven/metadata/sync_worker.rb b/app/workers/packages/maven/metadata/sync_worker.rb new file mode 100644 index 00000000000..eb7abf4cdd0 --- /dev/null +++ b/app/workers/packages/maven/metadata/sync_worker.rb @@ -0,0 +1,59 @@ +# frozen_string_literal: true + +module Packages + module Maven + module Metadata + class SyncWorker + include ApplicationWorker + include Gitlab::Utils::StrongMemoize + + queue_namespace :package_repositories + feature_category :package_registry + + deduplicate :until_executing + idempotent! + + loggable_arguments 2 + + SyncError = Class.new(StandardError) + + def perform(user_id, project_id, package_name) + @user_id = user_id + @project_id = project_id + @package_name = package_name + + return unless valid? + + result = ::Packages::Maven::Metadata::SyncService.new(container: project, current_user: user, params: { package_name: @package_name }) + .execute + + if result.success? + log_extra_metadata_on_done(:message, result.message) + else + raise SyncError.new(result.message) + end + + raise SyncError.new(result.message) unless result.success? + end + + private + + def valid? + @package_name.present? && user.present? && project.present? + end + + def user + strong_memoize(:user) do + User.find_by_id(@user_id) + end + end + + def project + strong_memoize(:project) do + Project.find_by_id(@project_id) + end + end + end + end + end +end diff --git a/changelogs/unreleased/11424-background-worker-for-updating-the-maven-metadata-xml-file.yml b/changelogs/unreleased/11424-background-worker-for-updating-the-maven-metadata-xml-file.yml new file mode 100644 index 00000000000..762f40be0f4 --- /dev/null +++ b/changelogs/unreleased/11424-background-worker-for-updating-the-maven-metadata-xml-file.yml @@ -0,0 +1,5 @@ +--- +title: Sync the maven metadata file upon package deletion through the UI +merge_request: 55207 +author: +type: fixed diff --git a/changelogs/unreleased/299884_remove_diff_highlighting_feature_flag.yml b/changelogs/unreleased/299884_remove_diff_highlighting_feature_flag.yml new file mode 100644 index 00000000000..4ce01f2dd48 --- /dev/null +++ b/changelogs/unreleased/299884_remove_diff_highlighting_feature_flag.yml @@ -0,0 +1,5 @@ +--- +title: Remove improved_merge_diff_highlighting feature flag +merge_request: 55771 +author: +type: added diff --git a/changelogs/unreleased/environment-scoped-group-variables.yml b/changelogs/unreleased/environment-scoped-group-variables.yml new file mode 100644 index 00000000000..d0cdb31851d --- /dev/null +++ b/changelogs/unreleased/environment-scoped-group-variables.yml @@ -0,0 +1,5 @@ +--- +title: Add environment_scope column to ci_group_variables +merge_request: 55256 +author: +type: other diff --git a/config/feature_flags/development/improved_merge_diff_highlighting.yml b/config/feature_flags/development/scoped_group_variables.yml index b397405a12c..bdeb453abb9 100644 --- a/config/feature_flags/development/improved_merge_diff_highlighting.yml +++ b/config/feature_flags/development/scoped_group_variables.yml @@ -1,8 +1,8 @@ --- -name: improved_merge_diff_highlighting -introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/52499 -rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/299884 -milestone: '13.9' +name: scoped_group_variables +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/55256 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/323298 +milestone: '13.10' type: development -group: group::source code -default_enabled: true +group: group::configure +default_enabled: false diff --git a/db/migrate/20210218040814_add_environment_scope_to_group_variables.rb b/db/migrate/20210218040814_add_environment_scope_to_group_variables.rb new file mode 100644 index 00000000000..5cc41f570aa --- /dev/null +++ b/db/migrate/20210218040814_add_environment_scope_to_group_variables.rb @@ -0,0 +1,45 @@ +# frozen_string_literal: true + +class AddEnvironmentScopeToGroupVariables < ActiveRecord::Migration[6.0] + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + OLD_INDEX = 'index_ci_group_variables_on_group_id_and_key' + NEW_INDEX = 'index_ci_group_variables_on_group_id_and_key_and_environment' + + disable_ddl_transaction! + + def up + unless column_exists?(:ci_group_variables, :environment_scope) + # rubocop:disable Migration/AddLimitToTextColumns + # Added in 20210305013509_add_text_limit_to_group_ci_variables_environment_scope + add_column :ci_group_variables, :environment_scope, :text, null: false, default: '*' + # rubocop:enable Migration/AddLimitToTextColumns + end + + add_concurrent_index :ci_group_variables, [:group_id, :key, :environment_scope], unique: true, name: NEW_INDEX + remove_concurrent_index_by_name :ci_group_variables, OLD_INDEX + end + + def down + remove_duplicates! + + add_concurrent_index :ci_group_variables, [:group_id, :key], unique: true, name: OLD_INDEX + remove_concurrent_index_by_name :ci_group_variables, NEW_INDEX + + remove_column :ci_group_variables, :environment_scope + end + + private + + def remove_duplicates! + execute <<-SQL + DELETE FROM ci_group_variables + WHERE id NOT IN ( + SELECT MIN(id) + FROM ci_group_variables + GROUP BY group_id, key + ) + SQL + end +end diff --git a/db/migrate/20210305013509_add_text_limit_to_group_ci_variables_environment_scope.rb b/db/migrate/20210305013509_add_text_limit_to_group_ci_variables_environment_scope.rb new file mode 100644 index 00000000000..f5fa073b954 --- /dev/null +++ b/db/migrate/20210305013509_add_text_limit_to_group_ci_variables_environment_scope.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +class AddTextLimitToGroupCiVariablesEnvironmentScope < ActiveRecord::Migration[6.0] + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + disable_ddl_transaction! + + def up + add_text_limit :ci_group_variables, :environment_scope, 255 + end + + def down + remove_text_limit :ci_group_variables, :environment_scope + end +end diff --git a/db/schema_migrations/20210218040814 b/db/schema_migrations/20210218040814 new file mode 100644 index 00000000000..3c55d951c14 --- /dev/null +++ b/db/schema_migrations/20210218040814 @@ -0,0 +1 @@ +6fe34be82f9aee6cbdb729a67d1d4ac0702c8d9774a038bfd4fd9d9cb28b1a2b
\ No newline at end of file diff --git a/db/schema_migrations/20210305013509 b/db/schema_migrations/20210305013509 new file mode 100644 index 00000000000..6649e76508b --- /dev/null +++ b/db/schema_migrations/20210305013509 @@ -0,0 +1 @@ +743344bb057d0e368c69cc3c90f72d560359d0753acf069e7423928c778a140a
\ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index e42eee4a99e..63f0f42c360 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -10555,7 +10555,9 @@ CREATE TABLE ci_group_variables ( created_at timestamp with time zone NOT NULL, updated_at timestamp with time zone NOT NULL, masked boolean DEFAULT false NOT NULL, - variable_type smallint DEFAULT 1 NOT NULL + variable_type smallint DEFAULT 1 NOT NULL, + environment_scope text DEFAULT '*'::text NOT NULL, + CONSTRAINT check_dfe009485a CHECK ((char_length(environment_scope) <= 255)) ); CREATE SEQUENCE ci_group_variables_id_seq @@ -21956,7 +21958,7 @@ CREATE INDEX index_ci_deleted_objects_on_pick_up_at ON ci_deleted_objects USING CREATE INDEX index_ci_freeze_periods_on_project_id ON ci_freeze_periods USING btree (project_id); -CREATE UNIQUE INDEX index_ci_group_variables_on_group_id_and_key ON ci_group_variables USING btree (group_id, key); +CREATE UNIQUE INDEX index_ci_group_variables_on_group_id_and_key_and_environment ON ci_group_variables USING btree (group_id, key, environment_scope); CREATE UNIQUE INDEX index_ci_instance_variables_on_key ON ci_instance_variables USING btree (key); diff --git a/lib/api/admin/sidekiq.rb b/lib/api/admin/sidekiq.rb index d91d4a0d4d5..7e561783685 100644 --- a/lib/api/admin/sidekiq.rb +++ b/lib/api/admin/sidekiq.rb @@ -12,11 +12,11 @@ module API namespace 'queues' do desc 'Drop jobs matching the given metadata from the Sidekiq queue' params do - Gitlab::ApplicationContext::KNOWN_KEYS.each do |key| + Labkit::Context::KNOWN_KEYS.each do |key| optional key, type: String, allow_blank: false end - at_least_one_of(*Gitlab::ApplicationContext::KNOWN_KEYS) + at_least_one_of(*Labkit::Context::KNOWN_KEYS) end delete ':queue_name' do result = diff --git a/lib/api/project_packages.rb b/lib/api/project_packages.rb index 0fdaa4b2656..babc7b9dd58 100644 --- a/lib/api/project_packages.rb +++ b/lib/api/project_packages.rb @@ -70,7 +70,11 @@ module API package = ::Packages::PackageFinder .new(user_project, params[:package_id]).execute - destroy_conditionally!(package) + destroy_conditionally!(package) do |package| + if package.destroy + package.sync_maven_metadata(current_user) + end + end end end end diff --git a/lib/gitlab/application_context.rb b/lib/gitlab/application_context.rb index 5675019ce83..5d07610feb5 100644 --- a/lib/gitlab/application_context.rb +++ b/lib/gitlab/application_context.rb @@ -7,9 +7,6 @@ module Gitlab Attribute = Struct.new(:name, :type) - LOG_KEY = Labkit::Context::LOG_KEY - KNOWN_KEYS = Labkit::Context::KNOWN_KEYS - APPLICATION_ATTRIBUTES = [ Attribute.new(:project, Project), Attribute.new(:namespace, Namespace), @@ -25,10 +22,6 @@ module Gitlab application_context.use(&block) end - def self.with_raw_context(attributes = {}, &block) - Labkit::Context.with_context(attributes, &block) - end - def self.push(args) application_context = new(**args) Labkit::Context.push(application_context.to_lazy_hash) diff --git a/lib/gitlab/diff/highlight.rb b/lib/gitlab/diff/highlight.rb index 035084d4861..a5259079345 100644 --- a/lib/gitlab/diff/highlight.rb +++ b/lib/gitlab/diff/highlight.rb @@ -3,13 +3,12 @@ module Gitlab module Diff class Highlight - attr_reader :diff_file, :diff_lines, :raw_lines, :repository, :project + attr_reader :diff_file, :diff_lines, :raw_lines, :repository delegate :old_path, :new_path, :old_sha, :new_sha, to: :diff_file, prefix: :diff def initialize(diff_lines, repository: nil) @repository = repository - @project = repository&.project if diff_lines.is_a?(Gitlab::Diff::File) @diff_file = diff_lines @@ -67,7 +66,7 @@ module Gitlab end def inline_diffs - @inline_diffs ||= InlineDiff.for_lines(@raw_lines, project: project) + @inline_diffs ||= InlineDiff.for_lines(@raw_lines) end def old_lines diff --git a/lib/gitlab/diff/highlight_cache.rb b/lib/gitlab/diff/highlight_cache.rb index 7932cd2a837..e0f1f8a1e58 100644 --- a/lib/gitlab/diff/highlight_cache.rb +++ b/lib/gitlab/diff/highlight_cache.rb @@ -7,8 +7,7 @@ module Gitlab include Gitlab::Utils::StrongMemoize EXPIRATION = 1.week - VERSION = 1 - NEXT_VERSION = 2 + VERSION = 2 delegate :diffable, to: :@diff_collection delegate :diff_options, to: :@diff_collection @@ -70,20 +69,12 @@ module Gitlab def key strong_memoize(:redis_key) do - ['highlighted-diff-files', diffable.cache_key, version, diff_options].join(":") + ['highlighted-diff-files', diffable.cache_key, VERSION, diff_options].join(":") end end private - def version - if Feature.enabled?(:improved_merge_diff_highlighting, diffable.project, default_enabled: :yaml) - NEXT_VERSION - else - VERSION - end - end - def set_highlighted_diff_lines(diff_file, content) diff_file.highlighted_diff_lines = content.map do |line| Gitlab::Diff::Line.safe_init_from_hash(line) diff --git a/lib/gitlab/diff/inline_diff.rb b/lib/gitlab/diff/inline_diff.rb index 1d00f502d1c..dd73e4d6c15 100644 --- a/lib/gitlab/diff/inline_diff.rb +++ b/lib/gitlab/diff/inline_diff.rb @@ -11,19 +11,15 @@ module Gitlab @offset = offset end - def inline_diffs(project: nil) + def inline_diffs # Skip inline diff if empty line was replaced with content return if old_line == "" - if Feature.enabled?(:improved_merge_diff_highlighting, project, default_enabled: :yaml) - CharDiff.new(old_line, new_line).changed_ranges(offset: offset) - else - deprecated_diff - end + CharDiff.new(old_line, new_line).changed_ranges(offset: offset) end class << self - def for_lines(lines, project: nil) + def for_lines(lines) pair_selector = Gitlab::Diff::PairSelector.new(lines) inline_diffs = [] @@ -32,7 +28,7 @@ module Gitlab old_line = lines[old_index] new_line = lines[new_index] - old_diffs, new_diffs = new(old_line, new_line, offset: 1).inline_diffs(project: project) + old_diffs, new_diffs = new(old_line, new_line, offset: 1).inline_diffs inline_diffs[old_index] = old_diffs inline_diffs[new_index] = new_diffs @@ -41,46 +37,6 @@ module Gitlab inline_diffs end end - - private - - # See: https://gitlab.com/gitlab-org/gitlab/-/issues/299884 - def deprecated_diff - lcp = longest_common_prefix(old_line, new_line) - lcs = longest_common_suffix(old_line[lcp..-1], new_line[lcp..-1]) - - lcp += offset - old_length = old_line.length + offset - new_length = new_line.length + offset - - old_diff_range = lcp..(old_length - lcs - 1) - new_diff_range = lcp..(new_length - lcs - 1) - - old_diffs = [old_diff_range] if old_diff_range.begin <= old_diff_range.end - new_diffs = [new_diff_range] if new_diff_range.begin <= new_diff_range.end - - [old_diffs, new_diffs] - end - - def longest_common_prefix(a, b) # rubocop:disable Naming/UncommunicativeMethodParamName - max_length = [a.length, b.length].max - - length = 0 - (0..max_length - 1).each do |pos| - old_char = a[pos] - new_char = b[pos] - - break if old_char != new_char - - length += 1 - end - - length - end - - def longest_common_suffix(a, b) # rubocop:disable Naming/UncommunicativeMethodParamName - longest_common_prefix(a.reverse, b.reverse) - end end end end diff --git a/lib/gitlab/gitaly_client.rb b/lib/gitlab/gitaly_client.rb index f4a89edecd1..e3788814dd5 100644 --- a/lib/gitlab/gitaly_client.rb +++ b/lib/gitlab/gitaly_client.rb @@ -215,7 +215,7 @@ module Gitlab 'client_name' => CLIENT_NAME } - context_data = Gitlab::ApplicationContext.current + context_data = Labkit::Context.current&.to_h feature_stack = Thread.current[:gitaly_feature_stack] feature = feature_stack && feature_stack[0] diff --git a/lib/gitlab/grape_logging/loggers/context_logger.rb b/lib/gitlab/grape_logging/loggers/context_logger.rb index 468a296886e..0a8f0872fbe 100644 --- a/lib/gitlab/grape_logging/loggers/context_logger.rb +++ b/lib/gitlab/grape_logging/loggers/context_logger.rb @@ -6,7 +6,7 @@ module Gitlab module Loggers class ContextLogger < ::GrapeLogging::Loggers::Base def parameters(_, _) - Gitlab::ApplicationContext.current + Labkit::Context.current.to_h end end end diff --git a/lib/gitlab/query_limiting.rb b/lib/gitlab/query_limiting.rb index 31e6b120e45..5e46e26e14e 100644 --- a/lib/gitlab/query_limiting.rb +++ b/lib/gitlab/query_limiting.rb @@ -4,9 +4,8 @@ module Gitlab module QueryLimiting # Returns true if we should enable tracking of query counts. # - # This is only enabled in production/staging if we're running on GitLab.com. - # This ensures we don't produce any errors that users can't do anything - # about themselves. + # This is only enabled in development and test to ensure we don't produce + # any errors that users of other environments can't do anything about themselves. def self.enable? Rails.env.development? || Rails.env.test? end @@ -19,7 +18,7 @@ module Gitlab # The issue URL is only meant to push developers into creating an issue # instead of blindly whitelisting offending blocks of code. def self.whitelist(issue_url) - return unless enable_whitelist? + return unless enable? unless issue_url.start_with?('https://') raise( @@ -30,9 +29,5 @@ module Gitlab Transaction&.current&.whitelisted = true end - - def self.enable_whitelist? - Rails.env.development? || Rails.env.test? - end end end diff --git a/lib/gitlab/sidekiq_queue.rb b/lib/gitlab/sidekiq_queue.rb index 4b71dfc0c1b..807c27a71ff 100644 --- a/lib/gitlab/sidekiq_queue.rb +++ b/lib/gitlab/sidekiq_queue.rb @@ -21,7 +21,7 @@ module Gitlab job_search_metadata = search_metadata .stringify_keys - .slice(*Gitlab::ApplicationContext::KNOWN_KEYS) + .slice(*Labkit::Context::KNOWN_KEYS) .transform_keys { |key| "meta.#{key}" } .compact diff --git a/spec/controllers/application_controller_spec.rb b/spec/controllers/application_controller_spec.rb index 6641a3ed914..4a729008e67 100644 --- a/spec/controllers/application_controller_spec.rb +++ b/spec/controllers/application_controller_spec.rb @@ -900,7 +900,7 @@ RSpec.describe ApplicationController do feature_category :issue_tracking def index - Gitlab::ApplicationContext.with_raw_context do |context| + Labkit::Context.with_context do |context| render json: context.to_h end end diff --git a/spec/controllers/sessions_controller_spec.rb b/spec/controllers/sessions_controller_spec.rb index 5bee7698c3f..c31ba6fe156 100644 --- a/spec/controllers/sessions_controller_spec.rb +++ b/spec/controllers/sessions_controller_spec.rb @@ -524,7 +524,7 @@ RSpec.describe SessionsController do it 'sets the username and caller_id in the context' do expect(controller).to receive(:destroy).and_wrap_original do |m, *args| - expect(Gitlab::ApplicationContext.current) + expect(Labkit::Context.current.to_h) .to include('meta.user' => user.username, 'meta.caller_id' => 'SessionsController#destroy') @@ -538,9 +538,9 @@ RSpec.describe SessionsController do context 'when not signed in' do it 'sets the caller_id in the context' do expect(controller).to receive(:new).and_wrap_original do |m, *args| - expect(Gitlab::ApplicationContext.current) + expect(Labkit::Context.current.to_h) .to include('meta.caller_id' => 'SessionsController#new') - expect(Gitlab::ApplicationContext.current) + expect(Labkit::Context.current.to_h) .not_to include('meta.user') m.call(*args) @@ -557,9 +557,9 @@ RSpec.describe SessionsController do it 'sets the caller_id in the context' do allow_any_instance_of(User).to receive(:lock_access!).and_wrap_original do |m, *args| - expect(Gitlab::ApplicationContext.current) + expect(Labkit::Context.current.to_h) .to include('meta.caller_id' => 'SessionsController#create') - expect(Gitlab::ApplicationContext.current) + expect(Labkit::Context.current.to_h) .not_to include('meta.user') m.call(*args) diff --git a/spec/lib/gitlab/application_context_spec.rb b/spec/lib/gitlab/application_context_spec.rb index 44e90994975..88f865adea7 100644 --- a/spec/lib/gitlab/application_context_spec.rb +++ b/spec/lib/gitlab/application_context_spec.rb @@ -27,20 +27,6 @@ RSpec.describe Gitlab::ApplicationContext do end end - describe '.with_raw_context' do - it 'yields the block' do - expect { |b| described_class.with_raw_context({}, &b) }.to yield_control - end - - it 'passes the attributes unaltered on to labkit' do - attrs = { foo: :bar } - - expect(Labkit::Context).to receive(:with_context).with(attrs) - - described_class.with_raw_context(attrs) {} - end - end - describe '.push' do it 'passes the expected context on to labkit' do fake_proc = duck_type(:call) @@ -124,7 +110,7 @@ RSpec.describe Gitlab::ApplicationContext do it 'does not cause queries' do context = described_class.new(project: create(:project), namespace: create(:group, :nested), user: create(:user)) - expect { context.use { Gitlab::ApplicationContext.current } }.not_to exceed_query_limit(0) + expect { context.use { Labkit::Context.current.to_h } }.not_to exceed_query_limit(0) end end end diff --git a/spec/lib/gitlab/diff/highlight_cache_spec.rb b/spec/lib/gitlab/diff/highlight_cache_spec.rb index 94717152488..efe8535ba3c 100644 --- a/spec/lib/gitlab/diff/highlight_cache_spec.rb +++ b/spec/lib/gitlab/diff/highlight_cache_spec.rb @@ -237,18 +237,8 @@ RSpec.describe Gitlab::Diff::HighlightCache, :clean_gitlab_redis_cache do describe '#key' do subject { cache.key } - it 'returns the next version of the cache' do + it 'returns cache key' do is_expected.to start_with("highlighted-diff-files:#{cache.diffable.cache_key}:2") end - - context 'when feature flag is disabled' do - before do - stub_feature_flags(improved_merge_diff_highlighting: false) - end - - it 'returns the original version of the cache' do - is_expected.to start_with("highlighted-diff-files:#{cache.diffable.cache_key}:1") - end - end end end diff --git a/spec/lib/gitlab/diff/inline_diff_spec.rb b/spec/lib/gitlab/diff/inline_diff_spec.rb index dce655d5690..714b5d813c4 100644 --- a/spec/lib/gitlab/diff/inline_diff_spec.rb +++ b/spec/lib/gitlab/diff/inline_diff_spec.rb @@ -52,17 +52,6 @@ RSpec.describe Gitlab::Diff::InlineDiff do expect(subject[0]).to eq([3..6]) expect(subject[1]).to eq([3..3, 17..22]) end - - context 'when feature flag is disabled' do - before do - stub_feature_flags(improved_merge_diff_highlighting: false) - end - - it 'finds all inline diffs' do - expect(subject[0]).to eq([3..19]) - expect(subject[1]).to eq([3..22]) - end - end end end diff --git a/spec/lib/gitlab/query_limiting_spec.rb b/spec/lib/gitlab/query_limiting_spec.rb index 0fcd865567d..4f70c65adca 100644 --- a/spec/lib/gitlab/query_limiting_spec.rb +++ b/spec/lib/gitlab/query_limiting_spec.rb @@ -63,6 +63,20 @@ RSpec.describe Gitlab::QueryLimiting do expect(transaction.count).to eq(before) end + + it 'whitelists when enabled' do + described_class.whitelist('https://example.com') + + expect(transaction.whitelisted).to eq(true) + end + + it 'does not whitelist when disabled' do + allow(described_class).to receive(:enable?).and_return(false) + + described_class.whitelist('https://example.com') + + expect(transaction.whitelisted).to eq(false) + end end end end diff --git a/spec/lib/gitlab/sidekiq_middleware/worker_context/server_spec.rb b/spec/lib/gitlab/sidekiq_middleware/worker_context/server_spec.rb index f736a7db774..ca473462d2e 100644 --- a/spec/lib/gitlab/sidekiq_middleware/worker_context/server_spec.rb +++ b/spec/lib/gitlab/sidekiq_middleware/worker_context/server_spec.rb @@ -18,7 +18,7 @@ RSpec.describe Gitlab::SidekiqMiddleware::WorkerContext::Server do worker_context user: nil def perform(identifier, *args) - self.class.contexts.merge!(identifier => Gitlab::ApplicationContext.current) + self.class.contexts.merge!(identifier => Labkit::Context.current.to_h) end end end diff --git a/spec/migrations/20210218040814_add_environment_scope_to_group_variables_spec.rb b/spec/migrations/20210218040814_add_environment_scope_to_group_variables_spec.rb new file mode 100644 index 00000000000..e525101f3a0 --- /dev/null +++ b/spec/migrations/20210218040814_add_environment_scope_to_group_variables_spec.rb @@ -0,0 +1,46 @@ +# frozen_string_literal: true + +require 'spec_helper' +require_migration!('add_environment_scope_to_group_variables') + +RSpec.describe AddEnvironmentScopeToGroupVariables do + let(:migration) { described_class.new } + let(:ci_group_variables) { table(:ci_group_variables) } + let(:group) { table(:namespaces).create!(name: 'group', path: 'group') } + + def create_variable!(group, key:, environment_scope: '*') + table(:ci_group_variables).create!( + group_id: group.id, + key: key, + environment_scope: environment_scope + ) + end + + describe '#down' do + context 'group has variables with duplicate keys' do + it 'deletes all but the first record' do + migration.up + + remaining_variable = create_variable!(group, key: 'key') + create_variable!(group, key: 'key', environment_scope: 'staging') + create_variable!(group, key: 'key', environment_scope: 'production') + + migration.down + + expect(ci_group_variables.pluck(:id)).to eq [remaining_variable.id] + end + end + + context 'group does not have variables with duplicate keys' do + it 'does not delete any records' do + migration.up + + create_variable!(group, key: 'key') + create_variable!(group, key: 'staging') + create_variable!(group, key: 'production') + + expect { migration.down }.not_to change { ci_group_variables.count } + end + end + end +end diff --git a/spec/models/ci/group_variable_spec.rb b/spec/models/ci/group_variable_spec.rb index c8eac4d8765..f0eec549da7 100644 --- a/spec/models/ci/group_variable_spec.rb +++ b/spec/models/ci/group_variable_spec.rb @@ -9,7 +9,17 @@ RSpec.describe Ci::GroupVariable do it { is_expected.to include_module(Presentable) } it { is_expected.to include_module(Ci::Maskable) } - it { is_expected.to validate_uniqueness_of(:key).scoped_to(:group_id).with_message(/\(\w+\) has already been taken/) } + it { is_expected.to include_module(HasEnvironmentScope) } + it { is_expected.to validate_uniqueness_of(:key).scoped_to([:group_id, :environment_scope]).with_message(/\(\w+\) has already been taken/) } + + describe '.by_environment_scope' do + let!(:matching_variable) { create(:ci_group_variable, environment_scope: 'production ') } + let!(:non_matching_variable) { create(:ci_group_variable, environment_scope: 'staging') } + + subject { Ci::GroupVariable.by_environment_scope('production') } + + it { is_expected.to contain_exactly(matching_variable) } + end describe '.unprotected' do subject { described_class.unprotected } diff --git a/spec/models/ci/variable_spec.rb b/spec/models/ci/variable_spec.rb index 26a7a2596af..93a24ba9157 100644 --- a/spec/models/ci/variable_spec.rb +++ b/spec/models/ci/variable_spec.rb @@ -14,6 +14,15 @@ RSpec.describe Ci::Variable do it { is_expected.to validate_uniqueness_of(:key).scoped_to(:project_id, :environment_scope).with_message(/\(\w+\) has already been taken/) } end + describe '.by_environment_scope' do + let!(:matching_variable) { create(:ci_variable, environment_scope: 'production ') } + let!(:non_matching_variable) { create(:ci_variable, environment_scope: 'staging') } + + subject { Ci::Variable.by_environment_scope('production') } + + it { is_expected.to contain_exactly(matching_variable) } + end + describe '.unprotected' do subject { described_class.unprotected } diff --git a/spec/models/concerns/ci/has_variable_spec.rb b/spec/models/concerns/ci/has_variable_spec.rb index b5390281064..e917ec6b802 100644 --- a/spec/models/concerns/ci/has_variable_spec.rb +++ b/spec/models/concerns/ci/has_variable_spec.rb @@ -11,6 +11,17 @@ RSpec.describe Ci::HasVariable do it { is_expected.not_to allow_value('foo bar').for(:key) } it { is_expected.not_to allow_value('foo/bar').for(:key) } + describe 'scopes' do + describe '.by_key' do + let!(:matching_variable) { create(:ci_variable, key: 'example') } + let!(:non_matching_variable) { create(:ci_variable, key: 'other') } + + subject { Ci::Variable.by_key('example') } + + it { is_expected.to contain_exactly(matching_variable) } + end + end + describe '#key=' do context 'when the new key is nil' do it 'strips leading and trailing whitespaces' do diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb index 422d9ce406a..20983744919 100644 --- a/spec/models/group_spec.rb +++ b/spec/models/group_spec.rb @@ -1318,9 +1318,10 @@ RSpec.describe Group do describe '#ci_variables_for' do let(:project) { create(:project, group: group) } + let(:environment_scope) { '*' } let!(:ci_variable) do - create(:ci_group_variable, value: 'secret', group: group) + create(:ci_group_variable, value: 'secret', group: group, environment_scope: environment_scope) end let!(:protected_variable) do @@ -1329,13 +1330,16 @@ RSpec.describe Group do subject { group.ci_variables_for('ref', project) } - it 'memoizes the result by ref', :request_store do + it 'memoizes the result by ref and environment', :request_store do + scoped_variable = create(:ci_group_variable, value: 'secret', group: group, environment_scope: 'scoped') + expect(project).to receive(:protected_for?).with('ref').once.and_return(true) - expect(project).to receive(:protected_for?).with('other').once.and_return(false) + expect(project).to receive(:protected_for?).with('other').twice.and_return(false) 2.times do - expect(group.ci_variables_for('ref', project)).to contain_exactly(ci_variable, protected_variable) + expect(group.ci_variables_for('ref', project, environment: 'production')).to contain_exactly(ci_variable, protected_variable) expect(group.ci_variables_for('other', project)).to contain_exactly(ci_variable) + expect(group.ci_variables_for('other', project, environment: 'scoped')).to contain_exactly(ci_variable, scoped_variable) end end @@ -1372,6 +1376,120 @@ RSpec.describe Group do it_behaves_like 'ref is protected' end + context 'when environment name is specified' do + let(:environment) { 'review/name' } + + subject do + group.ci_variables_for('ref', project, environment: environment) + end + + context 'when environment scope is exactly matched' do + let(:environment_scope) { 'review/name' } + + it { is_expected.to contain_exactly(ci_variable) } + end + + context 'when environment scope is matched by wildcard' do + let(:environment_scope) { 'review/*' } + + it { is_expected.to contain_exactly(ci_variable) } + end + + context 'when environment scope does not match' do + let(:environment_scope) { 'review/*/special' } + + it { is_expected.not_to contain_exactly(ci_variable) } + end + + context 'when environment scope has _' do + let(:environment_scope) { '*_*' } + + it 'does not treat it as wildcard' do + is_expected.not_to contain_exactly(ci_variable) + end + + context 'when environment name contains underscore' do + let(:environment) { 'foo_bar/test' } + let(:environment_scope) { 'foo_bar/*' } + + it 'matches literally for _' do + is_expected.to contain_exactly(ci_variable) + end + end + end + + # The environment name and scope cannot have % at the moment, + # but we're considering relaxing it and we should also make sure + # it doesn't break in case some data sneaked in somehow as we're + # not checking this integrity in database level. + context 'when environment scope has %' do + it 'does not treat it as wildcard' do + ci_variable.update_attribute(:environment_scope, '*%*') + + is_expected.not_to contain_exactly(ci_variable) + end + + context 'when environment name contains a percent' do + let(:environment) { 'foo%bar/test' } + + it 'matches literally for %' do + ci_variable.update(environment_scope: 'foo%bar/*') + + is_expected.to contain_exactly(ci_variable) + end + end + end + + context 'when variables with the same name have different environment scopes' do + let!(:partially_matched_variable) do + create(:ci_group_variable, + key: ci_variable.key, + value: 'partial', + environment_scope: 'review/*', + group: group) + end + + let!(:perfectly_matched_variable) do + create(:ci_group_variable, + key: ci_variable.key, + value: 'prefect', + environment_scope: 'review/name', + group: group) + end + + it 'puts variables matching environment scope more in the end' do + is_expected.to eq( + [ci_variable, + partially_matched_variable, + perfectly_matched_variable]) + end + end + + context 'when :scoped_group_variables feature flag is disabled' do + before do + stub_feature_flags(scoped_group_variables: false) + end + + context 'when environment scope is exactly matched' do + let(:environment_scope) { 'review/name' } + + it { is_expected.to contain_exactly(ci_variable) } + end + + context 'when environment scope is partially matched' do + let(:environment_scope) { 'review/*' } + + it { is_expected.to contain_exactly(ci_variable) } + end + + context 'when environment scope does not match' do + let(:environment_scope) { 'review/*/special' } + + it { is_expected.to contain_exactly(ci_variable) } + end + end + end + context 'when group has children' do let(:group_child) { create(:group, parent: group) } let(:group_child_2) { create(:group, parent: group_child) } diff --git a/spec/models/packages/package_spec.rb b/spec/models/packages/package_spec.rb index da768b3cebc..2ed2ff82d1f 100644 --- a/spec/models/packages/package_spec.rb +++ b/spec/models/packages/package_spec.rb @@ -813,4 +813,45 @@ RSpec.describe Packages::Package, type: :model do expect(package.package_settings).to eq(group.package_settings) end end + + describe '#sync_maven_metadata' do + let_it_be(:user) { create(:user) } + let_it_be(:package) { create(:maven_package) } + + subject { package.sync_maven_metadata(user) } + + shared_examples 'not enqueuing a sync worker job' do + it 'does not enqueue a sync worker job' do + expect(::Packages::Maven::Metadata::SyncWorker) + .not_to receive(:perform_async) + + subject + end + end + + it 'enqueues a sync worker job' do + expect(::Packages::Maven::Metadata::SyncWorker) + .to receive(:perform_async).with(user.id, package.project.id, package.name) + + subject + end + + context 'with no user' do + let(:user) { nil } + + it_behaves_like 'not enqueuing a sync worker job' + end + + context 'with a versionless maven package' do + let_it_be(:package) { create(:maven_package, version: nil) } + + it_behaves_like 'not enqueuing a sync worker job' + end + + context 'with a non maven package' do + let_it_be(:package) { create(:npm_package) } + + it_behaves_like 'not enqueuing a sync worker job' + end + end end diff --git a/spec/requests/api/api_spec.rb b/spec/requests/api/api_spec.rb index 4eae5678184..e1050d0b5f7 100644 --- a/spec/requests/api/api_spec.rb +++ b/spec/requests/api/api_spec.rb @@ -105,7 +105,7 @@ RSpec.describe API::API do it 'logs all application context fields' do allow_any_instance_of(Gitlab::GrapeLogging::Loggers::ContextLogger).to receive(:parameters) do - Gitlab::ApplicationContext.current.tap do |log_context| + Labkit::Context.current.to_h.tap do |log_context| expect(log_context).to match('correlation_id' => an_instance_of(String), 'meta.caller_id' => '/api/:version/projects/:id/issues', 'meta.remote_ip' => an_instance_of(String), @@ -121,7 +121,7 @@ RSpec.describe API::API do it 'skips fields that do not apply' do allow_any_instance_of(Gitlab::GrapeLogging::Loggers::ContextLogger).to receive(:parameters) do - Gitlab::ApplicationContext.current.tap do |log_context| + Labkit::Context.current.to_h.tap do |log_context| expect(log_context).to match('correlation_id' => an_instance_of(String), 'meta.caller_id' => '/api/:version/users', 'meta.remote_ip' => an_instance_of(String), diff --git a/spec/requests/api/project_packages_spec.rb b/spec/requests/api/project_packages_spec.rb index 1f3887cab8a..97414b3b18a 100644 --- a/spec/requests/api/project_packages_spec.rb +++ b/spec/requests/api/project_packages_spec.rb @@ -257,6 +257,10 @@ RSpec.describe API::ProjectPackages do context 'project is private' do let(:project) { create(:project, :private) } + before do + expect(::Packages::Maven::Metadata::SyncWorker).not_to receive(:perform_async) + end + it 'returns 404 for non authenticated user' do delete api(package_url) @@ -301,6 +305,19 @@ RSpec.describe API::ProjectPackages do expect(response).to have_gitlab_http_status(:no_content) end end + + context 'with a maven package' do + let_it_be(:package1) { create(:maven_package, project: project) } + + it 'enqueues a sync worker job' do + project.add_maintainer(user) + + expect(::Packages::Maven::Metadata::SyncWorker) + .to receive(:perform_async).with(user.id, project.id, package1.name) + + delete api(package_url, user) + end + end end end end diff --git a/spec/requests/api/projects_spec.rb b/spec/requests/api/projects_spec.rb index 33a3fad97ba..d2a33e32b30 100644 --- a/spec/requests/api/projects_spec.rb +++ b/spec/requests/api/projects_spec.rb @@ -1681,6 +1681,8 @@ RSpec.describe API::Projects do mirror requirements_enabled security_and_compliance_enabled + issues_template + merge_requests_template ] end diff --git a/spec/services/packages/maven/metadata/append_package_file_service_spec.rb b/spec/services/packages/maven/metadata/append_package_file_service_spec.rb new file mode 100644 index 00000000000..c406ab93630 --- /dev/null +++ b/spec/services/packages/maven/metadata/append_package_file_service_spec.rb @@ -0,0 +1,59 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe ::Packages::Maven::Metadata::AppendPackageFileService do + let_it_be(:package) { create(:maven_package, version: nil) } + + let(:service) { described_class.new(package: package, metadata_content: content) } + let(:content) { 'test' } + + describe '#execute' do + subject { service.execute } + + context 'with some content' do + it 'creates all the related package files', :aggregate_failures do + expect { subject }.to change { package.package_files.count }.by(5) + expect(subject).to be_success + + expect_file(metadata_file_name, with_content: content, with_content_type: 'application/xml') + expect_file("#{metadata_file_name}.md5") + expect_file("#{metadata_file_name}.sha1") + expect_file("#{metadata_file_name}.sha256") + expect_file("#{metadata_file_name}.sha512") + end + end + + context 'with nil content' do + let(:content) { nil } + + it_behaves_like 'returning an error service response', message: 'metadata content is not set' + end + + context 'with nil package' do + let(:package) { nil } + + it_behaves_like 'returning an error service response', message: 'package is not set' + end + + def expect_file(file_name, with_content: nil, with_content_type: '') + package_file = package.package_files.recent.with_file_name(file_name).first + + expect(package_file.file).to be_present + expect(package_file.file_name).to eq(file_name) + expect(package_file.size).to be > 0 + expect(package_file.file_md5).to be_present + expect(package_file.file_sha1).to be_present + expect(package_file.file_sha256).to be_present + expect(package_file.file.content_type).to eq(with_content_type) + + if with_content + expect(package_file.file.read).to eq(with_content) + end + end + + def metadata_file_name + ::Packages::Maven::Metadata.filename + end + end +end diff --git a/spec/services/packages/maven/metadata/create_versions_xml_service_spec.rb b/spec/services/packages/maven/metadata/create_versions_xml_service_spec.rb new file mode 100644 index 00000000000..109f7adab4e --- /dev/null +++ b/spec/services/packages/maven/metadata/create_versions_xml_service_spec.rb @@ -0,0 +1,277 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe ::Packages::Maven::Metadata::CreateVersionsXmlService do + let_it_be(:package) { create(:maven_package, version: nil) } + + let(:versions_in_database) { %w[1.3 2.0-SNAPSHOT 1.6 1.4 1.5-SNAPSHOT] } + let(:versions_in_xml) { %w[1.3 2.0-SNAPSHOT 1.6 1.4 1.5-SNAPSHOT] } + let(:version_latest) { nil } + let(:version_release) { '1.4' } + let(:service) { described_class.new(metadata_content: metadata_xml, package: package) } + + describe '#execute' do + subject { service.execute } + + before do + next unless package + + versions_in_database.each do |version| + create(:maven_package, name: package.name, version: version, project: package.project) + end + end + + shared_examples 'returning an xml with versions in the database' do + it 'returns an metadata versions xml with versions in the database', :aggregate_failures do + result = subject + + expect(result).to be_success + expect(versions_from(result.payload[:metadata_content])).to match_array(versions_in_database) + end + end + + shared_examples 'returning an xml with' do |release:, latest:| + it 'returns an xml with the updated release and latest versions', :aggregate_failures do + result = subject + + expect(result).to be_success + expect(result.payload[:changes_exist]).to be_truthy + xml = result.payload[:metadata_content] + expect(release_from(xml)).to eq(release) + expect(latest_from(xml)).to eq(latest) + end + end + + context 'with same versions in both sides' do + it 'returns no changes', :aggregate_failures do + result = subject + + expect(result).to be_success + expect(result.payload).to eq(changes_exist: false, empty_versions: false) + end + end + + context 'with more versions' do + let(:additional_versions) { %w[5.5 5.6 5.7-SNAPSHOT] } + + context 'in the xml side' do + let(:versions_in_xml) { versions_in_database + additional_versions } + + it_behaves_like 'returning an xml with versions in the database' + end + + context 'in the database side' do + let(:versions_in_database) { versions_in_xml + additional_versions } + + it_behaves_like 'returning an xml with versions in the database' + end + end + + context 'with completely different versions' do + let(:versions_in_database) { %w[1.0 1.1 1.2] } + let(:versions_in_xml) { %w[2.0 2.1 2.2] } + + it_behaves_like 'returning an xml with versions in the database' + end + + context 'with no versions in the database' do + let(:versions_in_database) { [] } + + it 'returns a success', :aggregate_failures do + result = subject + + expect(result).to be_success + expect(result.payload).to eq(changes_exist: true, empty_versions: true) + end + + context 'with an xml without a release version' do + let(:version_release) { nil } + + it 'returns a success', :aggregate_failures do + result = subject + + expect(result).to be_success + expect(result.payload).to eq(changes_exist: true, empty_versions: true) + end + end + end + + context 'with differences in both sides' do + let(:shared_versions) { %w[1.3 2.0-SNAPSHOT 1.6 1.4 1.5-SNAPSHOT] } + let(:additional_versions_in_xml) { %w[5.5 5.6 5.7-SNAPSHOT] } + let(:versions_in_xml) { shared_versions + additional_versions_in_xml } + let(:additional_versions_in_database) { %w[6.5 6.6 6.7-SNAPSHOT] } + let(:versions_in_database) { shared_versions + additional_versions_in_database } + + it_behaves_like 'returning an xml with versions in the database' + end + + context 'with a new release and latest from the database' do + let(:versions_in_database) { versions_in_xml + %w[4.1 4.2-SNAPSHOT] } + + it_behaves_like 'returning an xml with', release: '4.1', latest: nil + + context 'with a latest in the xml' do + let(:version_latest) { '1.6' } + + it_behaves_like 'returning an xml with', release: '4.1', latest: '4.2-SNAPSHOT' + end + end + + context 'with release and latest not existing in the database' do + let(:version_release) { '7.0' } + let(:version_latest) { '8.0-SNAPSHOT' } + + it_behaves_like 'returning an xml with', release: '1.4', latest: '1.5-SNAPSHOT' + end + + context 'with added versions in the database side no more recent than release' do + let(:versions_in_database) { versions_in_xml + %w[4.1 4.2-SNAPSHOT] } + + before do + ::Packages::Package.find_by(name: package.name, version: '4.1').update!(created_at: 2.weeks.ago) + ::Packages::Package.find_by(name: package.name, version: '4.2-SNAPSHOT').update!(created_at: 2.weeks.ago) + end + + it_behaves_like 'returning an xml with', release: '1.4', latest: nil + + context 'with a latest in the xml' do + let(:version_latest) { '1.6' } + + it_behaves_like 'returning an xml with', release: '1.4', latest: '1.5-SNAPSHOT' + end + end + + context 'only snapshot versions are in the database' do + let(:versions_in_database) { %w[4.2-SNAPSHOT] } + + it_behaves_like 'returning an xml with', release: nil, latest: nil + + it 'returns an xml without any release element' do + result = subject + + xml_doc = Nokogiri::XML(result.payload[:metadata_content]) + expect(xml_doc.xpath('//metadata/versioning/release')).to be_empty + end + end + + context 'last updated timestamp' do + let(:versions_in_database) { versions_in_xml + %w[4.1 4.2-SNAPSHOT] } + + it 'updates the last updated timestamp' do + original = last_updated_from(metadata_xml) + + result = subject + + expect(result).to be_success + expect(original).not_to eq(last_updated_from(result.payload[:metadata_content])) + end + end + + context 'with an incomplete metadata content' do + let(:metadata_xml) { '<metadata></metadata>' } + + it_behaves_like 'returning an error service response', message: 'metadata_content is invalid' + end + + context 'with an invalid metadata content' do + let(:metadata_xml) { '<meta></metadata>' } + + it_behaves_like 'returning an error service response', message: 'metadata_content is invalid' + end + + context 'with metadata content pointing to a file' do + let(:service) { described_class.new(metadata_content: file, package: package) } + let(:file) do + Tempfile.new('metadata').tap do |file| + if file_contents + file.write(file_contents) + file.flush + file.rewind + end + end + end + + after do + file.close + file.unlink + end + + context 'with valid content' do + let(:file_contents) { metadata_xml } + + it 'returns no changes' do + result = subject + + expect(result).to be_success + expect(result.payload).to eq(changes_exist: false, empty_versions: false) + end + end + + context 'with invalid content' do + let(:file_contents) { '<meta></metadata>' } + + it_behaves_like 'returning an error service response', message: 'metadata_content is invalid' + end + + context 'with no content' do + let(:file_contents) { nil } + + it_behaves_like 'returning an error service response', message: 'metadata_content is invalid' + end + end + + context 'with no package' do + let(:metadata_xml) { '' } + let(:package) { nil } + + it_behaves_like 'returning an error service response', message: 'package not set' + end + + context 'with no metadata content' do + let(:metadata_xml) { nil } + + it_behaves_like 'returning an error service response', message: 'metadata_content not set' + end + end + + def metadata_xml + Nokogiri::XML::Builder.new do |xml| + xml.metadata do + xml.groupId(package.maven_metadatum.app_group) + xml.artifactId(package.maven_metadatum.app_name) + xml.versioning do + xml.release(version_release) if version_release + xml.latest(version_latest) if version_latest + xml.lastUpdated('20210113130531') + xml.versions do + versions_in_xml.each do |version| + xml.version(version) + end + end + end + end + end.to_xml + end + + def versions_from(xml_content) + doc = Nokogiri::XML(xml_content) + doc.xpath('//metadata/versioning/versions/version').map(&:content) + end + + def release_from(xml_content) + doc = Nokogiri::XML(xml_content) + doc.xpath('//metadata/versioning/release').first&.content + end + + def latest_from(xml_content) + doc = Nokogiri::XML(xml_content) + doc.xpath('//metadata/versioning/latest').first&.content + end + + def last_updated_from(xml_content) + doc = Nokogiri::XML(xml_content) + doc.xpath('//metadata/versioning/lastUpdated').first.content + end +end diff --git a/spec/services/packages/maven/metadata/sync_service_spec.rb b/spec/services/packages/maven/metadata/sync_service_spec.rb new file mode 100644 index 00000000000..be298e95236 --- /dev/null +++ b/spec/services/packages/maven/metadata/sync_service_spec.rb @@ -0,0 +1,154 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe ::Packages::Maven::Metadata::SyncService do + using RSpec::Parameterized::TableSyntax + + let_it_be(:project) { create(:project) } + let_it_be(:user) { create(:user) } + let_it_be_with_reload(:versionless_package_for_versions) { create(:maven_package, name: 'test', version: nil, project: project) } + let_it_be_with_reload(:metadata_file_for_versions) { create(:package_file, :xml, package: versionless_package_for_versions) } + + let(:service) { described_class.new(container: project, current_user: user, params: { package_name: versionless_package_for_versions.name }) } + + describe '#execute' do + let(:create_versions_xml_service_double) { double(::Packages::Maven::Metadata::CreateVersionsXmlService, execute: create_versions_xml_service_response) } + let(:append_package_file_service_double) { double(::Packages::Maven::Metadata::AppendPackageFileService, execute: append_package_file_service_response) } + + let(:create_versions_xml_service_response) { ServiceResponse.success(payload: { changes_exist: true, empty_versions: false, metadata_content: 'test' }) } + let(:append_package_file_service_response) { ServiceResponse.success(message: 'New metadata package files created') } + + subject { service.execute } + + before do + allow(::Packages::Maven::Metadata::CreateVersionsXmlService) + .to receive(:new).with(metadata_content: an_instance_of(ObjectStorage::Concern::OpenFile), package: versionless_package_for_versions).and_return(create_versions_xml_service_double) + allow(::Packages::Maven::Metadata::AppendPackageFileService) + .to receive(:new).with(metadata_content: an_instance_of(String), package: versionless_package_for_versions).and_return(append_package_file_service_double) + end + + context 'permissions' do + where(:role, :expected_result) do + :anonymous | :rejected + :developer | :rejected + :maintainer | :accepted + end + + with_them do + if params[:role] == :anonymous + let_it_be(:user) { nil } + end + + before do + project.send("add_#{role}", user) unless role == :anonymous + end + + if params[:expected_result] == :rejected + it_behaves_like 'returning an error service response', message: 'Not allowed' + else + it_behaves_like 'returning a success service response', message: 'New metadata package files created' + end + end + end + + context 'with a maintainer' do + before do + project.add_maintainer(user) + end + + context 'with no changes' do + let(:create_versions_xml_service_response) { ServiceResponse.success(payload: { changes_exist: false }) } + + before do + expect(::Packages::Maven::Metadata::AppendPackageFileService).not_to receive(:new) + end + + it_behaves_like 'returning a success service response', message: 'No changes for versions xml' + end + + context 'with changes' do + let(:create_versions_xml_service_response) { ServiceResponse.success(payload: { changes_exist: true, empty_versions: false, metadata_content: 'new metadata' }) } + + it_behaves_like 'returning a success service response', message: 'New metadata package files created' + + context 'with empty versions' do + let(:create_versions_xml_service_response) { ServiceResponse.success(payload: { changes_exist: true, empty_versions: true }) } + + before do + expect(service.send(:versionless_package_for_versions)).to receive(:destroy!) + expect(::Packages::Maven::Metadata::AppendPackageFileService).not_to receive(:new) + end + + it_behaves_like 'returning a success service response', message: 'Versionless package for versions destroyed' + end + end + + context 'with a too big maven metadata file for versions' do + before do + metadata_file_for_versions.update!(size: 100.megabytes) + end + + it_behaves_like 'returning an error service response', message: 'Metadata file for versions is too big' + end + + context 'an error from the create versions xml service' do + let(:create_versions_xml_service_response) { ServiceResponse.error(message: 'metadata_content is invalid') } + + before do + expect(::Packages::Maven::Metadata::AppendPackageFileService).not_to receive(:new) + end + + it_behaves_like 'returning an error service response', message: 'metadata_content is invalid' + end + + context 'an error from the append package file service' do + let(:append_package_file_service_response) { ServiceResponse.error(message: 'metadata content is not set') } + + it_behaves_like 'returning an error service response', message: 'metadata content is not set' + end + + context 'without a package name' do + let(:service) { described_class.new(container: project, current_user: user, params: { package_name: nil }) } + + before do + expect(::Packages::Maven::Metadata::AppendPackageFileService).not_to receive(:new) + expect(::Packages::Maven::Metadata::CreateVersionsXmlService).not_to receive(:new) + end + + it_behaves_like 'returning an error service response', message: 'Blank package name' + end + + context 'without a versionless package for version' do + before do + versionless_package_for_versions.update!(version: '2.2.2') + expect(::Packages::Maven::Metadata::AppendPackageFileService).not_to receive(:new) + expect(::Packages::Maven::Metadata::CreateVersionsXmlService).not_to receive(:new) + end + + it_behaves_like 'returning an error service response', message: 'Non existing versionless package' + end + + context 'without a metadata package file for versions' do + before do + versionless_package_for_versions.package_files.update_all(file_name: 'test.txt') + expect(::Packages::Maven::Metadata::AppendPackageFileService).not_to receive(:new) + expect(::Packages::Maven::Metadata::CreateVersionsXmlService).not_to receive(:new) + end + + it_behaves_like 'returning an error service response', message: 'Non existing metadata file for versions' + end + + context 'without a project' do + let(:service) { described_class.new(container: nil, current_user: user, params: { package_name: versionless_package_for_versions.name }) } + + before do + expect(::Packages::Maven::Metadata::AppendPackageFileService).not_to receive(:new) + expect(::Packages::Maven::Metadata::CreateVersionsXmlService).not_to receive(:new) + end + + it_behaves_like 'returning an error service response', message: 'Not allowed' + end + end + end +end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 9a49c57e48c..ddda3ede083 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -304,10 +304,10 @@ RSpec.configure do |config| RequestStore.clear! end - config.around(:example, :context_aware) do |example| + config.around do |example| # Wrap each example in it's own context to make sure the contexts don't # leak - Gitlab::ApplicationContext.with_raw_context { example.run } + Labkit::Context.with_context { example.run } end config.around do |example| diff --git a/spec/support/services/service_response_shared_examples.rb b/spec/support/services/service_response_shared_examples.rb new file mode 100644 index 00000000000..186627347fb --- /dev/null +++ b/spec/support/services/service_response_shared_examples.rb @@ -0,0 +1,25 @@ +# frozen_string_literal: true + +RSpec.shared_examples 'returning an error service response' do |message: nil| + it 'returns an error service response' do + result = subject + + expect(result).to be_error + + if message + expect(result.message).to eq(message) + end + end +end + +RSpec.shared_examples 'returning a success service response' do |message: nil| + it 'returns a success service response' do + result = subject + + expect(result).to be_success + + if message + expect(result.message).to eq(message) + end + end +end diff --git a/spec/support/shared_examples/lib/api/ci/runner_shared_examples.rb b/spec/support/shared_examples/lib/api/ci/runner_shared_examples.rb index c775ca182e6..bdb0316bf5a 100644 --- a/spec/support/shared_examples/lib/api/ci/runner_shared_examples.rb +++ b/spec/support/shared_examples/lib/api/ci/runner_shared_examples.rb @@ -1,14 +1,14 @@ # frozen_string_literal: true RSpec.shared_examples 'API::CI::Runner application context metadata' do |api_route| - it 'contains correct context metadata', :context_aware do + it 'contains correct context metadata' do # Avoids popping the context from the thread so we can # check its content after the request. allow(Labkit::Context).to receive(:pop) send_request - Gitlab::ApplicationContext.with_raw_context do |context| + Labkit::Context.with_context do |context| expected_context = { 'meta.caller_id' => api_route, 'meta.user' => job.user.username, diff --git a/spec/support/shared_examples/requests/api/logging_application_context_shared_examples.rb b/spec/support/shared_examples/requests/api/logging_application_context_shared_examples.rb index 57e28e6df57..4a71b696d57 100644 --- a/spec/support/shared_examples/requests/api/logging_application_context_shared_examples.rb +++ b/spec/support/shared_examples/requests/api/logging_application_context_shared_examples.rb @@ -1,13 +1,21 @@ # frozen_string_literal: true RSpec.shared_examples 'storing arguments in the application context' do - it 'places the expected params in the application context', :context_aware do + around do |example| + Labkit::Context.with_context { example.run } + end + + it 'places the expected params in the application context' do # Stub the clearing of the context so we can validate it later + # The `around` block above makes sure we do clean it up later allow(Labkit::Context).to receive(:pop) subject - expect(Gitlab::ApplicationContext.current).to include(log_hash(expected_params)) + Labkit::Context.with_context do |context| + expect(context.to_h) + .to include(log_hash(expected_params)) + end end def log_hash(hash) diff --git a/spec/workers/background_migration_worker_spec.rb b/spec/workers/background_migration_worker_spec.rb index 4575c270042..8094efcaf04 100644 --- a/spec/workers/background_migration_worker_spec.rb +++ b/spec/workers/background_migration_worker_spec.rb @@ -101,7 +101,7 @@ RSpec.describe BackgroundMigrationWorker, :clean_gitlab_redis_shared_state do it 'sets the class that will be executed as the caller_id' do expect(Gitlab::BackgroundMigration).to receive(:perform) do - expect(Gitlab::ApplicationContext.current).to include('meta.caller_id' => 'Foo') + expect(Labkit::Context.current.to_h).to include('meta.caller_id' => 'Foo') end worker.perform('Foo', [10, 20]) diff --git a/spec/workers/concerns/worker_context_spec.rb b/spec/workers/concerns/worker_context_spec.rb index ebdb752d900..3de37b99aba 100644 --- a/spec/workers/concerns/worker_context_spec.rb +++ b/spec/workers/concerns/worker_context_spec.rb @@ -103,7 +103,7 @@ RSpec.describe WorkerContext do describe '#with_context' do it 'allows modifying context when the job is running' do worker.new.with_context(user: build_stubbed(:user, username: 'jane-doe')) do - expect(Gitlab::ApplicationContext.current).to include('meta.user' => 'jane-doe') + expect(Labkit::Context.current.to_h).to include('meta.user' => 'jane-doe') end end diff --git a/spec/workers/packages/maven/metadata/sync_worker_spec.rb b/spec/workers/packages/maven/metadata/sync_worker_spec.rb new file mode 100644 index 00000000000..e3e0e598264 --- /dev/null +++ b/spec/workers/packages/maven/metadata/sync_worker_spec.rb @@ -0,0 +1,133 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Packages::Maven::Metadata::SyncWorker, type: :worker do + let_it_be(:versionless_package_for_versions) { create(:maven_package, name: 'MyDummyMavenPkg', version: nil) } + let_it_be(:metadata_package_file) { create(:package_file, :xml, package: versionless_package_for_versions) } + + let(:versions) { %w[1.2 1.1 2.1 3.0-SNAPSHOT] } + let(:worker) { described_class.new } + + describe '#perform' do + let(:user) { create(:user) } + let(:project) { versionless_package_for_versions.project } + let(:package_name) { versionless_package_for_versions.name } + let(:role) { :maintainer } + let(:most_recent_metadata_file_for_versions) { versionless_package_for_versions.package_files.recent.with_file_name(Packages::Maven::Metadata.filename).first } + + before do + project.send("add_#{role}", user) + end + + subject { worker.perform(user.id, project.id, package_name) } + + context 'with a valid package name' do + before do + file = CarrierWaveStringFile.new_file(file_content: versions_xml_content, filename: 'maven-metadata.xml', content_type: 'application/xml') + metadata_package_file.update!(file: file) + + versions.each do |version| + create(:maven_package, name: versionless_package_for_versions.name, version: version, project: versionless_package_for_versions.project) + end + end + + context 'idempotent worker' do + include_examples 'an idempotent worker' do + let(:job_args) { [user.id, project.id, package_name] } + + it 'creates the updated metadata files', :aggregate_failures do + expect { subject }.to change { ::Packages::PackageFile.count }.by(5) + + most_recent_versions = versions_from(most_recent_metadata_file_for_versions.file.read) + expect(most_recent_versions.latest).to eq('3.0-SNAPSHOT') + expect(most_recent_versions.release).to eq('2.1') + expect(most_recent_versions.versions).to match_array(versions) + end + end + end + + it 'logs the message from the service' do + expect(worker).to receive(:log_extra_metadata_on_done).with(:message, 'New metadata package file created') + + subject + end + + context 'not in the passed project' do + let(:project) { create(:project) } + + it 'does not create the updated metadata files' do + expect { subject } + .to change { ::Packages::PackageFile.count }.by(0) + .and raise_error(described_class::SyncError, 'Non existing versionless package') + end + end + + context 'with a user with not enough permissions' do + let(:role) { :guest } + + it 'does not create the updated metadata files' do + expect { subject } + .to change { ::Packages::PackageFile.count }.by(0) + .and raise_error(described_class::SyncError, 'Not allowed') + end + end + end + + context 'with no package name' do + subject { worker.perform(user.id, project.id, nil) } + + it 'does not run' do + expect(::Packages::Maven::Metadata::SyncService).not_to receive(:new) + expect { subject }.not_to change { ::Packages::PackageFile.count } + end + end + + context 'with no user id' do + subject { worker.perform(nil, project.id, package_name) } + + it 'does not run' do + expect(::Packages::Maven::Metadata::SyncService).not_to receive(:new) + expect { subject }.not_to change { ::Packages::PackageFile.count } + end + end + + context 'with no project id' do + subject { worker.perform(user.id, nil, package_name) } + + it 'does not run' do + expect(::Packages::Maven::Metadata::SyncService).not_to receive(:new) + expect { subject }.not_to change { ::Packages::PackageFile.count } + end + end + end + + def versions_from(xml_content) + xml_doc = Nokogiri::XML(xml_content) + + OpenStruct.new( + release: xml_doc.xpath('//metadata/versioning/release').first.content, + latest: xml_doc.xpath('//metadata/versioning/latest').first.content, + versions: xml_doc.xpath('//metadata/versioning/versions/version').map(&:content) + ) + end + + def versions_xml_content + Nokogiri::XML::Builder.new do |xml| + xml.metadata do + xml.groupId(versionless_package_for_versions.maven_metadatum.app_group) + xml.artifactId(versionless_package_for_versions.maven_metadatum.app_name) + xml.versioning do + xml.release('1.3') + xml.latest('1.3') + xml.lastUpdated('20210113130531') + xml.versions do + xml.version('1.1') + xml.version('1.2') + xml.version('1.3') + end + end + end + end.to_xml + end +end |