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

Fix handling of plenty Nuget package versions #26075

Merged
merged 16 commits into from Jul 26, 2023
Merged
14 changes: 12 additions & 2 deletions routers/api/packages/nuget/api_v2.go
Expand Up @@ -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"`
}
Expand All @@ -300,14 +300,24 @@ 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,
XmlnsD: "http://schemas.microsoft.com/ado/2007/08/dataservices",
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,
}
Expand Down
8 changes: 4 additions & 4 deletions routers/api/packages/nuget/api_v3.go
Expand Up @@ -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{
Expand Down
20 changes: 20 additions & 0 deletions routers/api/packages/nuget/links.go
Expand Up @@ -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
Expand All @@ -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)
}
Comment on lines +42 to +48
Copy link
Member

Choose a reason for hiding this comment

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

Not a blocker
Could you change the u, q, vs shorthand to some significant names or could you annotate them in comments.

Copy link
Member Author

Choose a reason for hiding this comment

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

No, these are the common abbreviations for these types of variables. u is often an url, k and v are key and value. vs values in this case. The variables have a very limited scope, so it's easy to look them up.

https://github.com/golang/go/wiki/CodeReviewComments#variable-names

Copy link
Member

Choose a reason for hiding this comment

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

Sure,

}
u.RawQuery = q.Encode()
return u.String()
}
82 changes: 60 additions & 22 deletions routers/api/packages/nuget/nuget.go
Expand Up @@ -9,6 +9,7 @@ import (
"fmt"
"io"
"net/http"
"net/url"
"regexp"
"strconv"
"strings"
Expand Down Expand Up @@ -111,13 +112,14 @@ 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")
skip, take := ctx.FormInt("$skip"), ctx.FormInt("$top")
Copy link
Member

Choose a reason for hiding this comment

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

$skip what is the significance of $ in the word.
$top is it top or stop. could you annotate this in comment.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's just how the spec defines this parameter. The names are $skip and $top. Sadly one older client uses skip and take and that's the reason why they are mentioned as fallback here. I moved the official names to the top.

if skip == 0 {
skip = ctx.FormInt("$skip")
skip = ctx.FormInt("skip")
KN4CK3R marked this conversation as resolved.
Show resolved Hide resolved
}
if take == 0 {
take = ctx.FormInt("$top")
take = ctx.FormInt("take")
}
paginator := db.NewAbsoluteListOptions(skip, take)

pvs, total, err := packages_model.SearchVersions(ctx, &packages_model.PackageSearchOptions{
OwnerID: ctx.Package.Owner.ID,
Expand All @@ -126,10 +128,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)
Expand All @@ -142,8 +141,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,
)
Expand Down Expand Up @@ -193,7 +212,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,
)
Expand Down Expand Up @@ -222,7 +241,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,
)

Expand Down Expand Up @@ -251,7 +270,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,
)

Expand Down Expand Up @@ -280,7 +299,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,
)

Expand All @@ -291,7 +310,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
Expand All @@ -303,9 +334,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,
)

Expand Down Expand Up @@ -345,13 +389,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)
}
Expand Down
46 changes: 42 additions & 4 deletions tests/integration/api_packages_nuget_test.go
Expand Up @@ -12,6 +12,7 @@ import (
"io"
"net/http"
"net/http/httptest"
neturl "net/url"
"strconv"
"testing"
"time"
Expand Down Expand Up @@ -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})
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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)

Expand All @@ -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)
Expand Down