feat(management): additive Skip/Take paging on template/instance lists (arch-review P2)

Claude-Session: https://claude.ai/code/session_01MtdgwpEeCUn6cUA5f1LMPj
This commit is contained in:
Joseph Doherty
2026-07-10 05:57:14 -04:00
parent 8221ee797e
commit 655834f0b8
10 changed files with 152 additions and 13 deletions
+2 -2
View File
@@ -56,7 +56,7 @@ tree at the time of writing.
### Template Commands ### Template Commands
``` ```
scadabridge template list scadabridge template list [--detail] [--skip <n>] [--take <n>]
scadabridge template get --id <id> scadabridge template get --id <id>
scadabridge template create --name <name> [--description <desc>] [--parent-id <id>] scadabridge template create --name <name> [--description <desc>] [--parent-id <id>]
scadabridge template update --id <id> [--name <name>] [--description <desc>] [--parent-id <id>] scadabridge template update --id <id> [--name <name>] [--description <desc>] [--parent-id <id>]
@@ -80,7 +80,7 @@ scadabridge template native-alarm-source remove --id <id>
### Instance Commands ### Instance Commands
``` ```
scadabridge instance list [--site-id <id>] [--template-id <id>] [--search <term>] scadabridge instance list [--site-id <id>] [--template-id <id>] [--search <term>] [--skip <n>] [--take <n>]
scadabridge instance get --id <id> scadabridge instance get --id <id>
scadabridge instance create --name <name> --template-id <id> --site-id <id> [--area-id <id>] scadabridge instance create --name <name> --template-id <id> --site-id <id> [--area-id <id>]
scadabridge instance set-bindings --id <id> --bindings <json> scadabridge instance set-bindings --id <id> --bindings <json>
@@ -87,7 +87,7 @@ Both endpoints honour any site-scope rules attached to the caller's audit role b
### Templates ### 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. - **CreateTemplate** / **UpdateTemplate** / **DeleteTemplate**: Manage templates.
- **ValidateTemplate**: Run on-demand pre-deployment validation (flattening, naming collisions, script compilation). - **ValidateTemplate**: Run on-demand pre-deployment validation (flattening, naming collisions, script compilation).
- **GetTemplateDiff**: Compare deployed vs. template-derived configuration for an instance. - **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 ### 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. - **CreateInstance**: Create a new instance from a template.
- **UpdateInstanceOverrides**: Set attribute overrides on an instance. - **UpdateInstanceOverrides**: Set attribute overrides on an instance.
- **SetInstanceAlarmOverride** / **DeleteInstanceAlarmOverride** / **ListInstanceAlarmOverrides**: Manage per-instance computed-alarm overrides. - **SetInstanceAlarmOverride** / **DeleteInstanceAlarmOverride** / **ListInstanceAlarmOverrides**: Manage per-instance computed-alarm overrides.
@@ -212,19 +212,25 @@ public static class InstanceCommands
var siteIdOption = new Option<int?>("--site-id") { Description = "Filter by site ID" }; var siteIdOption = new Option<int?>("--site-id") { Description = "Filter by site ID" };
var templateIdOption = new Option<int?>("--template-id") { Description = "Filter by template ID" }; var templateIdOption = new Option<int?>("--template-id") { Description = "Filter by template ID" };
var searchOption = new Option<string?>("--search") { Description = "Search term" }; var searchOption = new Option<string?>("--search") { Description = "Search term" };
var skipOption = new Option<int>("--skip") { Description = "Offset paging: number of items to skip (default 0)" };
var takeOption = new Option<int?>("--take") { Description = "Offset paging: max items to return (1..1000; omit for unlimited)" };
var cmd = new Command("list") { Description = "List instances" }; var cmd = new Command("list") { Description = "List instances" };
cmd.Add(siteIdOption); cmd.Add(siteIdOption);
cmd.Add(templateIdOption); cmd.Add(templateIdOption);
cmd.Add(searchOption); cmd.Add(searchOption);
cmd.Add(skipOption);
cmd.Add(takeOption);
cmd.SetAction(async (ParseResult result) => cmd.SetAction(async (ParseResult result) =>
{ {
var siteId = result.GetValue(siteIdOption); var siteId = result.GetValue(siteIdOption);
var templateId = result.GetValue(templateIdOption); var templateId = result.GetValue(templateIdOption);
var search = result.GetValue(searchOption); var search = result.GetValue(searchOption);
var skip = result.GetValue(skipOption);
var take = result.GetValue(takeOption);
return await CommandHelpers.ExecuteCommandAsync( return await CommandHelpers.ExecuteCommandAsync(
result, urlOption, formatOption, usernameOption, passwordOption, result, urlOption, formatOption, usernameOption, passwordOption,
new ListInstancesCommand(siteId, templateId, search)); new ListInstancesCommand(siteId, templateId, search, skip, take));
}); });
return cmd; return cmd;
} }
@@ -59,13 +59,19 @@ public static class TemplateCommands
Description = "Include full template definitions (all attributes/alarms/scripts) in table output. " 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." + "Without it, table output is a compact summary (counts only). JSON output is always full."
}; };
var skipOption = new Option<int>("--skip") { Description = "Offset paging: number of items to skip (default 0)" };
var takeOption = new Option<int?>("--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)" }; var cmd = new Command("list") { Description = "List all templates (compact table summary; use --detail for the full dump)" };
cmd.Add(detailOption); cmd.Add(detailOption);
cmd.Add(skipOption);
cmd.Add(takeOption);
cmd.SetAction(async (ParseResult result) => cmd.SetAction(async (ParseResult result) =>
{ {
var detail = result.GetValue(detailOption); var detail = result.GetValue(detailOption);
var skip = result.GetValue(skipOption);
var take = result.GetValue(takeOption);
return await CommandHelpers.ExecuteCommandAsync( 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); tableProjector: detail ? null : TemplateTableProjection.ProjectSummary);
}); });
return cmd; return cmd;
+5 -1
View File
@@ -101,6 +101,8 @@ scadabridge --url <url> --format json template list # full JSON (always)
| Option | Required | Description | | Option | Required | Description |
|--------|----------|-------------| |--------|----------|-------------|
| `--detail` | no | Include full definitions in table output (no effect on JSON) | | `--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` #### `template get`
@@ -450,7 +452,7 @@ scadabridge --url <url> instance get --id <int>
List instances, with optional filters. List instances, with optional filters.
```sh ```sh
scadabridge --url <url> instance list [--site-id <int>] [--template-id <int>] [--search <string>] scadabridge --url <url> instance list [--site-id <int>] [--template-id <int>] [--search <string>] [--skip <int>] [--take <int>]
``` ```
| Option | Required | Description | | Option | Required | Description |
@@ -458,6 +460,8 @@ scadabridge --url <url> instance list [--site-id <int>] [--template-id <int>] [-
| `--site-id` | no | Filter by site ID | | `--site-id` | no | Filter by site ID |
| `--template-id` | no | Filter by template ID | | `--template-id` | no | Filter by template ID |
| `--search` | no | Search term matched against instance name | | `--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` #### `instance create`
@@ -1,6 +1,8 @@
namespace ZB.MOM.WW.ScadaBridge.Commons.Messages.Management; 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 GetInstanceCommand(int InstanceId);
public record CreateInstanceCommand(string UniqueName, int TemplateId, int SiteId, int? AreaId = null); public record CreateInstanceCommand(string UniqueName, int TemplateId, int SiteId, int? AreaId = null);
public record MgmtDeployInstanceCommand(int InstanceId); public record MgmtDeployInstanceCommand(int InstanceId);
@@ -1,6 +1,8 @@
namespace ZB.MOM.WW.ScadaBridge.Commons.Messages.Management; 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 GetTemplateCommand(int TemplateId);
public record CreateTemplateCommand(string Name, string? Description, int? ParentTemplateId); public record CreateTemplateCommand(string Name, string? Description, int? ParentTemplateId);
@@ -295,7 +295,7 @@ public class ManagementActor : ReceiveActor
return command switch return command switch
{ {
// Templates // Templates
ListTemplatesCommand => await HandleListTemplates(sp), ListTemplatesCommand cmd => await HandleListTemplates(sp, cmd),
GetTemplateCommand cmd => await HandleGetTemplate(sp, cmd), GetTemplateCommand cmd => await HandleGetTemplate(sp, cmd),
CreateTemplateCommand cmd => await HandleCreateTemplate(sp, cmd, user.Username), CreateTemplateCommand cmd => await HandleCreateTemplate(sp, cmd, user.Username),
UpdateTemplateCommand cmd => await HandleUpdateTemplate(sp, cmd, user.Username), UpdateTemplateCommand cmd => await HandleUpdateTemplate(sp, cmd, user.Username),
@@ -559,12 +559,22 @@ public class ManagementActor : ReceiveActor
// Template handlers // Template handlers
// ======================================================================== // ========================================================================
private static async Task<object?> HandleListTemplates(IServiceProvider sp) private static async Task<object?> HandleListTemplates(IServiceProvider sp, ListTemplatesCommand cmd)
{ {
var repo = sp.GetRequiredService<ITemplateEngineRepository>(); var repo = sp.GetRequiredService<ITemplateEngineRepository>();
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<T> Page<T>(IEnumerable<T> 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<object?> HandleGetTemplate(IServiceProvider sp, GetTemplateCommand cmd) private static async Task<object?> HandleGetTemplate(IServiceProvider sp, GetTemplateCommand cmd)
{ {
var repo = sp.GetRequiredService<ITemplateEngineRepository>(); var repo = sp.GetRequiredService<ITemplateEngineRepository>();
@@ -780,7 +790,9 @@ public class ManagementActor : ReceiveActor
var permittedIds = new HashSet<string>(user.PermittedSiteIds); var permittedIds = new HashSet<string>(user.PermittedSiteIds);
instances = instances.Where(i => permittedIds.Contains(i.SiteId.ToString())).ToList(); 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<object?> HandleGetInstance(IServiceProvider sp, GetInstanceCommand cmd, AuthenticatedUser user) private static async Task<object?> HandleGetInstance(IServiceProvider sp, GetInstanceCommand cmd, AuthenticatedUser user)
@@ -151,6 +151,41 @@ public class CommandTreeTests
Assert.Contains("clear", subNames); 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<int?>)takeOption));
}
[Theory] [Theory]
[InlineData(typeof(GetInstanceCommand))] [InlineData(typeof(GetInstanceCommand))]
[InlineData(typeof(ListSitesCommand))] [InlineData(typeof(ListSitesCommand))]
@@ -1378,6 +1378,78 @@ public class ManagementActorTests : TestKit, IDisposable
_artifactService.DidNotReceiveWithAnyArgs().DeployToAllSitesAsync(default!, default); _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<CancellationToken>()).Returns(templates);
var actor = CreateActor();
actor.Tell(Envelope(new ListTemplatesCommand(Skip: 10, Take: 5)));
var response = ExpectMsg<ManagementSuccess>(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<CancellationToken>()).Returns(templates);
var actor = CreateActor();
actor.Tell(Envelope(new ListTemplatesCommand())); // Take = null => unlimited
var response = ExpectMsg<ManagementSuccess>(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<ICentralUiRepository>();
uiRepo.GetInstancesFilteredAsync(Arg.Any<int?>(), Arg.Any<int?>(), Arg.Any<string?>(), Arg.Any<CancellationToken>())
.Returns(new List<Instance>
{
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<ManagementSuccess>(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) // SetInstanceOverrides atomicity (finding -015)
// //