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

fix(inbound-api): M2.6 review nits — legacy required default, recursion depth guard, return-validator comment (#13)

Browse Source 
- legacy flat-array "required":"false" (string) now treated as optional (matches migration)
- depth ceiling (32) on InboundApiSchema Parse/Validate recursion — guards against
  stack-overflow from a deeply-nested stored schema (Parse throws->400, Validate adds error)
- DocOptions.MaxDepth=128 so the application-level structural guard fires before the
  System.Text.Json reader ceiling (each schema level = ~3 JSON reader levels)
- comment the intentional ParameterValidator/ReturnValueValidator early-return asymmetry
- note intentional datetime->string legacy collapse in NormalizeType
- tests: legacy string-false optional, parse/validate depth ceiling, scalar return schema
This commit is contained in:
Joseph Doherty
2026-06-15 15:18:44 -04:00
parent 4b6187c853
commit 411d0c043b
5 changed files with 236 additions and 20 deletions
Download Patch File Download Diff File Expand all files Collapse all files
docs/plans/2026-06-15-stillpending-m2-implementation.md.tasks.json
+12 -7
View File
@@ -1,13 +1,13 @@
{
"planPath": "docs/plans/2026-06-15-stillpending-m2-implementation.md",
"tasks": [
{"id": 32, "ref": "M2.0", "subject": "M2.0 #32: EF model/snapshot drift (PendingModelChangesWarning)", "class": "high-risk", "status": "pending"},
{"id": 33, "ref": "M2.1", "subject": "M2.1 #22: native-alarm capability validation wired into deploy pipeline", "class": "standard", "status": "pending"},
{"id": 34, "ref": "M2.2", "subject": "M2.2 #10: connection-level diff surfaced in deployment diff", "class": "standard", "status": "pending"},
{"id": 35, "ref": "M2.3", "subject": "M2.3 #7: Database.CachedWrite transient/permanent SQL classification", "class": "high-risk", "status": "pending"},
{"id": 36, "ref": "M2.4", "subject": "M2.4 #8: alarm conditionFilter applied (OPC UA WhereClause + client routing)", "class": "high-risk", "status": "pending"},
{"id": 37, "ref": "M2.5", "subject": "M2.5 #9: per-script execution timeout (entity+migration+flatten+actor)", "class": "standard", "status": "pending", "blockedBy": [32]},
{"id": 38, "ref": "M2.6", "subject": "M2.6 #13: nested Object/List extended-type validation", "class": "standard", "status": "pending"},
{"id": 32, "ref": "M2.0", "subject": "M2.0 #32: EF model/snapshot drift (PendingModelChangesWarning)", "class": "high-risk", "status": "completed", "commits": ["2fb608f"]},
{"id": 33, "ref": "M2.1", "subject": "M2.1 #22: native-alarm capability validation wired into deploy pipeline", "class": "standard", "status": "completed", "commits": ["d690920", "41d828e"]},
{"id": 34, "ref": "M2.2", "subject": "M2.2 #10: connection-level diff surfaced in deployment diff", "class": "standard", "status": "completed", "commits": ["e9a84ba", "198770f"]},
{"id": 35, "ref": "M2.3", "subject": "M2.3 #7: Database.CachedWrite transient/permanent SQL classification", "class": "high-risk", "status": "completed", "commits": ["d052706", "de375ff"]},
{"id": 36, "ref": "M2.4", "subject": "M2.4 #8: alarm conditionFilter applied (OPC UA WhereClause + client routing)", "class": "high-risk", "status": "completed", "commits": ["8825df5", "00304a2"]},
{"id": 37, "ref": "M2.5", "subject": "M2.5 #9: per-script execution timeout (entity+migration+flatten+actor)", "class": "standard", "status": "completed", "blockedBy": [32], "commits": ["3edef09", "3032faa"]},
{"id": 38, "ref": "M2.6", "subject": "M2.6 #13: nested Object/List extended-type validation", "class": "standard", "status": "in_progress"},
{"id": 39, "ref": "M2.7", "subject": "M2.7 #20+#21: return-type + argument-type compatibility checks", "class": "standard", "status": "pending"},
{"id": 40, "ref": "M2.8", "subject": "M2.8 #23: binding-completeness Error + name-exists-at-site", "class": "standard", "status": "pending"},
{"id": 41, "ref": "M2.9", "subject": "M2.9 #17: MachineDataDb fail-fast (reverts Host-008)", "class": "small", "status": "pending"},
@@ -26,5 +26,10 @@
{"ref": "#16", "subject": "Transport stale-instance enumeration", "to": "M8 (Transport)"},
{"ref": "#19", "subject": "script started/completed events", "status": "done in M1.8"}
],
"followups": [
{"id": 52, "subject": "Investigate 2 partition-purge E2E test failures (AuditLogPurgeActor/PartitionPurge)", "from": "M2.0", "status": "pending"},
{"id": 53, "subject": "Dedup alarm-capable protocol predicate (3 copies → AlarmCapableProtocols)", "from": "M2.1", "status": "pending"},
{"id": 54, "subject": "Expose ExecutionTimeoutSeconds (+ MinTimeBetweenRuns) in CLI + UI script authoring", "from": "M2.5", "status": "pending"}
],
"lastUpdated": "2026-06-15"
}
src/ZB.MOM.WW.ScadaBridge.Commons/Types/InboundApi/InboundApiSchema.cs
+49 -13
View File
@@ -39,7 +39,15 @@ public sealed class InboundApiSchema
/// <summary>For <see cref="Type"/> = <c>array</c>: the schema every element must satisfy; null means element type was not declared (shape-only).</summary>
public InboundApiSchema? Items { get; init; }
private static readonly JsonDocumentOptions DocOptions = default;
/// <summary>Maximum allowed schema nesting depth for both Parse and Validate recursion.</summary>
private const int MaxDepth = 32;
// Allow the JSON reader to parse schemas up to ~3× our structural ceiling so
// the application-level ParseSchema depth guard (MaxDepth = 32) fires before
// the System.Text.Json reader ceiling. Each structural level contributes
// roughly 3 JSON-reader nesting levels (object → properties-object → value),
// so 128 reader levels comfortably accommodates 32+ structural levels.
private static readonly JsonDocumentOptions DocOptions = new() { MaxDepth = 128 };
/// <summary>
/// Parses a stored definition string into an <see cref="InboundApiSchema"/>.
@@ -51,7 +59,7 @@ public sealed class InboundApiSchema
/// </summary>
/// <param name="json">The definition JSON; null/whitespace yields <c>null</c>.</param>
/// <returns>The parsed schema, or <c>null</c> when the input is empty.</returns>
/// <exception cref="JsonException">The input is non-empty but not valid JSON, or is a JSON scalar/null at the root.</exception>
/// <exception cref="JsonException">The input is non-empty but not valid JSON, is a JSON scalar/null at the root, or the schema nesting exceeds <see cref="MaxDepth"/>.</exception>
public static InboundApiSchema? Parse(string? json)
{
if (string.IsNullOrWhiteSpace(json))
@@ -62,14 +70,19 @@ public sealed class InboundApiSchema
using var doc = JsonDocument.Parse(json, DocOptions);
return doc.RootElement.ValueKind switch
{
JsonValueKind.Object => ParseSchema(doc.RootElement),
JsonValueKind.Object => ParseSchema(doc.RootElement, depth: 0),
JsonValueKind.Array => ParseLegacyArray(doc.RootElement),
_ => throw new JsonException("Type definition must be a JSON object (JSON Schema) or legacy parameter array."),
};
}
private static InboundApiSchema ParseSchema(JsonElement el)
private static InboundApiSchema ParseSchema(JsonElement el, int depth)
{
if (depth > MaxDepth)
{
throw new JsonException($"Schema nesting exceeds the maximum allowed depth of {MaxDepth}.");
}
var type = el.TryGetProperty("type", out var t) && t.ValueKind == JsonValueKind.String
? NormalizeType(t.GetString())
: "string";
@@ -79,7 +92,7 @@ public sealed class InboundApiSchema
InboundApiSchema? items = null;
if (el.TryGetProperty("items", out var itemsEl) && itemsEl.ValueKind == JsonValueKind.Object)
{
items = ParseSchema(itemsEl);
items = ParseSchema(itemsEl, depth + 1);
}
return new InboundApiSchema { Type = "array", Items = items };
@@ -109,7 +122,7 @@ public sealed class InboundApiSchema
foreach (var prop in props.EnumerateObject())
{
var schema = prop.Value.ValueKind == JsonValueKind.Object
? ParseSchema(prop.Value)
? ParseSchema(prop.Value, depth + 1)
: new InboundApiSchema { Type = "string" };
fields.Add(new InboundApiSchemaField(prop.Name, requiredSet.Contains(prop.Name), schema));
}
@@ -142,7 +155,18 @@ public sealed class InboundApiSchema
}
var rawType = TryGetMember(item, "type", out var t) ? t.GetString() : "string";
var required = !TryGetMember(item, "required", out var rq) || rq.ValueKind != JsonValueKind.False;
// A field is optional only when "required" is explicitly false.
// The SQL migration uses a string comparison (LOWER(...) <> 'false'),
// so we must also accept the string "false" (case-insensitive) here —
// not only the JSON boolean false — to stay consistent with legacy rows
// that stored "required":"false" as a string.
var required = !TryGetMember(item, "required", out var rq)
|| (rq.ValueKind != JsonValueKind.False
&& !string.Equals(
rq.ValueKind == JsonValueKind.String ? rq.GetString() : null,
"false",
StringComparison.OrdinalIgnoreCase));
var normalized = NormalizeType(rawType);
InboundApiSchema schema;
@@ -198,6 +222,9 @@ public sealed class InboundApiSchema
"boolean" or "bool" => "boolean",
"integer" or "int" or "int32" or "int64" => "integer",
"number" or "float" or "double" or "decimal" => "number",
// datetime→string is intentional: the legacy migration's SQL
// normalization function maps "datetime" to "string" (no separate
// datetime wire type in the extended type system), so C# must match.
"string" or "datetime" => "string",
"object" => "object",
"array" or "list" => "array",
@@ -215,9 +242,18 @@ public sealed class InboundApiSchema
/// <param name="path">The path prefix for the value being validated (empty for the root).</param>
/// <param name="errors">Accumulator the validator appends path-qualified messages to.</param>
public void Validate(JsonElement value, string path, List<string> errors)
=> ValidateCore(value, path, errors, depth: 0);
private void ValidateCore(JsonElement value, string path, List<string> errors, int depth)
{
ArgumentNullException.ThrowIfNull(errors);
if (depth > MaxDepth)
{
errors.Add($"{Describe(path)}: schema nesting too deep (max {MaxDepth})");
return;
}
// A null value satisfies any declared type — a present-but-null field is
// allowed; a MISSING required field is reported by the enclosing object.
if (value.ValueKind == JsonValueKind.Null)
@@ -260,11 +296,11 @@ public sealed class InboundApiSchema
break;
case "object":
ValidateObject(value, path, errors);
ValidateObject(value, path, errors, depth);
break;
case "array":
ValidateArray(value, path, errors);
ValidateArray(value, path, errors, depth);
break;
default:
@@ -273,7 +309,7 @@ public sealed class InboundApiSchema
}
}
private void ValidateObject(JsonElement value, string path, List<string> errors)
private void ValidateObject(JsonElement value, string path, List<string> errors, int depth)
{
if (value.ValueKind != JsonValueKind.Object)
{
@@ -303,7 +339,7 @@ public sealed class InboundApiSchema
var fieldPath = JoinField(path, field.Name);
if (value.TryGetProperty(field.Name, out var fieldValue))
{
field.Schema.Validate(fieldValue, fieldPath, errors);
field.Schema.ValidateCore(fieldValue, fieldPath, errors, depth + 1);
}
else if (field.Required)
{
@@ -312,7 +348,7 @@ public sealed class InboundApiSchema
}
}
private void ValidateArray(JsonElement value, string path, List<string> errors)
private void ValidateArray(JsonElement value, string path, List<string> errors, int depth)
{
if (value.ValueKind != JsonValueKind.Array)
{
@@ -329,7 +365,7 @@ public sealed class InboundApiSchema
var index = 0;
foreach (var element in value.EnumerateArray())
{
Items.Validate(element, $"{path}[{index}]", errors);
Items.ValidateCore(element, $"{path}[{index}]", errors, depth + 1);
index++;
}
}
src/ZB.MOM.WW.ScadaBridge.InboundAPI/ReturnValueValidator.cs
+12
View File
@@ -63,6 +63,18 @@ public static class ReturnValueValidator
return ReturnValidationResult.Valid();
}
// INTENTIONAL asymmetry with ParameterValidator:
//
// ParameterValidator has an early-return guard for "schema.Type != object"
// because method parameters are ALWAYS a top-level JSON object (flat map of
// name→value); a non-object parameter schema is treated as unconstrained.
//
// ReturnValueValidator does NOT guard on schema.Type here. A method may
// declare a scalar return type (e.g. {"type":"string"} or {"type":"integer"})
// and the script is expected to return exactly that scalar JSON value.
// Guarding on type == "object" would silently bypass validation for scalar
// and array return schemas — do NOT add that guard here.
if (string.IsNullOrWhiteSpace(resultJson))
{
return ReturnValidationResult.Invalid(
tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/ParameterValidatorTests.cs
+73
View File
@@ -1,4 +1,5 @@
using System.Text.Json;
using ZB.MOM.WW.ScadaBridge.Commons.Types.InboundApi;
namespace ZB.MOM.WW.ScadaBridge.InboundAPI.Tests;
@@ -364,4 +365,76 @@ public class ParameterValidatorTests
Assert.False(bad.IsValid);
Assert.Contains("'count'", bad.ErrorMessage);
}
// FIX 1: legacy "required":"false" string → field is optional ─────────────
[Theory]
[InlineData("""[{"name":"opt","type":"String","required":"false"}]""")]
[InlineData("""[{"name":"opt","type":"String","required":"False"}]""")]
[InlineData("""[{"name":"opt","type":"String","required":"FALSE"}]""")]
public void LegacyFlatArray_RequiredStringFalse_FieldIsOptional(string def)
{
// An absent field whose "required" is the string "false" (any case)
// must be treated as optional — consistent with the SQL migration's
// LOWER(...) <> 'false' comparison that produced these rows.
var result = ParameterValidator.Validate(null, def);
Assert.True(result.IsValid, $"Expected optional field to be valid when absent; error: {result.ErrorMessage}");
}
[Fact]
public void LegacyFlatArray_RequiredStringFalse_FieldPresentAndTypedCorrectly_Passes()
{
const string def = """[{"name":"opt","type":"String","required":"false"}]""";
var result = ParameterValidator.Validate(Body("{\"opt\":\"hello\"}"), def);
Assert.True(result.IsValid);
}
// FIX 2: recursion depth guard on Parse ───────────────────────────────────
/// <summary>
/// Builds a JSON Schema string with <paramref name="depth"/> levels of nested
/// object-in-properties nesting. Each level wraps the previous in an object
/// with a single property "a". The result exceeds the Parse ceiling when
/// depth &gt; 32.
/// </summary>
private static string BuildDeeplyNestedSchema(int depth)
{
// Inner-most: a scalar
var schema = "{\"type\":\"string\"}";
for (var i = 0; i < depth; i++)
{
schema = "{\"type\":\"object\",\"properties\":{\"a\":" + schema + "}}";
}
return schema;
}
[Fact]
public void SchemaAtDepthCeiling_ParsesSuccessfully()
{
// Exactly 32 levels of nesting should parse without throwing.
var def = BuildDeeplyNestedSchema(32);
var schema = InboundApiSchema.Parse(def);
Assert.NotNull(schema);
}
[Fact]
public void SchemaExceedingDepthCeiling_ThrowsJsonException_NotStackOverflow()
{
// 33 levels exceeds the ceiling → JsonException (clean 400 via the
// caller's try/catch), NOT a StackOverflowException.
var def = BuildDeeplyNestedSchema(33);
Assert.Throws<System.Text.Json.JsonException>(() => InboundApiSchema.Parse(def));
}
[Fact]
public void SchemaExceedingDepthCeiling_ParameterValidator_ReturnsInvalid()
{
// End-to-end: ParameterValidator wraps Parse in try/catch(JsonException)
// → the caller gets Invalid rather than an unhandled exception.
var def = BuildDeeplyNestedSchema(33);
var result = ParameterValidator.Validate(Body("{\"a\":\"x\"}"), def);
Assert.False(result.IsValid);
Assert.Contains("Invalid parameter definitions", result.ErrorMessage);
}
}
tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/ReturnValueValidatorTests.cs
+90
View File
@@ -1,3 +1,5 @@
using ZB.MOM.WW.ScadaBridge.Commons.Types.InboundApi;
namespace ZB.MOM.WW.ScadaBridge.InboundAPI.Tests;
/// <summary>
@@ -304,4 +306,92 @@ public class ReturnValueValidatorTests
Assert.False(bad.IsValid);
Assert.Contains("totalUnits", bad.ErrorMessage);
}
// FIX 3: scalar return schema validates scalar return values ──────────────
// (Guards the intentional ParameterValidator/ReturnValueValidator asymmetry:
// ReturnValueValidator must NOT short-circuit on non-object schema types.)
[Fact]
public void ScalarStringReturnSchema_ValidatesScalarStringReturn()
{
// A {"type":"string"} return schema must accept a bare JSON string.
var result = ReturnValueValidator.Validate("\"hello\"", """{"type":"string"}""");
Assert.True(result.IsValid);
}
[Fact]
public void ScalarIntegerReturnSchema_ValidatesScalarIntegerReturn()
{
var result = ReturnValueValidator.Validate("42", """{"type":"integer"}""");
Assert.True(result.IsValid);
}
[Fact]
public void ScalarStringReturnSchema_RejectsIntegerReturn()
{
var result = ReturnValueValidator.Validate("42", """{"type":"string"}""");
Assert.False(result.IsValid);
Assert.Contains("String", result.ErrorMessage);
}
[Fact]
public void ScalarBooleanReturnSchema_ValidatesBooleanReturn()
{
var result = ReturnValueValidator.Validate("true", """{"type":"boolean"}""");
Assert.True(result.IsValid);
}
// FIX 2: recursion depth guard on Validate ─────────────────────────────────
[Fact]
public void ValidateExceedingDepthCeiling_AddsDepthError_DoesNotThrow()
{
// Build a schema programmatically (bypassing Parse) with 34 levels of
// nesting to exceed the ceiling of 32. Validate must add an error and
// return, NOT stack overflow.
//
// Parse prevents creating a >32-level schema from stored JSON, but
// InboundApiSchema is a public type constructable in code, so Validate
// must guard independently.
var deepSchema = BuildProgrammaticSchema(34);
var json = BuildDeeplyNestedValue(34);
using var doc = System.Text.Json.JsonDocument.Parse(json);
var errors = new System.Collections.Generic.List<string>();
// Must not throw — adds a depth error to the list instead.
deepSchema.Validate(doc.RootElement, string.Empty, errors);
Assert.NotEmpty(errors);
Assert.Contains("nesting too deep", errors[0], StringComparison.OrdinalIgnoreCase);
}
/// <summary>
/// Constructs an <see cref="InboundApiSchema"/> with <paramref name="depth"/>
/// levels of object-nesting programmatically (bypassing <c>Parse</c>) to
/// exercise the Validate depth ceiling independently of the Parse ceiling.
/// </summary>
private static InboundApiSchema BuildProgrammaticSchema(int depth)
{
InboundApiSchema inner = new() { Type = "string" };
for (var i = 0; i < depth; i++)
{
inner = new InboundApiSchema
{
Type = "object",
Fields = [new InboundApiSchemaField("a", Required: false, inner)],
};
}
return inner;
}
private static string BuildDeeplyNestedValue(int depth)
{
var value = "\"leaf\"";
for (var i = 0; i < depth; i++)
{
value = "{\"a\":" + value + "}";
}
return value;
}
}