From e5009d45f20f732e29f93f65630dc74100006d01 Mon Sep 17 00:00:00 2001 From: Nate Bross Date: Mon, 27 Apr 2026 22:39:21 -0500 Subject: [PATCH] fix: route Apply param map through metadata so positional params skip the label --- src/SharpFM.Model/Scripting/FmScript.cs | 61 +++++++-- .../Scripting/FmScriptApplyTests.cs | 125 ++++++++++++++++++ 2 files changed, 177 insertions(+), 9 deletions(-) create mode 100644 tests/SharpFM.Tests/Scripting/FmScriptApplyTests.cs diff --git a/src/SharpFM.Model/Scripting/FmScript.cs b/src/SharpFM.Model/Scripting/FmScript.cs index a983a50..91bef54 100644 --- a/src/SharpFM.Model/Scripting/FmScript.cs +++ b/src/SharpFM.Model/Scripting/FmScript.cs @@ -181,13 +181,14 @@ public IReadOnlyList Apply(ScriptStepOperation op) => private List ApplyAdd(ScriptStepOperation op) { if (op.StepName is null) return ["StepName is required for add operations."]; - if (!Registry.StepRegistry.ByName.ContainsKey(op.StepName)) + if (!Registry.StepRegistry.ByName.TryGetValue(op.StepName, out var metadata)) return [$"Unknown step name '{op.StepName}'."]; // Route through each POCO's FromDisplay factory with the caller's - // param map synthesized into labeled HR tokens. POCOs that take - // unlabeled positional params parse them in order. - var hrParams = SynthesizeHrParams(op.Params); + // param map synthesized into HR tokens. The synthesizer consults the + // step's metadata so positional params (no HrLabel) pass as raw + // values and labeled params get the canonical "Label: value" form. + var hrParams = SynthesizeHrParams(op.Params, metadata); var step = StepDisplayFactory.TryCreate(op.StepName, op.Enabled ?? true, hrParams); if (step is null) return [$"No typed POCO factory registered for '{op.StepName}'."]; @@ -214,7 +215,7 @@ private List ApplyUpdate(ScriptStepOperation op) if (metadata is null) return [$"Apply/update is not supported for step kind '{step.GetType().Name}'."]; - var hrParams = SynthesizeHrParams(op.Params); + var hrParams = SynthesizeHrParams(op.Params, metadata); var updated = StepDisplayFactory.TryCreate(metadata.Name, step.Enabled, hrParams); if (updated is null) return [$"No typed POCO factory registered for '{metadata.Name}'."]; @@ -223,20 +224,62 @@ private List ApplyUpdate(ScriptStepOperation op) return []; } - private static string[] SynthesizeHrParams(IReadOnlyDictionary? map) + /// + /// Convert a caller's param map into the ordered HR-token form each step's + /// FromDisplay factory expects. Iterates the step's metadata in declared + /// order, formatting each match as "HrLabel: value" when the param + /// has a label and as a raw positional value when it doesn't (e.g. + /// SetVariableStep.Name, IfStep.Condition — these go straight + /// into <Name> / <Calculation> without prefix). + /// + /// + /// The previous implementation labeled every non-empty key, which caused + /// positional params to receive a "Name: $foo" string verbatim into + /// XML — structurally valid but semantically broken under FileMaker. + /// + private static string[] SynthesizeHrParams( + IReadOnlyDictionary? map, + Registry.StepMetadata metadata) { if (map is null || map.Count == 0) return []; + + var consumedKeys = new HashSet(StringComparer.OrdinalIgnoreCase); var result = new List(map.Count); + + foreach (var paramMeta in metadata.Params) + { + var matchedKey = FindMatchingKey(map, paramMeta.Name); + if (matchedKey is null) continue; + + consumedKeys.Add(matchedKey); + var value = map[matchedKey]; + result.Add(paramMeta.HrLabel is not null + ? $"{paramMeta.HrLabel}: {value}" + : value); + } + + // Forward-compat: any keys we don't recognise pass through with the + // old formatting so newly-introduced params keep working until their + // metadata catches up. foreach (var (k, v) in map) { - // Bare values (key is empty) are positional tokens; labeled - // entries become "Label: value" tokens which each POCO's - // FromDisplay parser recognizes. + if (consumedKeys.Contains(k)) continue; result.Add(string.IsNullOrEmpty(k) ? v : $"{k}: {v}"); } + return result.ToArray(); } + private static string? FindMatchingKey(IReadOnlyDictionary map, string paramName) + { + foreach (var (k, _) in map) + { + if (string.Equals(k, paramName, StringComparison.OrdinalIgnoreCase)) + return k; + } + return null; + } + private List ApplyRemove(ScriptStepOperation op) { if (ValidateStepIndex(op.Index) is { } err) return [err]; diff --git a/tests/SharpFM.Tests/Scripting/FmScriptApplyTests.cs b/tests/SharpFM.Tests/Scripting/FmScriptApplyTests.cs new file mode 100644 index 0000000..6c634a2 --- /dev/null +++ b/tests/SharpFM.Tests/Scripting/FmScriptApplyTests.cs @@ -0,0 +1,125 @@ +using System.Collections.Generic; +using System.Linq; +using SharpFM.Model.Scripting; +using SharpFM.Model.Scripting.Steps; +using Xunit; + +namespace SharpFM.Tests.Scripting; + +/// +/// Regression coverage for 's param-map handling. +/// MCP / external callers send structured param dictionaries +/// ({"Name": "$i"}), which the apply path must translate into the +/// HR-token form each step's FromDisplay factory expects. Positional params +/// (no HrLabel in metadata) must arrive unprefixed, otherwise the +/// label leaks into the XML and breaks under FileMaker at runtime. +/// +public class FmScriptApplyTests +{ + private static FmScript EmptyScript() => new(new List()); + + [Fact] + public void ApplyAdd_SetVariable_PositionalNameDoesNotReceiveLabelPrefix() + { + var script = EmptyScript(); + + var op = new ScriptStepOperation( + Action: "add", + StepName: "Set Variable", + Params: new Dictionary { ["Name"] = "$i", ["Value"] = "1" }); + + Assert.Empty(script.Apply(op)); + + var step = Assert.IsType(script.Steps.Single()); + Assert.Equal("$i", step.Name); + Assert.Equal("1", step.Value.Text); + } + + [Fact] + public void ApplyAdd_ExitLoopIf_PositionalConditionDoesNotReceiveLabelPrefix() + { + var script = EmptyScript(); + + var op = new ScriptStepOperation( + Action: "add", + StepName: "Exit Loop If", + Params: new Dictionary { ["condition"] = "$i > $limit" }); + + Assert.Empty(script.Apply(op)); + + var step = Assert.IsType(script.Steps.Single()); + Assert.Equal("$i > $limit", step.Condition.Text); + } + + [Fact] + public void ApplyAdd_If_PositionalConditionDoesNotReceiveLabelPrefix() + { + var script = EmptyScript(); + + var op = new ScriptStepOperation( + Action: "add", + StepName: "If", + Params: new Dictionary { ["condition"] = "$x = 1" }); + + Assert.Empty(script.Apply(op)); + + var step = Assert.IsType(script.Steps.Single()); + Assert.Equal("$x = 1", step.Condition.Text); + } + + [Fact] + public void ApplyAdd_SetVariable_OutOfOrderParams_ProduceCorrectXml() + { + // Agent provides params in arbitrary order; metadata-driven ordering + // ensures the HR token sequence matches what FromDisplay expects. + var script = EmptyScript(); + + var op = new ScriptStepOperation( + Action: "add", + StepName: "Set Variable", + Params: new Dictionary { ["Value"] = "100", ["Name"] = "$count" }); + + Assert.Empty(script.Apply(op)); + + var step = Assert.IsType(script.Steps.Single()); + Assert.Equal("$count", step.Name); + Assert.Equal("100", step.Value.Text); + } + + [Fact] + public void ApplyAdd_SetVariable_KeyMatchIsCaseInsensitive() + { + var script = EmptyScript(); + + var op = new ScriptStepOperation( + Action: "add", + StepName: "Set Variable", + Params: new Dictionary { ["name"] = "$lc", ["value"] = "5" }); + + Assert.Empty(script.Apply(op)); + + var step = Assert.IsType(script.Steps.Single()); + Assert.Equal("$lc", step.Name); + Assert.Equal("5", step.Value.Text); + } + + [Fact] + public void ApplyUpdate_SetVariable_PositionalNameDoesNotReceiveLabelPrefix() + { + var script = EmptyScript(); + script.Apply(new ScriptStepOperation( + Action: "add", + StepName: "Set Variable", + Params: new Dictionary { ["Name"] = "$old", ["Value"] = "1" })); + + var update = new ScriptStepOperation( + Action: "update", + Index: 0, + Params: new Dictionary { ["Name"] = "$new" }); + + Assert.Empty(script.Apply(update)); + + var step = Assert.IsType(script.Steps[0]); + Assert.Equal("$new", step.Name); + } +}