From edc2c5e2002055dfbfedf009242a865d51deaa16 Mon Sep 17 00:00:00 2001 From: Benjin Dubishar Date: Wed, 29 Mar 2023 14:23:03 -0700 Subject: [PATCH] Surfacing better error messages about SQLCMD var names (#22509) * Surfacing better error messages about SQLCMD var names * correcting docstring * adding space to join char --- .../src/common/constants.ts | 5 ++- .../sql-database-projects/src/common/utils.ts | 19 +++++----- .../src/dialogs/addDatabaseReferenceDialog.ts | 8 ++-- .../dialogs/addDatabaseReferenceQuickpick.ts | 6 +-- .../src/test/utils.test.ts | 37 +++++++++---------- 5 files changed, 37 insertions(+), 38 deletions(-) diff --git a/extensions/sql-database-projects/src/common/constants.ts b/extensions/sql-database-projects/src/common/constants.ts index 90b70f99f5..d6a1a51f5d 100644 --- a/extensions/sql-database-projects/src/common/constants.ts +++ b/extensions/sql-database-projects/src/common/constants.ts @@ -123,6 +123,7 @@ export function updatedToSdkStyleError(projectName: string) { return localize('u export const enterNewName = localize('enterNewName', "Enter new name"); //#endregion +export const illegalSqlCmdChars = ['$', '@', '#', '"', '\'', '-']; export const reservedProjectFolders = ['Properties', 'SQLCMD Variables', 'Database References']; //#region Publish dialog strings @@ -393,6 +394,7 @@ export const prePostDeployCount = localize('prePostDeployCount', "To successfull export const invalidProjectReload = localize('invalidProjectReload', "Cannot access provided database project. Only valid, open database projects can be reloaded."); export const externalStreamingJobValidationPassed = localize('externalStreamingJobValidationPassed', "Validation of external streaming job passed."); export const errorRetrievingBuildFiles = localize('errorRetrievingBuildFiles', "Could not build project. Error retrieving files needed to build."); + export function projectAlreadyOpened(path: string) { return localize('projectAlreadyOpened', "Project '{0}' is already opened.", path); } export function projectAlreadyExists(name: string, path: string) { return localize('projectAlreadyExists', "A project named {0} already exists in {1}.", name, path); } export function noFileExist(fileName: string) { return localize('noFileExist', "File {0} doesn't exist", fileName); } @@ -412,7 +414,6 @@ export function unexpectedProjectContext(uri: string) { return localize('unexpec export function unableToPerformAction(action: string, uri: string) { return localize('unableToPerformAction', "Unable to locate '{0}' target: '{1}'", action, uri); } export function unableToFindObject(path: string, objType: string) { return localize('unableToFindFile', "Unable to find {1} with path '{0}'", path, objType); } export function deployScriptExists(scriptType: string) { return localize('deployScriptExists', "A {0} script already exists. The new script will not be included in build.", scriptType); } -export function notValidVariableName(name: string) { return localize('notValidVariableName', "The variable name '{0}' is not valid.", name); } export function cantAddCircularProjectReference(project: string) { return localize('cantAddCircularProjectReference', "A reference to project '{0}' cannot be added. Adding this project as a reference would cause a circular dependency", project); } export function unableToFindSqlCmdVariable(variableName: string) { return localize('unableToFindSqlCmdVariable', "Unable to find SQLCMD variable '{0}'", variableName); } export function unableToFindDatabaseReference(reference: string) { return localize('unableToFindReference', "Unable to find database reference {0}", reference); } @@ -421,6 +422,8 @@ export function invalidTargetPlatform(targetPlatform: string, supportedTargetPla export function errorReadingProject(section: string, path: string, error?: string) { return localize('errorReadingProjectGuid', "Error trying to read {0} of project '{1}'. {2}", section, path, error); } export function errorAddingDatabaseReference(referenceName: string, error: string) { return localize('errorAddingDatabaseReference', "Error adding database reference to {0}. Error: {1}", referenceName, error); } export function errorNotSupportedInVsCode(actionDescription: string) { return localize('errorNotSupportedInVsCode', "Error: {0} is not currently supported in SQL Database Projects for VS Code.", actionDescription); } +export function sqlcmdVariableNameCannotContainWhitespace(name: string) { return localize('sqlcmdVariableNameCannotBeWhitespace', "SQLCMD variable name '{0}' cannot contain whitespace", name); } +export function sqlcmdVariableNameCannotContainIllegalChars(name: string) { return localize('sqlcmdVariableNameCannotContainIllegalChars', "SQLCMD variable name '{0}' cannot contain any of the following characters: {1}", name, illegalSqlCmdChars.join(', ')); } //#endregion diff --git a/extensions/sql-database-projects/src/common/utils.ts b/extensions/sql-database-projects/src/common/utils.ts index ed82085d3a..aa3e626a3e 100644 --- a/extensions/sql-database-projects/src/common/utils.ts +++ b/extensions/sql-database-projects/src/common/utils.ts @@ -242,25 +242,24 @@ export function formatSqlCmdVariable(name: string): string { * Checks if it's a valid sqlcmd variable name * https://docs.microsoft.com/en-us/sql/ssms/scripting/sqlcmd-use-with-scripting-variables?redirectedfrom=MSDN&view=sql-server-ver15#guidelines-for-scripting-variable-names-and-values * @param name variable name to validate - */ -export function isValidSqlCmdVariableName(name: string | undefined): boolean { + * @returns null if valid, otherwise an error message describing why input is invalid +*/ +export function validateSqlCmdVariableName(name: string | undefined): string | null { // remove $() around named if it's there - name = removeSqlCmdVariableFormatting(name); + const cleanedName = removeSqlCmdVariableFormatting(name); // can't contain whitespace - if (!name || name.trim() === '' || name.includes(' ')) { - return false; + if (!cleanedName || cleanedName.trim() === '' || cleanedName.includes(' ')) { + return constants.sqlcmdVariableNameCannotContainWhitespace(name ?? ''); } // can't contain these characters - if (name.includes('$') || name.includes('@') || name.includes('#') || name.includes('"') || name.includes('\'') || name.includes('-')) { - return false; + if (constants.illegalSqlCmdChars.some(c => cleanedName?.includes(c))) { + return constants.sqlcmdVariableNameCannotContainIllegalChars(name ?? ''); } // TODO: tsql parsing to check if it's a reserved keyword or invalid tsql https://github.com/microsoft/azuredatastudio/issues/12204 - // TODO: give more detail why variable name was invalid https://github.com/microsoft/azuredatastudio/issues/12231 - - return true; + return null; } /** diff --git a/extensions/sql-database-projects/src/dialogs/addDatabaseReferenceDialog.ts b/extensions/sql-database-projects/src/dialogs/addDatabaseReferenceDialog.ts index b04ad7d678..b80f505bdf 100644 --- a/extensions/sql-database-projects/src/dialogs/addDatabaseReferenceDialog.ts +++ b/extensions/sql-database-projects/src/dialogs/addDatabaseReferenceDialog.ts @@ -564,9 +564,7 @@ export class AddDatabaseReferenceDialog { // check for invalid variables if (!this.validSqlCmdVariables()) { - let invalidName = !utils.isValidSqlCmdVariableName(this.databaseVariableTextbox?.value) ? this.databaseVariableTextbox!.value! : this.serverVariableTextbox!.value!; - invalidName = utils.removeSqlCmdVariableFormatting(invalidName); - newText = constants.notValidVariableName(invalidName); + newText = utils.validateSqlCmdVariableName(this.databaseVariableTextbox?.value) ?? utils.validateSqlCmdVariableName(this.serverVariableTextbox!.value!)!; } this.exampleUsage!.value = newText; @@ -574,8 +572,8 @@ export class AddDatabaseReferenceDialog { } private validSqlCmdVariables(): boolean { - if (this.databaseVariableTextbox?.enabled && this.databaseVariableTextbox?.value && !utils.isValidSqlCmdVariableName(this.databaseVariableTextbox?.value) - || this.serverVariableTextbox?.enabled && this.serverVariableTextbox?.value && !utils.isValidSqlCmdVariableName(this.serverVariableTextbox?.value)) { + if (this.databaseVariableTextbox?.enabled && this.databaseVariableTextbox?.value && utils.validateSqlCmdVariableName(this.databaseVariableTextbox?.value) + || this.serverVariableTextbox?.enabled && this.serverVariableTextbox?.value && utils.validateSqlCmdVariableName(this.serverVariableTextbox?.value)) { return false; } diff --git a/extensions/sql-database-projects/src/dialogs/addDatabaseReferenceQuickpick.ts b/extensions/sql-database-projects/src/dialogs/addDatabaseReferenceQuickpick.ts index 38ec9acf09..09f00de4cb 100644 --- a/extensions/sql-database-projects/src/dialogs/addDatabaseReferenceQuickpick.ts +++ b/extensions/sql-database-projects/src/dialogs/addDatabaseReferenceQuickpick.ts @@ -6,7 +6,7 @@ import path = require('path'); import * as vscode from 'vscode'; import * as constants from '../common/constants'; -import { getSqlProjectsInWorkspace, isValidSqlCmdVariableName } from '../common/utils'; +import { getSqlProjectsInWorkspace, validateSqlCmdVariableName } from '../common/utils'; import { DbServerValues, populateResultWithVars } from './utils'; import { AddDatabaseReferenceSettings } from '../controllers/projectController'; import { IDacpacReferenceSettings, IProjectReferenceSettings, ISystemDatabaseReferenceSettings } from '../models/IDatabaseReferenceSettings'; @@ -211,7 +211,7 @@ async function promptDbVar(defaultValue: string): Promise { title: constants.databaseVariable, value: defaultValue, validateInput: (value: string) => { - return isValidSqlCmdVariableName(value) ? '' : constants.notValidVariableName(value); + return validateSqlCmdVariableName(value) ?? ''; }, ignoreFocusOut: true }) ?? ''; @@ -235,7 +235,7 @@ async function promptServerVar(): Promise { title: constants.serverVariable, value: constants.otherSeverVariable, validateInput: (value: string) => { - return isValidSqlCmdVariableName(value) ? '' : constants.notValidVariableName(value); + return validateSqlCmdVariableName(value) ?? ''; }, ignoreFocusOut: true }) ?? ''; diff --git a/extensions/sql-database-projects/src/test/utils.test.ts b/extensions/sql-database-projects/src/test/utils.test.ts index 92e12f04b6..66551185da 100644 --- a/extensions/sql-database-projects/src/test/utils.test.ts +++ b/extensions/sql-database-projects/src/test/utils.test.ts @@ -58,29 +58,28 @@ describe('Tests to verify utils functions', function (): void { it('Should determine invalid sqlcmd variable names', () => { // valid names - should(utils.isValidSqlCmdVariableName('$(test)')).equal(true); - should(utils.isValidSqlCmdVariableName('$(test )')).equal(true, 'trailing spaces should be valid because they will be trimmed'); - should(utils.isValidSqlCmdVariableName('test')).equal(true); - should(utils.isValidSqlCmdVariableName('test ')).equal(true, 'trailing spaces should be valid because they will be trimmed'); - should(utils.isValidSqlCmdVariableName('$(test')).equal(true); - should(utils.isValidSqlCmdVariableName('$(test ')).equal(true, 'trailing spaces should be valid because they will be trimmed'); + should(utils.validateSqlCmdVariableName('$(test)')).equal(null); + should(utils.validateSqlCmdVariableName('$(test )')).equal(null, 'trailing spaces should be valid because they will be trimmed'); + should(utils.validateSqlCmdVariableName('test')).equal(null); + should(utils.validateSqlCmdVariableName('test ')).equal(null, 'trailing spaces should be valid because they will be trimmed'); + should(utils.validateSqlCmdVariableName('$(test')).equal(null); + should(utils.validateSqlCmdVariableName('$(test ')).equal(null, 'trailing spaces should be valid because they will be trimmed'); // whitespace - should(utils.isValidSqlCmdVariableName('')).equal(false); - should(utils.isValidSqlCmdVariableName(' ')).equal(false); - should(utils.isValidSqlCmdVariableName(' ')).equal(false); - should(utils.isValidSqlCmdVariableName('test abc')).equal(false); - should(utils.isValidSqlCmdVariableName(' ')).equal(false); + should(utils.validateSqlCmdVariableName('')).equal(constants.sqlcmdVariableNameCannotContainWhitespace('')); + should(utils.validateSqlCmdVariableName(' ')).equal(constants.sqlcmdVariableNameCannotContainWhitespace(' ')); + should(utils.validateSqlCmdVariableName(' ')).equal(constants.sqlcmdVariableNameCannotContainWhitespace(' ')); + should(utils.validateSqlCmdVariableName('test abc')).equal(constants.sqlcmdVariableNameCannotContainWhitespace('test abc')); + should(utils.validateSqlCmdVariableName(' ')).equal(constants.sqlcmdVariableNameCannotContainWhitespace(' ')); // invalid characters - should(utils.isValidSqlCmdVariableName('$($test')).equal(false); - should(utils.isValidSqlCmdVariableName('$test')).equal(false); - should(utils.isValidSqlCmdVariableName('$test')).equal(false); - should(utils.isValidSqlCmdVariableName('test@')).equal(false); - should(utils.isValidSqlCmdVariableName('test#')).equal(false); - should(utils.isValidSqlCmdVariableName('test"')).equal(false); - should(utils.isValidSqlCmdVariableName('test\'')).equal(false); - should(utils.isValidSqlCmdVariableName('test-1')).equal(false); + should(utils.validateSqlCmdVariableName('$($test')).equal(constants.sqlcmdVariableNameCannotContainIllegalChars('$($test')); + should(utils.validateSqlCmdVariableName('$test')).equal(constants.sqlcmdVariableNameCannotContainIllegalChars('$test')); + should(utils.validateSqlCmdVariableName('test@')).equal(constants.sqlcmdVariableNameCannotContainIllegalChars('test@')); + should(utils.validateSqlCmdVariableName('test#')).equal(constants.sqlcmdVariableNameCannotContainIllegalChars('test#')); + should(utils.validateSqlCmdVariableName('test"')).equal(constants.sqlcmdVariableNameCannotContainIllegalChars('test"')); + should(utils.validateSqlCmdVariableName('test\'')).equal(constants.sqlcmdVariableNameCannotContainIllegalChars('test\'')); + should(utils.validateSqlCmdVariableName('test-1')).equal(constants.sqlcmdVariableNameCannotContainIllegalChars('test-1')); }); it('Should convert from milliseconds to hr min sec correctly', () => {