Merge pull request #608 from LukasLohoff/baseservice-update-fix

breaking: fix serialization issues in BaseService#update and use `JsonPatch` for `updatePartial`

shogun-lib/src/main/java/de/terrestris/shogun/lib/controller/BaseController.java

@@ -16,15 +16,11 @@
  */
 package de.terrestris.shogun.lib.controller;
 
+import com.github.fge.jsonpatch.mergepatch.JsonMergePatch;
 import de.terrestris.shogun.lib.controller.security.permission.BasePermissionController;
 import de.terrestris.shogun.lib.model.BaseEntity;
 import de.terrestris.shogun.lib.service.BaseService;
 import lombok.extern.log4j.Log4j2;
-
-import java.time.OffsetDateTime;
-import java.util.List;
-import java.util.Map;
-import java.util.Optional;
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.context.MessageSource;
 import org.springframework.context.i18n.LocaleContextHolder;
@@ -37,6 +33,11 @@
 import org.springframework.web.bind.annotation.*;
 import org.springframework.web.server.ResponseStatusException;
 
+import java.time.OffsetDateTime;
+import java.util.List;
+import java.util.Optional;
+
+// TODO Specify and type extension of BaseService
 @Log4j2
 public abstract class BaseController<T extends BaseService<?, S>, S extends BaseEntity> extends BasePermissionController<T, S> {
 
@@ -535,26 +536,15 @@ public S update(@RequestBody S entity, @PathVariable("id") Long entityId) {
         }
     }
 
-    @PatchMapping("/{id}")
+    @PatchMapping(value = "/{id}")
     @ResponseStatus(HttpStatus.OK)
