66075: Feedback improvements 2:

DspaceObjectMetadataOperation
 - JsonPatchConverter removed
 - SQL exceptions thrown as is, others to DSpaceBadRequestException
 - DspaceObjectMetadataOperation.remove()
    > UnprocessableEntityException with exceptionalities
    > Already parsed value used in second parse

PatchOperation
 - M no longer extends DSO
 - supports in child classes with R objectToMatch

ItemWithdrawReplaceOperation
 - one try-catch block with relevant exception handling

dspace-server-webapp/src/main/java/org/dspace/app/rest/repository/patch/factories/impl/DspaceObjectMetadataOperation.java

@@ -16,7 +16,6 @@ import com.fasterxml.jackson.databind.JsonNode;
 import com.fasterxml.jackson.databind.ObjectMapper;
 import org.apache.commons.lang3.StringUtils;
 import org.apache.logging.log4j.Logger;
-import org.dspace.app.rest.converter.JsonPatchConverter;
 import org.dspace.app.rest.exception.DSpaceBadRequestException;
 import org.dspace.app.rest.exception.UnprocessableEntityException;
 import org.dspace.app.rest.model.patch.CopyOperation;
@@ -36,32 +35,32 @@ import org.springframework.stereotype.Component;
  * Options (can be done on other dso than Item also):
  *      - ADD metadata (with schema.identifier.qualifier) value of a dso (here: Item)
  *          <code>
- *              curl -X PATCH http://${dspace.url}/api/items/<:id-item> -H "
+ *              curl -X PATCH http://${dspace.url}/api/core/items/<:id-item> -H "
  *              Content-Type: application/json" -d '[{ "op": "add", "path": "
  *              /metadata/schema.identifier.qualifier(/0|-)}", "value": "metadataValue"]'
  *          </code>
  *      - REMOVE metadata
  *          <code>
- *              curl -X PATCH http://${dspace.url}/api/items/<:id-item> -H "
+ *              curl -X PATCH http://${dspace.url}/api/core/items/<:id-item> -H "
  *              Content-Type: application/json" -d '[{ "op": "remove",
  *              "path": "/metadata/schema.identifier.qualifier(/0|-)}"]'
  *          </code>
  *      - REPLACE metadata
  *          <code>
- *              curl -X PATCH http://${dspace.url}/api/items/<:id-item> -H "
+ *              curl -X PATCH http://${dspace.url}/api/core/items/<:id-item> -H "
  *              Content-Type: application/json" -d '[{ "op": "replace", "path": "
  *              /metadata/schema.identifier.qualifier}", "value": "metadataValue"]'
  *          </code>
  *      - ORDERING metadata
  *          <code>
- *              curl -X PATCH http://${dspace.url}/api/items/<:id-item> -H "
+ *              curl -X PATCH http://${dspace.url}/api/core/items/<:id-item> -H "
  *              Content-Type: application/json" -d '[{ "op": "move",
  *              "from": "/metadata/schema.identifier.qualifier/index"
  *              "path": "/metadata/schema.identifier.qualifier/newIndex"}]'
  *          </code>
  *      - COPY metadata
  *          <code>
- *              curl -X PATCH http://${dspace.url}/api/items/<:id-item> -H "
+ *              curl -X PATCH http://${dspace.url}/api/core/items/<:id-item> -H "
  *              Content-Type: application/json" -d '[{ "op": "copy",
  *              "from": "/metadata/schema.identifier.qualifier/indexToCopyFrom"
  *              "path": "/metadata/schema.identifier.qualifier/-"}]'
@@ -80,7 +79,6 @@ public class DspaceObjectMetadataOperation<R extends DSpaceObject> extends Patch
     private static final String METADATA_PATH = "/metadata";
 
     private ObjectMapper objectMapper = new ObjectMapper();
