diff --git a/src/api/artifact/controller.go b/src/api/artifact/controller.go index f817f4c00..15bd524dc 100644 --- a/src/api/artifact/controller.go +++ b/src/api/artifact/controller.go @@ -27,6 +27,7 @@ import ( "github.com/goharbor/harbor/src/api/artifact/descriptor" "github.com/goharbor/harbor/src/api/tag" "github.com/goharbor/harbor/src/internal" + "github.com/goharbor/harbor/src/internal/orm" "github.com/goharbor/harbor/src/pkg/artifactrash" "github.com/goharbor/harbor/src/pkg/artifactrash/model" "github.com/goharbor/harbor/src/pkg/blob" @@ -175,20 +176,30 @@ func (c *controller) ensureArtifact(ctx context.Context, repository, digest stri artifact.Type = descriptor.GetArtifactType(artifact.MediaType) // create it - id, err := c.artMgr.Create(ctx, artifact) - if err != nil { - // if got conflict error, try to get the artifact again - if ierror.IsConflictErr(err) { - art, err = c.artMgr.GetByDigest(ctx, repository, digest) - if err == nil { - return false, art, nil + // use orm.WithTransaction here to avoid the issue: + // https://www.postgresql.org/message-id/002e01c04da9%24a8f95c20%2425efe6c1%40lasting.ro + created := false + if err = orm.WithTransaction(func(ctx context.Context) error { + id, err := c.artMgr.Create(ctx, artifact) + if err != nil { + // if got conflict error, try to get the artifact again + if ierror.IsConflictErr(err) { + var e error + artifact, e = c.artMgr.GetByDigest(ctx, repository, digest) + if e != nil { + err = e + } } - return false, nil, err + return err } + created = true + artifact.ID = id + return nil + })(ctx); err != nil && !ierror.IsConflictErr(err) { return false, nil, err } - artifact.ID = id - return true, artifact, nil + + return created, artifact, nil } func (c *controller) Count(ctx context.Context, query *q.Query) (int64, error) { @@ -338,15 +349,20 @@ func (c *controller) deleteDeeply(ctx context.Context, id int64, isRoot bool) er return err } - _, err = c.artrashMgr.Create(ctx, &model.ArtifactTrash{ - MediaType: art.MediaType, - ManifestMediaType: art.ManifestMediaType, - RepositoryName: art.RepositoryName, - Digest: art.Digest, - }) - if err != nil && !ierror.IsErr(err, ierror.ConflictCode) { + // use orm.WithTransaction here to avoid the issue: + // https://www.postgresql.org/message-id/002e01c04da9%24a8f95c20%2425efe6c1%40lasting.ro + if err = orm.WithTransaction(func(ctx context.Context) error { + _, err = c.artrashMgr.Create(ctx, &model.ArtifactTrash{ + MediaType: art.MediaType, + ManifestMediaType: art.ManifestMediaType, + RepositoryName: art.RepositoryName, + Digest: art.Digest, + }) + return err + })(ctx); err != nil && !ierror.IsErr(err, ierror.ConflictCode) { return err } + // TODO fire delete artifact event return nil diff --git a/src/api/artifact/controller_test.go b/src/api/artifact/controller_test.go index 05b84137c..9e9d1bd8b 100644 --- a/src/api/artifact/controller_test.go +++ b/src/api/artifact/controller_test.go @@ -25,10 +25,12 @@ import ( "github.com/goharbor/harbor/src/common/models" "github.com/goharbor/harbor/src/internal" ierror "github.com/goharbor/harbor/src/internal/error" + "github.com/goharbor/harbor/src/internal/orm" "github.com/goharbor/harbor/src/pkg/artifact" "github.com/goharbor/harbor/src/pkg/q" model_tag "github.com/goharbor/harbor/src/pkg/tag/model/tag" tagtesting "github.com/goharbor/harbor/src/testing/api/tag" + ormtesting "github.com/goharbor/harbor/src/testing/lib/orm" arttesting "github.com/goharbor/harbor/src/testing/pkg/artifact" artrashtesting "github.com/goharbor/harbor/src/testing/pkg/artifactrash" "github.com/goharbor/harbor/src/testing/pkg/blob" @@ -162,7 +164,7 @@ func (c *controllerTestSuite) TestEnsureArtifact() { c.artMgr.On("GetByDigest").Return(&artifact.Artifact{ ID: 1, }, nil) - created, art, err := c.ctl.ensureArtifact(nil, "library/hello-world", digest) + created, art, err := c.ctl.ensureArtifact(orm.NewContext(nil, &ormtesting.FakeOrmer{}), "library/hello-world", digest) c.Require().Nil(err) c.False(created) c.Equal(int64(1), art.ID) @@ -177,7 +179,7 @@ func (c *controllerTestSuite) TestEnsureArtifact() { c.artMgr.On("GetByDigest").Return(nil, ierror.NotFoundError(nil)) c.artMgr.On("Create").Return(1, nil) c.abstractor.On("AbstractMetadata").Return(nil) - created, art, err = c.ctl.ensureArtifact(nil, "library/hello-world", digest) + created, art, err = c.ctl.ensureArtifact(orm.NewContext(nil, &ormtesting.FakeOrmer{}), "library/hello-world", digest) c.Require().Nil(err) c.True(created) c.Equal(int64(1), art.ID) @@ -194,7 +196,7 @@ func (c *controllerTestSuite) TestEnsure() { c.artMgr.On("Create").Return(1, nil) c.abstractor.On("AbstractMetadata").Return(nil) c.tagCtl.On("Ensure").Return(nil) - _, id, err := c.ctl.Ensure(nil, "library/hello-world", digest, "latest") + _, id, err := c.ctl.Ensure(orm.NewContext(nil, &ormtesting.FakeOrmer{}), "library/hello-world", digest, "latest") c.Require().Nil(err) c.repoMgr.AssertExpectations(c.T()) c.artMgr.AssertExpectations(c.T()) @@ -370,7 +372,7 @@ func (c *controllerTestSuite) TestGetByReference() { func (c *controllerTestSuite) TestDeleteDeeply() { // root artifact and doesn't exist c.artMgr.On("Get").Return(nil, ierror.NotFoundError(nil)) - err := c.ctl.deleteDeeply(nil, 1, true) + err := c.ctl.deleteDeeply(orm.NewContext(nil, &ormtesting.FakeOrmer{}), 1, true) c.Require().NotNil(err) c.Assert().True(ierror.IsErr(err, ierror.NotFoundCode)) @@ -379,7 +381,7 @@ func (c *controllerTestSuite) TestDeleteDeeply() { // child artifact and doesn't exist c.artMgr.On("Get").Return(nil, ierror.NotFoundError(nil)) - err = c.ctl.deleteDeeply(nil, 1, false) + err = c.ctl.deleteDeeply(orm.NewContext(nil, &ormtesting.FakeOrmer{}), 1, false) c.Require().Nil(err) // reset the mock @@ -397,7 +399,7 @@ func (c *controllerTestSuite) TestDeleteDeeply() { }, nil) c.repoMgr.On("Get").Return(&models.RepoRecord{}, nil) c.artrashMgr.On("Create").Return(0, nil) - err = c.ctl.deleteDeeply(nil, 1, false) + err = c.ctl.deleteDeeply(orm.NewContext(nil, &ormtesting.FakeOrmer{}), 1, false) c.Require().Nil(err) // reset the mock @@ -412,7 +414,7 @@ func (c *controllerTestSuite) TestDeleteDeeply() { ID: 1, }, }, nil) - err = c.ctl.deleteDeeply(nil, 1, true) + err = c.ctl.deleteDeeply(orm.NewContext(nil, &ormtesting.FakeOrmer{}), 1, true) c.Require().NotNil(err) // reset the mock @@ -429,24 +431,6 @@ func (c *controllerTestSuite) TestDeleteDeeply() { }, nil) err = c.ctl.deleteDeeply(nil, 1, false) c.Require().Nil(err) - - // reset the mock - c.SetupTest() - - // root artifact is referenced by other artifacts - c.artMgr.On("Get").Return(&artifact.Artifact{ID: 1}, nil) - c.tagCtl.On("List").Return(nil, nil) - c.blobMgr.On("List", nil, mock.AnythingOfType("models.ListParams")).Return(nil, nil).Once() - c.blobMgr.On("CleanupAssociationsForProject", nil, int64(0), mock.AnythingOfType("[]*models.Blob")).Return(nil).Once() - c.repoMgr.On("Get").Return(&models.RepoRecord{}, nil) - c.artMgr.On("ListReferences").Return(nil, nil) - c.tagCtl.On("DeleteOfArtifact").Return(nil) - c.artMgr.On("Delete").Return(nil) - c.labelMgr.On("RemoveAllFrom").Return(nil) - c.artrashMgr.On("Create").Return(0, nil) - c.tagCtl.On("DeleteTags").Return(nil) - err = c.ctl.deleteDeeply(nil, 1, true) - c.Require().Nil(err) } func (c *controllerTestSuite) TestCopy() { @@ -476,7 +460,7 @@ func (c *controllerTestSuite) TestCopy() { c.artMgr.On("Create").Return(1, nil) c.regCli.On("Copy").Return(nil) c.tagCtl.On("Ensure").Return(nil) - _, err := c.ctl.Copy(nil, "library/hello-world", "latest", "library/hello-world2") + _, err := c.ctl.Copy(orm.NewContext(nil, &ormtesting.FakeOrmer{}), "library/hello-world", "latest", "library/hello-world2") c.Require().Nil(err) } diff --git a/src/api/repository/controller.go b/src/api/repository/controller.go index fd43994f4..973b7494c 100644 --- a/src/api/repository/controller.go +++ b/src/api/repository/controller.go @@ -20,6 +20,7 @@ import ( "github.com/goharbor/harbor/src/common/models" "github.com/goharbor/harbor/src/common/utils" ierror "github.com/goharbor/harbor/src/internal/error" + "github.com/goharbor/harbor/src/internal/orm" "github.com/goharbor/harbor/src/pkg/project" "github.com/goharbor/harbor/src/pkg/q" "github.com/goharbor/harbor/src/pkg/repository" @@ -66,18 +67,15 @@ type controller struct { } func (c *controller) Ensure(ctx context.Context, name string) (bool, int64, error) { - query := &q.Query{ - Keywords: map[string]interface{}{ - "name": name, - }, - } - repositories, err := c.repoMgr.List(ctx, query) - if err != nil { - return false, 0, err - } // the repository already exists, return directly - if len(repositories) > 0 { - return false, repositories[0].RepositoryID, nil + repository, err := c.repoMgr.GetByName(ctx, name) + if err == nil { + return false, repository.RepositoryID, nil + } + + // got other error when get the repository, return the error + if !ierror.IsErr(err, ierror.NotFoundCode) { + return false, 0, err } // the repository doesn't exist, create it first @@ -86,24 +84,38 @@ func (c *controller) Ensure(ctx context.Context, name string) (bool, int64, erro if err != nil { return false, 0, err } - id, err := c.repoMgr.Create(ctx, &models.RepoRecord{ - ProjectID: project.ProjectID, - Name: name, - }) - if err != nil { - // if got conflict error, try to get again - if ierror.IsConflictErr(err) { - repositories, err = c.repoMgr.List(ctx, query) - if err != nil { - return false, 0, err - } - if len(repositories) > 0 { - return false, repositories[0].RepositoryID, nil + + var ( + created bool + id int64 + ) + // use orm.WithTransaction here to avoid the issue: + // https://www.postgresql.org/message-id/002e01c04da9%24a8f95c20%2425efe6c1%40lasting.ro + if err = orm.WithTransaction(func(ctx context.Context) error { + id, err = c.repoMgr.Create(ctx, &models.RepoRecord{ + ProjectID: project.ProjectID, + Name: name, + }) + if err != nil { + // if got conflict error, try to get again + if ierror.IsConflictErr(err) { + var e error + repository, e = c.repoMgr.GetByName(ctx, name) + if e != nil { + err = e + } else { + id = repository.RepositoryID + } } + return err } + created = true + return nil + })(ctx); err != nil && !ierror.IsConflictErr(err) { return false, 0, err } - return true, id, nil + + return created, id, nil } func (c *controller) Count(ctx context.Context, query *q.Query) (int64, error) { @@ -123,7 +135,6 @@ func (c *controller) GetByName(ctx context.Context, name string) (*models.RepoRe } func (c *controller) Delete(ctx context.Context, id int64) error { - // TODO auth // TODO how to make sure the logic included by middlewares(immutable, readonly, quota, etc) // TODO is covered when deleting the artifacts of the repository artifacts, err := c.artCtl.List(ctx, &q.Query{ diff --git a/src/api/repository/controller_test.go b/src/api/repository/controller_test.go index 0b1046a08..211cca9c0 100644 --- a/src/api/repository/controller_test.go +++ b/src/api/repository/controller_test.go @@ -15,15 +15,17 @@ package repository import ( - "testing" - "github.com/goharbor/harbor/src/api/artifact" "github.com/goharbor/harbor/src/common/models" + ierror "github.com/goharbor/harbor/src/internal/error" + "github.com/goharbor/harbor/src/internal/orm" artifacttesting "github.com/goharbor/harbor/src/testing/api/artifact" + ormtesting "github.com/goharbor/harbor/src/testing/lib/orm" "github.com/goharbor/harbor/src/testing/mock" "github.com/goharbor/harbor/src/testing/pkg/project" "github.com/goharbor/harbor/src/testing/pkg/repository" "github.com/stretchr/testify/suite" + "testing" ) type controllerTestSuite struct { @@ -47,12 +49,10 @@ func (c *controllerTestSuite) SetupTest() { func (c *controllerTestSuite) TestEnsure() { // already exists - c.repoMgr.On("List").Return([]*models.RepoRecord{ - { - RepositoryID: 1, - ProjectID: 1, - Name: "library/hello-world", - }, + c.repoMgr.On("GetByName").Return(&models.RepoRecord{ + RepositoryID: 1, + ProjectID: 1, + Name: "library/hello-world", }, nil) created, id, err := c.ctl.Ensure(nil, "library/hello-world") c.Require().Nil(err) @@ -64,12 +64,12 @@ func (c *controllerTestSuite) TestEnsure() { c.SetupTest() // doesn't exist - c.repoMgr.On("List").Return([]*models.RepoRecord{}, nil) + c.repoMgr.On("GetByName").Return(nil, ierror.NotFoundError(nil)) c.proMgr.On("Get", "library").Return(&models.Project{ ProjectID: 1, }, nil) c.repoMgr.On("Create").Return(1, nil) - created, id, err = c.ctl.Ensure(nil, "library/hello-world") + created, id, err = c.ctl.Ensure(orm.NewContext(nil, &ormtesting.FakeOrmer{}), "library/hello-world") c.Require().Nil(err) c.repoMgr.AssertExpectations(c.T()) c.proMgr.AssertExpectations(c.T()) diff --git a/src/api/tag/controller.go b/src/api/tag/controller.go index 34ff40086..d152d9345 100644 --- a/src/api/tag/controller.go +++ b/src/api/tag/controller.go @@ -5,6 +5,7 @@ import ( "github.com/goharbor/harbor/src/common/utils" "github.com/goharbor/harbor/src/common/utils/log" ierror "github.com/goharbor/harbor/src/internal/error" + "github.com/goharbor/harbor/src/internal/orm" "github.com/goharbor/harbor/src/pkg/artifact" "github.com/goharbor/harbor/src/pkg/artifactselector" "github.com/goharbor/harbor/src/pkg/immutabletag/match" @@ -88,18 +89,23 @@ func (c *controller) Ensure(ctx context.Context, repositoryID, artifactID int64, tag.PushTime = time.Now() return c.Update(ctx, tag, "ArtifactID", "PushTime") } + // the tag doesn't exist under the repository, create it - tag := &Tag{} - tag.RepositoryID = repositoryID - tag.ArtifactID = artifactID - tag.Name = name - tag.PushTime = time.Now() - _, err = c.Create(ctx, tag) - // ignore the conflict error - if err != nil && ierror.IsConflictErr(err) { - return nil + // use orm.WithTransaction here to avoid the issue: + // https://www.postgresql.org/message-id/002e01c04da9%24a8f95c20%2425efe6c1%40lasting.ro + if err = orm.WithTransaction(func(ctx context.Context) error { + tag := &Tag{} + tag.RepositoryID = repositoryID + tag.ArtifactID = artifactID + tag.Name = name + tag.PushTime = time.Now() + _, err = c.Create(ctx, tag) + return err + })(ctx); err != nil && !ierror.IsConflictErr(err) { + return err } - return err + + return nil } // Count ... diff --git a/src/api/tag/controller_test.go b/src/api/tag/controller_test.go index ea987a1d9..99a0eaffd 100644 --- a/src/api/tag/controller_test.go +++ b/src/api/tag/controller_test.go @@ -4,8 +4,10 @@ import ( "github.com/goharbor/harbor/src/common" coreConfig "github.com/goharbor/harbor/src/core/config" ierror "github.com/goharbor/harbor/src/internal/error" + "github.com/goharbor/harbor/src/internal/orm" pkg_artifact "github.com/goharbor/harbor/src/pkg/artifact" "github.com/goharbor/harbor/src/pkg/tag/model/tag" + ormtesting "github.com/goharbor/harbor/src/testing/lib/orm" "github.com/goharbor/harbor/src/testing/pkg/artifact" immutesting "github.com/goharbor/harbor/src/testing/pkg/immutabletag" "github.com/goharbor/harbor/src/testing/pkg/repository" @@ -55,7 +57,7 @@ func (c *controllerTestSuite) TestEnsureTag() { ID: 1, }, nil) c.immutableMtr.On("Match").Return(false, nil) - err := c.ctl.Ensure(nil, 1, 1, "latest") + err := c.ctl.Ensure(orm.NewContext(nil, &ormtesting.FakeOrmer{}), 1, 1, "latest") c.Require().Nil(err) c.tagMgr.AssertExpectations(c.T()) @@ -76,7 +78,7 @@ func (c *controllerTestSuite) TestEnsureTag() { ID: 1, }, nil) c.immutableMtr.On("Match").Return(false, nil) - err = c.ctl.Ensure(nil, 1, 1, "latest") + err = c.ctl.Ensure(orm.NewContext(nil, &ormtesting.FakeOrmer{}), 1, 1, "latest") c.Require().Nil(err) c.tagMgr.AssertExpectations(c.T()) @@ -90,7 +92,7 @@ func (c *controllerTestSuite) TestEnsureTag() { ID: 1, }, nil) c.immutableMtr.On("Match").Return(false, nil) - err = c.ctl.Ensure(nil, 1, 1, "latest") + err = c.ctl.Ensure(orm.NewContext(nil, &ormtesting.FakeOrmer{}), 1, 1, "latest") c.Require().Nil(err) c.tagMgr.AssertExpectations(c.T()) } diff --git a/src/testing/lib/orm/orm.go b/src/testing/lib/orm/orm.go new file mode 100644 index 000000000..aa5d262bd --- /dev/null +++ b/src/testing/lib/orm/orm.go @@ -0,0 +1,119 @@ +// Copyright Project Harbor Authors +// +// Licensed 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 orm + +import ( + "context" + "database/sql" + "github.com/astaxie/beego/orm" +) + +// FakeOrmer ... +type FakeOrmer struct{} + +// Read ... +func (f *FakeOrmer) Read(md interface{}, cols ...string) error { + return nil +} + +// ReadForUpdate ... +func (f *FakeOrmer) ReadForUpdate(md interface{}, cols ...string) error { + return nil +} + +// ReadOrCreate ... +func (f *FakeOrmer) ReadOrCreate(md interface{}, col1 string, cols ...string) (bool, int64, error) { + return false, 0, nil +} + +// Insert ... +func (f *FakeOrmer) Insert(interface{}) (int64, error) { + return 0, nil +} + +// InsertOrUpdate ... +func (f *FakeOrmer) InsertOrUpdate(md interface{}, colConflitAndArgs ...string) (int64, error) { + return 0, nil +} + +// InsertMulti ... +func (f *FakeOrmer) InsertMulti(bulk int, mds interface{}) (int64, error) { + return 0, nil +} + +// Update ... +func (f *FakeOrmer) Update(md interface{}, cols ...string) (int64, error) { + return 0, nil +} + +// Delete ... +func (f *FakeOrmer) Delete(md interface{}, cols ...string) (int64, error) { + return 0, nil +} + +// LoadRelated ... +func (f *FakeOrmer) LoadRelated(md interface{}, name string, args ...interface{}) (int64, error) { + return 0, nil +} + +// QueryM2M ... +func (f *FakeOrmer) QueryM2M(md interface{}, name string) orm.QueryM2Mer { + return nil +} + +// QueryTable ... +func (f *FakeOrmer) QueryTable(ptrStructOrTableName interface{}) orm.QuerySeter { + return nil +} + +// Using ... +func (f *FakeOrmer) Using(name string) error { + return nil +} + +// Begin ... +func (f *FakeOrmer) Begin() error { + return nil +} + +// BeginTx ... +func (f *FakeOrmer) BeginTx(ctx context.Context, opts *sql.TxOptions) error { + return nil +} + +// Commit ... +func (f *FakeOrmer) Commit() error { + return nil +} + +// Rollback ... +func (f *FakeOrmer) Rollback() error { + return nil +} + +// Raw ... +func (f *FakeOrmer) Raw(query string, args ...interface{}) orm.RawSeter { + return nil +} + +// Driver ... +func (f *FakeOrmer) Driver() orm.Driver { + return nil +} + +// DBStats ... +func (f *FakeOrmer) DBStats() *sql.DBStats { + return nil +}