Implemented handling of concurrent modification of objects

based on lastModified date

scm-webapp/src/main/java/sonia/scm/api/rest/ConcurrentModificationExceptionMapper.java

@@ -0,0 +1,15 @@
+package sonia.scm.api.rest;
+
+import sonia.scm.ConcurrentModificationException;
+
+import javax.ws.rs.core.Response;
+import javax.ws.rs.ext.ExceptionMapper;
+import javax.ws.rs.ext.Provider;
+
+@Provider
+public class ConcurrentModificationExceptionMapper implements ExceptionMapper<ConcurrentModificationException> {
+  @Override
+  public Response toResponse(ConcurrentModificationException exception) {
+    return Response.status(Response.Status.CONFLICT).build();
+  }
+}

scm-webapp/src/main/java/sonia/scm/api/v2/resources/GroupDtoToGroupMapper.java

@@ -4,12 +4,19 @@ import org.mapstruct.Mapper;
 import org.mapstruct.Mapping;
 import sonia.scm.group.Group;
 
+import java.time.Instant;
+
 
 @Mapper
 public abstract class GroupDtoToGroupMapper {
 
   @Mapping(target = "creationDate", ignore = true)
-  @Mapping(target = "lastModified", ignore = true)
   public abstract Group map(GroupDto groupDto);
 
+  Long mapDate(Instant value) {
+    if (value != null) {
+      return value.toEpochMilli();
+    }
+    return null;
+  }
 }

scm-webapp/src/main/java/sonia/scm/api/v2/resources/GroupResource.java

@@ -3,6 +3,7 @@ package sonia.scm.api.v2.resources;
 import com.webcohesion.enunciate.metadata.rs.ResponseCode;
 import com.webcohesion.enunciate.metadata.rs.StatusCodes;
 import com.webcohesion.enunciate.metadata.rs.TypeHint;
+import sonia.scm.ConcurrentModificationException;
 import sonia.scm.NotFoundException;
 import sonia.scm.group.Group;
 import sonia.scm.group.GroupManager;
@@ -97,7 +98,7 @@ public class GroupResource {
     @ResponseCode(code = 500, condition = "internal server error")
   })
   @TypeHint(TypeHint.NO_CONTENT.class)
-  public Response update(@PathParam("id") String name, @Valid GroupDto groupDto) throws NotFoundException {
+  public Response update(@PathParam("id") String name, @Valid GroupDto groupDto) throws NotFoundException, ConcurrentModificationException {
     return adapter.update(name, existing -> dtoToGroupMapper.map(groupDto));
   }
 }

scm-webapp/src/main/java/sonia/scm/api/v2/resources/IdResourceManagerAdapter.java

@@ -2,6 +2,7 @@ package sonia.scm.api.v2.resources;
 
 import de.otto.edison.hal.HalRepresentation;
 import sonia.scm.AlreadyExistsException;
+import sonia.scm.ConcurrentModificationException;
 import sonia.scm.Manager;
 import sonia.scm.ModelObject;
 import sonia.scm.NotFoundException;
@@ -36,7 +37,7 @@ class IdResourceManagerAdapter<MODEL_OBJECT extends ModelObject,
     return singleAdapter.get(loadBy(id), mapToDto);
   }
 
