Show error when trying to add file/folder that already exists to project (#11122)

* show error when trying to create file or folder that already exists

* add test for file that already exists

* add folder tests

* fix error messages

* hide Properties folder

* update comment

extensions/sql-database-projects/src/common/constants.ts

@@ -107,6 +107,8 @@ export function projectAlreadyOpened(path: string) { return localize('projectAlr
 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); }
 export function cannotResolvePath(path: string) { return localize('cannotResolvePath', "Cannot resolve path {0}", path); }
+export function fileAlreadyExists(filename: string) { return localize('fileAlreadyExists', "A file with the name '{0}' already exists on disk at this location. Please choose another name.", filename); }
+export function folderAlreadyExists(filename: string) { return localize('folderAlreadyExists', "A folder with the name '{0}' already exists on disk at this location. Please choose another name.", filename); }
 export function invalidInput(input: string) { return localize('invalidInput', "Invalid input: {0}", input); }
 
 export function mssqlNotFound(mssqlConfigDir: string) { return localize('mssqlNotFound', "Could not get mssql extension's install location at {0}", mssqlConfigDir); }
@@ -153,6 +155,7 @@ export const ArtifactReference = 'ArtifactReference';
 export const SuppressMissingDependenciesErrors = 'SuppressMissingDependenciesErrors';
 export const DatabaseVariableLiteralValue = 'DatabaseVariableLiteralValue';
 export const DSP = 'DSP';
+export const Properties = 'Properties';
 
 // SqlProj File targets
 export const NetCoreTargets = '$(NETCoreTargetsPath)\\Microsoft.Data.Tools.Schema.SqlTasks.targets';

extensions/sql-database-projects/src/controllers/projectController.ts

@@ -18,7 +18,7 @@ import { IConnectionProfile, TaskExecutionMode } from 'azdata';
 import { promises as fs } from 'fs';
 import { ApiWrapper } from '../common/apiWrapper';
 import { DeployDatabaseDialog } from '../dialogs/deployDatabaseDialog';
-import { Project, DatabaseReferenceLocation, SystemDatabase, TargetPlatform, ProjectEntry } from '../models/project';
+import { Project, DatabaseReferenceLocation, SystemDatabase, TargetPlatform, ProjectEntry, reservedProjectFolders } from '../models/project';
 import { SqlDatabaseProjectTreeViewProvider } from './databaseProjectTreeViewProvider';
 import { FolderNode, FileNode } from '../models/tree/fileFolderTreeItem';
 import { IDeploymentProfile, IGenerateScriptProfile, PublishSettings } from '../models/IDeploymentProfile';
@@ -280,9 +280,26 @@ export class ProjectsController {
 
 		const relativeFolderPath = path.join(this.getRelativePath(treeNode), newFolderName);
 
-		await project.addFolderItem(relativeFolderPath);
+		try {
+			// check if folder already exists or is a reserved folder
+			const absoluteFolderPath = path.join(project.projectFolderPath, relativeFolderPath);
+			const folderExists = await utils.exists(absoluteFolderPath);
 
+			if (folderExists || this.isReservedFolder(absoluteFolderPath, project.projectFolderPath)) {
+				throw new Error(constants.folderAlreadyExists(path.parse(absoluteFolderPath).name));
+			}
+
+			await project.addFolderItem(relativeFolderPath);
 			this.refreshProjectsTree();
+		} catch (err) {
+			this.apiWrapper.showErrorMessage(utils.getErrorMessage(err));
+		}
+	}
+
+	public isReservedFolder(absoluteFolderPath: string, projectFolderPath: string): boolean {
+		const sameName = reservedProjectFolders.find(f => f === path.parse(absoluteFolderPath).name) !== undefined;
+		const sameLocation = path.parse(absoluteFolderPath).dir === projectFolderPath;
+		return sameName && sameLocation;
 	}
 
 	public async addItemPromptFromNode(treeNode: BaseProjectTreeItem, itemTypeName?: string) {
@@ -315,16 +332,26 @@ export class ProjectsController {
 			return; // user cancelled
 		}
 
-		// TODO: file already exists?
-
 		const newFileText = this.macroExpansion(itemType.templateScript, { 'OBJECT_NAME': itemObjectName });
 		const relativeFilePath = path.join(relativePath, itemObjectName + constants.sqlFileExtension);
 
+		try {
+			// check if file already exists
+			const absoluteFilePath = path.join(project.projectFolderPath, relativeFilePath);
+			const fileExists = await utils.exists(absoluteFilePath);
+
+			if (fileExists) {
+				throw new Error(constants.fileAlreadyExists(path.parse(absoluteFilePath).name));
+			}
+
 			const newEntry = await project.addScriptItem(relativeFilePath, newFileText);
 
 			this.apiWrapper.executeCommand('vscode.open', newEntry.fsUri);
 
 			this.refreshProjectsTree();
+		} catch (err) {
+			this.apiWrapper.showErrorMessage(utils.getErrorMessage(err));
+		}
 	}
 
 	public async exclude(context: FileNode | FolderNode): Promise<void> {

extensions/sql-database-projects/src/models/project.ts

@@ -48,18 +48,24 @@ export class Project {
 		for (let ig = 0; ig < this.projFileXmlDoc.documentElement.getElementsByTagName(constants.ItemGroup).length; ig++) {
 			const itemGroup = this.projFileXmlDoc.documentElement.getElementsByTagName(constants.ItemGroup)[ig];
 
-			for (let b = 0; b < itemGroup.getElementsByTagName(constants.Build).length; b++) {
+			const buildElements = itemGroup.getElementsByTagName(constants.Build);
-				this.files.push(this.createProjectEntry(itemGroup.getElementsByTagName(constants.Build)[b].getAttribute(constants.Include), EntryType.File));
+			for (let b = 0; b < buildElements.length; b++) {
+				this.files.push(this.createProjectEntry(buildElements[b].getAttribute(constants.Include), EntryType.File));
 			}
 
-			for (let f = 0; f < itemGroup.getElementsByTagName(constants.Folder).length; f++) {
+			const folderElements = itemGroup.getElementsByTagName(constants.Folder);
-				this.files.push(this.createProjectEntry(itemGroup.getElementsByTagName(constants.Folder)[f].getAttribute(constants.Include), EntryType.Folder));
+			for (let f = 0; f < folderElements.length; f++) {
+				// don't add Properties folder since it isn't supported for now
+				if (folderElements[f].getAttribute(constants.Include) !== constants.Properties) {
+					this.files.push(this.createProjectEntry(folderElements[f].getAttribute(constants.Include), EntryType.Folder));
+				}
 			}
 		}
 
 		// find all import statements to include
-		for (let i = 0; i < this.projFileXmlDoc.documentElement.getElementsByTagName(constants.Import).length; i++) {
+		const importElements = this.projFileXmlDoc.documentElement.getElementsByTagName(constants.Import);
-			const importTarget = this.projFileXmlDoc.documentElement.getElementsByTagName(constants.Import)[i];
+		for (let i = 0; i < importElements.length; i++) {
+			const importTarget = importElements[i];
 			this.importedTargets.push(importTarget.getAttribute(constants.Project));
 		}
 
@@ -589,3 +595,5 @@ export enum SystemDatabase {
 	master,
 	msdb
 }
+
+export const reservedProjectFolders = ['Properties', 'Data Sources', 'Database References'];

extensions/sql-database-projects/src/test/project.test.ts

@@ -33,7 +33,7 @@ describe('Project: sqlproj content operations', function (): void {
 
 		// Files and folders
 		should(project.files.filter(f => f.type === EntryType.File).length).equal(4);
-		should(project.files.filter(f => f.type === EntryType.Folder).length).equal(5);
+		should(project.files.filter(f => f.type === EntryType.Folder).length).equal(4);
 
 		should(project.files.find(f => f.type === EntryType.Folder && f.relativePath === 'Views\\User')).not.equal(undefined); // mixed ItemGroup folder
 		should(project.files.find(f => f.type === EntryType.File && f.relativePath === 'Views\\User\\Profile.sql')).not.equal(undefined); // mixed ItemGroup file
@@ -81,7 +81,7 @@ describe('Project: sqlproj content operations', function (): void {
 		await project.addToProject(list);
 
 		should(project.files.filter(f => f.type === EntryType.File).length).equal(11);	// txt file shouldn't be added to the project
-		should(project.files.filter(f => f.type === EntryType.Folder).length).equal(3);	// 2folders + default Properties folder
+		should(project.files.filter(f => f.type === EntryType.Folder).length).equal(2);	// 2 folders
 	});
 
 	it('Should throw error while adding Folder and Build entries to sqlproj when a file/folder does not exist on disk', async function (): Promise<void> {

extensions/sql-database-projects/src/test/projectController.test.ts

@@ -18,13 +18,14 @@ import { SqlDatabaseProjectTreeViewProvider } from '../controllers/databaseProje
 import { ProjectsController } from '../controllers/projectController';
 import { promises as fs } from 'fs';
 import { createContext, TestContext, mockDacFxResult } from './testContext';
-import { Project, SystemDatabase, ProjectEntry } from '../models/project';
+import { Project, SystemDatabase, ProjectEntry, reservedProjectFolders } from '../models/project';
 import { DeployDatabaseDialog } from '../dialogs/deployDatabaseDialog';
 import { ApiWrapper } from '../common/apiWrapper';
 import { IDeploymentProfile, IGenerateScriptProfile } from '../models/IDeploymentProfile';
 import { exists } from '../common/utils';
 import { ProjectRootTreeItem } from '../models/tree/projectTreeItem';
 import { FolderNode } from '../models/tree/fileFolderTreeItem';
+import { BaseProjectTreeItem } from '../models/tree/baseTreeItem';
 
 let testContext: TestContext;
 
@@ -77,7 +78,7 @@ describe('ProjectsController: project controller operations', function (): void
 
 			const project = await projController.openProject(vscode.Uri.file(sqlProjPath));
 
-			should(project.files.length).equal(9); // detailed sqlproj tests in their own test file
+			should(project.files.length).equal(8); // detailed sqlproj tests in their own test file
 			should(project.dataSources.length).equal(2); // detailed datasources tests in their own test file
 		});
 
@@ -110,6 +111,75 @@ describe('ProjectsController: project controller operations', function (): void
 			}
 		});
 
+		it('Should show error if trying to add a file that already exists', async function (): Promise<void> {
+			const tableName = 'table1';
+			testContext.apiWrapper.reset();
+			testContext.apiWrapper.setup(x => x.showInputBox(TypeMoq.It.isAny(), TypeMoq.It.isAny())).returns(() => Promise.resolve(tableName));
+			testContext.apiWrapper.setup(x => x.showErrorMessage(TypeMoq.It.isAny())).returns((s) => { throw new Error(s); });
+
+			const projController = new ProjectsController(testContext.apiWrapper.object, new SqlDatabaseProjectTreeViewProvider());
+			const project = await testUtils.createTestProject(baselines.newProjectFileBaseline);
+
+			should(project.files.length).equal(0, 'There should be no files');
+			await projController.addItemPrompt(project, '', templates.script);
+			should(project.files.length).equal(1, 'File should be successfully added');
+			await testUtils.shouldThrowSpecificError(async () => await projController.addItemPrompt(project, '', templates.script), constants.fileAlreadyExists(tableName));
+		});
+
+		it('Should show error if trying to add a folder that already exists', async function (): Promise<void> {
+			const folderName = 'folder1';
+			testContext.apiWrapper.reset();
+			testContext.apiWrapper.setup(x => x.showInputBox(TypeMoq.It.isAny(), TypeMoq.It.isAny())).returns(() => Promise.resolve(folderName));
+			testContext.apiWrapper.setup(x => x.showErrorMessage(TypeMoq.It.isAny())).returns((s) => { throw new Error(s); });
+
+			const projController = new ProjectsController(testContext.apiWrapper.object, new SqlDatabaseProjectTreeViewProvider());
+			const project = await testUtils.createTestProject(baselines.newProjectFileBaseline);
+			const projectRoot = new ProjectRootTreeItem(project);
+
+			should(project.files.length).equal(0, 'There should be no other folders');
+			await projController.addFolderPrompt(projectRoot);
+			should(project.files.length).equal(1, 'Folder should be successfully added');
+			projController.refreshProjectsTree();
+
+			await verifyFolderNotAdded(folderName, projController, project, projectRoot);
+
+			// reserved folder names
+			for (let i in reservedProjectFolders) {
+				await verifyFolderNotAdded(reservedProjectFolders[i], projController, project, projectRoot);
+			}
+		});
+
+		it('Should be able to add folder with reserved name as long as not at project root', async function (): Promise<void> {
+			const folderName = 'folder1';
+			testContext.apiWrapper.reset();
+			testContext.apiWrapper.setup(x => x.showInputBox(TypeMoq.It.isAny(), TypeMoq.It.isAny())).returns(() => Promise.resolve(folderName));
+			testContext.apiWrapper.setup(x => x.showErrorMessage(TypeMoq.It.isAny())).returns((s) => { throw new Error(s); });
+
+			const projController = new ProjectsController(testContext.apiWrapper.object, new SqlDatabaseProjectTreeViewProvider());
+			const project = await testUtils.createTestProject(baselines.openProjectFileBaseline);
+			const projectRoot = new ProjectRootTreeItem(project);
+
+			// make sure it's ok to add these folders if they aren't where the reserved folders are at the root of the project
+			let node = projectRoot.children.find(c => c.friendlyName === 'Tables');
+			for (let i in reservedProjectFolders) {
+				await verfiyFolderAdded(reservedProjectFolders[i], projController, project, <BaseProjectTreeItem>node);
+			}
+		});
+
+		async function verfiyFolderAdded(folderName: string, projController: ProjectsController, project: Project, node: BaseProjectTreeItem): Promise<void> {
+			const beforeFileCount = project.files.length;
+			testContext.apiWrapper.setup(x => x.showInputBox(TypeMoq.It.isAny(), TypeMoq.It.isAny())).returns(() => Promise.resolve(folderName));
+			await projController.addFolderPrompt(node);
+			should(project.files.length).equal(beforeFileCount + 1, `File count should be increased by one after adding the folder ${folderName}`);
+		}
+
+		async function verifyFolderNotAdded(folderName: string, projController: ProjectsController, project: Project, node: BaseProjectTreeItem): Promise<void> {
+			const beforeFileCount = project.files.length;
+			testContext.apiWrapper.setup(x => x.showInputBox(TypeMoq.It.isAny(), TypeMoq.It.isAny())).returns(() => Promise.resolve(folderName));
+			await testUtils.shouldThrowSpecificError(async () => await projController.addFolderPrompt(node), constants.folderAlreadyExists(folderName));
+			should(project.files.length).equal(beforeFileCount, 'File count should be the same as before the folder was attempted to be added');
+		}
+
 		it('Should delete nested ProjectEntry from node', async function (): Promise<void> {
 			let proj = await testUtils.createTestProject(templates.newSqlProjectTemplate);
 			const setupResult = await setupDeleteExcludeTest(proj);
@@ -123,8 +193,8 @@ describe('ProjectsController: project controller operations', function (): void
 			await proj.readProjFile(); // reload edited sqlproj from disk
 
 			// confirm result
-			should(proj.files.length).equal(2, 'number of file/folder entries'); // lowerEntry and the contained scripts should be deleted
+			should(proj.files.length).equal(1, 'number of file/folder entries'); // lowerEntry and the contained scripts should be deleted
-			should(proj.files[1].relativePath).equal('UpperFolder');
+			should(proj.files[0].relativePath).equal('UpperFolder');
 
 			should(await exists(scriptEntry.fsUri.fsPath)).equal(false, 'script is supposed to be deleted');
 		});
@@ -142,8 +212,8 @@ describe('ProjectsController: project controller operations', function (): void
 			await proj.readProjFile(); // reload edited sqlproj from disk
 
 			// confirm result
-			should(proj.files.length).equal(2, 'number of file/folder entries'); // LowerFolder and the contained scripts should be deleted
+			should(proj.files.length).equal(1, 'number of file/folder entries'); // LowerFolder and the contained scripts should be deleted
-			should(proj.files[1].relativePath).equal('UpperFolder'); // UpperFolder should still be there
+			should(proj.files[0].relativePath).equal('UpperFolder'); // UpperFolder should still be there
 
 			should(await exists(scriptEntry.fsUri.fsPath)).equal(true, 'script is supposed to still exist on disk');
 		});
@@ -456,7 +526,7 @@ async function setupDeleteExcludeTest(proj: Project): Promise<[ProjectEntry, Pro
 	testContext.apiWrapper.setup(x => x.showWarningMessageOptions(TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny())).returns(() => Promise.resolve(constants.yesString));
 
 	// confirm setup
-	should(proj.files.length).equal(5, 'number of file/folder entries');
+	should(proj.files.length).equal(4, 'number of file/folder entries');
 	should(path.parse(scriptEntry.fsUri.fsPath).base).equal('someScript.sql');
 	should((await fs.readFile(scriptEntry.fsUri.fsPath)).toString()).equal('not a real script');