dohertj2/ScadaBridge
1
0
Fork 0
Code Issues 1 Pull Requests 1 Actions Packages Projects Releases Wiki Activity

fix(inbound-api): surface compile warning on non-compiling method save

Browse Source 
CompileAndRegister only swaps the cached handler on a successful compile, so a
management (or DB) save of a non-compiling inbound script silently kept the
previously-registered handler serving — stale results, still HTTP 200 — while
UpdateApiMethod reported success. A broken save thus looked like a no-op, with
the real Roslyn error only in the central log.

Surface the diagnostics instead: a new CompileAndRegister(method, out errors)
overload reports the compile/trust-model errors (the bool overload is kept for
existing callers), and HandleCreate/UpdateApiMethod return them as a top-level
compileWarning on the management result (null when the script compiled). The
save still persists and the previously registered version keeps serving — the
warning just stops a non-compiling save from looking like a success.

- InboundScriptExecutor: Compile returns its diagnostics alongside the handler.
- ManagementActor: TryCompileAndDescribe + flat ApiMethodResult projection so
  compileWarning rides at the top level without polluting the ApiMethod POCO.
- Component-InboundAPI.md: document the non-fatal-but-surfaced contract.
- Tests: executor diagnostics + non-compiling-update-keeps-previous-handler,
  plus an end-to-end ManagementActor test asserting compileWarning is returned.
This commit is contained in:
Joseph Doherty
2026-06-30 10:44:18 -04:00
parent 73ce2e4d0f
commit 67005ca4c0
5 changed files with 160 additions and 14 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/ZB.MOM.WW.ScadaBridge.InboundAPI/InboundScriptExecutor.cs
+27 -10
View File
@@ -100,21 +100,38 @@ public class InboundScriptExecutor
/// </summary>
/// <param name="method">The API method to compile and register.</param>
/// <returns>True if successfully compiled and registered, false otherwise.</returns>
public bool CompileAndRegister(ApiMethod method)
public bool CompileAndRegister(ApiMethod method) => CompileAndRegister(method, out _);
/// <summary>
/// Compiles and registers a single API method script, additionally reporting the
/// compilation diagnostics. Returns <c>false</c> together with the human-readable
/// error list if the script is empty, fails Roslyn compilation, or violates the
/// script trust model. On failure the <b>previously registered handler (if any) is
/// left in place</b> — 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
/// <c>HandleCreateApiMethod</c>/<c>HandleUpdateApiMethod</c>).
/// </summary>
/// <param name="method">The API method to compile and register.</param>
/// <param name="errors">On failure, the compilation/trust-model error messages; empty on success.</param>
/// <returns>True if successfully compiled and registered, false otherwise.</returns>
public bool CompileAndRegister(ApiMethod method, out IReadOnlyList<string> 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<string>();
_knownBadMethods.TryRemove(method.Name, out _);
return Register(method.Name, handler);
}
@@ -130,12 +147,12 @@ public class InboundScriptExecutor
/// <c>null</c> when the script is missing, fails to compile, or violates the
/// script trust model (InboundAPI-005). Does not mutate the handler cache.
/// </summary>
private Func<InboundScriptContext, Task<object?>>? Compile(ApiMethod method)
private (Func<InboundScriptContext, Task<object?>>? Handler, IReadOnlyList<string> 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<string>());
}
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.
src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs
+50 -4
View File
@@ -2596,9 +2596,12 @@ public class ManagementActor : ReceiveActor
};
await repo.AddApiMethodAsync(method);
await repo.SaveChangesAsync();
sp.GetService<InboundAPI.InboundScriptExecutor>()?.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<object?> 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<InboundAPI.InboundScriptExecutor>()?.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<object?> HandleDeleteApiMethod(IServiceProvider sp, DeleteApiMethodCommand cmd, string user)
@@ -2629,6 +2635,46 @@ public class ManagementActor : ReceiveActor
return true;
}
/// <summary>
/// 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
/// <c>InboundScriptExecutor.CompileAndRegister</c>). Returns <c>null</c> when the
/// script compiled, or when no executor is registered (e.g. a non-inbound host or a
/// test double).
/// </summary>
private static string? TryCompileAndDescribe(IServiceProvider sp, ApiMethod method, bool isUpdate)
{
var executor = sp.GetService<InboundAPI.InboundScriptExecutor>();
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}";
}
/// <summary>
/// Projects an <see cref="ApiMethod"/> to the Create/Update response shape, adding a
/// top-level <c>compileWarning</c> (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.
/// </summary>
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)
// ========================================================================