Automatic merge of gitlab-org/gitlab master

app/models/group_import_state.rb

@@ -3,6 +3,8 @@
 class GroupImportState < ApplicationRecord
   self.primary_key = :group_id
 
+  MAX_ERROR_LENGTH = 255
+
   belongs_to :group, inverse_of: :import_state
   belongs_to :user, optional: false
 
@@ -30,7 +32,7 @@ class GroupImportState < ApplicationRecord
     after_transition any => :failed do |state, transition|
       last_error = transition.args.first
 
-      state.update_column(:last_error, last_error) if last_error
+      state.update_column(:last_error, last_error.truncate(MAX_ERROR_LENGTH)) if last_error
     end
   end
 

changelogs/unreleased/273411-fj-fix-transient-error-in-branch-checking.yml

+---
+title: Avoid branch name checking when creating a new snippet
+merge_request: 48995
+author:
+type: fixed

changelogs/unreleased/georgekoltsov-add-attribute-cleaner-transformer.yml

+---
+title: Add Attributes cleaner to Group Migration
+merge_request: 48374
+author:
+type: changed

changelogs/unreleased/georgekoltsov-truncate-last-error-group-import.yml

+---
+title: Fix failed group imports getting stuck by long error messages
+merge_request: 48989
+author:
+type: fixed

doc/api/graphql/reference/gitlab_schema.graphql

@@ -1230,12 +1230,12 @@ Represents a project or group board
 """
 type Board {
   """
-  The board assignee.
+  The board assignee
   """
   assignee: User
 
   """
-  Epics associated with board issues.
+  Epics associated with board issues
   """
   epics(
     """
@@ -1265,12 +1265,12 @@ type Board {
   ): BoardEpicConnection
 
   """
-  Whether or not backlog list is hidden.
+  Whether or not backlog list is hidden
   """
   hideBacklogList: Boolean
 
   """
-  Whether or not closed list is hidden.
+  Whether or not closed list is hidden
   """
   hideClosedList: Boolean
 
@@ -1340,7 +1340,7 @@ type Board {
   ): BoardListConnection
 
   """
-  The board milestone.
+  The board milestone
   """
   milestone: Milestone
 
@@ -1350,7 +1350,7 @@ type Board {
   name: String
 
   """
-  Weight of the board.
+  Weight of the board
   """
   weight: Int
 }
