[housejester]

What

A connection of type: "sqlserver" reaches Microsoft SQL Server through @malloydata/db-sqlserver (Malloy 0.0.435): a SQL login over TCP with server, port, database, user, password, encrypt, trustServerCertificate, hostNameInCertificate and instanceName. The field names are the connector's own, so the pojo mapping is a pass-through and the authentication kinds and connection-string form the connector also takes are additive later.

• Server: the connector registered in connection.ts; static attributes dialectName: sqlserver, isPool: true, canStream: true, canPersist: false; validateConnectionShape checks that a port is an integer from 1 to 65535 and warns when both instanceName and port are set, since the driver takes the instance; a blank instanceName or hostNameInCertificate is read as unset, because the driver drops the port whenever instanceName is defined at all; public view with the password withheld; sqlserver in DOUBLED_ESCAPE_DIALECTS (the connector runs with QUOTED_IDENTIFIER ON, T-SQL has no backslash escape).
• Schema browsing: schemas are listed from information_schema.tables, so a schema holding only views appears (dbo the default), and tables from information_schema.columns, addressed schema.table. Both compare names as N'…' literals at the SQL Server call sites, so a non-ASCII schema lists under a non-Unicode collation.
• Experimental dialect: experimental_dialects.ts calls acceptExperimental() for the experimental dialects this server bundles a connector for. It runs at startup in server.ts and in the package-load worker, because the dialect registry is module state and each thread has its own. A model needs no ##! experimental.dialect.sqlserver pragma; the connector's README carries the dialect's upstream status and limits.
• Persist: the connector is read-only, so a colocated #@ persist on a SQL Server source is refused at load by the colocated gate with the new RefusedSource.reason value read_only_dialect. The package loads, the source serves live, and the package's warnings carry the reason, the same shape as the other eligibility refusals. A source or rollup that declares storage= is not refused here; the storage build's passthrough check already names the source types it can read. A spec checks, across every type in the API's Connection.type enum and in both directions, that the read-only set and the static canPersist agree.
• Errors: tedious's ELOGIN (every server-side login refusal, with the server's message) answers as rejected credentials; ESOCKET, EINSTLOOKUP and a connect-timeout ETIMEOUT as unreachable. A request-timeout ETIMEOUT is not, since the server ran the query. The cause walks follow mssql's originalError as well as cause, so an ESOCKET caused by a TLS verification failure is seen as one and stays unclassified: it answers as any unclassified driver failure on that route, and the log and the connection test name the certificate. On the single-table lookup, where the connector returns failures as text, an unreachable server answers 502 and a missing table 404; a refused login there is listed as not covered yet, as for Postgres.
• SDK: the field list and the "SQL Server" label; required-field validation on create and edit, with the port optional when an instance name is given; encrypt and trustServerCertificate as Yes/No selects, which meant teaching both dialogs' generic field renderer to honour selectOptions; and typedConnectionConfig, which sends port as an integer and the two switches as booleans, and leaves a blank optional field out rather than storing an empty string.
• Docs: a SQL Server section in docs/connections.md (fields, schema model, read-only status, and the two limits: every login refusal answers 424, including a transient Azure one, and the table-lookup gap), a paragraph on read-only connections in docs/materialization.md, the type lists in README and AGENTS, and an [Unreleased] release note.

Testing

• bun test: server 6808 pass, SDK 2121 pass, on the branch rebased onto current main. tsc, eslint and prettier clean on both packages.
• Unit coverage for each behaviour above, including fixtures that reproduce the driver's real error nesting (mssql's error carries tedious's on originalError, which carries Node's TLS error on cause); the certificate and table-lookup tests fail against the previous error walk.
• Trivy on the lockfile against main: no new findings from the mssql/tedious tree.
• Against a live SQL Server (mcr.microsoft.com/azure-sql-edge, collation SQL_Latin1_General_CP1_CI_AS), through the REST API on a bun src/server.ts run of this branch:
  • a model with no pragma compiles in the load worker and serves; flights -> by_carrier and orders -> by_customer return correct rows, the latter including a value with an embedded quote;
  • /schemas lists dbo, sales and a Cyrillic schema Продажи, whose table lists only with the N'…' literal;
  • POST /connections/test answers ok, Login failed for user 'sa'., a refused connect, and self signed certificate for each case;
  • schema listing answers 424 CONNECTION_AUTH_FAILED for a wrong password and 502 CONNECTION_FAILED for a closed port; the table lookup answers 502 for a closed port and 404 for a missing table;
  • a connection on port 14333 survives a PATCH carrying empty instanceName and hostNameInCertificate, and setting a real instance name breaks it, which shows the edit reached the live connection;
  • a #@ persist source appears in buildPlan.refusedSources with reason: read_only_dialect and still answers a query live.