-    private JsonPatchConverter jsonPatchConverter = new JsonPatchConverter(objectMapper);
 
     /**
      * Implements the patch operation for metadata operations.
@@ -160,8 +158,8 @@ public class DspaceObjectMetadataOperation<R extends DSpaceObject> extends Patch
                     );
             }
         } catch (IOException e) {
-            log.error("IOException in DspaceObjectMetadataOperation.performPatchOperation trying " +
-                    "to map json from operation.value to MetadataValue class.", e);
+            throw new DSpaceBadRequestException("IOException in DspaceObjectMetadataOperation.performPatchOperation" +
+                    " trying to map json from operation.value to MetadataValue class.", e);
         }
     }
 
@@ -188,7 +186,8 @@ public class DspaceObjectMetadataOperation<R extends DSpaceObject> extends Patch
                     metadataValue.getLanguage(), metadataValue.getValue(),
                     metadataValue.getAuthority(), metadataValue.getConfidence(), indexInt);
         } catch (SQLException e) {
-            log.error("SQLException in DspaceObjectMetadataOperation.add trying to add metadata to dso.", e);
+            throw new DSpaceBadRequestException("SQLException in DspaceObjectMetadataOperation.add trying to add " +
+                    "metadata to dso.", e);
         }
     }
 
@@ -222,17 +221,18 @@ public class DspaceObjectMetadataOperation<R extends DSpaceObject> extends Patch
                         && metadataValues.get(indexInt) != null) {
                     //remove that metadata
                     dsoService.removeMetadataValues(context, dso,
-                            Arrays.asList(metadataValues.get(Integer.parseInt(index))));
+                            Arrays.asList(metadataValues.get(indexInt)));
                 } else {
-                    throw new UnprocessableEntityException("There is no metadata of this type at that index");
+                    throw new UnprocessableEntityException("UnprocessableEntityException - There is no metadata of " +
+                            "this type at that index");
                 }
             } catch (NumberFormatException e) {
                 throw new IllegalArgumentException("This index (" + index + ") is not valid nr", e);
             } catch (ArrayIndexOutOfBoundsException e) {
                 throw new UnprocessableEntityException("There is no metadata of this type at that index");
             } catch (SQLException e) {
-                log.error("SQLException in DspaceObjectMetadataOperation.remove trying to remove " +
-                        "metadata from dso.", e);
+                throw new DSpaceBadRequestException("SQLException in DspaceObjectMetadataOperation.remove trying to " +
+                        "remove metadata from dso.", e);
             }
         }
     }
@@ -261,8 +261,8 @@ public class DspaceObjectMetadataOperation<R extends DSpaceObject> extends Patch
             try {
                 dsoService.clearMetadata(context, dso, Item.ANY, Item.ANY, Item.ANY, Item.ANY);
             } catch (SQLException e) {
-                log.error("SQLException in DspaceObjectMetadataOperation.replace trying to remove" +
-                        "and replace metadata from dso.", e);
+                throw new DSpaceBadRequestException("SQLException in DspaceObjectMetadataOperation.replace trying to " +
+                        "remove and replace metadata from dso.", e);
             }
         }
         // replace all metadata for existing key
@@ -271,8 +271,8 @@ public class DspaceObjectMetadataOperation<R extends DSpaceObject> extends Patch
                 dsoService.clearMetadata(context, dso, schema, element, qualifier, Item.ANY);
                 this.add(context, dso, dsoService, schema, element, qualifier, metadataValue, null);
             } catch (SQLException e) {
-                log.error("SQLException in DspaceObjectMetadataOperation.replace trying to remove " +
-                        "and replace metadata from dso.", e);
+                throw new DSpaceBadRequestException("SQLException in DspaceObjectMetadataOperation.replace trying to " +
+                        "remove and replace metadata from dso.", e);
             }
         }
         // replace single existing metadata value
@@ -344,7 +344,8 @@ public class DspaceObjectMetadataOperation<R extends DSpaceObject> extends Patch
             dsoService.moveMetadata(context, dso, schema, element, qualifier,
                     Integer.parseInt(indexFrom), Integer.parseInt(indexTo));
         } catch (SQLException e) {
-            log.error("SQLException in DspaceObjectMetadataOperation.move trying to move metadata in dso.", e);
+            throw new DSpaceBadRequestException("SQLException in DspaceObjectMetadataOperation.move trying to move " +
+                    "metadata in dso.", e);
         }
     }
 
@@ -378,7 +379,8 @@ public class DspaceObjectMetadataOperation<R extends DSpaceObject> extends Patch
     }
 
     @Override
-    public boolean supports(DSpaceObject R, String path) {
-        return ((path.startsWith(METADATA_PATH) || path.equals(METADATA_PATH)) && R instanceof DSpaceObject);
+    public boolean supports(R objectToMatch, String path) {
+        return ((path.startsWith(METADATA_PATH) || path.equals(METADATA_PATH))
+                && objectToMatch instanceof DSpaceObject);
     }
 }

dspace-server-webapp/src/main/java/org/dspace/app/rest/repository/patch/factories/impl/EPersonCertificateReplaceOperation.java

@@ -9,7 +9,6 @@ package org.dspace.app.rest.repository.patch.factories.impl;
 
 import org.dspace.app.rest.exception.DSpaceBadRequestException;
 import org.dspace.app.rest.model.patch.Operation;
-import org.dspace.content.DSpaceObject;
 import org.dspace.core.Context;
 import org.dspace.eperson.EPerson;
 import org.springframework.stereotype.Component;
@@ -24,7 +23,7 @@ import org.springframework.stereotype.Component;
  * </code>
  */
 @Component
