Automatic merge of gitlab-org/gitlab-ce master

app/models/concerns/issuable.rb

@@ -28,7 +28,7 @@ module Issuable
   # This object is used to gather issuable meta data for displaying
   # upvotes, downvotes, notes and closing merge requests count for issues and merge requests
   # lists avoiding n+1 queries and improving performance.
-  IssuableMeta = Struct.new(:upvotes, :downvotes, :notes_count, :merge_requests_count)
+  IssuableMeta = Struct.new(:upvotes, :downvotes, :user_notes_count, :merge_requests_count)
 
   included do
     cache_markdown_field :title, pipeline: :single_line
@@ -36,8 +36,8 @@ module Issuable
 
     redact_field :description
 
-    belongs_to :author, class_name: "User"
-    belongs_to :updated_by, class_name: "User"
+    belongs_to :author, class_name: 'User'
+    belongs_to :updated_by, class_name: 'User'
     belongs_to :last_edited_by, class_name: 'User'
     belongs_to :milestone
 

app/models/issue.rb

@@ -263,6 +263,10 @@ class Issue < ActiveRecord::Base
   end
   # rubocop: enable CodeReuse/ServiceClass
 
+  def merge_requests_count
+    merge_requests_closing_issues.count
+  end
+
   private
 
   def ensure_metrics

app/views/shared/_issuable_meta_data.html.haml

-- note_count         = @issuable_meta_data[issuable.id].notes_count
+- note_count         = @issuable_meta_data[issuable.id].user_notes_count
 - issue_votes        = @issuable_meta_data[issuable.id]
 - upvotes, downvotes = issue_votes.upvotes, issue_votes.downvotes
 - issuable_url       = @collection_type == "Issue" ? issue_path(issuable, anchor: 'notes') : merge_request_path(issuable, anchor: 'notes')

changelogs/unreleased/56726-fix-n-1-in-issues-and-merge-requests-api.yml

+---
+title: Fix N+1 query in Issues and MergeRequest API when issuable_metadata is present
+merge_request: 25042
+author: Alex Koval
+type: other

changelogs/unreleased/include-ci-yaml.yml

+---
+title: Validate 'include' keywords in gitlab-ci.yml configuration files.
+merge_request: 24098
+author: Paul Bonaud
+type: fixed

lib/api/entities.rb

@@ -473,6 +473,12 @@ module API
       expose(:project_id) { |entity| entity&.project.try(:id) }
       expose :title, :description
       expose :state, :created_at, :updated_at
+
+      # Avoids an N+1 query when metadata is included
+      def issuable_metadata(subject, options, method)
+        cached_subject = options.dig(:issuable_metadata, subject.id)
+        (cached_subject || subject).public_send(method) # rubocop: disable GitlabSecurity/PublicSend
+      end
     end
 
     class Diff < Grape::Entity
@@ -520,54 +526,32 @@ module API
     class IssueBasic < ProjectEntity
       expose :closed_at
       expose :closed_by, using: Entities::UserBasic
-      expose :labels do |issue, options|
+      expose :labels do |issue|
         # Avoids an N+1 query since labels are preloaded
         issue.labels.map(&:title).sort
       end
       expose :milestone, using: Entities::Milestone
       expose :assignees, :author, using: Entities::UserBasic
 
-      expose :assignee, using: ::API::Entities::UserBasic do |issue, options|
+      expose :assignee, using: ::API::Entities::UserBasic do |issue|
         issue.assignees.first
       end
 
-      expose :user_notes_count
-      expose :upvotes do |issue, options|
-        if options[:issuable_metadata]
-          # Avoids an N+1 query when metadata is included
-          options[:issuable_metadata][issue.id].upvotes
-        else
-          issue.upvotes
-        end
-      end
-      expose :downvotes do |issue, options|
-        if options[:issuable_metadata]
-          # Avoids an N+1 query when metadata is included
-          options[:issuable_metadata][issue.id].downvotes
-        else
-          issue.downvotes
-        end
-      end
+      expose(:user_notes_count)     { |issue, options| issuable_metadata(issue, options, :user_notes_count) }
+      expose(:merge_requests_count) { |issue, options| issuable_metadata(issue, options, :merge_requests_count) }
+      expose(:upvotes)              { |issue, options| issuable_metadata(issue, options, :upvotes) }
+      expose(:downvotes)            { |issue, options| issuable_metadata(issue, options, :downvotes) }
       expose :due_date
       expose :confidential
       expose :discussion_locked
 
-      expose :web_url do |issue, options|
+      expose :web_url do |issue|
         Gitlab::UrlBuilder.build(issue)
       end
 
       expose :time_stats, using: 'API::Entities::IssuableTimeStats' do |issue|
         issue
       end
