Fix handling of plenty Nuget package versions (#26075)

Fixes #25953 - Do not load full version information (v3) - Add pagination support (v2)
2023-07-26 21:43:21 +02:00 · 2023-07-26 21:43:21 +02:00 · 2d7fe4cc1e
commit 2d7fe4cc1e
parent 6ce89883eb
5 changed files with 138 additions and 38 deletions
--- a/routers/api/packages/nuget/api_v2.go
+++ b/routers/api/packages/nuget/api_v2.go
@ -289,7 +289,7 @@ type FeedResponse struct {
 	ID      string             `xml:"id"`
 	Title   TypedValue[string] `xml:"title"`
 	Updated time.Time          `xml:"updated"`
-	Link    FeedEntryLink      `xml:"link"`
+	Links   []FeedEntryLink    `xml:"link"`
 	Entries []*FeedEntry       `xml:"entry"`
 	Count   int64              `xml:"m:count"`
 }
@ -300,6 +300,16 @@ func createFeedResponse(l *linkBuilder, totalEntries int64, pds []*packages_mode
 		entries = append(entries, createEntry(l, pd, false))
 	}

+	links := []FeedEntryLink{
+		{Rel: "self", Href: l.Base},
+	}
+	if l.Next != nil {
+		links = append(links, FeedEntryLink{
+			Rel:  "next",
+			Href: l.GetNextURL(),
+		})
+	}
+
 	return &FeedResponse{
 		Xmlns:   "http://www.w3.org/2005/Atom",
 		Base:    l.Base,
@ -307,7 +317,7 @@ func createFeedResponse(l *linkBuilder, totalEntries int64, pds []*packages_mode
 		XmlnsM:  "http://schemas.microsoft.com/ado/2007/08/dataservices/metadata",
 		ID:      "http://schemas.datacontract.org/2004/07/",
 		Updated: time.Now(),
-		Link:    FeedEntryLink{Rel: "self", Href: l.Base},
+		Links:   links,
 		Count:   totalEntries,
 		Entries: entries,
 	}
--- a/routers/api/packages/nuget/api_v3.go
+++ b/routers/api/packages/nuget/api_v3.go
@ -166,10 +166,10 @@ type PackageVersionsResponse struct {
 	Versions []string `json:"versions"`
 }

-func createPackageVersionsResponse(pds []*packages_model.PackageDescriptor) *PackageVersionsResponse {
-	versions := make([]string, 0, len(pds))
-	for _, pd := range pds {
-		versions = append(versions, pd.Version.Version)
+func createPackageVersionsResponse(pvs []*packages_model.PackageVersion) *PackageVersionsResponse {
+	versions := make([]string, 0, len(pvs))
+	for _, pv := range pvs {
+		versions = append(versions, pv.Version)
 	}

 	return &PackageVersionsResponse{
--- a/routers/api/packages/nuget/links.go
+++ b/routers/api/packages/nuget/links.go
@ -5,10 +5,17 @@ package nuget

 import (
 	"fmt"
+	"net/url"
 )

+type nextOptions struct {
+	Path  string
+	Query url.Values
+}
+
 type linkBuilder struct {
 	Base string
+	Next *nextOptions
 }

 // GetRegistrationIndexURL builds the registration index url
@ -30,3 +37,16 @@ func (l *linkBuilder) GetPackageDownloadURL(id, version string) string {
 func (l *linkBuilder) GetPackageMetadataURL(id, version string) string {
 	return fmt.Sprintf("%s/Packages(Id='%s',Version='%s')", l.Base, id, version)
 }
+
+func (l *linkBuilder) GetNextURL() string {
+	u, _ := url.Parse(l.Base)
+	u = u.JoinPath(l.Next.Path)
+	q := u.Query()
+	for k, vs := range l.Next.Query {
+		for _, v := range vs {
+			q.Add(k, v)
+		}
+	}
+	u.RawQuery = q.Encode()
+	return u.String()
+}
--- a/routers/api/packages/nuget/nuget.go
+++ b/routers/api/packages/nuget/nuget.go
@ -9,6 +9,7 @@ import (
 	"fmt"
 	"io"
 	"net/http"
+	"net/url"
 	"regexp"
 	"strconv"
 	"strings"
@ -111,13 +112,8 @@ func getSearchTerm(ctx *context.Context) string {

 // https://github.com/NuGet/NuGet.Client/blob/dev/src/NuGet.Core/NuGet.Protocol/LegacyFeed/V2FeedQueryBuilder.cs
 func SearchServiceV2(ctx *context.Context) {
-	skip, take := ctx.FormInt("skip"), ctx.FormInt("take")
-	if skip == 0 {
-		skip = ctx.FormInt("$skip")
-	}
-	if take == 0 {
-		take = ctx.FormInt("$top")
-	}
+	skip, take := ctx.FormInt("$skip"), ctx.FormInt("$top")
+	paginator := db.NewAbsoluteListOptions(skip, take)

 	pvs, total, err := packages_model.SearchVersions(ctx, &packages_model.PackageSearchOptions{
 		OwnerID: ctx.Package.Owner.ID,
@ -126,10 +122,7 @@ func SearchServiceV2(ctx *context.Context) {
 			Value: getSearchTerm(ctx),
 		},
 		IsInternal: util.OptionalBoolFalse,
-		Paginator: db.NewAbsoluteListOptions(
-			skip,
-			take,
-		),
+		Paginator:  paginator,
 	})
 	if err != nil {
 		apiError(ctx, http.StatusInternalServerError, err)
@ -142,8 +135,28 @@ func SearchServiceV2(ctx *context.Context) {
 		return
 	}

+	skip, take = paginator.GetSkipTake()
+
+	var next *nextOptions
+	if len(pvs) == take {
+		next = &nextOptions{
+			Path:  "Search()",
+			Query: url.Values{},
+		}
+		searchTerm := ctx.FormTrim("searchTerm")
+		if searchTerm != "" {
+			next.Query.Set("searchTerm", searchTerm)
+		}
+		filter := ctx.FormTrim("$filter")
+		if filter != "" {
+			next.Query.Set("$filter", filter)
+		}
+		next.Query.Set("$skip", strconv.Itoa(skip+take))
+		next.Query.Set("$top", strconv.Itoa(take))
+	}
+
 	resp := createFeedResponse(
-		&linkBuilder{setting.AppURL + "api/packages/" + ctx.Package.Owner.Name + "/nuget"},
+		&linkBuilder{Base: setting.AppURL + "api/packages/" + ctx.Package.Owner.Name + "/nuget", Next: next},
 		total,
 		pds,
 	)
@ -193,7 +206,7 @@ func SearchServiceV3(ctx *context.Context) {
 	}

 	resp := createSearchResultResponse(
-		&linkBuilder{setting.AppURL + "api/packages/" + ctx.Package.Owner.Name + "/nuget"},
+		&linkBuilder{Base: setting.AppURL + "api/packages/" + ctx.Package.Owner.Name + "/nuget"},
 		count,
 		pds,
 	)
@ -222,7 +235,7 @@ func RegistrationIndex(ctx *context.Context) {
 	}

 	resp := createRegistrationIndexResponse(
-		&linkBuilder{setting.AppURL + "api/packages/" + ctx.Package.Owner.Name + "/nuget"},
+		&linkBuilder{Base: setting.AppURL + "api/packages/" + ctx.Package.Owner.Name + "/nuget"},
 		pds,
 	)

@ -251,7 +264,7 @@ func RegistrationLeafV2(ctx *context.Context) {
 	}

 	resp := createEntryResponse(
-		&linkBuilder{setting.AppURL + "api/packages/" + ctx.Package.Owner.Name + "/nuget"},
+		&linkBuilder{Base: setting.AppURL + "api/packages/" + ctx.Package.Owner.Name + "/nuget"},
 		pd,
 	)

@ -280,7 +293,7 @@ func RegistrationLeafV3(ctx *context.Context) {
 	}

 	resp := createRegistrationLeafResponse(
-		&linkBuilder{setting.AppURL + "api/packages/" + ctx.Package.Owner.Name + "/nuget"},
+		&linkBuilder{Base: setting.AppURL + "api/packages/" + ctx.Package.Owner.Name + "/nuget"},
 		pd,
 	)

@ -291,7 +304,19 @@ func RegistrationLeafV3(ctx *context.Context) {
 func EnumeratePackageVersionsV2(ctx *context.Context) {
 	packageName := strings.Trim(ctx.FormTrim("id"), "'")

-	pvs, err := packages_model.GetVersionsByPackageName(ctx, ctx.Package.Owner.ID, packages_model.TypeNuGet, packageName)
+	skip, take := ctx.FormInt("$skip"), ctx.FormInt("$top")
+	paginator := db.NewAbsoluteListOptions(skip, take)
+
+	pvs, total, err := packages_model.SearchVersions(ctx, &packages_model.PackageSearchOptions{
+		OwnerID: ctx.Package.Owner.ID,
+		Type:    packages_model.TypeNuGet,
+		Name: packages_model.SearchValue{
+			ExactMatch: true,
+			Value:      packageName,
+		},
+		IsInternal: util.OptionalBoolFalse,
+		Paginator:  paginator,
+	})
 	if err != nil {
 		apiError(ctx, http.StatusInternalServerError, err)
 		return
@ -303,9 +328,22 @@ func EnumeratePackageVersionsV2(ctx *context.Context) {
 		return
 	}

+	skip, take = paginator.GetSkipTake()
+
+	var next *nextOptions
+	if len(pvs) == take {
+		next = &nextOptions{
+			Path:  "FindPackagesById()",
+			Query: url.Values{},
+		}
+		next.Query.Set("id", packageName)
+		next.Query.Set("$skip", strconv.Itoa(skip+take))
+		next.Query.Set("$top", strconv.Itoa(take))
+	}
+
 	resp := createFeedResponse(
-		&linkBuilder{setting.AppURL + "api/packages/" + ctx.Package.Owner.Name + "/nuget"},
-		int64(len(pds)),
+		&linkBuilder{Base: setting.AppURL + "api/packages/" + ctx.Package.Owner.Name + "/nuget", Next: next},
+		total,
 		pds,
 	)

@ -345,13 +383,7 @@ func EnumeratePackageVersionsV3(ctx *context.Context) {
 		return
 	}

-	pds, err := packages_model.GetPackageDescriptors(ctx, pvs)
-	if err != nil {
-		apiError(ctx, http.StatusInternalServerError, err)
-		return
-	}
-
-	resp := createPackageVersionsResponse(pds)
+	resp := createPackageVersionsResponse(pvs)

 	ctx.JSON(http.StatusOK, resp)
 }
--- a/tests/integration/api_packages_nuget_test.go
+++ b/tests/integration/api_packages_nuget_test.go
@ -12,6 +12,7 @@ import (
 	"io"
 	"net/http"
 	"net/http/httptest"
+	neturl "net/url"
 	"strconv"
 	"testing"
 	"time"
@ -68,10 +69,16 @@ func TestPackageNuGet(t *testing.T) {
 		Content    string               `xml:",innerxml"`
 	}

+	type FeedEntryLink struct {
+		Rel  string `xml:"rel,attr"`
+		Href string `xml:"href,attr"`
+	}
+
 	type FeedResponse struct {
-		XMLName xml.Name     `xml:"feed"`
-		Entries []*FeedEntry `xml:"entry"`
-		Count   int64        `xml:"count"`
+		XMLName xml.Name        `xml:"feed"`
+		Links   []FeedEntryLink `xml:"link"`
+		Entries []*FeedEntry    `xml:"entry"`
+		Count   int64           `xml:"count"`
 	}

 	user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2})
@ -373,6 +380,25 @@ AAAjQmxvYgAAAGm7ENm9SGxMtAFVvPUsPJTF6PbtAAAAAFcVogEJAAAAAQAAAA==`)
 		})
 	})

+	containsOneNextLink := func(t *testing.T, links []FeedEntryLink) func() bool {
+		return func() bool {
+			found := 0
+			for _, l := range links {
+				if l.Rel == "next" {
+					found++
+					u, err := neturl.Parse(l.Href)
+					assert.NoError(t, err)
+					q := u.Query()
+					assert.Contains(t, q, "$skip")
+					assert.Contains(t, q, "$top")
+					assert.Equal(t, "1", q.Get("$skip"))
+					assert.Equal(t, "1", q.Get("$top"))
+				}
+			}
+			return found == 1
+		}
+	}
+
 	t.Run("SearchService", func(t *testing.T) {
 		cases := []struct {
 			Query           string
@ -393,7 +419,7 @@ AAAjQmxvYgAAAGm7ENm9SGxMtAFVvPUsPJTF6PbtAAAAAFcVogEJAAAAAQAAAA==`)
 				defer tests.PrintCurrentTest(t)()

 				for i, c := range cases {
-					req := NewRequest(t, "GET", fmt.Sprintf("%s/Search()?searchTerm='%s'&skip=%d&take=%d", url, c.Query, c.Skip, c.Take))
+					req := NewRequest(t, "GET", fmt.Sprintf("%s/Search()?searchTerm='%s'&$skip=%d&$top=%d", url, c.Query, c.Skip, c.Take))
 					req = AddBasicAuthHeader(req, user.Name)
 					resp := MakeRequest(t, req, http.StatusOK)

@ -403,7 +429,7 @@ AAAjQmxvYgAAAGm7ENm9SGxMtAFVvPUsPJTF6PbtAAAAAFcVogEJAAAAAQAAAA==`)
 					assert.Equal(t, c.ExpectedTotal, result.Count, "case %d: unexpected total hits", i)
 					assert.Len(t, result.Entries, c.ExpectedResults, "case %d: unexpected result count", i)

-					req = NewRequest(t, "GET", fmt.Sprintf("%s/Search()/$count?searchTerm='%s'&skip=%d&take=%d", url, c.Query, c.Skip, c.Take))
+					req = NewRequest(t, "GET", fmt.Sprintf("%s/Search()/$count?searchTerm='%s'&$skip=%d&$top=%d", url, c.Query, c.Skip, c.Take))
 					req = AddBasicAuthHeader(req, user.Name)
 					resp = MakeRequest(t, req, http.StatusOK)

@ -432,6 +458,17 @@ AAAjQmxvYgAAAGm7ENm9SGxMtAFVvPUsPJTF6PbtAAAAAFcVogEJAAAAAQAAAA==`)
 					assert.Equal(t, strconv.FormatInt(c.ExpectedTotal, 10), resp.Body.String(), "case %d: unexpected total hits", i)
 				}
 			})
+
+			t.Run("Next", func(t *testing.T) {
+				req := NewRequest(t, "GET", fmt.Sprintf("%s/Search()?searchTerm='test'&$skip=0&$top=1", url))
+				req = AddBasicAuthHeader(req, user.Name)
+				resp := MakeRequest(t, req, http.StatusOK)
+
+				var result FeedResponse
+				decodeXML(t, resp, &result)
+
+				assert.Condition(t, containsOneNextLink(t, result.Links))
+			})
 		})

 		t.Run("v3", func(t *testing.T) {
@ -558,7 +595,7 @@ AAAjQmxvYgAAAGm7ENm9SGxMtAFVvPUsPJTF6PbtAAAAAFcVogEJAAAAAQAAAA==`)
 		t.Run("v2", func(t *testing.T) {
 			defer tests.PrintCurrentTest(t)()

-			req := NewRequest(t, "GET", fmt.Sprintf("%s/FindPackagesById()?id='%s'", url, packageName))
+			req := NewRequest(t, "GET", fmt.Sprintf("%s/FindPackagesById()?id='%s'&$top=1", url, packageName))
 			req = AddBasicAuthHeader(req, user.Name)
 			resp := MakeRequest(t, req, http.StatusOK)

@ -567,6 +604,7 @@ AAAjQmxvYgAAAGm7ENm9SGxMtAFVvPUsPJTF6PbtAAAAAFcVogEJAAAAAQAAAA==`)

 			assert.Len(t, result.Entries, 1)
 			assert.Equal(t, packageVersion, result.Entries[0].Properties.Version)
+			assert.Condition(t, containsOneNextLink(t, result.Links))

 			req = NewRequest(t, "GET", fmt.Sprintf("%s/FindPackagesById()/$count?id='%s'", url, packageName))
 			req = AddBasicAuthHeader(req, user.Name)