-  public Response update(String id, Function<MODEL_OBJECT, MODEL_OBJECT> applyChanges) throws NotFoundException {
+  public Response update(String id, Function<MODEL_OBJECT, MODEL_OBJECT> applyChanges) throws NotFoundException, ConcurrentModificationException {
     return singleAdapter.update(
       loadBy(id),
       applyChanges,

scm-webapp/src/main/java/sonia/scm/api/v2/resources/RepositoryResource.java

@@ -3,6 +3,7 @@ package sonia.scm.api.v2.resources;
 import com.webcohesion.enunciate.metadata.rs.ResponseCode;
 import com.webcohesion.enunciate.metadata.rs.StatusCodes;
 import com.webcohesion.enunciate.metadata.rs.TypeHint;
+import sonia.scm.ConcurrentModificationException;
 import sonia.scm.NotFoundException;
 import sonia.scm.repository.NamespaceAndName;
 import sonia.scm.repository.Repository;
@@ -126,7 +127,7 @@ public class RepositoryResource {
     @ResponseCode(code = 500, condition = "internal server error")
   })
   @TypeHint(TypeHint.NO_CONTENT.class)
-  public Response update(@PathParam("namespace") String namespace, @PathParam("name") String name, @Valid RepositoryDto repositoryDto) throws NotFoundException {
+  public Response update(@PathParam("namespace") String namespace, @PathParam("name") String name, @Valid RepositoryDto repositoryDto) throws NotFoundException, ConcurrentModificationException {
     return adapter.update(
       loadBy(namespace, name),
       existing -> dtoToRepositoryMapper.map(repositoryDto, existing.getId()),

scm-webapp/src/main/java/sonia/scm/api/v2/resources/SingleResourceManagerAdapter.java

@@ -1,6 +1,7 @@
 package sonia.scm.api.v2.resources;
 
 import de.otto.edison.hal.HalRepresentation;
+import sonia.scm.ConcurrentModificationException;
 import sonia.scm.Manager;
 import sonia.scm.ModelObject;
 import sonia.scm.NotFoundException;
@@ -57,15 +58,23 @@ class SingleResourceManagerAdapter<MODEL_OBJECT extends ModelObject,
    * Update the model object for the given id according to the given function and returns a corresponding http response.
    * This handles all corner cases, eg. no matching object for the id or missing privileges.
    */
-  public Response update(Supplier<Optional<MODEL_OBJECT>> reader, Function<MODEL_OBJECT, MODEL_OBJECT> applyChanges, Predicate<MODEL_OBJECT> hasSameKey) throws NotFoundException {
+  public Response update(Supplier<Optional<MODEL_OBJECT>> reader, Function<MODEL_OBJECT, MODEL_OBJECT> applyChanges, Predicate<MODEL_OBJECT> hasSameKey) throws NotFoundException, ConcurrentModificationException {
     MODEL_OBJECT existingModelObject = reader.get().orElseThrow(NotFoundException::new);
     MODEL_OBJECT changedModelObject = applyChanges.apply(existingModelObject);
     if (!hasSameKey.test(changedModelObject)) {
       return Response.status(BAD_REQUEST).entity("illegal change of id").build();
     }
+    else if (modelObjectWasModifiedConcurrently(existingModelObject, changedModelObject)) {
+      throw new ConcurrentModificationException();
+    }
     return update(getId(existingModelObject), changedModelObject);
   }
 
+  private boolean modelObjectWasModifiedConcurrently(MODEL_OBJECT existing, MODEL_OBJECT updated) {
+    return (existing.getLastModified() != null && updated.getLastModified() != null)
+      && (existing.getLastModified() > updated.getLastModified());
+  }
+
   public Response delete(Supplier<Optional<MODEL_OBJECT>> reader) {
     return reader.get()
       .map(MODEL_OBJECT::getId)

scm-webapp/src/main/java/sonia/scm/api/v2/resources/UserDto.java

@@ -27,7 +27,6 @@ public class UserDto extends HalRepresentation {
   @Pattern(regexp = "^[A-z0-9\\.\\-_@]|[^ ]([A-z0-9\\.\\-_@ ]*[A-z0-9\\.\\-_@]|[^ ])?$")
   private String name;
   private String password;
-  @NotEmpty
   private String type;
   private Map<String, String> properties;
 

scm-webapp/src/main/java/sonia/scm/api/v2/resources/UserResource.java

@@ -3,6 +3,7 @@ package sonia.scm.api.v2.resources;
 import com.webcohesion.enunciate.metadata.rs.ResponseCode;
 import com.webcohesion.enunciate.metadata.rs.StatusCodes;
 import com.webcohesion.enunciate.metadata.rs.TypeHint;
+import sonia.scm.ConcurrentModificationException;
 import sonia.scm.NotFoundException;
 import sonia.scm.user.User;
 import sonia.scm.user.UserManager;
@@ -97,7 +98,7 @@ public class UserResource {
     @ResponseCode(code = 500, condition = "internal server error")
   })
   @TypeHint(TypeHint.NO_CONTENT.class)
-  public Response update(@PathParam("id") String name, @Valid UserDto userDto) throws NotFoundException {
+  public Response update(@PathParam("id") String name, @Valid UserDto userDto) throws NotFoundException, ConcurrentModificationException {
     return adapter.update(name, existing -> dtoToUserMapper.map(userDto, existing.getPassword()));
   }
 }

scm-webapp/src/main/java/sonia/scm/repository/HealthChecker.java

@@ -1,9 +1,9 @@
 /**
  * Copyright (c) 2010, Sebastian Sdorra All rights reserved.
- *
+ * <p>
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are met:
- *
+ * <p>
  * 1. Redistributions of source code must retain the above copyright notice,
  * this list of conditions and the following disclaimer. 2. Redistributions in
  * binary form must reproduce the above copyright notice, this list of
@@ -11,7 +11,7 @@
  * materials provided with the distribution. 3. Neither the name of SCM-Manager;
  * nor the names of its contributors may be used to endorse or promote products
  * derived from this software without specific prior written permission.
- *
+ * <p>
  * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
  * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
  * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
@@ -22,17 +22,12 @@
  * CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
  * OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
  * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
- *
+ * <p>
  * http://bitbucket.org/sdorra/scm-manager
- *
  */
 
-
-
 package sonia.scm.repository;
 
-//~--- non-JDK imports --------------------------------------------------------
-
 import com.github.sdorra.ssp.PermissionActionCheck;
 import com.google.common.collect.ImmutableList;
 import com.google.inject.Inject;
@@ -43,47 +38,29 @@ import sonia.scm.NotFoundException;
 
 import java.util.Set;
 
-//~--- JDK imports ------------------------------------------------------------
 
-/**
+public final class HealthChecker {
- *
- * @author Sebastian Sdorra
- */
-public final class HealthChecker
-{
 
-  /**
-   * the logger for HealthChecker
-   */
   private static final Logger logger =
     LoggerFactory.getLogger(HealthChecker.class);
 
-  //~--- constructors ---------------------------------------------------------
+  private final Set<HealthCheck> checks;
+
+  private final RepositoryManager repositoryManager;
 
-  /**
-   * Constructs ...
-   *
-   *
-   * @param checks
-   * @param repositoryManager
-   */
   @Inject
   public HealthChecker(Set<HealthCheck> checks,
-    RepositoryManager repositoryManager)
+                       RepositoryManager repositoryManager) {
-  {
     this.checks = checks;
     this.repositoryManager = repositoryManager;
   }
 
-  //~--- methods --------------------------------------------------------------
-
   public void check(String id) throws NotFoundException {
     RepositoryPermissions.healthCheck(id).check();
 
     Repository repository = repositoryManager.get(id);
 
-    if (repository == null)
+    if (repository == null) {
-    {
       throw new RepositoryNotFoundException(
         "could not find repository with id ".concat(id));
     }
@@ -98,27 +75,19 @@ public final class HealthChecker
     doCheck(repository);
   }
 
-  public void checkAll()
+  public void checkAll() {
-  {
     logger.debug("check health of all repositories");
 
     PermissionActionCheck<Repository> check = RepositoryPermissions.healthCheck();
 
-    for (Repository repository : repositoryManager.getAll())
+    for (Repository repository : repositoryManager.getAll()) {
-    {
+      if (check.isPermitted(repository)) {
-      if (check.isPermitted(repository))
+        try {
-      {
-        try
-        {
           check(repository);
-        }
+        } catch (ConcurrentModificationException | NotFoundException ex) {
-        catch (ConcurrentModificationException | NotFoundException ex)
-        {
           logger.error("health check ends with exception", ex);
         }
-      }
+      } else {
-      else
-      {
         logger.debug(
           "no permissions to execute health check for repository {}",
           repository.getId());
@@ -131,25 +100,20 @@ public final class HealthChecker
 
     HealthCheckResult result = HealthCheckResult.healthy();
 
-    for (HealthCheck check : checks)
+    for (HealthCheck check : checks) {
-    {
       logger.trace("execute health check {} for repository {}",
         check.getClass(), repository.getName());
       result = result.merge(check.check(repository));
     }
 
-    if (result.isUnhealthy())
+    if (result.isUnhealthy()) {
-    {
       logger.warn("repository {} is unhealthy: {}", repository.getName(),
         result);
-    }
+    } else {
-    else
-    {
       logger.info("repository {} is healthy", repository.getName());
     }
 
-    if (!(repository.isHealthy() && result.isHealthy()))
+    if (!(repository.isHealthy() && result.isHealthy())) {
-    {
       logger.trace("store health check results for repository {}",
         repository.getName());
       repository.setHealthCheckFailures(
@@ -158,11 +122,5 @@ public final class HealthChecker
     }
   }
 
-  //~--- fields ---------------------------------------------------------------
 
-  /** Field description */
-  private final Set<HealthCheck> checks;
-
-  /** Field description */
-  private final RepositoryManager repositoryManager;
 }

scm-webapp/src/test/java/sonia/scm/api/v2/resources/DispatcherMock.java

@@ -4,6 +4,7 @@ import org.jboss.resteasy.core.Dispatcher;
 import org.jboss.resteasy.mock.MockDispatcherFactory;
 import sonia.scm.api.rest.AlreadyExistsExceptionMapper;
 import sonia.scm.api.rest.AuthorizationExceptionMapper;
+import sonia.scm.api.rest.ConcurrentModificationExceptionMapper;
 
 public class DispatcherMock {
   public static Dispatcher createDispatcher(Object resource) {
@@ -12,6 +13,7 @@ public class DispatcherMock {
     dispatcher.getProviderFactory().registerProvider(NotFoundExceptionMapper.class);
     dispatcher.getProviderFactory().registerProvider(AlreadyExistsExceptionMapper.class);
     dispatcher.getProviderFactory().registerProvider(AuthorizationExceptionMapper.class);
+    dispatcher.getProviderFactory().registerProvider(ConcurrentModificationExceptionMapper.class);
     return dispatcher;
   }
 }

scm-webapp/src/test/java/sonia/scm/api/v2/resources/GroupDtoToGroupMapperTest.java

@@ -1,13 +1,13 @@
 package sonia.scm.api.v2.resources;
 
-import com.fasterxml.jackson.databind.node.TextNode;
 import org.junit.Test;
 import org.mapstruct.factory.Mappers;
 import sonia.scm.group.Group;
 
+import java.time.Instant;
 import java.util.Arrays;
 
-import static java.util.Arrays.asList;
+import static org.assertj.core.api.Assertions.assertThat;
 import static org.junit.Assert.assertEquals;
 
 public class GroupDtoToGroupMapperTest {
@@ -16,8 +16,10 @@ public class GroupDtoToGroupMapperTest {
   public void shouldMapAttributes() {
     GroupDto dto = new GroupDto();
     dto.setName("group");
+    dto.setLastModified(Instant.ofEpochMilli(1234));
     Group group = Mappers.getMapper(GroupDtoToGroupMapper.class).map(dto);
     assertEquals("group", group.getName());
+    assertThat(group.getLastModified()).isEqualTo(dto.getLastModified().toEpochMilli());
   }
 
   @Test

scm-webapp/src/test/java/sonia/scm/api/v2/resources/GroupRootResourceTest.java

@@ -13,6 +13,8 @@ import org.mockito.ArgumentCaptor;
 import org.mockito.InjectMocks;
 import org.mockito.Mock;
 import sonia.scm.PageResult;
+import sonia.scm.api.rest.JSONContextResolver;
+import sonia.scm.api.rest.ObjectMapperProvider;
 import sonia.scm.group.Group;
 import sonia.scm.group.GroupManager;
 import sonia.scm.web.VndMediaType;
@@ -28,8 +30,8 @@ import static java.util.Collections.singletonList;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertTrue;
-import static org.mockito.Matchers.any;
+import static org.mockito.ArgumentMatchers.any;
-import static org.mockito.Matchers.eq;
+import static org.mockito.ArgumentMatchers.eq;
 import static org.mockito.Mockito.doNothing;
 import static org.mockito.Mockito.when;
 import static org.mockito.MockitoAnnotations.initMocks;
@@ -74,6 +76,7 @@ public class GroupRootResourceTest {
     GroupRootResource groupRootResource = new GroupRootResource(MockProvider.of(groupCollectionResource), MockProvider.of(groupResource));
 
     dispatcher = createDispatcher(groupRootResource);
+    dispatcher.getProviderFactory().registerProviderInstance(new JSONContextResolver(new ObjectMapperProvider().get()));
   }
 
   @Test
@@ -143,6 +146,23 @@ public class GroupRootResourceTest {
     assertEquals(HttpServletResponse.SC_NOT_FOUND, response.getStatus());
   }
 
+  @Test
+  public void updateShouldFailOnConcurrentModification() throws URISyntaxException, IOException {
+    URL url = Resources.getResource("sonia/scm/api/v2/group-test-update-concurrent-modification.json");
+    byte[] groupJson = Resources.toByteArray(url);
+
+    MockHttpRequest request = MockHttpRequest
+      .put("/" + GroupRootResource.GROUPS_PATH_V2 + "admin")
+      .contentType(VndMediaType.GROUP)
+      .content(groupJson);
+
+    MockHttpResponse response = new MockHttpResponse();
+
+    dispatcher.invoke(request, response);
+
+    assertEquals(HttpServletResponse.SC_CONFLICT, response.getStatus());
+  }
+
   @Test
   public void shouldDeleteGroup() throws URISyntaxException {
     Group group = createDummyGroup();
@@ -216,6 +236,7 @@ public class GroupRootResourceTest {
     group.setName("admin");
     group.setCreationDate(0L);
     group.setMembers(Collections.singletonList("user"));
+    group.setLastModified(1234L);
     return group;
   }
 }

scm-webapp/src/test/resources/sonia/scm/api/v2/group-test-update-concurrent-modification.json

@@ -0,0 +1,13 @@
+{
+  "description": "Updated description",
+  "name": "admin",
+  "type": "xml",
+  "_embedded": {
+    "members": [
+      {
+        "name": "user"
+      }
+    ]
+  },
+  "lastModified": "1970-01-01T00:00:00.00Z"
+}