diff --git a/docs/requirements/Component-InboundAPI.md b/docs/requirements/Component-InboundAPI.md index d79febb0..be1854a7 100644 --- a/docs/requirements/Component-InboundAPI.md +++ b/docs/requirements/Component-InboundAPI.md @@ -95,7 +95,7 @@ Each API method definition includes: ] } ``` -- **Failure (4xx/5xx)**: The response body is an error object: +- **Failure (4xx/5xx)**: The response body is an error object carrying a human-readable `error` and a stable, machine-readable `code`: ```json { "error": "Site unreachable", @@ -103,6 +103,21 @@ Each API method definition includes: } ``` - HTTP status codes distinguish success from failure — no envelope wrapper. +- **Error codes.** Every failure body carries a `code` an external integrator can branch on (the `error` message is for humans and may change). The two authorization negatives — unknown method and key-not-in-scope — deliberately share one code (`NOT_APPROVED`) and one body so a caller cannot enumerate which method names exist: + +| `code` | HTTP status | Meaning | +|--------|-------------|---------| +| `UNAUTHORIZED` | 401 | Missing/invalid API key (every auth-stage failure maps here — no stage leak). | +| `NOT_APPROVED` | 403 | Method not found **or** key not in scope — indistinguishable by design. | +| `UNSUPPORTED_MEDIA_TYPE` | 415 | Request carried a body with a non-JSON `Content-Type`. A body with **no** `Content-Type` is still parsed leniently as JSON. | +| `INVALID_JSON` | 400 | Request body was not valid JSON. | +| `VALIDATION_FAILED` | 400 | Parameter validation failed (missing required field, wrong type, undeclared field). | +| `BODY_TOO_LARGE` | 413 | Request body exceeded `MaxRequestBodyBytes`. | +| `STANDBY_NODE` | 503 | Request hit a standby central node; the inbound API serves only the active node. | +| `TIMEOUT` | 500 | The method script exceeded its execution timeout. | +| `SITE_UNREACHABLE` | 500 | A routed `Route.To(...).Call(...)` could not reach the target site. | +| `SCRIPT_COMPILE_FAILED` | 500 | The method's script failed to compile (returned consistently on every node until a compiling version is saved). | +| `SCRIPT_ERROR` | 500 | Catch-all for any other script-execution failure (details are logged centrally, never leaked to the caller). | ### Extended Type System - API method parameter and return type definitions support an **extended type system** beyond the four template attribute types (Boolean, Integer, Float, String): diff --git a/src/ZB.MOM.WW.ScadaBridge.InboundAPI/EndpointExtensions.cs b/src/ZB.MOM.WW.ScadaBridge.InboundAPI/EndpointExtensions.cs index df822a27..85e5237c 100644 --- a/src/ZB.MOM.WW.ScadaBridge.InboundAPI/EndpointExtensions.cs +++ b/src/ZB.MOM.WW.ScadaBridge.InboundAPI/EndpointExtensions.cs @@ -51,6 +51,21 @@ public static class EndpointExtensions /// private const string UnauthorizedMessage = "Invalid or missing API key"; + /// + /// C3: builds the canonical inbound-API error body — { "error": message, "code": + /// code } — at the given HTTP status. Every error return in + /// routes through this helper so the + /// machine-readable code field (the audience is external integrators) is + /// present and consistent across all failure branches. The error field is + /// unchanged, so callers that only read it are unaffected (additive field). + /// + /// The machine-readable error code (e.g. UNAUTHORIZED, SITE_UNREACHABLE). + /// The human-readable, caller-safe error message. + /// The HTTP status code for the response. + /// A JSON result carrying the error + code at the given status. + private static IResult Error(string code, string message, int status) => + Results.Json(new { error = message, code }, statusCode: status); + /// Registers the POST /api/{methodName} inbound API endpoint with the active-node gate and body-size filter applied. /// The route builder to add the endpoint to. /// The same instance for chaining. @@ -108,9 +123,7 @@ public static class EndpointExtensions "Inbound API auth failure for method {Method}: {Failure} (status 401)", methodName, verification.Failure); - return Results.Json( - new { error = UnauthorizedMessage }, - statusCode: StatusCodes.Status401Unauthorized); + return Error("UNAUTHORIZED", UnauthorizedMessage, StatusCodes.Status401Unauthorized); } var identity = verification.Identity!; @@ -150,9 +163,7 @@ public static class EndpointExtensions "Inbound API authz failure for method {Method}: not approved (status 403)", methodName); - return Results.Json( - new { error = NotApprovedMessage }, - statusCode: StatusCodes.Status403Forbidden); + return Error("NOT_APPROVED", NotApprovedMessage, StatusCodes.Status403Forbidden); } // Telemetry follow-on: count this inbound request against the resolved, @@ -168,6 +179,20 @@ public static class EndpointExtensions httpContext.Items[AuditWriteMiddleware.AuditActorItemKey] = identity.DisplayName; + // S9: a body sent with an explicit non-JSON Content-Type is a media-type + // mismatch, not malformed JSON — reject it with 415 rather than a misleading + // 400 "invalid JSON". A body with NO Content-Type header is still parsed + // leniently as JSON below (preserving existing callers). The sniff is + // ordinal-ignore-case so "application/JSON" etc. are treated as JSON. + if (httpContext.Request.ContentLength > 0 + && !string.IsNullOrEmpty(httpContext.Request.ContentType) + && httpContext.Request.ContentType.Contains("json", StringComparison.OrdinalIgnoreCase) == false) + { + return Error( + "UNSUPPORTED_MEDIA_TYPE", "Content-Type must be application/json", + StatusCodes.Status415UnsupportedMediaType); + } + // Deserialize and validate parameters JsonElement? body = null; try @@ -188,9 +213,7 @@ public static class EndpointExtensions } catch (JsonException) { - return Results.Json( - new { error = "Invalid JSON in request body" }, - statusCode: 400); + return Error("INVALID_JSON", "Invalid JSON in request body", StatusCodes.Status400BadRequest); } // Thread the JSON-Schema $ref resolution seam into parameter @@ -209,9 +232,9 @@ public static class EndpointExtensions var paramResult = ParameterValidator.Validate(body, method.ParameterDefinitions, resolveRef); if (!paramResult.IsValid) { - return Results.Json( - new { error = paramResult.ErrorMessage }, - statusCode: 400); + return Error( + "VALIDATION_FAILED", paramResult.ErrorMessage ?? "Parameter validation failed", + StatusCodes.Status400BadRequest); } // Execute the method's script @@ -250,9 +273,13 @@ public static class EndpointExtensions "Inbound API script failure for method {Method}: {Error}", methodName, scriptResult.ErrorMessage); - return Results.Json( - new { error = scriptResult.ErrorMessage ?? "Internal server error" }, - statusCode: 500); + // C3: propagate the executor's machine-readable code (TIMEOUT, + // SITE_UNREACHABLE, SCRIPT_COMPILE_FAILED, …); an unclassified failure + // falls back to the SCRIPT_ERROR catch-all. + return Error( + scriptResult.ErrorCode ?? "SCRIPT_ERROR", + scriptResult.ErrorMessage ?? "Internal server error", + StatusCodes.Status500InternalServerError); } // Return the script result as JSON diff --git a/src/ZB.MOM.WW.ScadaBridge.InboundAPI/InboundApiEndpointFilter.cs b/src/ZB.MOM.WW.ScadaBridge.InboundAPI/InboundApiEndpointFilter.cs index a99af5c3..4cfc3edf 100644 --- a/src/ZB.MOM.WW.ScadaBridge.InboundAPI/InboundApiEndpointFilter.cs +++ b/src/ZB.MOM.WW.ScadaBridge.InboundAPI/InboundApiEndpointFilter.cs @@ -56,7 +56,7 @@ public sealed class InboundApiEndpointFilter : IEndpointFilter _logger.LogWarning( "Inbound API request rejected — this node is a standby (not the active central node)"); return Results.Json( - new { error = "Inbound API is only available on the active central node" }, + new { error = "Inbound API is only available on the active central node", code = "STANDBY_NODE" }, statusCode: StatusCodes.Status503ServiceUnavailable); } @@ -70,7 +70,7 @@ public sealed class InboundApiEndpointFilter : IEndpointFilter "Inbound API request rejected — body length {Length} exceeds limit {Limit}", declaredLength, maxBytes); return Results.Json( - new { error = "Request body too large" }, + new { error = "Request body too large", code = "BODY_TOO_LARGE" }, statusCode: StatusCodes.Status413PayloadTooLarge); } diff --git a/src/ZB.MOM.WW.ScadaBridge.InboundAPI/InboundScriptExecutor.cs b/src/ZB.MOM.WW.ScadaBridge.InboundAPI/InboundScriptExecutor.cs index 86e9ab4b..e3fe8806 100644 --- a/src/ZB.MOM.WW.ScadaBridge.InboundAPI/InboundScriptExecutor.cs +++ b/src/ZB.MOM.WW.ScadaBridge.InboundAPI/InboundScriptExecutor.cs @@ -384,7 +384,7 @@ public class InboundScriptExecutor // Fast-fail only when the CURRENT script text is the one already known bad. if (_knownBadMethods.TryGetValue(method.Name, out var badScript) && string.Equals(badScript, method.Script, StringComparison.Ordinal)) - return new InboundScriptResult(false, null, "Script compilation failed for this method"); + return new InboundScriptResult(false, null, "Script compilation failed for this method", "SCRIPT_COMPILE_FAILED"); // Lazy compile (handles methods created after startup, and stale/known-bad // recompiles). Compile outside the cache so a failed compile is not stored, @@ -396,7 +396,7 @@ public class InboundScriptExecutor // Routed through TryRecordBadMethod so the // cache is bounded under a flood of unique method names. TryRecordBadMethod(method.Name, method.Script ?? string.Empty); - return new InboundScriptResult(false, null, "Script compilation failed for this method"); + return new InboundScriptResult(false, null, "Script compilation failed for this method", "SCRIPT_COMPILE_FAILED"); } _knownBadMethods.TryRemove(method.Name, out _); @@ -448,17 +448,25 @@ public class InboundScriptExecutor if (timeoutCts.IsCancellationRequested && !cancellationToken.IsCancellationRequested) { _logger.LogWarning("Script execution timed out for method {Method}", method.Name); - return new InboundScriptResult(false, null, "Script execution timed out"); + return new InboundScriptResult(false, null, "Script execution timed out", "TIMEOUT"); } _logger.LogDebug("Inbound API request for method {Method} cancelled by client", method.Name); return new InboundScriptResult(false, null, "Request cancelled by client"); } + catch (SiteUnreachableException) + { + // C3: a routed Route.To(...).Call(...) reached a site that could not be + // contacted. Surface the machine-readable SITE_UNREACHABLE code (spec + // Component-InboundAPI failure body) instead of the generic SCRIPT_ERROR. + _logger.LogWarning("Routed site unreachable for method {Method}", method.Name); + return new InboundScriptResult(false, null, "Site unreachable", "SITE_UNREACHABLE"); + } catch (Exception ex) { _logger.LogError(ex, "Script execution failed for method {Method}", method.Name); // Safe error message, no internal details - return new InboundScriptResult(false, null, "Internal script error"); + return new InboundScriptResult(false, null, "Internal script error", "SCRIPT_ERROR"); } finally { @@ -537,7 +545,17 @@ public class InboundScriptContext /// /// Result of executing an inbound API script. /// +/// Whether the script executed successfully. +/// The serialized script return value on success; null otherwise. +/// A sanitized, caller-safe error message on failure; null on success. +/// +/// C3: an additive, machine-readable error code the endpoint surfaces in the failure +/// body's code field (e.g. TIMEOUT, SITE_UNREACHABLE, +/// SCRIPT_COMPILE_FAILED, SCRIPT_ERROR). Null when the executor does not +/// classify the failure; the endpoint then falls back to SCRIPT_ERROR. +/// public record InboundScriptResult( bool Success, string? ResultJson, - string? ErrorMessage); + string? ErrorMessage, + string? ErrorCode = null); diff --git a/src/ZB.MOM.WW.ScadaBridge.InboundAPI/RouteHelper.cs b/src/ZB.MOM.WW.ScadaBridge.InboundAPI/RouteHelper.cs index d94d9c3e..dca01913 100644 --- a/src/ZB.MOM.WW.ScadaBridge.InboundAPI/RouteHelper.cs +++ b/src/ZB.MOM.WW.ScadaBridge.InboundAPI/RouteHelper.cs @@ -4,6 +4,22 @@ using ZB.MOM.WW.ScadaBridge.Commons.Types; namespace ZB.MOM.WW.ScadaBridge.InboundAPI; +/// +/// C3: a routed call failed because the target site could not be reached (no contact / +/// unreachable). Derives from so existing +/// callers that catch the base type are unaffected, while +/// catches this more specific type to surface the +/// spec's SITE_UNREACHABLE error code instead of a generic SCRIPT_ERROR. +/// A routing failure whose message does not indicate unreachability stays a +/// plain . +/// +public sealed class SiteUnreachableException : InvalidOperationException +{ + /// Initializes a new with the given message. + /// The routing-level failure message that indicated the site was unreachable. + public SiteUnreachableException(string message) : base(message) { } +} + /// /// Route.To() helper for cross-site calls from inbound API scripts. /// Resolves instance to site, routes via , blocks until @@ -170,7 +186,7 @@ public class RouteTarget if (!response.Success) { - throw new InvalidOperationException( + throw RoutingFailure( response.ErrorMessage ?? "Remote script call failed"); } @@ -218,7 +234,7 @@ public class RouteTarget if (!response.Success) { - throw new InvalidOperationException( + throw RoutingFailure( response.ErrorMessage ?? "Remote attribute read failed"); } @@ -282,7 +298,7 @@ public class RouteTarget if (!response.Success) { - throw new InvalidOperationException( + throw RoutingFailure( response.ErrorMessage ?? "Remote attribute wait failed"); } @@ -333,7 +349,7 @@ public class RouteTarget if (!response.Success) { - throw new InvalidOperationException( + throw RoutingFailure( response.ErrorMessage ?? "Remote attribute write failed"); } } @@ -350,10 +366,30 @@ public class RouteTarget var siteId = await _instanceLocator.GetSiteIdForInstanceAsync(_instanceCode, cancellationToken); if (siteId == null) { - throw new InvalidOperationException( + // Route the site-resolution failure through the same classifier so a + // message indicating the site is unreachable surfaces as SITE_UNREACHABLE; + // a plain not-found / no-assigned-site stays a generic InvalidOperationException. + throw RoutingFailure( $"Instance '{_instanceCode}' not found or has no assigned site"); } return siteId; } + + /// + /// C3: classifies a routing-level failure message. When the message indicates the + /// site could not be reached (unreachable / no contact) the failure is a + /// — so + /// can surface the spec's SITE_UNREACHABLE code — otherwise a plain + /// . + /// + private static InvalidOperationException RoutingFailure(string message) => + IsUnreachable(message) + ? new SiteUnreachableException(message) + : new InvalidOperationException(message); + + private static bool IsUnreachable(string message) => + message.Contains("unreachable", StringComparison.OrdinalIgnoreCase) + || message.Contains("no contact", StringComparison.OrdinalIgnoreCase) + || message.Contains("no-contact", StringComparison.OrdinalIgnoreCase); } diff --git a/tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/EndpointContentTypeTests.cs b/tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/EndpointContentTypeTests.cs index 5c4a3a29..2e5c1cc2 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/EndpointContentTypeTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/EndpointContentTypeTests.cs @@ -92,6 +92,59 @@ public sealed class EndpointContentTypeTests : IDisposable Assert.Contains("42", body); } + [Fact] + public async Task NonJsonContentType_WithBody_Returns415() + { + // Task 5 (S9): a body with a non-JSON Content-Type must be rejected with 415 + + // UNSUPPORTED_MEDIA_TYPE rather than a misleading 400 "invalid JSON". A body + // with NO Content-Type header is still parsed leniently as JSON (preserving + // existing callers). + const string methodName = "echo415"; + + var method = new ApiMethod(methodName, "return Parameters[\"value\"];") + { + Id = 1, + TimeoutSeconds = 10, + ParameterDefinitions = """[{"name":"value","type":"Integer","required":true}]""", + }; + + var repo = Substitute.For(); + repo.GetMethodByNameAsync(methodName, Arg.Any()) + .Returns(method); + + using var host = await BuildHostAsync(repo); + var token = await SeedKeyAsync(host, methodName); + var client = host.GetTestClient(); + + // (a) text/plain body → 415 UNSUPPORTED_MEDIA_TYPE + var nonJson = new HttpRequestMessage(HttpMethod.Post, "/api/" + methodName) + { + Content = new ByteArrayContent(Encoding.UTF8.GetBytes("hello")), + }; + nonJson.Content.Headers.ContentType = MediaTypeHeaderValue.Parse("text/plain"); + nonJson.Headers.Authorization = new AuthenticationHeaderValue("Bearer", token); + + var nonJsonResponse = await client.SendAsync(nonJson); + var nonJsonBody = await nonJsonResponse.Content.ReadAsStringAsync(); + + Assert.Equal(HttpStatusCode.UnsupportedMediaType, nonJsonResponse.StatusCode); + Assert.Contains("UNSUPPORTED_MEDIA_TYPE", nonJsonBody); + + // (b) body with NO Content-Type header → still parses as JSON (lenient), 200. + var noContentType = new HttpRequestMessage(HttpMethod.Post, "/api/" + methodName) + { + Content = new ByteArrayContent(Encoding.UTF8.GetBytes("{\"value\":42}")), + }; + noContentType.Content.Headers.ContentType = null; + noContentType.Headers.Authorization = new AuthenticationHeaderValue("Bearer", token); + + var lenientResponse = await client.SendAsync(noContentType); + var lenientBody = await lenientResponse.Content.ReadAsStringAsync(); + + Assert.Equal(HttpStatusCode.OK, lenientResponse.StatusCode); + Assert.Contains("42", lenientBody); + } + /// Seeds a key scoped for and returns its Bearer token. private static async Task SeedKeyAsync(IHost host, string methodName) { diff --git a/tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/EndpointExtensionsTests.cs b/tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/EndpointExtensionsTests.cs index a34fe3cd..13226054 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/EndpointExtensionsTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/EndpointExtensionsTests.cs @@ -26,6 +26,7 @@ using System.Diagnostics.Metrics; using System.Net; using System.Net.Http.Headers; using System.Text; +using System.Text.Json; namespace ZB.MOM.WW.ScadaBridge.InboundAPI.Tests; @@ -67,8 +68,11 @@ public sealed class EndpointExtensionsTests : IDisposable // two bodies are equal-to-each-other) catches a single-branch divergence where // BOTH branches change in lockstep but away from the agreed enumeration-safe // body — the equal-to-each-other check alone would still pass in that case. - // Pinned to EndpointExtensions.NotApprovedMessage's JSON shape. - private const string NotApprovedBodyJson = """{"error":"API key not approved for this method"}"""; + // Pinned to EndpointExtensions.NotApprovedMessage's JSON shape. Task 5 (C3): the + // error body now additionally carries the machine-readable "code" field; both 403 + // branches still emit this identical body, preserving enumeration-safety. + private const string NotApprovedBodyJson = + """{"error":"API key not approved for this method","code":"NOT_APPROVED"}"""; // Each test gets its own throwaway SQLite database so seeded keys never leak // between tests; the file is deleted on Dispose. @@ -341,6 +345,45 @@ public sealed class EndpointExtensionsTests : IDisposable Assert.Equal(NotApprovedBodyJson, notInScopeBody); } + [Fact] + public async Task ErrorBodies_CarryMachineReadableCode() + { + // Task 5 (C3): every error body carries a machine-readable "code" field for + // external integrators — UNAUTHORIZED (401), INVALID_JSON (400), + // VALIDATION_FAILED (400). + var method = SeedMethod(1, "needsParam", "return Parameters[\"value\"];", + """[{"name":"value","type":"Integer","required":true}]"""); + + using var host = await BuildHostAsync(method); + var token = await SeedKeyAsync(host, "key1", "caller", new[] { "needsParam" }); + var client = host.GetTestClient(); + + // (a) missing credential → UNAUTHORIZED + var unauth = await client.SendAsync(new HttpRequestMessage(HttpMethod.Post, "/api/needsParam") + { + Content = new StringContent("{}", Encoding.UTF8, "application/json"), + }); + Assert.Equal(HttpStatusCode.Unauthorized, unauth.StatusCode); + Assert.Equal("UNAUTHORIZED", await ReadCodeAsync(unauth)); + + // (b) in-scope but invalid JSON → INVALID_JSON + var badJson = await client.SendAsync(BuildPost("needsParam", "{ not json", token)); + Assert.Equal(HttpStatusCode.BadRequest, badJson.StatusCode); + Assert.Equal("INVALID_JSON", await ReadCodeAsync(badJson)); + + // (c) missing required parameter → VALIDATION_FAILED + var missing = await client.SendAsync(BuildPost("needsParam", "{}", token)); + Assert.Equal(HttpStatusCode.BadRequest, missing.StatusCode); + Assert.Equal("VALIDATION_FAILED", await ReadCodeAsync(missing)); + } + + private static async Task ReadCodeAsync(HttpResponseMessage response) + { + var body = await response.Content.ReadAsStringAsync(); + using var doc = JsonDocument.Parse(body); + return doc.RootElement.TryGetProperty("code", out var code) ? code.GetString() : null; + } + [Fact] public async Task InvalidJsonBody_Returns400() { diff --git a/tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/InboundScriptExecutorTests.cs b/tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/InboundScriptExecutorTests.cs index cb46895c..9a929c4a 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/InboundScriptExecutorTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/InboundScriptExecutorTests.cs @@ -97,6 +97,32 @@ public class InboundScriptExecutorTests Assert.Equal("Internal script error", result.ErrorMessage); } + [Fact] + public async Task RoutedSiteUnreachable_Returns500WithSiteUnreachableCode() + { + // C3: a routed call whose site is unreachable must surface the machine-readable + // SITE_UNREACHABLE code (spec Component-InboundAPI failure body) rather than + // collapsing into a generic SCRIPT_ERROR. + var locator = Substitute.For(); + locator.GetSiteIdForInstanceAsync("X", Arg.Any()).Returns("SiteA"); + var router = Substitute.For(); + router.RouteToCallAsync("SiteA", Arg.Any(), Arg.Any()) + .Returns(ci => new RouteToCallResponse( + ((RouteToCallRequest)ci[1]).CorrelationId, + Success: false, ReturnValue: null, ErrorMessage: "Site unreachable", + DateTimeOffset.UtcNow)); + var route = new RouteHelper(locator, router); + + var method = new ApiMethod("routed", "return await Route.To(\"X\").Call(\"s\");") + { Id = 7, TimeoutSeconds = 10 }; + + var result = await _executor.ExecuteAsync( + method, new Dictionary(), route, TimeSpan.FromSeconds(10)); + + Assert.False(result.Success); + Assert.Equal("SITE_UNREACHABLE", result.ErrorCode); + } + [Fact] public async Task HandlerTimesOut_ReturnsTimeoutError() {