From 45705ca27b309fdbddb8a6a017295cfbcc15b2cb Mon Sep 17 00:00:00 2001
From: Anders Eknert <anders@styra.com>
Date: Tue, 26 Mar 2024 14:56:33 +0100
Subject: [PATCH] Don't flag unresolved imports in `prefer-package-imports`
 (#607)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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>
---
 .../rules/imports/prefer_package_imports.rego | 10 +++++
 .../imports/prefer_package_imports_test.rego  | 45 ++++++++++++++++---
 pkg/linter/linter_test.go                     |  2 +-
 3 files changed, 50 insertions(+), 7 deletions(-)

diff --git a/bundle/regal/rules/imports/prefer_package_imports.rego b/bundle/regal/rules/imports/prefer_package_imports.rego
index 2caceb35..5edbfa9c 100644
--- a/bundle/regal/rules/imports/prefer_package_imports.rego
+++ b/bundle/regal/rules/imports/prefer_package_imports.rego
@@ -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 |
diff --git a/bundle/regal/rules/imports/prefer_package_imports_test.rego b/bundle/regal/rules/imports/prefer_package_imports_test.rego
index ec746e73..b15983b7 100644
--- a/bundle/regal/rules/imports/prefer_package_imports_test.rego
+++ b/bundle/regal/rules/imports/prefer_package_imports_test.rego
@@ -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"),
@@ -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"],
diff --git a/pkg/linter/linter_test.go b/pkg/linter/linter_test.go
index 591a119a..0c3f14d7 100644
--- a/pkg/linter/linter_test.go
+++ b/pkg/linter/linter_test.go
@@ -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]