fix(mtconnect): share MTConnectError text extraction; pin hollow-error refusal

2026-08-05 06:23:08 -04:00
parent 9f17d7efa2
commit 3b8b5a77e1
3 changed files with 93 additions and 21 deletions
@@ -334,7 +334,10 @@ public sealed class MTConnectAgentClient : IMTConnectAgentClient, IDisposable
    ///     Read by <see cref="XName.LocalName"/> only, like every other document this driver parses:
    ///     the namespace URI carries the MTConnect version, so a qualified match would silently stop
    ///     recognising errors the day an Agent is upgraded — and an unrecognised error reads as a
-    ///     successful write.
+    ///     successful write. The error-text walk itself is
+    ///     <see cref="MTConnectXml.ExtractErrorText"/>, shared with the read paths'
+    ///     <see cref="MTConnectXml.DescribeUnexpectedRoot"/> so the two cannot drift on what counts
+    ///     as an Agent objection.
    /// </remarks>
    private static string? DescribeAgentError(string body)
    {
@@ -360,23 +363,11 @@ public sealed class MTConnectAgentClient : IMTConnectAgentClient, IDisposable
            return null;
        }

-        var errors = root.Descendants()
-            .Where(e => MTConnectXml.IsNamed(e, "Error"))
-            .Select(e =>
-            {
-                var code = MTConnectXml.OptionalAttribute(e, "errorCode");
-                var text = e.Value.Trim();
-
-                return code is null ? text : $"{code}: {text}";
-            })
-            .Where(text => text.Length > 0)
-            .ToList();
-
-        // An error document with no readable detail is still a refusal; reporting it as null would
-        // turn it back into an acceptance.
-        return errors.Count == 0
-            ? "The Agent answered with an MTConnectError document carrying no error detail."
-            : string.Join("; ", errors);
+        // An error document with no readable detail is still a refusal, so the shared helper's null
+        // — which means "nothing readable in here", NOT "this was not an error" — must become text
+        // rather than being passed on. Returning it as null would turn the refusal into acceptance.
+        return MTConnectXml.ExtractErrorText(root)
+               ?? "The Agent answered with an MTConnectError document carrying no error detail.";
    }

    /// <inheritdoc/>
@@ -127,6 +127,40 @@ internal static class MTConnectXml
            return message;
        }

+        var errors = ExtractErrorText(root);
+
+        return errors is null ? message : $"{message} The Agent reported: {errors}";
+    }
+
+    /// <summary>
+    ///     The Agent's own words from an <c>MTConnectError</c> document: every <c>&lt;Error&gt;</c>
+    ///     beneath <paramref name="root"/>, rendered <c>errorCode: text</c> where a code is present
+    ///     and joined with <c>"; "</c>. <c>null</c> when the document carries no readable error
+    ///     detail at all (no <c>&lt;Error&gt;</c> elements, or only blank ones).
+    /// </summary>
+    /// <remarks>
+    ///     <para>
+    ///         <b>Shared because both callers must read the same document the same way.</b> An
+    ///         Agent's objection reaches this driver through two unrelated doors — a read path that
+    ///         got an error document where it expected a model
+    ///         (<see cref="DescribeUnexpectedRoot"/>), and the write path deciding whether a 200
+    ///         actually accepted the observation. Two copies of this walk would drift on the same
+    ///         two silent rules the type-level remarks describe, and on the write side a missed
+    ///         <c>&lt;Error&gt;</c> does not merely degrade a message: it reads as a successful
+    ///         write.
+    ///     </para>
+    ///     <para>
+    ///         <b><c>null</c> means "no readable detail", NOT "not an error document".</b> Only the
+    ///         caller knows which document it is holding — check the root name first. A caller that
+    ///         treats a <c>null</c> here as "no error occurred" turns a hollow
+    ///         <c>&lt;MTConnectError/&gt;</c> into a success.
+    ///     </para>
+    /// </remarks>
+    /// <param name="root">The <c>MTConnectError</c> document's root element.</param>
+    public static string? ExtractErrorText(XElement root)
+    {
+        ArgumentNullException.ThrowIfNull(root);
+
        var errors = root.Descendants()
            .Where(e => IsNamed(e, "Error"))
            .Select(e =>
@@ -139,8 +173,6 @@ internal static class MTConnectXml
            .Where(text => text.Length > 0)
            .ToList();

-        return errors.Count == 0
-            ? message
-            : $"{message} The Agent reported: {string.Join("; ", errors)}";
+        return errors.Count == 0 ? null : string.Join("; ", errors);
    }
 }
@@ -212,6 +212,55 @@ public sealed class MTConnectAgentClientWriteTests
        outcome.AgentError.ShouldContain("Cannot find device data item: nosuchitem");
    }

+    /// <summary>
+    ///     An <c>MTConnectError</c> document carrying no readable <c>&lt;Error&gt;</c> detail is
+    ///     still a refusal. The Agent said no; that it said so unhelpfully changes nothing. The trap
+    ///     this pins is that the natural implementation — "return the joined error text, or null if
+    ///     there is none" — hands back the same <c>null</c> that means "this body was not an error
+    ///     document at all", silently converting the refusal into an acceptance.
+    /// </summary>
+    [Fact]
+    public async Task An_error_document_with_no_readable_detail_is_still_a_refusal()
+    {
+        const string HollowError =
+            """
+            <?xml version="1.0" encoding="UTF-8"?>
+            <MTConnectError xmlns="urn:mtconnect.org:MTConnectError:1.3">
+              <Header creationTime="2026-08-05T12:00:00Z" sender="stub-agent" instanceId="1" version="1.3.0.12" bufferSize="1"/>
+              <Errors/>
+            </MTConnectError>
+            """;
+
+        using var client = new MTConnectAgentClient(
+            Options(), new StubHandler((_, _) => Task.FromResult(XmlOk(HollowError))));
+
+        var outcome = await client.WriteObservationAsync("StubDevice", "dev1_msg", "hi", CancellationToken.None);
+
+        outcome.Accepted.ShouldBeFalse();
+        outcome.AgentError.ShouldNotBeNullOrWhiteSpace();
+    }
+
+    /// <summary>The same, for an <c>&lt;Error&gt;</c> element that is present but blank.</summary>
+    [Fact]
+    public async Task An_error_document_whose_error_element_is_blank_is_still_a_refusal()
+    {
+        const string BlankError =
+            """
+            <?xml version="1.0" encoding="UTF-8"?>
+            <MTConnectError xmlns="urn:mtconnect.org:MTConnectError:1.3">
+              <Errors><Error>   </Error></Errors>
+            </MTConnectError>
+            """;
+
+        using var client = new MTConnectAgentClient(
+            Options(), new StubHandler((_, _) => Task.FromResult(XmlOk(BlankError))));
+
+        var outcome = await client.WriteObservationAsync("StubDevice", "dev1_msg", "hi", CancellationToken.None);
+
+        outcome.Accepted.ShouldBeFalse();
+        outcome.AgentError.ShouldNotBeNullOrWhiteSpace();
+    }
+
    [Fact]
    public async Task A_non_success_status_is_a_refusal_naming_the_status()
    {