diff options
author | alexei.trebounskikh | 2018-07-20 00:02:20 +0000 |
---|---|---|
committer | Sam Davis | 2018-08-02 18:30:21 +0000 |
commit | 566c107ef49118c28e55eca5af3ca1733cb15f13 (patch) | |
tree | a395c4519d334f742b228ae6da6d8604cb8db956 | |
parent | a9f2c2937d19bcd7ba7e52714803877d827b7024 (diff) | |
download | org.eclipse.mylyn.tasks-566c107ef49118c28e55eca5af3ca1733cb15f13.tar.gz org.eclipse.mylyn.tasks-566c107ef49118c28e55eca5af3ca1733cb15f13.tar.xz org.eclipse.mylyn.tasks-566c107ef49118c28e55eca5af3ca1733cb15f13.zip |
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+$")); + } +} |