Skip to content

Commit

Permalink
Share lint query
Browse files Browse the repository at this point in the history 
This allows aggregating linting and standard linting to use the same
prepared query. This saves around 150ms on lint runs on my laptop.

Signed-off-by: Charlie Egan <[email protected]>
  • Loading branch information
@charlieegan3
charlieegan3 committed Oct 31, 2024
1 parent aeec96b commit b6ebd65
Show file tree
Hide file tree
Showing 4 changed files with 122 additions and 90 deletions.
28 changes: 19 additions & 9 deletions bundle/regal/main/main.rego
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,23 +16,33 @@ import data.regal.util

# METADATA
# description: set of all notices returned from linter rules
lint.notices := _notices
lint.notices := _notices if {
input.regal.operations.lint
}

# METADATA
# description: map of all aggregated data from aggregate rules, keyed by category/title
lint.aggregates := aggregate
# description: map of all ignore directives encountered when linting
lint.ignore_directives[input.regal.file.name] := ast.ignore_directives if {
input.regal.operations.lint
}

# METADATA
# description: map of all ignore directives encountered when linting
lint.ignore_directives[input.regal.file.name] := ast.ignore_directives
# description: all violations from non-aggregate rules
lint.violations := report if {
input.regal.operations.lint
}

# METADATA
# description: all violations from aggregate rules
lint_aggregate.violations := aggregate_report
# description: map of all aggregated data from aggregate rules, keyed by category/title
lint.aggregates := aggregate if {
input.regal.operations.collect
}

# METADATA
# description: all violations from non-aggregate rules
lint.violations := report
# description: all violations from aggregate rules
lint.aggregate.violations := aggregate_report if {
input.regal.operations.aggregate
}

