From 298bd4bfe521416f3802c2520b690699019df7d8 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Fri, 19 Jun 2026 12:29:39 -0400 Subject: [PATCH] review(Driver.OpcUaClient.Browser): add JsonStringEnumConverter (systemic enum bug) Cross-module fix from the review sweep. -003 (Medium): the browser's JsonOpts lacked JsonStringEnumConverter (the factory+probe both carry it), so AdminUI string-enum configs (AuthType/SecurityPolicy/SecurityMode/TargetNamespaceKind) threw on deserialize. Added the converter (accepts string AND numeric) + TDD. --- .../Driver.OpcUaClient.Browser/findings.md | 23 +++++++++ .../OpcUaClientDriverBrowser.cs | 11 ++++- .../OpcUaClientBrowseSessionTests.cs | 8 +-- .../OpcUaClientDriverBrowserTests.cs | 49 +++++++++++++++++++ 4 files changed, 86 insertions(+), 5 deletions(-) diff --git a/code-reviews/Driver.OpcUaClient.Browser/findings.md b/code-reviews/Driver.OpcUaClient.Browser/findings.md index ea4d9d9e..d7740991 100644 --- a/code-reviews/Driver.OpcUaClient.Browser/findings.md +++ b/code-reviews/Driver.OpcUaClient.Browser/findings.md @@ -70,3 +70,26 @@ The identical pattern exists in the runtime driver (`OpcUaClientDriver.cs`), mak **Recommendation:** Catch `OperationCanceledException` in the pagination loop, issue a fire-and-forget `BrowseNext(releaseContinuationPoints: true, continuationPoints: [cp])` before re-throwing, and apply the same fix to `Driver.OpcUaClient`. **Resolution:** Deferred — awaiting a cross-cutting fix that also updates `Driver.OpcUaClient`. Short session lifetime of the AdminUI picker limits practical impact. + +--- + +## Re-review 2026-06-19 + +Commit `7e1f34da`. Single targeted finding surfaced during code review. + +### Driver.OpcUaClient.Browser-003 + +| Field | Value | +|---|---| +| Severity | Medium | +| Category | Correctness & logic bugs / OtOpcUa conventions | +| Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Browser/OpcUaClientDriverBrowser.cs:19` | +| Status | Resolved | + +**Description:** `OpcUaClientDriverBrowser`'s static `JsonOpts` (`JsonSerializerOptions`) was constructed without a `JsonStringEnumConverter`. AdminUI emits enum-valued driver-config fields (`SecurityPolicy`, `SecurityMode`, `AuthType`, `TargetNamespaceKind`) as their **string** names (e.g. `"AuthType":"Certificate"`, `"SecurityPolicy":"Basic256Sha256"`). Without the converter, `System.Text.Json` throws a `JsonException` during deserialization of any such config, surfacing a confusing parse error instead of the browser's own domain-specific validation message. + +This is the same systemic enum-serialization bug fixed in the factory (`OpcUaClientDriverFactoryExtensions.JsonOptions`) and probe (`OpcUaClientDriverProbe._opts`), both of which carry the converter with an explicit comment stating the browser must parse configs the same way. The browser was the one site left unpatched. + +**Recommendation:** Add `new JsonStringEnumConverter()` to the browser's `JsonSerializerOptions.Converters`, matching the factory/probe pattern exactly. `JsonStringEnumConverter` accepts both string names and numeric ordinals, so existing numeric-authored configs require no migration. + +**Resolution:** Fixed 2026-06-19 (SHA `7e1f34da` working tree) — added `new JsonStringEnumConverter()` to `OpcUaClientDriverBrowser.JsonOpts.Converters` and imported `System.Text.Json.Serialization`; regression tests `OpenAsync_with_string_enum_AuthType_deserializes_correctly` and `OpenAsync_with_numeric_enum_AuthType_still_works` added to the unit test project. diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Browser/OpcUaClientDriverBrowser.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Browser/OpcUaClientDriverBrowser.cs index dd0a0e64..9d0e4f2d 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Browser/OpcUaClientDriverBrowser.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Browser/OpcUaClientDriverBrowser.cs @@ -1,4 +1,5 @@ using System.Text.Json; +using System.Text.Json.Serialization; using Microsoft.Extensions.Logging; using Microsoft.Extensions.Logging.Abstractions; using Opc.Ua; @@ -16,10 +17,18 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Browser; /// public sealed class OpcUaClientDriverBrowser : IDriverBrowser { + // Kept identical to OpcUaClientDriverFactoryExtensions.JsonOptions and + // OpcUaClientDriverProbe._opts so the browser parses a given DriverConfig JSON + // the same way as the factory and the probe. The JsonStringEnumConverter lets + // enum-valued knobs (SecurityPolicy / SecurityMode / AuthType / TargetNamespaceKind) + // be authored as their string names — the natural form for AdminUI-emitted JSON. + // JsonStringEnumConverter also accepts numeric ordinals, so existing numeric configs + // continue to work without migration. private static readonly JsonSerializerOptions JsonOpts = new() { - UnmappedMemberHandling = System.Text.Json.Serialization.JsonUnmappedMemberHandling.Skip, + UnmappedMemberHandling = JsonUnmappedMemberHandling.Skip, PropertyNameCaseInsensitive = true, + Converters = { new JsonStringEnumConverter() }, }; private readonly ILogger _logger; diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Browser.Tests/OpcUaClientBrowseSessionTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Browser.Tests/OpcUaClientBrowseSessionTests.cs index adadfb1e..7ea07a5e 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Browser.Tests/OpcUaClientBrowseSessionTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Browser.Tests/OpcUaClientBrowseSessionTests.cs @@ -19,10 +19,10 @@ public sealed class OpcUaClientBrowseSessionTests private static string Endpoint => Environment.GetEnvironmentVariable("OPCUA_SIM_ENDPOINT") ?? "opc.tcp://10.100.0.35:50000"; - // Enum values use their numeric ordinal because the browser uses default - // System.Text.Json with no JsonStringEnumConverter. SecurityPolicy.None, - // SecurityMode.None, OpcUaAuthType.Anonymous are all index 0, so omitting them - // also works — keeping them explicit for documentation value. + // SecurityPolicy.None, SecurityMode.None, OpcUaAuthType.Anonymous are all + // index 0 and can also be written as string names now that the browser carries + // JsonStringEnumConverter (Driver.OpcUaClient.Browser-003). Numeric ordinals are + // kept here as they were before the fix — both forms are accepted. private static string ConfigJson => $$""" { "EndpointUrl":"{{Endpoint}}", diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Browser.Tests/OpcUaClientDriverBrowserTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Browser.Tests/OpcUaClientDriverBrowserTests.cs index 969ce158..cc291ed5 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Browser.Tests/OpcUaClientDriverBrowserTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Browser.Tests/OpcUaClientDriverBrowserTests.cs @@ -54,6 +54,55 @@ public sealed class OpcUaClientDriverBrowserTests ex.Message.ShouldContain("Certificate"); } + // ---- Driver.OpcUaClient.Browser-003: JsonStringEnumConverter must be present ---- + + /// + /// AdminUI emits enum-valued driver-config fields (SecurityPolicy, SecurityMode, + /// AuthType, TargetNamespaceKind) as their string names (e.g. + /// "SecurityPolicy":"Basic256Sha256"). Without a + /// on the browser's + /// JsonSerializerOptions the deserialization throws a + /// before the browser's own validation can + /// run, giving the caller a confusing parse error instead of the expected + /// domain-specific message. + /// + /// This test passes a JSON blob with "AuthType":"Certificate" (string form) and + /// asserts that the browser's validation fires — i.e. deserialization succeeds and the + /// resulting mentions "Certificate". Before the + /// fix, a raw surfaces instead. + /// + [Fact] + public async Task OpenAsync_with_string_enum_AuthType_deserializes_correctly() + { + // "AuthType":"Certificate" — STRING form, as AdminUI emits. + var json = """{"EndpointUrl":"opc.tcp://127.0.0.1:1","AuthType":"Certificate"}"""; + // The browser should reach its own Certificate-auth guard and throw + // InvalidOperationException with a message that mentions "Certificate". + // Without JsonStringEnumConverter the deserialization itself throws JsonException, + // which means this Should.ThrowAsync assertion fails. + var ex = await Should.ThrowAsync( + () => _sut.OpenAsync(json, TestContext.Current.CancellationToken)); + // The browser's own guard must fire (not a JsonException from the deserializer). + ex.Message.ShouldContain("Certificate"); + } + + /// + /// Companion to : + /// confirms that + /// still accepts the integer/numeric ordinal form that the live-fixture tests currently + /// rely on, so no regression is introduced for existing numeric-authored configs. + /// + [Fact] + public async Task OpenAsync_with_numeric_enum_AuthType_still_works() + { + // "AuthType":2 — numeric ordinal form (OpcUaAuthType.Certificate == 2). + var json = """{"EndpointUrl":"opc.tcp://127.0.0.1:1","AuthType":2}"""; + var ex = await Should.ThrowAsync( + () => _sut.OpenAsync(json, TestContext.Current.CancellationToken)); + // JsonStringEnumConverter accepts both string and numeric ordinal forms. + ex.Message.ShouldContain("Certificate"); + } + // ---- Driver.OpcUaClient.Browser-001: AttributesAsync must refresh LastUsedUtc ---- ///