Make PipelineConfig unique again (#3215)

fix https://github.com/woodpecker-ci/woodpecker/issues/3093

reverts https://github.com/woodpecker-ci/woodpecker/pull/3128

server/forge/mocks/forge.go

@@ -1,4 +1,4 @@
-// Code generated by mockery v2.39.1. DO NOT EDIT.
+// Code generated by mockery v2.40.1. DO NOT EDIT.
 
 package mocks
 

server/model/config.go

@@ -15,26 +15,17 @@
 
 package model
 
-// ConfigStore persists pipeline configuration to storage.
-type ConfigStore interface {
-	ConfigsForPipeline(pipelineID int64) ([]*Config, error)
-	ConfigFindIdentical(repoID int64, hash string) (*Config, error)
-	ConfigFindApproved(*Config) (bool, error)
-	ConfigCreate(*Config) error
-	PipelineConfigCreate(*PipelineConfig) error
-}
-
 // Config represents a pipeline configuration.
 type Config struct {
 	ID     int64  `json:"-"    xorm:"pk autoincr 'config_id'"`
 	RepoID int64  `json:"-"    xorm:"UNIQUE(s) 'config_repo_id'"`
 	Hash   string `json:"hash" xorm:"UNIQUE(s) 'config_hash'"`
-	Name   string `json:"name" xorm:"config_name"`
+	Name   string `json:"name" xorm:"UNIQUE(s) 'config_name'"`
 	Data   []byte `json:"data" xorm:"LONGBLOB 'config_data'"`
 } //	@name Config
 
 // PipelineConfig is the n:n relation between Pipeline and Config
 type PipelineConfig struct {
-	ConfigID   int64 `json:"-"   xorm:"NOT NULL 'config_id'"`
+	ConfigID   int64 `json:"-"   xorm:"UNIQUE(s) NOT NULL 'config_id'"`
-	PipelineID int64 `json:"-"   xorm:"NOT NULL 'pipeline_id'"`
+	PipelineID int64 `json:"-"   xorm:"UNIQUE(s) NOT NULL 'pipeline_id'"`
 }

server/pipeline/config.go

@@ -15,9 +15,6 @@
 package pipeline
 
 import (
-	"crypto/sha256"
-	"fmt"
-
 	forge_types "go.woodpecker-ci.org/woodpecker/v2/server/forge/types"
 	"go.woodpecker-ci.org/woodpecker/v2/server/model"
 	"go.woodpecker-ci.org/woodpecker/v2/server/pipeline/stepbuilder"
@@ -25,24 +22,9 @@ import (
 )
 
 func findOrPersistPipelineConfig(store store.Store, currentPipeline *model.Pipeline, forgeYamlConfig *forge_types.FileMeta) (*model.Config, error) {
-	sha := fmt.Sprintf("%x", sha256.Sum256(forgeYamlConfig.Data))
+	return store.ConfigPersist(&model.Config{
-	conf, err := store.ConfigFindIdentical(currentPipeline.RepoID, sha)
-	if err != nil {
-		conf = &model.Config{
 		RepoID: currentPipeline.RepoID,
-			Data:   forgeYamlConfig.Data,
-			Hash:   sha,
 		Name:   stepbuilder.SanitizePath(forgeYamlConfig.Name),
-		}
+		Data:   forgeYamlConfig.Data,
-		err = store.ConfigCreate(conf)
+	})
-		if err != nil {
-			// retry in case we receive two hooks at the same time
-			conf, err = store.ConfigFindIdentical(currentPipeline.RepoID, sha)
-			if err != nil {
-				return nil, err
-			}
-		}
-	}
-
-	return conf, nil
 }

server/store/datastore/config.go

@@ -15,9 +15,15 @@
 package datastore
 
 import (
+	"crypto/sha256"
+	"errors"
+	"fmt"
+
 	"xorm.io/builder"
+	"xorm.io/xorm"
 
 	"go.woodpecker-ci.org/woodpecker/v2/server/model"
+	"go.woodpecker-ci.org/woodpecker/v2/server/store/types"
 )
 
 func (s storage) ConfigsForPipeline(pipelineID int64) ([]*model.Config, error) {
@@ -29,16 +35,40 @@ func (s storage) ConfigsForPipeline(pipelineID int64) ([]*model.Config, error) {
 		Find(&configs)
 }
 
-func (s storage) ConfigFindIdentical(repoID int64, hash string) (*model.Config, error) {
+func (s storage) configFindIdentical(sess *xorm.Session, repoID int64, hash, name string) (*model.Config, error) {
 	conf := new(model.Config)
-	if err := wrapGet(s.engine.Where(
+	if err := wrapGet(sess.Where(
-		builder.Eq{"config_repo_id": repoID, "config_hash": hash},
+		builder.Eq{"config_repo_id": repoID, "config_hash": hash, "config_name": name},
 	).Get(conf)); err != nil {
 		return nil, err
 	}
 	return conf, nil
 }
 
+func (s storage) ConfigPersist(conf *model.Config) (*model.Config, error) {
+	conf.Hash = fmt.Sprintf("%x", sha256.Sum256(conf.Data))
+
+	sess := s.engine.NewSession()
+	defer sess.Close()
+	if err := sess.Begin(); err != nil {
+		return nil, err
+	}
+
+	existingConfig, err := s.configFindIdentical(sess, conf.RepoID, conf.Hash, conf.Name)
+	if err != nil && !errors.Is(err, types.RecordNotExist) {
+		return nil, err
+	}
+	if existingConfig != nil {
+		return existingConfig, nil
+	}
+
+	if err := s.configCreate(sess, conf); err != nil {
+		return nil, err
+	}
+
+	return conf, sess.Commit()
+}
+
 func (s storage) ConfigFindApproved(config *model.Config) (bool, error) {
 	return s.engine.Table("pipelines").Select("pipelines.pipeline_id").
 		Join("INNER", "pipeline_config", "pipelines.pipeline_id = pipeline_config.pipeline_id").
@@ -48,8 +78,20 @@ func (s storage) ConfigFindApproved(config *model.Config) (bool, error) {
 }
 
 func (s storage) ConfigCreate(config *model.Config) error {
+	return s.configCreate(s.engine.NewSession(), config)
+}
+
+func (s storage) configCreate(sess *xorm.Session, config *model.Config) error {
+	// should never happen but just in case
+	if config.Name == "" {
+		return fmt.Errorf("insert config to store failed: 'Name' has to be set")
+	}
+	if config.Hash == "" {
+		return fmt.Errorf("insert config to store failed: 'Hash' has to be set")
+	}
+
 	// only Insert set auto created ID back to object
-	_, err := s.engine.Insert(config)
+	_, err := sess.Insert(config)
 	return err
 }
 

server/store/datastore/config_test.go

@@ -22,15 +22,16 @@ import (
 	"go.woodpecker-ci.org/woodpecker/v2/server/model"
 )
 
+var (
+	data = []byte("pipeline: [ { image: golang, commands: [ go build, go test ] } ]")
+	hash = "8d8647c9aa90d893bfb79dddbe901f03e258588121e5202632f8ae5738590b26"
+	name = "test"
+)
+
 func TestConfig(t *testing.T) {
 	store, closer := newTestStore(t, new(model.Config), new(model.PipelineConfig), new(model.Pipeline), new(model.Repo))
 	defer closer()
 
-	var (
-		data = "pipeline: [ { image: golang, commands: [ go build, go test ] } ]"
-		hash = "8d8647c9aa90d893bfb79dddbe901f03e258588121e5202632f8ae5738590b26"
-	)
-
 	repo := &model.Repo{
 		UserID:   1,
 		FullName: "bradrydzewski/test",
@@ -41,9 +42,9 @@ func TestConfig(t *testing.T) {
 
 	config := &model.Config{
 		RepoID: repo.ID,
-		Data:   []byte(data),
+		Data:   data,
 		Hash:   hash,
-		Name:   "default",
+		Name:   name,
 	}
 	assert.NoError(t, store.ConfigCreate(config))
 
@@ -61,13 +62,9 @@ func TestConfig(t *testing.T) {
 		},
 	))
 
-	config, err := store.ConfigFindIdentical(repo.ID, hash)
+	foundConfig, err := store.configFindIdentical(store.engine.NewSession(), repo.ID, hash, name)
 	assert.NoError(t, err)
-	assert.EqualValues(t, 1, config.ID)
+	assert.EqualValues(t, config, foundConfig)
-	assert.Equal(t, repo.ID, config.RepoID)
-	assert.Equal(t, data, string(config.Data))
-	assert.Equal(t, hash, config.Hash)
-	assert.Equal(t, "default", config.Name)
 
 	loaded, err := store.ConfigsForPipeline(pipeline.ID)
 	assert.NoError(t, err)
@@ -87,8 +84,6 @@ func TestConfigApproved(t *testing.T) {
 	assert.NoError(t, store.CreateRepo(repo))
 
 	var (
-		data            = "pipeline: [ { image: golang, commands: [ go build, go test ] } ]"
-		hash            = "8d8647c9aa90d893bfb79dddbe901f03e258588121e5202632f8ae5738590b26"
 		pipelineBlocked = &model.Pipeline{
 			RepoID: repo.ID,
 			Status: model.StatusBlocked,
@@ -110,8 +105,9 @@ func TestConfigApproved(t *testing.T) {
 	assert.NoError(t, store.CreatePipeline(pipelinePending))
 	conf := &model.Config{
 		RepoID: repo.ID,
-		Data:   []byte(data),
+		Data:   data,
 		Hash:   hash,
+		Name:   name,
 	}
 	assert.NoError(t, store.ConfigCreate(conf))
 	pipelineConfig := &model.PipelineConfig{
@@ -129,8 +125,9 @@ func TestConfigApproved(t *testing.T) {
 	assert.NoError(t, store.CreatePipeline(pipelineRunning))
 	conf2 := &model.Config{
 		RepoID: repo.ID,
-		Data:   []byte(data),
+		Data:   data,
 		Hash:   "xxx",
+		Name:   "xxx",
 	}
 	assert.NoError(t, store.ConfigCreate(conf2))
 	pipelineConfig2 := &model.PipelineConfig{
@@ -144,20 +141,34 @@ func TestConfigApproved(t *testing.T) {
 	assert.True(t, approved)
 }
 
-func TestConfigIndexes(t *testing.T) {
+func TestConfigCreate(t *testing.T) {
 	store, closer := newTestStore(t, new(model.Config), new(model.Step), new(model.Pipeline), new(model.Repo))
 	defer closer()
 
-	var (
+	// fail due to missing name
-		data = "pipeline: [ { image: golang, commands: [ go build, go test ] } ]"
+	assert.Error(t, store.ConfigCreate(
-		hash = "8d8647c9aa90d893bfb79dddbe901f03e258588121e5202632f8ae5738590b26"
+		&model.Config{
-	)
+			RepoID: 2,
+			Data:   data,
+			Hash:   hash,
+		},
+	))
+
+	// fail due to missing hash
+	assert.Error(t, store.ConfigCreate(
+		&model.Config{
+			RepoID: 2,
+			Data:   data,
+			Name:   name,
+		},
+	))
 
 	assert.NoError(t, store.ConfigCreate(
 		&model.Config{
 			RepoID: 2,
-			Data:   []byte(data),
+			Data:   data,
 			Hash:   hash,
+			Name:   name,
 		},
 	))
 
@@ -165,8 +176,55 @@ func TestConfigIndexes(t *testing.T) {
 	assert.Error(t, store.ConfigCreate(
 		&model.Config{
 			RepoID: 2,
-			Data:   []byte(data),
+			Data:   data,
 			Hash:   hash,
+			Name:   name,
 		},
 	))
 }
+
+func TestConfigPersist(t *testing.T) {
+	store, closer := newTestStore(t, new(model.Config))
+	defer closer()
+
+	conf1 := &model.Config{
+		RepoID: 2,
+		Data:   data,
+		Hash:   hash,
+		Name:   name,
+	}
+	conf2 := &model.Config{
+		RepoID: 2,
+		Data:   []byte("steps: [ { image: golang, commands: [ go generate ] } ]"),
+		Name:   "generate",
+	}
+
+	conf1, err := store.ConfigPersist(conf1)
+	assert.NoError(t, err)
+	assert.EqualValues(t, hash, conf1.Hash)
+	conf1secondInsert, err := store.ConfigPersist(conf1)
+	assert.NoError(t, err)
+	assert.EqualValues(t, conf1, conf1secondInsert)
+	count, err := store.engine.Count(new(model.Config))
+	assert.NoError(t, err)
+	assert.EqualValues(t, 1, count)
+
+	newConf2, err := store.ConfigPersist(conf2)
+	assert.NoError(t, err)
+	assert.EqualValues(t, "66f28f8d487a48aacf29d9feea13b0ab5dbb5025296b77a6addde93efcc4d82b", newConf2.Hash)
+	count, err = store.engine.Count(new(model.Config))
+	assert.NoError(t, err)
+	assert.EqualValues(t, 2, count)
+
+	// test for https://github.com/woodpecker-ci/woodpecker/issues/3093
+	_, err = store.ConfigPersist(&model.Config{
+		RepoID: 2,
+		Data:   data,
+		Hash:   hash,
+		Name:   "some other",
+	})
+	assert.NoError(t, err)
+	count, err = store.engine.Count(new(model.Config))
+	assert.NoError(t, err)
+	assert.EqualValues(t, 3, count)
+}

server/store/datastore/pipeline.go

@@ -157,6 +157,28 @@ func (s storage) deletePipeline(sess *xorm.Session, pipelineID int64) error {
 			}
 		}
 	}
+
+	if _, err := sess.Where("workflow_pipeline_id = ?", pipelineID).Delete(new(model.Workflow)); err != nil {
+		return err
+	}
+
+	var confIDs []int64
+	if err := sess.Table(new(model.PipelineConfig)).Select("config_id").Where("pipeline_id = ?", pipelineID).Find(&confIDs); err != nil {
+		return err
+	}
+	for _, confID := range confIDs {
+		exist, err := sess.Where(builder.Eq{"config_id": confID}.And(builder.Neq{"pipeline_id": pipelineID})).Exist(new(model.PipelineConfig))
+		if err != nil {
+			return err
+		}
+		if !exist {
+			// this config is only used for this pipeline. so delete it
+			if _, err := sess.Where(builder.Eq{"config_id": confID}).Delete(new(model.Config)); err != nil {
+				return err
+			}
+		}
+	}
+
 	if _, err := sess.Where("pipeline_id = ?", pipelineID).Delete(new(model.PipelineConfig)); err != nil {
 		return err
 	}

server/store/datastore/pipeline_test.go

@@ -23,6 +23,7 @@ import (
 	"github.com/stretchr/testify/assert"
 
 	"go.woodpecker-ci.org/woodpecker/v2/server/model"
+	"go.woodpecker-ci.org/woodpecker/v2/server/store/types"
 )
 
 func TestPipelines(t *testing.T) {
@@ -323,3 +324,67 @@ func TestPipelineIncrement(t *testing.T) {
 	assert.NoError(t, store.CreatePipeline(pipelineC))
 	assert.EqualValues(t, 1, pipelineC.Number)
 }
+
+func TestDeletePipeline(t *testing.T) {
+	store, closer := newTestStore(t, new(model.Pipeline), new(model.Repo), new(model.Workflow),
+		new(model.Step), new(model.LogEntry), new(model.PipelineConfig), new(model.Config))
+	defer closer()
+
+	_, err := store.engine.Insert(
+		&model.Pipeline{
+			ID:     2,
+			Number: 2,
+			RepoID: 7,
+		},
+		&model.Pipeline{
+			ID:     5,
+			Number: 3,
+			RepoID: 7,
+		},
+		&model.Pipeline{
+			ID:     8,
+			Number: 4,
+			RepoID: 7,
+		},
+		&model.Config{
+			ID:     23,
+			Hash:   "1234",
+			Name:   "test",
+			RepoID: 7,
+		},
+		&model.Config{
+			ID:     25,
+			Hash:   "6789",
+			Name:   "test",
+			RepoID: 7,
+		},
+		&model.PipelineConfig{
+			PipelineID: 2,
+			ConfigID:   23,
+		},
+		&model.PipelineConfig{
+			PipelineID: 5,
+			ConfigID:   23,
+		},
+		&model.PipelineConfig{
+			PipelineID: 8,
+			ConfigID:   25,
+		},
+	)
+	assert.NoError(t, err)
+
+	// delete non existing pipeline
+	assert.ErrorIs(t, types.RecordNotExist, store.DeletePipeline(&model.Pipeline{ID: 1}))
+
+	// delete pipeline with shares config
+	assert.NoError(t, store.DeletePipeline(&model.Pipeline{ID: 2}))
+	count, err := store.engine.Count(new(model.Config))
+	assert.NoError(t, err)
+	assert.EqualValues(t, 2, count)
+
+	// delete pipeline with unique config
+	assert.NoError(t, store.DeletePipeline(&model.Pipeline{ID: 8}))
+	count, err = store.engine.Count(new(model.Config))
+	assert.NoError(t, err)
+	assert.EqualValues(t, 1, count)
+}

server/store/datastore/repo_test.go

@@ -306,7 +306,8 @@ func TestRepoCrud(t *testing.T) {
 		new(model.Secret),
 		new(model.Registry),
 		new(model.Config),
-		new(model.Redirection))
+		new(model.Redirection),
+		new(model.Workflow))
 	defer closer()
 
 	repo := model.Repo{

server/store/mocks/store.go

@@ -1,4 +1,4 @@
-// Code generated by mockery v2.39.1. DO NOT EDIT.
+// Code generated by mockery v2.40.1. DO NOT EDIT.
 
 package mocks
 
@@ -220,29 +220,29 @@ func (_m *Store) ConfigFindApproved(_a0 *model.Config) (bool, error) {
 	return r0, r1
 }
 
-// ConfigFindIdentical provides a mock function with given fields: repoID, hash
+// ConfigPersist provides a mock function with given fields: _a0
-func (_m *Store) ConfigFindIdentical(repoID int64, hash string) (*model.Config, error) {
+func (_m *Store) ConfigPersist(_a0 *model.Config) (*model.Config, error) {
-	ret := _m.Called(repoID, hash)
+	ret := _m.Called(_a0)
 
 	if len(ret) == 0 {
-		panic("no return value specified for ConfigFindIdentical")
+		panic("no return value specified for ConfigPersist")
 	}
 
 	var r0 *model.Config
 	var r1 error
-	if rf, ok := ret.Get(0).(func(int64, string) (*model.Config, error)); ok {
+	if rf, ok := ret.Get(0).(func(*model.Config) (*model.Config, error)); ok {
-		return rf(repoID, hash)
+		return rf(_a0)
 	}
-	if rf, ok := ret.Get(0).(func(int64, string) *model.Config); ok {
+	if rf, ok := ret.Get(0).(func(*model.Config) *model.Config); ok {
-		r0 = rf(repoID, hash)
+		r0 = rf(_a0)
 	} else {
 		if ret.Get(0) != nil {
 			r0 = ret.Get(0).(*model.Config)
 		}
 	}
 
-	if rf, ok := ret.Get(1).(func(int64, string) error); ok {
+	if rf, ok := ret.Get(1).(func(*model.Config) error); ok {
-		r1 = rf(repoID, hash)
+		r1 = rf(_a0)
 	} else {
 		r1 = ret.Error(1)
 	}

server/store/store.go

@@ -112,7 +112,7 @@ type Store interface {
 
 	// Configs
 	ConfigsForPipeline(pipelineID int64) ([]*model.Config, error)
-	ConfigFindIdentical(repoID int64, hash string) (*model.Config, error)
+	ConfigPersist(*model.Config) (*model.Config, error)
 	ConfigFindApproved(*model.Config) (bool, error)
 	ConfigCreate(*model.Config) error
 	PipelineConfigCreate(*model.PipelineConfig) error