-public class EPersonCertificateReplaceOperation extends PatchOperation<EPerson> {
+public class EPersonCertificateReplaceOperation<R> extends PatchOperation<R> {
 
     /**
      * Path in json body of patch that uses this operation
@@ -32,26 +31,20 @@ public class EPersonCertificateReplaceOperation extends PatchOperation<EPerson>
     private static final String OPERATION_PATH_CERTIFICATE = "/certificate";
 
     @Override
-    public EPerson perform(Context context, EPerson eperson, Operation operation) {
+    public R perform(Context context, R object, Operation operation) {
         checkOperationValue(operation.getValue());
-        checkModelForExistingValue(eperson);
         Boolean requireCert = getBooleanOperationValue(operation.getValue());
-        eperson.setRequireCertificate(requireCert);
-        return eperson;
-
-    }
-
-    void checkModelForExistingValue(EPerson resource) {
-        // TODO: many (all?) boolean values on the rest model should never be null.
-        // So perhaps the error to throw in this case is different...IllegalStateException?
-        // Or perhaps do nothing (no check is required).
-        if ((Object) resource.getRequireCertificate() == null) {
-            throw new DSpaceBadRequestException("Attempting to replace a non-existent value.");
+        if (supports(object, operation.getPath())) {
+            EPerson eperson = (EPerson) object;
+            eperson.setRequireCertificate(requireCert);
+            return object;
+        } else {
+            throw new DSpaceBadRequestException("EPersonCertificateReplaceOperation does not support this operation.");
         }
     }
 
     @Override
-    public boolean supports(DSpaceObject R, String path) {
-        return (R instanceof EPerson && path.trim().equalsIgnoreCase(OPERATION_PATH_CERTIFICATE));
+    public boolean supports(R objectToMatch, String path) {
+        return (objectToMatch instanceof EPerson && path.trim().equalsIgnoreCase(OPERATION_PATH_CERTIFICATE));
     }
 }

dspace-server-webapp/src/main/java/org/dspace/app/rest/repository/patch/factories/impl/EPersonEmailReplaceOperation.java

@@ -9,7 +9,6 @@ package org.dspace.app.rest.repository.patch.factories.impl;
 
 import org.dspace.app.rest.exception.DSpaceBadRequestException;
 import org.dspace.app.rest.model.patch.Operation;
-import org.dspace.content.DSpaceObject;
 import org.dspace.core.Context;
 import org.dspace.eperson.EPerson;
 import org.springframework.stereotype.Component;
@@ -24,7 +23,7 @@ import org.springframework.stereotype.Component;
  * </code>
  */
 @Component
-public class EPersonEmailReplaceOperation extends PatchOperation<EPerson> {
+public class EPersonEmailReplaceOperation<R> extends PatchOperation<R> {
 
     /**
      * Path in json body of patch that uses this operation
@@ -32,21 +31,19 @@ public class EPersonEmailReplaceOperation extends PatchOperation<EPerson> {
     private static final String OPERATION_PATH_EMAIL = "/email";
 
     @Override
-    public EPerson perform(Context context, EPerson eperson, Operation operation) {
+    public R perform(Context context, R object, Operation operation) {
         checkOperationValue(operation.getValue());
-        checkModelForExistingValue(eperson);
-        eperson.setEmail((String) operation.getValue());
-        return eperson;
-    }
-
-    void checkModelForExistingValue(EPerson resource) {
-        if (resource.getEmail() == null) {
-            throw new DSpaceBadRequestException("Attempting to replace a non-existent value.");
+        if (supports(object, operation.getPath())) {
+            EPerson eperson = (EPerson) object;
+            eperson.setEmail((String) operation.getValue());
+            return object;
+        } else {
+            throw new DSpaceBadRequestException("EPersonEmailReplaceOperation does not support this operation");
         }
     }
 
     @Override
-    public boolean supports(DSpaceObject R, String path) {
-        return (R instanceof EPerson && path.trim().equalsIgnoreCase(OPERATION_PATH_EMAIL));
+    public boolean supports(R objectToMatch, String path) {
+        return (objectToMatch instanceof EPerson && path.trim().equalsIgnoreCase(OPERATION_PATH_EMAIL));
     }
 }

dspace-server-webapp/src/main/java/org/dspace/app/rest/repository/patch/factories/impl/EPersonLoginReplaceOperation.java

@@ -9,7 +9,6 @@ package org.dspace.app.rest.repository.patch.factories.impl;
 
 import org.dspace.app.rest.exception.DSpaceBadRequestException;
 import org.dspace.app.rest.model.patch.Operation;
-import org.dspace.content.DSpaceObject;
 import org.dspace.core.Context;
 import org.dspace.eperson.EPerson;
 import org.springframework.stereotype.Component;
@@ -24,7 +23,7 @@ import org.springframework.stereotype.Component;
  * </code>
  */
 @Component
-public class EPersonLoginReplaceOperation extends PatchOperation<EPerson> {
+public class EPersonLoginReplaceOperation<R>extends PatchOperation<R> {
 
     /**
      * Path in json body of patch that uses this operation
@@ -32,22 +31,20 @@ public class EPersonLoginReplaceOperation extends PatchOperation<EPerson> {
     private static final String OPERATION_PATH_PASSWORD = "/canLogin";
 
     @Override
-    public EPerson perform(Context context, EPerson eperson, Operation operation) {
+    public R perform(Context context, R object, Operation operation) {
         checkOperationValue(operation.getValue());
-        checkModelForExistingValue(eperson);
         Boolean canLogin = getBooleanOperationValue(operation.getValue());
-        eperson.setCanLogIn(canLogin);
-        return eperson;
-    }
-
-    void checkModelForExistingValue(EPerson resource) {
-        if ((Object) resource.canLogIn() == null) {
-            throw new DSpaceBadRequestException("Attempting to replace a non-existent value.");
+        if (supports(object, operation.getPath())) {
+            EPerson eperson = (EPerson) object;
+            eperson.setCanLogIn(canLogin);
+            return object;
+        } else {
+            throw new DSpaceBadRequestException("EPersonLoginReplaceOperation does not support this operation");
         }
     }
 
     @Override
-    public boolean supports(DSpaceObject R, String path) {
-        return (R instanceof EPerson && path.trim().equalsIgnoreCase(OPERATION_PATH_PASSWORD));
+    public boolean supports(R objectToMatch, String path) {
+        return (objectToMatch instanceof EPerson && path.trim().equalsIgnoreCase(OPERATION_PATH_PASSWORD));
     }
 }

dspace-server-webapp/src/main/java/org/dspace/app/rest/repository/patch/factories/impl/EPersonNetidReplaceOperation.java

@@ -9,7 +9,6 @@ package org.dspace.app.rest.repository.patch.factories.impl;
 
 import org.dspace.app.rest.exception.DSpaceBadRequestException;
 import org.dspace.app.rest.model.patch.Operation;
-import org.dspace.content.DSpaceObject;
 import org.dspace.core.Context;
 import org.dspace.eperson.EPerson;
 import org.springframework.stereotype.Component;
@@ -24,7 +23,7 @@ import org.springframework.stereotype.Component;
  * </code>
  */
 @Component
-public class EPersonNetidReplaceOperation extends PatchOperation<EPerson> {
+public class EPersonNetidReplaceOperation<R> extends PatchOperation<R> {
 
     /**
      * Path in json body of patch that uses this operation
@@ -32,21 +31,19 @@ public class EPersonNetidReplaceOperation extends PatchOperation<EPerson> {
     private static final String OPERATION_PATH_NETID = "/netid";
 
     @Override
-    public EPerson perform(Context context, EPerson eperson, Operation operation) {
+    public R perform(Context context, R object, Operation operation) {
         checkOperationValue(operation.getValue());
-        checkModelForExistingValue(eperson);
-        eperson.setNetid((String) operation.getValue());
-        return eperson;
-    }
-
-    void checkModelForExistingValue(EPerson resource) {
-        if (resource.getNetid() == null) {
-            throw new DSpaceBadRequestException("Attempting to replace a non-existent value.");
+        if (supports(object, operation.getPath())) {
+            EPerson eperson = (EPerson) object;
+            eperson.setNetid((String) operation.getValue());
+            return object;
+        } else {
+            throw new DSpaceBadRequestException("EPersonNetidReplaceOperation does not support this operation");
         }
     }
 
     @Override
-    public boolean supports(DSpaceObject R, String path) {
-        return (R instanceof EPerson && path.trim().equalsIgnoreCase(OPERATION_PATH_NETID));
+    public boolean supports(R objectToMatch, String path) {
+        return (objectToMatch instanceof EPerson && path.trim().equalsIgnoreCase(OPERATION_PATH_NETID));
     }
 }

dspace-server-webapp/src/main/java/org/dspace/app/rest/repository/patch/factories/impl/EPersonPasswordReplaceOperation.java

@@ -7,8 +7,8 @@
  */
 package org.dspace.app.rest.repository.patch.factories.impl;
 
+import org.dspace.app.rest.exception.DSpaceBadRequestException;
 import org.dspace.app.rest.model.patch.Operation;
-import org.dspace.content.DSpaceObject;
 import org.dspace.core.Context;
 import org.dspace.eperson.EPerson;
 import org.dspace.eperson.factory.EPersonServiceFactory;
@@ -25,7 +25,7 @@ import org.springframework.stereotype.Component;
  * </code>
  */
 @Component
-public class EPersonPasswordReplaceOperation extends PatchOperation<EPerson> {
+public class EPersonPasswordReplaceOperation<R> extends PatchOperation<R> {
 
     /**
      * Path in json body of patch that uses this operation
@@ -34,25 +34,19 @@ public class EPersonPasswordReplaceOperation extends PatchOperation<EPerson> {
     protected EPersonService ePersonService = EPersonServiceFactory.getInstance().getEPersonService();
 
     @Override
-    public EPerson perform(Context context, EPerson eperson, Operation operation) {
+    public R perform(Context context, R object, Operation operation) {
         checkOperationValue(operation.getValue());
-        checkModelForExistingValue(eperson);
-        ePersonService.setPassword(eperson, (String) operation.getValue());
-        return eperson;
-    }
-
-    void checkModelForExistingValue(EPerson resource) {
-        /*
-         * FIXME: the password field in eperson rest model is always null because
-         * the value is not set in the rest converter.
-         * We would normally throw an exception here since replace
-         * operations are not allowed on non-existent values, but that
-         * would prevent the password update from ever taking place.
-         */
+        if (supports(object, operation.getPath())) {
+            EPerson eperson = (EPerson) object;
+            ePersonService.setPassword(eperson, (String) operation.getValue());
+            return object;
+        } else {
+            throw new DSpaceBadRequestException("EPersonPasswordReplaceOperation does not support this operation");
+        }
     }
 
     @Override
-    public boolean supports(DSpaceObject R, String path) {
-        return (R instanceof EPerson && path.trim().equalsIgnoreCase(OPERATION_PASSWORD_CHANGE));
+    public boolean supports(R objectToMatch, String path) {
+        return (objectToMatch instanceof EPerson && path.trim().equalsIgnoreCase(OPERATION_PASSWORD_CHANGE));
     }
 }

dspace-server-webapp/src/main/java/org/dspace/app/rest/repository/patch/factories/impl/ItemDiscoverableReplaceOperation.java

@@ -9,7 +9,6 @@ package org.dspace.app.rest.repository.patch.factories.impl;
 
 import org.dspace.app.rest.exception.DSpaceBadRequestException;
 import org.dspace.app.rest.model.patch.Operation;
-import org.dspace.content.DSpaceObject;
 import org.dspace.content.Item;
 import org.dspace.core.Context;
 import org.springframework.stereotype.Component;
@@ -18,13 +17,13 @@ import org.springframework.stereotype.Component;
  * This is the implementation for Item resource patches.
  *
  * Example: <code>
- * curl -X PATCH http://${dspace.url}/api/item/<:id-item> -H "
+ * curl -X PATCH http://${dspace.url}/api/core/items/<:id-item> -H "
  * Content-Type: application/json" -d '[{ "op": "replace", "path": "
  * /discoverable", "value": true|false]'
  * </code>
  */
 @Component
-public class ItemDiscoverableReplaceOperation extends PatchOperation<Item> {
+public class ItemDiscoverableReplaceOperation<R> extends PatchOperation<R> {
 
     /**
      * Path in json body of patch that uses this operation
@@ -32,24 +31,21 @@ public class ItemDiscoverableReplaceOperation extends PatchOperation<Item> {
     private static final String OPERATION_PATH_DISCOVERABLE = "/discoverable";
 
     @Override
-    public Item perform(Context context, Item item, Operation operation) {
+    public R perform(Context context, R object, Operation operation) {
         checkOperationValue(operation.getValue());
-        checkModelForExistingValue(item);
         Boolean discoverable = getBooleanOperationValue(operation.getValue());
-        item.setDiscoverable(discoverable);
-        return item;
-
-    }
-
-    void checkModelForExistingValue(Item resource) {
-        if ((Object) resource.isDiscoverable() == null) {
-            throw new DSpaceBadRequestException("Attempting to replace a non-existent value.");
+        if (supports(object, operation.getPath())) {
+            Item item = (Item) object;
+            item.setDiscoverable(discoverable);
+            return object;
+        } else {
+            throw new DSpaceBadRequestException("ItemDiscoverableReplaceOperation does not support this operation");
         }
     }
 
     @Override
-    public boolean supports(DSpaceObject R, String path) {
-        return (R instanceof Item && path.trim().equalsIgnoreCase(OPERATION_PATH_DISCOVERABLE));
+    public boolean supports(R objectToMatch, String path) {
+        return (objectToMatch instanceof Item && path.trim().equalsIgnoreCase(OPERATION_PATH_DISCOVERABLE));
     }
 
 }

dspace-server-webapp/src/main/java/org/dspace/app/rest/repository/patch/factories/impl/ItemWithdrawReplaceOperation.java

@@ -10,10 +10,10 @@ package org.dspace.app.rest.repository.patch.factories.impl;
 import java.sql.SQLException;
 
 import org.dspace.app.rest.exception.DSpaceBadRequestException;
+import org.dspace.app.rest.exception.RESTAuthorizationException;
 import org.dspace.app.rest.exception.UnprocessableEntityException;
 import org.dspace.app.rest.model.patch.Operation;
 import org.dspace.authorize.AuthorizeException;
-import org.dspace.content.DSpaceObject;
 import org.dspace.content.Item;
 import org.dspace.content.factory.ContentServiceFactory;
 import org.dspace.content.service.ItemService;
@@ -24,13 +24,13 @@ import org.springframework.stereotype.Component;
  * This is the implementation for Item resource patches.
  * <p>
  * Example: <code>
- * curl -X PATCH http://${dspace.url}/api/item/<:id-item> -H "
+ * curl -X PATCH http://${dspace.url}/api/core/items/<:id-item> -H "
  * Content-Type: application/json" -d '[{ "op": "replace", "path": "
  * /withdrawn", "value": true|false]'
  * </code>
  */
 @Component
-public class ItemWithdrawReplaceOperation extends PatchOperation<Item> {
+public class ItemWithdrawReplaceOperation<R> extends PatchOperation<R> {
 
     /**
      * Path in json body of patch that uses this operation
@@ -39,64 +39,51 @@ public class ItemWithdrawReplaceOperation extends PatchOperation<Item> {
     private static final ItemService itemService = ContentServiceFactory.getInstance().getItemService();
 
     @Override
-    public Item perform(Context context, Item item, Operation operation) {
+    public R perform(Context context, R object, Operation operation) {
         checkOperationValue(operation.getValue());
-        checkModelForExistingValue(item);
 
         Boolean withdraw = getBooleanOperationValue(operation.getValue());
 
-        // This is a request to withdraw the item.
-        if (withdraw) {
-            // The item is currently not withdrawn and also not archived. Is this a possible situation?
-            if (!item.isWithdrawn() && !item.isArchived()) {
-                throw new UnprocessableEntityException("Cannot withdraw item when it is not in archive.");
-            }
-            // Item is already withdrawn. No-op, 200 response.
-            // (The operation is not idempotent since it results in a provenance note in the record.)
-            if (item.isWithdrawn()) {
-                return item;
-            }
+        if (supports(object, operation.getPath())) {
+            Item item = (Item) object;
+            // This is a request to withdraw the item.
             try {
-                itemService.withdraw(context, item);
-            } catch (SQLException e) {
-                // TODO
-                e.printStackTrace();
-            } catch (AuthorizeException e) {
-                // TODO
-                e.printStackTrace();
-            }
-            return item;
+                if (withdraw) {
+                    // The item is currently not withdrawn and also not archived. Is this a possible situation?
+                    if (!item.isWithdrawn() && !item.isArchived()) {
+                        throw new UnprocessableEntityException("Cannot withdraw item when it is not in archive.");
+                    }
+                    // Item is already withdrawn. No-op, 200 response.
+                    // (The operation is not idempotent since it results in a provenance note in the record.)
+                    if (item.isWithdrawn()) {
+                        return object;
+                    }
+                    itemService.withdraw(context, item);
+                    return object;
 
+                } else {
+                    // No need to reinstate item if it has not previously been not withdrawn.
+                    // No-op, 200 response. (The operation is not idempotent since it results
+                    // in a provenance note in the record.)
+                    if (!item.isWithdrawn()) {
+                        return object;
+                    }
+                    itemService.reinstate(context, item);
+                    return object;
+                }
+            } catch (AuthorizeException e) {
+                throw new RESTAuthorizationException("Unauthorized user for item withdrawal/reinstation");
+            } catch (SQLException e) {
+                throw new DSpaceBadRequestException("SQL exception during item withdrawal/reinstation");
+            }
         } else {
-            // No need to reinstate item if it has not previously been not withdrawn.
-            // No-op, 200 response. (The operation is not idempotent since it results
-            // in a provenance note in the record.)
-            if (!item.isWithdrawn()) {
-                return item;
-            }
-            try {
-                itemService.reinstate(context, item);
-            } catch (SQLException e) {
-                // TODO
-                e.printStackTrace();
-            } catch (AuthorizeException e) {
-                // TODO
-                e.printStackTrace();
-            }
-            return item;
-        }
-
-    }
-
-    void checkModelForExistingValue(Item resource) {
-        if ((Object) resource.isWithdrawn() == null) {
-            throw new DSpaceBadRequestException("Attempting to replace a non-existent value.");
+            throw new DSpaceBadRequestException("ItemWithdrawReplaceOperation does not support this operation");
         }
     }
 
     @Override
-    public boolean supports(DSpaceObject dso, String path) {
-        return (dso instanceof Item && path.trim().equalsIgnoreCase(OPERATION_PATH_WITHDRAW));
+    public boolean supports(R objectToMatch, String path) {
+        return (objectToMatch instanceof Item && path.trim().equalsIgnoreCase(OPERATION_PATH_WITHDRAW));
     }
 
 }

dspace-server-webapp/src/main/java/org/dspace/app/rest/repository/patch/factories/impl/PatchOperation.java

@@ -10,13 +10,12 @@ package org.dspace.app.rest.repository.patch.factories.impl;
 import org.apache.commons.lang3.BooleanUtils;
 import org.dspace.app.rest.exception.DSpaceBadRequestException;
 import org.dspace.app.rest.model.patch.Operation;
-import org.dspace.content.DSpaceObject;
 import org.dspace.core.Context;
 
 /**
  * Base class for all resource patch operations.
  */
-public abstract class PatchOperation<M extends DSpaceObject>
+public abstract class PatchOperation<M>
         implements ResourcePatchOperation<M> {
 
     /**
@@ -65,10 +64,11 @@ public abstract class PatchOperation<M extends DSpaceObject>
 
     /**
      * Determines whether or not this Patch Operation can do this patch (RestModel and path gets checked)
-     * @param M         dso, whose class must be instance of dso for which this PatchOperation was created
-     * @param path      Path given to the patch body, should match this type of Patch Operation
-     * @return          True if this PatchOperation class can do the patch for this given dso type and Path
+     * @param objectToMatch    Object whose class must be instance of type object
+     *                              for which this PatchOperation was created
+     * @param path             Path given to the patch body, should match this type of Patch Operation
+     * @return                 True if this PatchOperation class can do the patch for this given dso type and Path
      */
-    public abstract boolean supports(DSpaceObject M, String path);
+    public abstract boolean supports(M objectToMatch, String path);
 
 }

dspace-server-webapp/src/main/java/org/dspace/app/rest/repository/patch/factories/impl/ResourcePatchOperation.java

@@ -7,18 +7,19 @@
  */
 package org.dspace.app.rest.repository.patch.factories.impl;
 
+import java.sql.SQLException;
+
 import org.dspace.app.rest.exception.DSpaceBadRequestException;
 import org.dspace.app.rest.model.patch.Operation;
-import org.dspace.content.DSpaceObject;
 import org.dspace.core.Context;
 
 /**
  * The patch interface used by repository classes.
  * @param <M>
  */
-public interface ResourcePatchOperation<M extends DSpaceObject> {
+public interface ResourcePatchOperation<M> {
 
     M perform(Context context, M resource, Operation operation)
-            throws DSpaceBadRequestException;
+            throws DSpaceBadRequestException, SQLException;
 
 }