diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 8dfc5a1..5c3a52b 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -46,4 +46,4 @@ jobs: run: dotnet build PlanViewer.sln -c Release --no-restore - name: Run tests - run: dotnet test tests/PlanViewer.Core.Tests/PlanViewer.Core.Tests.csproj -c Release --no-build --verbosity normal + run: dotnet test tests/PlanViewer.Core.Tests/PlanViewer.Core.Tests.csproj -c Release --no-build --verbosity normal -- --hangdump --hangdump-timeout 5m --hangdump-type none diff --git a/.github/workflows/claude-code-review.yml b/.github/workflows/claude-code-review.yml index c3916c3..d4d8a8f 100644 --- a/.github/workflows/claude-code-review.yml +++ b/.github/workflows/claude-code-review.yml @@ -37,9 +37,17 @@ jobs: # were each already reviewed on their own PR, so re-reviewing the whole # release adds nothing — and the diff is large enough that it reliably # exhausts the turn budget and fails, putting a red X on the release. + # + # Also skip Dependabot PRs. They fail twice over instead of getting a + # review: the action rejects non-human actors outright ("Workflow initiated + # by non-human actor: dependabot"), and a dependabot-triggered run draws + # from the DEPENDABOT secret store, not the Actions one, so the OAuth token + # is empty anyway. A version bump has nothing an AI review would catch that + # build-and-test doesn't; skipped is neutral, not a red X on every bump. if: | github.event.pull_request.draft == false && github.event.pull_request.head.repo.full_name == github.repository && + github.actor != 'dependabot[bot]' && !(github.event.pull_request.head.ref == 'dev' && github.event.pull_request.base.ref == 'main') runs-on: ubuntu-latest diff --git a/.github/workflows/nightly.yml b/.github/workflows/nightly.yml index 3581793..f86ef58 100644 --- a/.github/workflows/nightly.yml +++ b/.github/workflows/nightly.yml @@ -65,7 +65,7 @@ jobs: dotnet restore tests/PlanViewer.Core.Tests/PlanViewer.Core.Tests.csproj - name: Run tests - run: dotnet test tests/PlanViewer.Core.Tests/PlanViewer.Core.Tests.csproj -c Release --verbosity normal + run: dotnet test tests/PlanViewer.Core.Tests/PlanViewer.Core.Tests.csproj -c Release --verbosity normal -- --hangdump --hangdump-timeout 5m --hangdump-type none - name: Publish App (all platforms) run: | diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 48d7596..92ff73f 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -52,7 +52,7 @@ jobs: run: | dotnet restore dotnet build -c Release - dotnet test tests/PlanViewer.Core.Tests/PlanViewer.Core.Tests.csproj -c Release --no-build --verbosity normal + dotnet test tests/PlanViewer.Core.Tests/PlanViewer.Core.Tests.csproj -c Release --no-build --verbosity normal -- --hangdump --hangdump-timeout 5m --hangdump-type none - name: Publish App (all platforms) run: | diff --git a/CITATION.cff b/CITATION.cff index 137f805..c3281a5 100644 --- a/CITATION.cff +++ b/CITATION.cff @@ -9,8 +9,8 @@ authors: website: "https://erikdarling.com" repository-code: "https://github.com/erikdarlingdata/PerformanceStudio" license: MIT -version: "1.19.1" -date-released: "2026-07-29" +version: "1.20.0" +date-released: "2026-08-21" keywords: - sql-server - execution-plan diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 83652dd..1e87677 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -62,6 +62,26 @@ PerformanceStudio/ - No unnecessary abstractions — keep it simple and direct - Tests use real `.sqlplan` XML fixtures, not mocks +## Calling native code + +**Never call a variadic C function through a plain `DllImport`.** `fcntl`, `open`, `ioctl` and the +`printf` family all take `...`, and on Apple arm64 variadic arguments are passed on the *stack* while +a fixed-signature P/Invoke passes them in *registers*. The callee reads a stack slot you never wrote. + +This does not throw, and it does not return an error. In #441 `fcntl(F_GETPATH)` returned 0 for +success and wrote up to 1KB through whatever pointer happened to be in that slot — an arbitrary write +into the process on every call — while the buffer we passed came back empty. It made `dotnet test` +unrunnable on macOS for months, sometimes as a GC livelock and sometimes as a deadlock, and CI never +saw any of it because Linux and Windows take different branches. + +Use a non-variadic equivalent instead (`proc_pidfdinfo` in place of `fcntl(F_GETPATH)`, for example). +`__arglist` is not an escape hatch — it throws `Vararg calling convention not supported` on this +target. + +When you do P/Invoke a struct-returning native call, derive the offsets in a comment from the system +header rather than leaving magic numbers, and check the returned size against what you expected. A +layout change should fail loudly, not hand back plausible-looking wrong bytes. + ## Adding Analysis Rules Rules live in `PlanAnalyzer.cs`. Each rule: diff --git a/global.json b/global.json new file mode 100644 index 0000000..3140116 --- /dev/null +++ b/global.json @@ -0,0 +1,5 @@ +{ + "test": { + "runner": "Microsoft.Testing.Platform" + } +} diff --git a/server/PlanShare/PlanShare.csproj b/server/PlanShare/PlanShare.csproj index 8d30dcd..77afe95 100644 --- a/server/PlanShare/PlanShare.csproj +++ b/server/PlanShare/PlanShare.csproj @@ -7,14 +7,14 @@ - + - + diff --git a/src/Directory.Build.props b/src/Directory.Build.props index 4637066..52a638c 100644 --- a/src/Directory.Build.props +++ b/src/Directory.Build.props @@ -15,7 +15,7 @@ Tests and server/ projects are outside src/ and are unaffected. --> - 1.19.1 + 1.20.0 Erik Darling Darling Data LLC Performance Studio diff --git a/src/PlanViewer.App/App.axaml.cs b/src/PlanViewer.App/App.axaml.cs index 0f328bf..8b23a24 100644 --- a/src/PlanViewer.App/App.axaml.cs +++ b/src/PlanViewer.App/App.axaml.cs @@ -6,9 +6,11 @@ using Avalonia.Markup.Xaml; using Avalonia.Platform; using Avalonia.Platform.Storage; +using Avalonia.Threading; using System.Linq; using System.Threading.Tasks; using PlanViewer.App.Services; +using PlanViewer.Core.Services; namespace PlanViewer.App; @@ -30,6 +32,13 @@ public override void OnFrameworkInitializationCompleted() desktop.MainWindow = new MainWindow(); } + // Entra MFA needs a parent window handle for the WAM broker, or the prompt never + // appears and the connection fails with 0xwindow_handle_required (issue #425). + // Registered once here because SqlAuthenticationProvider is process-wide, so this + // covers every connection Studio opens without touching each call site. No-op off + // Windows, where interactive auth uses the browser and needs no handle. + EntraInteractiveAuth.Register(ActiveWindowHandle); + if (RuntimeInformation.IsOSPlatform(OSPlatform.OSX)) { var iconPath = System.IO.Path.Combine(AppContext.BaseDirectory, "EDD.icns"); @@ -50,6 +59,50 @@ public override void OnFrameworkInitializationCompleted() base.OnFrameworkInitializationCompleted(); } + /// + /// The window that should own an Entra MFA prompt, resolved at the moment MSAL asks (issue #425). + /// + /// Prefers whichever window is currently active over the main window, because a connection is + /// usually triggered from the connection dialog — parenting the account picker to the main window behind + /// it would let the picker appear behind the dialog the user is looking at. Falls back to the main window, + /// then to , which MSAL treats the same as no handle: the prompt fails rather + /// than the app crashing, which is the right way round for an auth path. + /// + /// Resolved per call rather than captured once: a window's platform handle is not valid until the + /// window has been sourced, so a handle read at startup can be zero even though a real window exists a + /// moment later. + /// + /// Marshaled to the UI thread: MSAL invokes this from whatever thread SqlClient's token acquisition + /// happens to run on, and desktop.Windows is a UI-thread-owned collection that the UI thread can + /// mutate (a dialog opening or closing) mid-enumeration. Blocking on is + /// safe here because no connection path blocks the UI thread on auth — every open in the app is async. If + /// the dispatcher can't deliver anyway (shutdown timing), Zero degrades to MSAL's normal no-handle + /// failure instead of throwing from inside the auth callback. + /// + private static IntPtr ActiveWindowHandle() + { + try + { + return Dispatcher.UIThread.CheckAccess() + ? ActiveWindowHandleOnUIThread() + : Dispatcher.UIThread.Invoke(ActiveWindowHandleOnUIThread); + } + catch + { + return IntPtr.Zero; + } + } + + private static IntPtr ActiveWindowHandleOnUIThread() + { + if (Current?.ApplicationLifetime is not IClassicDesktopStyleApplicationLifetime desktop) + return IntPtr.Zero; + + var window = desktop.Windows.FirstOrDefault(w => w.IsActive) ?? desktop.MainWindow; + + return window?.TryGetPlatformHandle()?.Handle ?? IntPtr.Zero; + } + /// /// Handles macOS file-open activations (). The /// opened plan paths arrive here via the activation event rather than argv, so we diff --git a/src/PlanViewer.App/Controls/PlanViewerControl.Properties.cs b/src/PlanViewer.App/Controls/PlanViewerControl.Properties.cs index 4978979..dbd8369 100644 --- a/src/PlanViewer.App/Controls/PlanViewerControl.Properties.cs +++ b/src/PlanViewer.App/Controls/PlanViewerControl.Properties.cs @@ -818,9 +818,10 @@ private void ShowPropertiesPanel(PlanNode node) : w.Severity == PlanWarningSeverity.Warning ? "#FFB347" : "#6BB5FF"; var warnPanel = new StackPanel { Margin = new Thickness(10, 2, 10, 2) }; var legacyTag = w.IsLegacy ? " [legacy]" : ""; + var sourceTag = WarningSourceTag(w); var planWarnHeader = w.MaxBenefitPercent.HasValue - ? $"\u26A0 {w.WarningType}{legacyTag} \u2014 up to {FormatBenefitPercent(w.MaxBenefitPercent.Value)}% benefit" - : $"\u26A0 {w.WarningType}{legacyTag}"; + ? $"\u26A0 {w.WarningType}{sourceTag}{legacyTag} \u2014 up to {FormatBenefitPercent(w.MaxBenefitPercent.Value)}% benefit" + : $"\u26A0 {w.WarningType}{sourceTag}{legacyTag}"; warnPanel.Children.Add(new TextBlock { Text = planWarnHeader, @@ -901,9 +902,10 @@ private void ShowPropertiesPanel(PlanNode node) : w.Severity == PlanWarningSeverity.Warning ? "#FFB347" : "#6BB5FF"; var warnPanel = new StackPanel { Margin = new Thickness(10, 2, 10, 2) }; var nodeLegacyTag = w.IsLegacy ? " [legacy]" : ""; + var nodeSourceTag = WarningSourceTag(w); var nodeWarnHeader = w.MaxBenefitPercent.HasValue - ? $"\u26A0 {w.WarningType}{nodeLegacyTag} \u2014 up to {FormatBenefitPercent(w.MaxBenefitPercent.Value)}% benefit" - : $"\u26A0 {w.WarningType}{nodeLegacyTag}"; + ? $"\u26A0 {w.WarningType}{nodeSourceTag}{nodeLegacyTag} \u2014 up to {FormatBenefitPercent(w.MaxBenefitPercent.Value)}% benefit" + : $"\u26A0 {w.WarningType}{nodeSourceTag}{nodeLegacyTag}"; warnPanel.Children.Add(new TextBlock { Text = nodeWarnHeader, diff --git a/src/PlanViewer.App/Controls/PlanViewerControl.Rendering.cs b/src/PlanViewer.App/Controls/PlanViewerControl.Rendering.cs index 6955563..ae4ef22 100644 --- a/src/PlanViewer.App/Controls/PlanViewerControl.Rendering.cs +++ b/src/PlanViewer.App/Controls/PlanViewerControl.Rendering.cs @@ -516,6 +516,14 @@ private static string FormatBytes(double bytes) private static string FormatBenefitPercent(double pct) => pct >= 100 ? $"{pct:N0}" : $"{pct:N1}"; + /// + /// #436: marks the warnings SQL Server itself wrote into the plan, so they are not read as one of + /// our inferences. Only the engine's are tagged — they are the minority, and a badge on every + /// warning would carry no information. + /// + private static string WarningSourceTag(PlanWarning warning) => + warning.Source == PlanWarningSource.SqlServer ? " [SQL Server]" : ""; + private static bool HasSpillInPlanTree(PlanNode node) { foreach (var w in node.Warnings) diff --git a/src/PlanViewer.App/Controls/QuerySessionControl.Advice.cs b/src/PlanViewer.App/Controls/QuerySessionControl.Advice.cs index f5c96f1..85f2e7d 100644 --- a/src/PlanViewer.App/Controls/QuerySessionControl.Advice.cs +++ b/src/PlanViewer.App/Controls/QuerySessionControl.Advice.cs @@ -49,7 +49,21 @@ private void RobotAdvice_Click(object? sender, RoutedEventArgs e) var analysis = GetCurrentAnalysis(); if (analysis == null) { SetStatus("No plan to analyze", autoClear: false); return; } - var json = JsonSerializer.Serialize(analysis, new JsonSerializerOptions { WriteIndented = true }); + string json; + try + { + json = JsonSerializer.Serialize(analysis, AnalysisJson.Indented); + } + catch (Exception ex) when (ex is JsonException or NotSupportedException) + { + /* #430: Avalonia does not guard click handlers, so anything thrown on this path takes the + process down with no dialog and nothing logged. AnalysisJson's depth ceiling makes this + unreachable for any plan seen in the field — this catch is here so that "unreachable" is + not the only thing standing between a deep plan and a silent crash. */ + SetStatus($"Could not build robot advice for this plan: {ex.Message}", autoClear: false); + return; + } + ShowAdviceWindow("Advice for Robots", json); } diff --git a/src/PlanViewer.App/MainWindow.PlanViewer.cs b/src/PlanViewer.App/MainWindow.PlanViewer.cs index b1ab639..502543e 100644 --- a/src/PlanViewer.App/MainWindow.PlanViewer.cs +++ b/src/PlanViewer.App/MainWindow.PlanViewer.cs @@ -63,7 +63,20 @@ private DockPanel CreatePlanTabContent(PlanViewerControl viewer) { if (viewer.CurrentPlan == null) return; var analysis = ResultMapper.Map(viewer.CurrentPlan, "file", viewer.Metadata); - var json = JsonSerializer.Serialize(analysis, new JsonSerializerOptions { WriteIndented = true }); + string json; + try + { + json = JsonSerializer.Serialize(analysis, AnalysisJson.Indented); + } + catch (Exception ex) when (ex is JsonException or NotSupportedException) + { + /* #430: the same unguarded-click-handler crash as QuerySessionControl's Robot Advice + button — this entry point builds the payload independently, so it needed the same + depth ceiling and the same guard. */ + ShowError($"Could not build robot advice for this plan: {ex.Message}"); + return; + } + ShowAdviceWindow("Advice for Robots", json); }; diff --git a/src/PlanViewer.App/Mcp/McpHelpers.cs b/src/PlanViewer.App/Mcp/McpHelpers.cs index c7f9906..fa871c5 100644 --- a/src/PlanViewer.App/Mcp/McpHelpers.cs +++ b/src/PlanViewer.App/Mcp/McpHelpers.cs @@ -1,5 +1,6 @@ using System; using System.Text.Json; +using PlanViewer.Core.Output; namespace PlanViewer.App.Mcp; @@ -7,7 +8,14 @@ internal static class McpHelpers { public const int MaxTop = 100; - public static readonly JsonSerializerOptions JsonOptions = new() { WriteIndented = true }; + /* #430: MaxDepth, because several of these tools return an analysis carrying an OperatorTree and the + default ceiling of 64 is roughly 30 nested operators. Unlike the UI buttons an MCP tool failing here + does not crash the process, but it does return an error where the caller expected a plan. */ + public static readonly JsonSerializerOptions JsonOptions = new() + { + WriteIndented = true, + MaxDepth = AnalysisJson.MaxDepth, + }; public static string? Truncate(string? value, int maxLength) { diff --git a/src/PlanViewer.App/PlanViewer.App.csproj b/src/PlanViewer.App/PlanViewer.App.csproj index 2d7bbe7..652ea8b 100644 --- a/src/PlanViewer.App/PlanViewer.App.csproj +++ b/src/PlanViewer.App/PlanViewer.App.csproj @@ -9,22 +9,22 @@ - + - - - + + + - - + + - + diff --git a/tests/PlanViewer.Core.Tests/AnalysisJsonDepthTests.cs b/tests/PlanViewer.Core.Tests/AnalysisJsonDepthTests.cs new file mode 100644 index 0000000..b786d49 --- /dev/null +++ b/tests/PlanViewer.Core.Tests/AnalysisJsonDepthTests.cs @@ -0,0 +1,118 @@ +using System.Linq; +using System.Text.Json; +using PlanViewer.Core.Output; + +namespace PlanViewer.Core.Tests; + +/// +/// #430: "Robot Advice" crashed the app on a large plan. +/// +/// System.Text.Json defaults MaxDepth to 64 and throws past it. An operator tree nests once per +/// operator and each level costs two JSON levels (the object, then the Children array), so a plan +/// roughly 30 operators deep exhausts the default. The throw came out of an Avalonia click handler, +/// which Avalonia does not guard, so the process died with no dialog and nothing actionable logged. +/// +/// The reported stack named the cause precisely: the path was +/// $.Statements.OperatorTree.Children.Children...(30 deep)...NodeId — thirty CONSECUTIVE Children. +/// That matters, because the exception message offers two causes ("either be due to a cycle or if the +/// object depth is larger than the maximum allowed depth of 64") and they want opposite fixes. A +/// consecutive descent is depth. A cycle would have alternated, and raising the ceiling on a cycle +/// only moves the crash. +/// +public class AnalysisJsonDepthTests +{ + /// An analysis whose statement carries a single chain of nested operators. + private static AnalysisResult WithOperatorChain(int operators) + { + var node = new OperatorResult { PhysicalOp = "Leaf" }; + for (var i = 1; i < operators; i++) + node = new OperatorResult { PhysicalOp = "Nested", Children = { node } }; + + return new AnalysisResult { Statements = { new StatementResult { OperatorTree = node } } }; + } + + /// + /// The defect, reproduced against the options the call sites used to build inline. Without this the + /// fix below is unfalsifiable — a passing serialize proves nothing if nothing ever failed. + /// + [Fact] + public void TheOldInlineOptionsFailOnADeepPlan() + { + var exception = Assert.Throws(() => + JsonSerializer.Serialize(WithOperatorChain(60), new JsonSerializerOptions { WriteIndented = true })); + + Assert.Contains("depth", exception.Message, System.StringComparison.OrdinalIgnoreCase); + } + + /// The fix: the shared options carry the plan that used to crash the app. + [Theory] + [InlineData(60)] + [InlineData(200)] + [InlineData(400)] + public void TheSharedOptionsCarryADeepPlan(int operators) + { + var json = JsonSerializer.Serialize(WithOperatorChain(operators), AnalysisJson.Indented); + + Assert.Contains("\"Leaf\"", json, System.StringComparison.Ordinal); + /* Every level actually made it out, rather than the serializer stopping quietly partway. */ + Assert.Equal(operators - 1, System.Text.RegularExpressions.Regex.Matches(json, "\"Nested\"").Count); + } + + /// + /// The ceiling is headroom, not a guarantee — which is exactly why the two UI call sites also catch. + /// A plan past it must still fail as an exception the caller can turn into a message, not as + /// silently truncated JSON that reads like a complete plan. + /// + [Fact] + public void PastTheCeilingItStillThrowsRatherThanTruncating() + { + Assert.Throws(() => + JsonSerializer.Serialize(WithOperatorChain(AnalysisJson.MaxDepth + 10), AnalysisJson.Indented)); + } + + /// + /// The miss that #430's original fix left behind, and the reason the OPTIONS are shared and not + /// just the constant. + /// + /// Every CLI command that writes an analysis built its own JsonSerializerOptions. Making + /// MaxDepth a shared constant only fixed the sets that were edited to reference it — AnalyzeCommand's + /// two — while the identical pair in QueryStoreCommand kept the default ceiling of 64 and kept + /// failing on any plan deeper than ~30 operators. It failed quietly there: the per-plan catch turns + /// it into one "ERROR" row in summary.txt instead of an analysis, which is exactly the kind of + /// wrong-but-not-loud result nobody files a bug about. + /// + /// So this walks the options rather than trusting the call sites, and a new command that + /// rolls its own will fail here rather than in someone's Query Store sweep. + /// + [Fact] + public void EveryCliCommandWritesAnalysesWithTheCeiling() + { + var offenders = + (from type in typeof(PlanViewer.Cli.Commands.AnalyzeCommand).Assembly.GetTypes() + where type.Name.EndsWith("Command", System.StringComparison.Ordinal) + from field in type.GetFields(System.Reflection.BindingFlags.NonPublic + | System.Reflection.BindingFlags.Public + | System.Reflection.BindingFlags.Static) + where field.FieldType == typeof(JsonSerializerOptions) + let options = (JsonSerializerOptions?)field.GetValue(null) + where options is not null && options.MaxDepth != AnalysisJson.MaxDepth + select $"{type.Name}.{field.Name} (MaxDepth {options.MaxDepth})").ToList(); + + Assert.True(offenders.Count == 0, + "These write an analysis with the default depth ceiling and will fail on a deep plan: " + + string.Join(", ", offenders)); + } + + /// + /// Pins the headroom itself. 1024 is about 500 nested operators against the ~30 that used to fail; + /// a future edit dropping it back toward the default would re-open #430 for large plans only, which + /// is the shape of bug that reaches users rather than tests. + /// + [Fact] + public void TheCeilingIsFarAboveAnyRealPlan() + { + Assert.Equal(1024, AnalysisJson.MaxDepth); + Assert.Equal(AnalysisJson.MaxDepth, AnalysisJson.Indented.MaxDepth); + Assert.True(AnalysisJson.Indented.WriteIndented, "advice output is read by people as well as models"); + } +} diff --git a/tests/PlanViewer.Core.Tests/CliConnectionResolverTests.cs b/tests/PlanViewer.Core.Tests/CliConnectionResolverTests.cs new file mode 100644 index 0000000..9f37881 --- /dev/null +++ b/tests/PlanViewer.Core.Tests/CliConnectionResolverTests.cs @@ -0,0 +1,48 @@ +using PlanViewer.Cli.Commands; +using PlanViewer.Core.Interfaces; +using PlanViewer.Core.Services; + +namespace PlanViewer.Core.Tests; + +/// +/// Issue #425: the CLI accepts --auth entra but has no window to hand the WAM broker, so +/// must refuse up front with actionable guidance +/// instead of letting MSAL fail with 0xwindow_handle_required. This pins that refusal. +/// +/// Shares a collection with because both read or flip the +/// process-wide registration state behind ; running them in +/// parallel would let that state change between this test's skip check and its assertion. +/// +[Collection("EntraInteractiveAuth process-wide state")] +public class CliConnectionResolverTests +{ + [Fact] + public void BuildServerConnection_RefusesInteractiveEntraWhereItCannotWork() + { + /* Only reachable on Windows: off Windows IsSupported is permanently true (browser auth works + headless there) and the CLI correctly does not refuse. */ + Assert.SkipUnless(OperatingSystem.IsWindows(), "the refusal only exists where WAM does"); + + /* Registration state is process-wide and one-way, and the registration tests may have run earlier + in this process; reset the flag so this test observes the state the real CLI process is always + in — nothing ever registered. The shared collection keeps the registration tests from running + concurrently and re-flipping it mid-test. */ + EntraInteractiveAuth.ResetRegistrationForTests(); + + var ex = Assert.Throws(() => + CliConnectionResolver.BuildServerConnection("srv", "entra", trustCert: false, new NoCredentials())); + + Assert.Contains("headless", ex.Message, StringComparison.OrdinalIgnoreCase); + } + + /* Minimal stand-in: the resolver only asks whether a credential exists, and the entra refusal must fire + before credentials ever matter. */ + private sealed class NoCredentials : ICredentialService + { + public bool SaveCredential(string serverId, string username, string password) => false; + public (string Username, string Password)? GetCredential(string serverId) => null; + public bool DeleteCredential(string serverId) => false; + public bool CredentialExists(string serverId) => false; + public bool UpdateCredential(string serverId, string username, string password) => false; + } +} diff --git a/tests/PlanViewer.Core.Tests/EntraInteractiveAuthTests.cs b/tests/PlanViewer.Core.Tests/EntraInteractiveAuthTests.cs new file mode 100644 index 0000000..62ead1f --- /dev/null +++ b/tests/PlanViewer.Core.Tests/EntraInteractiveAuthTests.cs @@ -0,0 +1,61 @@ +using PlanViewer.Core.Services; + +namespace PlanViewer.Core.Tests; + +/// +/// Issue #425: interactive Entra auth needs a parent window handle on Windows, because SqlClient routes it +/// through the WAM broker. These pin the parts that are verifiable without a tenant — the OS gating and the +/// contract the windowless CLI depends on. Whether the picker actually authenticates can only be established +/// against a real Entra tenant, which is what the reporter offered to do. +/// +/// Shares a collection with : registration here flips the +/// process-wide state that the CLI refusal test keys off, so the two classes must not run in parallel. +/// +[Collection("EntraInteractiveAuth process-wide state")] +public class EntraInteractiveAuthTests +{ + [Fact] + public void Register_RejectsANullHandleProvider() + { + /* The whole point of the type is supplying a handle; accepting null would register a provider that + fails at prompt time instead of at wiring time, which is the harder bug to find. */ + Assert.Throws(() => EntraInteractiveAuth.Register(null!)); + } + + [Fact] + public void OffWindows_RegistrationIsSkippedButInteractiveAuthIsStillConsideredSupported() + { + /* macOS and Linux have no WAM broker: interactive auth goes through the system browser and needs no + handle, so registering a handle-supplying provider there would add a failure mode to the platforms + that currently work. Register must decline, and IsSupported must still be TRUE — otherwise the CLI + guard would refuse `--auth entra` on exactly the platforms where it is fine. */ + Assert.SkipWhen(OperatingSystem.IsWindows(), "Windows has WAM; this pins the non-Windows contract."); + + var called = false; + var registered = EntraInteractiveAuth.Register(() => { called = true; return IntPtr.Zero; }); + + Assert.False(registered, "off Windows there is nothing to register"); + Assert.False(called, "the handle provider must not even be consulted off Windows"); + Assert.True(EntraInteractiveAuth.IsSupported, + "browser-based interactive auth works off Windows, so the CLI must not refuse it there"); + } + + [Fact] + public void OnWindows_RegistersOnceAndIsIdempotent() + { + /* SqlAuthenticationProvider.SetProvider is process-wide, so a second registration would silently + replace the first — and with several entry points able to call this (the app today, the SSMS + extension later) "first one wins, later ones are no-ops" is the contract worth pinning. */ + Assert.SkipUnless(OperatingSystem.IsWindows(), "WAM registration only happens on Windows."); + + var first = EntraInteractiveAuth.Register(() => IntPtr.Zero); + var second = EntraInteractiveAuth.Register(() => new IntPtr(1234)); + + Assert.False(second, "a second Register must be a no-op rather than replacing the provider"); + Assert.True(EntraInteractiveAuth.IsSupported); + + /* first is only true when this test observed the very first registration in the process; another test + or the host may legitimately have gotten there first, so it is not asserted either way. */ + _ = first; + } +} diff --git a/tests/PlanViewer.Core.Tests/HistoricalCliContractTests.cs b/tests/PlanViewer.Core.Tests/HistoricalCliContractTests.cs index c2248a3..5be64e4 100644 --- a/tests/PlanViewer.Core.Tests/HistoricalCliContractTests.cs +++ b/tests/PlanViewer.Core.Tests/HistoricalCliContractTests.cs @@ -6,8 +6,13 @@ namespace PlanViewer.Core.Tests; public sealed class HistoricalCliContractTests { + /* Rolled for #436, which adds "source" to every warning in the JSON output so a consumer can tell + SQL Server's own warnings from Performance Studio's inferences. The change is additive — nothing + was removed or renamed, so a consumer reading fields by name is unaffected — but anything + diffing or hashing whole output sees different bytes, which is exactly what this constant is + here to make somebody decide on rather than discover. */ private const string ExpectedCompactOutputSha256 = - "0c609fed8e250d9366eb9a6cd5eaf40b661ee30d7ba2546bd7726960592e9d87"; + "06975e4e513eef23669c86bc2cfeec916f442461d605060bbd996bd44df13405"; [Fact] public async Task AnalyzeCompact_PreservesHistoricalOutputBytes() diff --git a/tests/PlanViewer.Core.Tests/OpenedFilePathResolverTests.cs b/tests/PlanViewer.Core.Tests/OpenedFilePathResolverTests.cs new file mode 100644 index 0000000..3a7579d --- /dev/null +++ b/tests/PlanViewer.Core.Tests/OpenedFilePathResolverTests.cs @@ -0,0 +1,71 @@ +using PlanViewer.Cli.ReplSurface; + +namespace PlanViewer.Core.Tests; + +/// +/// #441: the macOS resolver called fcntl(F_GETPATH) through a plain DllImport. fcntl(2) is variadic, +/// and on Apple arm64 variadic arguments are passed on the stack while a fixed-signature P/Invoke +/// puts them in registers — so the callee read a stack slot we never wrote. +/// +/// It did not fail. fcntl returned 0 for success and wrote up to MAXPATHLEN bytes through whatever +/// pointer that slot happened to hold, an arbitrary ~1KB write on every call, while the buffer we +/// passed came back empty. +/// +/// Nothing caught it, and that is the interesting part: the old test asserted on the returned +/// handle's Label and passed on Linux and Windows, which take entirely different branches. So this +/// asserts the one thing that was actually broken — that the resolver returns the real path of the +/// file that is genuinely open — and it asserts it on whatever platform the suite is running on. +/// +public class OpenedFilePathResolverTests +{ + [Fact] + public void GetFinalPath_ReturnsTheRealPathOfTheOpenHandle() + { + var path = Path.Combine(Path.GetTempPath(), $"resolver-{Guid.NewGuid():N}.sqlplan"); + File.WriteAllText(path, ""); + try + { + using var stream = new FileStream(path, FileMode.Open, FileAccess.Read); + + var resolved = OpenedFilePathResolver.GetFinalPath(stream); + + /* Compared by identity rather than by string, because macOS answers with the canonical + path — Path.GetTempPath() reports /var/folders/... while the kernel reports + /private/var/folders/..., and /var is a symlink to /private/var. A string comparison + here would fail for a reason that has nothing to do with the defect. */ + Assert.True(File.Exists(resolved), $"Resolver returned a path that does not exist: '{resolved}'"); + Assert.Equal( + new FileInfo(path).Length, + new FileInfo(resolved).Length); + Assert.Equal( + Path.GetFileName(path), + Path.GetFileName(resolved)); + } + finally + { + File.Delete(path); + } + } + + /// + /// The empty string is precisely what the broken call produced, and it is worth pinning that it + /// can never be mistaken for a valid answer: an empty path would sail through the extension and + /// containment checks in McpPlanPathPolicy as a Path.GetFullPath argument exception rather than + /// as a denial. + /// + [Fact] + public void GetFinalPath_NeverReturnsAnEmptyPath() + { + var path = Path.Combine(Path.GetTempPath(), $"resolver-{Guid.NewGuid():N}.sqlplan"); + File.WriteAllText(path, ""); + try + { + using var stream = new FileStream(path, FileMode.Open, FileAccess.Read); + Assert.False(string.IsNullOrWhiteSpace(OpenedFilePathResolver.GetFinalPath(stream))); + } + finally + { + File.Delete(path); + } + } +} diff --git a/tests/PlanViewer.Core.Tests/PlanAnalyzerTests.cs b/tests/PlanViewer.Core.Tests/PlanAnalyzerTests.cs index ff60299..393842c 100644 --- a/tests/PlanViewer.Core.Tests/PlanAnalyzerTests.cs +++ b/tests/PlanViewer.Core.Tests/PlanAnalyzerTests.cs @@ -264,6 +264,47 @@ public void Rule12a_NonSargable_ConvertImplicit() Assert.Contains(warnings, w => w.Message.Contains("CONVERT_IMPLICIT")); } + // --------------------------------------------------------------- + // Rule 12: Non-SARGable Predicate — CONVERT_IMPLICIT on the parameter + // --------------------------------------------------------------- + + /// + /// #436: a conversion is only non-SARGable when it converts the COLUMN. Data type precedence + /// decides which side SQL Server converts, and it converts the lower-precedence one — so a + /// numeric(18,0) column compared to an integer parameter converts the PARAMETER up, leaving the + /// column seekable. SQL Server agrees: it raises no PlanAffectingConvert warning on this plan, + /// which is why SSMS shows no warning icon on it either. + /// + /// The fixture is a real SQL Server 2025 actual plan of the reporter's repro, captured under + /// PARAMETERIZATION FORCED (hence [@0]) with no index on the search column. Rule 11 still + /// reports the scan and its residual predicate — that part is true, and it is the actionable + /// half. What must not appear is the claim that a conversion prevented a seek. + /// + [Fact] + public void Rule12c_NonSargable_ConvertImplicitOnParameter_NotFlagged() + { + var plan = PlanTestHelper.LoadAndAnalyze("convert_implicit_parameter_side_plan.sqlplan"); + + Assert.Empty(PlanTestHelper.WarningsOfType(plan, "Non-SARGable Predicate")); + Assert.NotEmpty(PlanTestHelper.WarningsOfType(plan, "Scan With Predicate")); + } + + /// + /// #436: the parameter-side conversion above now falls through to the function-on-column check, + /// and it sorts before the function in the predicate text. Reading only the FIRST function match + /// would find CONVERT_IMPLICIT, skip it as benign, and never look at the datepart() sitting on + /// the column — turning a fixed false positive into a new false negative. + /// + [Fact] + public void Rule12d_NonSargable_BenignConvertDoesNotMaskFunctionOnColumn() + { + var plan = PlanTestHelper.LoadAndAnalyze("convert_implicit_masking_function_plan.sqlplan"); + var warnings = PlanTestHelper.WarningsOfType(plan, "Non-SARGable Predicate"); + + Assert.NotEmpty(warnings); + Assert.Contains(warnings, w => w.Message.Contains("DATEPART")); + } + // --------------------------------------------------------------- // Rule 12: Non-SARGable Predicate — Function Call // --------------------------------------------------------------- diff --git a/tests/PlanViewer.Core.Tests/PlanViewer.Core.Tests.csproj b/tests/PlanViewer.Core.Tests/PlanViewer.Core.Tests.csproj index ff8e2e3..a06f49e 100644 --- a/tests/PlanViewer.Core.Tests/PlanViewer.Core.Tests.csproj +++ b/tests/PlanViewer.Core.Tests/PlanViewer.Core.Tests.csproj @@ -7,14 +7,36 @@ false true + true + + + --timeout 15m - + + - - + diff --git a/tests/PlanViewer.Core.Tests/Plans/convert_implicit_masking_function_plan.sqlplan b/tests/PlanViewer.Core.Tests/Plans/convert_implicit_masking_function_plan.sqlplan new file mode 100644 index 0000000..40db85f --- /dev/null +++ b/tests/PlanViewer.Core.Tests/Plans/convert_implicit_masking_function_plan.sqlplan @@ -0,0 +1,31 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/tests/PlanViewer.Core.Tests/Plans/convert_implicit_parameter_side_plan.sqlplan b/tests/PlanViewer.Core.Tests/Plans/convert_implicit_parameter_side_plan.sqlplan new file mode 100644 index 0000000..7ad9789 --- /dev/null +++ b/tests/PlanViewer.Core.Tests/Plans/convert_implicit_parameter_side_plan.sqlplan @@ -0,0 +1,2 @@ + + diff --git a/tests/PlanViewer.Core.Tests/WarningBaseline.txt b/tests/PlanViewer.Core.Tests/WarningBaseline.txt index 322cf24..05cef6f 100644 --- a/tests/PlanViewer.Core.Tests/WarningBaseline.txt +++ b/tests/PlanViewer.Core.Tests/WarningBaseline.txt @@ -7,6 +7,12 @@ Non-SARGable Predicate | Warning | CASE expression in a predicate prevents an in ### compile_memory_exceeded_plan.sqlplan Compile Memory Exceeded | Critical | Optimization was aborted early because the compile memory limit was exceeded. The plan is likely suboptimal. Simplify the query by breaking it into smaller steps using #temp tables. +### convert_implicit_masking_function_plan.sqlplan +Non-SARGable Predicate | Warning | Function call (DATEPART) on column prevents an index seek. Remove the function from the column side — apply it to the parameter instead, or create a computed column with the expression and index that.\nPredicate: CONVERT_IMPLICIT(int,[@0],0)=datepart(year,[TestDB].[dbo].[Orders].[OrderDate]) + +### convert_implicit_parameter_side_plan.sqlplan +Scan With Predicate | Critical | Scan with residual predicate — SQL Server is reading every row and filtering after the fact. This scan is 100% of the plan cost. This scan took 100% of elapsed time. Only 0.100% of rows survived filtering (50 of 50,000). Check that you have appropriate indexes.\nPredicate: [TestDB].[dbo].[IB03_GARANZ].[UNIV_SEZIONI] as [IB03].[UNIV_SEZIONI]=CONVERT_IMPLICIT(numeric(18,0),[@0],0) + ### convert_implicit_plan.sqlplan Implicit Conversion | Critical | Seek Plan: CONVERT_IMPLICIT(nvarchar(40),[ub].[DisplayName],0)=[@d] High Compile CPU | Warning | Query took 1,000ms of CPU just to compile a plan (before any data was read). Simplify the query by breaking it into smaller steps using #temp tables. diff --git a/tests/PlanViewer.Core.Tests/WarningSourceTests.cs b/tests/PlanViewer.Core.Tests/WarningSourceTests.cs new file mode 100644 index 0000000..d2098b8 --- /dev/null +++ b/tests/PlanViewer.Core.Tests/WarningSourceTests.cs @@ -0,0 +1,106 @@ +using System.IO; +using System.Linq; +using PlanViewer.Core.Models; +using PlanViewer.Core.Output; + +namespace PlanViewer.Core.Tests; + +/// +/// #436: a reader could not tell which warnings SQL Server itself raised and which ones we inferred, +/// because both arrive as a with nothing but type, severity and message. +/// +/// The distinction matters more than presentation. A warning the engine wrote into the plan's own +/// <Warnings> element is a record of what happened when the query ran — it spilled, it converted, +/// it had no statistics. One of our rules is an inference from plan shape, and an inference can be +/// wrong about a particular plan in a way the engine's own record cannot be. #436 was itself an +/// example: we claimed a conversion prevented a seek on a plan SQL Server had raised no conversion +/// warning about at all. +/// +public class WarningSourceTests +{ + /// + /// One plan carrying both kinds. "Implicit Conversion" is lifted from the PlanAffectingConvert + /// element SQL Server wrote; "Non-SARGable Predicate" is Rule 12 reading the predicate text. + /// + [Fact] + public void TheTwoKindsAreToldApartOnAPlanCarryingBoth() + { + var plan = PlanTestHelper.LoadAndAnalyze("convert_implicit_plan.sqlplan"); + + Assert.All( + PlanTestHelper.WarningsOfType(plan, "Implicit Conversion"), + w => Assert.Equal(PlanWarningSource.SqlServer, w.Source)); + + Assert.All( + PlanTestHelper.WarningsOfType(plan, "Non-SARGable Predicate"), + w => Assert.Equal(PlanWarningSource.PerformanceStudio, w.Source)); + } + + /// + /// The stamp is applied once, at the single return of ParseWarningsFromElement, rather than at + /// each construction — so this asserts the property that arrangement buys: across every committed + /// plan, no warning type is ever produced as both kinds. A type appearing as both would mean a + /// construction site got missed or an analyzer rule started claiming the engine's authority. + /// + [Fact] + public void NoWarningTypeIsEverProducedAsBothKinds() + { + var plansDir = Path.Combine(AppContext.BaseDirectory, "Plans"); + var confusions = + (from file in Directory.GetFiles(plansDir, "*.sqlplan") + let plan = PlanTestHelper.LoadAndAnalyze(Path.GetFileName(file)) + from warning in PlanTestHelper.AllWarnings(plan) + group warning.Source by warning.WarningType into byType + where byType.Distinct().Count() > 1 + select byType.Key).ToList(); + + Assert.True(confusions.Count == 0, + "These types are attributed to both SQL Server and us: " + string.Join(", ", confusions)); + } + + /// + /// Only the engine's warnings are tagged in rendered output. Tagging both would put a badge on + /// every line, which carries no information — our own advice is what a reader already expects + /// from a plan analyzer. + /// + [Fact] + public void OnlyTheEnginesWarningsAreTaggedInTextOutput() + { + var plan = PlanTestHelper.LoadAndAnalyze("convert_implicit_plan.sqlplan"); + var result = ResultMapper.Map(plan, "convert_implicit_plan.sqlplan"); + + var writer = new StringWriter(); + TextFormatter.WriteText(result, writer); + var text = writer.ToString(); + + var tagged = text.Split('\n').Where(l => l.Contains("[SQL Server]")).ToList(); + + Assert.NotEmpty(tagged); + Assert.All(tagged, line => Assert.Contains("Implicit Conversion", line)); + Assert.DoesNotContain("Non-SARGable Predicate [SQL Server]", text); + } + + /// The JSON and MCP consumers get it as a field rather than having to parse the tag out. + [Fact] + public void TheJsonOutputCarriesTheSource() + { + var plan = PlanTestHelper.LoadAndAnalyze("convert_implicit_plan.sqlplan"); + var result = ResultMapper.Map(plan, "convert_implicit_plan.sqlplan"); + + var all = result.Statements + .SelectMany(s => s.Warnings.Concat(Flatten(s.OperatorTree))) + .ToList(); + + Assert.Contains(all, w => w.Type == "Implicit Conversion" && w.Source == nameof(PlanWarningSource.SqlServer)); + Assert.Contains(all, w => w.Type == "Non-SARGable Predicate" && w.Source == nameof(PlanWarningSource.PerformanceStudio)); + Assert.All(all, w => Assert.NotEqual("", w.Source)); + } + + private static System.Collections.Generic.IEnumerable Flatten(OperatorResult? node) + { + if (node == null) yield break; + foreach (var w in node.Warnings) yield return w; + foreach (var child in node.Children) + foreach (var w in Flatten(child)) yield return w; + } +}