From 7b29d1da493fb51a74326879ef79d7dbeef1e0b4 Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Thu, 18 Jan 2024 22:50:29 +0100 Subject: [PATCH] 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 | 2 +- server/model/config.go | 15 +--- server/pipeline/config.go | 28 ++----- server/store/datastore/config.go | 50 ++++++++++- server/store/datastore/config_test.go | 106 ++++++++++++++++++------ server/store/datastore/pipeline.go | 22 +++++ server/store/datastore/pipeline_test.go | 65 +++++++++++++++ server/store/datastore/repo_test.go | 3 +- server/store/mocks/store.go | 22 ++--- server/store/store.go | 2 +- 10 files changed, 238 insertions(+), 77 deletions(-) diff --git a/server/forge/mocks/forge.go b/server/forge/mocks/forge.go index 7237255b1..6fe891fd8 100644 --- a/server/forge/mocks/forge.go +++ b/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 diff --git a/server/model/config.go b/server/model/config.go index ff5a4a0bb..c4322bc35 100644 --- a/server/model/config.go +++ b/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'"` - PipelineID int64 `json:"-" xorm:"NOT NULL 'pipeline_id'"` + ConfigID int64 `json:"-" xorm:"UNIQUE(s) NOT NULL 'config_id'"` + PipelineID int64 `json:"-" xorm:"UNIQUE(s) NOT NULL 'pipeline_id'"` } diff --git a/server/pipeline/config.go b/server/pipeline/config.go index 80ba15f9f..f54066593 100644 --- a/server/pipeline/config.go +++ b/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)) - 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), - } - 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 + return store.ConfigPersist(&model.Config{ + RepoID: currentPipeline.RepoID, + Name: stepbuilder.SanitizePath(forgeYamlConfig.Name), + Data: forgeYamlConfig.Data, + }) } diff --git a/server/store/datastore/config.go b/server/store/datastore/config.go index bae31d668..4198d9c06 100644 --- a/server/store/datastore/config.go +++ b/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( - builder.Eq{"config_repo_id": repoID, "config_hash": hash}, + if err := wrapGet(sess.Where( + 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 } diff --git a/server/store/datastore/config_test.go b/server/store/datastore/config_test.go index 97bc25afb..1e5f60805 100644 --- a/server/store/datastore/config_test.go +++ b/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.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) + assert.EqualValues(t, config, foundConfig) 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 ( - data = "pipeline: [ { image: golang, commands: [ go build, go test ] } ]" - hash = "8d8647c9aa90d893bfb79dddbe901f03e258588121e5202632f8ae5738590b26" - ) + // fail due to missing name + assert.Error(t, store.ConfigCreate( + &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) +} diff --git a/server/store/datastore/pipeline.go b/server/store/datastore/pipeline.go index 438caf5d8..1e6518485 100644 --- a/server/store/datastore/pipeline.go +++ b/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 } diff --git a/server/store/datastore/pipeline_test.go b/server/store/datastore/pipeline_test.go index d478e9137..8b86dde5e 100644 --- a/server/store/datastore/pipeline_test.go +++ b/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) +} diff --git a/server/store/datastore/repo_test.go b/server/store/datastore/repo_test.go index 2dcdb8093..630c4200f 100644 --- a/server/store/datastore/repo_test.go +++ b/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{ diff --git a/server/store/mocks/store.go b/server/store/mocks/store.go index 3022cb66c..f4c60c593 100644 --- a/server/store/mocks/store.go +++ b/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 -func (_m *Store) ConfigFindIdentical(repoID int64, hash string) (*model.Config, error) { - ret := _m.Called(repoID, hash) +// ConfigPersist provides a mock function with given fields: _a0 +func (_m *Store) ConfigPersist(_a0 *model.Config) (*model.Config, error) { + 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 { - return rf(repoID, hash) + if rf, ok := ret.Get(0).(func(*model.Config) (*model.Config, error)); ok { + return rf(_a0) } - if rf, ok := ret.Get(0).(func(int64, string) *model.Config); ok { - r0 = rf(repoID, hash) + if rf, ok := ret.Get(0).(func(*model.Config) *model.Config); ok { + 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 { - r1 = rf(repoID, hash) + if rf, ok := ret.Get(1).(func(*model.Config) error); ok { + r1 = rf(_a0) } else { r1 = ret.Error(1) } diff --git a/server/store/store.go b/server/store/store.go index 22075a3b8..8cd528c0c 100644 --- a/server/store/store.go +++ b/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