-
-      expose :merge_requests_count do |issue, options|
-        if options[:issuable_metadata]
-          # Avoids an N+1 query when metadata is included
-          options[:issuable_metadata][issue.id].merge_requests_count
-        else
-          issue.merge_requests_closing_issues.count
-        end
-      end
     end
 
     class Issue < IssueBasic
@@ -661,23 +645,12 @@ module API
         MarkupHelper.markdown_field(entity, :description)
       end
       expose :target_branch, :source_branch
-      expose :upvotes do |merge_request, options|
-        if options[:issuable_metadata]
-          options[:issuable_metadata][merge_request.id].upvotes
-        else
-          merge_request.upvotes
-        end
-      end
-      expose :downvotes do |merge_request, options|
-        if options[:issuable_metadata]
-          options[:issuable_metadata][merge_request.id].downvotes
-        else
-          merge_request.downvotes
-        end
-      end
+      expose(:user_notes_count) { |merge_request, options| issuable_metadata(merge_request, options, :user_notes_count) }
+      expose(:upvotes)          { |merge_request, options| issuable_metadata(merge_request, options, :upvotes) }
+      expose(:downvotes)        { |merge_request, options| issuable_metadata(merge_request, options, :downvotes) }
       expose :author, :assignee, using: Entities::UserBasic
       expose :source_project_id, :target_project_id
-      expose :labels do |merge_request, options|
+      expose :labels do |merge_request|
         # Avoids an N+1 query since labels are preloaded
         merge_request.labels.map(&:title).sort
       end
@@ -695,7 +668,6 @@ module API
       end
       expose :diff_head_sha, as: :sha
       expose :merge_commit_sha
-      expose :user_notes_count
       expose :discussion_locked
       expose :should_remove_source_branch?, as: :should_remove_source_branch
       expose :force_remove_source_branch?, as: :force_remove_source_branch
@@ -703,7 +675,7 @@ module API
       # Deprecated
       expose :allow_collaboration, as: :allow_maintainer_to_push, if: -> (merge_request, _) { merge_request.for_fork? }
 
-      expose :web_url do |merge_request, options|
+      expose :web_url do |merge_request|
         Gitlab::UrlBuilder.build(merge_request)
       end
 

lib/gitlab/ci/config/entry/global.rb

@@ -17,6 +17,9 @@ module Gitlab
           entry :image, Entry::Image,
             description: 'Docker image that will be used to execute jobs.'
 
+          entry :include, Entry::Includes,
+                description: 'List of external YAML files to include.'
+
           entry :services, Entry::Services,
             description: 'Docker images that will be linked to the container.'
 

lib/gitlab/ci/config/entry/include.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Ci
+    class Config
+      module Entry
+        ##
+        # Entry that represents a single include.
+        #
+        class Include < ::Gitlab::Config::Entry::Node
+          include ::Gitlab::Config::Entry::Validatable
+
+          ALLOWED_KEYS = %i[local file remote template].freeze
+
+          validations do
+            validates :config, hash_or_string: true
+            validates :config, allowed_keys: ALLOWED_KEYS
+          end
+        end
+      end
+    end
+  end
+end

lib/gitlab/ci/config/entry/includes.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Ci
+    class Config
+      module Entry
+        ##
+        # Entry that represents a list of include.
+        #
+        class Includes < ::Gitlab::Config::Entry::Node
+          include ::Gitlab::Config::Entry::Validatable
+
+          validations do
+            validates :config, type: Array
+          end
+
+          def self.aspects
+            super.append -> do
+              @config = Array.wrap(@config)
+
+              @config.each_with_index do |config, i|
+                @entries[i] = ::Gitlab::Config::Entry::Factory.new(Entry::Include)
+                                .value(config || {})
+                                .create!
+              end
+            end
+          end
+        end
+      end
+    end
+  end
+end

spec/lib/gitlab/ci/config/entry/global_spec.rb

@@ -13,7 +13,7 @@ describe Gitlab::Ci::Config::Entry::Global do
         expect(described_class.nodes.keys)
           .to match_array(%i[before_script image services
                              after_script variables stages
-                             types cache])
+                             types cache include])
       end
     end
   end
@@ -42,7 +42,7 @@ describe Gitlab::Ci::Config::Entry::Global do
         end
 
         it 'creates node object for each entry' do
-          expect(global.descendants.count).to eq 8
+          expect(global.descendants.count).to eq 9
         end
 
         it 'creates node object using valid class' do
@@ -189,7 +189,7 @@ describe Gitlab::Ci::Config::Entry::Global do
 
       describe '#nodes' do
         it 'instantizes all nodes' do
-          expect(global.descendants.count).to eq 8
+          expect(global.descendants.count).to eq 9
         end
 
         it 'contains unspecified nodes' do

spec/lib/gitlab/ci/yaml_processor_spec.rb

@@ -602,6 +602,85 @@ module Gitlab
         end
       end
 
