537208: Task data filename can get too long

* moved file-related operations to a separate class
* file name is only encoded if required
* file name is trimmed to stay below 255 characters
* encoded file name always used if file already exists
* added unit tests for file-related methods

Change-Id: I8b536b3a3df8168b997aa9be1ec82b9f7e314dfd
Task-Url: https://bugs.eclipse.org/bugs/show_bug.cgi?id=537208
Signed-off-by: alexei.trebounskikh <alexei.trebounskikh@tasktop.com>
(cherry picked from commit f422548545a931cd4ab2d7d3ba9068607828d902)

3 files changed, 228 insertions, 58 deletions

diff --git a/org.eclipse.mylyn.tasks.core/src/org/eclipse/mylyn/internal/tasks/core/data/TaskDataFileManager.java b/org.eclipse.mylyn.tasks.core/src/org/eclipse/mylyn/internal/tasks/core/data/TaskDataFileManager.java
new file mode 100644
index 000000000..3b8fcc7ef
--- /dev/null
+++ b/org.eclipse.mylyn.tasks.core/src/org/eclipse/mylyn/internal/tasks/core/data/TaskDataFileManager.java
@@ -0,0 +1,128 @@
+/*******************************************************************************
+ * Copyright (c) 2018 Tasktop Technologies and others.
+ * All rights reserved. This program and the accompanying materials
+ * are made available under the terms of the Eclipse Public License v1.0
+ * which accompanies this distribution, and is available at
+ * http://www.eclipse.org/legal/epl-v10.html
+ *
+ * Contributors:
+ *     Tasktop Technologies - initial API and implementation
+ *******************************************************************************/
+
+package org.eclipse.mylyn.internal.tasks.core.data;
+
+import java.io.File;
+import java.io.UnsupportedEncodingException;
+import java.net.URLEncoder;
+import java.util.function.Predicate;
+
+import org.eclipse.core.runtime.Assert;
+import org.eclipse.mylyn.commons.core.CoreUtil;
+import org.eclipse.mylyn.tasks.core.ITask;
+
+/**
+ * Encapsulates file-related operations of TaskDataManager
+ */
+public class TaskDataFileManager {
+
+	private static final String ENCODING_UTF_8 = "UTF-8"; //$NON-NLS-1$
+
+	private static final String EXTENSION = ".zip"; //$NON-NLS-1$
+
+	private static final String FOLDER_TASKS = "tasks"; //$NON-NLS-1$
+
+	private static final String FOLDER_DATA = "offline"; //$NON-NLS-1$
+
+	private static final String FOLDER_TASKS_1_0 = "offline"; //$NON-NLS-1$
+
+	private static final int FILENAME_MAX_LEN = 255 - EXTENSION.length(); // 255 is an OS limit for file name
+
+	private String dataPath;
+
+	public String getDataPath() {
+		return dataPath;
+	}
+
+	public void setDataPath(String dataPath) {
+		this.dataPath = dataPath;
+	}
+
+	public File getFile(ITask task, String kind) {
+		return getFile(task.getRepositoryUrl(), task, kind);
+	}
+
+	public File getFile(String repositoryUrl, ITask task, String kind) {
+		File path = getDirectory(repositoryUrl, task);
+		String fileName = getFileName(task, path);
+		return new File(path, fileName + EXTENSION);
+	}
+
+	private File getDirectory(String repositoryUrl, ITask task) {
+		Assert.isNotNull(dataPath);
+		String repositoryPath = task.getConnectorKind() + "-" + CoreUtil.asFileName(repositoryUrl); //$NON-NLS-1$
+		return new File(dataPath + File.separator + FOLDER_TASKS + File.separator + repositoryPath + File.separator
+				+ FOLDER_DATA);
+	}
+
+	private String getFileName(ITask task, File path) {
+		return getFileName(task, filename -> new File(path, filename + EXTENSION).exists());
+	}
+
+	// the method is made protected for unit testing
+	protected String getFileName(ITask task, Predicate<String> fileExists) {
+		String encodedFileName = CoreUtil.asFileName(task.getTaskId());
+
+		// for backwards-compatibility with versions that always encoded file names,
+		// we will use an encoded name if the file with an encoded name already exists
+		if (fileExists.test(encodedFileName)) {
+			return encodedFileName;
+		}
+
+		// if file with encoded name does not exist, we will only encode file name if it is required
+		String fileName;
+		if (requiresEncoding(task.getTaskId())) {
+			fileName = encodedFileName;
+		} else {
+			fileName = task.getTaskId();
+		}
+
+		// trim the file name if it is too long
+		return trimFilenameIfRequired(fileName);
+	}
+
+	/**
+	 * Checks if input contains characters other than ones returned by {@link CoreUtil.asFileName}
+	 *
+	 * @param fileName
+	 * @return true or false
+	 */
+	private boolean requiresEncoding(String fileName) {
+		return !fileName.matches("^[a-zA-Z0-9%\\.]+$"); //$NON-NLS-1$
+	}
+
+	private String trimFilenameIfRequired(String filename) {
+		if (filename.length() > FILENAME_MAX_LEN) {
+			// replace a long file name with a shorter name + the hash
+			String hashCode = getHashCode(filename);
+			return filename.substring(0, FILENAME_MAX_LEN - hashCode.length() - 1) + "." + hashCode; //$NON-NLS-1$
+		}
+
+		return filename;
+	}
+
+	private String getHashCode(String text) {
+		return Integer.toUnsignedString(text.hashCode());
+	}
+
+	public File getFile10(ITask task, String kind) {
+		try {
+			String pathName = URLEncoder.encode(task.getRepositoryUrl(), ENCODING_UTF_8);
+			String fileName = task.getTaskId() + EXTENSION;
+			File path = new File(dataPath + File.separator + FOLDER_TASKS_1_0, pathName);
+			return new File(path, fileName);
+		} catch (UnsupportedEncodingException e) {
+			throw new RuntimeException(e);
+		}
+
+	}
+}
diff --git a/org.eclipse.mylyn.tasks.core/src/org/eclipse/mylyn/internal/tasks/core/data/TaskDataManager.java b/org.eclipse.mylyn.tasks.core/src/org/eclipse/mylyn/internal/tasks/core/data/TaskDataManager.java
index ed54c59f3..0037ae5fb 100644
--- a/org.eclipse.mylyn.tasks.core/src/org/eclipse/mylyn/internal/tasks/core/data/TaskDataManager.java
+++ b/org.eclipse.mylyn.tasks.core/src/org/eclipse/mylyn/internal/tasks/core/data/TaskDataManager.java
@@ -12,8 +12,6 @@
 package org.eclipse.mylyn.internal.tasks.core.data;
 
 import java.io.File;
