Skip to content

Confirm before executing a plan file's query against a server - #402

Merged
erikdarlingdata merged 1 commit into
devfrom
feature/confirm-actual-plan
Jul 25, 2026
Merged

Confirm before executing a plan file's query against a server#402
erikdarlingdata merged 1 commit into
devfrom
feature/confirm-actual-plan

Conversation

@erikdarlingdata

Copy link
Copy Markdown
Owner

Adds the missing confirmation before "Get Actual Plan" executes a plan file's query against a live server. From the quarterly maintenance security review.

The gap

Two paths reach "execute this query to capture an actual plan":

  • Query session (QuerySessionControl.Execution.cs) - confirmed first
  • Loaded plan file (MainWindow.PlanViewer.cs) - prompted for a connection, then just ran it

The plan-file path is the riskier of the two. The SQL comes out of a .sqlplan the user opened rather than something they typed, and plan files get emailed around. So the one without a confirmation was the one that most needed it.

The dialog

It names the target, because "which server was that?" should be answered before the query runs, not after:

This will execute the query stored in this plan file against:

sql2022, database [StackOverflow]

The query text comes from the plan file, not from you. Review it first if
the file did not originate on this machine.

It runs with SET STATISTICS XML ON and all data results are discarded.

Continue?

Shared, not copied

The dialog moves to Dialogs/ConfirmationDialog.cs rather than being duplicated into MainWindow. Two separate implementations is precisely how one path ended up with no confirmation at all - and DdlScripter in this repo carries a comment about the same thing happening to it ("previously duplicated in both controls' code-behind, where the two copies had drifted"). QuerySessionControl now delegates, so its behavior is unchanged.

AppButton lives in Application.Resources (via Themes/DarkTheme.axaml), so resolving it from the owner window works exactly as it did from the control - worth stating since a miss there would throw at runtime rather than at build.

Test plan

  • Clean Debug build: 0 warnings
  • 202/202 tests pass
  • App launches and paints (regression check, since this touches a shared control)
  • The dialog itself is not driven end-to-end here - it needs a loaded plan with query text plus a live server connection. The refactor keeps the query-session path byte-identical in behavior, and the new call site is a straight await on the same helper.

🤖 Generated with Claude Code

"Get Actual Plan" on a loaded plan file prompted for a connection and
then ran the query. The query-session path already confirmed first; this
one did not, and it is the riskier of the two - the SQL comes out of a
.sqlplan the user opened rather than something they typed, and plan
files get emailed around.

The dialog names the target server and database, so "which server was
that?" is answered before the query runs rather than after.

Extracts the dialog into Dialogs/ConfirmationDialog.cs instead of
copying it. The two paths having separate implementations is how one of
them ended up with no confirmation at all; DdlScripter has the same
history of a duplicated helper drifting. QuerySessionControl now
delegates to the shared version, so its behavior is unchanged.

Verified: clean build 0 warnings, 202/202 tests, app launches and
paints. AppButton resolves from Application.Resources, so looking it up
from the owner window works the same as from the control.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@erikdarlingdata
erikdarlingdata merged commit 552de30 into dev Jul 25, 2026
3 checks passed
@erikdarlingdata
erikdarlingdata deleted the feature/confirm-actual-plan branch July 25, 2026 16:48
This was referenced Jul 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant