Fix Rule Evaluation Logic for Handling Multiple Rules of the Same Typ…

…e - Part 2 (Data Backfilling) (#2206)

* entity_profiles.contextual_rules are modified to have rule name key with rule_type or 'rule_type-rand_uuid' in case of collisions

* Existing rule_evaluations.rule_name columns are filled by referring to entity_profiles.contextual_rules[*].name key

* Handlers and Sql queries are updated to reflect not null constraint on rule_evaluations.rule_name column

* migration_profile_backfill_log table is created to support migrating down

Signed-off-by: Vyom-Yadav <[email protected]>

cmd/cli/app/profile/table_render.go

@@ -156,17 +156,9 @@ func RenderRuleEvaluationStatusTable(
 	t table.Table,
 ) {
 	for _, eval := range statuses {
-		ruleName := eval.RuleDescriptionName
-
-		// TODO: Remove this after migration, ruleName would be valid after updating existing evaluations (#1609)
-		//nolint:staticcheck // ignore SA1019: Deprecated field supported for backward compatibility
-		if ruleName == "" {
-			ruleName = eval.RuleName
-		}
-
 		t.AddRowWithColor(
 			layouts.NoColor(eval.RuleId),
-			layouts.NoColor(ruleName),
+			layouts.NoColor(eval.RuleDescriptionName),
 			layouts.NoColor(eval.Entity),
 			getColoredEvalStatus(eval.Status),
 			getRemediateStatusColor(eval.RemediationStatus),

database/migrations/000017_rule_evaluations_rule_name_backfilling.down.sql

@@ -0,0 +1,80 @@
+-- Copyright 2024 Stacklok, Inc
+--
+-- Licensed under the Apache License, Version 2.0 (the "License");
+-- you may not use this file except in compliance with the License.
+-- You may obtain a copy of the License at
+--
+--      http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+
+-- Begin transaction
+BEGIN;
+
+-- Function to remove the "name" field from a JSONB array. Function is immutable as it will return same results
+-- for same input arguments forever.
+CREATE OR REPLACE FUNCTION remove_name_from_jsonb_array(input_jsonb jsonb) RETURNS jsonb AS
+$$
+DECLARE
+  updated_array jsonb;
+  element       jsonb;
+BEGIN
+  updated_array := '[]'::jsonb;
+
+  FOR element IN SELECT * FROM jsonb_array_elements(input_jsonb)
+    LOOP
+      element := element - 'name';
+      updated_array := updated_array || jsonb_build_array(element);
+    END LOOP;
+
+  RETURN updated_array;
+END;
+$$ LANGUAGE plpgsql IMMUTABLE;
+
+-- Prevent concurrent updates to entity_profiles
+SELECT *
+FROM entity_profiles FOR UPDATE;
+
+-- Update the entity_profiles table to remove the "name" key from the "contextual_rules" JSONB array
+UPDATE entity_profiles
+SET contextual_rules = remove_name_from_jsonb_array(contextual_rules),
+    updated_at       = now()
+WHERE entity_profiles.profile_id IN (SELECT profile_id FROM migration_profile_backfill_log);
+
+-- Prevent concurrent updates to rule_evaluations
+SELECT *
+FROM rule_evaluations FOR UPDATE;
+
+-- Update the rule_name column to remove not null constraint
+ALTER TABLE rule_evaluations
+  ALTER COLUMN rule_name DROP NOT NULL;
+
+-- Delete duplicate rule evaluation results without considering rule_name
+-- Using CTID as postgres doesn't have min, max aggregators for uuid (too much code to add one)
+DELETE
+FROM rule_evaluations
+WHERE CTID IN (SELECT MIN(CTID) AS CTID
+               FROM rule_evaluations
+               GROUP BY entity, profile_id, repository_id, rule_type_id,
+                        COALESCE(pull_request_id, '00000000-0000-0000-0000-000000000000'::UUID),
+                        COALESCE(artifact_id, '00000000-0000-0000-0000-000000000000'::UUID)
+               HAVING COUNT(*) > 1)
+  AND profile_id IN (SELECT profile_id FROM migration_profile_backfill_log);
+
+-- Set rule_name column to null
+UPDATE rule_evaluations
+SET rule_name = null
+WHERE profile_id IN (SELECT profile_id FROM migration_profile_backfill_log);
+
+-- Drop the created function
+DROP FUNCTION IF EXISTS remove_name_from_jsonb_array(input_jsonb jsonb);
+
+-- Drop the migration log table
+DROP TABLE IF EXISTS migration_profile_backfill_log;
+
+-- transaction commit
+COMMIT;

database/migrations/000017_rule_evaluations_rule_name_backfilling.up.sql

@@ -0,0 +1,129 @@
+-- Copyright 2024 Stacklok, Inc
+--
+-- Licensed under the Apache License, Version 2.0 (the "License");
+-- you may not use this file except in compliance with the License.
+-- You may obtain a copy of the License at
+--
+--      http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+
+-- Begin transaction
+BEGIN;
+
+-- Function to check whether jsonb array element contain a name key
+CREATE OR REPLACE FUNCTION contains_name_key(input_jsonb jsonb) RETURNS boolean AS
+$$
+DECLARE
+  element jsonb;
+BEGIN
+  FOR element IN SELECT * FROM jsonb_array_elements(input_jsonb)
+    LOOP
+      IF element ? 'name' THEN
+        RETURN true;
+      END IF;
+    END LOOP;
+
+  RETURN false;
+END;
+$$ LANGUAGE plpgsql IMMUTABLE;
+
+-- Function to add a descriptive name to each element in a JSONB array
+CREATE OR REPLACE FUNCTION add_descriptive_name_to_jsonb_array(input_jsonb jsonb) RETURNS jsonb AS
+$$
+DECLARE
+  updated_array jsonb;
+  element       jsonb;
+  element_type  text;
+BEGIN
+  updated_array := '[]'::jsonb;
+
+  FOR element IN SELECT * FROM jsonb_array_elements(input_jsonb)
+    LOOP
+      element_type := element ->> 'type';
+
+      IF (SELECT COUNT(*) FROM jsonb_array_elements(input_jsonb) WHERE value ->> 'type' = element_type) > 1 THEN
+        element := jsonb_set(element, '{name}', ('"' || element_type || '_' || gen_random_uuid() || '"')::jsonb);
+      ELSE
+        element := jsonb_set(element, '{name}', ('"' || element_type || '"')::jsonb);
+      END IF;
+
+      updated_array := updated_array || jsonb_build_array(element);
+    END LOOP;
+
+  RETURN updated_array;
+END;
+$$ LANGUAGE plpgsql;
+
+-- Function to get the rule name for a given entity, profile and rule id.
+CREATE OR REPLACE FUNCTION get_rule_name(entity entities, profile_id uuid, rule_type_id uuid) RETURNS text AS
+$$
+DECLARE
+  rule_name      text;
+  rule_type_name text;
+  rules          jsonb;
+BEGIN
+  SELECT entity_profiles.contextual_rules
+  INTO rules
+  FROM entity_profiles
+  WHERE entity_profiles.profile_id = get_rule_name.profile_id
+    AND entity_profiles.entity = get_rule_name.entity;
+
+  SELECT rule_type.name INTO rule_type_name FROM rule_type WHERE id = get_rule_name.rule_type_id;
+
+  SELECT rule_element ->> 'name'
+  INTO rule_name
+  FROM jsonb_array_elements(rules) rule_element
+  WHERE rule_element ->> 'type' = rule_type_name;
+
+  RETURN rule_name;
+END;
+$$ LANGUAGE plpgsql STABLE;
+
+CREATE TABLE migration_profile_backfill_log (
+  profile_id UUID PRIMARY KEY,
+  FOREIGN KEY (profile_id) REFERENCES profiles (id) ON DELETE CASCADE
+);
+
+-- Prevent entity_profiles to be updated outside the transaction
+SELECT *
+FROM entity_profiles FOR UPDATE;
+
+-- Add updated profile_id to migration_profile_backfill_log, don't add if already exists
+WITH updated_profile_ids AS (
+  -- Update existing rules to have the name field, internally this field is mandatory
+  UPDATE entity_profiles
+    SET contextual_rules = add_descriptive_name_to_jsonb_array(entity_profiles.contextual_rules),
+      updated_at = now()
+    WHERE contains_name_key(entity_profiles.contextual_rules) = false
+    RETURNING entity_profiles.profile_id)
+INSERT
+INTO migration_profile_backfill_log (profile_id)
+SELECT DISTINCT profile_id
+FROM updated_profile_ids
+WHERE profile_id NOT IN (SELECT profile_id FROM migration_profile_backfill_log);
+
+-- Prevent rule_evaluations to be updated outside the transaction
+SELECT *
+FROM rule_evaluations FOR UPDATE;
+
+-- Update rule evaluations
+UPDATE rule_evaluations
+SET rule_name = get_rule_name(rule_evaluations.entity, rule_evaluations.profile_id, rule_evaluations.rule_type_id)
+WHERE rule_name IS NULL;
+
+-- Add non null constraint on rule_name
+ALTER TABLE rule_evaluations
+  ALTER COLUMN rule_name SET NOT NULL;
+
+-- Drop the created functions
+DROP FUNCTION IF EXISTS contains_name_key(input_jsonb jsonb);
+DROP FUNCTION IF EXISTS add_descriptive_name_to_jsonb_array(input_jsonb jsonb);
+DROP FUNCTION IF EXISTS get_rule_name(entity entities, profile_id uuid, rule_type_id uuid);
+
+-- transaction commit
+COMMIT;

database/query/profile_status.sql

@@ -2,7 +2,7 @@
 -- name: UpsertRuleEvaluations :one
 INSERT INTO rule_evaluations (
     profile_id, repository_id, artifact_id, pull_request_id, rule_type_id, entity, rule_name
-) VALUES ($1, $2, $3, $4, $5, $6, sqlc.narg(rule_name))
+) VALUES ($1, $2, $3, $4, $5, $6, $7)
 ON CONFLICT (profile_id, repository_id, COALESCE(artifact_id, '00000000-0000-0000-0000-000000000000'::UUID), COALESCE(pull_request_id, '00000000-0000-0000-0000-000000000000'::UUID), entity, rule_type_id, rule_name)
   DO UPDATE SET profile_id = $1
 RETURNING id;
@@ -79,8 +79,7 @@ WHERE p.project_id = $1;
 DELETE FROM rule_evaluations
 WHERE id IN (
     SELECT id FROM rule_evaluations as re
-    WHERE re.profile_id = $1 AND re.rule_type_id = $2 AND
-          (re.rule_name = sqlc.narg(rule_name) OR sqlc.narg(rule_name) IS NULL) FOR UPDATE);
+    WHERE re.profile_id = $1 AND re.rule_type_id = $2 AND re.rule_name = $3 FOR UPDATE);
 
 -- name: ListRuleEvaluationsByProfileId :many
 WITH
@@ -123,7 +122,7 @@ SELECT
     ad.alert_last_updated,
     res.repository_id,
     res.entity,
-    COALESCE(res.rule_name, ''),
+    res.rule_name,
     repo.repo_name,
     repo.repo_owner,
     repo.provider,

internal/controlplane/handlers_profile.go

@@ -662,7 +662,7 @@ func (s *Server) UpdateProfile(ctx context.Context,
 		return nil, status.Errorf(codes.Internal, "failed to get profile: %s", err)
 	}
 
-	oldRules, err := s.getRulesFromOldProfile(ctx, oldProfile, entityCtx)
+	oldRules, err := s.getRulesFromProfile(ctx, oldProfile, entityCtx)
 	if err != nil {
 		if errors.Is(err, sql.ErrNoRows) {
 			return nil, util.UserVisibleError(codes.NotFound, "profile not found")
@@ -813,7 +813,7 @@ func (s *Server) getAndValidateRulesFromProfile(
 	return rulesInProf, nil
 }
 
-func (s *Server) getRulesFromOldProfile(
+func (s *Server) getRulesFromProfile(
 	ctx context.Context,
 	prof *minderv1.Profile,
 	entityCtx engine.EntityContext,
@@ -839,11 +839,9 @@ func (s *Server) getRulesFromOldProfile(
 			return fmt.Errorf("cannot convert rule type %s to pb: %w", rtdb.Name, err)
 		}
 
-		// TODO: Remove r.Name and replace with computeRuleName(..) after migration is complete for #1609
-		// Existing rules (before migration) would have r.Name == ""
 		key := ruleTypeAndNamePair{
 			RuleType: r.GetType(),
-			RuleName: r.Name,
+			RuleName: computeRuleName(r),
 		}
 
 		rulesInProf[key] = entityAndRuleTuple{
@@ -1057,16 +1055,11 @@ func deleteRuleStatusesForProfile(
 ) error {
 	for ruleTypeAndName, rule := range unusedRuleStatuses {
 		log.Printf("deleting rule evaluations for rule %s in profile %s", rule.RuleID, profile.ID)
-		// TODO: Remove this after migration, ruleName would be valid after updating existing evaluations (#1609)
-		ruleName := sql.NullString{
-			String: ruleTypeAndName.RuleName,
-			Valid:  ruleTypeAndName.RuleName != "",
-		}
 
 		if err := querier.DeleteRuleStatusesForProfileAndRuleType(ctx, db.DeleteRuleStatusesForProfileAndRuleTypeParams{
 			ProfileID:  profile.ID,
 			RuleTypeID: rule.RuleID,
-			RuleName:   ruleName,
+			RuleName:   ruleTypeAndName.RuleName,
 		}); err != nil {
 			log.Printf("error deleting rule evaluations: %v", err)
 			return fmt.Errorf("error deleting rule evaluations: %w", err)

internal/engine/actions/alert/security_advisory/security_advisory.go

@@ -81,7 +81,7 @@ To address this security exposure, we recommend taking the following actions:
 
 - Profile: {{.Profile}}
 - Rule: {{.Rule}}
-{{if and (not (eq .Name "")) (ne .Name .Rule) -}}
+{{if (ne .Name .Rule) -}}
 - Name: {{.Name}}
 {{end -}}
 - Repository: {{.Repository}}