feat: enable referrers pagination #56

wangxiaoxuan273 · 2023-01-19T23:58:15Z

Signed-off-by: wangxiaoxuan273 [email protected]

Resolves #40

This pr enables the pagination feature of referrers API. The current implementation is slow as it calls generateReferrersList multiple times when pagination is needed.

Users can define page size by using the ?n query parameter. If not used, the default page size is set to 100. Pagination is automatically enabled when the number of returned referrers is greater than the page size (this is conformant to the distribution spec).

Test result with pageSize = 4. The test subject has 6 referrers and are returned with 2 pages.
Test result with pageSize = 5 using the same test subject. 2 Pages are returned.

Signed-off-by: wangxiaoxuan273 <[email protected]>

codecov-commenter · 2023-01-20T00:04:13Z

Codecov Report

Merging #56 (40cd89f) into main (f27b092) will decrease coverage by 0.14%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main      #56      +/-   ##
==========================================
- Coverage   56.27%   56.14%   -0.14%     
==========================================
  Files         108      108              
  Lines       11239    11266      +27     
==========================================
  Hits         6325     6325              
- Misses       4202     4229      +27     
  Partials      712      712

Impacted Files	Coverage Δ
registry/handlers/referrers.go	`0.00% <0.00%> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

shizhMSFT · 2023-01-20T03:14:31Z

registry/handlers/referrers.go

+	// query parameter. The default page size is 100.
+	pageSize, nParseError := strconv.Atoi(r.URL.Query().Get("n"))
+	if nParseError != nil || pageSize < minPageSize || pageSize > maxPageSize {
+		pageSize = maxPageSize


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

Changed maxPageSize to 10.

You should have three constants.

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

Added the three constants.

shizhMSFT · 2023-01-20T03:16:56Z

registry/handlers/referrers.go

@@ -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 {


This if statement is redundant.

registry/handlers/referrers.go

shizhMSFT · 2023-01-20T03:20:46Z

registry/handlers/referrers.go

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


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

Added the validation. Negative pageNumber would make startIndex invalid.

registry/handlers/referrers.go

shizhMSFT · 2023-01-20T03:24:36Z

registry/handlers/referrers.go

+	if artifactType != "" {
+		url = fmt.Sprintf("%s&artifactType=%s", url, artifactType)
+	}


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

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

Signed-off-by: wangxiaoxuan273 <[email protected]>

shizhMSFT · 2023-01-20T04:46:20Z

registry/handlers/referrers.go

+	// query parameter. The default page size is 100.
+	pageSize, nParseError := strconv.Atoi(r.URL.Query().Get("n"))
+	if nParseError != nil || pageSize < minPageSize || pageSize > maxPageSize {
+		pageSize = maxPageSize


You should have three constants.

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

shizhMSFT · 2023-01-20T04:47:01Z

registry/handlers/referrers.go

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


We don't need to name each error with a different name.

Suggested change

pageNumber, pParseError := strconv.Atoi(r.URL.Query().Get("p"))

pageNumber, err:= strconv.Atoi(r.URL.Query().Get("p"))

changed accordingly.

shizhMSFT · 2023-01-20T04:47:35Z

registry/handlers/referrers.go

 	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.


Comment needs to be updated.

Suggested change

// query parameter. The default page size is 100.

// query parameter. The default page size is 10.

sorry. Updated.

shizhMSFT · 2023-01-20T04:49:17Z

registry/handlers/referrers.go

+	if referrers == nil || startIndex > len(referrers) {
 		referrers = []v1.Descriptor{}
 	}


When fall into this code path, the code from line 107 - 113 is still executed but should not.

Used an else to enclose this part.

shizhMSFT · 2023-01-20T04:50:21Z

registry/handlers/referrers.go

@@ -195,6 +227,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 {


lastDigests is never used.

Forgot to delete this from legacy code. Deleted.

Signed-off-by: wangxiaoxuan273 <[email protected]>

shizhMSFT · 2023-01-20T07:02:06Z

registry/handlers/referrers.go

+func generateLinkHeader(repoName, subjectDigest, artifactType string, pageSize int, p int) string {
+	linkURL := fmt.Sprintf("/v2/%s/referrers/%s", repoName, subjectDigest)
+	v := url.Values{}
+	v.Add("p", fmt.Sprint(p))


nit: prefer strconv.Itoa to fmt.Sprint for performance.

Suggested change

v.Add("p", fmt.Sprint(p))

v.Add("p", strconv.Itoa(p))

shizhMSFT · 2023-01-20T07:02:17Z

registry/handlers/referrers.go

+		v.Add("artifactType", artifactType)
+	}
+	if pageSize > 0 {
+		v.Add("n", fmt.Sprint(pageSize))


shizhMSFT · 2023-01-20T07:04:31Z

registry/handlers/referrers.go

+	startIndex := pageNumber * pageSize
+
+	if referrers == nil || startIndex >= len(referrers) {
 		referrers = []v1.Descriptor{}
+	} else {
+		// if there's only 1 page of results left
+		if len(referrers)-startIndex <= pageSize {
+			referrers = referrers[startIndex:]
+		} else {
+			referrers = referrers[startIndex:(startIndex + pageSize)]
+			w.Header().Set("Link", generateLinkHeader(h.Repository.Named().Name(), h.Digest.String(), artifactTypeFilter, pageSize, pageNumber+1))
+		}
 	}


nit: code style: readability

Suggested change

startIndex := pageNumber * pageSize

if referrers == nil || startIndex >= len(referrers) {

referrers = []v1.Descriptor{}

} else {

// if there's only 1 page of results left

if len(referrers)-startIndex <= pageSize {

referrers = referrers[startIndex:]

} else {

referrers = referrers[startIndex:(startIndex + pageSize)]

w.Header().Set("Link", generateLinkHeader(h.Repository.Named().Name(), h.Digest.String(), artifactTypeFilter, pageSize, pageNumber+1))

}

}

startIndex := pageNumber * pageSize

if referrers == nil || len(referrers) <= startIndex {

referrers = []v1.Descriptor{}

} else if len(referrers) <= startIndex+pageSize {

// only 1 page of results left

referrers = referrers[startIndex:]

} else {

referrers = referrers[startIndex:(startIndex + pageSize)]

w.Header().Set("Link", generateLinkHeader(h.Repository.Named().Name(), h.Digest.String(), artifactTypeFilter, pageSize, pageNumber+1))

}

Signed-off-by: wangxiaoxuan273 <[email protected]>

shizhMSFT · 2023-01-20T08:21:03Z

registry/handlers/referrers.go

+		// only 1 page of results left
+		referrers = referrers[startIndex:]
+	} else {
+		referrers = referrers[startIndex:(startIndex + pageSize)]


nit:

Suggested change

referrers = referrers[startIndex:(startIndex + pageSize)]

referrers = referrers[startIndex:startIndex + pageSize]

shizhMSFT · 2023-01-20T08:23:36Z

registry/handlers/referrers.go

@@ -195,6 +229,19 @@ func readlink(ctx context.Context, path string, stDriver driver.StorageDriver) (
 	return digest.Parse(string(content))
 }

+func generateLinkHeader(repoName, subjectDigest, artifactType string, pageSize int, p int) string {


nit: code style: variable naming

Suggested change

func generateLinkHeader(repoName, subjectDigest, artifactType string, pageSize int, p int) string {

func generateLinkHeader(repoName, subjectDigest, artifactType string, pageSize int, pageNumber int) string {

Signed-off-by: wangxiaoxuan273 <[email protected]>

shizhMSFT

LGTM

feat: enable referrers pagination

6a1ed2b

Signed-off-by: wangxiaoxuan273 <[email protected]>

shizhMSFT reviewed Jan 20, 2023

View reviewed changes

resolved comments

10cac25

Signed-off-by: wangxiaoxuan273 <[email protected]>

wangxiaoxuan273 requested a review from shizhMSFT January 20, 2023 04:30

shizhMSFT reviewed Jan 20, 2023

View reviewed changes

wangxiaoxuan273 added 2 commits January 20, 2023 05:54

resolved all but 1 newest comments

ddf1eb2

Signed-off-by: wangxiaoxuan273 <[email protected]>

addressed the final comment

7240148

Signed-off-by: wangxiaoxuan273 <[email protected]>

wangxiaoxuan273 requested a review from shizhMSFT January 20, 2023 06:04

wangxiaoxuan273 added 2 commits January 20, 2023 06:12

fixed one bug

ceb1679

Signed-off-by: wangxiaoxuan273 <[email protected]>

refined

71780a4

Signed-off-by: wangxiaoxuan273 <[email protected]>

shizhMSFT reviewed Jan 20, 2023

View reviewed changes

resolved comments

bab3579

Signed-off-by: wangxiaoxuan273 <[email protected]>

wangxiaoxuan273 requested a review from shizhMSFT January 20, 2023 07:22

shizhMSFT reviewed Jan 20, 2023

View reviewed changes

resolved comments

40cd89f

Signed-off-by: wangxiaoxuan273 <[email protected]>

shizhMSFT approved these changes Jan 20, 2023

View reviewed changes

shizhMSFT merged commit 5468963 into oras-project:main Jan 20, 2023

wangxiaoxuan273 deleted the pagination branch January 20, 2023 08:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: enable referrers pagination #56

feat: enable referrers pagination #56

wangxiaoxuan273 commented Jan 19, 2023 •

edited

Loading

codecov-commenter commented Jan 20, 2023 •

edited

Loading

shizhMSFT Jan 20, 2023

wangxiaoxuan273 Jan 20, 2023

shizhMSFT Jan 20, 2023

wangxiaoxuan273 Jan 20, 2023

shizhMSFT Jan 20, 2023

wangxiaoxuan273 Jan 20, 2023

shizhMSFT Jan 20, 2023

wangxiaoxuan273 Jan 20, 2023 •

edited

Loading

shizhMSFT Jan 20, 2023

wangxiaoxuan273 Jan 20, 2023

shizhMSFT Jan 20, 2023

shizhMSFT Jan 20, 2023

wangxiaoxuan273 Jan 20, 2023

shizhMSFT Jan 20, 2023

wangxiaoxuan273 Jan 20, 2023

shizhMSFT Jan 20, 2023

wangxiaoxuan273 Jan 20, 2023 •

edited

Loading

shizhMSFT Jan 20, 2023

wangxiaoxuan273 Jan 20, 2023

shizhMSFT Jan 20, 2023

wangxiaoxuan273 Jan 20, 2023

shizhMSFT Jan 20, 2023

wangxiaoxuan273 Jan 20, 2023

shizhMSFT Jan 20, 2023

wangxiaoxuan273 Jan 20, 2023

shizhMSFT Jan 20, 2023

shizhMSFT Jan 20, 2023

shizhMSFT left a comment

	pageNumber, pParseError := strconv.Atoi(r.URL.Query().Get("p"))
	pageNumber, err:= strconv.Atoi(r.URL.Query().Get("p"))

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

	referrers = referrers[startIndex:(startIndex + pageSize)]
	referrers = referrers[startIndex:startIndex + pageSize]

	func generateLinkHeader(repoName, subjectDigest, artifactType string, pageSize int, p int) string {
	func generateLinkHeader(repoName, subjectDigest, artifactType string, pageSize int, pageNumber int) string {

feat: enable referrers pagination #56

feat: enable referrers pagination #56

Conversation

wangxiaoxuan273 commented Jan 19, 2023 • edited Loading

codecov-commenter commented Jan 20, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wangxiaoxuan273 Jan 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wangxiaoxuan273 Jan 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shizhMSFT left a comment

Choose a reason for hiding this comment

wangxiaoxuan273 commented Jan 19, 2023 •

edited

Loading

codecov-commenter commented Jan 20, 2023 •

edited

Loading

wangxiaoxuan273 Jan 20, 2023 •

edited

Loading

wangxiaoxuan273 Jan 20, 2023 •

edited

Loading