Fix transaction issue

More detail: // https://www.postgresql.org/message-id/002e01c04da9%24a8f95c20%2425efe6c1%40lasting.ro Signed-off-by: Wenkai Yin <yinw@vmware.com>
2020-02-28 16:33:19 +08:00 · 2020-02-28 16:33:19 +08:00 · e45eaeec74
parent 69119b6410
commit e45eaeec74
7 changed files with 230 additions and 92 deletions
--- 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
--- 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)
 }

--- 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{
--- 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())
--- 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 ...
--- 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())
 }
--- a/src/testing/lib/orm/orm.go
+++ 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
+}