FOUNDATIONS: Add Unit Tests For Sdk Services Clients And Brokers

.github/workflows/build.yml

@@ -30,19 +30,68 @@ jobs:
       run: >-
         $projects = Get-ChildItem -Path . -Filter "*Tests.Unit*.csproj" -Recurse
 
+        if ($projects.Count -eq 0) {
+          Write-Host "::error::No test projects matched *Tests.Unit*.csproj"
+          exit 1
+        }
+
+        $failedProjects = @()
+
         foreach ($project in $projects) {
           Write-Host "Running tests for: $($project.FullName)"
           dotnet test $project.FullName --no-build --verbosity normal
+          if ($LASTEXITCODE -ne 0) { $failedProjects += $project.Name }
+        }
+
+        if ($failedProjects.Count -gt 0) {
+          Write-Host "::error::Test failures in: $($failedProjects -join ', ')"
+          exit 1
         }
       shell: pwsh
     - name: Run Acceptance Tests
       run: >-
         $projects = Get-ChildItem -Path . -Filter "*Tests.Acceptance*.csproj" -Recurse
 
+        if ($projects.Count -eq 0) {
+          Write-Host "::error::No test projects matched *Tests.Acceptance*.csproj"
+          exit 1
+        }
+
+        $failedProjects = @()
+
         foreach ($project in $projects) {
           Write-Host "Running tests for: $($project.FullName)"
           dotnet test $project.FullName --no-build --verbosity normal
+          if ($LASTEXITCODE -ne 0) { $failedProjects += $project.Name }
+        }
+
+        if ($failedProjects.Count -gt 0) {
+          Write-Host "::error::Test failures in: $($failedProjects -join ', ')"
+          exit 1
         }
+      shell: pwsh
+    - name: Run Integration Tests
+      run: >-
+        $projects = Get-ChildItem -Path . -Filter "*Tests.Integration*.csproj" -Recurse
+
+        if ($projects.Count -eq 0) {
+          Write-Host "::error::No test projects matched *Tests.Integration*.csproj"
+          exit 1
+        }
+
+        $failedProjects = @()
+
+        foreach ($project in $projects) {
+          Write-Host "Running tests for: $($project.FullName)"
+          dotnet test $project.FullName --no-build --verbosity normal
+          if ($LASTEXITCODE -ne 0) { $failedProjects += $project.Name }
+        }
+
+        if ($failedProjects.Count -gt 0) {
+          Write-Host "::error::Test failures in: $($failedProjects -join ', ')"
+          exit 1
+        }
+      shell: pwsh
   add_tag:
     name: Tag and Release
     runs-on: ubuntu-latest

Documentation/DependencyGraph/README.md

@@ -46,12 +46,12 @@ view you were on, and switching carries your current selection across.
 - Whatever is selected is outlined and lettered in **amber**; rows the traced
   path passes through carry a faint blue tint. Click the background or Reset
   to clear. Search finds components and methods. The **utility brokers**
-  toggle reveals the DateTime / Identifier broker copies that are hidden by
+  toggle reveals the DateTime / Identifier / Logging broker copies and the logging external that are hidden by
   default for readability.
 
-At the last scan, 27 declared components and 84 declared edges draw as
+At the last scan, 29 declared components and 86 declared edges draw as
 **25 components · 79 flows** in the single-copy view and **100 nodes ·
-413 flows** per consumer (27 · 84 and 113 · 441 with utility brokers on).
+413 flows** per consumer (29 · 86 and 115 · 443 with utility brokers on).
 
 `.github/workflows/pages.yml` publishes this folder to GitHub Pages on every
 push to `main` that touches it — `index.html` is the site root. Nothing is
@@ -71,9 +71,15 @@ enabled once in the repository's Settings → Pages (source: GitHub Actions).
   `IApiPlatformClient` holding the same two sub-clients, but nothing
   constructs or registers it: `AddApiPlatformSdkCore` registers a hand-built
   `ApiPlatformClient` instead. It shows on the graph with no inbound flows.
-- **`PdsOrchestrationService` takes `IApiPlatformTokenBroker` and never uses
-  it.** The access token comes from `CareIdentityService.GetAccessTokenAsync`;
-  the injected broker is unused.
+- **`LoggingBroker` has no inbound flows on this graph, by design.** Every
+  service takes `ILoggingBroker`, but it is only ever reached from the
+  `CreateAndLog*` exception factories, and this graph draws happy-path calls
+  only. It is a utility broker, so it is hidden behind the toggle along with
+  the DateTime and Identifier brokers.
+- **Dependency failures are categorised by HTTP status.** Both foundation
+  services split `HttpRequestException`: a 4xx becomes a
+  `*DependencyValidationException` (the caller sent something the dependency
+  rejected), a 5xx or a transport failure becomes a `*DependencyException`.
 - **The storage brokers are the extension seam.** `IApiPlatformStateBroker`
   and `IApiPlatformTokenBroker` each have an in-memory implementation in the
   Sdk and a session-backed one in Sdk.AspNetCore. Both are registered with

