From 1aa28e6e5808c8a2ee5b5cc80aefd215f616a060 Mon Sep 17 00:00:00 2001 From: Don Browne Date: Thu, 30 May 2024 09:27:37 +0100 Subject: [PATCH 1/5] Dual write rule instances to new and old tables Relates to #3485 Any time a profile is created or updated, write the rule instances to both the new and old rule instance tables. Mark the entries in the old tables as migrated so that we skip over them when we run the migration process. --- database/mock/store.go | 59 ++++++++++ database/query/profiles.sql | 6 +- database/query/rule_instances.sql | 52 +++++++++ internal/db/profiles.sql.go | 6 +- internal/db/querier.go | 17 +++ internal/db/rule_instances.sql.go | 176 ++++++++++++++++++++++++++++++ internal/profiles/service.go | 129 +++++++++++++++++++++- 7 files changed, 438 insertions(+), 7 deletions(-) create mode 100644 database/query/rule_instances.sql create mode 100644 internal/db/rule_instances.sql.go diff --git a/database/mock/store.go b/database/mock/store.go index 13e1eadb68..cd9465d282 100644 --- a/database/mock/store.go +++ b/database/mock/store.go @@ -354,6 +354,20 @@ func (mr *MockStoreMockRecorder) DeleteInstallationIDByAppID(arg0, arg1 any) *go return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteInstallationIDByAppID", reflect.TypeOf((*MockStore)(nil).DeleteInstallationIDByAppID), arg0, arg1) } +// DeleteNonUpdatedRules mocks base method. +func (m *MockStore) DeleteNonUpdatedRules(arg0 context.Context, arg1 db.DeleteNonUpdatedRulesParams) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "DeleteNonUpdatedRules", arg0, arg1) + ret0, _ := ret[0].(error) + return ret0 +} + +// DeleteNonUpdatedRules indicates an expected call of DeleteNonUpdatedRules. +func (mr *MockStoreMockRecorder) DeleteNonUpdatedRules(arg0, arg1 any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteNonUpdatedRules", reflect.TypeOf((*MockStore)(nil).DeleteNonUpdatedRules), arg0, arg1) +} + // DeleteProfile mocks base method. func (m *MockStore) DeleteProfile(arg0 context.Context, arg1 db.DeleteProfileParams) error { m.ctrl.T.Helper() @@ -1108,6 +1122,36 @@ func (mr *MockStoreMockRecorder) GetRuleEvaluationByProfileIdAndRuleType(arg0, a return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetRuleEvaluationByProfileIdAndRuleType", reflect.TypeOf((*MockStore)(nil).GetRuleEvaluationByProfileIdAndRuleType), arg0, arg1, arg2, arg3, arg4, arg5) } +// GetRuleInstancesForProfile mocks base method. +func (m *MockStore) GetRuleInstancesForProfile(arg0 context.Context, arg1 uuid.UUID) ([]db.RuleInstance, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetRuleInstancesForProfile", arg0, arg1) + ret0, _ := ret[0].([]db.RuleInstance) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// GetRuleInstancesForProfile indicates an expected call of GetRuleInstancesForProfile. +func (mr *MockStoreMockRecorder) GetRuleInstancesForProfile(arg0, arg1 any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetRuleInstancesForProfile", reflect.TypeOf((*MockStore)(nil).GetRuleInstancesForProfile), arg0, arg1) +} + +// GetRuleInstancesForProfileEntity mocks base method. +func (m *MockStore) GetRuleInstancesForProfileEntity(arg0 context.Context, arg1 db.GetRuleInstancesForProfileEntityParams) ([]db.RuleInstance, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetRuleInstancesForProfileEntity", arg0, arg1) + ret0, _ := ret[0].([]db.RuleInstance) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// GetRuleInstancesForProfileEntity indicates an expected call of GetRuleInstancesForProfileEntity. +func (mr *MockStoreMockRecorder) GetRuleInstancesForProfileEntity(arg0, arg1 any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetRuleInstancesForProfileEntity", reflect.TypeOf((*MockStore)(nil).GetRuleInstancesForProfileEntity), arg0, arg1) +} + // GetRuleTypeByID mocks base method. func (m *MockStore) GetRuleTypeByID(arg0 context.Context, arg1 uuid.UUID) (db.RuleType, error) { m.ctrl.T.Helper() @@ -1761,6 +1805,21 @@ func (mr *MockStoreMockRecorder) UpsertRuleEvaluations(arg0, arg1 any) *gomock.C return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "UpsertRuleEvaluations", reflect.TypeOf((*MockStore)(nil).UpsertRuleEvaluations), arg0, arg1) } +// UpsertRuleInstance mocks base method. +func (m *MockStore) UpsertRuleInstance(arg0 context.Context, arg1 db.UpsertRuleInstanceParams) (uuid.UUID, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "UpsertRuleInstance", arg0, arg1) + ret0, _ := ret[0].(uuid.UUID) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// UpsertRuleInstance indicates an expected call of UpsertRuleInstance. +func (mr *MockStoreMockRecorder) UpsertRuleInstance(arg0, arg1 any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "UpsertRuleInstance", reflect.TypeOf((*MockStore)(nil).UpsertRuleInstance), arg0, arg1) +} + // UpsertRuleInstantiation mocks base method. func (m *MockStore) UpsertRuleInstantiation(arg0 context.Context, arg1 db.UpsertRuleInstantiationParams) (db.EntityProfileRule, error) { m.ctrl.T.Helper() diff --git a/database/query/profiles.sql b/database/query/profiles.sql index 9e9755faf3..df12c01b5b 100644 --- a/database/query/profiles.sql +++ b/database/query/profiles.sql @@ -28,7 +28,7 @@ INSERT INTO entity_profiles ( $1, $2, sqlc.arg(contextual_rules)::jsonb, - FALSE + TRUE ) RETURNING *; -- name: UpsertProfileForEntity :one @@ -40,7 +40,7 @@ INSERT INTO entity_profiles ( ) VALUES ($1, $2, sqlc.arg(contextual_rules)::jsonb, false) ON CONFLICT (entity, profile_id) DO UPDATE SET contextual_rules = sqlc.arg(contextual_rules)::jsonb, - migrated = FALSE + migrated = TRUE RETURNING *; -- name: DeleteProfileForEntity :exec @@ -110,7 +110,7 @@ GROUP BY profiles.id; -- name: CountProfilesByEntityType :many SELECT COUNT(p.id) AS num_profiles, ep.entity AS profile_entity FROM profiles AS p - JOIN entity_profiles AS ep ON p.id = ep.profile_id +JOIN entity_profiles AS ep ON p.id = ep.profile_id GROUP BY ep.entity; -- name: CountProfilesByName :one diff --git a/database/query/rule_instances.sql b/database/query/rule_instances.sql new file mode 100644 index 0000000000..159446f866 --- /dev/null +++ b/database/query/rule_instances.sql @@ -0,0 +1,52 @@ +-- 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. + +-- name: UpsertRuleInstance :one +INSERT INTO rule_instances ( + profile_id, + rule_type_id, + name, + entity_type, + def, + params, + created_at, + updated_at +) VALUES( + $1, + $2, + $3, + $4, + $5, + $6, + NOW(), + NOW() +) +ON CONFLICT (profile_id, entity_type, name) DO UPDATE SET + rule_type_id = $2, + def = $5, + params = $6, + updated_at = NOW() +RETURNING id; + +-- name: GetRuleInstancesForProfile :many +SELECT * FROM rule_instances WHERE profile_id = $1; + +-- name: GetRuleInstancesForProfileEntity :many +SELECT * FROM rule_instances WHERE profile_id = $1 AND entity_type = $2; + +-- name: DeleteNonUpdatedRules :exec +DELETE FROM rule_instances +WHERE profile_id = $1 +AND entity_type = $2 +AND id <> ALL(sqlc.arg(updated_ids)::UUID[]); \ No newline at end of file diff --git a/internal/db/profiles.sql.go b/internal/db/profiles.sql.go index bdee5a8724..8a984351e4 100644 --- a/internal/db/profiles.sql.go +++ b/internal/db/profiles.sql.go @@ -17,7 +17,7 @@ import ( const countProfilesByEntityType = `-- name: CountProfilesByEntityType :many SELECT COUNT(p.id) AS num_profiles, ep.entity AS profile_entity FROM profiles AS p - JOIN entity_profiles AS ep ON p.id = ep.profile_id +JOIN entity_profiles AS ep ON p.id = ep.profile_id GROUP BY ep.entity ` @@ -120,7 +120,7 @@ INSERT INTO entity_profiles ( $1, $2, $3::jsonb, - FALSE + TRUE ) RETURNING id, entity, profile_id, contextual_rules, created_at, updated_at, migrated ` @@ -578,7 +578,7 @@ INSERT INTO entity_profiles ( ) VALUES ($1, $2, $3::jsonb, false) ON CONFLICT (entity, profile_id) DO UPDATE SET contextual_rules = $3::jsonb, - migrated = FALSE + migrated = TRUE RETURNING id, entity, profile_id, contextual_rules, created_at, updated_at, migrated ` diff --git a/internal/db/querier.go b/internal/db/querier.go index e95228a69e..5d26672009 100644 --- a/internal/db/querier.go +++ b/internal/db/querier.go @@ -32,6 +32,7 @@ type Querier interface { DeleteArtifact(ctx context.Context, id uuid.UUID) error DeleteExpiredSessionStates(ctx context.Context) (int64, error) DeleteInstallationIDByAppID(ctx context.Context, appInstallationID int64) error + DeleteNonUpdatedRules(ctx context.Context, arg DeleteNonUpdatedRulesParams) error DeleteProfile(ctx context.Context, arg DeleteProfileParams) error DeleteProfileForEntity(ctx context.Context, arg DeleteProfileForEntityParams) error DeleteProject(ctx context.Context, id uuid.UUID) ([]DeleteProjectRow, error) @@ -95,6 +96,8 @@ type Querier interface { GetRepositoryByIDAndProject(ctx context.Context, arg GetRepositoryByIDAndProjectParams) (Repository, error) GetRepositoryByRepoID(ctx context.Context, repoID int64) (Repository, error) GetRepositoryByRepoName(ctx context.Context, arg GetRepositoryByRepoNameParams) (Repository, error) + GetRuleInstancesForProfile(ctx context.Context, profileID uuid.UUID) ([]RuleInstance, error) + GetRuleInstancesForProfileEntity(ctx context.Context, arg GetRuleInstancesForProfileEntityParams) ([]RuleInstance, error) GetRuleTypeByID(ctx context.Context, id uuid.UUID) (RuleType, error) GetRuleTypeByName(ctx context.Context, arg GetRuleTypeByNameParams) (RuleType, error) GetSubscriptionByProjectBundle(ctx context.Context, arg GetSubscriptionByProjectBundleParams) (Subscription, error) @@ -182,6 +185,20 @@ type Querier interface { UpsertRuleDetailsEval(ctx context.Context, arg UpsertRuleDetailsEvalParams) (uuid.UUID, error) UpsertRuleDetailsRemediate(ctx context.Context, arg UpsertRuleDetailsRemediateParams) (uuid.UUID, error) UpsertRuleEvaluations(ctx context.Context, arg UpsertRuleEvaluationsParams) (uuid.UUID, error) + // 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. + UpsertRuleInstance(ctx context.Context, arg UpsertRuleInstanceParams) (uuid.UUID, error) UpsertRuleInstantiation(ctx context.Context, arg UpsertRuleInstantiationParams) (EntityProfileRule, error) } diff --git a/internal/db/rule_instances.sql.go b/internal/db/rule_instances.sql.go new file mode 100644 index 0000000000..e591ee22cb --- /dev/null +++ b/internal/db/rule_instances.sql.go @@ -0,0 +1,176 @@ +// Code generated by sqlc. DO NOT EDIT. +// versions: +// sqlc v1.26.0 +// source: rule_instances.sql + +package db + +import ( + "context" + + "github.com/google/uuid" + "github.com/lib/pq" + "github.com/sqlc-dev/pqtype" +) + +const deleteNonUpdatedRules = `-- name: DeleteNonUpdatedRules :exec +DELETE FROM rule_instances +WHERE profile_id = $1 +AND entity_type = $2 +AND id <> ALL($3::UUID[]) +` + +type DeleteNonUpdatedRulesParams struct { + ProfileID uuid.UUID `json:"profile_id"` + EntityType Entities `json:"entity_type"` + UpdatedIds []uuid.UUID `json:"updated_ids"` +} + +func (q *Queries) DeleteNonUpdatedRules(ctx context.Context, arg DeleteNonUpdatedRulesParams) error { + _, err := q.db.ExecContext(ctx, deleteNonUpdatedRules, arg.ProfileID, arg.EntityType, pq.Array(arg.UpdatedIds)) + return err +} + +const getRuleInstancesForProfile = `-- name: GetRuleInstancesForProfile :many +SELECT id, profile_id, rule_type_id, name, entity_type, def, params, created_at, updated_at FROM rule_instances WHERE profile_id = $1 +` + +func (q *Queries) GetRuleInstancesForProfile(ctx context.Context, profileID uuid.UUID) ([]RuleInstance, error) { + rows, err := q.db.QueryContext(ctx, getRuleInstancesForProfile, profileID) + if err != nil { + return nil, err + } + defer rows.Close() + items := []RuleInstance{} + for rows.Next() { + var i RuleInstance + if err := rows.Scan( + &i.ID, + &i.ProfileID, + &i.RuleTypeID, + &i.Name, + &i.EntityType, + &i.Def, + &i.Params, + &i.CreatedAt, + &i.UpdatedAt, + ); err != nil { + return nil, err + } + items = append(items, i) + } + if err := rows.Close(); err != nil { + return nil, err + } + if err := rows.Err(); err != nil { + return nil, err + } + return items, nil +} + +const getRuleInstancesForProfileEntity = `-- name: GetRuleInstancesForProfileEntity :many +SELECT id, profile_id, rule_type_id, name, entity_type, def, params, created_at, updated_at FROM rule_instances WHERE profile_id = $1 AND entity_type = $2 +` + +type GetRuleInstancesForProfileEntityParams struct { + ProfileID uuid.UUID `json:"profile_id"` + EntityType Entities `json:"entity_type"` +} + +func (q *Queries) GetRuleInstancesForProfileEntity(ctx context.Context, arg GetRuleInstancesForProfileEntityParams) ([]RuleInstance, error) { + rows, err := q.db.QueryContext(ctx, getRuleInstancesForProfileEntity, arg.ProfileID, arg.EntityType) + if err != nil { + return nil, err + } + defer rows.Close() + items := []RuleInstance{} + for rows.Next() { + var i RuleInstance + if err := rows.Scan( + &i.ID, + &i.ProfileID, + &i.RuleTypeID, + &i.Name, + &i.EntityType, + &i.Def, + &i.Params, + &i.CreatedAt, + &i.UpdatedAt, + ); err != nil { + return nil, err + } + items = append(items, i) + } + if err := rows.Close(); err != nil { + return nil, err + } + if err := rows.Err(); err != nil { + return nil, err + } + return items, nil +} + +const upsertRuleInstance = `-- name: UpsertRuleInstance :one + +INSERT INTO rule_instances ( + profile_id, + rule_type_id, + name, + entity_type, + def, + params, + created_at, + updated_at +) VALUES( + $1, + $2, + $3, + $4, + $5, + $6, + NOW(), + NOW() +) +ON CONFLICT (profile_id, entity_type, name) DO UPDATE SET + rule_type_id = $2, + def = $5, + params = $6, + updated_at = NOW() +RETURNING id +` + +type UpsertRuleInstanceParams struct { + ProfileID uuid.UUID `json:"profile_id"` + RuleTypeID uuid.UUID `json:"rule_type_id"` + Name string `json:"name"` + EntityType Entities `json:"entity_type"` + Def pqtype.NullRawMessage `json:"def"` + Params pqtype.NullRawMessage `json:"params"` +} + +// 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. +func (q *Queries) UpsertRuleInstance(ctx context.Context, arg UpsertRuleInstanceParams) (uuid.UUID, error) { + row := q.db.QueryRowContext(ctx, upsertRuleInstance, + arg.ProfileID, + arg.RuleTypeID, + arg.Name, + arg.EntityType, + arg.Def, + arg.Params, + ) + var id uuid.UUID + err := row.Scan(&id) + return id, err +} diff --git a/internal/profiles/service.go b/internal/profiles/service.go index 5d77bcac4c..e58b8d3ca0 100644 --- a/internal/profiles/service.go +++ b/internal/profiles/service.go @@ -27,6 +27,7 @@ import ( "strings" "github.com/google/uuid" + "github.com/sqlc-dev/pqtype" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" "google.golang.org/protobuf/reflect/protoreflect" @@ -260,7 +261,19 @@ func (p *profileService) UpdateProfile( minderv1.Entity_ENTITY_BUILD_ENVIRONMENTS: profile.GetBuildEnvironment(), minderv1.Entity_ENTITY_PULL_REQUESTS: profile.GetPullRequest(), } { - if err := updateProfileRulesForEntity(ctx, ent, &updatedProfile, qtx, entRules, rules); err != nil { + if err = updateProfileRulesForEntity(ctx, ent, &updatedProfile, qtx, entRules, rules); err != nil { + return nil, err + } + + err = updateRuleInstances( + ctx, + qtx, + updatedProfile.ID, + entRules, + entities.EntityTypeToDB(ent), + rules, + ) + if err != nil { return nil, err } } @@ -360,6 +373,29 @@ func createProfileRulesForEntity( return nil } + for _, rule := range rules { + entityRuleTuple, ok := rulesInProf[RuleTypeAndNamePair{ + RuleType: rule.Type, + RuleName: rule.Name, + }] + if !ok { + return fmt.Errorf("unable to find rule type ID for %s/%s", rule.Name, rule.Type) + } + + _, err := upsertRuleInstance( + ctx, + qtx, + profile.ID, + rule, + entityRuleTuple.RuleID, + entities.EntityTypeToDB(entity), + ) + + if err != nil { + return err + } + } + marshalled, err := json.Marshal(rules) if err != nil { log.Printf("error marshalling %s rules: %v", entity, err) @@ -707,3 +743,94 @@ func deleteRuleStatusesForProfile( return nil } + +func upsertRuleInstance( + ctx context.Context, + qtx db.Querier, + profileID uuid.UUID, + rule *minderv1.Profile_Rule, + ruleTypeID uuid.UUID, + entityType db.Entities, +) (uuid.UUID, error) { + def, err := json.Marshal(rule.Def) + if err != nil { + return uuid.Nil, fmt.Errorf("unable to serialize rule def: %w", err) + } + + params, err := json.Marshal(rule.Params) + if err != nil { + return uuid.Nil, fmt.Errorf("unable to serialize rule params: %w", err) + } + + newInstance := db.UpsertRuleInstanceParams{ + ProfileID: profileID, + RuleTypeID: ruleTypeID, + Name: rule.Name, + EntityType: entityType, + Def: pqtype.NullRawMessage{ + RawMessage: def, + Valid: def != nil, + }, + Params: pqtype.NullRawMessage{ + RawMessage: params, + Valid: params != nil, + }, + } + + id, err := qtx.UpsertRuleInstance(ctx, newInstance) + if err != nil { + return uuid.Nil, fmt.Errorf("unable to insert new rule instance: %w", err) + } + + return id, nil +} + +func updateRuleInstances( + ctx context.Context, + qtx db.Querier, + profileID uuid.UUID, + newRules []*minderv1.Profile_Rule, + entityType db.Entities, + rulesInProf RuleMapping, +) error { + updatedIDs := make([]uuid.UUID, len(newRules)) + for i, rule := range newRules { + // TODO: Clean up this logic once we no longer have to support the old tables. + entityRuleTuple, ok := rulesInProf[RuleTypeAndNamePair{ + RuleType: rule.Type, + RuleName: rule.Name, + }] + if !ok { + return fmt.Errorf("unable to find rule type ID for %s/%s", rule.Name, rule.Type) + } + + id, err := upsertRuleInstance( + ctx, + qtx, + profileID, + rule, + entityRuleTuple.RuleID, + entityType, + ) + if err != nil { + // enough context in error from upsertRuleInstance + return err + } + + updatedIDs[i] = id + + } + + // Any rule which was not updated was deleted from the profile. + // Remove from the database as well. + err := qtx.DeleteNonUpdatedRules(ctx, db.DeleteNonUpdatedRulesParams{ + ProfileID: profileID, + EntityType: entityType, + UpdatedIds: updatedIDs, + }) + if err != nil { + return fmt.Errorf("error while cleaning up rule instances: %w", err) + } + + return nil +} From 63b9e40665f9cc87cf31cc8fec54c76edf612e94 Mon Sep 17 00:00:00 2001 From: Don Browne Date: Tue, 4 Jun 2024 11:48:50 +0100 Subject: [PATCH 2/5] use ANY instead of ALL --- database/query/rule_instances.sql | 2 +- internal/db/rule_instances.sql.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/database/query/rule_instances.sql b/database/query/rule_instances.sql index 159446f866..f2e7ea1135 100644 --- a/database/query/rule_instances.sql +++ b/database/query/rule_instances.sql @@ -49,4 +49,4 @@ SELECT * FROM rule_instances WHERE profile_id = $1 AND entity_type = $2; DELETE FROM rule_instances WHERE profile_id = $1 AND entity_type = $2 -AND id <> ALL(sqlc.arg(updated_ids)::UUID[]); \ No newline at end of file +AND NOT id = ANY(sqlc.arg(updated_ids)::UUID[]); \ No newline at end of file diff --git a/internal/db/rule_instances.sql.go b/internal/db/rule_instances.sql.go index e591ee22cb..8d04944bbd 100644 --- a/internal/db/rule_instances.sql.go +++ b/internal/db/rule_instances.sql.go @@ -17,7 +17,7 @@ const deleteNonUpdatedRules = `-- name: DeleteNonUpdatedRules :exec DELETE FROM rule_instances WHERE profile_id = $1 AND entity_type = $2 -AND id <> ALL($3::UUID[]) +AND NOT id = ANY($3::UUID[]) ` type DeleteNonUpdatedRulesParams struct { From 5c2c810f4635d4e34fd9be8c3a9afb158911114d Mon Sep 17 00:00:00 2001 From: Don Browne Date: Tue, 4 Jun 2024 12:01:38 +0100 Subject: [PATCH 3/5] remove repeated code --- internal/profiles/service.go | 138 +++++++++++++---------------------- 1 file changed, 51 insertions(+), 87 deletions(-) diff --git a/internal/profiles/service.go b/internal/profiles/service.go index e58b8d3ca0..2201c662cf 100644 --- a/internal/profiles/service.go +++ b/internal/profiles/service.go @@ -265,7 +265,7 @@ func (p *profileService) UpdateProfile( return nil, err } - err = updateRuleInstances( + updatedIDs, err := updateRuleInstances( ctx, qtx, updatedProfile.ID, @@ -276,6 +276,17 @@ func (p *profileService) UpdateProfile( if err != nil { return nil, err } + + // Any rule which was not updated was deleted from the profile. + // Remove from the database as well. + err = qtx.DeleteNonUpdatedRules(ctx, db.DeleteNonUpdatedRulesParams{ + ProfileID: updatedProfile.ID, + EntityType: entities.EntityTypeToDB(ent), + UpdatedIds: updatedIDs, + }) + if err != nil { + return nil, fmt.Errorf("error while cleaning up rule instances: %w", err) + } } unusedRuleStatuses := getUnusedOldRuleStatuses(rules, oldRules) @@ -373,27 +384,16 @@ func createProfileRulesForEntity( return nil } - for _, rule := range rules { - entityRuleTuple, ok := rulesInProf[RuleTypeAndNamePair{ - RuleType: rule.Type, - RuleName: rule.Name, - }] - if !ok { - return fmt.Errorf("unable to find rule type ID for %s/%s", rule.Name, rule.Type) - } - - _, err := upsertRuleInstance( - ctx, - qtx, - profile.ID, - rule, - entityRuleTuple.RuleID, - entities.EntityTypeToDB(entity), - ) - - if err != nil { - return err - } + _, err := updateRuleInstances( + ctx, + qtx, + profile.ID, + rules, + entities.EntityTypeToDB(entity), + rulesInProf, + ) + if err != nil { + return fmt.Errorf("error while creating rule instances: %w", err) } marshalled, err := json.Marshal(rules) @@ -744,47 +744,6 @@ func deleteRuleStatusesForProfile( return nil } -func upsertRuleInstance( - ctx context.Context, - qtx db.Querier, - profileID uuid.UUID, - rule *minderv1.Profile_Rule, - ruleTypeID uuid.UUID, - entityType db.Entities, -) (uuid.UUID, error) { - def, err := json.Marshal(rule.Def) - if err != nil { - return uuid.Nil, fmt.Errorf("unable to serialize rule def: %w", err) - } - - params, err := json.Marshal(rule.Params) - if err != nil { - return uuid.Nil, fmt.Errorf("unable to serialize rule params: %w", err) - } - - newInstance := db.UpsertRuleInstanceParams{ - ProfileID: profileID, - RuleTypeID: ruleTypeID, - Name: rule.Name, - EntityType: entityType, - Def: pqtype.NullRawMessage{ - RawMessage: def, - Valid: def != nil, - }, - Params: pqtype.NullRawMessage{ - RawMessage: params, - Valid: params != nil, - }, - } - - id, err := qtx.UpsertRuleInstance(ctx, newInstance) - if err != nil { - return uuid.Nil, fmt.Errorf("unable to insert new rule instance: %w", err) - } - - return id, nil -} - func updateRuleInstances( ctx context.Context, qtx db.Querier, @@ -792,7 +751,7 @@ func updateRuleInstances( newRules []*minderv1.Profile_Rule, entityType db.Entities, rulesInProf RuleMapping, -) error { +) ([]uuid.UUID, error) { updatedIDs := make([]uuid.UUID, len(newRules)) for i, rule := range newRules { // TODO: Clean up this logic once we no longer have to support the old tables. @@ -801,36 +760,41 @@ func updateRuleInstances( RuleName: rule.Name, }] if !ok { - return fmt.Errorf("unable to find rule type ID for %s/%s", rule.Name, rule.Type) + return nil, fmt.Errorf("unable to find rule type ID for %s/%s", rule.Name, rule.Type) } - id, err := upsertRuleInstance( - ctx, - qtx, - profileID, - rule, - entityRuleTuple.RuleID, - entityType, - ) + def, err := json.Marshal(rule.Def) if err != nil { - // enough context in error from upsertRuleInstance - return err + return nil, fmt.Errorf("unable to serialize rule def: %w", err) } - updatedIDs[i] = id + params, err := json.Marshal(rule.Params) + if err != nil { + return nil, fmt.Errorf("unable to serialize rule params: %w", err) + } - } + newInstance := db.UpsertRuleInstanceParams{ + ProfileID: profileID, + RuleTypeID: entityRuleTuple.RuleID, + Name: rule.Name, + EntityType: entityType, + Def: pqtype.NullRawMessage{ + RawMessage: def, + Valid: def != nil, + }, + Params: pqtype.NullRawMessage{ + RawMessage: params, + Valid: params != nil, + }, + } - // Any rule which was not updated was deleted from the profile. - // Remove from the database as well. - err := qtx.DeleteNonUpdatedRules(ctx, db.DeleteNonUpdatedRulesParams{ - ProfileID: profileID, - EntityType: entityType, - UpdatedIds: updatedIDs, - }) - if err != nil { - return fmt.Errorf("error while cleaning up rule instances: %w", err) + id, err := qtx.UpsertRuleInstance(ctx, newInstance) + if err != nil { + return nil, fmt.Errorf("unable to insert new rule instance: %w", err) + } + + updatedIDs[i] = id } - return nil + return updatedIDs, nil } From 7073b2d8d1e36c2e3926717145205e0488df2136 Mon Sep 17 00:00:00 2001 From: Don Browne Date: Tue, 4 Jun 2024 12:03:33 +0100 Subject: [PATCH 4/5] give function better name --- internal/profiles/service.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/internal/profiles/service.go b/internal/profiles/service.go index 2201c662cf..5ee0a885f2 100644 --- a/internal/profiles/service.go +++ b/internal/profiles/service.go @@ -265,7 +265,7 @@ func (p *profileService) UpdateProfile( return nil, err } - updatedIDs, err := updateRuleInstances( + updatedIDs, err := upsertRuleInstances( ctx, qtx, updatedProfile.ID, @@ -384,7 +384,7 @@ func createProfileRulesForEntity( return nil } - _, err := updateRuleInstances( + _, err := upsertRuleInstances( ctx, qtx, profile.ID, @@ -744,7 +744,7 @@ func deleteRuleStatusesForProfile( return nil } -func updateRuleInstances( +func upsertRuleInstances( ctx context.Context, qtx db.Querier, profileID uuid.UUID, From 78ee85b0fd197fb3d2e05cd01c365db81a69c91f Mon Sep 17 00:00:00 2001 From: Don Browne Date: Wed, 5 Jun 2024 14:12:14 +0100 Subject: [PATCH 5/5] make def/params non nullable --- ...000061_rule_instance_not_null_def.down.sql | 20 +++++++++++++++++++ .../000061_rule_instance_not_null_def.up.sql | 20 +++++++++++++++++++ internal/db/models.go | 18 ++++++++--------- internal/db/rule_instances.sql.go | 14 ++++++------- internal/profiles/service.go | 17 ++++------------ 5 files changed, 60 insertions(+), 29 deletions(-) create mode 100644 database/migrations/000061_rule_instance_not_null_def.down.sql create mode 100644 database/migrations/000061_rule_instance_not_null_def.up.sql diff --git a/database/migrations/000061_rule_instance_not_null_def.down.sql b/database/migrations/000061_rule_instance_not_null_def.down.sql new file mode 100644 index 0000000000..e1cb2a6518 --- /dev/null +++ b/database/migrations/000061_rule_instance_not_null_def.down.sql @@ -0,0 +1,20 @@ +-- 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; + +ALTER TABLE rule_instances ALTER COLUMN def DROP NOT NULL; +ALTER TABLE rule_instances ALTER COLUMN params DROP NOT NULL; + +COMMIT; diff --git a/database/migrations/000061_rule_instance_not_null_def.up.sql b/database/migrations/000061_rule_instance_not_null_def.up.sql new file mode 100644 index 0000000000..221892237a --- /dev/null +++ b/database/migrations/000061_rule_instance_not_null_def.up.sql @@ -0,0 +1,20 @@ +-- 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; + +ALTER TABLE rule_instances ALTER COLUMN def SET NOT NULL; +ALTER TABLE rule_instances ALTER COLUMN params SET NOT NULL; + +COMMIT; diff --git a/internal/db/models.go b/internal/db/models.go index 3d425e72cd..57e08af417 100644 --- a/internal/db/models.go +++ b/internal/db/models.go @@ -639,15 +639,15 @@ type RuleEvaluation struct { } type RuleInstance struct { - ID uuid.UUID `json:"id"` - ProfileID uuid.UUID `json:"profile_id"` - RuleTypeID uuid.UUID `json:"rule_type_id"` - Name string `json:"name"` - EntityType Entities `json:"entity_type"` - Def pqtype.NullRawMessage `json:"def"` - Params pqtype.NullRawMessage `json:"params"` - CreatedAt time.Time `json:"created_at"` - UpdatedAt time.Time `json:"updated_at"` + ID uuid.UUID `json:"id"` + ProfileID uuid.UUID `json:"profile_id"` + RuleTypeID uuid.UUID `json:"rule_type_id"` + Name string `json:"name"` + EntityType Entities `json:"entity_type"` + Def json.RawMessage `json:"def"` + Params json.RawMessage `json:"params"` + CreatedAt time.Time `json:"created_at"` + UpdatedAt time.Time `json:"updated_at"` } type RuleType struct { diff --git a/internal/db/rule_instances.sql.go b/internal/db/rule_instances.sql.go index 8d04944bbd..2fc742221e 100644 --- a/internal/db/rule_instances.sql.go +++ b/internal/db/rule_instances.sql.go @@ -7,10 +7,10 @@ package db import ( "context" + "encoding/json" "github.com/google/uuid" "github.com/lib/pq" - "github.com/sqlc-dev/pqtype" ) const deleteNonUpdatedRules = `-- name: DeleteNonUpdatedRules :exec @@ -140,12 +140,12 @@ RETURNING id ` type UpsertRuleInstanceParams struct { - ProfileID uuid.UUID `json:"profile_id"` - RuleTypeID uuid.UUID `json:"rule_type_id"` - Name string `json:"name"` - EntityType Entities `json:"entity_type"` - Def pqtype.NullRawMessage `json:"def"` - Params pqtype.NullRawMessage `json:"params"` + ProfileID uuid.UUID `json:"profile_id"` + RuleTypeID uuid.UUID `json:"rule_type_id"` + Name string `json:"name"` + EntityType Entities `json:"entity_type"` + Def json.RawMessage `json:"def"` + Params json.RawMessage `json:"params"` } // Copyright 2024 Stacklok, Inc diff --git a/internal/profiles/service.go b/internal/profiles/service.go index 5ee0a885f2..7cb3fb9a40 100644 --- a/internal/profiles/service.go +++ b/internal/profiles/service.go @@ -27,7 +27,6 @@ import ( "strings" "github.com/google/uuid" - "github.com/sqlc-dev/pqtype" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" "google.golang.org/protobuf/reflect/protoreflect" @@ -773,22 +772,14 @@ func upsertRuleInstances( return nil, fmt.Errorf("unable to serialize rule params: %w", err) } - newInstance := db.UpsertRuleInstanceParams{ + id, err := qtx.UpsertRuleInstance(ctx, db.UpsertRuleInstanceParams{ ProfileID: profileID, RuleTypeID: entityRuleTuple.RuleID, Name: rule.Name, EntityType: entityType, - Def: pqtype.NullRawMessage{ - RawMessage: def, - Valid: def != nil, - }, - Params: pqtype.NullRawMessage{ - RawMessage: params, - Valid: params != nil, - }, - } - - id, err := qtx.UpsertRuleInstance(ctx, newInstance) + Def: def, + Params: params, + }) if err != nil { return nil, fmt.Errorf("unable to insert new rule instance: %w", err) }