Skip to content

Commit

Permalink
Add a stored procedure to handle entity deletions
Browse files Browse the repository at this point in the history
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: mindersec#1610
  • Loading branch information
jhrozek committed Nov 13, 2023
1 parent 3e074a3 commit 50350c5
Show file tree
Hide file tree
Showing 3 changed files with 345 additions and 2 deletions.
66 changes: 66 additions & 0 deletions database/migrations/000007_status_delete_procedure.down.sql
Original file line number Diff line number Diff line change
@@ -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;
115 changes: 115 additions & 0 deletions database/migrations/000007_status_delete_procedure.up.sql
Original file line number Diff line number Diff line change
@@ -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;
166 changes: 164 additions & 2 deletions internal/db/profiles_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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)
Expand Down

0 comments on commit 50350c5

Please sign in to comment.