Documentation/DependencyGraph/graph-data.js

@@ -35,6 +35,7 @@
     { id: "ext-aspnetcore", name: "ASP.NET Core", kind: "external" },
     { id: "ext-bcl", name: ".NET base class library", kind: "external" },
     { id: "ext-adotnet", name: "ADotNet", kind: "external" },
+    { id: "ext-logging", name: "Microsoft.Extensions.Logging", kind: "external" },
     { id: "ext-nhs", name: "NHS Digital API Platform (remote)", kind: "external" },
   ];
 
@@ -69,6 +70,9 @@
       description: "ASP.NET Core session state. The Sdk.AspNetCore brokers throw when there is no HttpContext or session — the host must have called UseSession()." });
   C({ id: "EXT.Bcl", name: "System.Security.Cryptography / Text.Json", project: "ext-bcl", layer: "external", col: 8, shared: true, methods: [],
       description: "RandomNumberGenerator for the CSRF state, System.Text.Json (Web defaults) for payloads, Guid.NewGuid for the PDS X-Request-ID, DateTimeOffset.UtcNow for token expiry." });
+  C({ id: "EXT.Logging", name: "Microsoft.Extensions.Logging", project: "ext-logging", layer: "external", col: 8,
+      shared: true, utility: true, methods: [],
+      description: "AddApiPlatformSdkCore calls AddLogging(), so a host that configures no providers still resolves an ILoggerFactory and the SDK's error logging goes nowhere rather than failing." });
   C({ id: "EXT.Cis2", name: "NHS CIS2 (Care Identity Service)", project: "ext-nhs", layer: "external", col: 8, shared: true, methods: [],
       description: "OAuth2 authorization-code flow without PKCE — CIS2 does not support it. Auth, token and userinfo endpoints come from CareIdentityConfigurations." });
   C({ id: "EXT.Pds", name: "NHS Personal Demographics Service", project: "ext-nhs", layer: "external", col: 8, shared: true, methods: [],
@@ -121,7 +125,7 @@
      ================================================================== */
   C({ id: "PDS.Orchestration", name: "PdsOrchestrationService", project: "sdk", layer: "orchestration", col: 3,
       methods: ["SearchPatientsAsync"],
-      description: "Gets a CIS2 access token, refuses the call with UnauthorizedPdsOrchestrationException when it comes back empty, then hands it to PdsService. NOTE: it also takes IApiPlatformTokenBroker in its constructor but never calls it — the token comes from CareIdentityService." });
+      description: "Validates the search criteria, gets a CIS2 access token, refuses the call with UnauthorizedPdsOrchestrationException when it comes back empty, then hands it to PdsService." });
   D(["PDS.Orchestration", "SearchPatientsAsync"], ["CIS.Foundation", "GetAccessTokenAsync"]);
   D(["PDS.Orchestration", "SearchPatientsAsync"], ["PDS.Foundation", "SearchPatientsAsync"]);
 
@@ -196,6 +200,12 @@
       methods: ["GetNewGuid"] });
   D(["IdentifierBroker", "GetNewGuid"], ["EXT.Bcl", "Guid.NewGuid"]);
 
+  C({ id: "LoggingBroker", name: "LoggingBroker", project: "sdk", layer: "broker", col: 5, utility: true,
+      methods: ["LogErrorAsync", "LogCriticalAsync"],
+      description: "Wraps ILogger<LoggingBroker>. Every service takes it, but it is only ever reached from the CreateAndLog* exception factories — which this graph deliberately does not draw — so it has no inbound flows here. Registered with TryAddSingleton so a host can substitute its own." });
+  D(["LoggingBroker", "LogErrorAsync"], ["EXT.Logging", "ILogger.LogError"]);
+  D(["LoggingBroker", "LogCriticalAsync"], ["EXT.Logging", "ILogger.LogCritical"]);
+
   /* -- the two swappable storage brokers -------------------------------
      Both interfaces have an in-memory implementation shipped in the Sdk
      and a session-backed one in Sdk.AspNetCore. Which one you get is a
@@ -262,22 +272,22 @@
     "CIS.Client", "PDS.Client",
     "CIS.Processing", "PDS.Orchestration",
     "CIS.Foundation", "PDS.Foundation",
-    "HttpBroker", "CryptoBroker", "JsonBroker", "DateTimeBroker", "IdentifierBroker",
+    "HttpBroker", "CryptoBroker", "JsonBroker", "DateTimeBroker", "IdentifierBroker", "LoggingBroker",
     "StateBroker", "TokenBroker", "MemoryStateBroker", "MemoryTokenBroker",
     // NHSDigital.ApiPlatform.Sdk.AspNetCore
     "SessionStateBroker", "SessionTokenBroker",
     // NHSDigital.ApiPlatform.Infrastructure
     "INF.Program", "INF.ScriptGeneration",
     // externals
-    "EXT.HttpClientFactory", "EXT.Session", "EXT.Bcl", "EXT.Cis2", "EXT.Pds", "EXT.ADotNet",
+    "EXT.HttpClientFactory", "EXT.Session", "EXT.Bcl", "EXT.Logging", "EXT.Cis2", "EXT.Pds", "EXT.ADotNet",
   );
 
   /* ------------------------------------------------------------------
      Externals show exactly the public surface this solution calls.
      Derive their method rows from the declared edges so the rows and
      the arrows can never drift apart.
      ------------------------------------------------------------------ */
