From 50350c5316f06b81d6b9e97520de2d2a8f0ce790 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Thu, 9 Nov 2023 14:07:23 +0100 Subject: [PATCH] Add a stored procedure to handle entity deletions In case an entity was deleted (typically a PR but a repo delete is also a valid use-case), we would not update the profile status until the next profile run. For that, we need to add a new database trigger in a migration. Sort-of-related is a bug in the original procedure that we had where if all the current rules were skipped and we added a failure the overall status would still be a failure. I added a new version of the old procedure to cover that plus some tests. Fixes: #1610 --- .../000007_status_delete_procedure.down.sql | 66 +++++++ .../000007_status_delete_procedure.up.sql | 115 ++++++++++++ internal/db/profiles_test.go | 166 +++++++++++++++++- 3 files changed, 345 insertions(+), 2 deletions(-) create mode 100644 database/migrations/000007_status_delete_procedure.down.sql create mode 100644 database/migrations/000007_status_delete_procedure.up.sql diff --git a/database/migrations/000007_status_delete_procedure.down.sql b/database/migrations/000007_status_delete_procedure.down.sql new file mode 100644 index 0000000000..4a45346487 --- /dev/null +++ b/database/migrations/000007_status_delete_procedure.down.sql @@ -0,0 +1,66 @@ +-- Copyright 2023 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; + +DROP TRIGGER IF EXISTS update_profile_status_after_delete ON rule_evaluations; + +DROP FUNCTION IF EXISTS update_profile_status_on_delete(); + +-- This is the same version of the function from the 000001 migration. Just for completeness of down migration +CREATE OR REPLACE FUNCTION update_profile_status() RETURNS TRIGGER AS $$ +DECLARE +v_profile_id UUID; +BEGIN + -- Fetch the profile_id for the current rule_eval_id +SELECT profile_id INTO v_profile_id +FROM rule_evaluations +WHERE id = NEW.rule_eval_id; + +-- keep error if profile had errored +IF (NEW.status = 'error') THEN +UPDATE profile_status SET profile_status = 'error', last_updated = NOW() +WHERE profile_id = v_profile_id; +-- only mark profile run as skipped if every evaluation was skipped +ELSEIF (NEW.status = 'skipped') THEN +UPDATE profile_status SET profile_status = 'skipped', last_updated = NOW() +WHERE profile_id = v_profile_id AND NOT EXISTS (SELECT * FROM rule_evaluations res INNER JOIN rule_details_eval rde ON res.id = rde.rule_eval_id WHERE res.profile_id = v_profile_id AND rde.status != 'skipped'); +-- mark status as successful if all evaluations are successful or skipped +ELSEIF NOT EXISTS ( + SELECT * + FROM rule_evaluations res + INNER JOIN rule_details_eval rde ON res.id = rde.rule_eval_id + WHERE res.profile_id = v_profile_id AND rde.status != 'success' AND rde.status != 'skipped' + ) THEN +UPDATE profile_status SET profile_status = 'success', last_updated = NOW() +WHERE profile_id = v_profile_id; +-- mark profile as successful if it was pending and the new status is success +ELSEIF (NEW.status = 'success') THEN +UPDATE profile_status SET profile_status = 'success', last_updated = NOW() WHERE profile_id = v_profile_id AND profile_status = 'pending'; +-- mark status as failed if it was successful or pending and the new status is failure +-- and there are no errors +ELSIF (NEW.status = 'failure') AND NOT EXISTS ( + SELECT * + FROM rule_evaluations res + INNER JOIN rule_details_eval rde ON res.id = rde.rule_eval_id + WHERE res.profile_id = v_profile_id AND rde.status = 'error' + ) THEN +UPDATE profile_status SET profile_status = 'failure', last_updated = NOW() +WHERE profile_id = v_profile_id AND (profile_status = 'success' OR profile_status = 'pending') AND NEW.status = 'failure'; +END IF; +RETURN NEW; +END; +$$ LANGUAGE plpgsql; + +COMMIT; diff --git a/database/migrations/000007_status_delete_procedure.up.sql b/database/migrations/000007_status_delete_procedure.up.sql new file mode 100644 index 0000000000..3bcb436f97 --- /dev/null +++ b/database/migrations/000007_status_delete_procedure.up.sql @@ -0,0 +1,115 @@ +-- Copyright 2023 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. + +-- Start to make sure the function and trigger are either both added or none +BEGIN; + +-- Define the trigger function (if it doesn't already exist) +CREATE OR REPLACE FUNCTION update_profile_status_on_delete() RETURNS TRIGGER AS $$ +DECLARE + v_status eval_status_types; +BEGIN + SELECT CASE + WHEN EXISTS ( + SELECT 1 FROM rule_details_eval rde + INNER JOIN rule_evaluations res ON res.id = rde.rule_eval_id + WHERE profile_id = OLD.profile_id AND status = 'error' + ) THEN 'error' + WHEN EXISTS ( + SELECT 1 FROM rule_details_eval rde + INNER JOIN rule_evaluations res ON res.id = rde.rule_eval_id + WHERE profile_id = OLD.profile_id AND status = 'failure' + ) THEN 'failure' + WHEN NOT EXISTS ( + SELECT 1 FROM rule_details_eval rde + INNER JOIN rule_evaluations res ON res.id = rde.rule_eval_id + WHERE profile_id = OLD.profile_id + ) THEN 'pending' + WHEN NOT EXISTS ( + SELECT 1 FROM rule_details_eval rde + INNER JOIN rule_evaluations res ON res.id = rde.rule_eval_id + WHERE profile_id = OLD.profile_id AND status != 'skipped' + ) THEN 'skipped' + WHEN NOT EXISTS ( + SELECT 1 FROM rule_details_eval rde + INNER JOIN rule_evaluations res ON res.id = rde.rule_eval_id + WHERE profile_id = OLD.profile_id AND status NOT IN ('success', 'skipped') + ) THEN 'success' + ELSE ( + 'error' -- This should never happen, if yes, make it visible + ) + END INTO v_status; + + UPDATE profile_status SET profile_status = v_status, last_updated = NOW() + WHERE profile_id = OLD.profile_id; + + RETURN NULL; +END; +$$ LANGUAGE plpgsql; + +-- Create the trigger +CREATE TRIGGER update_profile_status_after_delete + AFTER DELETE ON rule_evaluations + FOR EACH ROW + EXECUTE FUNCTION update_profile_status_on_delete(); + +-- Update overall profile status if a rule evaluation status is updated +-- error takes precedence over failure, failure takes precedence over success +CREATE OR REPLACE FUNCTION update_profile_status() RETURNS TRIGGER AS $$ +DECLARE + v_profile_id UUID; +BEGIN + -- Fetch the profile_id for the current rule_eval_id + SELECT profile_id INTO v_profile_id + FROM rule_evaluations + WHERE id = NEW.rule_eval_id; + + -- keep error if profile had errored + IF (NEW.status = 'error') THEN + UPDATE profile_status SET profile_status = 'error', last_updated = NOW() + WHERE profile_id = v_profile_id; + -- only mark profile run as skipped if every evaluation was skipped + ELSEIF (NEW.status = 'skipped') THEN + UPDATE profile_status SET profile_status = 'skipped', last_updated = NOW() + WHERE profile_id = v_profile_id AND NOT EXISTS (SELECT * FROM rule_evaluations res INNER JOIN rule_details_eval rde ON res.id = rde.rule_eval_id WHERE res.profile_id = v_profile_id AND rde.status != 'skipped'); + -- mark status as successful if all evaluations are successful or skipped + ELSEIF NOT EXISTS ( + SELECT * + FROM rule_evaluations res + INNER JOIN rule_details_eval rde ON res.id = rde.rule_eval_id + WHERE res.profile_id = v_profile_id AND rde.status != 'success' AND rde.status != 'skipped' + ) THEN + UPDATE profile_status SET profile_status = 'success', last_updated = NOW() + WHERE profile_id = v_profile_id; + -- mark profile as successful if it was pending and the new status is success + ELSEIF (NEW.status = 'success') THEN + UPDATE profile_status SET profile_status = 'success', last_updated = NOW() WHERE profile_id = v_profile_id AND profile_status = 'pending'; + -- CHANGE: this is the only branch that changed from the original version in this migration + -- mark status as failed if it was successful or pending or skipped and the new status is failure + -- and there are no errors + ELSIF (NEW.status = 'failure') AND NOT EXISTS ( + SELECT * + FROM rule_evaluations res + INNER JOIN rule_details_eval rde ON res.id = rde.rule_eval_id + WHERE res.profile_id = v_profile_id AND rde.status = 'error' + ) THEN + UPDATE profile_status SET profile_status = 'failure', last_updated = NOW() + WHERE profile_id = v_profile_id AND (profile_status = 'success' OR profile_status = 'pending' OR profile_status = 'skipped') AND NEW.status = 'failure'; + END IF; + RETURN NEW; +END; +$$ LANGUAGE plpgsql; + +-- transaction commit +COMMIT; \ No newline at end of file diff --git a/internal/db/profiles_test.go b/internal/db/profiles_test.go index c65006d5db..3c3df86425 100644 --- a/internal/db/profiles_test.go +++ b/internal/db/profiles_test.go @@ -323,6 +323,36 @@ func TestCreateProfileStatusStoredProcedure(t *testing.T) { }, expectedStatusAfterModify: EvalStatusTypesFailure, }, + { + name: "Skipped then failure results in failure", + ruleStatusSetupFn: func(profile Profile, randomEntities *testRandomEntities) { + upsertEvalStatus( + t, profile.ID, randomEntities.repo.ID, randomEntities.ruleType1.ID, + EvalStatusTypesSkipped, "") + }, + expectedStatusAfterSetup: EvalStatusTypesSkipped, + ruleStatusModifyFn: func(profile Profile, randomEntities *testRandomEntities) { + upsertEvalStatus( + t, profile.ID, randomEntities.repo.ID, randomEntities.ruleType2.ID, + EvalStatusTypesFailure, "") + }, + expectedStatusAfterModify: EvalStatusTypesFailure, + }, + { + name: "Skipped then success results in success", + ruleStatusSetupFn: func(profile Profile, randomEntities *testRandomEntities) { + upsertEvalStatus( + t, profile.ID, randomEntities.repo.ID, randomEntities.ruleType1.ID, + EvalStatusTypesSkipped, "") + }, + expectedStatusAfterSetup: EvalStatusTypesSkipped, + ruleStatusModifyFn: func(profile Profile, randomEntities *testRandomEntities) { + upsertEvalStatus( + t, profile.ID, randomEntities.repo.ID, randomEntities.ruleType2.ID, + EvalStatusTypesSuccess, "") + }, + expectedStatusAfterModify: EvalStatusTypesSuccess, + }, } randomEntities := createTestRandomEntities(t) @@ -338,11 +368,143 @@ func TestCreateProfileStatusStoredProcedure(t *testing.T) { tt.ruleStatusSetupFn(profile, randomEntities) prfStatusRow := profileIDStatusByIdAndProject(t, profile.ID, randomEntities.proj.ID) - require.Equal(t, prfStatusRow.ProfileStatus, tt.expectedStatusAfterSetup) + require.Equal(t, tt.expectedStatusAfterSetup, prfStatusRow.ProfileStatus) tt.ruleStatusModifyFn(profile, randomEntities) prfStatusRow = profileIDStatusByIdAndProject(t, profile.ID, randomEntities.proj.ID) - require.Equal(t, prfStatusRow.ProfileStatus, tt.expectedStatusAfterModify) + require.Equal(t, tt.expectedStatusAfterModify, prfStatusRow.ProfileStatus) + + err := testQueries.DeleteProfile(context.Background(), profile.ID) + require.NoError(t, err) + }) + } +} + +func TestCreateProfileStatusStoredDeleteProcedure(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + ruleStatusSetupFn func(profile Profile, randomEntities *testRandomEntities, delRepo *Repository) + expectedStatusAfterSetup EvalStatusTypes + ruleStatusDeleteFn func(delRepo *Repository) + expectedStatusAfterModify EvalStatusTypes + }{ + { + name: "Removing last failure results in success", + ruleStatusSetupFn: func(profile Profile, randomEntities *testRandomEntities, delRepo *Repository) { + upsertEvalStatus( + t, profile.ID, randomEntities.repo.ID, randomEntities.ruleType1.ID, + EvalStatusTypesSuccess, "") + upsertEvalStatus( + t, profile.ID, randomEntities.repo.ID, randomEntities.ruleType2.ID, + EvalStatusTypesSuccess, "") + + upsertEvalStatus( + t, profile.ID, delRepo.ID, randomEntities.ruleType1.ID, + EvalStatusTypesFailure, "") + upsertEvalStatus( + t, profile.ID, delRepo.ID, randomEntities.ruleType2.ID, + EvalStatusTypesSuccess, "") + }, + expectedStatusAfterSetup: EvalStatusTypesFailure, + ruleStatusDeleteFn: func(delRepo *Repository) { + err := testQueries.DeleteRepository(context.Background(), delRepo.ID) + require.NoError(t, err) + }, + expectedStatusAfterModify: EvalStatusTypesSuccess, + }, + { + name: "Removing last error results in failure", + ruleStatusSetupFn: func(profile Profile, randomEntities *testRandomEntities, delRepo *Repository) { + upsertEvalStatus( + t, profile.ID, randomEntities.repo.ID, randomEntities.ruleType1.ID, + EvalStatusTypesFailure, "") + upsertEvalStatus( + t, profile.ID, randomEntities.repo.ID, randomEntities.ruleType2.ID, + EvalStatusTypesSuccess, "") + + upsertEvalStatus( + t, profile.ID, delRepo.ID, randomEntities.ruleType1.ID, + EvalStatusTypesSuccess, "") + upsertEvalStatus( + t, profile.ID, delRepo.ID, randomEntities.ruleType2.ID, + EvalStatusTypesError, "") + }, + + expectedStatusAfterSetup: EvalStatusTypesError, + ruleStatusDeleteFn: func(delRepo *Repository) { + err := testQueries.DeleteRepository(context.Background(), delRepo.ID) + require.NoError(t, err) + }, + expectedStatusAfterModify: EvalStatusTypesFailure, + }, + { + name: "Removing one error retains the other one", + ruleStatusSetupFn: func(profile Profile, randomEntities *testRandomEntities, delRepo *Repository) { + upsertEvalStatus( + t, profile.ID, randomEntities.repo.ID, randomEntities.ruleType1.ID, + EvalStatusTypesFailure, "") + upsertEvalStatus( + t, profile.ID, randomEntities.repo.ID, randomEntities.ruleType2.ID, + EvalStatusTypesError, "") + + upsertEvalStatus( + t, profile.ID, delRepo.ID, randomEntities.ruleType1.ID, + EvalStatusTypesSuccess, "") + upsertEvalStatus( + t, profile.ID, delRepo.ID, randomEntities.ruleType2.ID, + EvalStatusTypesError, "") + }, + + expectedStatusAfterSetup: EvalStatusTypesError, + ruleStatusDeleteFn: func(delRepo *Repository) { + err := testQueries.DeleteRepository(context.Background(), delRepo.ID) + require.NoError(t, err) + }, + expectedStatusAfterModify: EvalStatusTypesError, + }, + { + name: "Removing all but skipped returns skipped", + ruleStatusSetupFn: func(profile Profile, randomEntities *testRandomEntities, delRepo *Repository) { + upsertEvalStatus( + t, profile.ID, randomEntities.repo.ID, randomEntities.ruleType1.ID, + EvalStatusTypesSkipped, "") + + upsertEvalStatus( + t, profile.ID, delRepo.ID, randomEntities.ruleType1.ID, + EvalStatusTypesFailure, "") + }, + + expectedStatusAfterSetup: EvalStatusTypesFailure, + ruleStatusDeleteFn: func(delRepo *Repository) { + err := testQueries.DeleteRepository(context.Background(), delRepo.ID) + require.NoError(t, err) + }, + expectedStatusAfterModify: EvalStatusTypesSkipped, + }, + } + + randomEntities := createTestRandomEntities(t) + + for _, tt := range tests { + tt := tt + + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + profile := createRandomProfile(t, randomEntities.prov.Name, randomEntities.proj.ID) + require.NotEmpty(t, profile) + + delRepo := createRandomRepository(t, randomEntities.proj.ID, randomEntities.prov.Name) + + tt.ruleStatusSetupFn(profile, randomEntities, &delRepo) + prfStatusRow := profileIDStatusByIdAndProject(t, profile.ID, randomEntities.proj.ID) + require.Equal(t, tt.expectedStatusAfterSetup, prfStatusRow.ProfileStatus) + + tt.ruleStatusDeleteFn(&delRepo) + prfStatusRow = profileIDStatusByIdAndProject(t, profile.ID, randomEntities.proj.ID) + require.Equal(t, tt.expectedStatusAfterModify, prfStatusRow.ProfileStatus) err := testQueries.DeleteProfile(context.Background(), profile.ID) require.NoError(t, err)