DATAREST-885, DATAREST-885 - Cleanups in JSON Patch code.

Author: odrotbohm

spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/patch/AddOperation.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2014 the original author or authors.
+ * Copyright 2014-2016 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -16,26 +16,30 @@
 package org.springframework.data.rest.webmvc.json.patch;
 
 /**
- * Operation to add a new value to the given "path".
- * Will throw a {@link PatchException} if the path is invalid or if the given value 
- * is not assignable to the given path.
+ * Operation to add a new value to the given "path". Will throw a {@link PatchException} if the path is invalid or if
+ * the given value is not assignable to the given path.
  * 
  * @author Craig Walls
+ * @author Oliver Gierke
  */
-public class AddOperation extends PatchOperation {
+class AddOperation extends PatchOperation {
 
 	/**
 	 * Constructs the add operation
+	 * 
 	 * @param path The path where the value will be added. (e.g., '/foo/bar/4')
 	 * @param value The value to add.
 	 */
 	public AddOperation(String path, Object value) {
 		super("add", path, value);
 	}
-	
+
+	/*
+	 * (non-Javadoc)
+	 * @see org.springframework.data.rest.webmvc.json.patch.PatchOperation#perform(java.lang.Object, java.lang.Class)
+	 */
 	@Override
 	<T> void perform(Object targetObject, Class<T> type) {
 		addValue(targetObject, evaluateValueFromTarget(targetObject, type));
 	}
-	
 }

spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/patch/CopyOperation.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2014 the original author or authors.
+ * Copyright 2014-2016 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -37,8 +37,9 @@
  * </p>
  * 
  * @author Craig Walls
+ * @author Oliver Gierke
  */
-public class CopyOperation extends FromOperation {
+class CopyOperation extends FromOperation {
 
 	/**
 	 * Constructs the copy operation
@@ -50,9 +51,12 @@ public CopyOperation(String path, String from) {
 		super("copy", path, from);
 	}
 
+	/*
+	 * (non-Javadoc)
+	 * @see org.springframework.data.rest.webmvc.json.patch.PatchOperation#perform(java.lang.Object, java.lang.Class)
+	 */
 	@Override
 	<T> void perform(Object target, Class<T> type) {
-		addValue(target, pathToExpression(from).getValue(target));
+		addValue(target, pathToExpression(getFrom()).getValue(target));
 	}
-
 }

spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/patch/FromOperation.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2014 the original author or authors.
+ * Copyright 2014-2016 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -16,16 +16,19 @@
 package org.springframework.data.rest.webmvc.json.patch;
 
 /**
- * Abstract base class for operations requiring a source property, such as "copy" and "move".
- * (e.g., copy <i>from</i> here to there.
+ * Abstract base class for operations requiring a source property, such as "copy" and "move". (e.g., copy <i>from</i>
+ * here to there.
+ * 
  * @author Craig Walls
+ * @author Oliver Gierke
  */
-public abstract class FromOperation extends PatchOperation {
+abstract class FromOperation extends PatchOperation {
+
+	private final String from;
 
-	protected String from;
-	
 	/**
 	 * Constructs the operation
+	 * 
 	 * @param op The name of the operation to perform. (e.g., 'copy')
 	 * @param path The operation's target path. (e.g., '/foo/bar/4')
 	 * @param from The operation's source path. (e.g., '/foo/bar/5')
@@ -34,9 +37,8 @@ public FromOperation(String op, String path, String from) {
 		super(op, path);
 		this.from = from;
 	}
-	
+
 	public String getFrom() {
 		return from;
 	}
-
 }

spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/patch/JsonLateObjectEvaluator.java

@@ -15,6 +15,9 @@
  */
 package org.springframework.data.rest.webmvc.json.patch;
 
+import lombok.NonNull;
+import lombok.RequiredArgsConstructor;
+
 import com.fasterxml.jackson.databind.JsonNode;
 import com.fasterxml.jackson.databind.ObjectMapper;
 
@@ -23,23 +26,23 @@
  * 
  * @author Craig Walls
  */
+@RequiredArgsConstructor
 class JsonLateObjectEvaluator implements LateObjectEvaluator {
 
-	private static final ObjectMapper MAPPER = new ObjectMapper();
-
-	private JsonNode valueNode;
-
-	public JsonLateObjectEvaluator(JsonNode valueNode) {
-		this.valueNode = valueNode;
-	}
+	private final @NonNull ObjectMapper mapper;
+	private final @NonNull JsonNode valueNode;
 
+	/*
+	 * (non-Javadoc)
+	 * @see org.springframework.data.rest.webmvc.json.patch.LateObjectEvaluator#evaluate(java.lang.Class)
+	 */
 	@Override
 	public <T> Object evaluate(Class<T> type) {
+
 		try {
-			return MAPPER.readValue(valueNode.traverse(), type);
+			return mapper.readValue(valueNode.traverse(), type);
 		} catch (Exception e) {
 			return null;
 		}
 	}
-
 }

spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/patch/JsonPatchPatchConverter.java

@@ -15,13 +15,13 @@
  */
 package org.springframework.data.rest.webmvc.json.patch;
 
+import lombok.NonNull;
+import lombok.RequiredArgsConstructor;
+
 import java.util.ArrayList;
 import java.util.Iterator;
 import java.util.List;
 
-import org.springframework.util.Assert;
-
-import com.fasterxml.jackson.core.JsonPointer;
 import com.fasterxml.jackson.databind.JsonNode;
 import com.fasterxml.jackson.databind.ObjectMapper;
 import com.fasterxml.jackson.databind.node.ArrayNode;
@@ -35,21 +35,10 @@
  * @author Oliver Gierke
  * @author Mathias Düsterhöft
  */
+@RequiredArgsConstructor
 public class JsonPatchPatchConverter implements PatchConverter<JsonNode> {
 
-	private final ObjectMapper mapper;
-
-	/**
-	 * Creates a new {@link JsonPatchPatchConverter} for the given {@link ObjectMapper}.
-	 * 
-	 * @param mapper must not be {@literal null}.
-	 */
-	public JsonPatchPatchConverter(ObjectMapper mapper) {
-
-		Assert.notNull(mapper, "ObjectMapper must not be null!");
-
-		this.mapper = mapper;
-	}
+	private final @NonNull ObjectMapper mapper;
 
 	/**
 	 * Constructs a {@link Patch} object given a JsonNode.
@@ -58,13 +47,16 @@ public JsonPatchPatchConverter(ObjectMapper mapper) {
 	 * @return a {@link Patch}
 	 */
 	public Patch convert(JsonNode jsonNode) {
+
 		if (!(jsonNode instanceof ArrayNode)) {
 			throw new IllegalArgumentException("JsonNode must be an instance of ArrayNode");
 		}
 
 		ArrayNode opNodes = (ArrayNode) jsonNode;
 		List<PatchOperation> ops = new ArrayList<PatchOperation>(opNodes.size());
+
 		for (Iterator<JsonNode> elements = opNodes.elements(); elements.hasNext();) {
+
 			JsonNode opNode = elements.next();
 
 			String opType = opNode.get("op").textValue();
@@ -105,25 +97,33 @@ public JsonNode convert(Patch patch) {
 		List<PatchOperation> operations = patch.getOperations();
 		JsonNodeFactory nodeFactory = JsonNodeFactory.instance;
 		ArrayNode patchNode = nodeFactory.arrayNode();
+
 		for (PatchOperation operation : operations) {
+
 			ObjectNode opNode = nodeFactory.objectNode();
 			opNode.set("op", nodeFactory.textNode(operation.getOp()));
 			opNode.set("path", nodeFactory.textNode(operation.getPath()));
+
 			if (operation instanceof FromOperation) {
+
 				FromOperation fromOp = (FromOperation) operation;
 				opNode.set("from", nodeFactory.textNode(fromOp.getFrom()));
 			}
+
 			Object value = operation.getValue();
+
 			if (value != null) {
 				opNode.set("value", mapper.valueToTree(value));
 			}
+
 			patchNode.add(opNode);
 		}
 
 		return patchNode;
 	}
 
 	private Object valueFromJsonNode(String path, JsonNode valueNode) {
+
 		if (valueNode == null || valueNode.isNull()) {
 			return null;
 		} else if (valueNode.isTextual()) {
@@ -137,36 +137,11 @@ private Object valueFromJsonNode(String path, JsonNode valueNode) {
 		} else if (valueNode.isLong()) {
 			return valueNode.asLong();
 		} else if (valueNode.isObject()) {
-			return new JsonLateObjectEvaluator(valueNode);
+			return new JsonLateObjectEvaluator(mapper, valueNode);
 		} else if (valueNode.isArray()) {
 			// TODO: Convert valueNode to array
 		}
 
 		return null;
 	}
-
-	/**
-	 * Returns whether the trailing element of the given {@link JsonPointer} is a pointer into an array or collection.
-	 * 
-	 * @param pointer must not be {@literal null}.
-	 * @return
-	 */
-	private static boolean isCollectionElementReference(String pointer) {
-
-		String[] segments = pointer.split("/");
-
-		if (segments.length == 0) {
-			return false;
-		}
-
-		String trailing = segments[segments.length - 1];
-
-		try {
-			Integer.parseInt(trailing);
-			return true;
-		} catch (NumberFormatException o_O) {
-			return false;
-		}
-	}
-
 }

spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/patch/LateObjectEvaluator.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2014 the original author or authors.
+ * Copyright 2014-2016 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -19,24 +19,20 @@
  * <p>
  * Strategy interface for resolving values from an operation definition.
  * </p>
- * 
  * <p>
- * {@link Patch} implementation generically defines a patch without being tied to any particular
- * patch specification. But it's important to know the patch format when resolving the value of
- * an operation, as the value format will likely be tied to the patch specification. For example,
- * the <code>value</code> attribute of a JSON Patch operation will contain a JSON object. A different
- * patch specification may define values in some non-JSON format.
+ * {@link Patch} implementation generically defines a patch without being tied to any particular patch specification.
+ * But it's important to know the patch format when resolving the value of an operation, as the value format will likely
+ * be tied to the patch specification. For example, the <code>value</code> attribute of a JSON Patch operation will
+ * contain a JSON object. A different patch specification may define values in some non-JSON format.
  * </p>
- * 
  * <p>
- * This interface allows for pluggable evaluation of values, allowing {@link Patch} to remain
- * independent of any specific patch representation.
+ * This interface allows for pluggable evaluation of values, allowing {@link Patch} to remain independent of any
+ * specific patch representation.
  * </p>
  * 
  * @author Craig Walls
  */
 public interface LateObjectEvaluator {
 
 	<T> Object evaluate(Class<T> type);
-	
 }

spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/patch/MoveOperation.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2014 the original author or authors.
+ * Copyright 2014-2016 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -17,34 +17,37 @@
 
 /**
  * <p>
- * Operation that moves a value from the given "from" path to the given "path".
- * Will throw a {@link PatchException} if either path is invalid or if the from path is non-nullable.
+ * Operation that moves a value from the given "from" path to the given "path". Will throw a {@link PatchException} if
+ * either path is invalid or if the from path is non-nullable.
  * </p>
- * 
  * <p>
- * NOTE: When dealing with lists, the move operation may effectively be a no-op.
- * That's because the order of a list is probably dictated by a database query that produced the list.
- * Moving things around in the list will have no bearing on the values of each item in the list.
- * When the same list resource is retrieved again later, the order will again be decided by the query,
- * effectively undoing any previous move operation.
+ * NOTE: When dealing with lists, the move operation may effectively be a no-op. That's because the order of a list is
+ * probably dictated by a database query that produced the list. Moving things around in the list will have no bearing
+ * on the values of each item in the list. When the same list resource is retrieved again later, the order will again be
+ * decided by the query, effectively undoing any previous move operation.
  * </p>
  * 
  * @author Craig Walls
+ * @author Oliver Gierke
  */
-public class MoveOperation extends FromOperation {
+class MoveOperation extends FromOperation {
 
 	/**
 	 * Constructs the move operation.
+	 * 
 	 * @param path The path to move the source value to. (e.g., '/foo/bar/4')
 	 * @param from The source path from which a value will be moved. (e.g., '/foo/bar/5')
 	 */
 	public MoveOperation(String path, String from) {
 		super("move", path, from);
 	}
-	
+
+	/*
+	 * (non-Javadoc)
+	 * @see org.springframework.data.rest.webmvc.json.patch.PatchOperation#perform(java.lang.Object, java.lang.Class)
+	 */
 	@Override
 	<T> void perform(Object target, Class<T> type) {
-		addValue(target, popValueAtPath(target, from));
+		addValue(target, popValueAtPath(target, getFrom()));
 	}
-	
 }