Merge pull request #1185 from sharelatex/spd-better-unique-filenames

Use numeric suffixes to disambiguate duplicate project names

GitOrigin-RevId: 489b080d0514a33bbbf775095dd587f5e1a254a4

services/web/app/coffee/Features/Project/ProjectDetailsHandler.coffee

@@ -74,13 +74,7 @@ module.exports = ProjectDetailsHandler =
 		if arguments.length is 3 && typeof suffixes is 'function' # make suffixes an optional argument
 			callback = suffixes
 			suffixes = []
-		timestamp = new Date().toISOString().replace(/T(\d+):(\d+):(\d+)\..*/,' $1$2$3') # strip out unwanted characters
-		ProjectDetailsHandler.ensureProjectNameIsUnique user_id, name, suffixes.concat(" (#{timestamp})"), callback
-
-	_addSuffixToProjectName: (name, suffix = '') ->
-		# append the suffix and truncate the project title if needed
-		truncatedLength = ProjectDetailsHandler.MAX_PROJECT_NAME_LENGTH - suffix.length
-		return name.substr(0, truncatedLength) + suffix
+		ProjectDetailsHandler.ensureProjectNameIsUnique user_id, name, suffixes, callback
 
 	# FIXME: we should put a lock around this to make it completely safe, but we would need to do that at
 	# the point of project creation, rather than just checking the name at the start of the import.
@@ -106,8 +100,12 @@ module.exports = ProjectDetailsHandler =
 				candidateName = ProjectDetailsHandler._addSuffixToProjectName(name, suffix)
 				if isUnique(candidateName)
 					return callback(null, candidateName, true)
-			# we couldn't make the name unique, something is wrong
-			return callback new Errors.InvalidNameError("Project name could not be made unique")
+			# if there are no (more) suffixes, use a numeric one
+			uniqueName = ProjectDetailsHandler._addNumericSuffixToProjectName(name, allProjectNames)
+			if uniqueName?
+				callback(null, uniqueName, true)
+			else
+				callback(new Error("Failed to generate a unique name for file: #{name}"))
 	
 	fixProjectName: (name) ->
 		if name == "" || !name
@@ -156,3 +154,30 @@ module.exports = ProjectDetailsHandler =
 				Project.update {_id: project_id}, {$set: {tokens: tokens}}, (err) ->
 					return callback(err) if err?
 					callback(null, tokens)
+
+	_addSuffixToProjectName: (name, suffix = '') ->
+		# append the suffix and truncate the project title if needed
+		truncatedLength = ProjectDetailsHandler.MAX_PROJECT_NAME_LENGTH - suffix.length
+		return name.substr(0, truncatedLength) + suffix
+
+	_addNumericSuffixToProjectName: (name, allProjectNames) ->
+		NUMERIC_SUFFIX_MATCH = / \((\d+)\)$/
+		suffixedName = (basename, number) ->
+			suffix = " (#{number})"
+			return basename.substr(0, ProjectDetailsHandler.MAX_PROJECT_NAME_LENGTH - suffix.length) + suffix
+
+		match = name.match(NUMERIC_SUFFIX_MATCH)
+		basename = name
+		n = 1
+		last = allProjectNames.size + n
+
+		if match?
+			basename = name.replace(NUMERIC_SUFFIX_MATCH, '')
+			n = parseInt(match[1])
+
+		while n <= last
+			candidate = suffixedName(basename, n)
+			return candidate unless allProjectNames.has(candidate)
+			n += 1
+
+		return null

services/web/test/unit/coffee/Project/ProjectDetailsHandlerTests.coffee

@@ -163,12 +163,14 @@ describe 'ProjectDetailsHandler', ->
 	describe "ensureProjectNameIsUnique", ->
 		beforeEach ->
 			@result = {
-				owned: [{_id: 1, name:"name"}, {_id: 2, name: "name1"}, {_id: 3, name: "name11"}]
+				owned: [{_id: 1, name:"name"}, {_id: 2, name: "name1"}, {_id: 3, name: "name11"}, {_id: 100, name: "numeric"}]
 				readAndWrite: [{_id: 4, name:"name2"}, {_id: 5, name:"name22"}]
 				readOnly: [{_id:6, name:"name3"}, {_id:7, name: "name33"}]
 				tokenReadAndWrite: [{_id:8, name:"name4"}, {_id:9, name:"name44"}]
 				tokenReadOnly: [{_id:10, name:"name5"}, {_id:11, name:"name55"}, {_id:12, name:"x".repeat(15)}]
 			}
+			for i in [1..20].concat([30..40])
+				@result.owned.push {_id: 100 + i, name: "numeric (#{i})"}
 			@ProjectGetter.findAllUsersProjects = sinon.stub().callsArgWith(2, null, @result)
 
 		it "should leave a unique name unchanged", (done) ->
@@ -196,9 +198,34 @@ describe 'ProjectDetailsHandler', ->
 				expect(changed).to.equal true
 				done()
 
-		it "should return an error if the name cannot be made unique", (done) ->
-			@handler.ensureProjectNameIsUnique @user_id, "name", ["1", "5", "55"], (error, name, changed) ->
-				expect(error).to.eql new Errors.InvalidNameError("Project name could not be made unique")
+		it "should use a numeric index if no suffix is supplied", (done) ->
+			@handler.ensureProjectNameIsUnique @user_id, "name1", [], (error, name, changed) ->
+				expect(name).to.equal "name1 (1)"
+				expect(changed).to.equal true
+				done()
+
+		it "should use a numeric index if all suffixes are exhausted", (done) ->
+			@handler.ensureProjectNameIsUnique @user_id, "name", ["1", "11"], (error, name, changed) ->
+				expect(name).to.equal "name (1)"
+				expect(changed).to.equal true
+				done()
+
+		it "should find the next lowest available numeric index for the base name", (done) ->
+			@handler.ensureProjectNameIsUnique @user_id, "numeric", [], (error, name, changed) ->
+				expect(name).to.equal "numeric (21)"
+				expect(changed).to.equal true
+				done()
+
+		it "should find the next available numeric index when a numeric index is already present", (done) ->
+			@handler.ensureProjectNameIsUnique @user_id, "numeric (5)", [], (error, name, changed) ->
+				expect(name).to.equal "numeric (21)"
+				expect(changed).to.equal true
+				done()
+
+		it "should not find a numeric index lower than the one already present", (done) ->
+			@handler.ensureProjectNameIsUnique @user_id, "numeric (31)", [], (error, name, changed) ->
+				expect(name).to.equal "numeric (41)"
+				expect(changed).to.equal true
 				done()
 
 	describe "fixProjectName", ->