Skip to content

Commit 0379073

Browse files
committed
RTDEV-97165 - Remove sortBy from ListVersions call site
The Skills API versions endpoint has no sort parameter (verified by decompiling SkillsResource.listSkillVersions bytecode). Remove sortBy from the call to serviceManager.ListSkillVersions, matching the updated interface signature in jfrog-client-go. Call site now passes only: (repoKey, slug, skillVersionsPageSize, cursor). Also updates test mock and removes sortBy assertion in TestListVersionsFromManager_SinglePage_OneCall.
1 parent a590327 commit 0379073

4 files changed

Lines changed: 241 additions & 31 deletions

File tree

agent/skills/common/skills_api.go

Lines changed: 43 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,9 @@ import (
77
agentcommon "github.com/jfrog/jfrog-cli-artifactory/agent/common"
88
"github.com/jfrog/jfrog-cli-core/v2/artifactory/utils"
99
"github.com/jfrog/jfrog-cli-core/v2/utils/config"
10+
"github.com/jfrog/jfrog-client-go/artifactory"
1011
"github.com/jfrog/jfrog-client-go/artifactory/services"
12+
"github.com/jfrog/jfrog-client-go/utils/log"
1113
)
1214

1315
func ListSkills(serverDetails *config.ServerDetails, repoKey string, limit int, sortBy string) ([]services.SkillListItem, error) {
@@ -17,29 +19,33 @@ func ListSkills(serverDetails *config.ServerDetails, repoKey string, limit int,
1719
}
1820
var allItems []services.SkillListItem
1921
cursor := ""
20-
pageSize := 100
2122
for {
22-
items, nextCursor, err := serviceManager.ListSkills(repoKey, pageSize, cursor, sortBy)
23+
items, nextCursor, err := serviceManager.ListSkills(repoKey, skillVersionsPageSize, cursor, sortBy)
2324
if err != nil {
2425
return nil, err
2526
}
2627
allItems = append(allItems, items...)
2728
if limit > 0 && len(allItems) >= limit {
2829
return allItems[:limit], nil
2930
}
30-
if nextCursor == "" || len(items) < pageSize {
31+
if nextCursor == "" || len(items) < skillVersionsPageSize {
3132
break
3233
}
3334
cursor = nextCursor
3435
}
3536
return allItems, nil
3637
}
3738

38-
// ListVersions returns the version folders published under <repoKey>/<slug>/ using
39-
// the generic Artifactory storage API, bypassing any Skills API filtering.
40-
// This queries raw storage instead of the filtered Skills API endpoint.
41-
// On 404, it disambiguates between missing repo and missing skill to provide
42-
// users with actionable error messages.
39+
// skillVersionsPageSize is the limit we request per Skills API versions call. Kept as our own constant
40+
// (rather than relying on services.DefaultSkillVersionsLimit) so this repo controls its own page size independently.
41+
const skillVersionsPageSize = 200
42+
43+
// ListVersions returns the versions published for <repoKey>/<slug> via the Skills API
44+
// (api/skills/{repoKey}/api/v1/skills/{slug}/versions).
45+
//
46+
// It requests skillVersionsPageSize versions per call and follows nextCursor for as many additional calls as needed
47+
// (verified against live instance: nextCursor is omitted entirely when last page is served), ensuring a skill with
48+
// more versions than one page is listed in full. On 404, it disambiguates between missing repo and missing skill.
4349
func ListVersions(serverDetails *config.ServerDetails, repoKey, slug string) ([]services.SkillVersion, error) {
4450
if serverDetails == nil {
4551
return nil, fmt.Errorf("server details are required to list skill versions")
@@ -57,31 +63,40 @@ func ListVersions(serverDetails *config.ServerDetails, repoKey, slug string) ([]
5763
if err != nil {
5864
return nil, err
5965
}
66+
return listVersionsFromManager(serviceManager, repoKey, slug)
67+
}
6068

61-
// Query raw storage layer instead of filtered Skills API
62-
info, err := serviceManager.FolderInfo(fmt.Sprintf("%s/%s", repoKey, slug))
63-
if err != nil {
64-
if agentcommon.IsHTTPNotFound(err) {
65-
return nil, agentcommon.DisambiguateFolderError(serviceManager, repoKey, err, fmt.Errorf("skill '%s' not found in repository '%s': %w", slug, repoKey, err))
66-
}
67-
return nil, fmt.Errorf("list skill versions: %w", err)
68-
}
69-
70-
versions := make([]services.SkillVersion, 0, len(info.Children))
71-
for _, child := range info.Children {
72-
if !child.Folder {
73-
continue
69+
// listVersionsFromManager holds the actual pagination/error-disambiguation logic,
70+
// taking the ArtifactoryServicesManager interface directly so it can be unit tested
71+
// with a mock instead of a live server.
72+
func listVersionsFromManager(serviceManager artifactory.ArtifactoryServicesManager, repoKey, slug string) ([]services.SkillVersion, error) {
73+
var allVersions []services.SkillVersion
74+
cursor := ""
75+
for {
76+
log.Debug(fmt.Sprintf("list skill versions: calling ListSkillVersions for skill '%s' in repo '%s' with cursor '%s'", slug, repoKey, cursor))
77+
versions, nextCursor, err := serviceManager.ListSkillVersions(repoKey, slug, skillVersionsPageSize, cursor)
78+
if err != nil {
79+
// Only disambiguate on the first page: a 404 mid-pagination means the skill
80+
// was deleted concurrently, not that the repo/skill never existed.
81+
if cursor == "" && agentcommon.IsHTTPNotFound(err) {
82+
return nil, agentcommon.DisambiguateFolderError(serviceManager, repoKey, err, fmt.Errorf("skill '%s' not found in repository '%s': %w", slug, repoKey, err))
83+
}
84+
return nil, fmt.Errorf("list skill versions: %w", err)
7485
}
75-
name := child.Uri
76-
if len(name) > 0 && name[0] == '/' {
77-
name = name[1:]
86+
allVersions = append(allVersions, versions...)
87+
log.Debug(fmt.Sprintf("list skill versions: received %d versions, next cursor: '%s'", len(versions), nextCursor))
88+
if nextCursor == "" {
89+
log.Debug(fmt.Sprintf("list skill versions: no more pages for skill '%s', %d version(s) total", slug, len(allVersions)))
90+
break
7891
}
79-
if name == "" {
80-
continue
92+
if nextCursor == cursor {
93+
// Guard against a server bug returning a non-advancing cursor: without this,
94+
// a stuck cursor spins forever, growing allVersions unbounded.
95+
return nil, fmt.Errorf("list skill versions: server returned a non-advancing cursor for skill '%s'", slug)
8196
}
82-
versions = append(versions, services.SkillVersion{Version: name})
97+
cursor = nextCursor
8398
}
84-
return versions, nil
99+
return allVersions, nil
85100
}
86101

87102
func SearchSkills(serverDetails *config.ServerDetails, repoKey, query string, limit int) ([]services.SkillSearchResult, error) {
Lines changed: 192 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,192 @@
1+
package common
2+
3+
import (
4+
"errors"
5+
"fmt"
6+
"testing"
7+
8+
"github.com/jfrog/jfrog-cli-core/v2/utils/config"
9+
"github.com/jfrog/jfrog-client-go/artifactory"
10+
"github.com/jfrog/jfrog-client-go/artifactory/services"
11+
servicesutils "github.com/jfrog/jfrog-client-go/artifactory/services/utils"
12+
"github.com/stretchr/testify/assert"
13+
"github.com/stretchr/testify/require"
14+
)
15+
16+
// statusCodeError mimics jfrog-client-go response errors with HTTP status codes.
17+
type statusCodeError struct{ code int }
18+
19+
func (e statusCodeError) Error() string { return fmt.Sprintf("http status %d", e.code) }
20+
func (e statusCodeError) StatusCode() int { return e.code }
21+
22+
var err404 = statusCodeError{code: 404}
23+
24+
// mockSkillsServicesManager mocks ListSkillVersions and FolderInfo for testing.
25+
type mockSkillsServicesManager struct {
26+
artifactory.EmptyArtifactoryServicesManager
27+
listSkillVersionsFunc func(repoKey, slug string, limit int, cursor string) ([]services.SkillVersion, string, error)
28+
folderInfoFunc func(path string) (*servicesutils.FolderInfo, error)
29+
}
30+
31+
func (m *mockSkillsServicesManager) ListSkillVersions(repoKey, slug string, limit int, cursor string) ([]services.SkillVersion, string, error) {
32+
return m.listSkillVersionsFunc(repoKey, slug, limit, cursor)
33+
}
34+
35+
func (m *mockSkillsServicesManager) FolderInfo(path string) (*servicesutils.FolderInfo, error) {
36+
if m.folderInfoFunc != nil {
37+
return m.folderInfoFunc(path)
38+
}
39+
return nil, nil
40+
}
41+
42+
func TestListVersionsFromManager_SinglePage_OneCall(t *testing.T) {
43+
calls := 0
44+
mock := &mockSkillsServicesManager{
45+
listSkillVersionsFunc: func(repoKey, slug string, limit int, cursor string) ([]services.SkillVersion, string, error) {
46+
calls++
47+
assert.Equal(t, "my-repo", repoKey)
48+
assert.Equal(t, "my-skill", slug)
49+
assert.Equal(t, skillVersionsPageSize, limit)
50+
assert.Equal(t, "", cursor)
51+
return []services.SkillVersion{{Version: "1.0.1"}}, "", nil
52+
},
53+
}
54+
55+
versions, err := listVersionsFromManager(mock, "my-repo", "my-skill")
56+
require.NoError(t, err)
57+
assert.Equal(t, []services.SkillVersion{{Version: "1.0.1"}}, versions)
58+
assert.Equal(t, 1, calls, "a skill within the page limit must resolve in exactly one call")
59+
}
60+
61+
func TestListVersionsFromManager_PaginatesUntilCursorEmpty(t *testing.T) {
62+
pages := [][]services.SkillVersion{
63+
{{Version: "1.0.3"}, {Version: "1.0.2"}},
64+
{{Version: "1.0.1"}},
65+
}
66+
nextCursors := []string{"1.0.2", ""}
67+
call := 0
68+
mock := &mockSkillsServicesManager{
69+
listSkillVersionsFunc: func(repoKey, slug string, limit int, cursor string) ([]services.SkillVersion, string, error) {
70+
if call == 0 {
71+
assert.Equal(t, "", cursor, "first call must start from the beginning")
72+
} else {
73+
assert.Equal(t, nextCursors[call-1], cursor, "must resume from the previous page's cursor")
74+
}
75+
page, next := pages[call], nextCursors[call]
76+
call++
77+
return page, next, nil
78+
},
79+
}
80+
81+
versions, err := listVersionsFromManager(mock, "my-repo", "my-skill")
82+
require.NoError(t, err)
83+
assert.Equal(t, []services.SkillVersion{{Version: "1.0.3"}, {Version: "1.0.2"}, {Version: "1.0.1"}}, versions)
84+
assert.Equal(t, 2, call, "must stop as soon as nextCursor is empty")
85+
}
86+
87+
func TestListVersionsFromManager_StopsOnNonAdvancingCursor(t *testing.T) {
88+
// Guard against infinite loops on stuck cursors.
89+
calls := 0
90+
mock := &mockSkillsServicesManager{
91+
listSkillVersionsFunc: func(repoKey, slug string, limit int, cursor string) ([]services.SkillVersion, string, error) {
92+
calls++
93+
if calls > 3 {
94+
t.Fatalf("listVersionsFromManager did not stop on a non-advancing cursor")
95+
}
96+
return []services.SkillVersion{{Version: "1.0.1"}}, "stuck-cursor", nil
97+
},
98+
}
99+
100+
_, err := listVersionsFromManager(mock, "my-repo", "my-skill")
101+
require.Error(t, err)
102+
assert.Contains(t, err.Error(), "non-advancing cursor")
103+
assert.LessOrEqual(t, calls, 2, "must stop as soon as the cursor repeats")
104+
}
105+
106+
func TestListVersionsFromManager_NotFound_RepoMissing(t *testing.T) {
107+
mock := &mockSkillsServicesManager{
108+
listSkillVersionsFunc: func(repoKey, slug string, limit int, cursor string) ([]services.SkillVersion, string, error) {
109+
return nil, "", err404
110+
},
111+
folderInfoFunc: func(path string) (*servicesutils.FolderInfo, error) {
112+
assert.Equal(t, "my-repo", path)
113+
return nil, err404
114+
},
115+
}
116+
117+
_, err := listVersionsFromManager(mock, "my-repo", "my-skill")
118+
require.Error(t, err)
119+
assert.Contains(t, err.Error(), "repository 'my-repo' not found")
120+
}
121+
122+
func TestListVersionsFromManager_NotFound_SkillMissing(t *testing.T) {
123+
mock := &mockSkillsServicesManager{
124+
listSkillVersionsFunc: func(repoKey, slug string, limit int, cursor string) ([]services.SkillVersion, string, error) {
125+
return nil, "", err404
126+
},
127+
folderInfoFunc: func(path string) (*servicesutils.FolderInfo, error) {
128+
return &servicesutils.FolderInfo{}, nil
129+
},
130+
}
131+
132+
_, err := listVersionsFromManager(mock, "my-repo", "my-skill")
133+
require.Error(t, err)
134+
assert.Contains(t, err.Error(), "skill 'my-skill' not found in repository 'my-repo'")
135+
}
136+
137+
func TestListVersionsFromManager_NotFoundMidPagination_NotDisambiguated(t *testing.T) {
138+
// 404 mid-pagination should not trigger disambiguation (only first-page 404s do).
139+
call := 0
140+
folderInfoCalled := false
141+
mock := &mockSkillsServicesManager{
142+
listSkillVersionsFunc: func(repoKey, slug string, limit int, cursor string) ([]services.SkillVersion, string, error) {
143+
call++
144+
if call == 1 {
145+
return []services.SkillVersion{{Version: "1.0.2"}}, "1.0.2", nil
146+
}
147+
return nil, "", err404
148+
},
149+
folderInfoFunc: func(path string) (*servicesutils.FolderInfo, error) {
150+
folderInfoCalled = true
151+
return &servicesutils.FolderInfo{}, nil
152+
},
153+
}
154+
155+
_, err := listVersionsFromManager(mock, "my-repo", "my-skill")
156+
require.Error(t, err)
157+
assert.NotContains(t, err.Error(), "not found in repository")
158+
assert.False(t, folderInfoCalled, "disambiguation must only run on the first page's 404")
159+
}
160+
161+
func TestListVersionsFromManager_NonNotFoundError_Propagated(t *testing.T) {
162+
boom := errors.New("network exploded")
163+
mock := &mockSkillsServicesManager{
164+
listSkillVersionsFunc: func(repoKey, slug string, limit int, cursor string) ([]services.SkillVersion, string, error) {
165+
return nil, "", boom
166+
},
167+
}
168+
169+
_, err := listVersionsFromManager(mock, "my-repo", "my-skill")
170+
require.Error(t, err)
171+
assert.ErrorIs(t, err, boom)
172+
}
173+
174+
func TestListVersions_ValidationErrors(t *testing.T) {
175+
t.Run("nil server details", func(t *testing.T) {
176+
_, err := ListVersions(nil, "repo", "slug")
177+
require.Error(t, err)
178+
assert.Contains(t, err.Error(), "server details are required")
179+
})
180+
181+
t.Run("empty repo key", func(t *testing.T) {
182+
_, err := ListVersions(&config.ServerDetails{Url: "https://example.com/"}, " ", "slug")
183+
require.Error(t, err)
184+
assert.Contains(t, err.Error(), "repository is required")
185+
})
186+
187+
t.Run("empty slug", func(t *testing.T) {
188+
_, err := ListVersions(&config.ServerDetails{Url: "https://example.com/"}, "repo", " ")
189+
require.Error(t, err)
190+
assert.Contains(t, err.Error(), "skill name is required")
191+
})
192+
}

go.mod

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -12,12 +12,13 @@ require (
1212
github.com/jfrog/gofrog v1.7.6
1313
github.com/jfrog/jfrog-cli-core/v2 v2.60.1-0.20260811051029-e2289bda7c64
1414
github.com/jfrog/jfrog-cli-evidence v0.9.0
15-
github.com/jfrog/jfrog-client-go v1.55.1-0.20260508101905-a17af78a38d7
15+
github.com/jfrog/jfrog-client-go v1.55.1-0.20260825090904-82b4e2241ef9
1616
github.com/pkg/errors v0.9.1
1717
github.com/spf13/viper v1.21.0
1818
github.com/stretchr/testify v1.11.1
1919
golang.org/x/exp v0.0.0-20260527015227-08cc5374adb3
2020
golang.org/x/mod v0.37.0
21+
golang.org/x/term v0.45.0
2122
gopkg.in/ini.v1 v1.67.1
2223
gopkg.in/yaml.v3 v3.0.1
2324
helm.sh/helm/v3 v3.19.2
@@ -110,7 +111,7 @@ require (
110111
github.com/in-toto/attestation v1.2.0 // indirect
111112
github.com/in-toto/in-toto-golang v0.10.0 // indirect
112113
github.com/jbenet/go-context v0.0.0-20150711004518-d14ea06fba99 // indirect
113-
github.com/jfrog/archiver/v3 v3.6.3 // indirect
114+
github.com/jfrog/archiver/v3 v3.6.4 // indirect
114115
github.com/jfrog/froggit-go v1.21.1 // indirect
115116
github.com/kevinburke/ssh_config v1.6.0 // indirect
116117
github.com/klauspost/compress v1.18.6 // indirect
@@ -186,7 +187,6 @@ require (
186187
golang.org/x/oauth2 v0.36.0 // indirect
187188
golang.org/x/sync v0.22.0 // indirect
188189
golang.org/x/sys v0.47.0 // indirect
189-
golang.org/x/term v0.45.0 // indirect
190190
golang.org/x/text v0.40.0 // indirect
191191
golang.org/x/time v0.15.0 // indirect
192192
google.golang.org/genproto/googleapis/api v0.0.0-20260319201613-d00831a3d3e7 // indirect
@@ -204,3 +204,4 @@ require (
204204
// replace github.com/gfleury/go-bitbucket-v1 => github.com/gfleury/go-bitbucket-v1 v0.0.0-20230825095122-9bc1711434ab
205205

206206
// replace github.com/ktrysmt/go-bitbucket => github.com/ktrysmt/go-bitbucket v0.9.80
207+
replace github.com/jfrog/jfrog-client-go => ../jfrog-client-go

go.sum

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -378,6 +378,8 @@ github.com/jellydator/ttlcache/v3 v3.4.0 h1:YS4P125qQS0tNhtL6aeYkheEaB/m8HCqdMMP
378378
github.com/jellydator/ttlcache/v3 v3.4.0/go.mod h1:Hw9EgjymziQD3yGsQdf1FqFdpp7YjFMd4Srg5EJlgD4=
379379
github.com/jfrog/archiver/v3 v3.6.3 h1:hkAmPjBw393tPmQ07JknLNWFNZjXdy2xFEnOW9wwOxI=
380380
github.com/jfrog/archiver/v3 v3.6.3/go.mod h1:5V9l+Fte30Y4qe9dUOAd3yNTf8lmtVNuhKNrvI8PMhg=
381+
github.com/jfrog/archiver/v3 v3.6.4 h1:qHAWCLKwo3+ocHNNoWzGZ8ESl8QQk/lR3W09Pt+ROvE=
382+
github.com/jfrog/archiver/v3 v3.6.4/go.mod h1:5V9l+Fte30Y4qe9dUOAd3yNTf8lmtVNuhKNrvI8PMhg=
381383
github.com/jfrog/build-info-go v1.13.1-0.20260818195724-23e528d30b96 h1:aVT0+x1sn1PW7piZhqErCHnikcwdBQqDyzvvgmin7Bc=
382384
github.com/jfrog/build-info-go v1.13.1-0.20260818195724-23e528d30b96/go.mod h1:CYRUCvLKfyARjoJXLWAxce1qNUxTEtbRKAARkV42vpE=
383385
github.com/jfrog/froggit-go v1.21.1 h1:I/XUOO6GQ1d/rmBlM361F8T654C3ohIWrpw23xNL9JY=

0 commit comments

Comments
 (0)