-    public S updatePartial(@RequestBody Map<String, Object> values, @PathVariable("id") Long entityId) {
-        log.trace("Requested to partially update entity of type {} with ID {} ({})", getGenericClassName(), entityId, values);
+    public S updatePartial(@RequestBody JsonMergePatch patch, @PathVariable("id") Long entityId) {
+        log.trace("Requested to partially update entity of type {} with ID {} ({})", getGenericClassName(), entityId, patch);
 
         try {
-            Object idFromValues = values.get("id");
-            if (idFromValues == null) {
-                log.error("Field 'id' (entity {})is missing in the passed values: {}.", entityId, values);
-                throw new ResponseStatusException(HttpStatus.BAD_REQUEST);
-            }
-            Long id = Long.valueOf((Integer) idFromValues);
-            if (!entityId.equals(id)) {
-                log.error("IDs of update candidate (ID: {}) and update data ({}) don't match.", entityId, values);
-                throw new ResponseStatusException(HttpStatus.BAD_REQUEST);
-            }
-
             S persistedEntity = service.findOne(entityId).orElseThrow();
             if (persistedEntity != null) {
-                S updatedEntity = service.updatePartial(entityId, persistedEntity, values);
+                S updatedEntity = service.updatePartial(persistedEntity, patch);
 
                 log.trace("Successfully updated values for entity of type {} with ID {}",
                     getGenericClassName(), entityId);
@@ -588,7 +578,7 @@ public S updatePartial(@RequestBody Map<String, Object> values, @PathVariable("i
             );
         } catch (NumberFormatException nfe) {
             log.error("Can't parse 'id' field ({}) from values ({}). It has to be an Integer.: {}",
-                values, entityId, nfe.getMessage());
+                patch, entityId, nfe.getMessage());
             throw new ResponseStatusException(HttpStatus.BAD_REQUEST);
         } catch (ResponseStatusException rse) {
             throw rse;

shogun-lib/src/main/java/de/terrestris/shogun/lib/model/Application.java

@@ -25,6 +25,7 @@
 import org.hibernate.Hibernate;
 import org.hibernate.annotations.Cache;
 import org.hibernate.annotations.CacheConcurrencyStrategy;
+import org.hibernate.annotations.DynamicUpdate;
 import org.hibernate.annotations.Type;
 import org.hibernate.envers.AuditTable;
 import org.hibernate.envers.Audited;
@@ -35,6 +36,7 @@
 
 @Entity(name = "applications")
 @Table(schema = "shogun")
+@DynamicUpdate
 @Audited
 @AuditTable(value = "applications_rev", schema = "shogun_rev")
 @Cacheable

shogun-lib/src/main/java/de/terrestris/shogun/lib/model/File.java

@@ -22,6 +22,7 @@
 import org.hibernate.Hibernate;
 import org.hibernate.annotations.Cache;
 import org.hibernate.annotations.CacheConcurrencyStrategy;
+import org.hibernate.annotations.DynamicUpdate;
 import org.hibernate.annotations.Type;
 
 import javax.persistence.*;
@@ -30,6 +31,7 @@
 
 @Entity(name = "files")
 @Table(schema = "shogun")
+@DynamicUpdate
 @Cacheable
 @Cache(usage = CacheConcurrencyStrategy.READ_WRITE, region = "files")
 @AllArgsConstructor

shogun-lib/src/main/java/de/terrestris/shogun/lib/model/Group.java

@@ -21,6 +21,7 @@
 import org.hibernate.Hibernate;
 import org.hibernate.annotations.Cache;
 import org.hibernate.annotations.CacheConcurrencyStrategy;
+import org.hibernate.annotations.DynamicUpdate;
 import org.hibernate.envers.AuditTable;
 import org.hibernate.envers.Audited;
 
@@ -29,6 +30,7 @@
 
 @Entity(name = "groups")
 @Table(schema = "shogun")
+@DynamicUpdate
 @Audited
 @AuditTable(value = "groups_rev", schema = "shogun_rev")
 @Cacheable

shogun-lib/src/main/java/de/terrestris/shogun/lib/model/ImageFile.java

@@ -20,6 +20,7 @@
 import io.swagger.v3.oas.annotations.media.Schema;
 import lombok.*;
 import org.hibernate.Hibernate;
+import org.hibernate.annotations.DynamicUpdate;
 
 import javax.persistence.Cacheable;
 import javax.persistence.Column;
@@ -29,6 +30,7 @@
 
 @Entity(name = "imagefiles")
 @Table(schema = "shogun")
+@DynamicUpdate
 @Cacheable
 @Getter
 @Setter

shogun-lib/src/main/java/de/terrestris/shogun/lib/model/Layer.java

@@ -25,6 +25,7 @@
 import org.hibernate.Hibernate;
 import org.hibernate.annotations.Cache;
 import org.hibernate.annotations.CacheConcurrencyStrategy;
+import org.hibernate.annotations.DynamicUpdate;
 import org.hibernate.annotations.Type;
 import org.hibernate.envers.AuditTable;
 import org.hibernate.envers.Audited;
@@ -34,6 +35,7 @@
 
 @Entity(name = "layers")
 @Table(schema = "shogun")
+@DynamicUpdate
 @Audited
 @AuditTable(value = "layers_rev", schema = "shogun_rev")
 @Cacheable

shogun-lib/src/main/java/de/terrestris/shogun/lib/model/User.java

@@ -23,6 +23,7 @@
 import org.hibernate.Hibernate;
 import org.hibernate.annotations.Cache;
 import org.hibernate.annotations.CacheConcurrencyStrategy;
+import org.hibernate.annotations.DynamicUpdate;
 import org.hibernate.annotations.Type;
 import org.hibernate.envers.AuditTable;
 import org.hibernate.envers.Audited;
@@ -32,6 +33,7 @@
 
 @Entity(name = "users")
 @Table(schema = "shogun")
+@DynamicUpdate
 @Audited
 @AuditTable(value = "users_rev", schema = "shogun_rev")
 @Cacheable

shogun-lib/src/main/java/de/terrestris/shogun/lib/model/security/permission/GroupClassPermission.java

@@ -21,6 +21,7 @@
 import org.hibernate.Hibernate;
 import org.hibernate.annotations.Cache;
 import org.hibernate.annotations.CacheConcurrencyStrategy;
+import org.hibernate.annotations.DynamicUpdate;
 import org.hibernate.envers.AuditTable;
 import org.hibernate.envers.Audited;
 
@@ -32,6 +33,7 @@
 
 @Entity(name = "groupclasspermissions")
 @Table(schema = "shogun")
+@DynamicUpdate
 @Audited
 @AuditTable(value = "groupclasspermissions_rev", schema = "shogun_rev")
 @Cacheable

shogun-lib/src/main/java/de/terrestris/shogun/lib/model/security/permission/GroupInstancePermission.java

@@ -21,6 +21,7 @@
 import org.hibernate.Hibernate;
 import org.hibernate.annotations.Cache;
 import org.hibernate.annotations.CacheConcurrencyStrategy;
+import org.hibernate.annotations.DynamicUpdate;
 import org.hibernate.envers.AuditTable;
 import org.hibernate.envers.Audited;
 
@@ -32,6 +33,7 @@
 
 @Entity(name = "groupinstancepermissions")
 @Table(schema = "shogun")
+@DynamicUpdate
 @Audited
 @AuditTable(value = "groupinstancepermissions_rev", schema = "shogun_rev")
 @Cacheable

shogun-lib/src/main/java/de/terrestris/shogun/lib/model/security/permission/PermissionCollection.java

@@ -24,17 +24,18 @@
 import lombok.*;
 import org.hibernate.Hibernate;
 import org.hibernate.annotations.Cache;
-import org.hibernate.annotations.CacheConcurrencyStrategy;
-import org.hibernate.annotations.Fetch;
-import org.hibernate.annotations.FetchMode;
+import org.hibernate.annotations.*;
 
+import javax.persistence.Entity;
+import javax.persistence.Table;
 import javax.persistence.*;
 import java.util.HashSet;
 import java.util.Objects;
 import java.util.Set;
 
 @Entity(name = "permissions")
 @Table(schema = "shogun")
+@DynamicUpdate
 @Cacheable
 @Cache(usage = CacheConcurrencyStrategy.READ_WRITE, region = "permissions")
 @Getter

shogun-lib/src/main/java/de/terrestris/shogun/lib/model/security/permission/UserClassPermission.java

@@ -21,6 +21,7 @@
 import org.hibernate.Hibernate;
 import org.hibernate.annotations.Cache;
 import org.hibernate.annotations.CacheConcurrencyStrategy;
+import org.hibernate.annotations.DynamicUpdate;
 import org.hibernate.envers.AuditTable;
 import org.hibernate.envers.Audited;
 
@@ -32,6 +33,7 @@
 
 @Entity(name = "userclasspermissions")
 @Table(schema = "shogun")
+@DynamicUpdate
 @Audited
 @AuditTable(value = "userclasspermissions_rev", schema = "shogun_rev")
 @Cacheable

shogun-lib/src/main/java/de/terrestris/shogun/lib/model/security/permission/UserInstancePermission.java

@@ -21,6 +21,7 @@
 import org.hibernate.Hibernate;
 import org.hibernate.annotations.Cache;
 import org.hibernate.annotations.CacheConcurrencyStrategy;
+import org.hibernate.annotations.DynamicUpdate;
 import org.hibernate.envers.AuditTable;
 import org.hibernate.envers.Audited;
 
@@ -32,6 +33,7 @@
 
 @Entity(name = "userinstancepermissions")
 @Table(schema = "shogun")
+@DynamicUpdate
 @Audited
 @AuditTable(value = "userinstancepermissions_rev", schema = "shogun_rev")
 @Cacheable

shogun-lib/src/main/java/de/terrestris/shogun/lib/service/BaseService.java

@@ -18,7 +18,8 @@
 
 import com.fasterxml.jackson.databind.JsonNode;
 import com.fasterxml.jackson.databind.ObjectMapper;
-import com.fasterxml.jackson.databind.node.ObjectNode;
+import com.github.fge.jsonpatch.JsonPatchException;
+import com.github.fge.jsonpatch.mergepatch.JsonMergePatch;
 import de.terrestris.shogun.lib.enumeration.PermissionCollectionType;
 import de.terrestris.shogun.lib.model.BaseEntity;
 import de.terrestris.shogun.lib.model.User;
@@ -27,7 +28,6 @@
 import de.terrestris.shogun.lib.service.security.permission.UserInstancePermissionService;
 import de.terrestris.shogun.lib.service.security.provider.UserProviderService;
 import lombok.extern.log4j.Log4j2;
-import org.apache.commons.lang3.ObjectUtils;
 import org.hibernate.envers.AuditReader;
 import org.hibernate.envers.query.AuditEntity;
 import org.springframework.beans.factory.annotation.Autowired;
@@ -46,7 +46,6 @@
 import java.io.IOException;
 import java.time.OffsetDateTime;
 import java.util.List;
-import java.util.Map;
 import java.util.Optional;
 
 @Log4j2
@@ -134,27 +133,19 @@ public S create(S entity) {
     public S update(Long id, S entity) throws IOException {
         Optional<S> persistedEntityOpt = repository.findById(id);
 
-        ObjectNode jsonObject = objectMapper.valueToTree(entity);
-
         // Ensure the created timestamp will not be overridden.
         S persistedEntity = persistedEntityOpt.orElseThrow();
-        OffsetDateTime createdTimestamp = persistedEntity.getCreated();
-        String serialized = createdTimestamp != null ? createdTimestamp.toInstant().toString() : null;
-        jsonObject.put("created", serialized);
-
-        S updatedEntity = objectMapper.readerForUpdating(persistedEntity).readValue(jsonObject);
+        entity.setCreated(persistedEntity.getCreated());
 
-        return repository.save(updatedEntity);
+        return repository.save(entity);
     }
 
     @PreAuthorize("hasRole('ROLE_ADMIN') or hasPermission(#entity, 'UPDATE')")
     @Transactional(isolation = Isolation.SERIALIZABLE)
-    public S updatePartial(Long entityId, S entity, Map<String, Object> values) throws IOException {
-        if (ObjectUtils.notEqual(entityId, entity.getId())) {
-            throw new IOException("ID's of passed entity and parameter do not match. No partial update possible");
-        }
-        JsonNode jsonObject = objectMapper.valueToTree(values);
-        S updatedEntity = objectMapper.readerForUpdating(entity).readValue(jsonObject);
+    public S updatePartial(S entity, JsonMergePatch patch) throws IOException, JsonPatchException {
+        JsonNode entityNode = objectMapper.valueToTree(entity);
+        JsonNode patchedEntityNode = patch.apply(entityNode);
+        S updatedEntity = objectMapper.readerForUpdating(entity).readValue(patchedEntityNode);
         return repository.save(updatedEntity);
     }
 

shogun-lib/src/test/java/de/terrestris/shogun/lib/service/BaseServiceTest.java

@@ -178,10 +178,6 @@ public void update_ShouldCallCorrectRepositoryMethodAndShouldReturnTheCreatedEnt
         when(baseCrudRepositoryMock.findById(1909L)).thenReturn(Optional.of(mockEntity));
         when(baseCrudRepositoryMock.save(mockEntity)).thenReturn(mockEntity);
 
-        when(objectMapperMock.valueToTree(mockEntity)).thenReturn(returnNode);
-        when(objectMapperMock.readerForUpdating(mockEntity)).thenReturn(objectReaderMock);
-        when(objectReaderMock.readValue(returnNode)).thenReturn(mockEntity);
-
         S returnValue = (S) service.update(1909L, mockEntity);
 
         verify(baseCrudRepositoryMock, times(1)).findById(1909L);