From fcc8c0be7314d2d50aa5a3d8151f55a85965b5cb Mon Sep 17 00:00:00 2001 From: Marko Gacesa Date: Fri, 20 Oct 2023 11:06:33 +0000 Subject: [PATCH] use dedicated DB query to fetch all repo rules (#702) --- app/api/controller/pullreq/merge.go | 2 +- app/services/protection/service.go | 49 ++----------- app/services/protection/set.go | 10 ++- app/store/database.go | 3 + app/store/database/rule.go | 105 ++++++++++++++++++++++++++++ types/rule.go | 21 +++++- 6 files changed, 136 insertions(+), 54 deletions(-) diff --git a/app/api/controller/pullreq/merge.go b/app/api/controller/pullreq/merge.go index 42194f862..4eb7efabb 100644 --- a/app/api/controller/pullreq/merge.go +++ b/app/api/controller/pullreq/merge.go @@ -148,7 +148,7 @@ func (c *Controller) Merge( return types.MergeResponse{}, fmt.Errorf("failed to list status checks: %w", err) } - protectionRules, err := c.protectionManager.Repo(ctx, targetRepo.ID, enum.RuleStateActive, enum.RuleStateMonitor) + protectionRules, err := c.protectionManager.ForRepository(ctx, targetRepo.ID) if err != nil { return types.MergeResponse{}, fmt.Errorf("failed to fetch protection rules for the repository: %w", err) } diff --git a/app/services/protection/service.go b/app/services/protection/service.go index 9170f338c..30acd7321 100644 --- a/app/services/protection/service.go +++ b/app/services/protection/service.go @@ -22,7 +22,6 @@ import ( "github.com/harness/gitness/app/store" "github.com/harness/gitness/types" - "github.com/harness/gitness/types/enum" ) // NewManager creates new protection Manager. @@ -88,54 +87,14 @@ func (m *Manager) SanitizeJSON(ruleType types.RuleType, message json.RawMessage) return toJSON(r) } -func (m *Manager) Space(ctx context.Context, spaceID int64, ruleStates ...enum.RuleState) (Protection, error) { - // TODO: Include the rules of the parent spaces if any. - // TODO: Use some other function to fetch the rules, that returns just basic info about the rule - - rules, err := m.ruleStore.List(ctx, &spaceID, nil, &types.RuleFilter{ - ListQueryFilter: types.ListQueryFilter{ - Pagination: types.Pagination{ - Page: 1, - Size: 1000, - }, - Query: "", - }, - States: ruleStates, - Sort: enum.RuleSortCreated, - Order: enum.OrderAsc, - }) +func (m *Manager) ForRepository(ctx context.Context, repoID int64) (Protection, error) { + ruleInfos, err := m.ruleStore.ListAllRepoRules(ctx, repoID) if err != nil { - return ruleSet{}, fmt.Errorf("failed to list rules for space: %w", err) + return nil, fmt.Errorf("failed to list rules for repository: %w", err) } return ruleSet{ - rules: rules, - manager: m, - }, nil -} - -func (m *Manager) Repo(ctx context.Context, repoID int64, ruleStates ...enum.RuleState) (Protection, error) { - // TODO: Include rules of the space and its parent spaces. - // TODO: Use some other function to fetch the rules, that returns just basic info about the rule - - rules, err := m.ruleStore.List(ctx, nil, &repoID, &types.RuleFilter{ - ListQueryFilter: types.ListQueryFilter{ - Pagination: types.Pagination{ - Page: 1, - Size: 1000, - }, - Query: "", - }, - States: ruleStates, - Sort: enum.RuleSortCreated, - Order: enum.OrderAsc, - }) - if err != nil { - return ruleSet{}, fmt.Errorf("failed to list rules for repository: %w", err) - } - - return ruleSet{ - rules: rules, + rules: ruleInfos, manager: m, }, nil } diff --git a/app/services/protection/set.go b/app/services/protection/set.go index 9930c2a0c..06fce7b75 100644 --- a/app/services/protection/set.go +++ b/app/services/protection/set.go @@ -23,7 +23,7 @@ import ( ) type ruleSet struct { - rules []types.Rule + rules []types.RuleInfoInternal manager *Manager } @@ -33,9 +33,7 @@ func (s ruleSet) CanMerge(ctx context.Context, in CanMergeInput) (CanMergeOutput var out CanMergeOutput var violations []types.RuleViolations - for i := range s.rules { - r := s.rules[i] - + for _, r := range s.rules { bp := Pattern{} if err := json.Unmarshal(r.Pattern, &bp); err != nil { @@ -57,14 +55,14 @@ func (s ruleSet) CanMerge(ctx context.Context, in CanMergeInput) (CanMergeOutput return out, nil, err } - violations = append(violations, backFillRule(rVs, &r)...) + violations = append(violations, backFillRule(rVs, r.RuleInfo)...) out.DeleteSourceBranch = out.DeleteSourceBranch || rOut.DeleteSourceBranch } return out, violations, nil } -func backFillRule(vs []types.RuleViolations, rule *types.Rule) []types.RuleViolations { +func backFillRule(vs []types.RuleViolations, rule types.RuleInfo) []types.RuleViolations { violations := make([]types.RuleViolations, 0, len(vs)) for i := range vs { diff --git a/app/store/database.go b/app/store/database.go index 39f123953..05de5987c 100644 --- a/app/store/database.go +++ b/app/store/database.go @@ -400,6 +400,9 @@ type ( // List returns a list of protection rules of a repository or a space that matches the provided criteria. List(ctx context.Context, spaceID, repoID *int64, filter *types.RuleFilter) ([]types.Rule, error) + + // ListAllRepoRules returns a list of all protection rules that can be applied on a repository. + ListAllRepoRules(ctx context.Context, repoID int64) ([]types.RuleInfoInternal, error) } // WebhookStore defines the webhook data storage. diff --git a/app/store/database/rule.go b/app/store/database/rule.go index 66f14214f..709b0a350 100644 --- a/app/store/database/rule.go +++ b/app/store/database/rule.go @@ -341,6 +341,86 @@ func (s *RuleStore) List(ctx context.Context, spaceID, repoID *int64, filter *ty return s.mapToRules(ctx, dst), nil } +type ruleInfo struct { + SpacePath string `db:"space_path"` + RepoPath string `db:"repo_path"` + ID int64 `db:"rule_id"` + UID string `db:"rule_uid"` + Type types.RuleType `db:"rule_type"` + State enum.RuleState `db:"rule_state"` + Pattern string `db:"rule_pattern"` + Definition string `db:"rule_definition"` +} + +// ListAllRepoRules returns a list of all protection rules that can be applied on a repository. +// This includes the rules defined directly on the repository and all those defined on the parent spaces. +func (s *RuleStore) ListAllRepoRules(ctx context.Context, repoID int64) ([]types.RuleInfoInternal, error) { + const query = ` + WITH RECURSIVE + repo_info(repo_id, repo_uid, repo_space_id) AS ( + SELECT repo_id, repo_uid, repo_parent_id + FROM repositories + WHERE repo_id = $1 + ), + space_parents(space_id, space_uid, space_parent_id) AS ( + SELECT space_id, space_uid, space_parent_id + FROM spaces + INNER JOIN repo_info ON repo_info.repo_space_id = spaces.space_id + UNION ALL + SELECT spaces.space_id, spaces.space_uid, spaces.space_parent_id + FROM spaces + INNER JOIN space_parents ON space_parents.space_parent_id = spaces.space_id + ), + spaces_with_path(space_id, space_parent_id, space_uid, space_full_path) AS ( + SELECT space_id, space_parent_id, space_uid, space_uid + FROM space_parents + WHERE space_parent_id IS NULL + UNION ALL + SELECT + space_parents.space_id, + space_parents.space_parent_id, + space_parents.space_uid, + spaces_with_path.space_full_path || '/' || space_parents.space_uid + FROM space_parents + INNER JOIN spaces_with_path ON spaces_with_path.space_id = space_parents.space_parent_id + ) + SELECT + space_full_path AS "space_path" + ,'' as "repo_path" + ,rule_id + ,rule_uid + ,rule_type + ,rule_state + ,rule_pattern + ,rule_definition + FROM spaces_with_path + INNER JOIN rules ON rules.rule_space_id = spaces_with_path.space_id + WHERE rule_state IN ('active', 'monitor') + UNION ALL + SELECT + '' as "space_path" + ,space_full_path || '/' || repo_info.repo_uid AS "repo_path" + ,rule_id + ,rule_uid + ,rule_type + ,rule_state + ,rule_pattern + ,rule_definition + FROM rules + INNER JOIN repo_info ON repo_info.repo_id = rules.rule_repo_id + INNER JOIN spaces_with_path ON spaces_with_path.space_id = repo_info.repo_space_id + WHERE rule_state IN ('active', 'monitor')` + + db := dbtx.GetAccessor(ctx, s.db) + + result := make([]ruleInfo, 0) + if err := db.SelectContext(ctx, &result, query, repoID); err != nil { + return nil, database.ProcessSQLErrorf(err, "Failed executing custom list query") + } + + return s.mapToRuleInfos(result), nil +} + func (*RuleStore) applyParentID( stmt squirrel.SelectBuilder, spaceID, repoID *int64, @@ -433,3 +513,28 @@ func mapToInternalRule(in *types.Rule) rule { Definition: string(in.Definition), } } + +func (*RuleStore) mapToRuleInfo(in *ruleInfo) types.RuleInfoInternal { + return types.RuleInfoInternal{ + RuleInfo: types.RuleInfo{ + SpacePath: in.SpacePath, + RepoPath: in.RepoPath, + ID: in.ID, + UID: in.UID, + Type: in.Type, + State: in.State, + }, + Pattern: json.RawMessage(in.Pattern), + Definition: json.RawMessage(in.Definition), + } +} + +func (s *RuleStore) mapToRuleInfos( + ruleInfos []ruleInfo, +) []types.RuleInfoInternal { + res := make([]types.RuleInfoInternal, len(ruleInfos)) + for i := 0; i < len(ruleInfos); i++ { + res[i] = s.mapToRuleInfo(&ruleInfos[i]) + } + return res +} diff --git a/types/rule.go b/types/rule.go index 58cad077a..5f6099107 100644 --- a/types/rule.go +++ b/types/rule.go @@ -61,7 +61,7 @@ type Violation struct { // RuleViolations holds several violations of a rule. type RuleViolations struct { - Rule *Rule `json:"rule"` + Rule RuleInfo `json:"rule"` Violations []Violation `json:"violations"` } @@ -80,5 +80,22 @@ func (violations *RuleViolations) Addf(code, format string, params ...any) { } func (violations *RuleViolations) IsCritical() bool { - return violations.Rule == nil || violations.Rule.State == enum.RuleStateActive + return violations.Rule.State == enum.RuleStateActive +} + +// RuleInfo holds basic info about a rule that is used to describe the rule in RuleViolations. +type RuleInfo struct { + SpacePath string `json:"space_path,omitempty"` + RepoPath string `json:"repo_path,omitempty"` + + ID int64 `json:"-"` + UID string `json:"uid"` + Type RuleType `json:"type"` + State enum.RuleState `json:"state"` +} + +type RuleInfoInternal struct { + RuleInfo + Pattern json.RawMessage + Definition json.RawMessage }