From d185743ef057d229fbadbba3b55dac213071037e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Pfeuffer?= Date: Thu, 25 Oct 2018 13:48:00 +0200 Subject: [PATCH] Create error response for concurrent modifications --- .../scm/ConcurrentModificationException.java | 32 ++++++++++++++++++- .../java/sonia/scm/NotFoundException.java | 8 ++--- .../sonia/scm/user/UserManagerTestBase.java | 8 ++--- ...ConcurrentModificationExceptionMapper.java | 4 ++- .../sonia/scm/api/v2/resources/ErrorDto.java | 5 +++ .../scm/api/v2/resources/GroupResource.java | 2 +- .../resources/IdResourceManagerAdapter.java | 2 +- .../api/v2/resources/RepositoryResource.java | 5 +-- .../SingleResourceManagerAdapter.java | 6 ++-- .../scm/api/v2/resources/UserResource.java | 2 +- 10 files changed, 57 insertions(+), 17 deletions(-) diff --git a/scm-core/src/main/java/sonia/scm/ConcurrentModificationException.java b/scm-core/src/main/java/sonia/scm/ConcurrentModificationException.java index f0340a6a59..f0129093a3 100644 --- a/scm-core/src/main/java/sonia/scm/ConcurrentModificationException.java +++ b/scm-core/src/main/java/sonia/scm/ConcurrentModificationException.java @@ -1,4 +1,34 @@ package sonia.scm; -public class ConcurrentModificationException extends Exception { +import java.util.Collections; +import java.util.List; + +import static java.util.Collections.unmodifiableList; +import static java.util.stream.Collectors.joining; + +public class ConcurrentModificationException extends RuntimeException { + private final List context; + + public ConcurrentModificationException(Class type, String id) { + this(Collections.singletonList(new ContextEntry(type, id))); + } + + public ConcurrentModificationException(String type, String id) { + this(Collections.singletonList(new ContextEntry(type, id))); + } + + private ConcurrentModificationException(List context) { + super(createMessage(context)); + this.context = context; + } + + public List getContext() { + return unmodifiableList(context); + } + + private static String createMessage(List context) { + return context.stream() + .map(c -> c.getType().toLowerCase() + " with id " + c.getId()) + .collect(joining(" in ", "", " has been modified concurrently")); + } } diff --git a/scm-core/src/main/java/sonia/scm/NotFoundException.java b/scm-core/src/main/java/sonia/scm/NotFoundException.java index d562a674bb..36b4d37cb1 100644 --- a/scm-core/src/main/java/sonia/scm/NotFoundException.java +++ b/scm-core/src/main/java/sonia/scm/NotFoundException.java @@ -15,14 +15,15 @@ public class NotFoundException extends RuntimeException { private final List context; public NotFoundException(Class type, String id) { - this.context = Collections.singletonList(new ContextEntry(type, id)); + this(Collections.singletonList(new ContextEntry(type, id))); } public NotFoundException(String type, String id) { - this.context = Collections.singletonList(new ContextEntry(type, id)); + this(Collections.singletonList(new ContextEntry(type, id))); } private NotFoundException(List context) { + super(createMessage(context)); this.context = context; } @@ -46,8 +47,7 @@ public class NotFoundException extends RuntimeException { return unmodifiableList(context); } - @Override - public String getMessage() { + private static String createMessage(List context) { return context.stream() .map(c -> c.getType().toLowerCase() + " with id " + c.getId()) .collect(joining(" in ", "could not find ", "")); diff --git a/scm-test/src/main/java/sonia/scm/user/UserManagerTestBase.java b/scm-test/src/main/java/sonia/scm/user/UserManagerTestBase.java index dddce344ce..9fe8669fc9 100644 --- a/scm-test/src/main/java/sonia/scm/user/UserManagerTestBase.java +++ b/scm-test/src/main/java/sonia/scm/user/UserManagerTestBase.java @@ -97,7 +97,7 @@ public abstract class UserManagerTestBase extends ManagerTestBase { } @Test(expected = NotFoundException.class) - public void testDeleteNotFound() throws Exception { + public void testDeleteNotFound() { manager.delete(UserTestData.createDent()); } @@ -181,7 +181,7 @@ public abstract class UserManagerTestBase extends ManagerTestBase { } @Test - public void testModify() throws AlreadyExistsException, NotFoundException, ConcurrentModificationException { + public void testModify() throws AlreadyExistsException { User zaphod = UserTestData.createZaphod(); manager.create(zaphod); @@ -238,7 +238,7 @@ public abstract class UserManagerTestBase extends ManagerTestBase { } @Test - public void testRefresh() throws AlreadyExistsException, NotFoundException { + public void testRefresh() throws AlreadyExistsException { User zaphod = UserTestData.createZaphod(); manager.create(zaphod); @@ -299,7 +299,7 @@ public abstract class UserManagerTestBase extends ManagerTestBase { return user; } - private void modifyAndDeleteUser(User user) throws IOException, NotFoundException, ConcurrentModificationException { + private void modifyAndDeleteUser(User user) { String name = user.getName(); String nd = name.concat(" new displayname"); diff --git a/scm-webapp/src/main/java/sonia/scm/api/rest/ConcurrentModificationExceptionMapper.java b/scm-webapp/src/main/java/sonia/scm/api/rest/ConcurrentModificationExceptionMapper.java index d01b2fd7de..49aed6a29a 100644 --- a/scm-webapp/src/main/java/sonia/scm/api/rest/ConcurrentModificationExceptionMapper.java +++ b/scm-webapp/src/main/java/sonia/scm/api/rest/ConcurrentModificationExceptionMapper.java @@ -1,6 +1,8 @@ package sonia.scm.api.rest; import sonia.scm.ConcurrentModificationException; +import sonia.scm.api.v2.resources.ErrorDto; +import sonia.scm.web.VndMediaType; import javax.ws.rs.core.Response; import javax.ws.rs.ext.ExceptionMapper; @@ -10,6 +12,6 @@ import javax.ws.rs.ext.Provider; public class ConcurrentModificationExceptionMapper implements ExceptionMapper { @Override public Response toResponse(ConcurrentModificationException exception) { - return Response.status(Response.Status.CONFLICT).build(); + return Response.status(Response.Status.CONFLICT).entity(ErrorDto.from(exception)).type(VndMediaType.ERROR_TYPE).build(); } } diff --git a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/ErrorDto.java b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/ErrorDto.java index 3ebcdc6e8e..3b43c37c5a 100644 --- a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/ErrorDto.java +++ b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/ErrorDto.java @@ -3,6 +3,7 @@ package sonia.scm.api.v2.resources; import com.fasterxml.jackson.annotation.JsonInclude; import lombok.Getter; import org.slf4j.MDC; +import sonia.scm.ConcurrentModificationException; import sonia.scm.ContextEntry; import sonia.scm.NotFoundException; @@ -32,4 +33,8 @@ public class ErrorDto { static ErrorDto from(NotFoundException notFoundException) { return new ErrorDto(MDC.get("transaction_id"), "todo", notFoundException.getContext(), notFoundException.getMessage()); } + + public static ErrorDto from(ConcurrentModificationException concurrentModificationException) { + return new ErrorDto(MDC.get("transaction_id"), "todo", concurrentModificationException.getContext(), concurrentModificationException.getMessage()); + } } diff --git a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/GroupResource.java b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/GroupResource.java index 789a9847cb..308b40b1a5 100644 --- a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/GroupResource.java +++ b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/GroupResource.java @@ -98,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 ConcurrentModificationException { + public Response update(@PathParam("id") String name, @Valid GroupDto groupDto) { return adapter.update(name, existing -> dtoToGroupMapper.map(groupDto)); } } diff --git a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/IdResourceManagerAdapter.java b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/IdResourceManagerAdapter.java index 533bc49da0..d2e4bd856a 100644 --- a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/IdResourceManagerAdapter.java +++ b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/IdResourceManagerAdapter.java @@ -39,7 +39,7 @@ class IdResourceManagerAdapter applyChanges) throws ConcurrentModificationException { + public Response update(String id, Function applyChanges) { return singleAdapter.update( loadBy(id), applyChanges, diff --git a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/RepositoryResource.java b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/RepositoryResource.java index 77df2d8e50..f8b56229b1 100644 --- a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/RepositoryResource.java +++ b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/RepositoryResource.java @@ -138,7 +138,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 ConcurrentModificationException { + public Response update(@PathParam("namespace") String namespace, @PathParam("name") String name, @Valid RepositoryDto repositoryDto) { return adapter.update( loadBy(namespace, name), existing -> processUpdate(repositoryDto, existing), @@ -204,7 +204,8 @@ public class RepositoryResource { } private Supplier loadBy(String namespace, String name) { - return () -> Optional.ofNullable(manager.get(new NamespaceAndName(namespace, name))).orElseThrow(() -> new NotFoundException(Repository.class, namespace + "/" + name)); + NamespaceAndName namespaceAndName = new NamespaceAndName(namespace, name); + return () -> Optional.ofNullable(manager.get(namespaceAndName)).orElseThrow(() -> NotFoundException.notFound(namespaceAndName).build()); } private Predicate nameAndNamespaceStaysTheSame(String namespace, String name) { diff --git a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/SingleResourceManagerAdapter.java b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/SingleResourceManagerAdapter.java index 334dc72b20..963f25eaf6 100644 --- a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/SingleResourceManagerAdapter.java +++ b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/SingleResourceManagerAdapter.java @@ -33,6 +33,7 @@ class SingleResourceManagerAdapter extends AbstractManagerResource { private final Function> errorHandler; + private final Class type; SingleResourceManagerAdapter(Manager manager, Class type) { this(manager, type, e -> Optional.empty()); @@ -44,6 +45,7 @@ class SingleResourceManagerAdapter> errorHandler) { super(manager, type); this.errorHandler = errorHandler; + this.type = type; } /** @@ -58,14 +60,14 @@ class SingleResourceManagerAdapter reader, Function applyChanges, Predicate hasSameKey) throws ConcurrentModificationException { + Response update(Supplier reader, Function applyChanges, Predicate hasSameKey) { MODEL_OBJECT existingModelObject = reader.get(); 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(); + throw new ConcurrentModificationException(type, existingModelObject.getId()); } return update(getId(existingModelObject), changedModelObject); } diff --git a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/UserResource.java b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/UserResource.java index ef4b8a6eb4..c2fbc00453 100644 --- a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/UserResource.java +++ b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/UserResource.java @@ -101,7 +101,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 ConcurrentModificationException { + public Response update(@PathParam("id") String name, @Valid UserDto userDto) { return adapter.update(name, existing -> dtoToUserMapper.map(userDto, existing.getPassword())); }