• Through the Console in Chrome, on a connection on port 14333: create from the dialog, the row's "SQL Server" label, the connection explorer listing schemas and tables, edit (stored switch values shown, password kept, blank optional fields not stored, connection still working afterwards), and delete.

[jswir]

Thanks for this. I ran the branch against a local SQL Server 2022 container (mcr.microsoft.com/mssql/server:2022-latest) with a real customer-shaped database. Most of it works well:

• The package loaded with no errors, and queries ran and returned results.
• Schema and table listing worked, and the password was withheld from every response.
• The connection-test endpoint reported ok, wrong password, closed port and self-signed certificate correctly.
• Add connection in the Console worked.

I found three things I'd like fixed before merge. All three reproduced live.

1. Saving a connection through Edit breaks it

How it breaks:

1. A connection uses a non-default port, for example 14333. Open Edit, change nothing (or just the password), and save.
2. The Edit dialog sends empty fields as "", so the stored config gets instanceName: "" and hostNameInCertificate: "" (EditConnectionDialog.tsx:374). The Add dialog leaves empty fields out, so new connections are fine.
3. The driver drops the port whenever instanceName !== undefined (@malloydata/db-sqlserver sqlserver_connection.js:196), and it counts "" as set.
4. The connection now dials localhost:1433 and answers 502 "database could not be reached".

The server doesn't catch it either: the "instanceName and port both set" warning tests truthiness (connection_config.ts:663).

Suggested fix: treat an empty string as "not set" on both sides. Drop it in the Edit dialog the way Add does, and normalize "" to undefined in the server's sqlserver config pass-through, so a hand-written config can't hit this. A test that edits a connection with a custom port would pin it.

2. A certificate error answers as "database unreachable"

The release note says a certificate the client can't verify is not read as unreachable. Live, with encrypt on and trustServerCertificate off against a self-signed server, schema listing answers 502 CONNECTION_FAILED ("the database could not be reached").

This is the real error chain under Bun:

mssql ConnectionError   code=ESOCKET   (no .cause, has .originalError)
└ tedious ConnectionError code=ESOCKET   .cause →
  └ Error code=DEPTH_ZERO_SELF_SIGNED_CERT

causeChainHasTlsVerificationFailure follows only .cause, so it stops at the outer mssql error and never sees the certificate code. The unit test in errors.spec.ts builds the error with .cause set directly, so it passes without exercising the chain the driver actually produces.

Suggested fix: follow originalError as well as cause, in both this walk and the outer one in isConnectionFailure. The test fixture should then match the real nesting.

3. The single-table lookup doesn't get the new statuses

On GET …/schemas/dbo/tables/{tablePath}, live:

Case	Status
wrong password	plain 502, no reason
closed port	plain 502, no reason
table that doesn't exist	plain 502, no reason

The driver returns failures here as text with the error code dropped: Error fetching schema for table <path>: 0: <message>. fetchTable strips Error fetching schema for <path>: , without table and without 0: , so the prefix never matches.

Postgres has the same gap for a wrong password. The 0.9.7 release note lists it under "Not covered yet" (RELEASE_NOTES.md:266). To be consistent with that:

• Add SQL Server to that list for a rejected login on the table lookup, and qualify the "424 / 502" sentence in this PR's release note and in docs/connections.md.
• Recognize a dead server from the text, as Postgres does. Match the driver's prefix, then add an anchored pattern for tedious's Failed to connect to <host>:<port> - … wording that excludes the certificate case.
• Optionally, map the driver's Table <path> not found to a 404. Postgres doesn't do this today either, so it's fine as a follow-up.

Smaller things, not blocking

• A port that isn't a number silently becomes 1433. {"port": "14x33"} reaches the driver unchecked, and the connection test reports Failed to connect to localhost:1433. A server-side check that port is an integer would name the bad value instead.
• Every login refusal answers 424. tedious tags all of them ELOGIN, including transient Azure ones (40613 "database not currently available", 40501, 40197), and it drops the server's error number. So a paused serverless database is reported as a wrong password. I couldn't test this locally. A one-line caveat in docs/connections.md is probably enough.
• "Read-only" is declared in two places. READ_ONLY_DIALECTS in materialization_eligibility.ts and canPersist: false in connection_config.ts must agree by hand, and the spec only checks one direction. Deriving one from the other, or checking both directions in the spec, would stop them drifting apart.

