This is an automated email from the ASF dual-hosted git repository.
klesh pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/devlake.git
The following commit(s) were added to refs/heads/main by this push:
new f5c90644f fix(project): validate metric plugin options when saving a
project (#9228)
f5c90644f is described below
commit f5c90644f2cf99c93d1fa6a91331a760b99278f0
Author: AwEi <[email protected]>
AuthorDate: Sun Oct 18 14:10:52 2026 +0800
fix(project): validate metric plugin options when saving a project (#9228)
* refactor(project): extract metric plugin plan generation into a helper
* fix(project): validate metric plugin options when saving a project
---
backend/plugins/monorepo/impl/impl.go | 2 +-
backend/server/services/blueprint_makeplan_v200.go | 37 ++--
backend/server/services/project.go | 31 +++-
.../services/project_metric_validation_test.go | 191 +++++++++++++++++++++
4 files changed, 240 insertions(+), 21 deletions(-)
diff --git a/backend/plugins/monorepo/impl/impl.go
b/backend/plugins/monorepo/impl/impl.go
index 5267b2885..e7bf67d90 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/blueprint_makeplan_v200.go
b/backend/server/services/blueprint_makeplan_v200.go
index e1cf9b228..8b63b674a 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 {
diff --git a/backend/server/services/project.go
b/backend/server/services/project.go
index acf4abe57..c93d44f25 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 000000000..afcb33470
--- /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)
+}
