fix(project): validate metric plugin options when saving a project

backend/plugins/monorepo/impl/impl.go

@@ -144,7 +144,7 @@ func (p Monorepo) MakeMetricPluginPipelinePlanV200(projectName string, options j
 		return nil, errors.BadInput.New(
 			"the monorepo plugin requires a subProjects list in its metric plugin options")
 	}
-	// Validate eagerly so a bad regex is reported when the blueprint is saved rather
+	// Validate eagerly so a bad regex is reported when the project or blueprint is saved rather
 	// than midway through a pipeline run.
 	if _, err := tasks.NewSubProjectMatcher(op.SubProjects); err != nil {
 		return nil, err

backend/server/services/blueprint_makeplan_v200.go

@@ -119,25 +119,11 @@ func GeneratePlanJsonV200(
 	metricPlans := make([]coreModels.PipelinePlan, len(metrics))
 	i := 0
 	for metricPluginName, metricPluginOptJson := range metrics {
-		p, err := plugin.GetPlugin(metricPluginName)
+		metricPlans[i], err = makeMetricPluginPlan(projectName, metricPluginName, metricPluginOptJson)
 		if err != nil {
 			return nil, err
 		}
-		if pluginBp, ok := p.(plugin.MetricPluginBlueprintV200); ok {
-			// If we enable one metric plugin, even if it has nil option, we still process it
-			if len(metricPluginOptJson) == 0 {
-				metricPluginOptJson = json.RawMessage("{}")
-			}
-			metricPlans[i], err = pluginBp.MakeMetricPluginPipelinePlanV200(projectName, metricPluginOptJson)
-			if err != nil {
-				return nil, err
-			}
-			i++
-		} else {
-			return nil, errors.Default.New(
-				fmt.Sprintf("plugin %s does not support MetricPluginBlueprintV200", metricPluginName),
-			)
-		}
+		i++
 	}
 	var planForProjectMapping coreModels.PipelinePlan
 	if projectName != "" {
@@ -160,6 +146,25 @@ func GeneratePlanJsonV200(
 	return plan, err
 }
 
+// makeMetricPluginPlan generates the pipeline plan of a single metric plugin.
+func makeMetricPluginPlan(projectName, pluginName string, option json.RawMessage) (coreModels.PipelinePlan, errors.Error) {
+	p, err := plugin.GetPlugin(pluginName)
+	if err != nil {
+		return nil, err
+	}
+	pluginBp, ok := p.(plugin.MetricPluginBlueprintV200)
+	if !ok {
+		return nil, errors.Default.New(
+			fmt.Sprintf("plugin %s does not support MetricPluginBlueprintV200", pluginName),
+		)
+	}
+	// If we enable one metric plugin, even if it has nil option, we still process it
+	if len(option) == 0 {
+		option = json.RawMessage("{}")
+	}
+	return pluginBp.MakeMetricPluginPipelinePlanV200(projectName, option)
+}
+
 func removeCollectorTasks(plan coreModels.PipelinePlan) coreModels.PipelinePlan {
 	for j, stage := range plan {
 		for k, task := range stage {

backend/server/services/project.go

@@ -106,6 +106,9 @@ func CreateProject(projectInput *models.ApiInputProject) (*models.ApiOutputProje
 	if err := VerifyStruct(projectInput); err != nil {
 		return nil, err
 	}
+	if err := validateProjectMetrics(projectInput.Name, projectInput.Metrics); err != nil {
+		return nil, err
+	}
 
 	// create transaction to update multiple tables
 	var err errors.Error
@@ -203,6 +206,15 @@ func PatchProject(name string, body map[string]interface{}) (*models.ApiOutputPr
 		return nil, err
 	}
 
+	// allowed to changed the name
+	if projectInput.Name == "" {
+		projectInput.Name = name
+	}
+	err = validateProjectMetrics(projectInput.Name, projectInput.Metrics)
+	if err != nil {
+		return nil, err
+	}
+
 	// wrap all operation inside a transaction
 	tx := db.Begin()
 	defer func() {
@@ -219,10 +231,6 @@ func PatchProject(name string, body map[string]interface{}) (*models.ApiOutputPr
 		return nil, err
 	}
 
-	// allowed to changed the name
-	if projectInput.Name == "" {
-		projectInput.Name = name
-	}
 	project.BaseProject = projectInput.BaseProject
 
 	// name changed, updates the related entities as well
@@ -449,6 +457,21 @@ func getProjectByName(tx dal.Dal, name string, additionalClauses ...dal.Clause)
 	return project, nil
 }
 
+// validateProjectMetrics dry-runs the plan generation of every enabled metric so that invalid
+// plugin options are rejected when the project is saved rather than when its blueprint runs.
+// The generated plans are discarded, which relies on MakeMetricPluginPipelinePlanV200 having no side effects.
+func validateProjectMetrics(projectName string, metrics []*models.BaseMetric) errors.Error {
+	for _, metric := range metrics {
+		if metric == nil || !metric.Enable {
+			continue
+		}
+		if _, err := makeMetricPluginPlan(projectName, metric.PluginName, metric.PluginOption); err != nil {
+			return errors.BadInput.Wrap(err, fmt.Sprintf("invalid options for metric plugin %s", metric.PluginName))
+		}
+	}
+	return nil
+}
+
 func refreshProjectMetrics(tx dal.Transaction, projectInput *models.ApiInputProject) errors.Error {
 	err := tx.Delete(&models.ProjectMetricSetting{}, dal.Where("project_name = ?", projectInput.Name))
 	if err != nil {

backend/server/services/project_metric_validation_test.go

@@ -0,0 +1,191 @@
+/*
+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 services
+
+import (
+	"encoding/json"
+	"testing"
+
+	"github.com/apache/devlake/core/errors"
+	"github.com/apache/devlake/core/models"
+	mockdal "github.com/apache/devlake/mocks/core/dal"
+	mockplugin "github.com/apache/devlake/mocks/core/plugin"
+	"github.com/go-playground/validator/v10"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// registerTestMetricPlugin registers a metric plugin mock. Calls without a matching expectation
+// fail the test, so a mock without expectations proves the plugin is never invoked.
+func registerTestMetricPlugin(t *testing.T, name string) *mockplugin.CompositeMetricPluginBlueprintV200 {
+	t.Helper()
+	metricPlugin := mockplugin.NewCompositeMetricPluginBlueprintV200(t)
+	metricPlugin.On("Name").Return(name).Maybe()
+	registerProjectDeleteTestPlugin(t, metricPlugin)
+	return metricPlugin
+}
+
+func TestValidateProjectMetrics(t *testing.T) {
+	t.Run("passes enabled metrics with valid options", func(t *testing.T) {
+		metricPlugin := registerTestMetricPlugin(t, "test-metric-valid")
+		option := json.RawMessage(`{"pattern":"^feat/"}`)
+		metricPlugin.On("MakeMetricPluginPipelinePlanV200", "test-project", option).
+			Return(models.PipelinePlan{}, nil).Once()
+
+		err := validateProjectMetrics("test-project", []*models.BaseMetric{
+			{PluginName: "test-metric-valid", PluginOption: option, Enable: true},
+		})
+
+		assert.NoError(t, err)
+	})
+
+	t.Run("treats empty options as an empty object", func(t *testing.T) {
+		metricPlugin := registerTestMetricPlugin(t, "test-metric-empty")
+		metricPlugin.On("MakeMetricPluginPipelinePlanV200", "test-project", json.RawMessage("{}")).
+			Return(models.PipelinePlan{}, nil).Once()
+
+		err := validateProjectMetrics("test-project", []*models.BaseMetric{
+			{PluginName: "test-metric-empty", Enable: true},
+		})
+
+		assert.NoError(t, err)
+	})
+
+	t.Run("rejects invalid options as bad input", func(t *testing.T) {
+		metricPlugin := registerTestMetricPlugin(t, "test-metric-invalid")
+		pluginErr := errors.Default.New("invalid somePattern")
+		metricPlugin.On("MakeMetricPluginPipelinePlanV200", "test-project", json.RawMessage(`{}`)).
+			Return(nil, pluginErr).Once()
+
+		err := validateProjectMetrics("test-project", []*models.BaseMetric{
+			{PluginName: "test-metric-invalid", PluginOption: json.RawMessage(`{}`), Enable: true},
+		})
+
+		require.Error(t, err)
+		assert.Equal(t, errors.BadInput, err.GetType())
+		assert.ErrorIs(t, err, pluginErr)
+		assert.Contains(t, err.Error(), "invalid options for metric plugin test-metric-invalid")
+	})
+
+	t.Run("skips disabled and nil metrics", func(t *testing.T) {
+		registerTestMetricPlugin(t, "test-metric-disabled")
+
+		err := validateProjectMetrics("test-project", []*models.BaseMetric{
+			nil,
+			{PluginName: "test-metric-disabled", PluginOption: json.RawMessage(`{"bad"`), Enable: false},
+			{PluginName: "test-metric-not-registered", Enable: false},
+		})
+
+		assert.NoError(t, err)
+	})
+
+	t.Run("rejects enabled metrics whose plugin is not registered", func(t *testing.T) {
+		err := validateProjectMetrics("test-project", []*models.BaseMetric{
+			{PluginName: "test-metric-not-registered", Enable: true},
+		})
+
+		require.Error(t, err)
+		assert.Equal(t, errors.BadInput, err.GetType())
+		// GetPlugin's message depends on the global registry state, so only the wrapper is asserted
+		assert.Contains(t, err.Error(), "invalid options for metric plugin test-metric-not-registered")
+	})
+
+	t.Run("rejects enabled metrics whose plugin is not a metric plugin", func(t *testing.T) {
+		registerProjectDeleteTestPlugin(t, &testOrdinaryPlugin{name: "test-metric-ordinary"})
+
+		err := validateProjectMetrics("test-project", []*models.BaseMetric{
+			{PluginName: "test-metric-ordinary", Enable: true},
+		})
+
+		require.Error(t, err)
+		assert.Equal(t, errors.BadInput, err.GetType())
+		assert.Contains(t, err.Error(), "does not support MetricPluginBlueprintV200")
+	})
+}
+
+func withProjectTestValidator(t *testing.T) {
+	t.Helper()
+	previousVld := vld
+	vld = validator.New()
+	t.Cleanup(func() {
+		vld = previousVld
+	})
+}
+
+func TestCreateProject_RejectsInvalidMetricOptionsBeforeWriting(t *testing.T) {
+	withProjectTestValidator(t)
+	// no expectations: any database call fails the test
+	withProjectTestDatabase(t, mockdal.NewDal(t))
+	metricPlugin := registerTestMetricPlugin(t, "test-metric-create")
+	pluginErr := errors.BadInput.New("invalid somePattern")
+	metricPlugin.On("MakeMetricPluginPipelinePlanV200", "new-project", json.RawMessage(`{"somePattern":"("}`)).
+		Return(nil, pluginErr).Once()
+
+	output, err := CreateProject(&models.ApiInputProject{
+		BaseProject: models.BaseProject{Name: "new-project"},
+		Metrics: []*models.BaseMetric{
+			{PluginName: "test-metric-create", PluginOption: json.RawMessage(`{"somePattern":"("}`), Enable: true},
+		},
+	})
+
+	assert.Nil(t, output)
+	require.Error(t, err)
+	assert.Equal(t, errors.BadInput, err.GetType())
+	assert.ErrorIs(t, err, pluginErr)
+}
+
+func TestPatchProject_RejectsInvalidMetricOptionsBeforeWriting(t *testing.T) {
+	// no expectations: any database call fails the test
+	withProjectTestDatabase(t, mockdal.NewDal(t))
+	metricPlugin := registerTestMetricPlugin(t, "test-metric-patch")
+	pluginErr := errors.BadInput.New("invalid somePattern")
+	metricPlugin.On("MakeMetricPluginPipelinePlanV200", "renamed-project", json.RawMessage(`{"somePattern":"("}`)).
+		Return(nil, pluginErr).Once()
+
+	output, err := PatchProject("old-project", map[string]interface{}{
+		"name": "renamed-project",
+		"metrics": []interface{}{
+			map[string]interface{}{
+				"pluginName":   "test-metric-patch",
+				"pluginOption": map[string]interface{}{"somePattern": "("},
+				"enable":       true,
+			},
+		},
+	})
+
+	assert.Nil(t, output)
+	require.Error(t, err)
+	assert.Equal(t, errors.BadInput, err.GetType())
+	assert.ErrorIs(t, err, pluginErr)
+}
+
+func TestPatchProject_ValidatesMetricOptionsWithCurrentNameWhenNotRenamed(t *testing.T) {
+	withProjectTestDatabase(t, mockdal.NewDal(t))
+	metricPlugin := registerTestMetricPlugin(t, "test-metric-patch-name")
+	pluginErr := errors.BadInput.New("invalid somePattern")
+	metricPlugin.On("MakeMetricPluginPipelinePlanV200", "old-project", json.RawMessage(`{}`)).
+		Return(nil, pluginErr).Once()
+
+	_, err := PatchProject("old-project", map[string]interface{}{
+		"metrics": []interface{}{
+			map[string]interface{}{"pluginName": "test-metric-patch-name", "pluginOption": map[string]interface{}{}, "enable": true},
+		},
+	})
+
+	assert.ErrorIs(t, err, pluginErr)
+}