test(core): guard the framework against plugin imports

backend/core/layering_test.go

@@ -0,0 +1,153 @@
+/*
+Licensed to the Apache Software Foundation (ASF) under one or more
+contributor license agreements.  See the NOTICE file distributed with
+this work for additional information regarding copyright ownership.
+The ASF licenses this file to You 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.
+*/
+
+package core
+
+import (
+	"errors"
+	"io/fs"
+	"os"
+	"os/exec"
+	"path/filepath"
+	"sort"
+	"strings"
+	"testing"
+)
+
+const (
+	modulePrefix = "github.com/apache/devlake/"
+	pluginPrefix = modulePrefix + "plugins/"
+)
+
+// frameworkRoots are the packages that must stay plugin-agnostic.
+//
+// The dependency direction is: plugins depend on core and helpers; core,
+// helpers and server never depend on a concrete plugin. Plugins reach the
+// framework through the interfaces in core/plugin, and the framework reaches
+// plugins only through those same interfaces, resolved by name at runtime.
+//
+// server/ is in the list because it is the application, not the framework: it
+// may link the generated registry (see allowedEdges) but must never reach for an
+// individual plugin.
+//
+// The rule matters more now that plugins are compiled into the server binary by
+// default: one framework→plugin import links that plugin, and everything it
+// imports, into the core payload, defeats subset builds, and creates an import
+// cycle for any plugin that needs the framework package doing the importing.
+var frameworkRoots = []string{
+	modulePrefix + "core/...",
+	modulePrefix + "helpers/...",
+	modulePrefix + "server/...",
+}
+
+// allowedEdges holds the only framework→plugin edges that are tolerated,
+// keyed by the exact importing package. Every entry is a layering exception
+// that has to be argued for; keep this map as close to empty as possible.
+var allowedEdges = map[string]string{
+	// The composition root links the generated plugin registry, which
+	// blank-imports every plugin. It is the one place that is allowed to know
+	// that plugins exist as concrete packages. It must never import a single
+	// plugin directly, and no other package under server/ gets the same
+	// privilege.
+	modulePrefix + "server": modulePrefix + "plugins/all",
+}
+
+// TestFrameworkDoesNotImportPlugins fails when anything under core/, helpers/ or
+// server/ imports a plugin package, including from _test.go files.
+//
+// Tests count because a test that needs a plugin is a design smell: it usually
+// means the framework helper under test is only exercised through one concrete
+// plugin's fixtures. Move the test next to that plugin instead, or give the
+// helper a plugin-independent fixture.
+func TestFrameworkDoesNotImportPlugins(t *testing.T) {
+	args := []string{
+		"list",
+		"-f", `{{.ImportPath}}|{{join .Imports " "}}|{{join .TestImports " "}}|{{join .XTestImports " "}}`,
+	}
+	args = append(args, frameworkRoots...)
+
+	out, err := exec.Command("go", args...).Output()
+	if err != nil {
+		var exitErr *exec.ExitError
+		if errors.As(err, &exitErr) {
+			t.Fatalf("go list failed: %v\n%s", err, exitErr.Stderr)
+		}
+		t.Fatalf("go list failed: %v", err)
+	}
+
+	seen := map[string]struct{}{}
+	for _, line := range strings.Split(strings.TrimSpace(string(out)), "\n") {
+		fields := strings.Split(line, "|")
+		if len(fields) != 4 {
+			t.Fatalf("unexpected go list output line %q", line)
+		}
+		importer := fields[0]
+		for _, group := range fields[1:] {
+			for _, imported := range strings.Fields(group) {
+				if !strings.HasPrefix(imported, pluginPrefix) {
+					continue
+				}
+				if allowedEdges[importer] == imported {
+					continue
+				}
+				seen[importer+" imports "+imported] = struct{}{}
+			}
+		}
+	}
+
+	if len(seen) == 0 {
+		trackFrameworkSources(t)
+		return
+	}
+
+	violations := make([]string, 0, len(seen))
+	for v := range seen {
+		violations = append(violations, v)
+	}
+	sort.Strings(violations)
+
+	t.Fatalf("the framework must not import plugins, found %d edge(s):\n\t%s",
+		len(violations), strings.Join(violations, "\n\t"))
+}
+
+// trackFrameworkSources registers every framework source file with the test log.
+//
+// The test cache only knows about the package under test, not about the other
+// packages this test inspects through `go list`. Without this, a plain
+// `go test ./core` would replay a cached PASS after someone adds a plugin
+// import in helpers/ or server/. Opening the files makes them part of the cache
+// entry, so any change to them invalidates it.
+func trackFrameworkSources(t *testing.T) {
+	t.Helper()
+
+	out, err := exec.Command("go", "env", "GOMOD").Output()
+	if err != nil {
+		return
+	}
+	moduleDir := filepath.Dir(strings.TrimSpace(string(out)))
+
+	for _, dir := range []string{"core", "helpers", "server"} {
+		_ = filepath.WalkDir(filepath.Join(moduleDir, dir), func(path string, d fs.DirEntry, err error) error {
+			if err != nil || d.IsDir() || !strings.HasSuffix(path, ".go") {
+				return nil
+			}
+			// Best effort: the cache entry only needs the file to be opened.
+			_, _ = os.ReadFile(path)
+			return nil
+		})
+	}
+}

