From 0373e08e582687a33f654a06d22da28ca2d8deee Mon Sep 17 00:00:00 2001 From: Wenkai Yin Date: Sat, 25 Jan 2020 15:26:44 +0800 Subject: [PATCH] Add query string to listing artifact API to support specify only show the tagged artifacts Specify the query string "tags=NOT_NULL" to the listing artifact API to only return the tagged artifacts Signed-off-by: Wenkai Yin --- api/v2.0/swagger.yaml | 7 + src/pkg/artifact/dao/dao.go | 56 ++++- src/pkg/artifact/dao/dao_test.go | 330 ++++++++++++++++++++-------- src/pkg/q/query.go | 16 ++ src/pkg/q/query_test.go | 46 ++++ src/server/v2.0/handler/artifact.go | 3 + 6 files changed, 368 insertions(+), 90 deletions(-) create mode 100644 src/pkg/q/query_test.go diff --git a/api/v2.0/swagger.yaml b/api/v2.0/swagger.yaml index 49fda123d..bb9de5d1f 100644 --- a/api/v2.0/swagger.yaml +++ b/api/v2.0/swagger.yaml @@ -34,6 +34,11 @@ paths: description: Query the artifacts by type. Valid values can be "IMAGE", "CHART", etc. type: string required: false + - name: tags + in: query + description: Without the "tags" query string, both tagged and untagged artifacts will be returned. Specify it as "*" to return only tagged artifacts; specify it as "nil" to return only untagged artifacts. + type: string + required: false - name: with_tag in: query description: Specify whether the tags are inclued inside the returning artifacts @@ -102,6 +107,8 @@ paths: - $ref: '#/parameters/projectName' - $ref: '#/parameters/repositoryName' - $ref: '#/parameters/reference' + - $ref: '#/parameters/page' + - $ref: '#/parameters/pageSize' - name: with_tag in: query description: Specify whether the tags are inclued inside the returning artifacts diff --git a/src/pkg/artifact/dao/dao.go b/src/pkg/artifact/dao/dao.go index 9e50a9b4e..a8268d56b 100644 --- a/src/pkg/artifact/dao/dao.go +++ b/src/pkg/artifact/dao/dao.go @@ -16,6 +16,7 @@ package dao import ( "context" + beegoorm "github.com/astaxie/beego/orm" ierror "github.com/goharbor/harbor/src/internal/error" "github.com/goharbor/harbor/src/internal/orm" "github.com/goharbor/harbor/src/pkg/q" @@ -43,6 +44,27 @@ type DAO interface { DeleteReferences(ctx context.Context, parentID int64) (err error) } +const ( + // TODO replace the table name "artifact_2" after upgrade + // both tagged and untagged artifacts + all = `IN ( + SELECT DISTINCT art.id FROM artifact_2 art + LEFT JOIN tag ON art.id=tag.artifact_id + LEFT JOIN artifact_reference ref ON art.id=ref.child_id + WHERE tag.id IS NOT NULL OR ref.id IS NULL)` + // only untagged artifacts + untagged = `IN ( + SELECT DISTINCT art.id FROM artifact_2 art + LEFT JOIN tag ON art.id=tag.artifact_id + LEFT JOIN artifact_reference ref ON art.id=ref.child_id + WHERE tag.id IS NULL AND ref.id IS NULL)` + // only tagged artifacts + tagged = `IN ( + SELECT DISTINCT art.id FROM artifact_2 art + LEFT JOIN tag ON art.id=tag.artifact_id + WHERE tag.id IS NOT NULL)` +) + // New returns an instance of the default DAO func New() DAO { return &dao{} @@ -57,7 +79,7 @@ func (d *dao) Count(ctx context.Context, query *q.Query) (int64, error) { Keywords: query.Keywords, } } - qs, err := orm.QuerySetter(ctx, &Artifact{}, query) + qs, err := querySetter(ctx, query) if err != nil { return 0, err } @@ -65,7 +87,7 @@ func (d *dao) Count(ctx context.Context, query *q.Query) (int64, error) { } func (d *dao) List(ctx context.Context, query *q.Query) ([]*Artifact, error) { artifacts := []*Artifact{} - qs, err := orm.QuerySetter(ctx, &Artifact{}, query) + qs, err := querySetter(ctx, query) if err != nil { return nil, err } @@ -181,3 +203,33 @@ func (d *dao) DeleteReferences(ctx context.Context, parentID int64) error { _, err = qs.Delete() return err } + +func querySetter(ctx context.Context, query *q.Query) (beegoorm.QuerySeter, error) { + // as we modify the query in this method, copy it to avoid the impact for the original one + query = q.Copy(query) + // show both tagged and untagged artifacts by default + rawFilter := all + if query != nil && len(query.Keywords) > 0 { + value, ok := query.Keywords["Tags"] + if ok { + switch value.(string) { + case "nil": + // only show untagged artifacts + rawFilter = untagged + case "*": + // only show tagged artifacts + rawFilter = tagged + default: + return nil, ierror.New(nil).WithCode(ierror.BadRequestCode). + WithMessage("invalid value of tags: %s", value.(string)) + } + // as the "Tags" isn't a table column, remove the "Tags" from the query to avoid orm error + delete(query.Keywords, "Tags") + } + } + qs, err := orm.QuerySetter(ctx, &Artifact{}, query) + if err != nil { + return nil, err + } + return qs.FilterRaw("id", rawFilter), nil +} diff --git a/src/pkg/artifact/dao/dao_test.go b/src/pkg/artifact/dao/dao_test.go index 4ef46c0b1..ad37d2f39 100644 --- a/src/pkg/artifact/dao/dao_test.go +++ b/src/pkg/artifact/dao/dao_test.go @@ -21,109 +21,176 @@ import ( ierror "github.com/goharbor/harbor/src/internal/error" "github.com/goharbor/harbor/src/internal/orm" "github.com/goharbor/harbor/src/pkg/q" + tagdao "github.com/goharbor/harbor/src/pkg/tag/dao" + "github.com/goharbor/harbor/src/pkg/tag/model/tag" + v1 "github.com/opencontainers/image-spec/specs-go/v1" "github.com/stretchr/testify/suite" "testing" "time" ) -var ( - typee = "IMAGE" - mediaType = "application/vnd.oci.image.config.v1+json" - manifestMediaType = "application/vnd.oci.image.manifest.v1+json" - projectID int64 = 1 - repositoryID int64 = 1 - digest = "sha256:418fb88ec412e340cdbef913b8ca1bbe8f9e8dc705f9617414c1f2c8db980180" -) - type daoTestSuite struct { suite.Suite - dao DAO - artifactID int64 - referenceID int64 - ctx context.Context + dao DAO + tagDAO tagdao.DAO + parentArtID int64 + childArt01ID int64 + childArt02ID int64 + reference01ID int64 + reference02ID int64 + tagID int64 + ctx context.Context } func (d *daoTestSuite) SetupSuite() { d.dao = New() + d.tagDAO = tagdao.New() common_dao.PrepareTestForPostgresSQL() d.ctx = orm.NewContext(nil, beegoorm.NewOrm()) } func (d *daoTestSuite) SetupTest() { - artifact := &Artifact{ - Type: typee, - MediaType: mediaType, - ManifestMediaType: manifestMediaType, - ProjectID: projectID, - RepositoryID: repositoryID, - Digest: digest, - Size: 1024, - PushTime: time.Now(), - PullTime: time.Now(), - ExtraAttrs: `{"attr1":"value1"}`, + now := time.Now() + parentArt := &Artifact{ + Type: "IMAGE", + MediaType: v1.MediaTypeImageConfig, + ManifestMediaType: v1.MediaTypeImageIndex, + ProjectID: 1, + RepositoryID: 1, + Digest: "parent_digest", + PushTime: now, + PullTime: now, Annotations: `{"anno1":"value1"}`, } - id, err := d.dao.Create(d.ctx, artifact) + id, err := d.dao.Create(d.ctx, parentArt) d.Require().Nil(err) - d.artifactID = id + d.parentArtID = id + + childArt01 := &Artifact{ + Type: "IMAGE", + MediaType: v1.MediaTypeImageConfig, + ManifestMediaType: v1.MediaTypeImageManifest, + ProjectID: 1, + RepositoryID: 1, + Digest: "child_digest_01", + Size: 1024, + PushTime: now, + PullTime: now, + ExtraAttrs: `{"attr1":"value1"}`, + } + id, err = d.dao.Create(d.ctx, childArt01) + d.Require().Nil(err) + d.childArt01ID = id + + childArt02 := &Artifact{ + Type: "IMAGE", + MediaType: v1.MediaTypeImageConfig, + ManifestMediaType: v1.MediaTypeImageManifest, + ProjectID: 1, + RepositoryID: 1, + Digest: "child_digest_02", + Size: 1024, + PushTime: now, + PullTime: now, + ExtraAttrs: `{"attr1":"value1"}`, + } + id, err = d.dao.Create(d.ctx, childArt02) + d.Require().Nil(err) + d.childArt02ID = id id, err = d.dao.CreateReference(d.ctx, &ArtifactReference{ - ParentID: d.artifactID, - ChildID: d.artifactID, + ParentID: d.parentArtID, + ChildID: d.childArt01ID, }) d.Require().Nil(err) - d.referenceID = id + d.reference01ID = id + + id, err = d.dao.CreateReference(d.ctx, &ArtifactReference{ + ParentID: d.parentArtID, + ChildID: d.childArt02ID, + }) + d.Require().Nil(err) + d.reference02ID = id + + id, err = d.tagDAO.Create(d.ctx, &tag.Tag{ + RepositoryID: 1, + ArtifactID: d.childArt01ID, + Name: "latest", + PushTime: now, + PullTime: now, + }) + d.Require().Nil(err) + d.tagID = id } func (d *daoTestSuite) TearDownTest() { - err := d.dao.DeleteReferences(d.ctx, d.artifactID) + err := d.dao.DeleteReferences(d.ctx, d.parentArtID) d.Require().Nil(err) - err = d.dao.Delete(d.ctx, d.artifactID) + d.tagDAO.Delete(d.ctx, d.tagID) + d.Require().Nil(err) + err = d.dao.Delete(d.ctx, d.childArt01ID) + d.Require().Nil(err) + err = d.dao.Delete(d.ctx, d.childArt02ID) + d.Require().Nil(err) + err = d.dao.Delete(d.ctx, d.parentArtID) d.Require().Nil(err) } func (d *daoTestSuite) TestCount() { - // nil query - total, err := d.dao.Count(d.ctx, nil) + // query by repository ID: both tagged and untagged artifacts + totalOfAll, err := d.dao.Count(d.ctx, &q.Query{ + Keywords: map[string]interface{}{ + "RepositoryID": 1, + }, + }) d.Require().Nil(err) - d.True(total > 0) + d.True(totalOfAll >= 2) + + // only query tagged artifacts + totalOfTagged, err := d.dao.Count(d.ctx, &q.Query{ + Keywords: map[string]interface{}{ + "RepositoryID": 1, + "Tags": "*", + }, + }) + d.Require().Nil(err) + d.Equal(totalOfAll-1, totalOfTagged) + + // only query untagged artifacts + totalOfUnTagged, err := d.dao.Count(d.ctx, &q.Query{ + Keywords: map[string]interface{}{ + "RepositoryID": 1, + "Tags": "nil", + }, + }) + d.Require().Nil(err) + d.Equal(totalOfAll-1, totalOfUnTagged) + + // invalid tags value + _, err = d.dao.Count(d.ctx, &q.Query{ + Keywords: map[string]interface{}{ + "RepositoryID": 1, + "Tags": "invalid_value", + }, + }) + d.Require().NotNil(err) + d.True(ierror.IsErr(err, ierror.BadRequestCode)) // query by repository ID and digest - total, err = d.dao.Count(d.ctx, &q.Query{ + total, err := d.dao.Count(d.ctx, &q.Query{ Keywords: map[string]interface{}{ - "repository_id": repositoryID, - "digest": digest, + "RepositoryID": 1, + "Digest": "parent_digest", }, }) d.Require().Nil(err) d.Equal(int64(1), total) - // query by repository ID and digest - total, err = d.dao.Count(d.ctx, &q.Query{ - Keywords: map[string]interface{}{ - "repository_id": repositoryID, - "digest": digest, - }, - }) - d.Require().Nil(err) - d.Equal(int64(1), total) - - // populate more data - id, err := d.dao.Create(d.ctx, &Artifact{ - Type: typee, - MediaType: mediaType, - ManifestMediaType: manifestMediaType, - ProjectID: projectID, - RepositoryID: repositoryID, - Digest: "sha256:digest", - }) - d.Require().Nil(err) - defer func() { - err = d.dao.Delete(d.ctx, id) - d.Require().Nil(err) - }() // set pagination in query total, err = d.dao.Count(d.ctx, &q.Query{ + Keywords: map[string]interface{}{ + "RepositoryID": 1, + }, PageNumber: 1, PageSize: 1, }) @@ -132,28 +199,114 @@ func (d *daoTestSuite) TestCount() { } func (d *daoTestSuite) TestList() { - // nil query - artifacts, err := d.dao.List(d.ctx, nil) + // query by repository ID: both tagged and untagged artifacts + artifacts, err := d.dao.List(d.ctx, &q.Query{ + Keywords: map[string]interface{}{ + "RepositoryID": 1, + }, + }) d.Require().Nil(err) - found := false + + parentArtFound := false + childArt01Found := false + childArt02Found := false for _, artifact := range artifacts { - if artifact.ID == d.artifactID { - found = true - break + if artifact.ID == d.parentArtID { + parentArtFound = true + continue + } + if artifact.ID == d.childArt01ID { + childArt01Found = true + continue + } + if artifact.ID == d.childArt02ID { + childArt02Found = true + continue } } - d.True(found) + d.True(parentArtFound) + d.True(childArt01Found) + d.False(childArt02Found) + + // only query tagged artifacts + artifacts, err = d.dao.List(d.ctx, &q.Query{ + Keywords: map[string]interface{}{ + "RepositoryID": 1, + "Tags": "*", + }, + }) + d.Require().Nil(err) + parentArtFound = false + childArt01Found = false + childArt02Found = false + for _, artifact := range artifacts { + if artifact.ID == d.parentArtID { + parentArtFound = true + continue + } + if artifact.ID == d.childArt01ID { + childArt01Found = true + continue + } + if artifact.ID == d.childArt02ID { + childArt02Found = true + continue + } + } + d.False(parentArtFound) + d.True(childArt01Found) + d.False(childArt02Found) + + // only query untagged artifacts + artifacts, err = d.dao.List(d.ctx, &q.Query{ + Keywords: map[string]interface{}{ + "RepositoryID": 1, + "Tags": "nil", + }, + }) + d.Require().Nil(err) + parentArtFound = false + childArt01Found = false + childArt02Found = false + for _, artifact := range artifacts { + if artifact.ID == d.parentArtID { + parentArtFound = true + continue + } + if artifact.ID == d.childArt01ID { + childArt01Found = true + continue + } + if artifact.ID == d.childArt02ID { + childArt02Found = true + continue + } + } + d.True(parentArtFound) + d.False(childArt01Found) + d.False(childArt02Found) // query by repository ID and digest artifacts, err = d.dao.List(d.ctx, &q.Query{ Keywords: map[string]interface{}{ - "repository_id": repositoryID, - "digest": digest, + "RepositoryID": 1, + "Digest": "parent_digest", }, }) d.Require().Nil(err) - d.Require().Equal(1, len(artifacts)) - d.Equal(d.artifactID, artifacts[0].ID) + d.Require().Len(artifacts, 1) + d.Equal(d.parentArtID, artifacts[0].ID) + + // set pagination in query + artifacts, err = d.dao.List(d.ctx, &q.Query{ + Keywords: map[string]interface{}{ + "RepositoryID": 1, + }, + PageNumber: 1, + PageSize: 1, + }) + d.Require().Nil(err) + d.Require().Len(artifacts, 1) } func (d *daoTestSuite) TestGet() { @@ -163,10 +316,10 @@ func (d *daoTestSuite) TestGet() { d.True(ierror.IsErr(err, ierror.NotFoundCode)) // get the exist artifact - artifact, err := d.dao.Get(d.ctx, d.artifactID) + artifact, err := d.dao.Get(d.ctx, d.parentArtID) d.Require().Nil(err) d.Require().NotNil(artifact) - d.Equal(d.artifactID, artifact.ID) + d.Equal(d.parentArtID, artifact.ID) } func (d *daoTestSuite) TestCreate() { @@ -174,12 +327,12 @@ func (d *daoTestSuite) TestCreate() { // conflict artifact := &Artifact{ - Type: typee, - MediaType: mediaType, - ManifestMediaType: manifestMediaType, - ProjectID: projectID, - RepositoryID: repositoryID, - Digest: digest, + Type: "IMAGE", + MediaType: v1.MediaTypeImageConfig, + ManifestMediaType: v1.MediaTypeImageManifest, + ProjectID: 1, + RepositoryID: 1, + Digest: "child_digest_01", Size: 1024, PushTime: time.Now(), PullTime: time.Now(), @@ -200,7 +353,7 @@ func (d *daoTestSuite) TestDelete() { d.True(ierror.IsErr(err, ierror.NotFoundCode)) // foreign key constraint - err = d.dao.Delete(d.ctx, d.artifactID) + err = d.dao.Delete(d.ctx, d.childArt01ID) d.Require().NotNil(err) d.True(ierror.IsErr(err, ierror.ViolateForeignKeyConstraintCode)) } @@ -209,12 +362,12 @@ func (d *daoTestSuite) TestUpdate() { // pass now := time.Now() err := d.dao.Update(d.ctx, &Artifact{ - ID: d.artifactID, + ID: d.parentArtID, PushTime: now, }, "PushTime") d.Require().Nil(err) - artifact, err := d.dao.Get(d.ctx, d.artifactID) + artifact, err := d.dao.Get(d.ctx, d.parentArtID) d.Require().Nil(err) d.Require().NotNil(artifact) d.Equal(now.Unix(), artifact.PullTime.Unix()) @@ -232,15 +385,15 @@ func (d *daoTestSuite) TestCreateReference() { // conflict _, err := d.dao.CreateReference(d.ctx, &ArtifactReference{ - ParentID: d.artifactID, - ChildID: d.artifactID, + ParentID: d.parentArtID, + ChildID: d.childArt01ID, }) d.Require().NotNil(err) d.True(ierror.IsErr(err, ierror.ConflictCode)) // foreign key constraint _, err = d.dao.CreateReference(d.ctx, &ArtifactReference{ - ParentID: d.artifactID, + ParentID: d.parentArtID, ChildID: 1000, }) d.Require().NotNil(err) @@ -250,12 +403,13 @@ func (d *daoTestSuite) TestCreateReference() { func (d *daoTestSuite) TestListReferences() { references, err := d.dao.ListReferences(d.ctx, &q.Query{ Keywords: map[string]interface{}{ - "parent_id": d.artifactID, + "ParentID": d.parentArtID, + "ChildID": d.childArt01ID, }, }) d.Require().Nil(err) d.Require().Equal(1, len(references)) - d.Equal(d.referenceID, references[0].ID) + d.Equal(d.reference01ID, references[0].ID) } func (d *daoTestSuite) TestDeleteReferences() { diff --git a/src/pkg/q/query.go b/src/pkg/q/query.go index 74c8e3da9..b4a27b35c 100644 --- a/src/pkg/q/query.go +++ b/src/pkg/q/query.go @@ -23,3 +23,19 @@ type Query struct { // List of key words Keywords map[string]interface{} } + +// Copy the specified query object +func Copy(query *Query) *Query { + if query == nil { + return nil + } + q := &Query{ + PageNumber: query.PageNumber, + PageSize: query.PageSize, + Keywords: map[string]interface{}{}, + } + for key, value := range query.Keywords { + q.Keywords[key] = value + } + return q +} diff --git a/src/pkg/q/query_test.go b/src/pkg/q/query_test.go new file mode 100644 index 000000000..12a41e3f5 --- /dev/null +++ b/src/pkg/q/query_test.go @@ -0,0 +1,46 @@ +// 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 q + +import ( + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "testing" +) + +func TestCopy(t *testing.T) { + // nil + q := Copy(nil) + assert.Nil(t, q) + + // not nil + query := &Query{ + PageNumber: 1, + PageSize: 10, + Keywords: map[string]interface{}{ + "key": "value", + }, + } + q = Copy(query) + require.NotNil(t, q) + assert.Equal(t, int64(1), q.PageNumber) + assert.Equal(t, int64(10), q.PageSize) + assert.Equal(t, "value", q.Keywords["key"].(string)) + // changes for the copy doesn't effect the original one + q.PageSize = 20 + q.Keywords["key"] = "value2" + assert.Equal(t, int64(10), query.PageSize) + assert.Equal(t, "value", query.Keywords["key"].(string)) +} diff --git a/src/server/v2.0/handler/artifact.go b/src/server/v2.0/handler/artifact.go index 2f2f9158b..b1fdca788 100644 --- a/src/server/v2.0/handler/artifact.go +++ b/src/server/v2.0/handler/artifact.go @@ -58,6 +58,9 @@ func (a *artifactAPI) ListArtifacts(ctx context.Context, params operation.ListAr if params.Type != nil { query.Keywords["Type"] = *(params.Type) } + if params.Tags != nil { + query.Keywords["Tags"] = *(params.Tags) + } if params.Page != nil { query.PageNumber = *(params.Page) }