@@ -22658,12 +22658,12 @@ input UpdateBoardInput {
   clientMutationId: String
 
   """
-  Whether or not backlog list is hidden.
+  Whether or not backlog list is hidden
   """
   hideBacklogList: Boolean
 
   """
-  Whether or not closed list is hidden.
+  Whether or not closed list is hidden
   """
   hideClosedList: Boolean
 

doc/api/graphql/reference/gitlab_schema.json

@@ -3305,7 +3305,7 @@
           "fields": [
             {
               "name": "assignee",
-              "description": "The board assignee.",
+              "description": "The board assignee",
               "args": [
 
               ],
@@ -3319,7 +3319,7 @@
             },
             {
               "name": "epics",
-              "description": "Epics associated with board issues.",
+              "description": "Epics associated with board issues",
               "args": [
                 {
                   "name": "issueFilters",
@@ -3382,7 +3382,7 @@
             },
             {
               "name": "hideBacklogList",
-              "description": "Whether or not backlog list is hidden.",
+              "description": "Whether or not backlog list is hidden",
               "args": [
 
               ],
@@ -3396,7 +3396,7 @@
             },
             {
               "name": "hideClosedList",
-              "description": "Whether or not closed list is hidden.",
+              "description": "Whether or not closed list is hidden",
               "args": [
 
               ],
@@ -3554,7 +3554,7 @@
             },
             {
               "name": "milestone",
-              "description": "The board milestone.",
+              "description": "The board milestone",
               "args": [
 
               ],
@@ -3582,7 +3582,7 @@
             },
             {
               "name": "weight",
-              "description": "Weight of the board.",
+              "description": "Weight of the board",
               "args": [
 
               ],
@@ -66126,7 +66126,7 @@
             },
             {
               "name": "hideBacklogList",
-              "description": "Whether or not backlog list is hidden.",
+              "description": "Whether or not backlog list is hidden",
               "type": {
                 "kind": "SCALAR",
                 "name": "Boolean",
@@ -66136,7 +66136,7 @@
             },
             {
               "name": "hideClosedList",
-              "description": "Whether or not closed list is hidden.",
+              "description": "Whether or not closed list is hidden",
               "type": {
                 "kind": "SCALAR",
                 "name": "Boolean",

doc/api/graphql/reference/index.md

@@ -236,16 +236,16 @@ Represents a project or group board.
 
 | Field | Type | Description |
 | ----- | ---- | ----------- |
-| `assignee` | User | The board assignee. |
-| `epics` | BoardEpicConnection | Epics associated with board issues. |
-| `hideBacklogList` | Boolean | Whether or not backlog list is hidden. |
-| `hideClosedList` | Boolean | Whether or not closed list is hidden. |
+| `assignee` | User | The board assignee |
+| `epics` | BoardEpicConnection | Epics associated with board issues |
+| `hideBacklogList` | Boolean | Whether or not backlog list is hidden |
+| `hideClosedList` | Boolean | Whether or not closed list is hidden |
 | `id` | ID! | ID (global ID) of the board |
 | `labels` | LabelConnection | Labels of the board |
 | `lists` | BoardListConnection | Lists of the board |
-| `milestone` | Milestone | The board milestone. |
+| `milestone` | Milestone | The board milestone |
 | `name` | String | Name of the board |
-| `weight` | Int | Weight of the board. |
+| `weight` | Int | Weight of the board |
 
 ### BoardEpic
 

ee/app/graphql/ee/types/board_type.rb

@@ -7,27 +7,27 @@ module EE
 
       prepended do
         field :assignee, type: ::Types::UserType, null: true,
-              description: 'The board assignee.'
+              description: 'The board assignee'
 
         field :epics, ::Types::Boards::BoardEpicType.connection_type, null: true,
-              description: 'Epics associated with board issues.',
+              description: 'Epics associated with board issues',
               resolver: ::Resolvers::BoardGroupings::EpicsResolver,
               complexity: 5
 
         field :hide_backlog_list, type: GraphQL::BOOLEAN_TYPE, null: true,
-              description: 'Whether or not backlog list is hidden.'
+              description: 'Whether or not backlog list is hidden'
 
         field :hide_closed_list, type: GraphQL::BOOLEAN_TYPE, null: true,
-              description: 'Whether or not closed list is hidden.'
+              description: 'Whether or not closed list is hidden'
 
         field :labels, ::Types::LabelType.connection_type, null: true,
               description: 'Labels of the board'
 
         field :milestone, type: ::Types::MilestoneType, null: true,
-              description: 'The board milestone.'
+              description: 'The board milestone'
 
         field :weight, type: GraphQL::INT_TYPE, null: true,
-              description: 'Weight of the board.'
+              description: 'Weight of the board'
       end
     end
   end

ee/lib/bulk_imports/ee/groups/pipelines/epics_pipeline.rb

@@ -13,6 +13,7 @@ module BulkImports
           transformer ::BulkImports::Common::Transformers::HashKeyDigger,
             key_path: %w[data group epics]
           transformer ::BulkImports::Common::Transformers::UnderscorifyKeysTransformer
+          transformer ::BulkImports::Common::Transformers::ProhibitedAttributesTransformer
 
           loader BulkImports::EE::Groups::Loaders::EpicsLoader
 

ee/spec/lib/bulk_imports/ee/groups/pipelines/epics_pipeline_spec.rb

@@ -61,7 +61,8 @@ RSpec.describe BulkImports::EE::Groups::Pipelines::EpicsPipeline do
       expect(described_class.transformers)
         .to contain_exactly(
           { klass: BulkImports::Common::Transformers::HashKeyDigger, options: { key_path: %w[data group epics] } },
-          { klass: BulkImports::Common::Transformers::UnderscorifyKeysTransformer, options: nil }
+          { klass: BulkImports::Common::Transformers::UnderscorifyKeysTransformer, options: nil },
+          { klass: BulkImports::Common::Transformers::ProhibitedAttributesTransformer, options: nil }
         )
     end
 

lib/bulk_imports/common/transformers/prohibited_attributes_transformer.rb

+# frozen_string_literal: true
+
+module BulkImports
+  module Common
+    module Transformers
+      class ProhibitedAttributesTransformer
+        PROHIBITED_REFERENCES = Regexp.union(
+          /\Acached_markdown_version\Z/,
+          /\Aid\Z/,
+          /_id\Z/,
+          /_ids\Z/,
+          /_html\Z/,
+          /attributes/,
+          /\Aremote_\w+_(url|urls|request_header)\Z/ # carrierwave automatically creates these attribute methods for uploads
+        ).freeze
+
+        def initialize(options = {})
+          @options = options
+        end
+
+        def transform(context, data)
+          data.each_with_object({}) do |(key, value), result|
+            prohibited = prohibited_key?(key)
+
+            unless prohibited
+              result[key] = value.is_a?(Hash) ? transform(context, value) : value
+            end
+          end
+        end
+
+        private
+
+        def prohibited_key?(key)
+          key.to_s =~ PROHIBITED_REFERENCES
+        end
+      end
+    end
+  end
+end

lib/bulk_imports/groups/pipelines/group_pipeline.rb

@@ -12,6 +12,7 @@ module BulkImports
 
         transformer Common::Transformers::HashKeyDigger, key_path: %w[data group]
         transformer Common::Transformers::UnderscorifyKeysTransformer
+        transformer Common::Transformers::ProhibitedAttributesTransformer
         transformer Groups::Transformers::GroupAttributesTransformer
 
         loader Groups::Loaders::GroupLoader

lib/bulk_imports/groups/pipelines/subgroup_entities_pipeline.rb

@@ -7,6 +7,7 @@ module BulkImports
         include Pipeline
 
         extractor BulkImports::Groups::Extractors::SubgroupsExtractor
+        transformer Common::Transformers::ProhibitedAttributesTransformer
         transformer BulkImports::Groups::Transformers::SubgroupToEntityTransformer
         loader BulkImports::Common::Loaders::EntityLoader
       end

lib/gitlab/checks/snippet_check.rb

@@ -10,12 +10,13 @@ module Gitlab
       ATTRIBUTES = %i[oldrev newrev ref branch_name tag_name logger].freeze
       attr_reader(*ATTRIBUTES)
 
-      def initialize(change, default_branch:, logger:)
+      def initialize(change, default_branch:, root_ref:, logger:)
         @oldrev, @newrev, @ref = change.values_at(:oldrev, :newrev, :ref)
         @branch_name = Gitlab::Git.branch_name(@ref)
         @tag_name = Gitlab::Git.tag_name(@ref)
 
         @default_branch = default_branch
+        @root_ref = root_ref
         @logger = logger
         @logger.append_message("Running checks for ref: #{@branch_name || @tag_name}")
       end
@@ -34,8 +35,13 @@ module Gitlab
 
       private
 
+      # If the `root_ref` is not present means that this is the first commit to the
+      # repository and when the default branch is going to be created.
+      # We allow the first branch creation no matter the name because
+      # it can be even an imported snippet from an instance with a different
+      # default branch.
       def creation?
-        @branch_name != @default_branch && super
+        super && @root_ref && (@branch_name != @default_branch)
       end
     end
   end

lib/gitlab/git_access_snippet.rb

@@ -114,7 +114,7 @@ module Gitlab
 
     override :check_single_change_access
     def check_single_change_access(change, _skip_lfs_integrity_check: false)
-      Checks::SnippetCheck.new(change, default_branch: snippet.default_branch, logger: logger).validate!
+      Checks::SnippetCheck.new(change, default_branch: snippet.default_branch, root_ref: snippet.repository.root_ref, logger: logger).validate!
       Checks::PushFileCountCheck.new(change, repository: repository, limit: Snippet.max_file_limit, logger: logger).validate!
     rescue Checks::TimedLogger::TimeoutError
       raise TimeoutError, logger.full_message

spec/lib/bulk_imports/common/transformers/prohibited_attributes_transformer_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe BulkImports::Common::Transformers::ProhibitedAttributesTransformer do
+  describe '#transform' do
+    let_it_be(:hash) do
+      {
+        'id' => 101,
+        'service_id' => 99,
+        'moved_to_id' => 99,
+        'namespace_id' => 99,
+        'ci_id' => 99,
+        'random_project_id' => 99,
+        'random_id' => 99,
+        'milestone_id' => 99,
+        'project_id' => 99,
+        'user_id' => 99,
+        'random_id_in_the_middle' => 99,
+        'notid' => 99,
+        'import_source' => 'test',
+        'import_type' => 'test',
+        'non_existent_attr' => 'test',
+        'some_html' => '<p>dodgy html</p>',
+        'legit_html' => '<p>legit html</p>',
+        '_html' => '<p>perfectly ordinary html</p>',
+        'cached_markdown_version' => 12345,
+        'custom_attributes' => 'test',
+        'some_attributes_metadata' => 'test',
+        'group_id' => 99,
+        'commit_id' => 99,
+        'issue_ids' => [1, 2, 3],
+        'merge_request_ids' => [1, 2, 3],
+        'note_ids' => [1, 2, 3],
+        'remote_attachment_url' => 'http://something.dodgy',
+        'remote_attachment_request_header' => 'bad value',
+        'remote_attachment_urls' => %w(http://something.dodgy http://something.okay),
+        'attributes' => {
+          'issue_ids' => [1, 2, 3],
+          'merge_request_ids' => [1, 2, 3],
+          'note_ids' => [1, 2, 3]
+        },
+        'variables_attributes' => {
+          'id' => 1
+        },
+        'attr_with_nested_attrs' => {
+          'nested_id' => 1,
+          'nested_attr' => 2
+        }
+      }
+    end
+
+    let(:expected_hash) do
+      {
+        'random_id_in_the_middle' => 99,
+        'notid' => 99,
+        'import_source' => 'test',
+        'import_type' => 'test',
+        'non_existent_attr' => 'test',
+        'attr_with_nested_attrs' => {
+          'nested_attr' => 2
+        }
+      }
+    end
+
+    it 'removes prohibited attributes' do
+      transformed_hash = subject.transform(nil, hash)
+
+      expect(transformed_hash).to eq(expected_hash)
+    end
+  end
+end

spec/lib/bulk_imports/groups/pipelines/group_pipeline_spec.rb

@@ -91,6 +91,7 @@ RSpec.describe BulkImports::Groups::Pipelines::GroupPipeline do
         .to contain_exactly(
           { klass: BulkImports::Common::Transformers::HashKeyDigger, options: { key_path: %w[data group] } },
           { klass: BulkImports::Common::Transformers::UnderscorifyKeysTransformer, options: nil },
+          { klass: BulkImports::Common::Transformers::ProhibitedAttributesTransformer, options: nil },
           { klass: BulkImports::Groups::Transformers::GroupAttributesTransformer, options: nil }
         )
     end

spec/lib/bulk_imports/groups/pipelines/subgroup_entities_pipeline_spec.rb

@@ -66,8 +66,8 @@ RSpec.describe BulkImports::Groups::Pipelines::SubgroupEntitiesPipeline do
 
     it 'has transformers' do
       expect(described_class.transformers).to contain_exactly(
-        klass: BulkImports::Groups::Transformers::SubgroupToEntityTransformer,
-        options: nil
+        { klass: BulkImports::Common::Transformers::ProhibitedAttributesTransformer, options: nil },
+        { klass: BulkImports::Groups::Transformers::SubgroupToEntityTransformer, options: nil }
       )
     end
 

spec/lib/gitlab/checks/snippet_check_spec.rb

@@ -13,7 +13,7 @@ RSpec.describe Gitlab::Checks::SnippetCheck do
   let(:creation) { false }
   let(:deletion) { false }
 
-  subject { Gitlab::Checks::SnippetCheck.new(changes, default_branch: default_branch, logger: logger) }
+  subject { Gitlab::Checks::SnippetCheck.new(changes, default_branch: default_branch, root_ref: snippet.repository.root_ref, logger: logger) }
 
   describe '#validate!' do
     it 'does not raise any error' do
@@ -45,10 +45,18 @@ RSpec.describe Gitlab::Checks::SnippetCheck do
         let(:branch_name) { 'feature' }
       end
 
-      context "when branch is 'master'" do
-        let(:ref) { 'refs/heads/master' }
+      context 'when branch is the same as the default branch' do
+        let(:ref) { "refs/heads/#{default_branch}" }
 
-        it "allows the operation" do
+        it 'allows the operation' do
+          expect { subject.validate! }.not_to raise_error
+        end
+      end
+
+      context 'when snippet has an empty repo' do
+        let_it_be(:snippet) { create(:personal_snippet, :empty_repo) }
+
+        it 'allows the operation' do
           expect { subject.validate! }.not_to raise_error
         end
       end

spec/models/group_import_state_spec.rb

@@ -70,4 +70,24 @@ RSpec.describe GroupImportState do
       end
     end
   end
+
+  context 'when import failed' do
+    context 'when error message is present' do
+      it 'truncates error message' do
+        group_import_state = build(:group_import_state, :started)
+        group_import_state.fail_op('e' * 300)
+
+        expect(group_import_state.last_error.length).to eq(255)
+      end
+    end
+
+    context 'when error message is missing' do
+      it 'has no error message' do
+        group_import_state = build(:group_import_state, :started)
+        group_import_state.fail_op
+
+        expect(group_import_state.last_error).to be_nil
+      end
+    end
+  end
 end

spec/models/service_spec.rb

@@ -245,7 +245,7 @@ RSpec.describe Service do
 
       context 'with a previous existing service (MockCiService) and a new service (Asana)' do
         before do
-          Service.insert(type: 'MockCiService', instance: true)
+          Service.insert({ type: 'MockCiService', instance: true })
           Service.delete_by(type: 'AsanaService', instance: true)
         end
 
@@ -291,7 +291,7 @@ RSpec.describe Service do
 
         context 'with a previous existing service (Previous) and a new service (Asana)' do
           before do
-            Service.insert(type: 'PreviousService', template: true)
+            Service.insert({ type: 'PreviousService', template: true })
             Service.delete_by(type: 'AsanaService', template: true)
           end