From 0e175017aa9c31c6a68f94b0e3e327acf26d2e8a Mon Sep 17 00:00:00 2001 From: wang yan Date: Tue, 23 Jun 2020 13:00:29 +0800 Subject: [PATCH] continue updating code per review comments Signed-off-by: wang yan --- src/controller/blob/controller.go | 17 +++++++++++---- src/controller/blob/controller_test.go | 21 +++++++++++++------ src/pkg/blob/dao/dao.go | 17 +++++++++++---- src/pkg/blob/dao/dao_test.go | 4 ++-- src/server/middleware/blob/head_blob.go | 22 +++++++++----------- src/server/middleware/blob/head_blob_test.go | 5 +++-- src/testing/controller/blob/controller.go | 17 +++++---------- 7 files changed, 61 insertions(+), 42 deletions(-) diff --git a/src/controller/blob/controller.go b/src/controller/blob/controller.go index 817a2912c..2444cc59a 100644 --- a/src/controller/blob/controller.go +++ b/src/controller/blob/controller.go @@ -24,6 +24,7 @@ import ( "github.com/goharbor/harbor/src/lib/log" "github.com/goharbor/harbor/src/lib/orm" "github.com/goharbor/harbor/src/pkg/blob" + blob_models "github.com/goharbor/harbor/src/pkg/blob/models" ) var ( @@ -74,8 +75,8 @@ type Controller interface { // GetAcceptedBlobSize returns the accepted size of stream upload blob. GetAcceptedBlobSize(sessionID string) (int64, error) - // Touch updates the blob status and increase version every time. - Touch(ctx context.Context, blob *blob.Blob) (int64, error) + // Touch updates the blob status to StatusNone and increase version every time. + Touch(ctx context.Context, blob *blob.Blob) error // Update updates the blob, it cannot handle blob status transitions. Update(ctx context.Context, blob *blob.Blob) error @@ -322,8 +323,16 @@ func (c *controller) GetAcceptedBlobSize(sessionID string) (int64, error) { return size, nil } -func (c *controller) Touch(ctx context.Context, blob *blob.Blob) (int64, error) { - return c.blobMgr.UpdateBlobStatus(ctx, blob) +func (c *controller) Touch(ctx context.Context, blob *blob.Blob) error { + blob.Status = blob_models.StatusNone + count, err := c.blobMgr.UpdateBlobStatus(ctx, blob) + if err != nil { + return err + } + if count == 0 { + return errors.New(nil).WithMessage(fmt.Sprintf("no blob item is updated to StatusNone, id:%d, digest:%s", blob.ID, blob.Digest)).WithCode(errors.NotFoundCode) + } + return nil } func (c *controller) Update(ctx context.Context, blob *blob.Blob) error { diff --git a/src/controller/blob/controller_test.go b/src/controller/blob/controller_test.go index e33c2c331..b647cfec7 100644 --- a/src/controller/blob/controller_test.go +++ b/src/controller/blob/controller_test.go @@ -17,6 +17,7 @@ package blob import ( "context" "fmt" + "github.com/goharbor/harbor/src/lib/errors" "github.com/goharbor/harbor/src/pkg/blob/models" "testing" @@ -28,6 +29,8 @@ import ( blobtesting "github.com/goharbor/harbor/src/testing/pkg/blob" "github.com/google/uuid" "github.com/stretchr/testify/suite" + + pkg_blob "github.com/goharbor/harbor/src/pkg/blob" ) type ControllerTestSuite struct { @@ -268,19 +271,25 @@ func (suite *ControllerTestSuite) TestGetSetAcceptedBlobSize() { suite.Equal(int64(100), size) } -func (suite *ControllerTestSuite) TestUpdateStatus() { +func (suite *ControllerTestSuite) TestTouch() { ctx := suite.Context() + err := Ctl.Touch(ctx, &blob.Blob{ + Status: models.StatusNone, + }) + suite.NotNil(err) + suite.True(errors.IsNotFoundErr(err)) + digest := suite.prepareBlob() blob, err := Ctl.Get(ctx, digest) suite.Nil(err) - - suite.Equal(blob.Status, models.StatusNone) blob.Status = models.StatusDelete - count, err := Ctl.Touch(ctx, blob) + _, err = pkg_blob.Mgr.UpdateBlobStatus(suite.Context(), blob) suite.Nil(err) - suite.Equal(blob.Status, models.StatusDelete) - suite.Equal(int64(1), count) + + err = Ctl.Touch(ctx, blob) + suite.Nil(err) + suite.Equal(blob.Status, models.StatusNone) } func (suite *ControllerTestSuite) TestDelete() { diff --git a/src/pkg/blob/dao/dao.go b/src/pkg/blob/dao/dao.go index b2b5dd6ff..08a50d1f7 100644 --- a/src/pkg/blob/dao/dao.go +++ b/src/pkg/blob/dao/dao.go @@ -18,6 +18,7 @@ import ( "context" "fmt" "github.com/goharbor/harbor/src/lib/errors" + "github.com/goharbor/harbor/src/lib/log" "strings" "time" @@ -200,20 +201,28 @@ func (d *dao) UpdateBlobStatus(ctx context.Context, blob *models.Blob) (int64, e WHERE "id" IN ( SELECT T0."id" FROM "blob" T0 WHERE T0."version" >= $4 AND T0."id" = $5 AND T0."status" IN ('delete', 'deleting') ) */ - return qt.SetCond(c).Filter("id", blob.ID). + count, err := qt.SetCond(c).Filter("id", blob.ID). Filter("status__in", models.StatusMap[blob.Status]). Update(data) + if err != nil { + return count, err + } + if count == 0 { + log.Warningf("no blob is updated according to query condition, id: %d, status_in, %v", blob.ID, models.StatusMap[blob.Status]) + return 0, nil + } + return count, nil } -// UpdateBlob cannot handle the status change. +// UpdateBlob cannot handle the status change and version increase, for handling blob status change, please call +// for the UpdateBlobStatus. func (d *dao) UpdateBlob(ctx context.Context, blob *models.Blob) error { o, err := orm.FromContext(ctx) if err != nil { return err } - blob.Version = blob.Version + 1 blob.UpdateTime = time.Now() - _, err = o.Update(blob, "size", "content_type", "version", "update_time") + _, err = o.Update(blob, "size", "content_type", "update_time") return err } diff --git a/src/pkg/blob/dao/dao_test.go b/src/pkg/blob/dao/dao_test.go index 023394ab6..e2bce29a1 100644 --- a/src/pkg/blob/dao/dao_test.go +++ b/src/pkg/blob/dao/dao_test.go @@ -139,7 +139,7 @@ func (suite *DaoTestSuite) TestUpdateBlob() { blob, err := suite.dao.GetBlobByDigest(ctx, digest) if suite.Nil(err) { suite.Equal(int64(100), blob.Size) - suite.Equal(int64(1), blob.Version) + suite.Equal(int64(0), blob.Version) } } @@ -147,7 +147,7 @@ func (suite *DaoTestSuite) TestUpdateBlob() { suite.Nil(suite.dao.UpdateBlob(ctx, blob), "cannot be updated.") blob, err = suite.dao.GetBlobByDigest(ctx, digest) if suite.Nil(err) { - suite.Equal(int64(2), blob.Version) + suite.Equal(int64(0), blob.Version) suite.Equal(models.StatusNone, blob.Status) } } diff --git a/src/server/middleware/blob/head_blob.go b/src/server/middleware/blob/head_blob.go index ca05725b5..b791a9f4f 100644 --- a/src/server/middleware/blob/head_blob.go +++ b/src/server/middleware/blob/head_blob.go @@ -9,6 +9,7 @@ import ( "github.com/goharbor/harbor/src/lib/log" blob_models "github.com/goharbor/harbor/src/pkg/blob/models" "github.com/goharbor/harbor/src/server/middleware" + "github.com/goharbor/harbor/src/server/middleware/requestid" "net/http" ) @@ -26,29 +27,26 @@ func HeadBlobMiddleware() func(http.Handler) http.Handler { // handleHead ... func handleHead(req *http.Request) error { none := lib.ArtifactInfo{} - art := lib.GetArtifactInfo(req.Context()) - if art == none { - return errors.New("cannot get the artifact information from request context").WithCode(errors.NotFoundCode) + // for head blob, the GetArtifactInfo is actually get the information of blob. + blobInfo := lib.GetArtifactInfo(req.Context()) + if blobInfo == none { + return errors.New("cannot get the blob information from request context").WithCode(errors.NotFoundCode) } - bb, err := blob.Ctl.Get(req.Context(), art.Digest) + bb, err := blob.Ctl.Get(req.Context(), blobInfo.Digest) if err != nil { return err } switch bb.Status { case blob_models.StatusNone, blob_models.StatusDelete: - bb.Status = blob_models.StatusNone - count, err := blob.Ctl.Touch(req.Context(), bb) + err := blob.Ctl.Touch(req.Context(), bb) if err != nil { - log.Errorf("failed to update blob: %s status to None, error:%v", art.Digest, err) - return err - } - if count == 0 { - return errors.New("the asking blob is in GC, mark it as non existing").WithCode(errors.NotFoundCode) + log.Errorf("failed to update blob: %s status to StatusNone, error:%v", blobInfo.Digest, err) + return errors.Wrapf(err, fmt.Sprintf("the request id is: %s", req.Header.Get(requestid.HeaderXRequestID))) } case blob_models.StatusDeleting, blob_models.StatusDeleteFailed: - return errors.New("the asking blob is in GC, mark it as non existing").WithCode(errors.NotFoundCode) + return errors.New(nil).WithMessage(fmt.Sprintf("the asking blob is in GC, mark it as non existing, request id: %s", req.Header.Get(requestid.HeaderXRequestID))).WithCode(errors.NotFoundCode) default: return errors.New(nil).WithMessage(fmt.Sprintf("wrong blob status, %s", bb.Status)) } diff --git a/src/server/middleware/blob/head_blob_test.go b/src/server/middleware/blob/head_blob_test.go index 057fe592b..19787c8f0 100644 --- a/src/server/middleware/blob/head_blob_test.go +++ b/src/server/middleware/blob/head_blob_test.go @@ -6,6 +6,7 @@ import ( "github.com/goharbor/harbor/src/controller/blob" "github.com/goharbor/harbor/src/lib" "github.com/goharbor/harbor/src/lib/orm" + pkg_blob "github.com/goharbor/harbor/src/pkg/blob" blob_models "github.com/goharbor/harbor/src/pkg/blob/models" htesting "github.com/goharbor/harbor/src/testing" "github.com/stretchr/testify/suite" @@ -64,9 +65,9 @@ func (suite *HeadBlobUploadMiddlewareTestSuite) TestHeadBlobStatusDeleting() { suite.Nil(err) // status-none -> status-delete -> status-deleting - _, err = blob.Ctl.Touch(suite.Context(), &blob_models.Blob{ID: id, Status: blob_models.StatusDelete}) + _, err = pkg_blob.Mgr.UpdateBlobStatus(suite.Context(), &blob_models.Blob{ID: id, Status: blob_models.StatusDelete}) suite.Nil(err) - _, err = blob.Ctl.Touch(suite.Context(), &blob_models.Blob{ID: id, Status: blob_models.StatusDeleting, Version: 1}) + _, err = pkg_blob.Mgr.UpdateBlobStatus(suite.Context(), &blob_models.Blob{ID: id, Status: blob_models.StatusDeleting, Version: 1}) suite.Nil(err) req := suite.NewRequest(http.MethodHead, fmt.Sprintf("/v2/%s/blobs/%s", projectName, digest), nil) diff --git a/src/testing/controller/blob/controller.go b/src/testing/controller/blob/controller.go index 1c16eb32f..d0bb69743 100644 --- a/src/testing/controller/blob/controller.go +++ b/src/testing/controller/blob/controller.go @@ -271,24 +271,17 @@ func (_m *Controller) Sync(ctx context.Context, references []distribution.Descri } // Touch provides a mock function with given fields: ctx, _a1 -func (_m *Controller) Touch(ctx context.Context, _a1 *models.Blob) (int64, error) { +func (_m *Controller) Touch(ctx context.Context, _a1 *models.Blob) error { ret := _m.Called(ctx, _a1) - var r0 int64 - if rf, ok := ret.Get(0).(func(context.Context, *models.Blob) int64); ok { + var r0 error + if rf, ok := ret.Get(0).(func(context.Context, *models.Blob) error); ok { r0 = rf(ctx, _a1) } else { - r0 = ret.Get(0).(int64) + r0 = ret.Error(0) } - var r1 error - if rf, ok := ret.Get(1).(func(context.Context, *models.Blob) error); ok { - r1 = rf(ctx, _a1) - } else { - r1 = ret.Error(1) - } - - return r0, r1 + return r0 } // Update provides a mock function with given fields: ctx, _a1