diff --git a/docs/requirements/Component-InboundAPI.md b/docs/requirements/Component-InboundAPI.md index 08976a49..ae22165b 100644 --- a/docs/requirements/Component-InboundAPI.md +++ b/docs/requirements/Component-InboundAPI.md @@ -124,6 +124,8 @@ API method scripts are compiled at central startup — all method definitions ar - Updating a method via the CLI (`api-method update --id --code '...'`) or Management API triggers immediate recompilation (`CompileAndRegister`). The updated script takes effect on the next API call — no node restart is required. - Creating a new method after startup: if the method is created but not yet compiled, the first invocation triggers lazy (on-demand) compilation. +> **Compile failures on save are non-fatal but surfaced.** `CompileAndRegister` only replaces the cached delegate when the new script compiles; on failure it leaves the **previously registered handler (if any) in place** — a broken save never takes a working method offline. The change still persists, so the Create/Update Management result carries a top-level **`compileWarning`** (null when the script compiled) listing the Roslyn/trust-model errors. On **update**, the previous version keeps serving while the broken script is *not* live; on **create**, the method returns `"Script compilation failed"` until a compiling version is saved. This means a saved script that does not compile is **not silently a no-op** — the caller (CLI / Central UI) sees the warning, and the real diagnostics are also logged centrally. + ### Direct SQL Warning > **Do not edit API method scripts via direct SQL.** The in-memory compiled script will not be updated until the next node restart. Always use the CLI, Management API, or Central UI to modify API method scripts. diff --git a/src/ZB.MOM.WW.ScadaBridge.InboundAPI/InboundScriptExecutor.cs b/src/ZB.MOM.WW.ScadaBridge.InboundAPI/InboundScriptExecutor.cs index 9ef6078a..e0f60bfd 100644 --- a/src/ZB.MOM.WW.ScadaBridge.InboundAPI/InboundScriptExecutor.cs +++ b/src/ZB.MOM.WW.ScadaBridge.InboundAPI/InboundScriptExecutor.cs @@ -100,21 +100,38 @@ public class InboundScriptExecutor /// /// The API method to compile and register. /// True if successfully compiled and registered, false otherwise. - public bool CompileAndRegister(ApiMethod method) + public bool CompileAndRegister(ApiMethod method) => CompileAndRegister(method, out _); + + /// + /// Compiles and registers a single API method script, additionally reporting the + /// compilation diagnostics. Returns false together with the human-readable + /// error list if the script is empty, fails Roslyn compilation, or violates the + /// script trust model. On failure the previously registered handler (if any) is + /// left in place — a broken save never replaces a working delegate — so the + /// management Create/Update path can surface a non-fatal "saved but did not compile" + /// warning while the prior version keeps serving requests (see + /// HandleCreateApiMethod/HandleUpdateApiMethod). + /// + /// The API method to compile and register. + /// On failure, the compilation/trust-model error messages; empty on success. + /// True if successfully compiled and registered, false otherwise. + public bool CompileAndRegister(ApiMethod method, out IReadOnlyList errors) { - var handler = Compile(method); + var (handler, compileErrors) = Compile(method); if (handler == null) { // InboundAPI-009: record the failure so the lazy-compile path does not // keep recompiling a broken script on every request. InboundAPI-024: // routed through the capped TryRecordBadMethod helper so the cache // cannot grow without bound under a flood of unique method names. + errors = compileErrors; TryRecordBadMethod(method.Name); return false; } // The method definition was (re)compiled successfully — drop any stale // failure record so a fixed script is no longer treated as bad. + errors = Array.Empty(); _knownBadMethods.TryRemove(method.Name, out _); return Register(method.Name, handler); } @@ -130,12 +147,12 @@ public class InboundScriptExecutor /// null when the script is missing, fails to compile, or violates the /// script trust model (InboundAPI-005). Does not mutate the handler cache. /// - private Func>? Compile(ApiMethod method) + private (Func>? Handler, IReadOnlyList Errors) Compile(ApiMethod method) { if (string.IsNullOrWhiteSpace(method.Script)) { _logger.LogWarning("API method {Method} has no script code", method.Name); - return null; + return (null, new[] { "Method has no script code." }); } // InboundAPI-005: enforce the script trust model before compiling. Roslyn @@ -147,7 +164,7 @@ public class InboundScriptExecutor _logger.LogWarning( "API method {Method} script rejected — trust model violation(s): {Violations}", method.Name, string.Join("; ", violations)); - return null; + return (null, violations.Select(v => "Trust model violation: " + v).ToList()); } try @@ -182,20 +199,20 @@ public class InboundScriptExecutor _logger.LogWarning( "API method {Method} script compilation failed: {Errors}", method.Name, string.Join("; ", errors)); - return null; + return (null, errors); } _logger.LogInformation("API method {Method} script compiled", method.Name); - return async ctx => + return (async ctx => { var state = await compiled.RunAsync(ctx, ctx.CancellationToken); return state.ReturnValue; - }; + }, Array.Empty()); } catch (Exception ex) { _logger.LogError(ex, "Failed to compile API method {Method} script", method.Name); - return null; + return (null, new[] { "Compilation error: " + ex.Message }); } } @@ -301,7 +318,7 @@ public class InboundScriptExecutor // Lazy compile on first request (handles methods created after startup). // Compile outside the cache so a failed compile is not stored, then add // atomically so concurrent first-callers share a single handler instance. - var compiled = Compile(method); + var (compiled, _) = Compile(method); if (compiled == null) { // Cache the failure so the next request short-circuits above. diff --git a/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs b/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs index c30b6239..4b455b89 100644 --- a/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs +++ b/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs @@ -2596,9 +2596,12 @@ public class ManagementActor : ReceiveActor }; await repo.AddApiMethodAsync(method); await repo.SaveChangesAsync(); - sp.GetService()?.CompileAndRegister(method); + // The script is persisted regardless; surface a non-fatal warning when it does + // not compile so the caller is not misled into thinking a broken method is live + // (a never-compiled method returns "Script compilation failed" on first call). + var compileWarning = TryCompileAndDescribe(sp, method, isUpdate: false); await AuditAsync(sp, user, "Create", "ApiMethod", method.Id.ToString(), method.Name, method); - return method; + return ApiMethodResult(method, compileWarning); } private static async Task HandleUpdateApiMethod(IServiceProvider sp, UpdateApiMethodCommand cmd, string user) @@ -2612,9 +2615,12 @@ public class ManagementActor : ReceiveActor method.ReturnDefinition = cmd.ReturnDefinition; await repo.UpdateApiMethodAsync(method); await repo.SaveChangesAsync(); - sp.GetService()?.CompileAndRegister(method); + // On a compile failure the previously registered handler keeps serving — the + // saved-but-broken script does NOT go live. Surface that as a warning rather + // than a silent success (the change still persists either way). + var compileWarning = TryCompileAndDescribe(sp, method, isUpdate: true); await AuditAsync(sp, user, "Update", "ApiMethod", method.Id.ToString(), method.Name, method); - return method; + return ApiMethodResult(method, compileWarning); } private static async Task HandleDeleteApiMethod(IServiceProvider sp, DeleteApiMethodCommand cmd, string user) @@ -2629,6 +2635,46 @@ public class ManagementActor : ReceiveActor return true; } + /// + /// Recompiles and re-registers an inbound API method's script after a Create/Update, + /// returning a non-null human-readable warning when the script did NOT compile. The + /// save is always persisted; on compile failure the previously registered handler (if + /// any) keeps serving requests — the broken script never replaces a working one (see + /// InboundScriptExecutor.CompileAndRegister). Returns null when the + /// script compiled, or when no executor is registered (e.g. a non-inbound host or a + /// test double). + /// + private static string? TryCompileAndDescribe(IServiceProvider sp, ApiMethod method, bool isUpdate) + { + var executor = sp.GetService(); + if (executor is null) + return null; + if (executor.CompileAndRegister(method, out var errors)) + return null; + + var detail = string.Join("; ", errors); + return isUpdate + ? $"Script saved but failed to compile; the previously registered version (if any) remains active and the new script is NOT live. Compilation errors: {detail}" + : $"Script saved but failed to compile; the method will return a compilation error until a compiling version is saved. Compilation errors: {detail}"; + } + + /// + /// Projects an to the Create/Update response shape, adding a + /// top-level compileWarning (null when the script compiled). Kept as a flat + /// projection so the method fields stay top-level for the CLI/Central UI and the + /// persistence-ignorant POCO entity is not polluted with a transient management field. + /// + private static object ApiMethodResult(ApiMethod method, string? compileWarning) => new + { + method.Id, + method.Name, + method.Script, + method.ParameterDefinitions, + method.ReturnDefinition, + method.TimeoutSeconds, + compileWarning, + }; + // ======================================================================== // Additional Security handlers (API key update, scope rules) // ======================================================================== diff --git a/tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/InboundScriptExecutorTests.cs b/tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/InboundScriptExecutorTests.cs index dc7de810..527d31b8 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/InboundScriptExecutorTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/InboundScriptExecutorTests.cs @@ -243,6 +243,56 @@ public class InboundScriptExecutorTests Assert.True(_executor.CompileAndRegister(method)); } + // --- Compile-diagnostics overload: surface errors so a non-compiling save is not a silent no-op --- + + [Fact] + public void CompileAndRegister_NonCompilingScript_ReportsErrors() + { + // A genuine Roslyn error (undefined symbol) must be reported via the out-param, + // not just swallowed to a bare false. The Management Create/Update path turns + // these into the caller-visible compileWarning. + var method = new ApiMethod("broken", "return doesNotExist;") { Id = 1, TimeoutSeconds = 10 }; + + var ok = _executor.CompileAndRegister(method, out var errors); + + Assert.False(ok); + Assert.NotEmpty(errors); + } + + [Fact] + public void CompileAndRegister_ValidScript_ReportsNoErrors() + { + var method = new ApiMethod("fine", "return 1;") { Id = 1, TimeoutSeconds = 10 }; + + var ok = _executor.CompileAndRegister(method, out var errors); + + Assert.True(ok); + Assert.Empty(errors); + } + + [Fact] + public async Task CompileAndRegister_NonCompilingUpdate_KeepsPreviousHandler() + { + // Register a working handler, then "update" the same method name with a script + // that does not compile. The broken save must NOT replace the working delegate — + // the previously registered version keeps serving requests (this is exactly the + // behaviour that makes a broken save non-fatal while the warning is surfaced). + var good = new ApiMethod("evolving", "return 111;") { Id = 1, TimeoutSeconds = 10 }; + Assert.True(_executor.CompileAndRegister(good, out var goodErrors)); + Assert.Empty(goodErrors); + + var broken = new ApiMethod("evolving", "this is not valid C#") { Id = 1, TimeoutSeconds = 10 }; + Assert.False(_executor.CompileAndRegister(broken, out var brokenErrors)); + Assert.NotEmpty(brokenErrors); + + // The old (good) handler still serves — the broken script never went live. + var result = await _executor.ExecuteAsync( + broken, new Dictionary(), _route, TimeSpan.FromSeconds(10)); + + Assert.True(result.Success); + Assert.Contains("111", result.ResultJson); + } + // --- InboundAPI-002: lazy compile-and-fetch must be atomic, never KeyNotFoundException --- [Fact] diff --git a/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/ManagementActorTests.cs b/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/ManagementActorTests.cs index 26a93db9..256d8426 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/ManagementActorTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/ManagementActorTests.cs @@ -377,6 +377,37 @@ public class ManagementActorTests : TestKit, IDisposable Assert.Contains("Designer", response.Message); } + [Fact] + public void UpdateApiMethod_NonCompilingScript_SucceedsButReturnsCompileWarning() + { + // The save still persists, but a script that does not compile must surface a + // top-level compileWarning in the result rather than silently looking like a + // success — and the previously registered handler keeps serving (CompileAndRegister + // never replaces a working delegate with a broken one). + var existing = new Commons.Entities.InboundApi.ApiMethod("MyMethod", "return 1;") + { + Id = 7, + TimeoutSeconds = 10 + }; + var apiRepo = Substitute.For(); + apiRepo.GetApiMethodByIdAsync(7, Arg.Any()).Returns(existing); + apiRepo.SaveChangesAsync(Arg.Any()).Returns(1); + _services.AddScoped(_ => apiRepo); + _services.AddSingleton(sp => new ZB.MOM.WW.ScadaBridge.InboundAPI.InboundScriptExecutor( + NullLogger.Instance, sp)); + + var actor = CreateActor(); + // Undefined symbol -> genuine Roslyn compile error. + var envelope = Envelope( + new UpdateApiMethodCommand(7, "return doesNotCompile;", 10, null, null), "Designer"); + + actor.Tell(envelope); + + var response = ExpectMsg(TimeSpan.FromSeconds(10)); + Assert.Contains("compileWarning", response.JsonData); + Assert.Contains("failed to compile", response.JsonData); + } + [Fact] public void AddTemplateAttribute_WithDeploymentRole_ReturnsUnauthorized() {