Skip to content

ProjectsGrade.grades_for returns same grades for all vulnerables

Summary

When it's given more more than one vulnerable, Vulnerabilities::ProjectsGrade.grades_for returns the grades of all given vulnerable for each vulnerable.

See !172720 (comment 2217560966)

This means that the grades are summed when multiple vulnerables are requested. However, the product currently only uses queries with one vulnerable at the time. This means that the bug is imperceptible unless using the API directly and querying grades for multiple objects at once.

Steps to reproduce

The following spec pass when it's added to ee/spec/models/vulnerabilities/projects_grade_spec.rb. project_7 belongs to the other_group, and all other projects belong to group.

    context 'when the given vulnerables are groups' do
      let(:vulnerables) { [group, other_group] }

      it 'returns all letter grades for each vulnerable' do
        vulnerables.each do |vulnerable|
          expected_projects_grades = [
            described_class.new(vulnerable, 'a', [project_1.id, project_7.id]),
            described_class.new(vulnerable, 'b', [project_2.id, project_3.id]),
            described_class.new(vulnerable, 'c', [project_4.id]),
            described_class.new(vulnerable, 'f', [project_5.id])
          ]

          expect(projects_grades[vulnerable].map(&compare_key)).to match_array(expected_projects_grades.map(&compare_key))
        end
      end
    end
diff for spec file
diff --git a/ee/spec/models/vulnerabilities/projects_grade_spec.rb b/ee/spec/models/vulnerabilities/projects_grade_spec.rb
index cbebcc8cdf07..466fb2f810fa 100644
--- a/ee/spec/models/vulnerabilities/projects_grade_spec.rb
+++ b/ee/spec/models/vulnerabilities/projects_grade_spec.rb
@@ -5,12 +5,14 @@
 RSpec.describe Vulnerabilities::ProjectsGrade, feature_category: :vulnerability_management do
   let_it_be(:group) { create(:group) }
   let_it_be(:subgroup) { create(:group, parent: group) }
+  let_it_be(:other_group) { create(:group) }
   let_it_be(:project_1) { create(:project, group: group) }
   let_it_be(:project_2) { create(:project, group: group) }
   let_it_be(:project_3) { create(:project, group: group) }
   let_it_be(:project_4) { create(:project, group: group) }
   let_it_be(:project_5) { create(:project, group: group) }
   let_it_be(:project_6) { create(:project, group: subgroup) }
+  let_it_be(:project_7) { create(:project, group: other_group) }
   let_it_be(:archived_project) { create(:project, :archived, group: group).tap { |p| create(:vulnerability_statistic, :grade_a, project: p) } }
   let_it_be(:unrelated_project) { create(:project).tap { |p| create(:vulnerability_statistic, :grade_a, project: p) } }
 
@@ -20,13 +22,15 @@
   let_it_be(:vulnerability_statistic_4) { create(:vulnerability_statistic, :grade_c, project: project_4) }
   let_it_be(:vulnerability_statistic_5) { create(:vulnerability_statistic, :grade_f, project: project_5) }
   let_it_be(:vulnerability_statistic_6) { create(:vulnerability_statistic, :grade_d, project: project_6) }
+  let_it_be(:vulnerability_statistic_7) { create(:vulnerability_statistic, :grade_a, project: project_7) }
 
   describe '.grades_for' do
     let(:compare_key) { ->(projects_grade) { [projects_grade.grade, projects_grade.project_ids.sort] } }
     let(:include_subgroups) { false }
     let(:filter) { nil }
+    let(:vulnerables) { [vulnerable] }
 
-    subject(:projects_grades) { described_class.grades_for([vulnerable], filter: filter, include_subgroups: include_subgroups) }
+    subject(:projects_grades) { described_class.grades_for(vulnerables, filter: filter, include_subgroups: include_subgroups) }
 
     context 'when the given vulnerable is a Group' do
       let(:vulnerable) { group }
@@ -103,6 +107,27 @@
       end
     end
 
+    context 'when the given vulnerables are groups' do
+      let(:vulnerables) { [group, other_group] }
+
+      # The following expectation captures a buggy behavior that is
+      # covered by https://gitlab.com/gitlab-org/gitlab/-/issues/507992.
+      # It ensures that the behavior remains consistent remains unchanged
+      # as we refactor the code, and until the bug is fixed.
+      it 'returns all letter grades for each vulnerable' do
+        vulnerables.each do |vulnerable|
+          expected_projects_grades = [
+            described_class.new(vulnerable, 'a', [project_1.id, project_7.id]),
+            described_class.new(vulnerable, 'b', [project_2.id, project_3.id]),
+            described_class.new(vulnerable, 'c', [project_4.id]),
+            described_class.new(vulnerable, 'f', [project_5.id])
+          ]
+
+          expect(projects_grades[vulnerable].map(&compare_key)).to match_array(expected_projects_grades.map(&compare_key))
+        end
+      end
+    end
+
     context 'when the given vulnerable is an InstanceSecurityDashboard' do
       let(:user) { create(:user) }
       let(:vulnerable) { InstanceSecurityDashboard.new(user) }

What is the current bug behavior?

.grades_for returns a Hash where each given vulnerable has the grades of all given vulnerable.

What is the expected correct behavior?

.grades_for returns a Hash where each given vulnerable has its own grades.

Relevant logs and/or screenshots

Output of checks

Possible fixes

/cc @bwill @minac

Edited by Brian Williams