Clean up lock implementation

wise-webapp/src/main/java/com/wisemapping/rest/MindmapController.java

@@ -27,7 +27,6 @@ import com.wisemapping.validator.MapInfoValidator;
 import org.apache.commons.validator.routines.EmailValidator;
 import org.apache.log4j.Logger;
 import org.jetbrains.annotations.NotNull;
-import org.jetbrains.annotations.Nullable;
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.beans.factory.annotation.Qualifier;
 import org.springframework.http.HttpStatus;
@@ -36,7 +35,6 @@ import org.springframework.stereotype.Controller;
 import org.springframework.validation.BeanPropertyBindingResult;
 import org.springframework.validation.BindingResult;
 import org.springframework.web.bind.annotation.*;
-import org.springframework.web.server.ResponseStatusException;
 
 import javax.servlet.http.HttpServletResponse;
 import java.io.IOException;
@@ -111,8 +109,8 @@ public class MindmapController extends BaseController {
     }
 
     @RequestMapping(method = RequestMethod.PUT, value = "/maps/{id}/document", consumes = {"application/xml", "application/json"}, produces = {"application/json", "application/xml"})
-    @ResponseBody
+    @ResponseStatus(value = HttpStatus.NO_CONTENT)
-    public Long updateDocument(@RequestBody RestMindmap restMindmap, @PathVariable int id, @RequestParam(required = false) boolean minor, @RequestParam(required = true) long timestamp, @RequestParam(required = true) long session) throws WiseMappingException, IOException {
+    public void updateDocument(@RequestBody RestMindmap restMindmap, @PathVariable int id, @RequestParam(required = false) boolean minor) throws WiseMappingException, IOException {
 
         final Mindmap mindmap = findMindmapById(id);
         final User user = Utils.getUser();
@@ -125,7 +123,7 @@ public class MindmapController extends BaseController {
 
         // Have permissions ?
         final LockManager lockManager = mindmapService.getLockManager();
-        long result = lockManager.verifyAndUpdateLock(mindmap, user, session, timestamp);
+        lockManager.lock(mindmap, user);
 
         // Update collaboration properties ...
         final CollaborationProperties collaborationProperties = mindmap.findCollaborationProperties(user);
@@ -137,8 +135,6 @@ public class MindmapController extends BaseController {
 
         // Update map ...
         saveMindmapDocument(minor, mindmap, user);
-
-        return result;
     }
 
     @RequestMapping(method = RequestMethod.GET, value = {"/maps/{id}/document/xml", "/maps/{id}/document/xml-pub"}, consumes = {"text/plain"}, produces = {"application/xml; charset=UTF-8"})
@@ -451,25 +447,6 @@ public class MindmapController extends BaseController {
         mindmapService.updateCollaboration(user, collaboration.get());
     }
 
-    @RequestMapping(method = RequestMethod.PUT, value = "/maps/{id}/locks/{lockid}", consumes = {"text/plain"}, produces = {"application/json", "application/xml"})
-    public ResponseEntity<RestLockInfo> lockMindmap(@RequestBody String value, @PathVariable int id, @PathVariable long lockid) throws WiseMappingException {
-        final User user = Utils.getUser();
-        final LockManager lockManager = mindmapService.getLockManager();
-        final Mindmap mindmap = findMindmapById(id);
-
-        ResponseEntity<RestLockInfo> result = new ResponseEntity<>(null, HttpStatus.NO_CONTENT);
-        if (Boolean.parseBoolean(value)) {
-            if (!lockManager.isLocked(mindmap)) {
-                final LockInfo lockInfo = lockManager.lock(mindmap, user, lockid);
-                final RestLockInfo restLockInfo = new RestLockInfo(lockInfo, user);
-                result = new ResponseEntity<>(restLockInfo, HttpStatus.OK);
-            }
-        } else {
-            lockManager.unlock(mindmap, user);
-        }
-        return result;
-    }
-
     @RequestMapping(method = RequestMethod.DELETE, value = "/maps/batch")
     @ResponseStatus(value = HttpStatus.NO_CONTENT)
     public void batchDelete(@RequestParam() String ids) throws IOException, WiseMappingException {
@@ -587,5 +564,20 @@ public class MindmapController extends BaseController {
         mindmapService.updateMindmap(mindmap, false);
     }
 
+    @RequestMapping(method = RequestMethod.PUT, value = "/maps/{id}/lock", consumes = {"text/plain"}, produces = {"application/json", "application/xml"})
+    public ResponseEntity<RestLockInfo> lockMindmap(@RequestBody String value, @PathVariable int id) throws WiseMappingException {
+        final User user = Utils.getUser();
+        final LockManager lockManager = mindmapService.getLockManager();
+        final Mindmap mindmap = findMindmapById(id);
 
+        ResponseEntity<RestLockInfo> result = new ResponseEntity<>(null, HttpStatus.NO_CONTENT);
+        if (Boolean.parseBoolean(value)) {
+            final LockInfo lockInfo = lockManager.lock(mindmap, user);
+            final RestLockInfo restLockInfo = new RestLockInfo(lockInfo, user);
+            result = new ResponseEntity<>(restLockInfo, HttpStatus.OK);
+        } else {
+            lockManager.unlock(mindmap, user);
+        }
+        return result;
+    }
 }

wise-webapp/src/main/java/com/wisemapping/rest/model/RestLockInfo.java

@@ -40,8 +40,6 @@ import javax.xml.bind.annotation.XmlRootElement;
 @JsonIgnoreProperties(ignoreUnknown = true)
 public class RestLockInfo {
 
-    private long session;
-    private long timestamp;
     private String email;
 
     // This is required only for compliance with the JAXB serializer.
@@ -50,22 +48,9 @@ public class RestLockInfo {
     }
 
     public RestLockInfo(@Nullable LockInfo lockInfo, @NotNull User user) {
-        this.session = lockInfo.getSession();
-        this.timestamp = lockInfo.getTimestamp();
         this.email = user.getEmail();
     }
 
-    public long getTimestamp() {
-        return this.timestamp;
-    }
-
-    public long getSession() {
-        return this.session;
-    }
-
-    public void setSession(long session) {
-        this.session = session;
-    }
 
     public String getEmail() {
         return email;
@@ -74,8 +59,5 @@ public class RestLockInfo {
     public void setEmail(String email) {
         this.email = email;
     }
-
-    public void setTimestamp(long value) {
-        this.timestamp = value;
-    }
 }
+

wise-webapp/src/main/java/com/wisemapping/service/LockInfo.java

@@ -27,16 +27,22 @@ import java.util.Calendar;
 public class LockInfo {
     final private User user;
     private Calendar timeout;
-    private long session;
     private static final int EXPIRATION_MIN = 30;
-    private long timestamp = -1;
-    private long previousTimestamp;
 
-    public LockInfo(@NotNull User user, @NotNull Mindmap mindmap, long session) {
+    public int getMapId() {
+        return mapId;
+    }
+
+    public void setMapId(int mapId) {
+        this.mapId = mapId;
+    }
+
+    private int mapId;
+
+    public LockInfo(@NotNull User user, @NotNull Mindmap mindmap) {
         this.user = user;
+        this.mapId = mindmap.getId();
         this.updateTimeout();
-        this.updateTimestamp(mindmap);
-        this.session = session;
     }
 
     public User getUser() {
@@ -54,33 +60,12 @@ public class LockInfo {
 
     }
 
-    public long getSession() {
-        return session;
-    }
-
-    public void setSession(long session) {
-        this.session = session;
-    }
-
-    public long getTimestamp() {
-        return timestamp;
-    }
-
-    public long getPreviousTimestamp() {
-        return previousTimestamp;
-    }
-
-    public void updateTimestamp(@NotNull Mindmap mindmap) {
-        this.previousTimestamp = this.timestamp;
-        this.timestamp = mindmap.getLastModificationTime().getTimeInMillis();
-    }
-
     @Override
     public String toString() {
         return "LockInfo{" +
                 "user=" + user +
-                ", session=" + session +
+                ", timeout=" + timeout +
-                ", timestamp=" + timestamp +
+                ", mapId=" + mapId +
                 '}';
     }
 }

wise-webapp/src/main/java/com/wisemapping/service/LockManager.java

@@ -21,7 +21,6 @@ package com.wisemapping.service;
 import com.wisemapping.exceptions.AccessDeniedSecurityException;
 import com.wisemapping.exceptions.LockException;
 import com.wisemapping.exceptions.SessionExpiredException;
-import com.wisemapping.exceptions.WiseMappingException;
 import com.wisemapping.model.Mindmap;
 import com.wisemapping.model.User;
 import org.jetbrains.annotations.NotNull;
@@ -31,19 +30,14 @@ public interface LockManager {
 
     LockInfo getLockInfo(@NotNull Mindmap mindmap);
 
-    LockInfo updateExpirationTimeout(@NotNull Mindmap mindmap, @NotNull User user);
-
     void unlock(@NotNull Mindmap mindmap, @NotNull User user) throws LockException, AccessDeniedSecurityException;
 
     void unlockAll(@NotNull User user) throws LockException, AccessDeniedSecurityException;
 
-    boolean isLockedBy(@NotNull Mindmap mindmap, @NotNull User collaborator);
+    boolean isLockedBy(@NotNull Mindmap mindmap, @NotNull User user);
 
     long generateSession();
 
     @NotNull
-    LockInfo lock(@NotNull Mindmap mindmap, @NotNull User user, long session) throws LockException;
+    LockInfo lock(@NotNull Mindmap mindmap, @NotNull User user) throws LockException;
-
-    long verifyAndUpdateLock(@NotNull Mindmap mindmap, @NotNull User user, long session, long timestamp) throws
-            LockException, SessionExpiredException;
 }

wise-webapp/src/main/java/com/wisemapping/service/LockManagerImpl.java

@@ -20,7 +20,6 @@ package com.wisemapping.service;
 
 import com.wisemapping.exceptions.AccessDeniedSecurityException;
 import com.wisemapping.exceptions.LockException;
-import com.wisemapping.exceptions.SessionExpiredException;
 import com.wisemapping.model.CollaborationRole;
 import com.wisemapping.model.Mindmap;
 import com.wisemapping.model.User;
@@ -47,23 +46,6 @@ class LockManagerImpl implements LockManager {
         return lockInfoByMapId.get(mindmap.getId());
     }
 
-    @Override
-    public LockInfo updateExpirationTimeout(@NotNull Mindmap mindmap, @NotNull User user) {
-        if (!this.isLocked(mindmap)) {
-            throw new IllegalStateException("Lock lost for map. No update possible.");
-        }
-
-        final LockInfo result = this.getLockInfo(mindmap);
-        if (!result.getUser().identityEquality(user)) {
-            throw new IllegalStateException("Could not update map lock timeout if you are not the locking user. User:" + result.getUser() + ", " + user);
-        }
-
-        result.updateTimeout();
-        result.updateTimestamp(mindmap);
-        logger.debug("Timeout updated for:" + mindmap.getId());
-        return result;
-    }
-
     @Override
     public void unlockAll(@NotNull final User user) throws LockException, AccessDeniedSecurityException {
         final Set<Integer> mapIds = lockInfoByMapId.keySet();
@@ -77,14 +59,7 @@ class LockManagerImpl implements LockManager {
 
     @Override
     public void unlock(@NotNull Mindmap mindmap, @NotNull User user) throws LockException, AccessDeniedSecurityException {
-        if (isLocked(mindmap) && !isLockedBy(mindmap, user)) {
+        verifyHasLock(mindmap, user);
-            throw new LockException("Lock can be only revoked by the locker.");
-        }
-
-        if (!mindmap.hasPermissions(user, CollaborationRole.EDITOR)) {
-            throw new AccessDeniedSecurityException(mindmap.getId(), user);
-        }
-
         this.unlock(mindmap.getId());
     }
 
@@ -111,25 +86,38 @@ class LockManagerImpl implements LockManager {
 
     @NotNull
     @Override
-    public LockInfo lock(@NotNull Mindmap mindmap, @NotNull User user, long session) throws LockException {
+    public LockInfo lock(@NotNull Mindmap mindmap, @NotNull User user) throws LockException {
         if (isLocked(mindmap) && !isLockedBy(mindmap, user)) {
             throw LockException.createLockLost(mindmap, user, this);
         }
 
+        // Do I need to create a new lock ?
         LockInfo result = lockInfoByMapId.get(mindmap.getId());
-        if (result != null) {
+        if (result == null) {
-            // Update timeout only...
+            logger.debug("Creating new lock for map id:" + mindmap.getId());
-            logger.debug("Update timestamp:" + mindmap.getId());
+            result = new LockInfo(user, mindmap);
-            updateExpirationTimeout(mindmap, user);
-            // result.setSession(session);
-        } else {
-            logger.debug("Lock map id:" + mindmap.getId());
-            result = new LockInfo(user, mindmap, session);
             lockInfoByMapId.put(mindmap.getId(), result);
         }
+
+        // Update timestamp ...
+        logger.debug("Updating timeout:" + result);
+        result.updateTimeout();
+
         return result;
     }
 
+    private void verifyHasLock(@NotNull Mindmap mindmap, @NotNull User user) throws LockException, AccessDeniedSecurityException {
+        // Only editor can have lock ...
+        if (!mindmap.hasPermissions(user, CollaborationRole.EDITOR)) {
+            throw new AccessDeniedSecurityException(mindmap.getId(), user);
+        }
+
+        // Is the lock assigned to the user ...
+        if (isLocked(mindmap) && !isLockedBy(mindmap, user)) {
+            throw LockException.createLockLost(mindmap, user, this);
+        }
+    }
+
     public LockManagerImpl() {
         lockInfoByMapId = new ConcurrentHashMap<>();
         expirationTimer.schedule(new TimerTask() {
@@ -149,46 +137,4 @@ class LockManagerImpl implements LockManager {
             }
         }, ONE_MINUTE_MILLISECONDS, ONE_MINUTE_MILLISECONDS);
     }
-
-    public long verifyAndUpdateLock(@NotNull Mindmap mindmap, @NotNull User user, @Nullable long session, @NotNull long timestamp) throws LockException, SessionExpiredException {
-        synchronized (this) {
-            // Could the map be updated ?
-            verifyLock(mindmap, user, session, timestamp);
-
-            // Update timestamp for lock ...
-            final LockInfo lockInfo = this.updateExpirationTimeout(mindmap, user);
-            return lockInfo.getTimestamp();
-        }
-    }
-
-    private void verifyLock(@NotNull Mindmap mindmap, @NotNull User user, long session, long timestamp) throws LockException, SessionExpiredException {
-
-        // The lock was lost, reclaim as the ownership of it.
-        final boolean lockLost = this.isLocked(mindmap);
-        if (!lockLost) {
-            this.lock(mindmap, user, session);
-        }
-
-        final LockInfo lockInfo = this.getLockInfo(mindmap);
-        if (lockInfo.getUser().identityEquality(user)) {
-            long savedTimestamp = mindmap.getLastModificationTime().getTimeInMillis();
-            final boolean outdated = savedTimestamp > timestamp;
-
-            if (lockInfo.getSession() == session) {
-                // Timestamp might not be returned to the client. This try to cover this case, ignoring the client timestamp check.
-                final User lastEditor = mindmap.getLastEditor();
-                boolean editedBySameUser = lastEditor == null || user.identityEquality(lastEditor);
-                if (outdated && !editedBySameUser) {
-                    throw new SessionExpiredException("Map has been updated by " + (lastEditor.getEmail()) + ",Timestamp:" + timestamp + "," + savedTimestamp + ", User:" + lastEditor.getId() + ":" + user.getId() + ",Mail:'" + lastEditor.getEmail() + "':'" + user.getEmail(), lastEditor);
-                }
-            } else if (outdated) {
-                logger.warn("Sessions:" + session + ":" + lockInfo.getSession() + ",Timestamp: " + timestamp + ": " + savedTimestamp);
-                // @Todo: Temporally disabled to unblock save action. More research needed.
-//                throw new MultipleSessionsOpenException("Sessions:" + session + ":" + lockInfo.getSession() + ",Timestamp: " + timestamp + ": " + savedTimestamp);
-            }
-        } else {
-            throw new SessionExpiredException("Different Users.", lockInfo.getUser());
-        }
-    }
-
 }

wise-webapp/src/main/java/com/wisemapping/webmvc/MindmapController.java

@@ -47,7 +47,6 @@ import java.util.Locale;
 @Controller
 public class MindmapController {
 
-    public static final String LOCK_SESSION_ATTRIBUTE = "lockSession";
     @Qualifier("mindmapService")
     @Autowired
     private MindmapService mindmapService;
@@ -76,19 +75,16 @@ public class MindmapController {
     private String showEditorPage(int id, @NotNull final Model model, boolean requiresLock) throws WiseMappingException {
         final MindMapBean mindmapBean = findMindmapBean(id);
         final Mindmap mindmap = mindmapBean.getDelegated();
-        final User collaborator = Utils.getUser();
+        final User user = Utils.getUser();
         final Locale locale = LocaleContextHolder.getLocale();
 
         // Is the mindmap locked ?.
         boolean isLocked = false;
-        boolean readOnlyMode = !requiresLock || !mindmap.hasPermissions(collaborator, CollaborationRole.EDITOR);
+        boolean readOnlyMode = !requiresLock || !mindmap.hasPermissions(user, CollaborationRole.EDITOR);
         if (!readOnlyMode) {
             final LockManager lockManager = this.mindmapService.getLockManager();
-            if (lockManager.isLocked(mindmap) && !lockManager.isLockedBy(mindmap, collaborator)) {
+            if (lockManager.isLocked(mindmap) && !lockManager.isLockedBy(mindmap, user)) {
                 isLocked = true;
-            } else {
-                model.addAttribute("lockTimestamp", mindmap.getLastModificationTime().getTimeInMillis());
-                model.addAttribute(LOCK_SESSION_ATTRIBUTE, lockManager.generateSession());
             }
             model.addAttribute("lockInfo", lockManager.getLockInfo(mindmap));
         }
@@ -97,8 +93,7 @@ public class MindmapController {
 
         // Configure default locale for the editor ...
         model.addAttribute("locale", locale.toString().toLowerCase());
-        model.addAttribute("principal", collaborator);
+        model.addAttribute("principal", user);
-        model.addAttribute("memoryPersistence", false);
         model.addAttribute("mindmapLocked", isLocked);
 
         return "mindmapEditor";
@@ -107,23 +102,17 @@ public class MindmapController {
     @RequestMapping(value = "maps/{id}/view", method = RequestMethod.GET)
     public String showMindmapViewerPage(@PathVariable int id, @NotNull Model model) throws WiseMappingException {
         final String result = showPrintPage(id, model);
-        model.addAttribute("readOnlyMode", true);
         return result;
     }
 
     @RequestMapping(value = "maps/{id}/try", method = RequestMethod.GET)
     public String showMindmapTryPage(@PathVariable int id, @NotNull Model model) throws WiseMappingException {
-        final String result = showEditorPage(id, model, false);
+        return  showEditorPage(id, model, false);
-        model.addAttribute("memoryPersistence", true);
-        model.addAttribute("readOnlyMode", false);
-        return result;
     }
 
     @RequestMapping(value = "maps/{id}/{hid}/view", method = RequestMethod.GET)
     public String showMindmapViewerRevPage(@PathVariable int id, @PathVariable int hid, @NotNull Model model) throws WiseMappingException {
-
         final String result = showPrintPage(id, model);
-        model.addAttribute("readOnlyMode", true);
         model.addAttribute("hid", String.valueOf(hid));
         return result;
     }

wise-webapp/src/main/webapp/jsp/mindmapEditor.jsp

@@ -33,8 +33,6 @@
     </script>
     <script type="text/javascript">
         var mapId = '${mindmap.id}';
-        var lockTimestamp = '${lockTimestamp}';
-            var lockSession = '${lockSession}';
         var mindmapLocked = ${mindmapLocked};
         var mindmapLockedMsg = '<spring:message code="MINDMAP_LOCKED" arguments="${lockInfo.user.fullName},${lockInfo.user.email}"/>';
         var userOptions = ${mindmap.properties};

wise-webapp/src/main/webapp/jsp/mindmapViewonly.jsp

@@ -15,7 +15,6 @@
     <script type="text/javascript">
           var mapId = '${mindmap.id}';
           var historyId = '${hid}';
-          var memoryPersistence = true;
           var readOnly = true;
           var userOptions = ${mindmap.properties};
           var locale = '${locale}';

wise-webapp/src/test/java/com/wisemapping/test/rest/RestMindmapITCase.java

@@ -222,12 +222,8 @@ public class RestMindmapITCase {
         final HttpHeaders lockHeaders = createHeaders(mediaType);
         lockHeaders.setContentType(MediaType.TEXT_PLAIN);
 
-        HttpEntity<String> lockEntity = new HttpEntity<>("true", lockHeaders);
-        final ResponseEntity<RestLockInfo> lockResponse = template.exchange(HOST_PORT + resourceUri + "/locks/{lockid}", HttpMethod.PUT, lockEntity, RestLockInfo.class, SESSION_ID);
-        final RestLockInfo lockInfo = lockResponse.getBody();
-
         // Update map ...
-        final String resourceUrl = HOST_PORT + resourceUri.toString() + "/document?session=" + lockInfo.getSession() + "&timestamp=" + lockInfo.getTimestamp();
+        final String resourceUrl = HOST_PORT + resourceUri.toString() + "/document";
         requestHeaders.setContentType(MediaType.APPLICATION_XML);
         final HttpEntity<RestMindmap> updateEntity = new HttpEntity<>(mapToUpdate, requestHeaders);
         template.put(resourceUrl, updateEntity);
@@ -237,6 +233,10 @@ public class RestMindmapITCase {
         final ResponseEntity<RestMindmap> response = template.exchange(HOST_PORT + resourceUri, HttpMethod.GET, findMapEntity, RestMindmap.class);
         assertEquals(response.getBody().getXml(), mapToUpdate.getXml());
         assertEquals(response.getBody().getProperties(), mapToUpdate.getProperties());
+
+        // Unlock ...
+        HttpEntity<String> lockEntity = new HttpEntity<>("false", lockHeaders);
+        template.exchange(HOST_PORT + resourceUri + "/locks/{lockid}", HttpMethod.PUT, lockEntity, RestLockInfo.class, SESSION_ID);
     }
 
     @Test(dataProviderClass = RestHelper.class, dataProvider = "ContentType-Provider-Function")