Repository navigation
feat: DM-5394: add the SQL Server named instance to DatabaseConnectionConfig - #80
Open
AaronBarnes07 wants to merge 1 commit into
Open
AaronBarnes07 wants to merge 1 commit into
AaronBarnes07 wants to merge 1 commit into
Conversation
…nConfig A SQL Server connection reaches a named instance through the SQL Server Browser and stores no port, so the port becomes optional alongside the new field: the client could not deserialise such a connection at all.
This branch has not been deployed
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
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.
DatabaseConnectionConfiggainsinstance, the SQL Server named instance taken from the Instance input in the connection form's advanced settings. It is sent only formssqlandmssql_linked, the two engines that declare it on the server, pruned in_serializethe waydata_encodingandiam_role_arnalready are.portbecomes optional in the same change. A connection that names an instance is reached through the SQL Server Browser and is saved with the Port field blank, and the admin server stores the config withexclude_none=True, soGET /api/connections/returns noportkey for it. The client'sport: intrejected that payload outright, so the new field would have been unreadable on the one connection shape it exists for.Jira: https://datamasque.atlassian.net/browse/DM-5394
Decisions this change rests on
portbecame optional, which the ticket did not ask for. The server has hadport: int | None = Noneon every database connection for a long time, and the merged ui-testing testtest_sql_server_named_instance_saved_with_blank_port_keeps_instance_on_editasserts the blank port is the point of a named instance. I rejected addinginstancealone, because it ships a field that cannot be read back on the connection it is for, and I rejected a per-engine port validator, because the server imposes no such requirement and the client should not be stricter than the API it wraps.SnowflakeConnectionConfig.portis alreadyOptional[int], so this follows the file rather than setting a new precedent. Evidence that the admin server rejects a create with noportwould overturn it; I checked, and validation runs through the same pydantic schema that defaults it toNone.instanceis kept formssql_linked, not onlymssql. Both server schema variants declare it and the connection form offers the input for both. The agent ignores the value on the linked-server path, so the field is carried rather than acted on for that engine. I rejected dropping it client-side for that engine, because it would lose a value the server round-trips. The server removinginstancefromMssqlLinkedConnectionwould change it.No version bump. The
HISTORY.rstentry goes under1.6.0 (unreleased), which is the repo's convention. I rejected bumping the version here, because it couples a model change to a release decision. A maintainer wanting this released on its own would change it; the PyPI publish is a separate step either way.Test evidence
Run in a worktree on the branch head,
1ff1346, on 2026-11-09 and re-run unchanged on 2026-11-12 before this pull request was opened:make lint: ruff check, ruff format --check and mypy all clean, 39 source files type-checked.uv run pytest: 579 passed.tests/test_connections.pycovering read-back with and without an instance, the round trip back to the API, the create request body, the linked-server variant, and the prune for five non-SQL-Server engines. The red half was proved by reverting onlyconnection.pyand re-running: 9 of the 10 fail without the model change. The tenth,test_database_connection_omits_the_named_instance_when_unset, passes either way because an absent field is absent either way; it is kept for symmetry withtest_database_connection_omits_tagging_iam_role_when_unset, which has the same property.I reviewed the diff and fixed one thing: a test docstring explained the prune by comparing it to the
data_encodingprune, and now states that these engines have noinstancefield on the server. Two style-guard hits, intests/test_connections.pyanddatamasque/client/models/connection.py, are pre-existing and outside this diff, confirmed withgit blame, so I left them.No AIT run and no UI test run: the change is confined to the client library and touches no masking engine, no
admin-frontend/and noui-testing/.Not in this change
Switching the ui-testing named-instance assertions to the typed model needs a released datamasque-python, so it follows this merging and the release. It also needs more than this field: the
TODO DM-5394inget_connections_json_by_namegives a second reason for the raw-JSON workaround, thatlist_connectionsraises when any connection on the instance fails to deserialise. That is untouched by this change and would exist regardless.No companion merge requests: this change is confined to one repository.