From 3776652208d54c749bc12bab64e9c0e75a43348b Mon Sep 17 00:00:00 2001 From: ognjenkatic Date: Wed, 26 Aug 2026 13:00:42 +0200 Subject: [PATCH] CxODEV-1884: carry a diagnostic message on the structured_error contract StructuredErrorException supported only code and reason, so callers that need both a short, stable reason and a detailed explanation had nowhere to put the detail and had to fold it into reason. That defeats matching on reason downstream, and leaves consumers with no separate diagnostic field. Add StructuredError.Message, populated from the exception. No Message property is added to the exception itself -- it already has one by virtue of being an Exception, and the new constructors set it via base(). The mapping only emits message when it differs from reason, so payloads from existing call sites are byte-identical and the field is omitted entirely (NullValueHandling.Ignore), keeping the shape at version 1. message trails referenceError in the new constructors rather than following reason. Overload resolution cannot pick between (code, reason, referenceError, message) and the existing (code, reason, referenceError, innerException) when the fourth argument is an untyped null -- and (code, reason, message, null), a message with no drill-down URI, is the common case. Placing the nullable parameter third leaves only (code, reason, referenceError, null) ambiguous, which the three-argument constructor already expresses. Both execution managers had a hand-copied exception-to-payload mapping and the test mirrored rather than called it, so a new field could pass tests while being silently dropped by the type-poll path. Extract the mapping into StructuredError.FromException and point all three at it. Co-Authored-By: Claude Fable 5 --- .../ConductorSharp.Client.csproj | 2 +- .../ConductorSharp.Engine.csproj | 2 +- .../Exceptions/StructuredErrorException.cs | 39 +++++- src/ConductorSharp.Engine/ExecutionManager.cs | 12 +- .../Model/StructuredError.cs | 35 +++++- .../TypePollSpreadingExecutionManager.cs | 12 +- ...ctorSharp.KafkaCancellationNotifier.csproj | 2 +- .../ConductorSharp.Patterns.csproj | 2 +- .../ConductorSharp.Toolkit.csproj | 2 +- .../Unit/StructuredErrorTests.cs | 117 +++++++++++++++--- 10 files changed, 177 insertions(+), 48 deletions(-) diff --git a/src/ConductorSharp.Client/ConductorSharp.Client.csproj b/src/ConductorSharp.Client/ConductorSharp.Client.csproj index 0bda519..5f4755c 100644 --- a/src/ConductorSharp.Client/ConductorSharp.Client.csproj +++ b/src/ConductorSharp.Client/ConductorSharp.Client.csproj @@ -6,7 +6,7 @@ Codaxy Codaxy ConductorSharp.Client - 4.1.0 + 4.2.0 Client library for Netflix Conductor, with some additional quality of life features. https://github.com/codaxy/conductor-sharp netflix;conductor diff --git a/src/ConductorSharp.Engine/ConductorSharp.Engine.csproj b/src/ConductorSharp.Engine/ConductorSharp.Engine.csproj index cae17be..86cc835 100644 --- a/src/ConductorSharp.Engine/ConductorSharp.Engine.csproj +++ b/src/ConductorSharp.Engine/ConductorSharp.Engine.csproj @@ -6,7 +6,7 @@ Codaxy Codaxy ConductorSharp.Engine - 4.1.0 + 4.2.0 Client library for Netflix Conductor, with some additional quality of life features. https://github.com/codaxy/conductor-sharp netflix;conductor diff --git a/src/ConductorSharp.Engine/Exceptions/StructuredErrorException.cs b/src/ConductorSharp.Engine/Exceptions/StructuredErrorException.cs index 86d1dd9..5a928f1 100644 --- a/src/ConductorSharp.Engine/Exceptions/StructuredErrorException.cs +++ b/src/ConductorSharp.Engine/Exceptions/StructuredErrorException.cs @@ -5,17 +5,22 @@ namespace ConductorSharp.Engine.Exceptions /// /// Thrown by a worker to attach a structured, sanitized error classification to the failed task's output. /// When caught by the execution manager, the // - /// are serialized under the structured_error output key (see + /// and the diagnostic message are serialized under the structured_error output key (see /// ), in addition to the plain /// error_message, so downstream consumers can read a stable classification without parsing free-text /// reasons. Plain exceptions are unaffected and keep producing only error_message. /// + /// + /// There is deliberately no Message property here: the diagnostic message is carried by the inherited + /// , which the message-taking constructors set. When no message is supplied it + /// falls back to , matching the behaviour of the original constructors. + /// public class StructuredErrorException : Exception { /// Stable, opaque classification code. Consumers map this to a failure response. public string Code { get; } - /// Human-readable, sanitized reason. Safe to surface across a layer boundary. + /// Short, stable, sanitized reason. Safe to surface across a layer boundary. public string Reason { get; } /// Optional URI pointing at the entity where the failure originated (drill-down link). @@ -36,5 +41,35 @@ public StructuredErrorException(string code, string reason, string referenceErro Reason = reason; ReferenceError = referenceError; } + + /// + /// Declares a diagnostic distinct from the short, stable + /// . Pass null for to fall back to the reason, + /// and null for when there is no entity to drill down into. + /// + /// + /// trails rather than following + /// on purpose. Overload resolution cannot choose between + /// this constructor and the one when the fourth argument is an untyped null, + /// so the nullable parameter is placed third, where it is typed the same either way. The only call this + /// leaves ambiguous is (code, reason, referenceError, null) — a declared-but-null inner exception, + /// which the three-argument constructor already expresses. Disambiguate with a named argument if needed. + /// + public StructuredErrorException(string code, string reason, string referenceError, string message) + : base(message ?? reason) + { + Code = code; + Reason = reason; + ReferenceError = referenceError; + } + + /// + public StructuredErrorException(string code, string reason, string referenceError, string message, Exception innerException) + : base(message ?? reason, innerException) + { + Code = code; + Reason = reason; + ReferenceError = referenceError; + } } } diff --git a/src/ConductorSharp.Engine/ExecutionManager.cs b/src/ConductorSharp.Engine/ExecutionManager.cs index 2ef451c..7eb1896 100644 --- a/src/ConductorSharp.Engine/ExecutionManager.cs +++ b/src/ConductorSharp.Engine/ExecutionManager.cs @@ -247,19 +247,9 @@ await _taskManager.UpdateAsync( pollResponse.WorkflowInstanceId ); - var errorMessage = new ErrorOutput { ErrorMessage = exception.Message }; - // A worker may throw a StructuredErrorException to attach a sanitized, stable classification to the // failed task's output. Plain exceptions keep producing only error_message, preserving backward compatibility. - if (exception is StructuredErrorException structuredException) - { - errorMessage.StructuredError = new StructuredError - { - Code = structuredException.Code, - Reason = structuredException.Reason, - ReferenceError = structuredException.ReferenceError - }; - } + var errorMessage = new ErrorOutput { ErrorMessage = exception.Message, StructuredError = StructuredError.FromException(exception) }; // TODO: We should verify that this is alright, it is possible that when executed concurrently, // the updates caused by LogAsync will be discarded because the call of UpdateAsync(TaskResult...) diff --git a/src/ConductorSharp.Engine/Model/StructuredError.cs b/src/ConductorSharp.Engine/Model/StructuredError.cs index 1457b0e..e61bbea 100644 --- a/src/ConductorSharp.Engine/Model/StructuredError.cs +++ b/src/ConductorSharp.Engine/Model/StructuredError.cs @@ -1,3 +1,6 @@ +using System; +using ConductorSharp.Engine.Exceptions; + namespace ConductorSharp.Engine.Model { /// @@ -12,13 +15,43 @@ public class StructuredError /// Stable, opaque classification code (e.g. an implementation-defined code, or UNCLASSIFIED). public string Code { get; set; } - /// Human-readable, sanitized reason. + /// Short, stable, sanitized reason. Consumers may key off this text, so keep it terse. public string Reason { get; set; } + /// + /// Optional diagnostic detail, longer and more specific than — the explanation an + /// operator needs, kept out of so that stays short and stable. Null when the producer + /// supplied nothing distinct from the reason, in which case it is omitted from serialized output + /// (NullValueHandling.Ignore) and the payload is unchanged from before this field existed. + /// + public string Message { get; set; } + /// Optional URI pointing at the entity where the failure originated (drill-down link). public string ReferenceError { get; set; } /// Payload shape version marker. Defaults to . public int Version { get; set; } = CurrentVersion; + + /// + /// Maps a thrown exception onto the payload, returning null for anything that is not a + /// so plain exceptions keep producing only error_message. + /// This is the single exception-to-payload mapping: both execution managers and the contract tests call it, + /// so the two poll strategies cannot drift apart as the shape evolves. + /// + public static StructuredError FromException(Exception exception) + { + if (exception is not StructuredErrorException structuredException) + return null; + + return new StructuredError + { + Code = structuredException.Code, + Reason = structuredException.Reason, + // Exception.Message falls back to Reason when the thrower supplied no distinct detail, so only + // carry it when it actually adds something. Existing call sites keep their exact payload. + Message = structuredException.Message == structuredException.Reason ? null : structuredException.Message, + ReferenceError = structuredException.ReferenceError + }; + } } } diff --git a/src/ConductorSharp.Engine/TypePollSpreadingExecutionManager.cs b/src/ConductorSharp.Engine/TypePollSpreadingExecutionManager.cs index 6fcab40..550bf1e 100644 --- a/src/ConductorSharp.Engine/TypePollSpreadingExecutionManager.cs +++ b/src/ConductorSharp.Engine/TypePollSpreadingExecutionManager.cs @@ -256,19 +256,9 @@ await _taskManager.UpdateAsync( pollResponse.WorkflowInstanceId ); - var errorMessage = new ErrorOutput { ErrorMessage = exception.Message }; - // A worker may throw a StructuredErrorException to attach a sanitized, stable classification to the // failed task's output. Plain exceptions keep producing only error_message, preserving backward compatibility. - if (exception is StructuredErrorException structuredException) - { - errorMessage.StructuredError = new StructuredError - { - Code = structuredException.Code, - Reason = structuredException.Reason, - ReferenceError = structuredException.ReferenceError - }; - } + var errorMessage = new ErrorOutput { ErrorMessage = exception.Message, StructuredError = StructuredError.FromException(exception) }; // TODO: We should verify that this is alright, it is possible that when executed concurrently, // the updates caused by LogAsync will be discarded because the call of UpdateAsync(TaskResult...) diff --git a/src/ConductorSharp.KafkaCancellationNotifier/ConductorSharp.KafkaCancellationNotifier.csproj b/src/ConductorSharp.KafkaCancellationNotifier/ConductorSharp.KafkaCancellationNotifier.csproj index ac791c8..fd0d8ab 100644 --- a/src/ConductorSharp.KafkaCancellationNotifier/ConductorSharp.KafkaCancellationNotifier.csproj +++ b/src/ConductorSharp.KafkaCancellationNotifier/ConductorSharp.KafkaCancellationNotifier.csproj @@ -4,7 +4,7 @@ net6.0 enable enable - 4.1.0 + 4.2.0 Codaxy Codaxy diff --git a/src/ConductorSharp.Patterns/ConductorSharp.Patterns.csproj b/src/ConductorSharp.Patterns/ConductorSharp.Patterns.csproj index bb0e862..c186b85 100644 --- a/src/ConductorSharp.Patterns/ConductorSharp.Patterns.csproj +++ b/src/ConductorSharp.Patterns/ConductorSharp.Patterns.csproj @@ -7,7 +7,7 @@ False Codaxy Codaxy - 4.1.0 + 4.2.0 diff --git a/src/ConductorSharp.Toolkit/ConductorSharp.Toolkit.csproj b/src/ConductorSharp.Toolkit/ConductorSharp.Toolkit.csproj index af3976e..f7e33f3 100644 --- a/src/ConductorSharp.Toolkit/ConductorSharp.Toolkit.csproj +++ b/src/ConductorSharp.Toolkit/ConductorSharp.Toolkit.csproj @@ -7,7 +7,7 @@ disable true dotnet-conductorsharp - 4.1.0 + 4.2.0 diff --git a/test/ConductorSharp.Engine.Tests/Unit/StructuredErrorTests.cs b/test/ConductorSharp.Engine.Tests/Unit/StructuredErrorTests.cs index 6fd7856..c4ab097 100644 --- a/test/ConductorSharp.Engine.Tests/Unit/StructuredErrorTests.cs +++ b/test/ConductorSharp.Engine.Tests/Unit/StructuredErrorTests.cs @@ -1,4 +1,5 @@ using System.Collections.Generic; +using System.Linq; using ConductorSharp.Client; using ConductorSharp.Client.Util; using ConductorSharp.Engine.Exceptions; @@ -12,40 +13,33 @@ namespace ConductorSharp.Engine.Tests.Unit { public class StructuredErrorTests { - // Mirrors the execution-manager catch block: builds the ErrorOutput (setting StructuredError for a - // StructuredErrorException) and serializes it. TryParse below asserts this output round-trips through the - // shared serializer, pinning the property-derived key/shape to StructuredErrorSerializer.OutputKey. + // Mirrors the execution-manager catch block. It calls StructuredError.FromException — the same mapping both + // ExecutionManager and TypePollSpreadingExecutionManager use — rather than reimplementing it, so a field + // added to the payload cannot pass here while being dropped by one of the managers. private static IDictionary SerializeCatchOutput(System.Exception exception) { - var output = new ErrorOutput { ErrorMessage = exception.Message }; - - if (exception is StructuredErrorException structuredException) - { - output.StructuredError = new StructuredError - { - Code = structuredException.Code, - Reason = structuredException.Reason, - ReferenceError = structuredException.ReferenceError - }; - } + var output = new ErrorOutput { ErrorMessage = exception.Message, StructuredError = StructuredError.FromException(exception) }; return SerializationHelper.ObjectToDictionary(output, ConductorConstants.IoJsonSerializerSettings); } + private static JToken StructuredErrorOf(IDictionary dict) => + JObject.Parse(JsonConvert.SerializeObject(dict))[StructuredErrorSerializer.OutputKey]; + [Fact] public void StructuredErrorException_produces_snake_case_structured_error() { - var exception = new StructuredErrorException("RESOURCE_UNAVAILABLE", "No port available", "https://rom/resourceOrder/42"); + var exception = new StructuredErrorException("RESOURCE_UNAVAILABLE", "No port available", "https://example.org/entity/42"); var dict = SerializeCatchOutput(exception); Assert.True(dict.ContainsKey("error_message")); Assert.True(dict.ContainsKey(StructuredErrorSerializer.OutputKey)); - var structured = JObject.Parse(JsonConvert.SerializeObject(dict))["structured_error"]; + var structured = StructuredErrorOf(dict); Assert.Equal("RESOURCE_UNAVAILABLE", (string)structured["code"]); Assert.Equal("No port available", (string)structured["reason"]); - Assert.Equal("https://rom/resourceOrder/42", (string)structured["reference_error"]); + Assert.Equal("https://example.org/entity/42", (string)structured["reference_error"]); Assert.Equal(StructuredError.CurrentVersion, (int)structured["version"]); } @@ -60,14 +54,88 @@ public void PlainException_output_is_backward_compatible() Assert.Single(dict); } + [Fact] + public void Message_is_carried_under_snake_case_message_key() + { + var exception = new StructuredErrorException( + "VALIDATION_FAILED", + "Input field not recognized", + "https://example.org/entity/7", + "Field 'widget_id' is not present in schema 'default'." + ); + + var structured = StructuredErrorOf(SerializeCatchOutput(exception)); + + Assert.Equal("VALIDATION_FAILED", (string)structured["code"]); + Assert.Equal("Input field not recognized", (string)structured["reason"]); + Assert.Equal("Field 'widget_id' is not present in schema 'default'.", (string)structured["message"]); + Assert.Equal("https://example.org/entity/7", (string)structured["reference_error"]); + } + + [Fact] + public void Message_reaches_error_message_and_reason_for_incompletion() + { + // error_message is set from Exception.Message, which the message-taking constructor overrides. The same + // value is what the execution manager sends as TaskResult.ReasonForIncompletion (the Conductor UI banner). + var exception = new StructuredErrorException("CODE", "Short reason", null, "Long diagnostic detail"); + + Assert.Equal("Long diagnostic detail", exception.Message); + Assert.Equal("Long diagnostic detail", (string)SerializeCatchOutput(exception)["error_message"]); + } + + [Fact] + public void Message_is_omitted_when_no_message_was_supplied() + { + // Guards the backward-compatibility promise: pre-existing call sites must keep their exact payload. + var structured = StructuredErrorOf( + SerializeCatchOutput(new StructuredErrorException("CODE", "Short reason", "https://example.org/entity/1")) + ); + + Assert.Null(structured["message"]); + Assert.Equal( + new[] { "code", "reason", "reference_error", "version" }, + ((JObject)structured).Properties().Select(p => p.Name).OrderBy(n => n) + ); + } + + [Fact] + public void Message_is_omitted_when_it_only_repeats_the_reason() + { + var exception = new StructuredErrorException("CODE", "Same text", null, "Same text"); + + Assert.Null(StructuredErrorOf(SerializeCatchOutput(exception))["message"]); + } + + [Fact] + public void Message_survives_the_round_trip() + { + var dict = SerializeCatchOutput( + new StructuredErrorException("CODE", "Short reason", "https://example.org/entity/9", "Long diagnostic detail") + ); + + Assert.True(StructuredErrorSerializer.TryParse(dict, out var parsed)); + Assert.Equal("CODE", parsed.Code); + Assert.Equal("Short reason", parsed.Reason); + Assert.Equal("Long diagnostic detail", parsed.Message); + Assert.Equal("https://example.org/entity/9", parsed.ReferenceError); + } + + [Fact] + public void FromException_returns_null_for_a_plain_exception() + { + Assert.Null(StructuredError.FromException(new System.InvalidOperationException("boom"))); + } + [Fact] public void RoundTrip_helper_output_is_parsed_back() { + // The signal-sender producer: no exception to catch, so the payload is rendered from the model directly. var error = new StructuredError { Code = "UNCLASSIFIED", Reason = "generic failure", - ReferenceError = "https://rom/resourceOrder/7" + Message = "downstream call failed: connection refused", + ReferenceError = "https://example.org/entity/7" }; var outputData = StructuredErrorSerializer.ToOutputData(error); @@ -75,6 +143,7 @@ public void RoundTrip_helper_output_is_parsed_back() Assert.True(StructuredErrorSerializer.TryParse(outputData, out var parsed)); Assert.Equal(error.Code, parsed.Code); Assert.Equal(error.Reason, parsed.Reason); + Assert.Equal(error.Message, parsed.Message); Assert.Equal(error.ReferenceError, parsed.ReferenceError); Assert.Equal(error.Version, parsed.Version); } @@ -123,5 +192,17 @@ public void TryParse_returns_false_when_code_missing() Assert.False(StructuredErrorSerializer.TryParse(dict, out _)); } + + [Fact] + public void TryParse_tolerates_a_message_only_payload_by_degrading() + { + // A message without a code is still unstructured: the caller must fall back to the generic path. + var dict = new Dictionary + { + [StructuredErrorSerializer.OutputKey] = new Dictionary { ["message"] = "detail but no code" } + }; + + Assert.False(StructuredErrorSerializer.TryParse(dict, out _)); + } } }