Skip to content

Commit 4fccd37

Browse files
johnstairsjohnstairs
authored
Fix paging errors (#234)
1 parent 1b9db67 commit 4fccd37

File tree

11 files changed

+150
-62
lines changed

11 files changed

+150
-62
lines changed

‎cli/integrationtest/controlplane_test.go‎

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1214,6 +1214,54 @@ func TestListRunsPaging(t *testing.T) {
12141214
require.Empty(t, runs)
12151215
}
12161216

1217+
func TestListRunsBoundaries(t *testing.T) {
1218+
t.Parallel()
1219+
1220+
_, stderr, _err := runTyger("run", "list", "--limit", "-1")
1221+
require.Error(t, _err)
1222+
require.Contains(t, stderr, "Limit must be a non-negative integer")
1223+
1224+
runs := []model.Run{}
1225+
output := runTygerSucceeds(t, "run", "list", "--limit", "0")
1226+
require.NoError(t, json.Unmarshal([]byte(output), &runs))
1227+
require.Empty(t, runs)
1228+
1229+
output = runTygerSucceeds(t, "run", "list", "--limit", "3000")
1230+
require.NoError(t, json.Unmarshal([]byte(output), &runs))
1231+
}
1232+
1233+
func TestListBuffersBoundaries(t *testing.T) {
1234+
t.Parallel()
1235+
1236+
_, stderr, _err := runTyger("buffer", "list", "--limit", "-1")
1237+
require.Error(t, _err)
1238+
require.Contains(t, stderr, "Limit must be a non-negative integer")
1239+
1240+
buffers := []model.Buffer{}
1241+
output := runTygerSucceeds(t, "buffer", "list", "--limit", "0")
1242+
require.NoError(t, json.Unmarshal([]byte(output), &buffers))
1243+
require.Empty(t, buffers)
1244+
1245+
output = runTygerSucceeds(t, "buffer", "list", "--limit", "3000")
1246+
require.NoError(t, json.Unmarshal([]byte(output), &buffers))
1247+
}
1248+
1249+
func TestListCodespecsBoundaries(t *testing.T) {
1250+
t.Parallel()
1251+
1252+
_, stderr, _err := runTyger("codespec", "list", "--limit", "-1")
1253+
require.Error(t, _err)
1254+
require.Contains(t, stderr, "Limit must be a non-negative integer")
1255+
1256+
codespecs := []model.Codespec{}
1257+
output := runTygerSucceeds(t, "codespec", "list", "--limit", "0")
1258+
require.NoError(t, json.Unmarshal([]byte(output), &codespecs))
1259+
require.Empty(t, codespecs)
1260+
1261+
output = runTygerSucceeds(t, "codespec", "list", "--limit", "3000")
1262+
require.NoError(t, json.Unmarshal([]byte(output), &codespecs))
1263+
}
1264+
12171265
func TestListCodespecsFromCli(t *testing.T) {
12181266
t.Parallel()
12191267
prefix := strings.ToLower(t.Name()) + "_"

‎cli/internal/cmd/buffer.go‎

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,6 @@ import (
1111
"fmt"
1212
"io"
1313
"io/fs"
14-
"math"
1514
"net/http"
1615
"net/url"
1716
"os"
@@ -476,7 +475,7 @@ The SIZE argument must be a number with an optional unit (e.g. 10MB). 1KB and 1K
476475
}
477476

478477
func newBufferListCommand() *cobra.Command {
479-
limit := 0
478+
totalLimit := 1000
480479
tagEntries := make(map[string]string)
481480
excludeTagEntries := make(map[string]string)
482481
softDeleted := false
@@ -488,12 +487,15 @@ func newBufferListCommand() *cobra.Command {
488487
DisableFlagsInUseLine: true,
489488
RunE: func(cmd *cobra.Command, args []string) error {
490489
listOptions := url.Values{}
491-
if limit > 0 {
492-
listOptions.Add("limit", strconv.Itoa(limit))
493-
} else {
494-
limit = math.MaxInt
490+
491+
limitSpecified := cmd.Flags().Lookup("limit").Changed
492+
pageLimit := totalLimit
493+
if !limitSpecified {
494+
pageLimit = totalLimit + 1 // If the limit is not specified, we will fetch one extra item to know check if there are more items.
495495
}
496496

497+
listOptions.Add("limit", strconv.Itoa(pageLimit))
498+
497499
if softDeleted {
498500
listOptions.Add("softDeleted", strconv.FormatBool(softDeleted))
499501
}
@@ -506,14 +508,14 @@ func newBufferListCommand() *cobra.Command {
506508
listOptions.Add(fmt.Sprintf("excludeTag[%s]", name), value)
507509
}
508510

509-
return controlplane.InvokePageRequests[model.Buffer](cmd.Context(), "/buffers", listOptions, limit, !cmd.Flags().Lookup("limit").Changed)
511+
return controlplane.InvokePageRequests[model.Buffer](cmd.Context(), "/buffers", listOptions, totalLimit, !limitSpecified)
510512
},
511513
}
512514

513515
cmd.Flags().StringToStringVar(&tagEntries, "tag", nil, "Only include buffers with the given tag. Can be specified multiple times.")
514516
cmd.Flags().StringToStringVar(&excludeTagEntries, "exclude-tag", nil, "Exclude buffers with the given tag. Can be specified multiple times.")
515517
cmd.Flags().BoolVar(&softDeleted, "soft-deleted", softDeleted, "Only include soft-deleted buffers.")
516-
cmd.Flags().IntVarP(&limit, "limit", "l", 1000, "The maximum number of buffers to list. Default 1000")
518+
cmd.Flags().IntVarP(&totalLimit, "limit", "l", totalLimit, "The maximum number of buffers to list. Default 1000")
517519

518520
return cmd
519521
}

‎cli/internal/cmd/codespec.go‎

Lines changed: 17 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@ import (
77
"encoding/json"
88
"errors"
99
"fmt"
10-
"math"
1110
"net/http"
1211
"net/url"
1312
"os"
@@ -340,10 +339,10 @@ func getCodespecVersionFromResponse(resp *http.Response) (int, error) {
340339
}
341340

342341
func codespecListCommand() *cobra.Command {
343-
var flags struct {
344-
limit int
345-
prefix string
346-
}
342+
var (
343+
totalLimit int = 1000
344+
prefix string
345+
)
347346

348347
cmd := &cobra.Command{
349348
Use: "list [--prefix STRING] [--limit COUNT]",
@@ -353,21 +352,25 @@ func codespecListCommand() *cobra.Command {
353352
Args: cobra.NoArgs,
354353
RunE: func(cmd *cobra.Command, args []string) error {
355354
queryOptions := url.Values{}
356-
if flags.limit > 0 {
357-
queryOptions.Add("limit", strconv.Itoa(flags.limit))
358-
} else {
359-
flags.limit = math.MaxInt
355+
356+
limitSpecified := cmd.Flags().Lookup("limit").Changed
357+
pageLimit := totalLimit
358+
if !limitSpecified {
359+
pageLimit = totalLimit + 1 // If the limit is not specified, we will fetch one extra item to check if there are more items.
360360
}
361-
if flags.prefix != "" {
362-
queryOptions.Add("prefix", flags.prefix)
361+
362+
queryOptions.Add("limit", strconv.Itoa(pageLimit))
363+
364+
if prefix != "" {
365+
queryOptions.Add("prefix", prefix)
363366
}
364367

365-
return controlplane.InvokePageRequests[model.Codespec](cmd.Context(), "/codespecs", queryOptions, flags.limit, !cmd.Flags().Lookup("limit").Changed)
368+
return controlplane.InvokePageRequests[model.Codespec](cmd.Context(), "/codespecs", queryOptions, totalLimit, !limitSpecified)
366369
},
367370
}
368371

369-
cmd.Flags().StringVarP(&flags.prefix, "prefix", "p", "", "Show only codespecs that start with this prefix")
370-
cmd.Flags().IntVarP(&flags.limit, "limit", "l", 1000, "The maximum number of codespecs to list. Default 1000")
372+
cmd.Flags().StringVarP(&prefix, "prefix", "p", "", "Show only codespecs that start with this prefix")
373+
cmd.Flags().IntVarP(&totalLimit, "limit", "l", totalLimit, "The maximum number of codespecs to list")
371374

372375
return cmd
373376
}

‎cli/internal/cmd/run.go‎

Lines changed: 25 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -633,12 +633,13 @@ func newRunWatchCommand() *cobra.Command {
633633
}
634634

635635
func newRunListCommand() *cobra.Command {
636-
var flags struct {
637-
limit int
638-
since string
639-
tags map[string]string
640-
statuses []model.RunStatus
641-
}
636+
637+
var (
638+
totalLimit int = 1000
639+
since string
640+
tags map[string]string
641+
statuses []model.RunStatus
642+
)
642643

643644
cmd := &cobra.Command{
644645
Use: "list [--since DATE/TIME] [--tag key=value ...] [--status STATUS] [--limit COUNT]",
@@ -647,29 +648,33 @@ func newRunListCommand() *cobra.Command {
647648
DisableFlagsInUseLine: true,
648649
RunE: func(cmd *cobra.Command, args []string) error {
649650
queryOptions := url.Values{}
650-
if flags.limit > 0 {
651-
queryOptions.Add("limit", strconv.Itoa(flags.limit))
652-
} else {
653-
flags.limit = math.MaxInt
651+
652+
limitSpecified := cmd.Flags().Lookup("limit").Changed
653+
pageLimit := totalLimit
654+
if !limitSpecified {
655+
pageLimit = totalLimit + 1 // If the limit is not specified, we will fetch one extra item to check if there are more items.
654656
}
655-
if flags.since != "" {
657+
658+
queryOptions.Add("limit", strconv.Itoa(pageLimit))
659+
660+
if since != "" {
656661
now := time.Now()
657-
tm, err := timeparser.ParseTimeStr(flags.since, &now)
662+
tm, err := timeparser.ParseTimeStr(since, &now)
658663
if err != nil {
659-
return fmt.Errorf("failed to parse time %s", flags.since)
664+
return fmt.Errorf("failed to parse time %s", since)
660665
}
661666
queryOptions.Add("since", tm.UTC().Format(time.RFC3339Nano))
662667
}
663668

664-
for k, v := range flags.tags {
669+
for k, v := range tags {
665670
queryOptions.Add(fmt.Sprintf("tag[%s]", k), v)
666671
}
667672

668-
for _, status := range flags.statuses {
673+
for _, status := range statuses {
669674
queryOptions.Add("status", status.String())
670675
}
671676

672-
return controlplane.InvokePageRequests[model.Run](cmd.Context(), "/runs", queryOptions, flags.limit, !cmd.Flags().Lookup("limit").Changed)
677+
return controlplane.InvokePageRequests[model.Run](cmd.Context(), "/runs", queryOptions, totalLimit, !limitSpecified)
673678
},
674679
}
675680

@@ -678,13 +683,13 @@ func newRunListCommand() *cobra.Command {
678683
runStatuses[status] = []string{status.String()}
679684
}
680685

681-
cmd.Flags().StringVarP(&flags.since, "since", "s", "", "Results before this datetime (specified in local time) are not included")
682-
cmd.Flags().StringToStringVar(&flags.tags, "tag", nil, "Only include runs with the given tag. Can be specified multiple times.")
686+
cmd.Flags().StringVarP(&since, "since", "s", "", "Results before this datetime (specified in local time) are not included")
687+
cmd.Flags().StringToStringVar(&tags, "tag", nil, "Only include runs with the given tag. Can be specified multiple times.")
683688
cmd.Flags().Var(
684-
enumflag.NewSlice(&flags.statuses, "status", runStatuses, enumflag.EnumCaseInsensitive),
689+
enumflag.NewSlice(&statuses, "status", runStatuses, enumflag.EnumCaseInsensitive),
685690
"status",
686691
"Only include runs with the given status. When specified multiple times, any of the given statuses are matched.")
687-
cmd.Flags().IntVarP(&flags.limit, "limit", "l", 1000, "The maximum number of runs to list. Default 1000")
692+
cmd.Flags().IntVarP(&totalLimit, "limit", "l", totalLimit, "The maximum number of runs to list")
688693

689694
return cmd
690695
}

‎cli/internal/controlplane/requests.go‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -168,7 +168,7 @@ func InvokeRequest(ctx context.Context, method string, relativeUrl string, query
168168
return resp, nil
169169
}
170170

171-
func InvokePageRequests[T any](ctx context.Context, requestUrl string, queryParams url.Values, limit int, warnIfTruncated bool) error {
171+
func InvokePageRequests[T any](ctx context.Context, requestUrl string, queryParams url.Values, totalLimit int, warnIfTruncated bool) error {
172172
firstPage := true
173173
totalPrinted := 0
174174
truncated := false
@@ -179,6 +179,7 @@ func InvokePageRequests[T any](ctx context.Context, requestUrl string, queryPara
179179
if err != nil {
180180
return err
181181
}
182+
queryParams = nil // Clear query params for subsequent requests
182183

183184
if firstPage && page.NextLink == "" {
184185
formattedRuns, err := json.MarshalIndent(page.Items, " ", " ")
@@ -206,7 +207,7 @@ func InvokePageRequests[T any](ctx context.Context, requestUrl string, queryPara
206207

207208
fmt.Print(string(formattedRun))
208209
totalPrinted++
209-
if totalPrinted == limit {
210+
if totalPrinted == totalLimit {
210211
truncated = i < len(page.Items)-1 || page.NextLink != ""
211212
goto End
212213
}

‎server/Common/Api/QueryParameters.cs‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
11
// Copyright (c) Microsoft Corporation.
22
// Licensed under the MIT License.
33

4+
using System.ComponentModel.DataAnnotations;
5+
46
namespace Tyger.Common.Api;
57

68
public static class QueryParameters
@@ -48,4 +50,14 @@ public static bool ParseAndValidateTtlQueryParameter(this HttpContext context, o
4850

4951
return true;
5052
}
53+
54+
public static uint GetValidatedPageLimit(int? limit, uint defaultLimit = 20, uint maxLimit = 2000)
55+
{
56+
return limit switch
57+
{
58+
null => defaultLimit,
59+
< 0 => throw new ValidationException("Limit must be a non-negative integer."),
60+
_ => (uint)Math.Min(limit.Value, maxLimit)
61+
};
62+
}
5163
}

‎server/ControlPlane/Buffers/BufferManager.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,7 @@ public async Task<UpdateWithPreconditionResult<Buffer>> UpdateBuffer(BufferUpdat
7171
}
7272

7373
public async Task<(IList<Buffer>, string? nextContinuationToken)> GetBuffers(IDictionary<string, string>? tags, IDictionary<string, string>? excludeTags,
74-
bool softDeleted, int limit, string? continuationToken, CancellationToken cancellationToken)
74+
bool softDeleted, uint limit, string? continuationToken, CancellationToken cancellationToken)
7575
{
7676
return await _repository.GetBuffers(tags, excludeTags, softDeleted, limit, continuationToken, cancellationToken);
7777
}

‎server/ControlPlane/Buffers/Buffers.cs‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -79,7 +79,8 @@ public static void MapBuffers(this RouteGroupBuilder root)
7979

8080
buffers.MapGet("/", async (BufferManager manager, HttpContext context, int? limit, [FromQuery(Name = "_ct")] string? continuationToken, CancellationToken cancellationToken) =>
8181
{
82-
limit = limit is null ? 20 : Math.Min(limit.Value, 2000);
82+
var validatedLimit = QueryParameters.GetValidatedPageLimit(limit);
83+
8384
var tagQuery = context.GetTagQueryParameters();
8485
var excludeTagQuery = context.GetTagQueryParameters("excludeTag");
8586

@@ -92,7 +93,7 @@ public static void MapBuffers(this RouteGroupBuilder root)
9293
}
9394
}
9495

95-
(var buffers, var nextContinuationToken) = await manager.GetBuffers(tagQuery, excludeTagQuery, softDeleted, limit.Value, continuationToken, cancellationToken);
96+
(var buffers, var nextContinuationToken) = await manager.GetBuffers(tagQuery, excludeTagQuery, softDeleted, validatedLimit, continuationToken, cancellationToken);
9697

9798
string? nextLink;
9899
if (nextContinuationToken is null)

‎server/ControlPlane/Codespecs/Codespecs.cs‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -66,8 +66,9 @@ public static void MapCodespecs(this RouteGroupBuilder root)
6666

6767
codespecs.MapGet("/", async (Repository repository, int? limit, string? prefix, [FromQuery(Name = "_ct")] string? continuationToken, HttpContext context) =>
6868
{
69-
limit = limit is null ? 20 : Math.Min(limit.Value, 2000);
70-
(var codespecs, var nextContinuationToken) = await repository.GetCodespecs(limit.Value, prefix, continuationToken, context.RequestAborted);
69+
var validatedLimit = QueryParameters.GetValidatedPageLimit(limit);
70+
71+
(var codespecs, var nextContinuationToken) = await repository.GetCodespecs(validatedLimit, prefix, continuationToken, context.RequestAborted);
7172

7273
string? nextLink;
7374
if (nextContinuationToken is null)

0 commit comments

Comments
 (0)