_rules_to_run[category] contains title if {
some category, title
Expand Down
68 changes: 39 additions & 29 deletions bundle/regal/main/main_test.rego
View file
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,10 @@ test_ignore_directive_collected_in_aggregate_rule if {
# regal ignore:unresolved-import
import data.unresolved
`)
lint := main.lint with input as module

mock_input := object.union(module, {"regal": {"operations": {"lint": true}}})

lint := main.lint with input as mock_input

lint.ignore_directives == {"p.rego": {6: ["unresolved-import"]}}
}
Expand Down Expand Up @@ -294,7 +297,11 @@ test_camelcase if {
camelCase == "yes"
}
`
result := main with input as regal.parse_module("p.rego", policy)

module := regal.parse_module("p.rego", policy)
mock_input := object.union(module, {"regal": {"operations": {"lint": true}}})

result := main with input as mock_input
with input.regal.file.name as "stdin"
with config.merged_config as {
"capabilities": {},
Expand All @@ -318,35 +325,34 @@ test_main_lint if {

module := regal.parse_module("p.rego", policy)

cfg := {"rules": {"style": {"use-assignment-operator": {"level": "error"}}}}
mock_input := object.union(module, {"regal": {"operations": {"lint": true}}})

result := main.lint with input as module with config.merged_config as cfg
cfg := {"rules": {"style": {"use-assignment-operator": {"level": "error"}}}}

result == {
"aggregates": {},
"ignore_directives": {"p.rego": {}},
"notices": set(),
"violations": {{
"category": "style",
"description": "Prefer := over = for assignment",
"level": "error",
"location": {
"col": 4,
"file": "p.rego",
result := main.lint with input as mock_input with config.merged_config as cfg

result.violations == {{
"category": "style",
"description": "Prefer := over = for assignment",
"level": "error",
"location": {
"col": 4,
"file": "p.rego",
"row": 2,
"end": {
"col": 5,
"row": 2,
"end": {
"col": 5,
"row": 2,
},
"text": "\tx = 1",
},
"related_resources": [{
"description": "documentation",
"ref": "https://docs.styra.com/regal/rules/style/use-assignment-operator",
}],
"title": "use-assignment-operator",
}},
}
"text": "\tx = 1",
},
"related_resources": [{
"description": "documentation",
"ref": "https://docs.styra.com/regal/rules/style/use-assignment-operator",
}],
"title": "use-assignment-operator",
}}
result.ignore_directives == {"p.rego": {}}
result.notices == set()
}

test_rules_to_run_not_excluded if {
Expand All @@ -373,6 +379,7 @@ test_notices if {
# regal ignore:leaked-internal-reference
notices := main.lint.notices with main._rules_to_run as {"idiomatic": {"testme"}}
with data.regal.rules.idiomatic.testme.notices as {notice}
with input.regal.operations.lint as true

notices == {notice}
}
Expand Down Expand Up @@ -416,7 +423,10 @@ test_aggregate_custom_rule if {
test_aggregate_report_custom_rule if {
mock_input := {
"aggregates_internal": {"custom/test": {}},
"regal": {"file": {"name": "p.rego"}},
"regal": {
"file": {"name": "p.rego"},
"operations": {"aggregate": true},
},
}

mock_rules := {"custom": {"test": {"aggregate_report": {{
Expand All @@ -429,7 +439,7 @@ test_aggregate_report_custom_rule if {

report == {{"category": "custom", "title": "test"}}

violations := main.lint_aggregate.violations with input as mock_input
violations := main.lint.aggregate.violations with input as mock_input
with data.custom.regal.rules as mock_rules

violations == report
Expand Down
17 changes: 16 additions & 1 deletion internal/embeds/schemas/regal-ast.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -381,6 +381,20 @@
},
"type": "object"
},
"operations": {
"type": "object",
"properties": {
"lint": {
"type": "boolean"
},
"collect": {
"type": "boolean"
},
"aggregate": {
"type": "boolean"
}
}
},
"context": {
"description": "extra attributes provided in the specific evaluation context",
"type": "object",
Expand Down Expand Up @@ -408,7 +422,8 @@
},
"type": "object",
"required": [
"file"
"file",
"operations"
]
}
}
Expand Down
99 changes: 48 additions & 51 deletions pkg/linter/linter.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,16 +63,7 @@ type Linter struct {
}

//nolint:gochecknoglobals
var (
// Single file provided as input.
lintQuery = ast.MustParseBody(`lint := {
"violations": data.regal.main.lint.violations,
"notices": data.regal.main.lint.notices,
}`)
// More than one file provided as input.
lintAndCollectQuery = ast.MustParseBody("lint := data.regal.main.lint")
lintWithAggregatesQuery = ast.MustParseBody("lint_aggregate := data.regal.main.lint_aggregate")
)
var lintQuery = ast.MustParseBody("lint := data.regal.main.lint")

// NewLinter creates a new Regal linter.
func NewLinter() Linter {
Expand Down Expand Up @@ -326,7 +317,17 @@ func (l Linter) Lint(ctx context.Context) (report.Report, error) {

finalReport.Violations = append(finalReport.Violations, goReport.Violations...)

regoReport, err := l.lintWithRegoRules(ctx, input)
regoArgs, err := l.prepareRegoArgs(lintQuery)
if err != nil {
return report.Report{}, fmt.Errorf("failed preparing query for linting: %w", err)
}

pq, err := rego.New(regoArgs...).PrepareForEval(ctx)
if err != nil {
return report.Report{}, fmt.Errorf("failed preparing query for linting: %w", err)
}

regoReport, err := l.lintWithRegoRules(ctx, &pq, input)
if err != nil {
return report.Report{}, fmt.Errorf("failed to lint using Rego rules: %w", err)
}
Expand Down Expand Up @@ -362,7 +363,7 @@ func (l Linter) Lint(ctx context.Context) (report.Report, error) {
}

if len(allAggregates) > 0 {
aggregateReport, err := l.lintWithRegoAggregateRules(ctx, allAggregates, regoReport.IgnoreDirectives)
aggregateReport, err := l.lintWithRegoAggregateRules(ctx, &pq, allAggregates, regoReport.IgnoreDirectives)
if err != nil {
return report.Report{}, fmt.Errorf("failed to lint using Rego aggregate rules: %w", err)
}
Expand Down Expand Up @@ -795,34 +796,26 @@ func loadModulesFromCustomRuleFS(customRuleFS fs.FS, rootPath string) (map[strin
return files, nil
}

func (l Linter) lintWithRegoRules(ctx context.Context, input rules.Input) (report.Report, error) {
func (l Linter) lintWithRegoRules(
ctx context.Context,
pq *rego.PreparedEvalQuery,
input rules.Input,
) (report.Report, error) {
l.startTimer(regalmetrics.RegalLintRego)
defer l.stopTimer(regalmetrics.RegalLintRego)

ctx, cancel := context.WithCancel(ctx)
defer cancel()

var query ast.Body
if len(input.FileNames) > 1 || l.useCollectQuery {
query = lintAndCollectQuery
} else {
query = lintQuery
}

regoArgs, err := l.prepareRegoArgs(query)
if err != nil {
return report.Report{}, fmt.Errorf("failed preparing query for linting: %w", err)
}

pq, err := rego.New(regoArgs...).PrepareForEval(ctx)
if err != nil {
return report.Report{}, fmt.Errorf("failed preparing query for linting: %w", err)
}

regoReport := report.Report{}
regoReport.Aggregates = make(map[string][]report.Aggregate)
regoReport.IgnoreDirectives = make(map[string]map[string][]string)

var operationCollect bool
if len(input.FileNames) > 1 || l.useCollectQuery {
operationCollect = true
}

var wg sync.WaitGroup

var mu sync.Mutex
Expand All @@ -846,6 +839,14 @@ func (l Linter) lintWithRegoRules(ctx context.Context, input rules.Input) (repor
return
}

regalInput, ok := enhancedAST["regal"].(map[string]any)
if ok {
regalInput["operations"] = map[string]bool{
"lint": true,
"collect": operationCollect,
}
}

evalArgs := []rego.EvalOption{
rego.EvalInput(enhancedAST),
}
Expand All @@ -867,7 +868,7 @@ func (l Linter) lintWithRegoRules(ctx context.Context, input rules.Input) (repor
return
}

result, err := resultSetToReport(resultSet)
result, err := resultSetToReport(resultSet, false)
if err != nil {
errCh <- fmt.Errorf("failed to convert result set to report: %w", err)

Expand Down Expand Up @@ -923,6 +924,7 @@ func (l Linter) lintWithRegoRules(ctx context.Context, input rules.Input) (repor

func (l Linter) lintWithRegoAggregateRules(
ctx context.Context,
pq *rego.PreparedEvalQuery,
aggregates map[string][]report.Aggregate,
ignoreDirectives map[string]map[string][]string,
) (report.Report, error) {
Expand All @@ -932,16 +934,6 @@ func (l Linter) lintWithRegoAggregateRules(
ctx, cancel := context.WithCancel(ctx)
defer cancel()

regoArgs, err := l.prepareRegoArgs(lintWithAggregatesQuery)
if err != nil {
return report.Report{}, fmt.Errorf("failed preparing query for linting: %w", err)
}

pq, err := rego.New(regoArgs...).PrepareForEval(ctx)
if err != nil {
return report.Report{}, fmt.Errorf("failed preparing query for linting: %w", err)
}

input := map[string]any{
// This will be replaced by the routing policy to provide each
// aggregate rule only the aggregated data from the same rule
Expand All @@ -951,6 +943,7 @@ func (l Linter) lintWithRegoAggregateRules(
// refer to input.regal in an aggregate_report rule
"ignore_directives": ignoreDirectives,
"regal": map[string]any{
"operations": map[string]bool{"aggregate": true},
"file": map[string]any{
"name": "__aggregate_report__",
"lines": []string{},
Expand All @@ -969,7 +962,7 @@ func (l Linter) lintWithRegoAggregateRules(
return report.Report{}, fmt.Errorf("error encountered in query evaluation %w", err)
}

result, err := resultSetToReport(resultSet)
result, err := resultSetToReport(resultSet, true)
if err != nil {
return report.Report{}, fmt.Errorf("failed to convert result set to report: %w", err)
}
Expand All @@ -981,22 +974,26 @@ func (l Linter) lintWithRegoAggregateRules(
return result, nil
}

func resultSetToReport(resultSet rego.ResultSet) (report.Report, error) {
func resultSetToReport(resultSet rego.ResultSet, aggregate bool) (report.Report, error) {
if len(resultSet) != 1 {
return report.Report{}, fmt.Errorf("expected 1 item in resultset, got %d", len(resultSet))
}

r := report.Report{}

if binding, ok := resultSet[0].Bindings["lint"]; ok {
if err := rio.JSONRoundTrip(binding, &r); err != nil {
return report.Report{}, fmt.Errorf("JSON rountrip failed for bindings: %v %w", binding, err)
if aggregate {
if binding, ok := resultSet[0].Bindings["lint"].(map[string]any); ok {
if aggregateBinding, ok := binding["aggregate"]; ok {
if err := rio.JSONRoundTrip(aggregateBinding, &r); err != nil {
return report.Report{}, fmt.Errorf("JSON rountrip failed for bindings: %v %w", binding, err)
}
}
}
}

if binding, ok := resultSet[0].Bindings["lint_aggregate"]; ok {
if err := rio.JSONRoundTrip(binding, &r); err != nil {
return report.Report{}, fmt.Errorf("JSON rountrip failed for bindings: %v %w", binding, err)
} else {
if binding, ok := resultSet[0].Bindings["lint"]; ok {
if err := rio.JSONRoundTrip(binding, &r); err != nil {
return report.Report{}, fmt.Errorf("JSON rountrip failed for bindings: %v %w", binding, err)
}
}
}

Expand Down

0 comments on commit b6ebd65

Please sign in to comment.