From c9d608a46dd9e47ee25a2dc2052efc6345a9288d Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Wed, 19 Jun 2019 21:24:23 -0700 Subject: [PATCH 1/2] Remove support for checking legacy security reports The checking of the existence of the legacy security reports was using a significant number of SQL queries in the merge request widget. This commit removes support for them so that they will no longer be accessible from the widget, but the data remains intact. Part of https://gitlab.com/gitlab-org/gitlab-ce/issues/63228 --- ee/app/models/ee/ci/pipeline.rb | 20 +------ ee/app/presenters/ee/ci/pipeline_presenter.rb | 10 +--- ...legacy-security-reports-from-mr-widget.yml | 5 ++ .../projects/pipelines_controller_spec.rb | 2 +- ee/spec/models/ci/pipeline_spec.rb | 59 ------------------- .../merge_request_widget_entity_spec.rb | 27 +-------- 6 files changed, 9 insertions(+), 114 deletions(-) create mode 100644 ee/changelogs/unreleased/sh-remove-legacy-security-reports-from-mr-widget.yml diff --git a/ee/app/models/ee/ci/pipeline.rb b/ee/app/models/ee/ci/pipeline.rb index ad0fe9872daaa2..c5965817dede33 100644 --- a/ee/app/models/ee/ci/pipeline.rb +++ b/ee/app/models/ee/ci/pipeline.rb @@ -130,7 +130,7 @@ def batch_lookup_report_artifact_for_file_type(file_type) end def any_report_artifact_for_type(file_type) - report_artifact_for_file_type(file_type) || legacy_report_artifact_for_file_type(file_type) + report_artifact_for_file_type(file_type) end def report_artifact_for_file_type(file_type) @@ -139,24 +139,6 @@ def report_artifact_for_file_type(file_type) job_artifacts.where(file_type: ::Ci::JobArtifact.file_types[file_type]).last end - def legacy_report_artifact_for_file_type(file_type) - return unless available_licensed_report_type?(file_type) - - legacy_names = LEGACY_REPORT_FORMATS[file_type] - return unless legacy_names - - builds.success.latest.where(name: legacy_names[:names]).each do |build| - legacy_names[:files].each do |file_name| - next unless build.has_artifact?(file_name) - - return OpenStruct.new(build: build, path: file_name) - end - end - - # In case there is no artifact return nil - nil - end - def expose_license_management_data? any_report_artifact_for_type(:license_management) end diff --git a/ee/app/presenters/ee/ci/pipeline_presenter.rb b/ee/app/presenters/ee/ci/pipeline_presenter.rb index 65200801cc223d..ade2b8a6724e4a 100644 --- a/ee/app/presenters/ee/ci/pipeline_presenter.rb +++ b/ee/app/presenters/ee/ci/pipeline_presenter.rb @@ -27,20 +27,12 @@ def expose_security_dashboard? def downloadable_path_for_report_type(file_type) if (job_artifact = batch_lookup_report_artifact_for_file_type(file_type)) && can?(current_user, :read_build, job_artifact.job) - return download_project_job_artifacts_path( + download_project_job_artifacts_path( job_artifact.project, job_artifact.job, file_type: file_type, proxy: true) end - - if (build_artifact = legacy_report_artifact_for_file_type(file_type)) && - can?(current_user, :read_build, build_artifact.build) - return raw_project_job_artifacts_path( - build_artifact.build.project, - build_artifact.build, - path: build_artifact.path) - end end end end diff --git a/ee/changelogs/unreleased/sh-remove-legacy-security-reports-from-mr-widget.yml b/ee/changelogs/unreleased/sh-remove-legacy-security-reports-from-mr-widget.yml new file mode 100644 index 00000000000000..fc44bcd24ceccc --- /dev/null +++ b/ee/changelogs/unreleased/sh-remove-legacy-security-reports-from-mr-widget.yml @@ -0,0 +1,5 @@ +--- +title: Remove support for checking legacy security reports +merge_request: 14291 +author: +type: performance diff --git a/ee/spec/controllers/projects/pipelines_controller_spec.rb b/ee/spec/controllers/projects/pipelines_controller_spec.rb index 7778f5f6faad0d..c411be534e07ee 100644 --- a/ee/spec/controllers/projects/pipelines_controller_spec.rb +++ b/ee/spec/controllers/projects/pipelines_controller_spec.rb @@ -200,7 +200,7 @@ def get_pipeline_json(pipeline) describe 'GET security' do context 'with a sast artifact' do before do - create(:ee_ci_build, :legacy_sast, pipeline: pipeline) + create(:ee_ci_build, :sast, pipeline: pipeline) end context 'with feature enabled' do diff --git a/ee/spec/models/ci/pipeline_spec.rb b/ee/spec/models/ci/pipeline_spec.rb index d2877ee22562ce..193315424e9456 100644 --- a/ee/spec/models/ci/pipeline_spec.rb +++ b/ee/spec/models/ci/pipeline_spec.rb @@ -26,26 +26,6 @@ end end - describe '#with_legacy_security_reports scope' do - let(:pipeline_1) { create(:ci_pipeline_without_jobs, project: project) } - let(:pipeline_2) { create(:ci_pipeline_without_jobs, project: project) } - let(:pipeline_3) { create(:ci_pipeline_without_jobs, project: project) } - let(:pipeline_4) { create(:ci_pipeline_without_jobs, project: project) } - let(:pipeline_5) { create(:ci_pipeline_without_jobs, project: project) } - - before do - create(:ee_ci_build, :legacy_sast, pipeline: pipeline_1) - create(:ee_ci_build, :legacy_dependency_scanning, pipeline: pipeline_2) - create(:ee_ci_build, :legacy_container_scanning, pipeline: pipeline_3) - create(:ee_ci_build, :legacy_dast, pipeline: pipeline_4) - create(:ee_ci_build, :success, :artifacts, name: 'foobar', pipeline: pipeline_5) - end - - it "returns pipeline with security reports" do - expect(described_class.with_legacy_security_reports).to contain_exactly(pipeline_1, pipeline_2, pipeline_3, pipeline_4) - end - end - describe '#with_vulnerabilities scope' do let!(:pipeline_1) { create(:ci_pipeline_without_jobs, project: project) } let!(:pipeline_2) { create(:ci_pipeline_without_jobs, project: project) } @@ -146,45 +126,6 @@ end end - describe '#legacy_report_artifact_for_file_type' do - let(:build_name) { ::EE::Ci::Pipeline::LEGACY_REPORT_FORMATS[file_type][:names].first } - let(:artifact_path) { ::EE::Ci::Pipeline::LEGACY_REPORT_FORMATS[file_type][:files].first } - - let!(:build) do - create( - :ci_build, - :success, - :artifacts, - name: build_name, - pipeline: pipeline, - options: { - artifacts: { - paths: [artifact_path] - } - } - ) - end - - subject { pipeline.legacy_report_artifact_for_file_type(file_type) } - - described_class::LEGACY_REPORT_FORMATS.each do |file_type, _| - licensed_features = described_class::REPORT_LICENSED_FEATURES[file_type] - - context "for file_type: #{file_type}" do - let(:file_type) { file_type } - let(:expected) { OpenStruct.new(build: build, path: artifact_path) } - - if licensed_features.nil? - it_behaves_like 'unlicensed report type' - elsif licensed_features.size == 1 - it_behaves_like 'licensed report type', licensed_features.first - else - it_behaves_like 'multi-licensed report type', licensed_features - end - end - end - end - describe '#security_reports' do subject { pipeline.security_reports } diff --git a/ee/spec/serializers/merge_request_widget_entity_spec.rb b/ee/spec/serializers/merge_request_widget_entity_spec.rb index efe4d5b6fc0023..360244a4791efe 100644 --- a/ee/spec/serializers/merge_request_widget_entity_spec.rb +++ b/ee/spec/serializers/merge_request_widget_entity_spec.rb @@ -97,16 +97,6 @@ def create_all_artifacts end end - context "with legacy report artifacts" do - before do - create(:ee_ci_build, :"legacy_#{artifact_type}", pipeline: pipeline) - end - - it "has data entry" do - expect(subject.as_json).to include(json_entry) - end - end - context "without artifacts" do it "does not have data entry" do expect(subject.as_json).not_to include(json_entry) @@ -167,26 +157,11 @@ def create_all_artifacts end end - context 'when legacy artifact is defined' do - before do - create(:ee_ci_build, :legacy_license_management, pipeline: pipeline) - end - - it 'is included, if license manage management features are on' do - expect(subject.as_json).to include(:license_management) - expect(subject.as_json[:license_management]).to include(:head_path) - expect(subject.as_json[:license_management]).to include(:base_path) - expect(subject.as_json[:license_management]).to include(:managed_licenses_path) - expect(subject.as_json[:license_management]).to include(:can_manage_licenses) - expect(subject.as_json[:license_management]).to include(:license_management_full_report_path) - end - end - describe '#managed_licenses_path' do let(:managed_licenses_path) { expose_path(api_v4_projects_managed_licenses_path(id: project.id)) } before do - create(:ee_ci_build, :legacy_license_management, pipeline: pipeline) + create(:ee_ci_build, :license_management, pipeline: pipeline) end it 'is a path for target project' do -- GitLab From f19de7dac0f0b11dcd2c1e2d604aa0084c0b6320 Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Fri, 21 Jun 2019 06:04:02 -0700 Subject: [PATCH 2/2] Fix failing specs in pipeline_spec.rb --- ee/spec/features/projects/pipelines/pipeline_spec.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/ee/spec/features/projects/pipelines/pipeline_spec.rb b/ee/spec/features/projects/pipelines/pipeline_spec.rb index aaca39182dfe00..cfb536d846d8ae 100644 --- a/ee/spec/features/projects/pipelines/pipeline_spec.rb +++ b/ee/spec/features/projects/pipelines/pipeline_spec.rb @@ -98,7 +98,7 @@ context 'with a sast artifact' do before do - create(:ee_ci_build, :legacy_sast, pipeline: pipeline) + create(:ee_ci_build, :sast, pipeline: pipeline) visit security_project_pipeline_path(project, pipeline) end @@ -135,7 +135,7 @@ context 'with a license management artifact' do before do - create(:ee_ci_build, :legacy_license_management, pipeline: pipeline) + create(:ee_ci_build, :license_management, pipeline: pipeline) visit licenses_project_pipeline_path(project, pipeline) end @@ -143,7 +143,7 @@ it 'shows jobs tab pane as active' do expect(page).to have_content('Licenses') expect(page).to have_css('#js-tab-licenses') - expect(find('.js-licenses-counter')).to have_content('0') + expect(find('.js-licenses-counter')).to have_content('4') end it 'shows security report section' do -- GitLab