Skip to content

Commit

Permalink
Don't flag unresolved imports in prefer-package-imports (#607)
Browse files Browse the repository at this point in the history
This fixes one issue reported in #605, but not the issue itself, as
that'll need more consideration — concerns mostly described in #344

Signed-off-by: Anders Eknert <anders@styra.com>
  • Loading branch information
anderseknert committed Mar 26, 2024
1 parent 9a670bf commit 45705ca
Show file tree
Hide file tree
Showing 3 changed files with 50 additions and 7 deletions.
10 changes: 10 additions & 0 deletions bundle/regal/rules/imports/prefer_package_imports.rego
Original file line number Diff line number Diff line change
Expand Up @@ -54,12 +54,22 @@ aggregate_report contains violation if {
}

some imp in all_imports
resolves(imp.path, all_package_paths)
not imp.path in all_package_paths
not imp.path in ignored_import_paths

violation := result.fail(rego.metadata.chain(), {"location": imp.location})
}

# METADATA
# description: |
# returns true if the path "resolves" to *any*
# package part of the same length as the path
resolves(path, pkg_paths) if count([path |
some pkg_path in pkg_paths
pkg_path == array.slice(path, 0, count(pkg_path))
]) > 0

ignored_import_paths contains path if {
some item in cfg["ignore-import-paths"]
path := [part |
Expand Down
45 changes: 39 additions & 6 deletions bundle/regal/rules/imports/prefer_package_imports_test.rego
Original file line number Diff line number Diff line change
Expand Up @@ -27,17 +27,30 @@ test_aggregate_collects_imports_with_location if {
}}
}

test_fail_aggregate_report_on_import_without_matching_package if {
r := rule.aggregate_report with input.aggregate as {{"aggregate_data": {
"package_path": ["a"],
"imports": [{"path": ["b"], "location": {"col": 1, "file": "policy.rego", "row": 3, "text": "import data.b"}}],
}}}
test_fail_aggregate_report_on_imported_rule if {
r := rule.aggregate_report with input.aggregate as {
{"aggregate_data": {
"package_path": ["a"],
"imports": [
{
# likely import of rule — should fail
"path": ["b", "c"],
"location": {"col": 1, "file": "policy.rego", "row": 3, "text": "import data.b.c"},
},
# import of package, should not fail
{"path": ["b"], "location": {"col": 1, "file": "policy.rego", "row": 4, "text": "import data.b"}},
# unresolved import, should not fail
{"path": ["c"], "location": {"col": 1, "file": "policy.rego", "row": 5, "text": "import data.c"}},
],
}},
{"aggregate_data": {"package_path": ["b"], "imports": []}},
}

r == {{
"category": "imports",
"description": "Prefer importing packages over rules",
"level": "error",
"location": {"col": 1, "file": "policy.rego", "row": 3, "text": "import data.b"},
"location": {"col": 1, "file": "policy.rego", "row": 3, "text": "import data.b.c"},
"related_resources": [{
"description": "documentation",
"ref": config.docs.resolve_url("$baseUrl/$category/prefer-package-imports", "imports"),
Expand All @@ -64,6 +77,26 @@ test_success_aggregate_report_on_import_with_matching_package if {
r == set()
}

# unresolved imports should be flagged by an `unresoled-import`
# rule instead. see https://github.com/StyraInc/regal/issues/300
test_success_aggregate_report_on_import_with_unresolved_path if {
r := rule.aggregate_report with input.aggregate as {
{"aggregate_data": {
"package_path": ["a"],
"imports": [{
"path": ["foo"],
"location": {"col": 1, "file": "policy.rego", "row": 3, "text": "import data.b"},
}],
}},
{"aggregate_data": {
"package_path": ["bar"],
"imports": [],
}},
}

r == set()
}

test_success_aggregate_report_ignored_import_path if {
aggregate := {{"aggregate_data": {
"package_path": ["a"],
Expand Down
2 changes: 1 addition & 1 deletion pkg/linter/linter_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -527,7 +527,7 @@ func TestLintWithAggregateRule(t *testing.T) {
result := testutil.Must(linter.Lint(context.Background()))(t)

if len(result.Violations) != 1 {
t.Fatalf("expected no violation, got %d", len(result.Violations))
t.Fatalf("expected one violation, got %d", len(result.Violations))
}

violation := result.Violations[0]
Expand Down

0 comments on commit 45705ca

Please sign in to comment.