From 05801085b0dc3d55cb23a6f671bd604266c2869a Mon Sep 17 00:00:00 2001 From: Simon Detheridge Date: Mon, 26 Nov 2018 16:14:49 +0000 Subject: [PATCH] Merge pull request #1185 from sharelatex/spd-better-unique-filenames Use numeric suffixes to disambiguate duplicate project names GitOrigin-RevId: 489b080d0514a33bbbf775095dd587f5e1a254a4 --- .../Project/ProjectDetailsHandler.coffee | 43 +++++++++++++++---- .../Project/ProjectDetailsHandlerTests.coffee | 35 +++++++++++++-- 2 files changed, 65 insertions(+), 13 deletions(-) diff --git a/services/web/app/coffee/Features/Project/ProjectDetailsHandler.coffee b/services/web/app/coffee/Features/Project/ProjectDetailsHandler.coffee index dedc92e273..240980907a 100644 --- a/services/web/app/coffee/Features/Project/ProjectDetailsHandler.coffee +++ b/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 \ No newline at end of file diff --git a/services/web/test/unit/coffee/Project/ProjectDetailsHandlerTests.coffee b/services/web/test/unit/coffee/Project/ProjectDetailsHandlerTests.coffee index 18d73ba77e..116703fa4d 100644 --- a/services/web/test/unit/coffee/Project/ProjectDetailsHandlerTests.coffee +++ b/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", ->