[jswir]

I ran the same checks against SQL Server 2019 (mcr.microsoft.com/mssql/server:2019-latest, 15.0.4500.2). It behaves the same as 2022: queries, schema and table listing, and the 424 for a wrong password all work, and the certificate case answers 502 as described above.

One more thing, confirmed on 2019: a schema with a non-ASCII name lists no tables.

1. The database uses a Latin1 collation (SQL_Latin1_General_CP1_CI_AS, a common default) and has a schema Продажи containing one table.
2. Schema listing returns Продажи correctly.
3. Table listing for that schema returns [].

listTablesForSQLServer filters with table_schema = '<name>', and sqlInFilter builds table_name IN ('…') the same way. Without the N prefix, SQL Server converts the literal to the database's code page, so Продажи becomes question marks and matches nothing. information_schema columns are nvarchar, so the fix is to write N'…' at these two SQL Server sites. sqlLiteral is shared with dialects that have no N'…' syntax, so the change belongs in the SQL Server function, not in the shared helper.

[housejester]

Thanks for testing this against real servers. All of it is addressed on the latest push.

1. Edit with a non-default port: the Edit dialog now leaves a blank optional field out, and the server reads a blank instanceName or hostNameInCertificate as unset. Verified on a connection on port 14333, through the Console and through a raw PATCH carrying the empty strings.
2. Certificate error: both walks now follow originalError as well as cause, and the fixture matches the driver's real nesting. It no longer answers 502. On the listing routes it answers as any unclassified driver failure, and the docs now say so.
3. Table lookup: the connector's prefix and its 0: marker are recognized, so a closed port answers 502 and a missing table 404. A refused login there is listed under "not covered yet", as for Postgres.
4. Non-ASCII schema: SQL Server's two literal sites now write N'…'. Reproduced and verified under SQL_Latin1_General_CP1_CI_AS.

The smaller items are in too: a port check at load, the Azure login caveat in the docs, and the read-only parity test across every connection type in both directions.

[jswir]

Thanks, this all looks good. I re-reviewed the two new commits (94ccf1ab7, 33eddc0a1) and ran the touched specs: 356 pass across the five server files, and common.spec.ts in the SDK passes. I didn't re-run the live SQL Server checks this time.

• Edit with a custom port: fixed on both sides. I also checked that leaving a field out on Edit doesn't keep the old value: mergeConnectionUpdate replaces the whole sqlserverConnection object and only carries the password forward, so clearing the instance name in the Console still clears it.
• Certificate error: fixed. The walks follow originalError, and the test now asserts the outer error has no .cause, so it can't pass without walking the real chain.
• Table lookup: fixed. A closed port answers 502 and a missing table 404. The refused-login gap is documented, as for Postgres.
• Non-ASCII schema: fixed with N'…' in the SQL Server function only.
• The smaller items (port check, Azure caveat, read-only parity test in both directions) are all in.

Three small things, none blocking:

1. A refused login on the table lookup could become 424 later. The connector's text keeps SQL Server's error number, so 18456: Login failed… could map to rejected credentials. I haven't checked that the number survives for a login failure, so this is a follow-up, not a change for this PR.
2. Some TLS errors still read as unreachable. The new message pattern only excludes text containing "certificate", so a server that doesn't speak TLS ("wrong version number") still answers 502. Rare, and arguably fine.
3. One line of the PR description is out of date. It says schema listing uses sys.schemas joined to sys.tables. The code now reads information_schema.tables, which also lists schemas that hold only views.

[housejester]

Thanks for the re-review.

1. I checked the number for a refused login on the table lookup: the connector reports it as 0: Login failed for user 'sa'., because the login fails while the pool connects, before the server answers a query, so 18456 never reaches the text. Mapping it would mean matching the message, so I've left it documented as not covered, as for Postgres.
2. Agreed it's arguably fine. A server that doesn't speak TLS usually means the wrong port or something in between, which is closer to unreachable than to a certificate problem, so I've left it at 502.
3. Fixed. I updated the description throughout to match the code.

The branch is also rebased onto current main.