Repository navigation
The Add Server dialog can say PostgreSQL (#3499) - #3504
Merged
Merged
Conversation
The service carries PostgreSQL targets end to end -- the registry row has held engine and port since V70, the probe answers in that engine's version vocabulary (#3244), the collectors and the get_pg_* MCP surface serve the data -- and the desktop dialog that onboards servers offered no way to say the word. The viewer's row model, its upsert, and every one of its registry reads simply predated the columns: writes defaulted them ('sqlserver'/0) and reads never surfaced them, so an operator adding a PostgreSQL target got a SQL Server row whatever they meant, and the README's claim that the dialog is a PostgreSQL onboarding route described the intended state, not the shipped one. Reported externally with tag-pinned citations and an expected-behavior list precise enough to use as the acceptance spec -- and it was. The dialog grows an engine selector; SQL Server stays the checked default and that arm is pinned to render exactly the pre-selector form plus the selector row itself. The PostgreSQL arm exposes port (blank = 5432, the same omitted-means-default contract as MCP add_servers, so a default-port target derives the SAME identity whichever door onboarded it), keeps the shared Database/Encryption/Trust controls with a note mapping them onto sslmode, hides the SQL-Server-only controls by Visibility only -- hidden is not reset, which is what lets an edited row's stored values survive the round trip -- and pins auth to username/password, the one mode the PostgreSQL connect path honors, enforced again at save with the same tailored refusal the backend applies (#3486's discipline: name what IS supported). Engine and port ride Test Connection -- probing the exact connection the save will store, instead of reaching 5432-as-SQL-Server and vouching for a row that then fails every sweep -- and land in the registry write; port is omitted from args_json at its default so a SQL Server request's bytes are unchanged. The edit path loads a PostgreSQL row back recognizing every stored engine spelling (a darling.json seed persists its raw text), locks the engine radios -- the engine is part of what the server IS, and #2158 made an edit keep its identity, so a flip would interleave two engines' histories under one id -- and saves the stored engine string verbatim. The identity plumbing beneath gets the same completion. ComputeServerId takes engine and port with inert defaults (no pre-existing id moves; a PostgreSQL Add derives what add_servers would). The address-collision guard compares the full #2218 identity, with engine folded to a KIND in SQL rather than compared raw: the stored spelling is whatever onboarded the row, and because the upsert lands ON CONFLICT (server_id), a raw-string miss against a same-identity row spelled differently is not a duplicate but a silent clobber of the existing registration -- the exact write the guard exists to refuse. The bulk dialog's dedupe gate inherits the full-identity key for the same reason (a PostgreSQL row in the store no longer reads a pasted same-host SQL Server add as a duplicate); its engine picker is deliberately deferred, stated in the class doc: its paste grammar already spends the numeric field on the SQL Server port-rides-the-host convention, and bulk PostgreSQL onboarding stays with add_servers, a bulk tool by construction. Pinned in the #3491 idiom: the pure seams behaviourally (ParsePortText, IsPostgres parity with the service's own TargetEngine parse, row defaults, identity parity, args round-trip through MonitoredServer, dedupe-gate arms), the WPF shapes textually (the XAML default state IS the SQL Server form, the edit lock and prefill, the save gate, the test-connect threading). The viewer role's column grants already carried engine and port (V68), verified on both the shipped .sql and the managed-roles twin.
erikdarlingdata
enabled auto-merge (squash)
September 17, 2026 12:17
…ts walked up on their own RepoFileAdoptionTests holds exactly what the #3489 review nit asked for - one declared reader, everyone else adopting it - and the new dialog tests declared a private walk-up instead. Adopted; the helper census is the reason a thirty-fifth copy cannot ship.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #3499 — ceremonially; the watcher closes it at merge.
The gap, as the reporter framed it
The service monitors PostgreSQL fleets end to end — the registry row carries the engine, test-connect answers with a discriminator (#3244), and the MCP/CLI onboarding routes work daily in production — but the Viewer's Add Server dialog never grew the engine dimension, and the README named the dialog as a PostgreSQL onboarding route it could not perform. The external report that surfaced this came with tag-pinned source citations and an expected-behavior list precise enough to serve as the acceptance spec, which is how it was used.
What the dialog can now say
An engine selector (SQL Server stays the checked default — an untouched dialog renders exactly the pre-selector form, pinned), and a PostgreSQL arm that exposes what the backend's contract actually honors: host, a port box where blank means the driver default (matching MCP's omit contract, so a default-port dialog Add derives the same server_id as an MCP-omit onboarding — one server, one identity, whichever door it came through), the maintenance-database note, and the sslmode mapping on the shared encryption controls. Authentication pins to SQL auth because that is the one mode the PostgreSQL connect path honors — the Windows/Entra radios hide, and a credential profile resolving to a non-SQL mode is refused at build time with a tailored message in #3486's style, the same rule the backend's own validation enforces. The SQL-semantic fields the tool accepts engine-agnostically are hidden-not-reset on the PG arm, so an edit round-trip preserves stored values.
The identity plumbing underneath, where the real hazards were
The viewer's upsert and reads gained the engine and port columns;
ComputeServerIdtakes them inertly; and the address-collision guard now compares the full #2218 identity with the engine folded to a kind in SQL — the stored spellings vary (postgres,aurora-postgresql), and a raw string compare would have turned a same-identity-different-spelling miss into a silent ON CONFLICT clobber of an existing row. Test Connection carries engine and port, with the port omitted at default so the SQL-arm request stays byte-stable for older services. On edit, the engine radios lock — history is keyed to the engine, and the #2158 preserved-identity contract holds — while the stored engine spelling rides through verbatim.AddMultipleServersDialogdeliberately does not grow the picker: its paste grammar spends the numeric field on the SQL Server host,port convention, and bulk PostgreSQL onboarding stays withadd_servers— stated in the class doc. It did take the correctness fix the audit found: its duplicate gate now keys on the full identity, so a stored PostgreSQL row no longer false-dupes a pasted same-host SQL Server add.The record around it
The README's onboarding sentence needed no edit — it is simply true now. The cross-linked issues were both closed and neither tracked this work (#2530 was presentation, #3244 the reply-side discriminator this dialog now consumes). Lite has no PostgreSQL targets by design, so no twin obligation exists. Tests: the port parser's arms, the engine-recognition parity pin (the row's IsPostgres must match TargetEngine's parse forever), the untouched-dialog regression pin, the 18-column and identity-parity facts, the port-rides/port-omitted request shapes, and the dedupe-gate fact. Tests cannot run on macOS (net10.0-windows); verified by full-solution build and pin-by-pin source verification — CI is the first execution, review-dark per the known pre-release window.