diff --git a/docs/requirements/Component-CLI.md b/docs/requirements/Component-CLI.md index 77eef4f3..e0b67a04 100644 --- a/docs/requirements/Component-CLI.md +++ b/docs/requirements/Component-CLI.md @@ -56,7 +56,7 @@ tree at the time of writing. ### Template Commands ``` -scadabridge template list +scadabridge template list [--detail] [--skip ] [--take ] scadabridge template get --id scadabridge template create --name [--description ] [--parent-id ] scadabridge template update --id [--name ] [--description ] [--parent-id ] @@ -80,7 +80,7 @@ scadabridge template native-alarm-source remove --id ### Instance Commands ``` -scadabridge instance list [--site-id ] [--template-id ] [--search ] +scadabridge instance list [--site-id ] [--template-id ] [--search ] [--skip ] [--take ] scadabridge instance get --id scadabridge instance create --name --template-id --site-id [--area-id ] scadabridge instance set-bindings --id --bindings diff --git a/docs/requirements/Component-ManagementService.md b/docs/requirements/Component-ManagementService.md index f3667e0a..4b1ab9df 100644 --- a/docs/requirements/Component-ManagementService.md +++ b/docs/requirements/Component-ManagementService.md @@ -87,7 +87,7 @@ Both endpoints honour any site-scope rules attached to the caller's audit role b ### Templates -- **ListTemplates** / **GetTemplate**: Query template definitions. +- **ListTemplates** / **GetTemplate**: Query template definitions. `ListTemplates` supports additive offset paging via `Skip` / `Take` (arch-review P2): `Take = null` (the default) preserves the historical unpaged behaviour (unlimited), a set `Take` is clamped to `1..1000` and `Skip` floors at `0`. - **CreateTemplate** / **UpdateTemplate** / **DeleteTemplate**: Manage templates. - **ValidateTemplate**: Run on-demand pre-deployment validation (flattening, naming collisions, script compilation). - **GetTemplateDiff**: Compare deployed vs. template-derived configuration for an instance. @@ -112,7 +112,7 @@ Both endpoints honour any site-scope rules attached to the caller's audit role b ### Instances -- **ListInstances** / **GetInstance**: Query instances, with filtering by site and area. +- **ListInstances** / **GetInstance**: Query instances, with filtering by site and area. `ListInstances` supports additive offset paging via `Skip` / `Take` (arch-review P2), applied **after** the in-memory site-scope filter so a site-scoped user never sees an out-of-scope instance surface into their page window; same `Take = null` unlimited default and `1..1000` clamp as `ListTemplates`. (`QueryDeployments` and `ExportBundle` still load the full table internally — a logged scale follow-up, arch-review P2 deferred.) - **CreateInstance**: Create a new instance from a template. - **UpdateInstanceOverrides**: Set attribute overrides on an instance. - **SetInstanceAlarmOverride** / **DeleteInstanceAlarmOverride** / **ListInstanceAlarmOverrides**: Manage per-instance computed-alarm overrides. diff --git a/src/ZB.MOM.WW.ScadaBridge.CLI/Commands/InstanceCommands.cs b/src/ZB.MOM.WW.ScadaBridge.CLI/Commands/InstanceCommands.cs index 9a6fcadf..84de0585 100644 --- a/src/ZB.MOM.WW.ScadaBridge.CLI/Commands/InstanceCommands.cs +++ b/src/ZB.MOM.WW.ScadaBridge.CLI/Commands/InstanceCommands.cs @@ -212,19 +212,25 @@ public static class InstanceCommands var siteIdOption = new Option("--site-id") { Description = "Filter by site ID" }; var templateIdOption = new Option("--template-id") { Description = "Filter by template ID" }; var searchOption = new Option("--search") { Description = "Search term" }; + var skipOption = new Option("--skip") { Description = "Offset paging: number of items to skip (default 0)" }; + var takeOption = new Option("--take") { Description = "Offset paging: max items to return (1..1000; omit for unlimited)" }; var cmd = new Command("list") { Description = "List instances" }; cmd.Add(siteIdOption); cmd.Add(templateIdOption); cmd.Add(searchOption); + cmd.Add(skipOption); + cmd.Add(takeOption); cmd.SetAction(async (ParseResult result) => { var siteId = result.GetValue(siteIdOption); var templateId = result.GetValue(templateIdOption); var search = result.GetValue(searchOption); + var skip = result.GetValue(skipOption); + var take = result.GetValue(takeOption); return await CommandHelpers.ExecuteCommandAsync( result, urlOption, formatOption, usernameOption, passwordOption, - new ListInstancesCommand(siteId, templateId, search)); + new ListInstancesCommand(siteId, templateId, search, skip, take)); }); return cmd; } diff --git a/src/ZB.MOM.WW.ScadaBridge.CLI/Commands/TemplateCommands.cs b/src/ZB.MOM.WW.ScadaBridge.CLI/Commands/TemplateCommands.cs index df94a89e..7d12163f 100644 --- a/src/ZB.MOM.WW.ScadaBridge.CLI/Commands/TemplateCommands.cs +++ b/src/ZB.MOM.WW.ScadaBridge.CLI/Commands/TemplateCommands.cs @@ -59,13 +59,19 @@ public static class TemplateCommands Description = "Include full template definitions (all attributes/alarms/scripts) in table output. " + "Without it, table output is a compact summary (counts only). JSON output is always full." }; + var skipOption = new Option("--skip") { Description = "Offset paging: number of items to skip (default 0)" }; + var takeOption = new Option("--take") { Description = "Offset paging: max items to return (1..1000; omit for unlimited)" }; var cmd = new Command("list") { Description = "List all templates (compact table summary; use --detail for the full dump)" }; cmd.Add(detailOption); + cmd.Add(skipOption); + cmd.Add(takeOption); cmd.SetAction(async (ParseResult result) => { var detail = result.GetValue(detailOption); + var skip = result.GetValue(skipOption); + var take = result.GetValue(takeOption); return await CommandHelpers.ExecuteCommandAsync( - result, urlOption, formatOption, usernameOption, passwordOption, new ListTemplatesCommand(), + result, urlOption, formatOption, usernameOption, passwordOption, new ListTemplatesCommand(skip, take), tableProjector: detail ? null : TemplateTableProjection.ProjectSummary); }); return cmd; diff --git a/src/ZB.MOM.WW.ScadaBridge.CLI/README.md b/src/ZB.MOM.WW.ScadaBridge.CLI/README.md index 8322bf03..5b6505c0 100644 --- a/src/ZB.MOM.WW.ScadaBridge.CLI/README.md +++ b/src/ZB.MOM.WW.ScadaBridge.CLI/README.md @@ -101,6 +101,8 @@ scadabridge --url --format json template list # full JSON (always) | Option | Required | Description | |--------|----------|-------------| | `--detail` | no | Include full definitions in table output (no effect on JSON) | +| `--skip` | no | Offset paging: number of items to skip (default 0) | +| `--take` | no | Offset paging: max items to return (clamped 1..1000; omit for unlimited) | #### `template get` @@ -450,7 +452,7 @@ scadabridge --url instance get --id List instances, with optional filters. ```sh -scadabridge --url instance list [--site-id ] [--template-id ] [--search ] +scadabridge --url instance list [--site-id ] [--template-id ] [--search ] [--skip ] [--take ] ``` | Option | Required | Description | @@ -458,6 +460,8 @@ scadabridge --url instance list [--site-id ] [--template-id ] [- | `--site-id` | no | Filter by site ID | | `--template-id` | no | Filter by template ID | | `--search` | no | Search term matched against instance name | +| `--skip` | no | Offset paging: number of items to skip (default 0). Applied after site-scope filtering. | +| `--take` | no | Offset paging: max items to return (clamped 1..1000; omit for unlimited) | #### `instance create` diff --git a/src/ZB.MOM.WW.ScadaBridge.Commons/Messages/Management/InstanceCommands.cs b/src/ZB.MOM.WW.ScadaBridge.Commons/Messages/Management/InstanceCommands.cs index 84b2a2cd..229da1cd 100644 --- a/src/ZB.MOM.WW.ScadaBridge.Commons/Messages/Management/InstanceCommands.cs +++ b/src/ZB.MOM.WW.ScadaBridge.Commons/Messages/Management/InstanceCommands.cs @@ -1,6 +1,8 @@ namespace ZB.MOM.WW.ScadaBridge.Commons.Messages.Management; -public record ListInstancesCommand(int? SiteId = null, int? TemplateId = null, string? SearchTerm = null); +// Skip/Take are additive offset paging (arch-review P2), applied AFTER in-memory +// site-scope filtering. Take = null preserves the historical unpaged behaviour. +public record ListInstancesCommand(int? SiteId = null, int? TemplateId = null, string? SearchTerm = null, int Skip = 0, int? Take = null); public record GetInstanceCommand(int InstanceId); public record CreateInstanceCommand(string UniqueName, int TemplateId, int SiteId, int? AreaId = null); public record MgmtDeployInstanceCommand(int InstanceId); diff --git a/src/ZB.MOM.WW.ScadaBridge.Commons/Messages/Management/TemplateCommands.cs b/src/ZB.MOM.WW.ScadaBridge.Commons/Messages/Management/TemplateCommands.cs index bf04c079..35178d9b 100644 --- a/src/ZB.MOM.WW.ScadaBridge.Commons/Messages/Management/TemplateCommands.cs +++ b/src/ZB.MOM.WW.ScadaBridge.Commons/Messages/Management/TemplateCommands.cs @@ -1,6 +1,8 @@ namespace ZB.MOM.WW.ScadaBridge.Commons.Messages.Management; -public record ListTemplatesCommand; +// Skip/Take are additive offset paging (arch-review P2). Take = null preserves the +// historical unpaged behaviour (unlimited), so existing callers/UI are untouched. +public record ListTemplatesCommand(int Skip = 0, int? Take = null); public record GetTemplateCommand(int TemplateId); public record CreateTemplateCommand(string Name, string? Description, int? ParentTemplateId); diff --git a/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs b/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs index e8ddf3d5..5b820136 100644 --- a/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs +++ b/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs @@ -295,7 +295,7 @@ public class ManagementActor : ReceiveActor return command switch { // Templates - ListTemplatesCommand => await HandleListTemplates(sp), + ListTemplatesCommand cmd => await HandleListTemplates(sp, cmd), GetTemplateCommand cmd => await HandleGetTemplate(sp, cmd), CreateTemplateCommand cmd => await HandleCreateTemplate(sp, cmd, user.Username), UpdateTemplateCommand cmd => await HandleUpdateTemplate(sp, cmd, user.Username), @@ -559,12 +559,22 @@ public class ManagementActor : ReceiveActor // Template handlers // ======================================================================== - private static async Task HandleListTemplates(IServiceProvider sp) + private static async Task HandleListTemplates(IServiceProvider sp, ListTemplatesCommand cmd) { var repo = sp.GetRequiredService(); - return await repo.GetAllTemplatesAsync(); + var templates = await repo.GetAllTemplatesAsync(); + return Page(templates, cmd.Skip, cmd.Take); } + // Additive offset paging (arch-review P2). Take = null (or non-positive) keeps + // today's unlimited behaviour; Take is clamped to 1..1000; Skip floors at 0. + // Applied AFTER any in-memory scope filtering by the caller. + private static List Page(IEnumerable source, int skip, int? take) => + source + .Skip(Math.Max(0, skip)) + .Take(take is > 0 and <= 1000 ? take.Value : int.MaxValue) + .ToList(); + private static async Task HandleGetTemplate(IServiceProvider sp, GetTemplateCommand cmd) { var repo = sp.GetRequiredService(); @@ -780,7 +790,9 @@ public class ManagementActor : ReceiveActor var permittedIds = new HashSet(user.PermittedSiteIds); instances = instances.Where(i => permittedIds.Contains(i.SiteId.ToString())).ToList(); } - return instances; + // Paging is applied AFTER the site-scope filter so a scoped user never sees + // an out-of-scope instance surface into their page window. + return Page(instances, cmd.Skip, cmd.Take); } private static async Task HandleGetInstance(IServiceProvider sp, GetInstanceCommand cmd, AuthenticatedUser user) diff --git a/tests/ZB.MOM.WW.ScadaBridge.CLI.Tests/CommandTreeTests.cs b/tests/ZB.MOM.WW.ScadaBridge.CLI.Tests/CommandTreeTests.cs index 5ea591cf..91029ba3 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.CLI.Tests/CommandTreeTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.CLI.Tests/CommandTreeTests.cs @@ -151,6 +151,41 @@ public class CommandTreeTests Assert.Contains("clear", subNames); } + [Fact] + public void TemplateList_HasSkipAndTakePagingOptions() + { + // arch-review P2: `template list` gained additive --skip/--take offset paging. + var template = TemplateCommands.Build(Url, Format, Username, Password); + var list = template.Subcommands.Single(c => c.Name == "list"); + var optionNames = list.Options.Select(o => o.Name).ToList(); + Assert.Contains("--skip", optionNames); + Assert.Contains("--take", optionNames); + } + + [Fact] + public void InstanceList_HasSkipAndTakePagingOptions() + { + // arch-review P2: `instance list` gained additive --skip/--take offset paging. + var instance = InstanceCommands.Build(Url, Format, Username, Password); + var list = instance.Subcommands.Single(c => c.Name == "list"); + var optionNames = list.Options.Select(o => o.Name).ToList(); + Assert.Contains("--skip", optionNames); + Assert.Contains("--take", optionNames); + } + + [Fact] + public void InstanceList_ParsesTakeOptionValue() + { + // Parse-level check that --take binds to an int value on the list command. + var instance = InstanceCommands.Build(Url, Format, Username, Password); + var list = instance.Subcommands.Single(c => c.Name == "list"); + var takeOption = list.Options.Single(o => o.Name == "--take"); + + var parse = list.Parse(new[] { "--skip", "10", "--take", "5" }); + Assert.Empty(parse.Errors); + Assert.Equal(5, parse.GetValue((Option)takeOption)); + } + [Theory] [InlineData(typeof(GetInstanceCommand))] [InlineData(typeof(ListSitesCommand))] diff --git a/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/ManagementActorTests.cs b/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/ManagementActorTests.cs index 0ac90f32..ea1b4d0a 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/ManagementActorTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/ManagementActorTests.cs @@ -1378,6 +1378,78 @@ public class ManagementActorTests : TestKit, IDisposable _artifactService.DidNotReceiveWithAnyArgs().DeployToAllSitesAsync(default!, default); } + // ======================================================================== + // Additive Skip/Take paging on template/instance lists (arch-review P2) + // + // Take = null preserves the historical unlimited behaviour. When set, paging + // applies AFTER any in-memory site-scope filtering, so a scoped user never sees + // an out-of-scope entity surface into their page window. + // ======================================================================== + + [Fact] + public void ListTemplates_SkipTake_ReturnsRequestedWindow() + { + var templates = Enumerable.Range(1, 30) + .Select(i => new Template($"T-{i:D3}") { Id = i }) + .ToList(); + _templateRepo.GetAllTemplatesAsync(Arg.Any()).Returns(templates); + + var actor = CreateActor(); + actor.Tell(Envelope(new ListTemplatesCommand(Skip: 10, Take: 5))); + + var response = ExpectMsg(TimeSpan.FromSeconds(5)); + // Items 11..15 (1-based) present; the pages on either side absent. + Assert.Contains("T-011", response.JsonData); + Assert.Contains("T-015", response.JsonData); + Assert.DoesNotContain("T-010", response.JsonData); + Assert.DoesNotContain("T-016", response.JsonData); + } + + [Fact] + public void ListTemplates_DefaultTake_ReturnsAll() + { + var templates = Enumerable.Range(1, 30) + .Select(i => new Template($"T-{i:D3}") { Id = i }) + .ToList(); + _templateRepo.GetAllTemplatesAsync(Arg.Any()).Returns(templates); + + var actor = CreateActor(); + actor.Tell(Envelope(new ListTemplatesCommand())); // Take = null => unlimited + + var response = ExpectMsg(TimeSpan.FromSeconds(5)); + Assert.Contains("T-001", response.JsonData); + Assert.Contains("T-030", response.JsonData); + } + + [Fact] + public void ListInstances_PagingAppliesAfterSiteScopeFilter() + { + // Scoped user permitted to site 1 only. The repo returns 3 in-scope (site 1) + // instances plus 2 out-of-scope (site 2). Take:2 must yield 2 of THEIR + // instances and never an out-of-scope one. + var uiRepo = Substitute.For(); + uiRepo.GetInstancesFilteredAsync(Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(new List + { + new("I-001") { Id = 1, SiteId = 1 }, + new("I-002") { Id = 2, SiteId = 1 }, + new("I-003") { Id = 3, SiteId = 1 }, + new("I-101") { Id = 101, SiteId = 2 }, + new("I-102") { Id = 102, SiteId = 2 }, + }); + _services.AddScoped(_ => uiRepo); + + var actor = CreateActor(); + actor.Tell(ScopedEnvelope(new ListInstancesCommand(Take: 2), new[] { "1" }, "Deployer")); + + var response = ExpectMsg(TimeSpan.FromSeconds(5)); + Assert.Contains("I-001", response.JsonData); + Assert.Contains("I-002", response.JsonData); + Assert.DoesNotContain("I-003", response.JsonData); // paged out + Assert.DoesNotContain("I-101", response.JsonData); // out of scope + Assert.DoesNotContain("I-102", response.JsonData); // out of scope + } + // ======================================================================== // SetInstanceOverrides atomicity (finding -015) //