Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: enable referrers pagination #56

Merged
merged 8 commits into from
Jan 20, 2023
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 44 additions & 0 deletions registry/handlers/referrers.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,10 @@ package handlers
import (
"context"
"encoding/json"
"fmt"
"net/http"
"path"
"strconv"

"github.com/distribution/distribution/v3"
dcontext "github.com/distribution/distribution/v3/context"
Expand Down Expand Up @@ -50,6 +52,9 @@ type referrersHandler struct {
Digest digest.Digest
}

const maxPageSize = 100
const minPageSize = 1

// GetReferrers fetches the list of referrers as an image index from the storage.
func (h *referrersHandler) GetReferrers(w http.ResponseWriter, r *http.Request) {
dcontext.GetLogger(h).Debug("GetReferrers")
Expand All @@ -59,13 +64,25 @@ func (h *referrersHandler) GetReferrers(w http.ResponseWriter, r *http.Request)
return
}

// extract the artifactType filter.
var annotations map[string]string
var artifactTypeFilter string
if artifactTypeFilter = r.URL.Query().Get("artifactType"); artifactTypeFilter != "" {
annotations = map[string]string{
v1.AnnotationReferrersFiltersApplied: "artifactType",
}
}

// extract the page size info. Users define page size by using the n
// query parameter. The default page size is 100.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment needs to be updated.

Suggested change
// query parameter. The default page size is 100.
// query parameter. The default page size is 10.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry. Updated.

pageSize, nParseError := strconv.Atoi(r.URL.Query().Get("n"))
if nParseError != nil || pageSize < minPageSize || pageSize > maxPageSize {
pageSize = maxPageSize

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may want to make the default size to 10, which is common.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed maxPageSize to 10.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should have three constants.

constant (
    pageSizeMin     = 1
    pageSizeDefault = 10
    pageSizeMax     = 100
)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added the three constants.

}

// extract the page number info.
pageNumber, _ := strconv.Atoi(r.URL.Query().Get("p"))

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pageNumber is not validated. What if it is negative?

Copy link
Author

@wangxiaoxuan273 wangxiaoxuan273 Jan 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added the validation. Negative pageNumber would make startIndex invalid.


referrers, err := h.generateReferrersList(h, h.Digest, artifactTypeFilter)
shizhMSFT marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
if _, ok := err.(distribution.ErrManifestUnknownRevision); ok {
Expand All @@ -80,6 +97,19 @@ func (h *referrersHandler) GetReferrers(w http.ResponseWriter, r *http.Request)
referrers = []v1.Descriptor{}
}

// only consider pagination if # of referrers is greater than page size
if len(referrers) > pageSize {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This if statement is redundant.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

deleted.

startIndex := pageNumber * pageSize

// if there's only 1 page of results left
if len(referrers)-startIndex <= pageSize {
referrers = referrers[startIndex:]
shizhMSFT marked this conversation as resolved.
Show resolved Hide resolved
} else {
referrers = referrers[startIndex:(startIndex + pageSize)]
w.Header().Set("Link", generateLinkHeader(h.Repository.Named().Name(), h.Digest.String(), artifactTypeFilter, []string{}, pageSize, pageNumber+1))
}
}

response := v1.Index{
Versioned: specs.Versioned{SchemaVersion: 2},
MediaType: v1.MediaTypeImageIndex,
Expand Down Expand Up @@ -195,6 +225,20 @@ func readlink(ctx context.Context, path string, stDriver driver.StorageDriver) (
return digest.Parse(string(content))
}

func generateLinkHeader(repoName, subjectDigest, artifactType string, lastDigests []string, nPage int, p int) string {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lastDigests is never used.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forgot to delete this from legacy code. Deleted.

url := fmt.Sprintf("/v2/%s/referrers/%s?p=%d",
repoName,
subjectDigest,
p)
if artifactType != "" {
url = fmt.Sprintf("%s&artifactType=%s", url, artifactType)
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You may consider to use url.Value since artifactType may contain special characters like /.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Used url.Values here. Thanks for this good advice.

if nPage > 0 {
url = fmt.Sprintf("%s&n=%d", url, nPage)
}
return fmt.Sprintf("<%s>; rel=\"next\"", url)
}

func generateReferrerFromArtifact(ctx context.Context,
blobStatter distribution.BlobStatter,
referrerDigest digest.Digest,
Expand Down