Merge branch '208723-pipelines-licenses' into 'master'

Remove SELECT N+1 on software license policies

See merge request gitlab-org/gitlab!34866

ee/app/controllers/ee/projects/pipelines_controller.rb

@@ -6,6 +6,10 @@ module EE
       extend ActiveSupport::Concern
       extend ::Gitlab::Utils::Override
 
+      prepended do
+        before_action :authorize_read_licenses!, only: [:licenses]
+      end
+
       def security
         if pipeline.expose_security_dashboard?
           render_show
@@ -16,17 +20,14 @@ module EE
 
       def licenses
         report_exists = pipeline.expose_license_scanning_data?
+        return access_to_licenses_denied! unless report_exists
 
         respond_to do |format|
-          if report_exists
-            format.html { render_show }
-            format.json do
-              data = LicenseScanningReportsSerializer.new(project: project, current_user: current_user).represent(pipeline&.license_scanning_report&.licenses)
-              render json: data, status: :ok
-            end
-          else
-            format.html { redirect_to pipeline_path(pipeline) }
-            format.json { head :not_found }
+          format.html { render_show }
+          format.json do
+            render status: :ok, json: LicenseScanningReportsSerializer.new.represent(
+              project.license_compliance(pipeline).find_policies(detected_only: true)
+            )
           end
         end
       end
@@ -34,6 +35,21 @@ module EE
       def codequality_report
         render_show
       end
+
+      private
+
+      # This overrides the default implementation
+      # because this controller chose to respond with a 302 instead of a 404
+      def authorize_read_licenses!
+        access_to_licenses_denied! unless can?(current_user, :read_licenses, project)
+      end
+
+      def access_to_licenses_denied!
+        respond_to do |format|
+          format.html { redirect_to pipeline_path(pipeline) }
+          format.json { head :not_found }
+        end
+      end
     end
   end
 end

ee/app/models/ee/project.rb

@@ -711,8 +711,8 @@ module EE
       ::Feature.enabled?(:project_compliance_merge_request_approval_settings, self)
     end
 
-    def license_compliance
-      strong_memoize(:license_compliance) { SCA::LicenseCompliance.new(self) }
+    def license_compliance(pipeline = latest_pipeline_with_reports(::Ci::JobArtifact.license_scanning_reports))
+      SCA::LicenseCompliance.new(self, pipeline)
     end
 
     override :template_source?

ee/app/models/sca/license_compliance.rb

@@ -9,8 +9,9 @@ module SCA
       desc: -> (items) { items.reverse }
     }.with_indifferent_access
 
-    def initialize(project)
+    def initialize(project, pipeline)
       @project = project
+      @pipeline = pipeline
     end
 
     def policies
@@ -42,7 +43,7 @@ module SCA
 
     private
 
-    attr_reader :project
+    attr_reader :project, :pipeline
 
     def known_policies
       strong_memoize(:known_policies) do
@@ -60,12 +61,6 @@ module SCA
       end.compact.to_h
     end
 
-    def pipeline
-      strong_memoize(:pipeline) do
-        project.latest_pipeline_with_reports(::Ci::JobArtifact.license_scanning_reports)
-      end
-    end
-
     def license_scan_report
       return empty_report if pipeline.blank?
 

ee/app/models/sca/license_policy.rb

@@ -8,9 +8,11 @@ module SCA
       name: ->(policy) { policy.name }
     }.with_indifferent_access
 
-    attr_reader :id, :name, :url, :dependencies, :spdx_identifier, :classification
+    attr_reader :id, :name, :url, :dependencies, :spdx_identifier, :classification,
+                :approval_status
 
     def initialize(reported_license, software_policy)
+      @approval_status = software_policy&.approval_status || 'unclassified'
       @id = software_policy&.id
       @name = software_policy&.name || reported_license&.name
       @url = reported_license&.url

ee/app/serializers/license_scanning_reports_serializer.rb

 # frozen_string_literal: true
 
 class LicenseScanningReportsSerializer < BaseSerializer
-  entity LicenseScanningReportLicenseEntity
+  entity ::Security::LicensePolicyEntity
 end

ee/app/serializers/security/license_policy_entity.rb

+# frozen_string_literal: true
+
+module Security
+  class LicensePolicyEntity < Grape::Entity
+    expose :name
+    expose :dependencies, using: LicenseScanningReportDependencyEntity
+    expose :url
+
+    expose :classification do |entity|
+      {
+        id: entity.id,
+        name: entity.name,
+        approval_status: entity.approval_status
+      }
+    end
+
+    expose :count do |entity|
+      entity.dependencies.count
+    end
+  end
+end

ee/app/services/projects/licenses/create_policy_service.rb

@@ -5,17 +5,13 @@ module Projects
     class CreatePolicyService < ::BaseService
       def execute
         policy = create_policy(find_software_license, params[:classification])
-        success(software_license_policy: license_compliance.report_for(policy))
+        success(software_license_policy: project.license_compliance.report_for(policy))
       rescue ActiveRecord::RecordInvalid => exception
         error(exception.record.errors, :unprocessable_entity)
       end
 
       private
 
