From 6bdada75493ba796b8383c2a327ad57aa437bb9d Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Sun, 24 May 2026 07:46:24 -0400 Subject: [PATCH] fix(transport): drop blocker false positives for stdlib + member access The DetectBlockersAsync heuristic was catching every PascalCase "Identifier(" or "Identifier." token in script bodies and treating it as a candidate SharedScript or ExternalSystem reference. On a normal template catalog this surfaced 30+ blocker rows for .NET stdlib (DateTimeOffset, Convert, ToString, Dispose, UtcNow...), ScadaLink runtime API roots (Notify, Database, ExternalSystem, Scripts...), and SQL keywords inside string literals (COUNT), blocking the import. Two surgical fixes: 1. Skip identifiers preceded by `.` so `obj.Method()` no longer flags `Method` as a top-level reference. 2. Maintain a `KnownNonReferenceNames` denylist for the small set of well-known stdlib / runtime / SQL tokens that can never be user-defined SharedScripts or ExternalSystems. The documented use case -- a top-level free-standing call to a missing SharedScript or ExternalSystem (e.g. `MissingHelper()` at the start of an expression, or `ErpSystem.Call(...)` where ErpSystem is the external-system identifier) -- still produces a blocker row, pinned by the existing test plus a new noise-filter regression test. --- .../Import/BundleImporter.cs | 47 +++++++++++++++--- .../Import/BundleImporterPreviewTests.cs | 49 +++++++++++++++++++ 2 files changed, 90 insertions(+), 6 deletions(-) diff --git a/src/ScadaLink.Transport/Import/BundleImporter.cs b/src/ScadaLink.Transport/Import/BundleImporter.cs index e5bb362..a388f64 100644 --- a/src/ScadaLink.Transport/Import/BundleImporter.cs +++ b/src/ScadaLink.Transport/Import/BundleImporter.cs @@ -384,13 +384,16 @@ public sealed class BundleImporter : IBundleImporter } // For each candidate, only report it as a blocker if it looks like a - // resource reference (PascalCase, length > 1) AND it's not present - // anywhere we can satisfy it. We deliberately do not look at language - // keywords or stdlib helpers — the test surface only ever uses - // well-named identifiers. + // resource reference (PascalCase, length > 1), isn't a well-known + // language / runtime / SQL token, AND isn't present anywhere we can + // satisfy it. The denylist is the noise filter that keeps the + // heuristic usable on real script bodies — without it, every member + // access (`obj.ToString()`) and stdlib type (`DateTimeOffset`) gets + // flagged. foreach (var candidate in referencedFromBundle.OrderBy(n => n, StringComparer.Ordinal)) { if (!LooksLikeResourceName(candidate)) continue; + if (KnownNonReferenceNames.Contains(candidate)) continue; var isShared = sharedScriptNames.Contains(candidate); var isExternal = externalSystemNames.Contains(candidate); if (isShared || isExternal) continue; @@ -413,11 +416,13 @@ public sealed class BundleImporter : IBundleImporter if (string.IsNullOrEmpty(body)) return; // Find every "Identifier(" or "Identifier." occurrence. The boundary // before the identifier must NOT be an identifier char so we don't - // match the trailing portion of a longer token. + // match the trailing portion of a longer token, AND must not be a + // dot — otherwise `obj.Method()` would flag `Method` as a top-level + // reference. Member-access trailing identifiers are skipped. for (var i = 0; i < body.Length; i++) { if (!IsIdentifierStart(body[i])) continue; - if (i > 0 && IsIdentifierChar(body[i - 1])) continue; + if (i > 0 && (IsIdentifierChar(body[i - 1]) || body[i - 1] == '.')) continue; var start = i; while (i < body.Length && IsIdentifierChar(body[i])) i++; if (i >= body.Length) break; @@ -429,6 +434,36 @@ public sealed class BundleImporter : IBundleImporter } } + /// + /// Names that look like PascalCase references but are never user-defined + /// SharedScripts or ExternalSystems. Filters the false-positive noise the + /// identifier scan produces against real script bodies: .NET stdlib types + /// and helpers, ScadaLink runtime API roots, and common SQL keywords that + /// appear inside string literals. Match is case-sensitive (Ordinal). + /// + private static readonly HashSet KnownNonReferenceNames = new(StringComparer.Ordinal) + { + // .NET / C# stdlib + "Boolean", "Byte", "Char", "Console", "Convert", "DateTime", + "DateTimeOffset", "Decimal", "Dispose", "Double", "Enumerable", + "Exception", "Guid", "Int16", "Int32", "Int64", "List", "Math", + "Now", "Object", "Single", "String", "Task", "TimeSpan", "ToBoolean", + "ToDateTime", "ToDecimal", "ToDouble", "ToInt16", "ToInt32", "ToInt64", + "ToList", "ToSingle", "ToString", "UtcNow", + + // ScadaLink script runtime API roots and well-known members + "Attribute", "Attributes", "Call", "CallScript", "CallShared", + "Connection", "CreateCommand", "Database", "ExecuteAsync", + "ExecuteNonQueryAsync", "ExecuteReaderAsync", "ExecuteScalarAsync", + "ExternalSystem", "GetAsync", "GetAttribute", "Instance", "Notify", + "Request", "Response", "Route", "Scheduler", "Scripts", "Send", + "SetAsync", "SetAttribute", + + // SQL keywords commonly seen inside string literals + "COUNT", "FROM", "GROUP", "INSERT", "JOIN", "ORDER", "SELECT", + "UPDATE", "WHERE", + }; + private static bool LooksLikeResourceName(string name) { if (name.Length < 2) return false; diff --git a/tests/ScadaLink.Transport.IntegrationTests/Import/BundleImporterPreviewTests.cs b/tests/ScadaLink.Transport.IntegrationTests/Import/BundleImporterPreviewTests.cs index 8727851..4cf2ce9 100644 --- a/tests/ScadaLink.Transport.IntegrationTests/Import/BundleImporterPreviewTests.cs +++ b/tests/ScadaLink.Transport.IntegrationTests/Import/BundleImporterPreviewTests.cs @@ -250,4 +250,53 @@ public sealed class BundleImporterPreviewTests : IDisposable Assert.DoesNotContain(preview.Items, i => i.Kind == ConflictKind.Blocker && i.Name == "HelperFn"); } + + [Fact] + public async Task PreviewAsync_does_not_flag_stdlib_or_runtime_member_accesses_as_blockers() + { + // Arrange: a template script that uses a representative mix of stdlib + // calls, runtime-API roots, and member-access patterns. None of these + // are user-defined SharedScripts or ExternalSystems and the previous + // heuristic was flagging every one of them. + await using (var scope = _provider.CreateAsyncScope()) + { + var ctx = scope.ServiceProvider.GetRequiredService(); + var t = new Template("Pump") { Description = "noise-check" }; + t.Scripts.Add(new TemplateScript("init", """ + var now = DateTimeOffset.UtcNow; + var s = Convert.ToString(123); + await Notify.Send("alerts", "msg"); + var x = await Database.ExecuteScalarAsync("SELECT COUNT(*) FROM t"); + var y = await ExternalSystem.Call("erp", "ping"); + obj.Dispose(); + """)); + ctx.Templates.Add(t); + await ctx.SaveChangesAsync(); + } + + var bundleStream = await ExportTemplatesAsync(); + var bytes = await StreamToBytes(bundleStream); + + // Act + ImportPreview preview; + await using (var scope = _provider.CreateAsyncScope()) + { + var importer = scope.ServiceProvider.GetRequiredService(); + var session = await importer.LoadAsync(new MemoryStream(bytes), passphrase: null); + preview = await importer.PreviewAsync(session.SessionId); + } + + // Assert: none of the well-known names produce blocker rows. + string[] noiseNames = + { + "DateTimeOffset", "UtcNow", "Convert", "ToString", "Notify", "Send", + "Database", "ExecuteScalarAsync", "COUNT", "ExternalSystem", "Call", + "Dispose", + }; + foreach (var name in noiseNames) + { + Assert.DoesNotContain(preview.Items, i => + i.Kind == ConflictKind.Blocker && i.Name == name); + } + } }