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>
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 0000000..3b8fcc7
--- /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 ed54c59..0037ae5 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.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 @@
*/
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
}
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 @@
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 void setDataPath(String dataPath) {
- this.dataPath = dataPath;
+ fileManager.setDataPath(dataPath);
}
/**
@@ -520,7 +480,7 @@
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 @@
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 @@
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 0000000..9eb1696
--- /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+$"));
+ }
+}
