From 80be9eb8595dd9c66757735f41090ca2284962d1 Mon Sep 17 00:00:00 2001 From: brinehart Date: Sun, 1 Nov 2026 22:11:07 -0500 Subject: [PATCH] fix(api): correct GetManyClients paging (#51) Treat request.Page as a 1-based page number (default 1) and reuse request.Skip as the page size (default 10). Compute the offset as (page - 1) * pageSize so consecutive pages no longer overlap, and build NextUrl from the same page/pageSize variables. Set Total on every response and emit NextUrl only when more rows exist beyond the window. Update the client.proto comments for page/skip to match the new meaning (comment-only, no schema or field-number change). Add ClientsGrpcServiceTests covering page 1, a non-overlapping page 2, the last page without NextUrl, default paging, and Total on every response. Alias the transitive MicroZen.OAuth2 reference in the test project so the duplicated MicroZen.Grpc.V1 proto types resolve to MicroZen.Api. Scope: paging only; the searchTerm filter (#50) is untouched. Closes #51 --- .../MicroZen.Api.Test.csproj | 7 + .../Services/ClientsGrpcServiceTests.cs | 204 ++++++++++++++++++ core/csharp/MicroZen.Api/Protos/client.proto | 4 +- .../Services/ClientsGrpcService.cs | 24 ++- 4 files changed, 230 insertions(+), 9 deletions(-) create mode 100644 core/csharp/MicroZen.Api.Test/Services/ClientsGrpcServiceTests.cs diff --git a/core/csharp/MicroZen.Api.Test/MicroZen.Api.Test.csproj b/core/csharp/MicroZen.Api.Test/MicroZen.Api.Test.csproj index bd9d287..bac5c3d 100644 --- a/core/csharp/MicroZen.Api.Test/MicroZen.Api.Test.csproj +++ b/core/csharp/MicroZen.Api.Test/MicroZen.Api.Test.csproj @@ -27,6 +27,13 @@ + + diff --git a/core/csharp/MicroZen.Api.Test/Services/ClientsGrpcServiceTests.cs b/core/csharp/MicroZen.Api.Test/Services/ClientsGrpcServiceTests.cs new file mode 100644 index 0000000..8750f28 --- /dev/null +++ b/core/csharp/MicroZen.Api.Test/Services/ClientsGrpcServiceTests.cs @@ -0,0 +1,204 @@ +using System.Threading; +using Grpc.Core; +using Microsoft.EntityFrameworkCore; +using MicroZen.Api.Context; +using MicroZen.Api.Entities; +using MicroZen.Grpc.V1; +using static Xunit.Assert; +// The service class and its generated gRPC base share the simple name "ClientsGrpcService" +// (MicroZen.Api.Services vs MicroZen.Grpc.V1); alias the concrete service under test. +using ClientsGrpcService = MicroZen.Api.Services.ClientsGrpcService; + +namespace MicroZen.Core.Api.Test.Services; + +/// +/// Tests for paging behaviour (issue #51). +/// +/// Follows the reference shape in Context/MicroZenContextTests.cs: a unique +/// in-memory database per fixture, the EncryptionKey set in the constructor and +/// cleared in , and GIVEN / WHEN / THEN comments. +/// +/// After the #51 fix: +/// - request.Page is a 1-based page number (default 1). +/// - request.Skip is the page size (default 10). +/// - The offset is (page - 1) * pageSize, so pages never overlap. +/// - NextUrl carries page={page+1} and skip={pageSize}, and is absent on the last page. +/// - Total is set on every response. +/// +public class ClientsGrpcServiceTests : IDisposable +{ + private const int TotalClients = 25; + private const int PageSize = 10; + + private readonly DbContextOptions _options; + + public ClientsGrpcServiceTests() + { + // Required by MicroZenContext.OnModelCreating (column encryption). + Environment.SetEnvironmentVariable("EncryptionKey", "1234567890"); + + // A unique database name isolates this fixture from other test classes. + _options = new DbContextOptionsBuilder() + .UseInMemoryDatabase(databaseName: $"ClientsGrpcServiceTests_{Guid.NewGuid():N}") + .Options; + } + + /// + /// Seeds API clients with zero-padded, name-ordered + /// names ("Client 01" .. "Client 25") so OrderBy(c => c.Name) is deterministic. + /// + private async Task SeedClientsAsync() + { + await using var context = new MicroZenContext(_options); + for (var i = 1; i <= TotalClients; i++) + { + context.Clients.Add(new Client + { + Name = $"Client {i:D2}", + Type = ClientType.Api, + Description = $"Seeded client {i:D2}", + }); + } + await context.SaveChangesAsync(); + } + + private static ManyClientsRequest Request(int page, int skip) => + new() { Type = ClientType.Api, Page = page, Skip = skip }; + + [Fact] + public async Task GetManyClients_Page1_ReturnsFirstWindowWithNextUrl() + { + // GIVEN 25 seeded API clients. + await SeedClientsAsync(); + await using var context = new MicroZenContext(_options); + var service = new ClientsGrpcService(context); + + // WHEN the first page is requested with a page size of 10. + var response = await service.GetManyClients(Request(page: 1, skip: PageSize), new TestServerCallContext()); + + // THEN the first 10 clients by name are returned, Total is 25, and NextUrl points to page 2. + Equal(PageSize, response.Clients.Count); + Equal(TotalClients, response.Total); + Equal( + new[] { "Client 01", "Client 02", "Client 03", "Client 04", "Client 05", "Client 06", "Client 07", "Client 08", "Client 09", "Client 10" }, + response.Clients.Select(c => c.Name)); + NotNull(response.NextUrl); + Contains("page=2", response.NextUrl); + Contains("skip=10", response.NextUrl); + } + + [Fact] + public async Task GetManyClients_Page2_IsNonOverlappingWithPage1() + { + // GIVEN 25 seeded API clients. + await SeedClientsAsync(); + await using var context = new MicroZenContext(_options); + var service = new ClientsGrpcService(context); + + // WHEN page 1 and page 2 are both requested with a page size of 10. + var page1 = await service.GetManyClients(Request(page: 1, skip: PageSize), new TestServerCallContext()); + var page2 = await service.GetManyClients(Request(page: 2, skip: PageSize), new TestServerCallContext()); + + // THEN page 2 returns clients 11..20 and shares no client with page 1. + Equal(PageSize, page2.Clients.Count); + Equal( + new[] { "Client 11", "Client 12", "Client 13", "Client 14", "Client 15", "Client 16", "Client 17", "Client 18", "Client 19", "Client 20" }, + page2.Clients.Select(c => c.Name)); + + var page1Names = page1.Clients.Select(c => c.Name).ToHashSet(); + var page2Names = page2.Clients.Select(c => c.Name).ToHashSet(); + Empty(page1Names.Intersect(page2Names)); + + NotNull(page2.NextUrl); + Contains("page=3", page2.NextUrl); + Contains("skip=10", page2.NextUrl); + } + + [Fact] + public async Task GetManyClients_LastPage_EmitsNoNextUrl() + { + // GIVEN 25 seeded API clients. + await SeedClientsAsync(); + await using var context = new MicroZenContext(_options); + var service = new ClientsGrpcService(context); + + // WHEN the final page (page 3, size 10) is requested. + var response = await service.GetManyClients(Request(page: 3, skip: PageSize), new TestServerCallContext()); + + // THEN clients 21..25 are returned, Total is 25, and there is no NextUrl. + Equal(5, response.Clients.Count); + Equal(TotalClients, response.Total); + Equal( + new[] { "Client 21", "Client 22", "Client 23", "Client 24", "Client 25" }, + response.Clients.Select(c => c.Name)); + True(string.IsNullOrEmpty(response.NextUrl)); + } + + [Fact] + public async Task GetManyClients_WithoutPagingArguments_AppliesDefaults() + { + // GIVEN 25 seeded API clients. + await SeedClientsAsync(); + await using var context = new MicroZenContext(_options); + var service = new ClientsGrpcService(context); + + // WHEN no paging arguments are supplied (Page=0, Skip=0). + var response = await service.GetManyClients(Request(page: 0, skip: 0), new TestServerCallContext()); + + // THEN it behaves as page 1 with the default size 10 (10 rows from offset 0). + Equal(PageSize, response.Clients.Count); + Equal(TotalClients, response.Total); + Equal( + new[] { "Client 01", "Client 02", "Client 03", "Client 04", "Client 05", "Client 06", "Client 07", "Client 08", "Client 09", "Client 10" }, + response.Clients.Select(c => c.Name)); + } + + [Fact] + public async Task GetManyClients_SetsTotalOnEveryResponse() + { + // GIVEN 25 seeded API clients. + await SeedClientsAsync(); + await using var context = new MicroZenContext(_options); + var service = new ClientsGrpcService(context); + + // WHEN each page is requested in turn. + var page1 = await service.GetManyClients(Request(page: 1, skip: PageSize), new TestServerCallContext()); + var page2 = await service.GetManyClients(Request(page: 2, skip: PageSize), new TestServerCallContext()); + var lastPage = await service.GetManyClients(Request(page: 3, skip: PageSize), new TestServerCallContext()); + + // THEN Total is set on every response, whether or not a next page exists. + Equal(TotalClients, page1.Total); + Equal(TotalClients, page2.Total); + Equal(TotalClients, lastPage.Total); + } + + public void Dispose() + { + GC.SuppressFinalize(this); + Environment.SetEnvironmentVariable("EncryptionKey", null); + } + + /// + /// Minimal stub. + /// never reads the call context, so every member is a no-op default. + /// + private sealed class TestServerCallContext : ServerCallContext + { + protected override string MethodCore => string.Empty; + protected override string HostCore => string.Empty; + protected override string PeerCore => string.Empty; + protected override DateTime DeadlineCore => DateTime.MaxValue; + protected override Metadata RequestHeadersCore => new(); + protected override CancellationToken CancellationTokenCore => CancellationToken.None; + protected override Metadata ResponseTrailersCore => new(); + protected override Status StatusCore { get; set; } + protected override WriteOptions? WriteOptionsCore { get; set; } + protected override AuthContext AuthContextCore => new(string.Empty, new Dictionary>()); + + protected override ContextPropagationToken CreatePropagationTokenCore(ContextPropagationOptions? options) => + throw new NotSupportedException(); + + protected override Task WriteResponseHeadersAsyncCore(Metadata responseHeaders) => + Task.CompletedTask; + } +} diff --git a/core/csharp/MicroZen.Api/Protos/client.proto b/core/csharp/MicroZen.Api/Protos/client.proto index bcf6eb7..36c334a 100644 --- a/core/csharp/MicroZen.Api/Protos/client.proto +++ b/core/csharp/MicroZen.Api/Protos/client.proto @@ -129,9 +129,9 @@ message ManyClientsRequest { ClientType type = 1; // The search term to filter for Clients by google.protobuf.StringValue searchTerm = 2; - // How many clients to return. Defaults to 10 if none specified. + // The 1-based page number to return. Defaults to 1 if none specified. int32 page = 3; - // How many clients to skip. Defaults to 0 if none specified. + // The page size (number of clients per page). Defaults to 10 if none specified. int32 skip = 4; } diff --git a/core/csharp/MicroZen.Api/Services/ClientsGrpcService.cs b/core/csharp/MicroZen.Api/Services/ClientsGrpcService.cs index c0cbdeb..6ed38e4 100644 --- a/core/csharp/MicroZen.Api/Services/ClientsGrpcService.cs +++ b/core/csharp/MicroZen.Api/Services/ClientsGrpcService.cs @@ -16,6 +16,11 @@ namespace MicroZen.Api.Services; /// public class ClientsGrpcService(MicroZenContext db) : MicroZen.Grpc.V1.ClientsGrpcService.ClientsGrpcServiceBase { + /// + /// The default number of clients to return per page when the caller does not specify a page size. + /// + private const int DefaultPageSize = 10; + /// public override async Task GetManyClients(ManyClientsRequest request, ServerCallContext context) { @@ -28,8 +33,10 @@ public override async Task GetManyClients(ManyClientsReques } var response = new ManyClientsResponse(); - var page = request.Page > 0 ? request.Page : 10; - var skip = request.Skip > 0 ? request.Skip : 0; + var page = request.Page > 0 ? request.Page : 1; // 1-based page number + var pageSize = request.Skip > 0 ? request.Skip : DefaultPageSize; // rows per page + var skip = (page - 1) * pageSize; + var total = await db.Clients.Where(predicate).CountAsync(); var clients = await db.Clients .Include(c => c.OAuth2Credentials) @@ -37,14 +44,17 @@ public override async Task GetManyClients(ManyClientsReques .Where(predicate) .OrderBy(c => c.Name) .Skip(skip) - .Take(page) + .Take(pageSize) .Select(c => c.ToMessage()) .ToListAsync(); - var anyMoreClients = await db.Clients.Where(predicate).Take(page + 1).Skip(skip).AnyAsync(); - if (anyMoreClients) + + // There is a next page only if more rows exist beyond the current window. + var hasMore = skip + clients.Count < total; + response.Total = total; + if (hasMore) { - response.NextUrl = $"clients?page={request.Page + 1}&skip={request.Skip}&searchTerm={request.SearchTerm}&type={request.Type.ToString().ToUpper()}"; - response.Total = total; + response.NextUrl = + $"clients?page={page + 1}&skip={pageSize}&searchTerm={request.SearchTerm}&type={request.Type.ToString().ToUpper()}"; } response.Clients.AddRange(clients); return response;