-  for (const extId of ["EXT.HttpClientFactory", "EXT.Session", "EXT.Bcl", "EXT.Cis2", "EXT.Pds", "EXT.ADotNet"]) {
+  for (const extId of ["EXT.HttpClientFactory", "EXT.Session", "EXT.Bcl", "EXT.Logging", "EXT.Cis2", "EXT.Pds", "EXT.ADotNet"]) {
     const comp = components.find(c => c.id === extId);
     const called = [];
     for (const e of edges) {

NHSDigital.ApiPlatform.Infrastructure/Services/ScriptGenerationService.cs

@@ -80,27 +80,21 @@ public void GenerateBuildScript(string branchName, string projectName, string do
                                 {
                                     Name = "Run Unit Tests",
                                     Shell = "pwsh",
-                                    Run =
-                                        """
-                                        $projects = Get-ChildItem -Path . -Filter "*Tests.Unit*.csproj" -Recurse
-                                        foreach ($project in $projects) {
-                                          Write-Host "Running tests for: $($project.FullName)"
-                                          dotnet test $project.FullName --no-build --verbosity normal
-                                        }
-                                        """
+                                    Run = CreateTestRunScript("*Tests.Unit*.csproj")
                                 },
 
                                 new TestTask
                                 {
                                     Name = "Run Acceptance Tests",
-                                    Run =
-                                        """
-                                        $projects = Get-ChildItem -Path . -Filter "*Tests.Acceptance*.csproj" -Recurse
-                                        foreach ($project in $projects) {
-                                          Write-Host "Running tests for: $($project.FullName)"
-                                          dotnet test $project.FullName --no-build --verbosity normal
-                                        }
-                                        """
+                                    Shell = "pwsh",
+                                    Run = CreateTestRunScript("*Tests.Acceptance*.csproj")
+                                },
+
+                                new TestTask
+                                {
+                                    Name = "Run Integration Tests",
+                                    Shell = "pwsh",
+                                    Run = CreateTestRunScript("*Tests.Integration*.csproj")
                                 }
                             }
                         }
@@ -197,5 +191,30 @@ public void GeneratePrLintScript(string branchName)
                 adoPipeline: githubPipeline,
                 path: buildScriptPath);
         }
+
+        /// <summary>
+        /// Runs every test project matching <paramref name="projectFilter"/> and fails the step if ANY of
+        /// them failed. A bare loop reports only the last project's exit code, which silently hides a
+        /// failure in every project but the last. An empty match is also a failure — it means the glob has
+        /// drifted away from the projects it was meant to cover.
+        /// </summary>
+        private static string CreateTestRunScript(string projectFilter) =>
+            $$"""
+            $projects = Get-ChildItem -Path . -Filter "{{projectFilter}}" -Recurse
+            if ($projects.Count -eq 0) {
+              Write-Host "::error::No test projects matched {{projectFilter}}"
+              exit 1
+            }
+            $failedProjects = @()
+            foreach ($project in $projects) {
+              Write-Host "Running tests for: $($project.FullName)"
+              dotnet test $project.FullName --no-build --verbosity normal
+              if ($LASTEXITCODE -ne 0) { $failedProjects += $project.Name }
+            }
+            if ($failedProjects.Count -gt 0) {
+              Write-Host "::error::Test failures in: $($failedProjects -join ', ')"
+              exit 1
+            }
+            """;
     }
 }

NHSDigital.ApiPlatform.Sdk.AspNetCore.Tests.Unit/Brokers/Storages/FakeSession.cs