+      describe "Include" do
+        let(:opts) { {} }
+
+        let(:config) do
+          {
+            include: include_content,
+            rspec: { script: "test" }
+          }
+        end
+
+        subject { Gitlab::Ci::YamlProcessor.new(YAML.dump(config), opts) }
+
+        context "when validating a ci config file with no project context" do
+          context "when an array is provided" do
+            let(:include_content) { ["/local.gitlab-ci.yml"] }
+
+            it "does not return any error" do
+              expect { subject }.not_to raise_error
+            end
+          end
+
+          context "when an array of wrong keyed object is provided" do
+            let(:include_content) { [{ yolo: "/local.gitlab-ci.yml" }] }
+
+            it "returns a validation error" do
+              expect { subject }.to raise_error(Gitlab::Ci::YamlProcessor::ValidationError)
+            end
+          end
+
+          context "when an array of mixed typed objects is provided" do
+            let(:include_content) do
+              [
+                'https://gitlab.com/awesome-project/raw/master/.before-script-template.yml',
+                '/templates/.after-script-template.yml',
+                { template: 'Auto-DevOps.gitlab-ci.yml' }
+              ]
+            end
+
+            it "does not return any error" do
+              expect { subject }.not_to raise_error
+            end
+          end
+
+          context "when the include type is incorrect" do
+            let(:include_content) { { name: "/local.gitlab-ci.yml" } }
+
+            it "returns an invalid configuration error" do
+              expect { subject }.to raise_error(Gitlab::Ci::YamlProcessor::ValidationError)
+            end
+          end
+        end
+
+        context "when validating a ci config file within a project" do
+          let(:include_content) { "/local.gitlab-ci.yml" }
+          let(:project) { create(:project, :repository) }
+          let(:opts) { { project: project, sha: project.commit.sha } }
+
+          context "when the included internal file is present" do
+            before do
+              expect(project.repository).to receive(:blob_data_at)
+                .and_return(YAML.dump({ job1: { script: 'hello' } }))
+            end
+
+            it "does not return an error" do
+              expect { subject }.not_to raise_error
+            end
+          end
+
+          context "when the included internal file is not present" do
+            it "returns an error with missing file details" do
+              expect { subject }.to raise_error(
+                Gitlab::Ci::YamlProcessor::ValidationError,
+                "Local file `#{include_content}` does not exist!"
+              )
+            end
+          end
+        end
+      end
+
       describe "When" do
         %w(on_success on_failure always).each do |when_state|
           it "returns #{when_state} when defined" do

spec/lib/gitlab/issuable_metadata_spec.rb

@@ -28,12 +28,12 @@ describe Gitlab::IssuableMetadata do
       expect(data.count).to eq(2)
       expect(data[issue.id].upvotes).to eq(1)
       expect(data[issue.id].downvotes).to eq(0)
-      expect(data[issue.id].notes_count).to eq(0)
+      expect(data[issue.id].user_notes_count).to eq(0)
       expect(data[issue.id].merge_requests_count).to eq(1)
 
       expect(data[closed_issue.id].upvotes).to eq(0)
       expect(data[closed_issue.id].downvotes).to eq(1)
-      expect(data[closed_issue.id].notes_count).to eq(0)
+      expect(data[closed_issue.id].user_notes_count).to eq(0)
       expect(data[closed_issue.id].merge_requests_count).to eq(0)
     end
   end
@@ -51,12 +51,12 @@ describe Gitlab::IssuableMetadata do
       expect(data.count).to eq(2)
       expect(data[merge_request.id].upvotes).to eq(1)
       expect(data[merge_request.id].downvotes).to eq(1)
-      expect(data[merge_request.id].notes_count).to eq(1)
+      expect(data[merge_request.id].user_notes_count).to eq(1)
       expect(data[merge_request.id].merge_requests_count).to eq(0)
 
       expect(data[merge_request_closed.id].upvotes).to eq(0)
       expect(data[merge_request_closed.id].downvotes).to eq(0)
-      expect(data[merge_request_closed.id].notes_count).to eq(0)
+      expect(data[merge_request_closed.id].user_notes_count).to eq(0)
       expect(data[merge_request_closed.id].merge_requests_count).to eq(0)
     end
   end

spec/requests/api/merge_requests_spec.rb

@@ -320,6 +320,18 @@ describe API::MergeRequests do
       expect(json_response.first['title']).to eq merge_request_closed.title
       expect(json_response.first['id']).to eq merge_request_closed.id
     end
+
+    it 'avoids N+1 queries' do
+      control = ActiveRecord::QueryRecorder.new do
+        get api("/projects/#{project.id}/merge_requests", user)
+      end.count
+
+      create(:merge_request, author: user, assignee: user, source_project: project, target_project: project, created_at: base_time)
+
+      expect do
+        get api("/projects/#{project.id}/merge_requests", user)
+      end.not_to exceed_query_limit(control)
+    end
   end
 
   describe "GET /groups/:id/merge_requests" do