backend/helpers/e2ehelper/data_flow_tester_test.go

@@ -19,10 +19,9 @@ package e2ehelper
 
 import (
 	"github.com/apache/devlake/core/dal"
+	"github.com/apache/devlake/core/errors"
 	"github.com/apache/devlake/core/models/common"
 	plugin "github.com/apache/devlake/core/plugin"
-	gitlabModels "github.com/apache/devlake/plugins/gitlab/models"
-	"github.com/apache/devlake/plugins/gitlab/tasks"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -39,29 +38,48 @@ func (t TestModel) TableName() string {
 	return "_tool_test_model"
 }
 
+type TestOptions struct {
+	ProjectId int
+}
+
+type TestTaskData struct {
+	Options *TestOptions
+}
+
+// testExtractIssuesMeta stands in for a real plugin's subtask so this example
+// stays self-contained: helpers must not import a concrete plugin.
+var testExtractIssuesMeta = plugin.SubTaskMeta{
+	Name: "extractIssues",
+	EntryPoint: func(taskCtx plugin.SubTaskContext) errors.Error {
+		return nil
+	},
+}
+
 func ExampleDataFlowTester() {
 	var t *testing.T // stub
 
-	var gitlab plugin.PluginMeta
-	dataflowTester := NewDataFlowTester(t, "gitlab", gitlab)
+	var pluginMeta plugin.PluginMeta
+	dataflowTester := NewDataFlowTester(t, "test_plugin", pluginMeta)
 
-	taskData := &tasks.GitlabTaskData{
-		Options: &tasks.GitlabOptions{
+	taskData := &TestTaskData{
+		Options: &TestOptions{
 			ProjectId: 666888,
 		},
 	}
 
 	// import raw data table
-	dataflowTester.ImportCsvIntoRawTable("./tables/_raw_gitlab_api_issues.csv", "_raw_gitlab_api_issues")
+	dataflowTester.ImportCsvIntoRawTable("./tables/_raw_test_plugin_api_issues.csv", "_raw_test_plugin_api_issues")
 
 	// verify extraction
-	dataflowTester.FlushTabler(gitlabModels.GitlabProject{})
-	dataflowTester.Subtask(tasks.ExtractApiIssuesMeta, taskData)
+	dataflowTester.FlushTabler(&TestModel{})
+	dataflowTester.Subtask(testExtractIssuesMeta, taskData)
 	dataflowTester.VerifyTable(
-		gitlabModels.GitlabIssue{},
-		"tables/_tool_gitlab_issues.csv",
+		&TestModel{},
+		"tables/_tool_test_model.csv",
 		[]string{
-			"gitlab_id",
+			"connection_id",
+			"issue_id",
+			"label_name",
 			"_raw_data_params",
 			"_raw_data_table",
 			"_raw_data_id",
@@ -108,10 +126,14 @@ func TestGetTableMetaData(t *testing.T) {
 		fields, err := dal.GetPrimarykeyColumnNames(dataflowTester.Dal, &TestModel{})
 		assert.Equal(t, err, nil)
 		assert.Equal(t, 3, len(fields))
+		// GetPrimarykeyColumnNames qualifies each column with the table name, so
+		// the result can be used as-is in join conditions whose column names
+		// collide with reserved identifiers. Callers that want the bare column
+		// name strip the prefix themselves.
 		for _, e := range []string{
-			"connection_id",
-			"issue_id",
-			"label_name",
+			"_tool_test_model.connection_id",
+			"_tool_test_model.issue_id",
+			"_tool_test_model.label_name",
 		} {
 			assert.Contains(t, fields, e)
 		}

backend/server/services/blueprint_makeplan_v200_test.go

@@ -27,7 +27,6 @@ import (
 	"github.com/apache/devlake/core/models/domainlayer/ticket"
 	"github.com/apache/devlake/core/plugin"
 	mockplugin "github.com/apache/devlake/mocks/core/plugin"
-	"github.com/apache/devlake/plugins/org/tasks"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/mock"
 )
@@ -71,9 +70,17 @@ func TestMakePlanV200(t *testing.T) {
 
 	// mock org plugin
 	org := new(mockplugin.CompositeProjectMapper)
+	// The org plugin normally builds a tasks.ProjectMapping here. This test
+	// mocks the plugin, so the value only has to round-trip through the plan:
+	// keep it local rather than importing plugins/org/tasks, which would make
+	// the framework depend on a plugin.
+	projectMapping := map[string]interface{}{
+		"projectName": projectName,
+		"scopes":      []interface{}{},
+	}
 	orgPlan := coreModels.PipelinePlan{
 		{
-			{Plugin: "org", Subtasks: []string{"setProjectMapping"}, Options: map[string]interface{}{"projectMappings": []interface{}{tasks.NewProjectMapping(projectName, githubOutputScopes)}}},
+			{Plugin: "org", Subtasks: []string{"setProjectMapping"}, Options: map[string]interface{}{"projectMappings": []interface{}{projectMapping}}},
 		},
 	}
 	org.On("MapProject", projectName, githubOutputScopes).Return(orgPlan, nil)