diff --git a/backend/core/plugin/plugin_blueprint.go b/backend/core/plugin/plugin_blueprint.go index 35c36b36b25..dac851c1814 100644 --- a/backend/core/plugin/plugin_blueprint.go +++ b/backend/core/plugin/plugin_blueprint.go @@ -20,6 +20,7 @@ package plugin import ( "encoding/json" + "github.com/apache/incubator-devlake/core/dal" "github.com/apache/incubator-devlake/core/errors" "github.com/apache/incubator-devlake/core/models" ) @@ -92,6 +93,23 @@ type ProjectMapper interface { MapProject(projectName string, scopes []Scope) (models.PipelinePlan, errors.Error) } +// ProjectDeleteHook is implemented by plugins that persist state whose +// lifecycle is tied to a DevLake project. +// +// BeforeDeleteProject is called inside the core deletion transaction before +// the project's core records are deleted. Returning an error aborts deletion. +// +// Contract: +// - Database mutations must use only the supplied transaction. +// - The plugin must not commit or roll back the transaction. +// - External side effects cannot be rolled back by the database transaction. +// - Plugins must not depend on hook execution order across plugins. +// Plugins that do not maintain project-scoped state do not need to implement +// this interface. +type ProjectDeleteHook interface { + BeforeDeleteProject(tx dal.Transaction, projectName string) errors.Error +} + // CompositeDataSourcePluginBlueprintV200 is for unit test type CompositeDataSourcePluginBlueprintV200 interface { PluginMeta diff --git a/backend/helpers/pluginhelper/services/blueprint_helper.go b/backend/helpers/pluginhelper/services/blueprint_helper.go index aae0535f79c..f91e97bc2ca 100644 --- a/backend/helpers/pluginhelper/services/blueprint_helper.go +++ b/backend/helpers/pluginhelper/services/blueprint_helper.go @@ -262,7 +262,18 @@ func (b *BlueprintManager) DeleteBlueprint(id uint64) errors.Error { } } }() - err = tx.Delete(&models.BlueprintLabel{}, dal.Where("blueprint_id = ?", id)) + err = b.DeleteBlueprintInTransaction(tx, id) + if err != nil { + return err + } + err = tx.Commit() + return err +} + +// DeleteBlueprintInTransaction removes a blueprint and its dependent records using +// the caller's transaction. The caller owns commit and rollback. +func (b *BlueprintManager) DeleteBlueprintInTransaction(tx dal.Transaction, id uint64) errors.Error { + err := tx.Delete(&models.BlueprintLabel{}, dal.Where("blueprint_id = ?", id)) if err != nil { return err } @@ -274,9 +285,15 @@ func (b *BlueprintManager) DeleteBlueprint(id uint64) errors.Error { if err != nil { return err } - errors.Must(tx.Delete(&models.BlueprintConnection{}, dal.Where("blueprint_id = ?", id))) - errors.Must(tx.Delete(&models.BlueprintScope{}, dal.Where("blueprint_id = ?", id))) - return tx.Commit() + err = tx.Delete(&models.BlueprintConnection{}, dal.Where("blueprint_id = ?", id)) + if err != nil { + return err + } + err = tx.Delete(&models.BlueprintScope{}, dal.Where("blueprint_id = ?", id)) + if err != nil { + return err + } + return nil } func (b *BlueprintManager) fillBlueprintDetail(blueprint *models.Blueprint) errors.Error { diff --git a/backend/helpers/pluginhelper/services/blueprint_helper_test.go b/backend/helpers/pluginhelper/services/blueprint_helper_test.go new file mode 100644 index 00000000000..33538e6eb50 --- /dev/null +++ b/backend/helpers/pluginhelper/services/blueprint_helper_test.go @@ -0,0 +1,51 @@ +/* +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 ( + "testing" + + "github.com/apache/incubator-devlake/core/errors" + dalmocks "github.com/apache/incubator-devlake/mocks/core/dal" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" +) + +func TestDeleteBlueprintInTransaction(t *testing.T) { + t.Run("uses the caller transaction for every delete", func(t *testing.T) { + tx := dalmocks.NewTransaction(t) + tx.On("Delete", mock.Anything, mock.Anything).Return(nil).Times(4) + + manager := &BlueprintManager{} + err := manager.DeleteBlueprintInTransaction(tx, 42) + + assert.NoError(t, err) + }) + + t.Run("returns dependent deletion failures", func(t *testing.T) { + tx := dalmocks.NewTransaction(t) + expected := errors.Default.New("unable to delete blueprint") + tx.On("Delete", mock.Anything, mock.Anything).Return(nil).Once() + tx.On("Delete", mock.Anything, mock.Anything).Return(expected).Once() + + manager := &BlueprintManager{} + err := manager.DeleteBlueprintInTransaction(tx, 42) + + assert.ErrorIs(t, err, expected) + }) +} diff --git a/backend/server/services/project.go b/backend/server/services/project.go index a1b92b03ae4..5ed0941fed0 100644 --- a/backend/server/services/project.go +++ b/backend/server/services/project.go @@ -28,6 +28,7 @@ import ( "github.com/apache/incubator-devlake/core/errors" "github.com/apache/incubator-devlake/core/models" "github.com/apache/incubator-devlake/core/models/domainlayer/crossdomain" + "github.com/apache/incubator-devlake/core/plugin" helper "github.com/apache/incubator-devlake/helpers/pluginhelper/api" ) @@ -327,6 +328,9 @@ func thereAreUnfinishedPipelinesUnderProject(projectName string) (bool, errors.E if err != nil { return false, err } + if blueprint == nil { + return false, nil + } return thereAreUnfinishedPipelinesUnderBlueprint(blueprint.ID) } @@ -366,10 +370,6 @@ func DeleteProject(name string) errors.Error { if pipelinesAreUnfinished { return errors.Default.New("There are unfinished pipelines in the current project. It cannot be deleted at this time.") } - err = deleteProjectBlueprint(name) - if err != nil { - return err - } tx := db.Begin() defer func() { if r := recover(); r != nil || err != nil { @@ -379,6 +379,13 @@ func DeleteProject(name string) errors.Error { } } }() + if err = runProjectDeleteHooks(tx, name); err != nil { + return err + } + err = deleteProjectBlueprint(tx, name) + if err != nil { + return err + } err = tx.Delete(&models.Project{}, dal.Where("name = ?", name)) if err != nil { return errors.Default.Wrap(err, "error deleting project") @@ -399,20 +406,33 @@ func DeleteProject(name string) errors.Error { if err != nil { return errors.Default.Wrap(err, "error deleting project Issue metric") } - return tx.Commit() + err = tx.Commit() + return err +} + +func runProjectDeleteHooks(tx dal.Transaction, projectName string) errors.Error { + return plugin.TraversalPlugin(func(name string, pluginInst plugin.PluginMeta) errors.Error { + if hook, ok := pluginInst.(plugin.ProjectDeleteHook); ok { + if err := hook.BeforeDeleteProject(tx, projectName); err != nil { + return errors.Default.Wrap(err, fmt.Sprintf("error executing delete hook for plugin %s", name)) + } + } + return nil + }) } -func deleteProjectBlueprint(projectName string) errors.Error { - bp, err := bpManager.GetDbBlueprintByProjectName(projectName) +func deleteProjectBlueprint(tx dal.Transaction, projectName string) errors.Error { + bp := &models.Blueprint{} + err := tx.First(bp, dal.Where("project_name = ?", projectName)) if err != nil { - if !db.IsErrorNotFound(err) { + if !tx.IsErrorNotFound(err) { return errors.Default.Wrap(err, fmt.Sprintf("error finding blueprint associated with project %s", projectName)) } - } else { - err = bpManager.DeleteBlueprint(bp.ID) - if err != nil { - return errors.Default.Wrap(err, fmt.Sprintf("error deleting blueprint associated with project %s", projectName)) - } + return nil + } + err = bpManager.DeleteBlueprintInTransaction(tx, bp.ID) + if err != nil { + return errors.Default.Wrap(err, fmt.Sprintf("error deleting blueprint associated with project %s", projectName)) } return nil } diff --git a/backend/server/services/project_test.go b/backend/server/services/project_test.go new file mode 100644 index 00000000000..ca1029b459a --- /dev/null +++ b/backend/server/services/project_test.go @@ -0,0 +1,216 @@ +/* +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 ( + "fmt" + "testing" + + "github.com/apache/incubator-devlake/core/dal" + "github.com/apache/incubator-devlake/core/errors" + "github.com/apache/incubator-devlake/core/models" + "github.com/apache/incubator-devlake/core/plugin" + blueprintservices "github.com/apache/incubator-devlake/helpers/pluginhelper/services" + dalmocks "github.com/apache/incubator-devlake/mocks/core/dal" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" +) + +type testOrdinaryPlugin struct { + name string +} + +func (p *testOrdinaryPlugin) Description() string { return "test ordinary plugin" } +func (p *testOrdinaryPlugin) RootPkgPath() string { return "plugins/test_ordinary" } +func (p *testOrdinaryPlugin) Name() string { return p.name } + +type testHookPlugin struct { + name string + enabled bool + deleteCalls []struct { + tx dal.Transaction + projectName string + } + deleteErr errors.Error +} + +func (p *testHookPlugin) Description() string { return "test hook plugin" } +func (p *testHookPlugin) RootPkgPath() string { return "plugins/test_hook" } +func (p *testHookPlugin) Name() string { return p.name } +func (p *testHookPlugin) BeforeDeleteProject(tx dal.Transaction, projectName string) errors.Error { + if !p.enabled { + return nil + } + p.deleteCalls = append(p.deleteCalls, struct { + tx dal.Transaction + projectName string + }{tx: tx, projectName: projectName}) + return p.deleteErr +} + +func TestRunProjectDeleteHooks(t *testing.T) { + t.Run("skips ordinary plugins without ProjectDeleteHook", func(t *testing.T) { + ordinary := &testOrdinaryPlugin{name: "test-ordinary-skip"} + assert.NoError(t, plugin.RegisterPlugin(ordinary.Name(), ordinary)) + + tx := dalmocks.NewTransaction(t) + assert.NoError(t, runProjectDeleteHooks(tx, "test-project")) + }) + + t.Run("invokes implementing plugins with exact transaction and project name", func(t *testing.T) { + hook := &testHookPlugin{name: "test-hook-invoke", enabled: true} + t.Cleanup(func() { hook.enabled = false }) + assert.NoError(t, plugin.RegisterPlugin(hook.Name(), hook)) + + tx := dalmocks.NewTransaction(t) + assert.NoError(t, runProjectDeleteHooks(tx, "test-project")) + + assert.Equal(t, 1, len(hook.deleteCalls)) + assert.Equal(t, tx, hook.deleteCalls[0].tx) + assert.Equal(t, "test-project", hook.deleteCalls[0].projectName) + }) + + t.Run("returns wrapped error on hook veto", func(t *testing.T) { + expectedErr := errors.Default.New("project delete vetoed by plugin") + hook := &testHookPlugin{ + name: "test-hook-veto", + enabled: true, + deleteErr: expectedErr, + } + t.Cleanup(func() { hook.enabled = false }) + assert.NoError(t, plugin.RegisterPlugin(hook.Name(), hook)) + + tx := dalmocks.NewTransaction(t) + err := runProjectDeleteHooks(tx, "test-project") + + assert.Error(t, err) + assert.ErrorIs(t, err, expectedErr) + assert.Contains(t, err.Error(), fmt.Sprintf("error executing delete hook for plugin %s", hook.Name())) + }) +} + +func TestDeleteProject_RollsBackOnDeleteHookVeto(t *testing.T) { + hookErr := errors.Default.New("hook rejection") + hook := &testHookPlugin{ + name: "test-hook-rollback-veto", + enabled: true, + deleteErr: hookErr, + } + t.Cleanup(func() { hook.enabled = false }) + assert.NoError(t, plugin.RegisterPlugin(hook.Name(), hook)) + + mockDB := dalmocks.NewDal(t) + tx := dalmocks.NewTransaction(t) + notFound := errors.NotFound.New("blueprint not found") + + mockDB.On("First", mock.Anything, mock.Anything).Return(nil).Once() + mockDB.On("First", mock.Anything, mock.Anything).Return(notFound).Once() + mockDB.On("IsErrorNotFound", mock.Anything).Return(true).Twice() + mockDB.On("Begin").Return(tx).Once() + tx.On("Rollback").Return(nil).Once() + + previousDB, previousManager := db, bpManager + db = mockDB + bpManager = blueprintservices.NewBlueprintManager(mockDB) + t.Cleanup(func() { + db = previousDB + bpManager = previousManager + }) + + err := DeleteProject("project-veto") + + assert.Error(t, err) + assert.ErrorIs(t, err, hookErr) + assert.Contains(t, err.Error(), fmt.Sprintf("error executing delete hook for plugin %s", hook.Name())) + assert.Equal(t, 1, len(hook.deleteCalls)) + assert.Equal(t, "project-veto", hook.deleteCalls[0].projectName) +} + +func TestDeleteProject_SuccessfulDeletionInSingleTransaction(t *testing.T) { + hook := &testHookPlugin{ + name: "test-hook-success", + enabled: true, + } + t.Cleanup(func() { hook.enabled = false }) + assert.NoError(t, plugin.RegisterPlugin(hook.Name(), hook)) + + mockDB := dalmocks.NewDal(t) + tx := dalmocks.NewTransaction(t) + + // 1. Verify project exists + mockDB.On("First", mock.MatchedBy(func(target interface{}) bool { + _, ok := target.(*models.Project) + return ok + }), mock.Anything).Return(nil).Once() + + // 2. Blueprint lookup before transaction + mockDB.On("First", mock.MatchedBy(func(target interface{}) bool { + bp, ok := target.(*models.Blueprint) + if ok { + bp.ID = 42 + } + return ok + }), mock.Anything).Return(nil).Once() + mockDB.On("Pluck", "name", mock.Anything, mock.Anything).Return(nil).Once() + mockDB.On("All", mock.MatchedBy(func(target interface{}) bool { + _, ok := target.(*[]*models.BlueprintConnection) + return ok + }), mock.Anything).Return(nil).Once() + + // 3. Pipeline lookup for unfinished pipelines check + mockDB.On("Count", mock.Anything).Return(int64(0), nil).Once() + mockDB.On("All", mock.MatchedBy(func(target interface{}) bool { + _, ok := target.(*[]*models.Pipeline) + return ok + }), mock.Anything).Return(nil).Once() + + // 4. Begin transaction + mockDB.On("Begin").Return(tx).Once() + + // 5. Blueprint lookup inside transaction for deletion + tx.On("First", mock.MatchedBy(func(target interface{}) bool { + bp, ok := target.(*models.Blueprint) + if ok { + bp.ID = 42 + } + return ok + }), mock.Anything).Return(nil).Once() + + // 6. DeleteBlueprintInTransaction calls tx.Delete 4 times + // 7. Core project deletes call tx.Delete 5 times + // Total tx.Delete calls = 9 + tx.On("Delete", mock.Anything, mock.Anything).Return(nil).Times(9) + + // 8. Single commit for everything + tx.On("Commit").Return(nil).Once() + + previousDB, previousManager := db, bpManager + db = mockDB + bpManager = blueprintservices.NewBlueprintManager(mockDB) + t.Cleanup(func() { + db = previousDB + bpManager = previousManager + }) + + err := DeleteProject("project-success") + + assert.NoError(t, err) + assert.Equal(t, 1, len(hook.deleteCalls)) + assert.Equal(t, tx, hook.deleteCalls[0].tx) + assert.Equal(t, "project-success", hook.deleteCalls[0].projectName) +}