-import java.io.UnsupportedEncodingException;
-import java.net.URLEncoder;
 import java.util.Collection;
 import java.util.Date;
 import java.util.List;
@@ -27,7 +25,6 @@ import org.eclipse.core.runtime.ISafeRunnable;
 import org.eclipse.core.runtime.IStatus;
 import org.eclipse.core.runtime.SafeRunner;
 import org.eclipse.core.runtime.Status;
-import org.eclipse.mylyn.commons.core.CoreUtil;
 import org.eclipse.mylyn.commons.core.DelegatingProgressMonitor;
 import org.eclipse.mylyn.commons.core.IDelegatingProgressMonitor;
 import org.eclipse.mylyn.commons.core.StatusHandler;
@@ -56,18 +53,6 @@ import org.eclipse.mylyn.tasks.core.data.TaskData;
  */
 public class TaskDataManager implements ITaskDataManager {
 
-	private static final String ENCODING_UTF_8 = "UTF-8"; //$NON-NLS-1$
-
-	private static final String EXTENSION = ".zip"; //$NON-NLS-1$
-
-	private static final String FOLDER_TASKS = "tasks"; //$NON-NLS-1$
-
-	private static final String FOLDER_DATA = "offline"; //$NON-NLS-1$
-
-	private static final String FOLDER_TASKS_1_0 = "offline"; //$NON-NLS-1$
-
-	private String dataPath;
-
 	private final IRepositoryManager repositoryManager;
 
 	private final TaskDataStore taskDataStore;
@@ -78,15 +63,17 @@ public class TaskDataManager implements ITaskDataManager {
 
 	private final List<ITaskDataManagerListener> listeners = new CopyOnWriteArrayList<ITaskDataManagerListener>();
 
-	private final SynchronizationManger synchronizationManger;
+	private final SynchronizationManger synchronizationManager;
+
+	private final TaskDataFileManager fileManager = new TaskDataFileManager();
 
 	public TaskDataManager(TaskDataStore taskDataStore, IRepositoryManager repositoryManager, TaskList taskList,
-			TaskActivityManager taskActivityManager, SynchronizationManger synchronizationManger) {
+			TaskActivityManager taskActivityManager, SynchronizationManger synchronizationManager) {
 		this.taskDataStore = taskDataStore;
 		this.repositoryManager = repositoryManager;
 		this.taskList = taskList;
 		this.taskActivityManager = taskActivityManager;
-		this.synchronizationManger = synchronizationManger;
+		this.synchronizationManager = synchronizationManager;
 	}
 
 	public void addListener(ITaskDataManagerListener listener) {
@@ -162,7 +149,7 @@ public class TaskDataManager implements ITaskDataManager {
 		final boolean[] changed = new boolean[1];
 		taskList.run(new ITaskListRunnable() {
 			public void execute(IProgressMonitor monitor) throws CoreException {
-				final File file = getFile(task, kind);
+				final File file = fileManager.getFile(task, kind);
 				taskDataStore.putTaskData(ensurePathExists(file), state);
 				switch (task.getSynchronizationState()) {
 				case SYNCHRONIZED:
@@ -227,7 +214,7 @@ public class TaskDataManager implements ITaskDataManager {
 							state = taskDataStore.getTaskDataState(ensurePathExists(file));
 						}
 						TaskData lastReadData = (state != null) ? state.getLastReadData() : null;
-						TaskDataDiff diff = synchronizationManger.createDiff(taskData, lastReadData, monitor);
+						TaskDataDiff diff = synchronizationManager.createDiff(taskData, lastReadData, monitor);
 						suppressIncoming = Boolean.toString(!diff.hasChanged());
 
 						switch (task.getSynchronizationState()) {
@@ -295,9 +282,9 @@ public class TaskDataManager implements ITaskDataManager {
 	private File getMigratedFile(ITask task, String kind) throws CoreException {
 		Assert.isNotNull(task);
 		Assert.isNotNull(kind);
-		File file = getFile(task, kind);
+		File file = fileManager.getFile(task, kind);
 		if (!file.exists()) {
-			File oldFile = getFile10(task, kind);
+			File oldFile = fileManager.getFile10(task, kind);
 			if (oldFile.exists()) {
 				TaskDataState state = taskDataStore.getTaskDataState(oldFile);
 				// save migrated task data right away
@@ -314,7 +301,7 @@ public class TaskDataManager implements ITaskDataManager {
 		final TaskDataManagerEvent event = new TaskDataManagerEvent(this, itask);
 		taskList.run(new ITaskListRunnable() {
 			public void execute(IProgressMonitor monitor) throws CoreException {
-				File dataFile = getFile(task, kind);
+				File dataFile = fileManager.getFile(task, kind);
 				if (dataFile.exists()) {
 					taskDataStore.discardEdits(dataFile);
 				}
@@ -337,42 +324,15 @@ public class TaskDataManager implements ITaskDataManager {
 	}
 
 	private File findFile(ITask task, String kind) {
-		File file = getFile(task, kind);
+		File file = fileManager.getFile(task, kind);
 		if (file.exists()) {
 			return file;
 		}
-		return getFile10(task, kind);
+		return fileManager.getFile10(task, kind);
 	}
 
 	public String getDataPath() {
-		return dataPath;
-	}
-
-	private File getFile(ITask task, String kind) {
-		return getFile(task.getRepositoryUrl(), task, kind);
-	}
-
-	private File getFile(String repositoryUrl, ITask task, String kind) {
-//			String pathName = task.getConnectorKind() + "-"
-//					+ URLEncoder.encode(task.getRepositoryUrl(), ENCODING_UTF_8);
-//			String fileName = kind + "-" + URLEncoder.encode(task.getTaskId(), ENCODING_UTF_8) + EXTENSION;
-		String repositoryPath = task.getConnectorKind() + "-" + CoreUtil.asFileName(repositoryUrl); //$NON-NLS-1$
-		String fileName = CoreUtil.asFileName(task.getTaskId()) + EXTENSION;
-		File path = new File(dataPath + File.separator + FOLDER_TASKS + File.separator + repositoryPath + File.separator
-				+ FOLDER_DATA);
-		return new File(path, fileName);
-	}
-
-	private File getFile10(ITask task, String kind) {
-		try {
-			String pathName = URLEncoder.encode(task.getRepositoryUrl(), ENCODING_UTF_8);
-			String fileName = task.getTaskId() + EXTENSION;
-			File path = new File(dataPath + File.separator + FOLDER_TASKS_1_0, pathName);
-			return new File(path, fileName);
-		} catch (UnsupportedEncodingException e) {
-			throw new RuntimeException(e);
-		}
-
+		return fileManager.getDataPath();
 	}
 
 	public TaskData getTaskData(ITask task) throws CoreException {
@@ -446,7 +406,7 @@ public class TaskDataManager implements ITaskDataManager {
 		final AbstractTask task = (AbstractTask) itask;
 		taskList.run(new ITaskListRunnable() {
 			public void execute(IProgressMonitor monitor) throws CoreException {
-				File file = getFile(task, task.getConnectorKind());
+				File file = fileManager.getFile(task, task.getConnectorKind());
 				if (file.exists()) {
 					taskDataStore.deleteTaskData(file);
 					task.setSynchronizationState(SynchronizationState.SYNCHRONIZED);
@@ -457,7 +417,7 @@ public class TaskDataManager implements ITaskDataManager {
 	}
 
 	public void setDataPath(String dataPath) {
-		this.dataPath = dataPath;
+		fileManager.setDataPath(dataPath);
 	}
 
 	/**
@@ -520,7 +480,7 @@ public class TaskDataManager implements ITaskDataManager {
 		final boolean[] changed = new boolean[1];
 		taskList.run(new ITaskListRunnable() {
 			public void execute(IProgressMonitor monitor) throws CoreException {
-				taskDataStore.putEdits(getFile(task, kind), editsData);
+				taskDataStore.putEdits(fileManager.getFile(task, kind), editsData);
 				switch (task.getSynchronizationState()) {
 				case INCOMING:
 				case INCOMING_NEW:
@@ -589,7 +549,7 @@ public class TaskDataManager implements ITaskDataManager {
 				if (file.exists()) {
 					TaskDataState oldState = taskDataStore.getTaskDataState(file);
 					if (oldState != null) {
-						File newFile = getFile(newStorageRepositoryUrl, task, kind);
+						File newFile = fileManager.getFile(newStorageRepositoryUrl, task, kind);
 						TaskDataState newState = new TaskDataState(oldState.getConnectorKind(), newRepositoryUrl,
 								oldState.getTaskId());
 						newState.merge(oldState);
@@ -627,7 +587,7 @@ public class TaskDataManager implements ITaskDataManager {
 				if (file.exists()) {
 					TaskDataState oldState = taskDataStore.getTaskDataState(file);
 					if (oldState != null) {
-						File newFile = getFile(task.getRepositoryUrl(), newTask, kind);
+						File newFile = fileManager.getFile(task.getRepositoryUrl(), newTask, kind);
 						TaskDataState newState = new TaskDataState(oldState.getConnectorKind(), task.getRepositoryUrl(),
 								newTask.getTaskId());
 						newState.merge(oldState);
diff --git a/org.eclipse.mylyn.tasks.tests/src/org/eclipse/mylyn/tasks/tests/TaskDataFileManagerTest.java b/org.eclipse.mylyn.tasks.tests/src/org/eclipse/mylyn/tasks/tests/TaskDataFileManagerTest.java
new file mode 100644
index 000000000..9eb169632
--- /dev/null
+++ b/org.eclipse.mylyn.tasks.tests/src/org/eclipse/mylyn/tasks/tests/TaskDataFileManagerTest.java
@@ -0,0 +1,82 @@
+/*******************************************************************************
+ * Copyright (c) 2004, 2015 Tasktop Technologies and others.
+ * All rights reserved. This program and the accompanying materials
+ * are made available under the terms of the Eclipse Public License v1.0
+ * which accompanies this distribution, and is available at
+ * http://www.eclipse.org/legal/epl-v10.html
+ *
+ * Contributors:
+ *     Tasktop Technologies - initial API and implementation
+ *******************************************************************************/
+
+package org.eclipse.mylyn.tasks.tests;
+
+import java.io.File;
+
+import org.apache.commons.lang.StringUtils;
+import org.eclipse.mylyn.internal.tasks.core.data.TaskDataFileManager;
+import org.eclipse.mylyn.tasks.core.ITask;
+
+import junit.framework.TestCase;
+
+/**
+ * @author Alexei Trebounskikh
+ */
+public class TaskDataFileManagerTest extends TestCase {
+
+	private class TestTaskDataFileManager extends TaskDataFileManager {
+		public String getFileName(ITask task, boolean fileExists) {
+			return super.getFileName(task, name -> fileExists);
+		}
+
+	}
+
+	private final TestTaskDataFileManager fileManager = new TestTaskDataFileManager();
+
+	public void testShortFileName() {
+		// <max, exists, not requires encoding == encoded anyway for backwards compatibility
+		assertEquals("11111%2520", fileManager.getFileName(TaskTestUtil.createMockTask("11111%20"), true));
+		// <max, does not exist, not requires encoding == not encoded
+		assertEquals("11111%20", fileManager.getFileName(TaskTestUtil.createMockTask("11111%20"), false));
+		// <max, does not exist, requires encoding == encoded
+		assertEquals("11111%2520%2B", fileManager.getFileName(TaskTestUtil.createMockTask("11111%20+"), false));
+		// <max, exists, requires encoding == encoded
+		assertEquals("11111%2520%2B", fileManager.getFileName(TaskTestUtil.createMockTask("11111%20+"), true));
+	}
+
+	public void testLongFileNameThatDoesNotRequireEncoding() {
+		// >max, does not exist, not requires encoding == not encoded + trimmed
+		String str = StringUtils.repeat("1", 256);
+		assertEquals(StringUtils.repeat("1", 242) + ".71634944",
+				fileManager.getFileName(TaskTestUtil.createMockTask(str), false));
+
+		// >max, exists, not requires encoding == use as is
+		assertEquals(str, fileManager.getFileName(TaskTestUtil.createMockTask(str), true));
+	}
+
+	public void testLongFileNameThatRequiresEncoding() {
+		// >max, does not exist, requires encoding == encoded + trimmed
+		String str = "+" + StringUtils.repeat("1", 255);
+		String result = fileManager.getFileName(TaskTestUtil.createMockTask(str), false);
+		assertEquals("%2B" + StringUtils.repeat("1", 237) + ".3664039548", result);
+
+		// >max, exists, requires encoding == encoded + NOT trimmed
+		result = fileManager.getFileName(TaskTestUtil.createMockTask(str), true);
+		assertEquals(str.replaceAll("\\+", "%2B"), result);
+	}
+
+	public void testGetSetDataPath() {
+		final String path = "path";
+		fileManager.setDataPath(path);
+		assertEquals(path, fileManager.getDataPath());
+	}
+
+	public void testGetFile() {
+		final String path = "path";
+		fileManager.setDataPath(path);
+
+		final String taskId = "taskId";
+		final File result = fileManager.getFile("url", TaskTestUtil.createMockTask(taskId), "kind");
+		assertTrue(result.getPath().matches("^" + path + "\\S+" + taskId + "\\.\\S+$"));
+	}
+}