-
Notifications
You must be signed in to change notification settings - Fork 571
[Application QuickStart] [Backend] DAB Connection string transformation for Docker containers #21180
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR implements connection string transformation for DAB (Data API Builder) deployments in Docker containers. When DAB runs in a Docker container and needs to connect to SQL Server, localhost references in connection strings must be transformed to enable proper communication across container boundaries.
Changes:
- Added
sqlServerContainerNameproperty toDabConnectionInfointerface to support containerized SQL Server scenarios - Implemented
transformConnectionInfoForDockermethod that replaces localhost variants (localhost, 127.0.0.1, (local), .) withhost.docker.internalfor Docker networking - Updated
runDeploymentStepsignature to acceptDabConnectionInfoinstead of just connection string - Added comprehensive unit tests covering transformation logic edge cases and integration scenarios
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| extensions/mssql/src/sharedInterfaces/dab.ts | Added sqlServerContainerName optional property to DabConnectionInfo interface and updated runDeploymentStep parameter from connectionString to connectionInfo |
| extensions/mssql/src/services/dabService.ts | Implemented transformConnectionInfoForDocker with helper methods to parse and transform localhost addresses; integrated transformation into generateConfig method |
| extensions/mssql/src/schemaDesigner/schemaDesignerWebviewController.ts | Added _sqlServerContainerName field and resolveSqlServerContainerName method to extract container name from connection profile; updated DAB service calls to pass DabConnectionInfo |
| extensions/mssql/test/unit/dab/dabService.test.ts | Added extensive test suites covering localhost variants, port/instance preservation, containerized SQL Server scenarios, and edge cases; updated existing tests to use DabConnectionInfo |
| extensions/mssql/test/unit/schemaDesignerWebviewController.test.ts | Added tests for connection string transformation in config generation and container name resolution from different sources |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
PR Changes
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #21180 +/- ##
==========================================
+ Coverage 69.13% 69.21% +0.07%
==========================================
Files 255 255
Lines 25160 25200 +40
Branches 3284 3293 +9
==========================================
+ Hits 17395 17441 +46
+ Misses 7629 7623 -6
Partials 136 136
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const serverValue = serverMatch[1].trim(); | ||
|
|
||
| // Parse the server address to check if it's localhost | ||
| const host = this.parseHostFromServerValue(serverValue); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this information already in connectionInfo?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
connectionString was already available in DabService. I will file an improvement item for using connectionManager to do the rewrite.
| let newServerValue: string; | ||
| if (sqlServerContainerName) { | ||
| // Extract port if present (comma-separated), discard any instance name | ||
| const commaIndex = serverValue.indexOf(","); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use the connection info object from to get these details directly instead of parsing it from connection string.
I think we should use the original connection info, clone it, mutate it and then create a connection string out of it. It will be less brittle than regexes.
vscode-mssql/extensions/mssql/src/connectionconfig/connectionDialogWebviewController.ts
Lines 606 to 627 in a29a77f
| const connectionDetails = | |
| this._mainController.connectionManager.createConnectionDetails( | |
| cleanedConnection, | |
| ); | |
| let tempUserId = false; | |
| if ( | |
| connectionDetails.options.authenticationType === | |
| AuthenticationType.AzureMFA && | |
| connectionDetails.options.user === undefined | |
| ) { | |
| // STS call for getting connection string expects a user when AzureMFA is used; if user is not set, set it to empty string | |
| connectionDetails.options.user = ""; | |
| tempUserId = true; | |
| } | |
| connectionString = | |
| await this._mainController.connectionManager.getConnectionString( | |
| connectionDetails, | |
| true /* includePassword */, | |
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think thats probably the best way to do it too.
Although, we might have to do some more plumbing in SchemaDesignerWebviewController to do that whereas connectionString was already available. We are only replacing in a narrow case - Server part when its localhost. So, I think the regex matches here are less error prone.
So, I will merge this for now and file an improvement item for using connectionManager to do the rewrite.
aasimkhan30
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good with minor comments.
Description
The default connection string from the Object Explorer will point to localhost (or other forms) for local sqlserver deployment. For DAB deployed in Docker we need to transform this for DAB (to host.docker.internal) to be able to redirect to host correctly.
sqlServerContainerNameproperty to theDab.DabConnectionInfointerfaceTesting and validation:
Code Changes Checklist
npm run test)Reviewers: Please read our reviewer guidelines