-      def license_compliance
-        @license_compliance ||= ::SCA::LicenseCompliance.new(project)
-      end
-
       def create_policy(software_license, classification)
         raise error_for(:classification, :invalid) unless known?(classification)
 

ee/changelogs/unreleased/208723-pipelines-licenses.yml

+---
+title: Remove SELECT N+1 on software license policies
+merge_request: 34866
+author:
+type: fixed

ee/spec/controllers/projects/pipelines_controller_spec.rb

@@ -101,7 +101,7 @@ RSpec.describe Projects::PipelinesController do
 
         it 'will return license scanning report in json format' do
           expect(payload.size).to eq(pipeline.license_scanning_report.licenses.size)
-          expect(payload.first.keys).to eq(%w(name classification dependencies count url))
+          expect(payload.first.keys).to match_array(%w(name classification dependencies count url))
         end
 
         it 'will return mit license approved status' do
@@ -115,6 +115,34 @@ RSpec.describe Projects::PipelinesController do
           expect(payload.first['name']).to eq('Apache 2.0')
           expect(payload.last['name']).to eq('unknown')
         end
+
+        it 'returns a JSON representation of the license data' do
+          expect(payload).to be_present
+
+          payload.each do |item|
+            expect(item['name']).to be_present
+            expect(item['classification']).to have_key('id')
+            expect(item.dig('classification', 'approval_status')).to be_present
+            expect(item.dig('classification', 'name')).to be_present
+            expect(item).to have_key('dependencies')
+            item['dependencies'].each do |dependency|
+              expect(dependency['name']).to be_present
+            end
+            expect(item['count']).to be_present
+            expect(item).to have_key('url')
+          end
+        end
+
+        context "when not authorized" do
+          before do
+            allow(controller).to receive(:can?).and_call_original
+            allow(controller).to receive(:can?).with(user, :read_licenses, project).and_return(false)
+
+            licenses_with_json
+          end
+
+          specify { expect(response).to have_gitlab_http_status(:not_found) }
+        end
       end
 
       context 'with feature disabled' do

ee/spec/models/sca/license_compliance_spec.rb

@@ -3,7 +3,7 @@
 require "spec_helper"
 
 RSpec.describe SCA::LicenseCompliance do
-  subject { described_class.new(project) }
+  subject { project.license_compliance }
 
   let(:project) { create(:project, :repository, :private) }
   let(:mit) { create(:software_license, :mit) }

ee/spec/requests/projects/pipelines_controller_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Projects::PipelinesController, type: :request do
+  let_it_be(:project) { create(:project, :repository, :private) }
+  let_it_be(:user) { create(:user) }
+
+  before do
+    login_as(user)
+  end
+
+  describe "GET #licenses" do
+    subject { get licenses_project_pipeline_path(project, pipeline) }
+
+    context 'when the project has software license policies' do
+      let_it_be(:pipeline) { create(:ci_pipeline, project: project, builds: [create(:ee_ci_build, :license_scan_v2_1, :success)]) }
+
+      before do
+        stub_licensed_features(license_scanning: true)
+        subject # Warm the cache
+      end
+
+      it 'does not cause extra queries' do
+        control_count = ActiveRecord::QueryRecorder.new { subject }
+
+        create_list(:software_license_policy, 5, project: project)
+
+        expect { subject }.not_to exceed_query_limit(control_count)
+      end
+    end
+  end
+end

ee/spec/serializers/licenses_list_entity_spec.rb

@@ -4,7 +4,7 @@ require 'spec_helper'
 
 RSpec.describe LicensesListEntity do
   let!(:pipeline) { create(:ee_ci_pipeline, :with_license_scanning_report, project: project) }
-  let(:license_compliance) { ::SCA::LicenseCompliance.new(project) }
+  let(:license_compliance) { project.license_compliance }
 
   before do
     stub_licensed_features(license_scanning: true)

ee/spec/serializers/licenses_list_serializer_spec.rb

@@ -12,7 +12,7 @@ RSpec.describe LicensesListSerializer do
 
     let(:project) { create(:project, :repository) }
     let!(:pipeline) { create(:ee_ci_pipeline, :with_license_scanning_report, project: project) }
-    let(:license_compliance) { ::SCA::LicenseCompliance.new(project) }
+    let(:license_compliance) { project.license_compliance }
     let(:user) { create(:user) }
     let(:ci_build) { create(:ee_ci_build, :success) }
 

ee/spec/serializers/security/license_policy_entity_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Security::LicensePolicyEntity do
+  let(:license) { build(:license_scanning_license, :mit).tap { |x| x.add_dependency('rails') } }
+  let(:policy) { build(:software_license_policy, :allowed) }
+  let(:entity) { described_class.new(SCA::LicensePolicy.new(license, policy)) }
+
+  describe '#as_json' do
+    subject { entity.as_json }
+
+    specify { expect(subject[:name]).to eql(policy.name) }
+    specify { expect(subject[:classification]).to eql({ id: policy.id, name: policy.name, approval_status: policy.approval_status }) }
+    specify { expect(subject[:dependencies]).to match_array([{ name: 'rails' }]) }
+    specify { expect(subject[:count]).to be(1) }
+    specify { expect(subject[:url]).to eql(license.url) }
+  end
+end