From 0d82e7dcc70e65f5c7a7793ff7709237b313ccd1 Mon Sep 17 00:00:00 2001 From: Sebastian Stenzel Date: Fri, 12 Feb 2016 14:31:47 +0100 Subject: [PATCH] code simplicifaction + lockmanager unit tests --- .../filesystem/delegating/DelegatingNode.java | 2 +- .../ExclusiveSharedLockManager.java | 41 ++--- .../ExclusiveSharedLockManagerTest.java | 163 ++++++++++++++++++ 3 files changed, 179 insertions(+), 27 deletions(-) create mode 100644 main/frontend-webdav/src/test/java/org/cryptomator/frontend/webdav/jackrabbitservlet/ExclusiveSharedLockManagerTest.java diff --git a/main/filesystem-api/src/main/java/org/cryptomator/filesystem/delegating/DelegatingNode.java b/main/filesystem-api/src/main/java/org/cryptomator/filesystem/delegating/DelegatingNode.java index 153c0cd833..48e0fd1898 100644 --- a/main/filesystem-api/src/main/java/org/cryptomator/filesystem/delegating/DelegatingNode.java +++ b/main/filesystem-api/src/main/java/org/cryptomator/filesystem/delegating/DelegatingNode.java @@ -14,7 +14,7 @@ import org.cryptomator.filesystem.Node; -abstract class DelegatingNode implements Node { +public abstract class DelegatingNode implements Node { protected final T delegate; diff --git a/main/frontend-webdav/src/main/java/org/cryptomator/frontend/webdav/jackrabbitservlet/ExclusiveSharedLockManager.java b/main/frontend-webdav/src/main/java/org/cryptomator/frontend/webdav/jackrabbitservlet/ExclusiveSharedLockManager.java index ae460070dc..07cd42e546 100644 --- a/main/frontend-webdav/src/main/java/org/cryptomator/frontend/webdav/jackrabbitservlet/ExclusiveSharedLockManager.java +++ b/main/frontend-webdav/src/main/java/org/cryptomator/frontend/webdav/jackrabbitservlet/ExclusiveSharedLockManager.java @@ -41,22 +41,22 @@ private synchronized ActiveLock createLockInternal(LockInfo lockInfo, DavNode FileSystemResourceLocator locator = resource.getLocator(); removedExpiredLocksInLocatorHierarchy(locator); + // look for existing locks on this resource or its ancestors: ActiveLock existingExclusiveLock = getLock(lockInfo.getType(), Scope.EXCLUSIVE, resource); ActiveLock existingSharedLock = getLock(lockInfo.getType(), Scope.SHARED, resource); boolean hasExclusiveLock = existingExclusiveLock != null; boolean hasSharedLock = existingSharedLock != null; boolean isLocked = hasExclusiveLock || hasSharedLock; if ((Scope.EXCLUSIVE.equals(lockInfo.getScope()) && isLocked) || (Scope.SHARED.equals(lockInfo.getScope()) && hasExclusiveLock)) { - throw new DavException(DavServletResponse.SC_LOCKED, "Resource already locked."); + throw new DavException(DavServletResponse.SC_LOCKED, "Resource (or parent resource) already locked."); } - for (Entry> entry : lockedResources.entrySet()) { - final FileSystemResourceLocator entryLocator = entry.getKey(); - final Collection entryLocks = entry.getValue().values(); - if (isAncestor(entryLocator, locator) && isAffectedByParentLocks(lockInfo, locator, entryLocks, entryLocator)) { - throw new DavException(DavServletResponse.SC_LOCKED, "Parent resource already locked. " + entryLocator); - } else if (isAncestor(locator, entryLocator) && isAffectedByChildLocks(lockInfo, locator, entryLocks, entryLocator)) { - throw new DavException(DavServletResponse.SC_CONFLICT, "Subresource already locked. " + entryLocator); + // look for locked children: + for (Entry> potentialChild : lockedResources.entrySet()) { + final FileSystemResourceLocator childLocator = potentialChild.getKey(); + final Collection childLocks = potentialChild.getValue().values(); + if (isChild(locator, childLocator) && isAffectedByChildLocks(lockInfo, locator, childLocks, childLocator)) { + throw new DavException(DavServletResponse.SC_CONFLICT, "Subresource already locked. " + childLocator); } } @@ -65,11 +65,12 @@ private synchronized ActiveLock createLockInternal(LockInfo lockInfo, DavNode } private void removedExpiredLocksInLocatorHierarchy(FileSystemResourceLocator locator) { + Objects.requireNonNull(locator); lockedResources.getOrDefault(locator, Collections.emptyMap()).values().removeIf(ActiveLock::isExpired); locator.parent().ifPresent(this::removedExpiredLocksInLocatorHierarchy); } - private boolean isAncestor(FileSystemResourceLocator parent, FileSystemResourceLocator child) { + private boolean isChild(FileSystemResourceLocator parent, FileSystemResourceLocator child) { if (parent instanceof FolderLocator) { FolderLocator folder = (FolderLocator) parent; return folder.isAncestorOf(child); @@ -78,18 +79,6 @@ private boolean isAncestor(FileSystemResourceLocator parent, FileSystemResourceL } } - private boolean isAffectedByParentLocks(LockInfo childLockInfo, FileSystemResourceLocator childLocator, Collection parentLocks, FileSystemResourceLocator parentLocator) { - assert childLocator.parent().isPresent(); - for (ActiveLock lock : parentLocks) { - if (Scope.SHARED.equals(childLockInfo.getScope()) && Scope.SHARED.equals(lock.getScope())) { - continue; - } else if (lock.isDeep() || childLocator.parent().get().equals(parentLocator)) { - return true; - } - } - return false; - } - private boolean isAffectedByChildLocks(LockInfo parentLockInfo, FileSystemResourceLocator parentLocator, Collection childLocks, FileSystemResourceLocator childLocator) { assert childLocator.parent().isPresent(); for (ActiveLock lock : childLocks) { @@ -107,7 +96,7 @@ public ActiveLock refreshLock(LockInfo lockInfo, String lockToken, DavResource r ActiveLock lock = getLock(lockInfo.getType(), lockInfo.getScope(), resource); if (lock == null) { throw new DavException(DavServletResponse.SC_PRECONDITION_FAILED); - } else if (!lock.getToken().equals(lockToken)) { + } else if (!lock.isLockedByToken(lockToken)) { throw new DavException(DavServletResponse.SC_LOCKED); } lock.setTimeout(lockInfo.getTimeout()); @@ -144,24 +133,24 @@ private synchronized void releaseLockInternal(String lockToken, DavNode resou @Override public ActiveLock getLock(Type type, Scope scope, DavResource resource) { if (resource instanceof DavNode) { - return getLockInternal(type, scope, ((DavNode) resource).getLocator()); + return getLockInternal(type, scope, ((DavNode) resource).getLocator(), 0); } else { throw new IllegalArgumentException("Unsupported resource type " + resource.getClass()); } } - private ActiveLock getLockInternal(Type type, Scope scope, FileSystemResourceLocator locator) { + private ActiveLock getLockInternal(Type type, Scope scope, FileSystemResourceLocator locator, int depth) { // try to find a lock directly on this resource: if (lockedResources.containsKey(locator)) { for (ActiveLock lock : lockedResources.get(locator).values()) { - if (type.equals(lock.getType()) && scope.equals(lock.getScope())) { + if (type.equals(lock.getType()) && scope.equals(lock.getScope()) && (depth == 0 || lock.isDeep())) { return lock; } } } // or otherwise look for parent locks: if (locator.parent().isPresent()) { - return getLockInternal(type, scope, locator.parent().get()); + return getLockInternal(type, scope, locator.parent().get(), depth++); } else { return null; } diff --git a/main/frontend-webdav/src/test/java/org/cryptomator/frontend/webdav/jackrabbitservlet/ExclusiveSharedLockManagerTest.java b/main/frontend-webdav/src/test/java/org/cryptomator/frontend/webdav/jackrabbitservlet/ExclusiveSharedLockManagerTest.java new file mode 100644 index 0000000000..f4ccfa06b5 --- /dev/null +++ b/main/frontend-webdav/src/test/java/org/cryptomator/frontend/webdav/jackrabbitservlet/ExclusiveSharedLockManagerTest.java @@ -0,0 +1,163 @@ +package org.cryptomator.frontend.webdav.jackrabbitservlet; + +import java.util.Optional; + +import org.apache.jackrabbit.webdav.DavException; +import org.apache.jackrabbit.webdav.DavServletResponse; +import org.apache.jackrabbit.webdav.lock.ActiveLock; +import org.apache.jackrabbit.webdav.lock.LockInfo; +import org.apache.jackrabbit.webdav.lock.Scope; +import org.apache.jackrabbit.webdav.lock.Type; +import org.cryptomator.filesystem.jackrabbit.FileLocator; +import org.cryptomator.filesystem.jackrabbit.FolderLocator; +import org.junit.Assert; +import org.junit.Before; +import org.junit.Test; +import org.mockito.Mockito; + +public class ExclusiveSharedLockManagerTest { + + private FolderLocator parentLocator; + private DavFolder parentResource; + private FileLocator childLocator; + private DavFile childResource; + private LockInfo lockInfo; + private ExclusiveSharedLockManager lockManager; + + @Before + public void setup() { + parentLocator = Mockito.mock(FolderLocator.class); + Mockito.when(parentLocator.name()).thenReturn("parent"); + Mockito.when(parentLocator.parent()).thenReturn(Optional.empty()); + + parentResource = Mockito.mock(DavFolder.class); + Mockito.when(parentResource.getLocator()).thenReturn(parentLocator); + + childLocator = Mockito.mock(FileLocator.class); + Mockito.when(childLocator.name()).thenReturn("child"); + Mockito.when(childLocator.parent()).thenReturn(Optional.of(parentLocator)); + Mockito.when(parentLocator.isAncestorOf(childLocator)).thenReturn(true); + + childResource = Mockito.mock(DavFile.class); + Mockito.when(childResource.getLocator()).thenReturn(childLocator); + + lockInfo = Mockito.mock(LockInfo.class); + Mockito.when(lockInfo.getScope()).thenReturn(Scope.EXCLUSIVE); + Mockito.when(lockInfo.getType()).thenReturn(Type.WRITE); + Mockito.when(lockInfo.getOwner()).thenReturn("test"); + Mockito.when(lockInfo.getTimeout()).thenReturn(3600l); + Mockito.when(lockInfo.isDeep()).thenReturn(true); + + lockManager = new ExclusiveSharedLockManager(); + } + + @Test + public void testLockCreation() throws DavException { + ActiveLock lock = lockManager.createLock(lockInfo, parentResource); + Assert.assertEquals(Scope.EXCLUSIVE, lock.getScope()); + Assert.assertEquals(Type.WRITE, lock.getType()); + Assert.assertEquals("test", lock.getOwner()); + Assert.assertFalse(lock.isExpired()); + Assert.assertTrue(lock.isDeep()); + } + + @Test + public void testLockCreationInParent() throws DavException { + ActiveLock lock = lockManager.createLock(lockInfo, parentResource); + + Assert.assertTrue(lockManager.hasLock(lock.getToken(), parentResource)); + Assert.assertFalse(lockManager.hasLock(lock.getToken(), childResource)); + + ActiveLock parentLock = lockManager.getLock(Type.WRITE, Scope.EXCLUSIVE, parentResource); + ActiveLock childLock = lockManager.getLock(Type.WRITE, Scope.EXCLUSIVE, childResource); + Assert.assertEquals(lock, parentLock); + Assert.assertEquals(lock, childLock); + } + + @Test + public void testLockCreationInChild() throws DavException { + ActiveLock lock = lockManager.createLock(lockInfo, childResource); + + Assert.assertTrue(lockManager.hasLock(lock.getToken(), childResource)); + Assert.assertFalse(lockManager.hasLock(lock.getToken(), parentResource)); + + ActiveLock parentLock = lockManager.getLock(Type.WRITE, Scope.EXCLUSIVE, parentResource); + ActiveLock childLock = lockManager.getLock(Type.WRITE, Scope.EXCLUSIVE, childResource); + Assert.assertNull(parentLock); + Assert.assertEquals(lock, childLock); + } + + @Test + public void testMultipleSharedLocks() throws DavException { + Mockito.when(lockInfo.getScope()).thenReturn(Scope.SHARED); + + ActiveLock lock1 = lockManager.createLock(lockInfo, parentResource); + ActiveLock lock2 = lockManager.createLock(lockInfo, childResource); + + Assert.assertTrue(lockManager.hasLock(lock1.getToken(), parentResource)); + Assert.assertTrue(lockManager.hasLock(lock2.getToken(), childResource)); + + Assert.assertNotEquals(lock1, lock2); + } + + @Test + public void testReleaseLock() throws DavException { + ActiveLock lock = lockManager.createLock(lockInfo, parentResource); + Assert.assertNotNull(lockManager.getLock(Type.WRITE, Scope.EXCLUSIVE, parentResource)); + + lockManager.releaseLock(lock.getToken(), parentResource); + Assert.assertNull(lockManager.getLock(Type.WRITE, Scope.EXCLUSIVE, parentResource)); + } + + @Test + public void testReleaseNonExistingLock() throws DavException { + lockManager.releaseLock("doesn't exist", parentResource); + Assert.assertNull(lockManager.getLock(Type.WRITE, Scope.EXCLUSIVE, parentResource)); + } + + @Test + public void testRefreshLock() throws DavException { + ActiveLock originalLock = lockManager.createLock(lockInfo, parentResource); + long originalTimeout = originalLock.getTimeout(); + + Mockito.when(lockInfo.getTimeout()).thenReturn(7200l); + ActiveLock updatedLock = lockManager.refreshLock(lockInfo, originalLock.getToken(), parentResource); + long updatedTimeout = updatedLock.getTimeout(); + + Assert.assertTrue(updatedTimeout > originalTimeout); + } + + @Test(expected = DavException.class) + public void testRefreshNonExistingLock() throws DavException { + lockManager.refreshLock(lockInfo, "doesn't exist", parentResource); + } + + @Test(expected = DavException.class) + public void testRefreshLockWithInvalidToken() throws DavException { + lockManager.createLock(lockInfo, parentResource); + lockManager.refreshLock(lockInfo, "doesn't exist", parentResource); + } + + @Test + public void testLockCreationWhenParentAlreadyLocked() throws DavException { + lockManager.createLock(lockInfo, parentResource); + try { + lockManager.createLock(lockInfo, childResource); + Assert.fail("Should have thrown excpetion"); + } catch (DavException e) { + Assert.assertEquals(DavServletResponse.SC_LOCKED, e.getErrorCode()); + } + } + + @Test + public void testLockCreationWhenChildAlreadyLocked() throws DavException { + lockManager.createLock(lockInfo, childResource); + try { + lockManager.createLock(lockInfo, parentResource); + Assert.fail("Should have thrown excpetion"); + } catch (DavException e) { + Assert.assertEquals(DavServletResponse.SC_CONFLICT, e.getErrorCode()); + } + } + +}