v2 - Use tedious mssql module instead of sqlcmd #96

zijchen · 2022-05-20T18:57:10Z

Use tedious node-mssql module for connection instead of using sqlcmd. This change adds the module and changes the IP address detection to use it. Script action still calls sqlcmd directly.

SqlConnectionStringBuilder has been replaced by SqlConnectionConfig which is a wrapper on mssql.config. Removed our own implementation for different parts of the connection string, instead leverage the parsing function from mssql to get a config object from a connection string, with some extra validation. AAD auth will be added in this class in subsequent PRs.

TODO in future PRs:

AAD/other authentication type support
Replace Script action with node-mssql execute directly
Input parameter breaking changes

zijchen · 2022-05-24T20:42:34Z

@dzsquared @udeeshagautam thoughts on merging PR directly to main branch or have all changes on a feature branch until we're ready for v2 release?

dzsquared · 2022-05-24T21:27:21Z

@dzsquared Drew Skwiers-Koballa FTE @udeeshagautam Udeesha Gautam FTE thoughts on merging PR directly to main branch or have all changes on a feature branch until we're ready for v2 release?

please merge to a v2 branch, folks may have @master referenced in workflows using the action.

src/SqlUtils.ts

zijchen · 2022-06-01T00:10:15Z

I made 1 more large edit to this PR: SqlConnectionStringBuilder class has been replaced by SqlConnectionStringConfig which is a wrapper on mssql.config. Connection is made directly with the config object which sets us up nicely for AAD auth in the next PR.

Benjin · 2022-06-01T21:48:22Z

src/Constants.ts

@@ -1,5 +1,7 @@
 export default class Constants {
    static readonly ipv4MatchPattern = /\b\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3}\b/;