@@ -0,0 +1,32 @@
+// ---------------------------------------------------------
+// Copyright (c) North East London ICB. All rights reserved.
+// ---------------------------------------------------------
+
+using System.Collections.Generic;
+using System.Threading;
+using System.Threading.Tasks;
+using Microsoft.AspNetCore.Http;
+
+namespace NHSDigital.ApiPlatform.Sdk.AspNetCore.Tests.Unit.Brokers.Storages
+{
+    internal sealed class FakeSession : ISession
+    {
+        private readonly Dictionary<string, byte[]> store = new Dictionary<string, byte[]>();
+
+        public bool IsAvailable => true;
+        public string Id => "fake-session";
+        public IEnumerable<string> Keys => this.store.Keys;
+
+        public void Clear() => this.store.Clear();
+
+        public Task CommitAsync(CancellationToken cancellationToken = default) => Task.CompletedTask;
+
+        public Task LoadAsync(CancellationToken cancellationToken = default) => Task.CompletedTask;
+
+        public void Remove(string key) => this.store.Remove(key);
+
+        public void Set(string key, byte[] value) => this.store[key] = value;
+
+        public bool TryGetValue(string key, out byte[] value) => this.store.TryGetValue(key, out value);
+    }
+}

NHSDigital.ApiPlatform.Sdk.AspNetCore.Tests.Unit/Brokers/Storages/SessionApiPlatformStateBrokerTests.cs

@@ -0,0 +1,102 @@
+// ---------------------------------------------------------
+// Copyright (c) North East London ICB. All rights reserved.
+// ---------------------------------------------------------
+
+using System;
+using System.Threading.Tasks;
+using FluentAssertions;
+using Microsoft.AspNetCore.Http;
+using Moq;
+using NHSDigital.ApiPlatform.Sdk.AspNetCore.Brokers.Storages;
+using NHSDigital.ApiPlatform.Sdk.Brokers.Storages;
+using Tynamix.ObjectFiller;
+using Xunit;
+
+namespace NHSDigital.ApiPlatform.Sdk.AspNetCore.Tests.Unit.Brokers.Storages
+{
+    public class SessionApiPlatformStateBrokerTests
+    {
+        private readonly Mock<IHttpContextAccessor> httpContextAccessorMock;
+        private readonly FakeSession fakeSession;
+        private readonly IApiPlatformStateBroker apiPlatformStateBroker;
+
+        public SessionApiPlatformStateBrokerTests()
+        {
+            this.httpContextAccessorMock = new Mock<IHttpContextAccessor>();
+            this.fakeSession = new FakeSession();
+
+            var httpContext = new DefaultHttpContext
+            {
+                Session = this.fakeSession
+            };
+
+            this.httpContextAccessorMock.Setup(accessor =>
+                accessor.HttpContext)
+                    .Returns(httpContext);
+
+            this.apiPlatformStateBroker =
+                new SessionApiPlatformStateBroker(this.httpContextAccessorMock.Object);
+        }
+
+        [Fact]
+        public async Task ShouldStoreCsrfStateAsync()
+        {
+            // given
+            string randomState = GetRandomString();
+
+            // when
+            await this.apiPlatformStateBroker.StoreCsrfStateAsync(randomState);
+
+            // then
+            string actualState = await this.apiPlatformStateBroker.GetCsrfStateAsync();
+            actualState.Should().Be(randomState);
+        }
+
+        [Fact]
+        public async Task ShouldReturnNullOnGetCsrfStateIfStateWasNeverStoredAsync()
+        {
+            // given
+            // when
+            string actualState = await this.apiPlatformStateBroker.GetCsrfStateAsync();
+
+            // then
+            actualState.Should().BeNull();
+        }
+
+        [Fact]
+        public async Task ShouldClearCsrfStateAsync()
+        {
+            // given
+            await this.apiPlatformStateBroker.StoreCsrfStateAsync(GetRandomString());
+
+            // when
+            await this.apiPlatformStateBroker.ClearCsrfStateAsync();
+
+            // then
+            string actualState = await this.apiPlatformStateBroker.GetCsrfStateAsync();
+            actualState.Should().BeNull();
+        }
+
+        [Fact]
+        public async Task ShouldThrowInvalidOperationExceptionOnStoreCsrfStateIfHttpContextIsMissingAsync()
+        {
+            // given
+            var httpContextAccessorMock = new Mock<IHttpContextAccessor>();
+
+            httpContextAccessorMock.Setup(accessor =>
+                accessor.HttpContext)
+                    .Returns((HttpContext)null);
+
+            IApiPlatformStateBroker stateBroker =
+                new SessionApiPlatformStateBroker(httpContextAccessorMock.Object);
+
+            // when
+            // then
+            await Assert.ThrowsAsync<InvalidOperationException>(async () =>
+                await stateBroker.StoreCsrfStateAsync(GetRandomString()));
+        }
+
+        private static string GetRandomString() =>
+            new MnemonicString(wordCount: 1, wordMinLength: 8, wordMaxLength: 12).GetValue();
+    }
+}