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

Remove N+1 in license scanning report comparison

See merge request gitlab-org/gitlab!42895

ee/app/models/ee/ci/build.rb

@@ -76,11 +76,11 @@ module EE
       end
 
       def collect_license_scanning_reports!(license_scanning_report)
+        return license_scanning_report unless project.feature_available?(:license_scanning)
+
         each_report(::Ci::JobArtifact::LICENSE_SCANNING_REPORT_FILE_TYPES) do |file_type, blob|
           next if ::Feature.disabled?(:parse_license_management_reports, default_enabled: true)
 
-          next unless project.feature_available?(:license_scanning)
-
           ::Gitlab::Ci::Parsers.fabricate!(file_type).parse!(blob, license_scanning_report)
         end
 

ee/app/models/sca/license_compliance.rb

@@ -41,11 +41,29 @@ module SCA
       build_policy(license_scan_report[policy.software_license.canonical_id], policy)
     end
 
+    def diff_with(other)
+      license_scan_report
+        .diff_with(other.license_scan_report)
+        .transform_values do |reported_licenses|
+          reported_licenses.map do |reported_license|
+            build_policy(reported_license, known_policies[reported_license.canonical_id])
+          end
+        end
+    end
+
+    def license_scan_report
+      strong_memoize(:license_scan_report) do
+        pipeline.blank? ? empty_report : pipeline.license_scanning_report
+      end
+    end
+
     private
 
     attr_reader :project, :pipeline
 
     def known_policies
+      return {} if project.blank?
+
       strong_memoize(:known_policies) do
         project.software_license_policies.including_license.unreachable_limit.map do |policy|
           [policy.software_license.canonical_id, report_for(policy)]
@@ -61,23 +79,6 @@ module SCA
       end.compact.to_h
     end
 
-    def license_scan_report
-      return empty_report if pipeline.blank?
-
-      strong_memoize(:license_scan_report) do
-        pipeline.license_scanning_report.tap do |report|
-          report.apply_details_from!(dependency_list_report)
-        end
-      rescue ::Gitlab::Ci::Parsers::LicenseCompliance::LicenseScanning::LicenseScanningParserError
-        empty_report
-      end
-    end
-
-    def dependency_list_report
-      pipeline.dependency_list_report
-    rescue ::Gitlab::Ci::Parsers::LicenseCompliance::LicenseScanning::LicenseScanningParserError
-    end
-
     def empty_report
       ::Gitlab::Ci::Reports::LicenseScanning::Report.new
     end

ee/app/serializers/license_scanning_report_license_entity.rb

-# frozen_string_literal: true
-
-class LicenseScanningReportLicenseEntity < Grape::Entity
-  include RequestAwareEntity
-
-  expose :name
-  expose :classification
-  expose :dependencies, using: LicenseScanningReportDependencyEntity
-  expose :count
-  expose :url
-
-  def classification
-    default = { id: nil, name: value_for(:name), approval_status: 'unclassified' }
-    found = SoftwareLicensePoliciesFinder.new(request&.current_user, request&.project, name: value_for(:name)).find
-    ManagedLicenseEntity.represent(found || default)
-  end
-end

ee/app/serializers/license_scanning_reports_comparer_entity.rb

 # frozen_string_literal: true
 
 class LicenseScanningReportsComparerEntity < Grape::Entity
-  expose :new_licenses, using: LicenseScanningReportLicenseEntity
-  expose :existing_licenses, using: LicenseScanningReportLicenseEntity
-  expose :removed_licenses, using: LicenseScanningReportLicenseEntity
+  expose :new_licenses, using: ::Security::LicensePolicyEntity
+  expose :existing_licenses, using: ::Security::LicensePolicyEntity
+  expose :removed_licenses, using: ::Security::LicensePolicyEntity
 end

ee/app/services/ci/compare_license_scanning_reports_service.rb

@@ -11,7 +11,7 @@ module Ci
     end
 
     def get_report(pipeline)
