fix(management): ListInstances pages, site-scopes and prefix-searches in SQL (review 2026-10-21 08/P2)
This commit is contained in:
@@ -120,7 +120,7 @@ The whole folder surface is reachable from the CLI as `template folder list|crea
|
||||
|
||||
### Instances
|
||||
|
||||
- **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`. (`ExportBundle` still loads the full table internally — a logged scale follow-up, arch-review P2 deferred. `QueryDeployments` was closed by WP2.5, below.)
|
||||
- **ListInstances** / **GetInstance**: Query instances, with filtering by site and area. `ListInstances` supports additive offset paging via `Skip` / `Take` (arch-review P2); same `Take = null` unlimited default and `1..1000` clamp as `ListTemplates`. Paging, the site-scope filter and search all run **in SQL** (`ICentralUiRepository.GetInstancesPagedAsync`, review 2026-10-21 08/P2): a site-scoped caller's `PermittedSiteIds` become the query's site filter (intersected with an explicit `SiteId`; a non-integer grant is dropped, which can only narrow), so a page can never surface an out-of-scope instance. **`SearchTerm` is a unique-name prefix match (`StartsWith`)**, not a substring match — the previous leading-wildcard `LIKE '%term%'` could not use `IX_Instances_UniqueName`. The response is still a bare array (the total count is not returned). (`ExportBundle` still loads the full table internally — a logged scale follow-up, arch-review P2 deferred. `QueryDeployments` was closed by WP2.5, below.)
|
||||
- **CreateInstance**: Create a new instance from a template.
|
||||
- **UpdateInstanceOverrides** (`SetInstanceOverrides`): Set attribute overrides on an instance. Every requested attribute is validated against the instance's template up front (unknown or locked ⇒ the whole batch is rejected before any write), then the batch is applied as **one bulk read of the existing override rows plus one commit** (WP2.5). It previously ran K independent read-modify-commit cycles — each re-reading the instance, the template's attributes and the existing overrides, then committing and writing its own audit row — so the all-or-nothing promise was only as good as the pre-validation. It is now a real single-transaction apply, with one audit row summarising the batch.
|
||||
- **MgmtDeploySite** (`SiteId`, WP2.5): Bulk-deploy every deployable instance at one site. Site scope is enforced on the SITE (the command's target); every instance the batch touches belongs to that site by construction. Same `Deployer` authority as `MgmtDeployInstance` — it is N of those, not a new class of privilege — and registered as a long-running command so the caller does not time out mid-batch. Semantics, phasing and failure policy: see `Component-DeploymentManager.md` → Bulk site deployment.
|
||||
|
||||
@@ -573,7 +573,7 @@ scadabridge --url <url> instance list [--site-id <int>] [--template-id <int>] [-
|
||||
|--------|----------|-------------|
|
||||
| `--site-id` | no | Filter by site ID |
|
||||
| `--template-id` | no | Filter by template ID |
|
||||
| `--search` | no | Search term matched against instance name |
|
||||
| `--search` | no | Unique-name **prefix** (`StartsWith`; was a substring match before review 2026-10-21 08/P2) |
|
||||
| `--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) |
|
||||
|
||||
|
||||
@@ -946,17 +946,29 @@ public class ManagementActor : ReceiveActor
|
||||
|
||||
private static async Task<object?> HandleListInstances(IServiceProvider sp, ListInstancesCommand cmd, AuthenticatedUser user)
|
||||
{
|
||||
var repo = sp.GetRequiredService<ICentralUiRepository>();
|
||||
var instances = await repo.GetInstancesFilteredAsync(cmd.SiteId, cmd.TemplateId, cmd.SearchTerm);
|
||||
// Filter by permitted sites for site-scoped users
|
||||
// Paged, site-scoped and searched IN SQL (review 2026-10-21 08/P2): this used to read
|
||||
// every matching instance (the whole table for an unfiltered CLI `instance list`),
|
||||
// scope-filter and page in memory, and search with a leading-wildcard LIKE. The scope
|
||||
// is passed as the repository's site filter, so a page can never surface an
|
||||
// out-of-scope instance. Search is a unique-name PREFIX (StartsWith), which can use
|
||||
// IX_Instances_UniqueName. The response stays a bare array; the total is not returned.
|
||||
IReadOnlyCollection<int>? siteIds = cmd.SiteId is { } siteId ? new[] { siteId } : null;
|
||||
if (user.PermittedSiteIds.Length > 0 && !user.Roles.Contains(Roles.Administrator, StringComparer.OrdinalIgnoreCase))
|
||||
{
|
||||
var permittedIds = new HashSet<string>(user.PermittedSiteIds);
|
||||
instances = instances.Where(i => permittedIds.Contains(i.SiteId.ToString())).ToList();
|
||||
// A non-integer grant is DROPPED (can only narrow), never ignored (would widen).
|
||||
var permitted = user.PermittedSiteIds
|
||||
.Select(id => int.TryParse(id, out var parsed) ? (int?)parsed : null)
|
||||
.OfType<int>()
|
||||
.ToHashSet();
|
||||
siteIds = siteIds is null
|
||||
? permitted.ToList()
|
||||
: siteIds.Where(permitted.Contains).ToList();
|
||||
}
|
||||
// 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);
|
||||
|
||||
var take = cmd.Take is > 0 ? Math.Min(cmd.Take.Value, 1000) : int.MaxValue;
|
||||
var repo = sp.GetRequiredService<ICentralUiRepository>();
|
||||
var (rows, _) = await repo.GetInstancesPagedAsync(siteIds, cmd.TemplateId, cmd.SearchTerm, Math.Max(0, cmd.Skip), take);
|
||||
return rows;
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
|
||||
@@ -2490,34 +2490,67 @@ public class ManagementActorTests : TestKit, IDisposable
|
||||
Assert.Contains("T-030", response.JsonData);
|
||||
}
|
||||
|
||||
/// <summary>Verifies that list instances: paging applies after site scope filter.</summary>
|
||||
/// <summary>
|
||||
/// Verifies that list instances pages and site-scopes IN SQL (review 2026-10-21 08/P2): a
|
||||
/// scoped user's permitted sites are passed as the repository's site filter, so paging
|
||||
/// can never surface an out-of-scope instance, and Skip/Take/search reach the query.
|
||||
/// </summary>
|
||||
[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>
|
||||
IReadOnlyCollection<int>? passedSites = null;
|
||||
uiRepo.GetInstancesPagedAsync(
|
||||
Arg.Do<IReadOnlyCollection<int>?>(s => passedSites = s), null, "I-", 0, 2, 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 },
|
||||
});
|
||||
}, 3));
|
||||
_services.AddScoped(_ => uiRepo);
|
||||
|
||||
var actor = CreateActor();
|
||||
actor.Tell(ScopedEnvelope(new ListInstancesCommand(Take: 2), new[] { "1" }, "Deployer"));
|
||||
actor.Tell(ScopedEnvelope(new ListInstancesCommand(SearchTerm: "I-", Take: 2), new[] { "1", "not-an-int" }, "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
|
||||
Assert.StartsWith("[", response.JsonData.TrimStart()); // response shape unchanged: a bare array
|
||||
Assert.Equal(new[] { 1 }, passedSites); // non-integer grant dropped, never widened
|
||||
uiRepo.DidNotReceive().GetInstancesFilteredAsync(
|
||||
Arg.Any<int?>(), Arg.Any<int?>(), Arg.Any<string?>(), Arg.Any<CancellationToken>());
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// A scoped user's explicit SiteId is intersected with their grant; an out-of-grant site
|
||||
/// yields an empty filter (nothing), and an unscoped caller's SiteId passes through.
|
||||
/// </summary>
|
||||
[Fact]
|
||||
public void ListInstances_SiteIdIsIntersectedWithTheScope_AndUnlimitedTakeIsUnbounded()
|
||||
{
|
||||
var uiRepo = Substitute.For<ICentralUiRepository>();
|
||||
var calls = new List<(IReadOnlyCollection<int>? Sites, int Take)>();
|
||||
uiRepo.GetInstancesPagedAsync(Arg.Any<IReadOnlyCollection<int>?>(), Arg.Any<int?>(), Arg.Any<string?>(),
|
||||
Arg.Any<int>(), Arg.Any<int>(), Arg.Any<CancellationToken>())
|
||||
.Returns(ci =>
|
||||
{
|
||||
calls.Add(((IReadOnlyCollection<int>?)ci[0], (int)ci[4]));
|
||||
return (Array.Empty<Instance>(), 0);
|
||||
});
|
||||
_services.AddScoped(_ => uiRepo);
|
||||
var actor = CreateActor();
|
||||
|
||||
actor.Tell(ScopedEnvelope(new ListInstancesCommand(SiteId: 2), new[] { "1" }, "Deployer"));
|
||||
ExpectMsg<ManagementSuccess>(TimeSpan.FromSeconds(5));
|
||||
actor.Tell(Envelope(new ListInstancesCommand(SiteId: 2), "Deployer"));
|
||||
ExpectMsg<ManagementSuccess>(TimeSpan.FromSeconds(5));
|
||||
actor.Tell(Envelope(new ListInstancesCommand(), "Deployer"));
|
||||
ExpectMsg<ManagementSuccess>(TimeSpan.FromSeconds(5));
|
||||
|
||||
Assert.Empty(calls[0].Sites!);
|
||||
Assert.Equal(new[] { 2 }, calls[1].Sites);
|
||||
Assert.Null(calls[2].Sites);
|
||||
Assert.Equal(int.MaxValue, calls[2].Take);
|
||||
}
|
||||
|
||||
// ========================================================================
|
||||
|
||||
Reference in New Issue
Block a user