+    static readonly connectionStringTester = /^[;\s]*([\w\s]+=(?:('[^']*(''[^']*)*')|("[^"]*(""[^"]*)*")|((?!['"])[^;]*)))(;[;\s]*([\w\s]+=(?:('[^']*(''[^']*)*')|("[^"]*(""[^"]*)*")|((?!['"])[^;]*))))*[;\s]*$/;


Oof, that's a beefy regex. Regex is a powerful tool, but should be wielded carefully because it's difficult to review/debug. Did you get this regex from somewhere official, or write it yourself?

I think I'd rather have a function that manually parses a connection string; probs would be more performant, too.

It's from the file I replaced (SqlConnectionConfig replaced SqlConnectionStringBuilder)

sql-action/src/SqlConnectionStringBuilder.ts

Lines 26 to 27 in 7e69fdc

const connectionStringParserRegex = /(?<key>[\w\s]+)=(?<val>('[^']*(''[^']*)*')|("[^"]*(""[^"]*)*")|((?!['"])[^;]*))/g

const connectionStringTester = /^[;\s]*([\w\s]+=(?:('[^']*(''[^']*)*')|("[^"]*(""[^"]*)*")|((?!['"])[^;]*)))(;[;\s]*([\w\s]+=(?:('[^']*(''[^']*)*')|("[^"]*(""[^"]*)*")|((?!['"])[^;]*))))*[;\s]*$/

src/FirewallManager.ts

Benjin · 2022-06-01T21:54:39Z

src/FirewallManager.ts

@@ -11,7 +11,7 @@ export default class FirewallManager {
            core.debug(`Client has access to Sql server. Skip adding firewall exception.`);
            return;
        }
-        console.log(`Client does not have access to Sql server. Adding firewall exception for client's IP address.`)
+        console.log(`Client does not have access to Sql server. Adding firewall exception for client's IP address.`);


core.debug vs. console.log? Dunno whether these should use the same printout method.

core.debug only logs when debug logging is enabled

console.log will display 100% of the time, so I think this makes sense

src/SqlConnectionConfig.ts

Benjin · 2022-06-01T22:22:07Z

src/main.ts

@@ -47,20 +43,28 @@ export default async function run() {
    }
 }

+function setUserAgentVariable(): void {


Is this UA construction defined/specced anywhere that you could add a link to?

I'm not sure, couldn't find any documentation. This logic was already there, I just moved it to its own private function. I'll keep an eye out in the docs if I come across anything like this.

Benjin · 2022-06-01T22:24:29Z

src/AzureSqlAction.ts

@@ -123,7 +122,7 @@ export default class AzureSqlAction {

        switch (inputs.sqlpackageAction) {
            case SqlPackageAction.Publish: {
-                args += `/Action:Publish /TargetConnectionString:"${inputs.connectionString.connectionString}" /SourceFile:"${inputs.dacpacPackage}"`;
+                args += `/Action:Publish /TargetConnectionString:"${inputs.connectionConfig.ConnectionString}" /SourceFile:"${inputs.dacpacPackage}"`;


Might you need to escape any doublequotes inside the connection string here?

Good point, I'll add a test case for that also

Actually I don't believe this needs to be escaped. This string is the same as the one user supplied on the command line, so it should be properly escaped already otherwise it will fail validation early on.

kisantia · 2022-06-06T18:06:50Z

src/SqlUtils.ts

-                    stderr: (data: Buffer) => sqlCmdError += data.toString()
-                }
-            });
+            core.debug(`Validating if client ' has access to SQL Server '${connectionConfig.Config.server}'.`);


extra ' here: Validating if client ' has access...?

* v2 - Use tedious mssql module instead of sqlcmd (#96) * Use tedious mssql library instead of sqlcmd * Fix mssql connection * Fix SqlUtils tests * Use config instead of connection string * Replace conn string builder with mssql config * Connect to master db * Restore connection string validation regex * PR comments, fix error handling * Update main.js * Use try catch for error handling * Fix typo * v2 - Change script action from sqlcmd to mssql query (#99) * Change script action from sqlcmd to mssql query * Update action.yml * Fully qualify Table1 in sql script * Add more debug logging * Clone config before changing db to master * Cleanup * Set TEST_DB name before cleanup * Use runner.temp * Always cleanup * PR comments * Fix database cleanup step in pr-check (#101) * Debug script contents * Fix sed command * Remove debug * v2 - Add support for AAD password, AAD default, and AAD service principal auth (#100) * Use tedious mssql library instead of sqlcmd * Fix mssql connection * Fix SqlUtils tests * Use config instead of connection string * Replace conn string builder with mssql config * Connect to master db * Restore connection string validation regex * AAD auth * Add support for client and tenant IDs * Add more debug messaging * Fix connection string find array * Make client-id and tenant-id action inputs * Fix error handling * More fixes * Use try catch instead * Add tests to pr-check.yml * Change script action from sqlcmd to mssql query * Update action.yml * Fully qualify Table1 in sql script * Add more debug logging * Clone config before changing db to master * Cleanup * Set TEST_DB name before cleanup * Use runner.temp * Always cleanup * Add tests for different auth types * Mask tenant and client IDs * Add AAD password test to pr-check.yml * Fix yaml * Limit max-parallel to 2 * Add test for service principal * PR comments * Fix typo * Cleanup sqlcmd references (#102) * Retry connection with user DB if master connection fails (#104) * Retry with DB connection if master fails * Add tests * Add ConnectionResult interface * Add missing doc comment * PR comments * PR Comments * Download and extract go-sqlcmd before main action (#108) * Add setup script to download go-sqlcmd * Add sqlcmd call to pr-check.yml * Add bz2 specific extract tar * Move setup code to main * Move setup code to main * Fix casing of Setup.ts * Use go-sqlcmd for script action (#113) * call go-sqlcmd for script action * Fix auth options not flowing through * Add test cases * Restore sqlcmd variable in cleanup script * Fix pr-check cleanup * Undo changes to pr-check.yml * Undo pr-check.yml changes * PR comments * Change inputs (#105) * Update SQL API version to 2021-11-01 (#117) * Add support for Script, DeployReport, and DriftReport (#106) * Change inputs * Add other publish like actions * Add tests * Fix test * PR comments * Add mockReturnValue for auth type test * Remove MacOS from ci.yml * links to documentation on each argument type (#123) * v2 - Remove client-id and tenant-id as inputs (#124) * Set tenantId and clientId only when passed in * Default to empty string * Default to empty string * Replace mssql call with go-sqlcmd query * Capture sqlcmd error message * Add more debug * Capture stdout too * Fix config passing * Completely remove client-id and tenant-id * cleanup * Update sqlcmd query * adding connection string format to docs (#138) adding connection string format to docs Co-authored-by: Drew Skwiers-Koballa <dzsquared@users.noreply.github.com>

* Use tedious mssql library instead of sqlcmd * Fix mssql connection * Fix SqlUtils tests * Use config instead of connection string * Replace conn string builder with mssql config * Connect to master db * Restore connection string validation regex * PR comments, fix error handling * Update main.js * Use try catch for error handling * Fix typo

* v2 - Use tedious mssql module instead of sqlcmd (#96) * Use tedious mssql library instead of sqlcmd * Fix mssql connection * Fix SqlUtils tests * Use config instead of connection string * Replace conn string builder with mssql config * Connect to master db * Restore connection string validation regex * PR comments, fix error handling * Update main.js * Use try catch for error handling * Fix typo * Fix path to cleanup.sql

Use tedious mssql library instead of sqlcmd

8f5773a

zijchen requested review from dzsquared and yorek as code owners May 20, 2022 18:57

zijchen had a problem deploying to Automation test May 20, 2022 18:57 Error

zijchen had a problem deploying to Automation test May 20, 2022 18:57 Failure

zijchen marked this pull request as draft May 20, 2022 18:57

Fix mssql connection

ad0198a

zijchen temporarily deployed to Automation test May 23, 2022 20:13 Inactive

Fix SqlUtils tests

499ffe5

zijchen temporarily deployed to Automation test May 24, 2022 20:35 Inactive

zijchen changed the title ~~[WIP] Use tedious mssql library instead of sqlcmd~~ Use tedious mssql library instead of sqlcmd May 24, 2022

zijchen marked this pull request as ready for review May 24, 2022 20:41

zijchen requested a review from udeeshagautam May 24, 2022 20:41

zijchen requested review from kisantia and llali May 24, 2022 20:42

kisantia reviewed May 24, 2022

View reviewed changes

src/SqlUtils.ts Outdated Show resolved Hide resolved

zijchen changed the base branch from master to v2 May 24, 2022 21:48

zijchen changed the title ~~Use tedious mssql library instead of sqlcmd~~ v2 - Use tedious mssql library instead of sqlcmd May 25, 2022

zijchen added 2 commits May 31, 2022 14:14

Use config instead of connection string

deb020e

Replace conn string builder with mssql config

fceb850

zijchen had a problem deploying to Automation test May 31, 2022 23:23 Error

zijchen had a problem deploying to Automation test May 31, 2022 23:23 Failure

Connect to master db

01f886e

zijchen temporarily deployed to Automation test May 31, 2022 23:33 Inactive

zijchen changed the title ~~v2 - Use tedious mssql library instead of sqlcmd~~ v2 - Use tedious mssql module instead of sqlcmd Jun 1, 2022

zijchen requested review from ssreerama and Benjin June 1, 2022 03:10

Restore connection string validation regex

203615f

zijchen temporarily deployed to Automation test June 1, 2022 19:59 Inactive

Benjin reviewed Jun 1, 2022

View reviewed changes

PR comments, fix error handling

a962de8

zijchen had a problem deploying to Automation test June 3, 2022 00:49 Failure

Update main.js

758935f

zijchen temporarily deployed to Automation test June 3, 2022 00:50 Inactive

Use try catch for error handling

211f06c

zijchen temporarily deployed to Automation test June 4, 2022 03:07 Inactive

zijchen requested review from Benjin and kisantia June 6, 2022 15:29

kisantia approved these changes Jun 6, 2022

View reviewed changes

Fix typo

f007b63

zijchen temporarily deployed to Automation test June 6, 2022 21:43 Inactive

zijchen merged commit eb605f8 into v2 Jun 7, 2022

zijchen deleted the zijchen/node-mssql branch June 21, 2022 19:37

zijchen mentioned this pull request Jun 21, 2022

Allow "Database" instead of requiring "Initial Catalog" in connection string #72

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

v2 - Use tedious mssql module instead of sqlcmd #96

v2 - Use tedious mssql module instead of sqlcmd #96

zijchen commented May 20, 2022 •

edited

Loading

zijchen commented May 24, 2022

dzsquared commented May 24, 2022

zijchen commented Jun 1, 2022

Benjin Jun 1, 2022

zijchen Jun 1, 2022

Benjin Jun 1, 2022

zijchen Jun 3, 2022

Benjin Jun 1, 2022

zijchen Jun 3, 2022

Benjin Jun 1, 2022

zijchen Jun 1, 2022

zijchen Jun 3, 2022

kisantia Jun 6, 2022

	const connectionStringParserRegex = /(?<key>[\w\s]+)=(?<val>('[^'](''[^'])')\|("[^"](""[^"])")\|((?!['"])[^;]*))/g
	const connectionStringTester = /^[;\s]([\w\s]+=(?:('[^'](''[^'])')\|("[^"](""[^"])")\|((?!['"])[^;])))(;[;\s]([\w\s]+=(?:('[^'](''[^'])')\|("[^"](""[^"])")\|((?!['"])[^;]))))[;\s]$/

v2 - Use tedious mssql module instead of sqlcmd #96

v2 - Use tedious mssql module instead of sqlcmd #96

Conversation

zijchen commented May 20, 2022 • edited Loading

zijchen commented May 24, 2022

dzsquared commented May 24, 2022

zijchen commented Jun 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zijchen commented May 20, 2022 •

edited

Loading