-      pipeline&.license_scanning_report
+      ::SCA::LicenseCompliance.new(pipeline&.project, pipeline)
     end
 
     private

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

+---
+title: Remove N+1 in license scanning report comparison
+merge_request: 42895
+author:
+type: fixed

ee/lib/gitlab/ci/reports/dependency_list/dependency.rb

@@ -58,7 +58,7 @@ module Gitlab
             vulnerabilities.each do |v|
               next if v.empty?
 
-              unique_vulnerabilities.add(Vulnerability.new(v))
+              unique_vulnerabilities.add(::Gitlab::Ci::Reports::DependencyList::Vulnerability.new(v))
             end
 
             unique_vulnerabilities

ee/lib/gitlab/ci/reports/license_scanning/report.rb

@@ -70,7 +70,7 @@ module Gitlab
 
           def diff_with(other_report)
             base = self.licenses
-            head = other_report.licenses
+            head = other_report&.licenses || []
 
             {
               added: (head - base),

ee/lib/gitlab/ci/reports/license_scanning/reports_comparer.rb

@@ -10,7 +10,7 @@ module Gitlab
           attr_reader :base_report, :head_report
 
           def initialize(base_report, head_report)
-            @base_report = base_report || ::Gitlab::Ci::Reports::LicenseScanning::Report.new
+            @base_report = base_report
             @head_report = head_report
           end
 

ee/spec/lib/gitlab/ci/reports/license_scanning/report_spec.rb

@@ -173,6 +173,22 @@ RSpec.describe Gitlab::Ci::Reports::LicenseScanning::Report do
       licenses.map(&:name)
     end
 
+    context 'when the other report is not available' do
+      subject { base_report.diff_with(nil) }
+
+      let(:base_report) { build(:license_scan_report, :version_2) }
+
+      before do
+        base_report
+          .add_license(id: 'MIT', name: 'MIT License')
+          .add_dependency('rails')
+      end
+
+      specify { expect(names_from(subject[:removed])).to contain_exactly('MIT License') }
+      specify { expect(subject[:added]).to be_empty }
+      specify { expect(subject[:unchanged]).to be_empty }
+    end
+
     context 'when diffing two v1 reports' do
       let(:base_report) { build(:license_scan_report, :version_1) }
       let(:head_report) { build(:license_scan_report, :version_1) }

ee/spec/models/sca/license_compliance_spec.rb

@@ -359,4 +359,64 @@ RSpec.describe SCA::LicenseCompliance do
       it { expect(subject.latest_build_for_default_branch).to eq(license_scan_build) }
     end
   end
+
+  describe "#diff_with" do
+    context "when the head pipeline has not run" do
+      subject { project.license_compliance(nil).diff_with(base_compliance) }
+
+      let!(:base_compliance) { project.license_compliance(base_pipeline) }
+      let!(:base_pipeline) { create(:ci_pipeline, :success, project: project, builds: [license_scan_build]) }
+      let(:license_scan_build) { create(:ee_ci_build, :license_scan_v2_1, :success) }
+
+      specify { expect(subject[:added]).to all(be_instance_of(::SCA::LicensePolicy)) }
+      specify { expect(subject[:added].count).to eq(3) }
+      specify { expect(subject[:removed]).to be_empty }
+      specify { expect(subject[:unchanged]).to be_empty }
+    end
+
+    context "when nothing has changed between the head and the base pipeline" do
+      subject { project.license_compliance(head_pipeline).diff_with(base_compliance) }
+
+      let!(:head_compliance) { project.license_compliance(head_pipeline) }
+      let!(:head_pipeline) { create(:ci_pipeline, :success, project: project, builds: [create(:ee_ci_build, :license_scan_v2_1, :success)]) }
+
+      let!(:base_compliance) { project.license_compliance(base_pipeline) }
+      let!(:base_pipeline) { create(:ci_pipeline, :success, project: project, builds: [create(:ee_ci_build, :license_scan_v2_1, :success)]) }
+
+      specify { expect(subject[:added]).to be_empty }
+      specify { expect(subject[:removed]).to be_empty }
+      specify { expect(subject[:unchanged]).to all(be_instance_of(::SCA::LicensePolicy)) }
+      specify { expect(subject[:unchanged].count).to eq(3) }
+    end
+
+    context "when the base pipeline removed some licenses" do
+      subject { project.license_compliance(head_pipeline).diff_with(base_compliance) }
+
+      let!(:head_compliance) { project.license_compliance(head_pipeline) }
+      let!(:head_pipeline) { create(:ci_pipeline, :success, project: project, builds: [create(:ee_ci_build, :license_scan_v2_1, :success)]) }
+
+      let!(:base_compliance) { project.license_compliance(base_pipeline) }
+      let!(:base_pipeline) { create(:ci_pipeline, :success, project: project, builds: [create(:ee_ci_build, :success)]) }
+
+      specify { expect(subject[:added]).to be_empty }
+      specify { expect(subject[:unchanged]).to be_empty }
+      specify { expect(subject[:removed]).to all(be_instance_of(::SCA::LicensePolicy)) }
+      specify { expect(subject[:removed].count).to eq(3) }
+    end
+
+    context "when the base pipeline added some licenses" do
+      subject { project.license_compliance(head_pipeline).diff_with(base_compliance) }
+
+      let!(:head_compliance) { project.license_compliance(head_pipeline) }
+      let!(:head_pipeline) { create(:ci_pipeline, :success, project: project, builds: [create(:ee_ci_build, :success)]) }
+
+      let!(:base_compliance) { project.license_compliance(base_pipeline) }
+      let!(:base_pipeline) { create(:ci_pipeline, :success, project: project, builds: [create(:ee_ci_build, :license_scan_v2_1, :success)]) }
+
+      specify { expect(subject[:added]).to all(be_instance_of(::SCA::LicensePolicy)) }
+      specify { expect(subject[:added].count).to eq(3) }
+      specify { expect(subject[:removed]).to be_empty }
+      specify { expect(subject[:unchanged]).to be_empty }
+    end
+  end
 end

ee/spec/serializers/license_scanning_report_license_entity_spec.rb

-# frozen_string_literal: true
-
-require 'spec_helper'
-
-RSpec.describe LicenseScanningReportLicenseEntity do
-  include LicenseScanningReportHelper
-
-  let(:user) { build(:user) }
-  let(:project) { build(:project, :repository) }
-  let(:license) { create_license }
-  let(:request) { double('request') }
-  let(:entity) { described_class.new(license, request: request) }
-
-  before do
-    allow(request).to receive(:current_user).and_return(user)
-    allow(request).to receive(:project).and_return(project)
-  end
-
-  describe '#as_json' do
-    subject { entity.as_json }
-
-    it 'emits the correct approval_status' do
-      expect(subject[:classification][:approval_status]).to eq('unclassified')
-    end
-
-    it 'contains the correct dependencies' do
-      expect(subject[:dependencies].count).to eq(2)
-      expect(subject[:dependencies][0][:name]).to eq('Dependency1')
-    end
-  end
-end

ee/spec/serializers/license_scanning_reports_comparer_entity_spec.rb

 # frozen_string_literal: true
-
 require 'spec_helper'
 
 RSpec.describe LicenseScanningReportsComparerEntity do
-  include LicenseScanningReportHelper
-
-  let(:user) { build(:user) }
-  let(:project) { build(:project, :repository) }
-  let(:request) { double('request') }
-  let(:comparer) { create_comparer }
-  let(:entity) { described_class.new(comparer, request: request) }
-
-  before do
-    allow(request).to receive(:current_user).and_return(user)
-    allow(request).to receive(:project).and_return(project)
-  end
+  let(:entity) { described_class.new(::Gitlab::Ci::Reports::LicenseScanning::ReportsComparer.new(project.license_compliance(base_pipeline), project.license_compliance(head_pipeline))) }
+  let(:project) { create_default(:project, :repository) }
+  let(:base_pipeline) { create(:ci_pipeline, :success, project: project, builds: [create(:ee_ci_build, :license_scan_v2_1, :success)]) }
+  let(:head_pipeline) { create(:ci_pipeline, :success, project: project, builds: [create(:ee_ci_build, :success)]) }
 
   describe '#as_json' do
     subject { entity.as_json }
@@ -23,10 +14,6 @@ RSpec.describe LicenseScanningReportsComparerEntity do
       expect(subject).to have_key(:new_licenses)
       expect(subject).to have_key(:existing_licenses)
       expect(subject).to have_key(:removed_licenses)
-
-      expect(subject[:new_licenses][0][:name]).to eq('License4')
-      expect(subject[:existing_licenses].count).to be(2)
-      expect(subject[:removed_licenses][0][:name]).to eq('License1')
     end
   end
 end

ee/spec/services/ci/compare_license_scanning_reports_service_spec.rb

@@ -3,6 +3,8 @@
 require 'spec_helper'
 
 RSpec.describe Ci::CompareLicenseScanningReportsService do
+  include ProjectForksHelper
+
   let_it_be(:project) { create(:project, :repository) }
   let(:service) { described_class.new(project, nil) }
 
@@ -13,6 +15,23 @@ RSpec.describe Ci::CompareLicenseScanningReportsService do
   describe '#execute' do
     subject { service.execute(base_pipeline, head_pipeline) }
 
+    context "when loading data for multiple reports" do
+      it 'loads the data efficiently' do
+        base_pipeline = create(:ci_pipeline, :success, project: project)
+        head_pipeline = create(:ci_pipeline, :success, project: project, builds: [create(:ci_build, :success, job_artifacts: [create(:ee_ci_job_artifact, :license_scan)])])
+
+        control_count = ActiveRecord::QueryRecorder.new do
+          service.execute(base_pipeline.reload, head_pipeline.reload)
+        end.count
+
+        new_head_pipeline = create(:ci_pipeline, :success, project: project, builds: [create(:ee_ci_build, :license_scan_v2_1, :success)])
+
+        expect do
+          service.execute(base_pipeline.reload, new_head_pipeline.reload)
+        end.not_to exceed_query_limit(control_count)
+      end
+    end
+
     context 'when head pipeline has license scanning reports' do
       let!(:base_pipeline) { nil }
       let!(:head_pipeline) { create(:ee_ci_pipeline, :with_license_scanning_report, project: project) }
@@ -24,6 +43,26 @@ RSpec.describe Ci::CompareLicenseScanningReportsService do
       end
     end
 
+    context "when head pipeline has not run and base pipeline is for a forked project" do
+      let(:service) { described_class.new(project, maintainer) }
+      let(:maintainer) { create(:user) }
+      let(:contributor) { create(:user) }
+      let(:project) { create(:project, :public, :repository) }
+      let(:base_pipeline) { nil }
+      let(:head_pipeline) { create(:ee_ci_pipeline, :with_license_scanning_report, project: forked_project, user: contributor) }
+      let(:forked_project) { fork_project(project, contributor, namespace: contributor.namespace) }
+
+      before do
+        project.add_maintainer(maintainer)
+        project.add_developer(contributor)
+      end
+
+      it 'reports new licenses' do
+        expect(subject[:status]).to eq(:parsed)
+        expect(subject[:data]['new_licenses'].count).to be > 1
+      end
+    end
+
     context 'when base and head pipelines have test reports' do
       let!(:base_pipeline) { create(:ee_ci_pipeline, :with_license_scanning_report, project: project) }
       let!(:head_pipeline) { create(:ee_ci_pipeline, :with_license_scanning_feature_branch, project: project) }