From 63ca46d385df47acf85187bd1b2a2269456e3611 Mon Sep 17 00:00:00 2001 From: Chen-Wei Sun Date: Thu, 15 Oct 2026 10:20:03 +0800 Subject: [PATCH 1/2] refactor(project): extract metric plugin plan generation into a helper --- .../services/blueprint_makeplan_v200.go | 37 +++++++++++-------- 1 file changed, 21 insertions(+), 16 deletions(-) diff --git a/backend/server/services/blueprint_makeplan_v200.go b/backend/server/services/blueprint_makeplan_v200.go index e1cf9b228e9..8b63b674ad3 100644 --- a/backend/server/services/blueprint_makeplan_v200.go +++ b/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 { From c7a52084b7336cb583a80fa1fc8994123ff5d52a Mon Sep 17 00:00:00 2001 From: Chen-Wei Sun Date: Thu, 15 Oct 2026 10:29:20 +0800 Subject: [PATCH 2/2] fix(project): validate metric plugin options when saving a project --- backend/plugins/monorepo/impl/impl.go | 2 +- backend/server/services/project.go | 31 ++- .../project_metric_validation_test.go | 191 ++++++++++++++++++ 3 files changed, 219 insertions(+), 5 deletions(-) create mode 100644 backend/server/services/project_metric_validation_test.go diff --git a/backend/plugins/monorepo/impl/impl.go b/backend/plugins/monorepo/impl/impl.go index 5267b28851b..e7bf67d9027 100644 --- a/backend/plugins/monorepo/impl/impl.go +++ b/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 diff --git a/backend/server/services/project.go b/backend/server/services/project.go index acf4abe573f..c93d44f2553 100644 --- a/backend/server/services/project.go +++ b/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 { diff --git a/backend/server/services/project_metric_validation_test.go b/backend/server/services/project_metric_validation_test.go new file mode 100644 index 00000000000..afcb33470a4 --- /dev/null +++ b/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) +}