From 017b69c4e1af6b6dc4c2c6ee2b2d45a00c7252d7 Mon Sep 17 00:00:00 2001 From: Oliver Drotbohm Date: Tue, 29 Jan 2019 14:06:49 +0100 Subject: [PATCH] DATAREST-1338 - JsonPatch handling is now able to traverse maps. We're now able to correctly identify Map keys within JsonPatch paths so that e.g. people/Dave/name properly translates into a people['Dave'].name SpEL expression. Streamlined the design of SpelPath to avoid duplicate parsing and properly express the difference between typed and untyped paths in the type hierarchy. Slightly refactored the type hierarchy so that only methods that make sense in either untyped or typed state appear on the instance returned (getLeafType(Class) was exposed on TypedSpelPath before, which was confusing). --- .../rest/webmvc/json/patch/AddOperation.java | 8 +- .../rest/webmvc/json/patch/CopyOperation.java | 8 +- .../rest/webmvc/json/patch/MoveOperation.java | 8 +- .../data/rest/webmvc/json/patch/Patch.java | 21 +- .../webmvc/json/patch/PatchOperation.java | 6 +- .../webmvc/json/patch/RemoveOperation.java | 6 +- .../webmvc/json/patch/ReplaceOperation.java | 6 +- .../data/rest/webmvc/json/patch/SpelPath.java | 314 ++++++++++++------ .../rest/webmvc/json/patch/TestOperation.java | 5 +- .../json/patch/ReplaceOperationTests.java | 30 ++ .../webmvc/json/patch/SpelPathUnitTests.java | 47 ++- 11 files changed, 329 insertions(+), 130 deletions(-) diff --git a/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/patch/AddOperation.java b/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/patch/AddOperation.java index 34356f3e3..d17fab144 100644 --- a/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/patch/AddOperation.java +++ b/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/patch/AddOperation.java @@ -15,6 +15,8 @@ */ package org.springframework.data.rest.webmvc.json.patch; +import org.springframework.data.rest.webmvc.json.patch.SpelPath.UntypedSpelPath; + /** * 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. @@ -30,12 +32,12 @@ class AddOperation extends PatchOperation { * @param path The path where the value will be added. (e.g., '/foo/bar/4') * @param value The value to add. */ - private AddOperation(SpelPath path, Object value) { + private AddOperation(UntypedSpelPath path, Object value) { super("add", path, value); } public static AddOperation of(String path, Object value) { - return new AddOperation(SpelPath.of(path), value); + return new AddOperation(SpelPath.untyped(path), value); } /* @@ -58,6 +60,6 @@ class AddOperation extends PatchOperation { return super.evaluateValueFromTarget(targetObject, entityType); } - return evaluate(path.getLeafType(entityType)); + return evaluate(path.bindTo(entityType).getLeafType()); } } diff --git a/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/patch/CopyOperation.java b/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/patch/CopyOperation.java index e1150c064..9e1f74db8 100644 --- a/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/patch/CopyOperation.java +++ b/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/patch/CopyOperation.java @@ -17,6 +17,8 @@ package org.springframework.data.rest.webmvc.json.patch; import lombok.RequiredArgsConstructor; +import org.springframework.data.rest.webmvc.json.patch.SpelPath.UntypedSpelPath; + /** *

* Operation to copy a value from the given "from" path to the given "path". Will throw a {@link PatchException} if @@ -41,7 +43,7 @@ import lombok.RequiredArgsConstructor; */ class CopyOperation extends PatchOperation { - private final SpelPath from; + private final UntypedSpelPath from; /** * Constructs the copy operation @@ -49,7 +51,7 @@ class CopyOperation extends PatchOperation { * @param path The path to copy the source value to. (e.g., '/foo/bar/4') * @param from The source path from which a value will be copied. (e.g., '/foo/bar/5') */ - public CopyOperation(SpelPath path, SpelPath from) { + public CopyOperation(UntypedSpelPath path, UntypedSpelPath from) { super("copy", path); @@ -66,7 +68,7 @@ class CopyOperation extends PatchOperation { private final String from; CopyOperation to(String to) { - return new CopyOperation(SpelPath.of(to), SpelPath.of(from)); + return new CopyOperation(SpelPath.untyped(to), SpelPath.untyped(from)); } } diff --git a/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/patch/MoveOperation.java b/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/patch/MoveOperation.java index 08ed3d385..b0933bbc2 100644 --- a/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/patch/MoveOperation.java +++ b/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/patch/MoveOperation.java @@ -18,6 +18,8 @@ package org.springframework.data.rest.webmvc.json.patch; import lombok.AccessLevel; import lombok.RequiredArgsConstructor; +import org.springframework.data.rest.webmvc.json.patch.SpelPath.UntypedSpelPath; + /** *

* Operation that moves a value from the given "from" path to the given "path". Will throw a {@link PatchException} if @@ -35,7 +37,7 @@ import lombok.RequiredArgsConstructor; */ class MoveOperation extends PatchOperation { - private final SpelPath from; + private final UntypedSpelPath from; /** * Constructs the move operation. @@ -43,7 +45,7 @@ class MoveOperation extends PatchOperation { * @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') */ - private MoveOperation(SpelPath path, SpelPath from) { + private MoveOperation(UntypedSpelPath path, UntypedSpelPath from) { super("move", path); @@ -60,7 +62,7 @@ class MoveOperation extends PatchOperation { private final String from; public MoveOperation to(String to) { - return new MoveOperation(SpelPath.of(to), SpelPath.of(from)); + return new MoveOperation(SpelPath.untyped(to), SpelPath.untyped(from)); } } diff --git a/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/patch/Patch.java b/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/patch/Patch.java index 349548fa3..a589ed88a 100644 --- a/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/patch/Patch.java +++ b/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/patch/Patch.java @@ -15,8 +15,11 @@ */ package org.springframework.data.rest.webmvc.json.patch; +import java.util.Iterator; import java.util.List; +import org.springframework.data.util.Streamable; + /** *

* Represents a Patch. @@ -29,7 +32,7 @@ import java.util.List; * @author Craig Walls * @author Oliver Gierke */ -public class Patch { +public class Patch implements Streamable { private final List operations; @@ -44,6 +47,13 @@ public class Patch { return operations.size(); } + /** + * Returns all underlying {@link PatchOperation}s. + * + * @return + * @deprecated since 3.2, prefer streaming via {@link #stream()}. + */ + @Deprecated public List getOperations() { return operations; } @@ -85,4 +95,13 @@ public class Patch { return in; } + + /* + * (non-Javadoc) + * @see java.lang.Iterable#iterator() + */ + @Override + public Iterator iterator() { + return operations.iterator(); + } } diff --git a/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/patch/PatchOperation.java b/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/patch/PatchOperation.java index 4f6ed4da5..3e6a72d39 100644 --- a/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/patch/PatchOperation.java +++ b/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/patch/PatchOperation.java @@ -18,6 +18,8 @@ package org.springframework.data.rest.webmvc.json.patch; import lombok.NonNull; import lombok.RequiredArgsConstructor; +import org.springframework.data.rest.webmvc.json.patch.SpelPath.UntypedSpelPath; + /** * Abstract base class representing and providing support methods for patch operations. * @@ -29,7 +31,7 @@ import lombok.RequiredArgsConstructor; public abstract class PatchOperation { protected final @NonNull String op; - protected final @NonNull SpelPath path; + protected final @NonNull UntypedSpelPath path; protected final Object value; /** @@ -38,7 +40,7 @@ public abstract class PatchOperation { * @param op the operation name. (e.g., 'move') * @param path the path to perform the operation on. (e.g., '/1/description') */ - public PatchOperation(String op, SpelPath path) { + public PatchOperation(String op, UntypedSpelPath path) { this(op, path, null); } diff --git a/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/patch/RemoveOperation.java b/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/patch/RemoveOperation.java index 983717546..01150eb54 100644 --- a/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/patch/RemoveOperation.java +++ b/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/patch/RemoveOperation.java @@ -15,6 +15,8 @@ */ package org.springframework.data.rest.webmvc.json.patch; +import org.springframework.data.rest.webmvc.json.patch.SpelPath.UntypedSpelPath; + /** * Operation that removes the value at the given path. Will throw a {@link PatchException} if the given path isn't valid * or if the path is non-nullable. @@ -29,12 +31,12 @@ class RemoveOperation extends PatchOperation { * * @param path The path of the value to be removed. (e.g., '/foo/bar/4') */ - private RemoveOperation(SpelPath path) { + private RemoveOperation(UntypedSpelPath path) { super("remove", path); } public static RemoveOperation valueAt(String path) { - return new RemoveOperation(SpelPath.of(path)); + return new RemoveOperation(SpelPath.untyped(path)); } /* diff --git a/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/patch/ReplaceOperation.java b/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/patch/ReplaceOperation.java index 6587092a4..8e1ed29fd 100644 --- a/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/patch/ReplaceOperation.java +++ b/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/patch/ReplaceOperation.java @@ -18,6 +18,8 @@ package org.springframework.data.rest.webmvc.json.patch; import lombok.AccessLevel; import lombok.RequiredArgsConstructor; +import org.springframework.data.rest.webmvc.json.patch.SpelPath.UntypedSpelPath; + /** * Operation that replaces the value at the given path with a new value. * @@ -32,7 +34,7 @@ class ReplaceOperation extends PatchOperation { * @param path The path whose value is to be replaced. (e.g., '/foo/bar/4') * @param value The value that will replace the current path value. */ - private ReplaceOperation(SpelPath path, Object value) { + private ReplaceOperation(UntypedSpelPath path, Object value) { super("replace", path, value); } @@ -46,7 +48,7 @@ class ReplaceOperation extends PatchOperation { private final String path; public ReplaceOperation with(Object value) { - return new ReplaceOperation(SpelPath.of(path), value); + return new ReplaceOperation(SpelPath.untyped(path), value); } } diff --git a/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/patch/SpelPath.java b/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/patch/SpelPath.java index 4df29867a..fb9ae68ad 100644 --- a/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/patch/SpelPath.java +++ b/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/patch/SpelPath.java @@ -26,12 +26,14 @@ import java.util.Collection; import java.util.List; import java.util.Map; import java.util.Optional; -import java.util.stream.Collectors; +import java.util.stream.Stream; import org.springframework.core.CollectionFactory; import org.springframework.core.convert.TypeDescriptor; import org.springframework.data.mapping.PropertyPath; import org.springframework.data.mapping.PropertyReferenceException; +import org.springframework.data.util.ClassTypeInformation; +import org.springframework.data.util.TypeInformation; import org.springframework.expression.EvaluationContext; import org.springframework.expression.Expression; import org.springframework.expression.ExpressionException; @@ -39,9 +41,11 @@ import org.springframework.expression.spel.SpelEvaluationException; import org.springframework.expression.spel.SpelMessage; import org.springframework.expression.spel.standard.SpelExpressionParser; import org.springframework.expression.spel.support.SimpleEvaluationContext; +import org.springframework.lang.Nullable; import org.springframework.util.Assert; import org.springframework.util.CollectionUtils; import org.springframework.util.ConcurrentReferenceHashMap; +import org.springframework.util.StringUtils; /** * Value object to represent a SpEL-backed patch path. @@ -53,53 +57,28 @@ class SpelPath { private static final SpelExpressionParser SPEL_EXPRESSION_PARSER = new SpelExpressionParser(); private static final String APPEND_CHARACTER = "-"; - private static final Map PATHS = new ConcurrentReferenceHashMap<>(32); + private static final Map UNTYPED_PATHS = new ConcurrentReferenceHashMap<>(32); protected final @Getter String path; - protected final Expression expression; - - private SpelPath(String path) { - - Assert.notNull(path, "Path must not be null!"); - - this.path = path; - this.expression = SPEL_EXPRESSION_PARSER.parseExpression(pathToSpEL(path)); - } /** - * Returns a {@link SpelPath} for the given source. + * Returns a {@link UntypedSpelPath} for the given source. * * @param source must not be {@literal null}. * @return */ - public static SpelPath of(String source) { - return PATHS.computeIfAbsent(source, SpelPath::new); + public static UntypedSpelPath untyped(String source) { + return UNTYPED_PATHS.computeIfAbsent(source, UntypedSpelPath::new); } /** - * Returns a {@link TypedSpelPath} binding the expression to the given type. + * Returns a {@link TypedSpelPath} for the given source and type. * - * @param type must not be {@literal null}. + * @param source must not be {@literal null}. * @return */ - public TypedSpelPath bindTo(Class type) { - - Assert.notNull(type, "Type must not be null!"); - - return TypedSpelPath.of(this, type); - } - - /** - * Returns the leaf type of the underlying expression or the given type - * - * @param type - * @return - */ - public Class getLeafType(Class type) { - - return TypedSpelPath.verifyPath(path, type) // - .> map(it -> it.getType()) // - .orElse(type); + public static TypedSpelPath typed(String source, Class type) { + return untyped(source).bindTo(type); } /** @@ -111,10 +90,6 @@ class SpelPath { return path.endsWith("-"); } - private SpelPath getParent() { - return SpelPath.of(path.substring(0, path.lastIndexOf('/'))); - } - /* * (non-Javadoc) * @see java.lang.Object#toString() @@ -135,7 +110,7 @@ class SpelPath { return true; } - if (!(SpelPath.class.isInstance(obj))) { + if (!SpelPath.class.isInstance(obj)) { return false; } @@ -153,54 +128,24 @@ class SpelPath { return path.hashCode(); } - private static String pathToSpEL(String path) { - return pathNodesToSpEL(path.split("\\/")); - } + static class UntypedSpelPath extends SpelPath { - private static String pathNodesToSpEL(String[] pathNodes) { - - StringBuilder spelBuilder = new StringBuilder(); - - for (int i = 0; i < pathNodes.length; i++) { - - String pathNode = pathNodes[i]; - - if (pathNode.length() == 0) { - continue; - } - - if (APPEND_CHARACTER.equals(pathNode)) { - - if (spelBuilder.length() > 0) { - spelBuilder.append("."); - } - - spelBuilder.append("$[true]"); - continue; - } - - try { - - int index = Integer.parseInt(pathNode); - spelBuilder.append('[').append(index).append(']'); - - } catch (NumberFormatException e) { - - if (spelBuilder.length() > 0) { - spelBuilder.append('.'); - } - - spelBuilder.append(pathNode); - } + private UntypedSpelPath(String path) { + super(path); } - String spel = spelBuilder.toString(); + /** + * Returns a {@link TypedSpelPath} binding the expression to the given type. + * + * @param type must not be {@literal null}. + * @return + */ + public TypedSpelPath bindTo(Class type) { - if (spel.length() == 0) { - spel = "#this"; + Assert.notNull(type, "Type must not be null!"); + + return TypedSpelPath.of(this, type); } - - return spel; } /** @@ -211,26 +156,26 @@ class SpelPath { @EqualsAndHashCode(callSuper = true) static class TypedSpelPath extends SpelPath { - private static final String INVALID_PATH_REFERENCE = "Invalid path reference %s on type %s (from source %s)!"; + private static final String INVALID_PATH_REFERENCE = "Invalid path reference %s on type %s!"; private static final String INVALID_COLLECTION_INDEX = "Invalid collection index %s for collection of size %s. Use '…/-' or the collection's actual size as index to append to it!"; private static final Map TYPED_PATHS = new ConcurrentReferenceHashMap<>(32); private static final EvaluationContext CONTEXT = SimpleEvaluationContext.forReadWriteDataBinding().build(); + private final Expression expression; private final Class type; @Value(staticConstructor = "of") private static class CacheKey { Class type; - SpelPath path; + UntypedSpelPath path; } - private TypedSpelPath(SpelPath path, Class type) { + private TypedSpelPath(UntypedSpelPath path, Class type) { - super(path.path, path.expression); - - verifyPath(path.path, type); + super(path.path); this.type = type; + this.expression = toSpel(path.path, type); } /** @@ -240,7 +185,7 @@ class SpelPath { * @param type must not be {@literal null}. * @return */ - public static TypedSpelPath of(SpelPath path, Class type) { + public static TypedSpelPath of(UntypedSpelPath path, Class type) { Assert.notNull(path, "Path must not be null!"); Assert.notNull(type, "Type must not be null!"); @@ -272,13 +217,30 @@ class SpelPath { * @param target must not be {@literal null}. * @param value can be {@literal null}. */ - public void setValue(Object target, Object value) { + public void setValue(Object target, @Nullable Object value) { Assert.notNull(target, "Target must not be null!"); expression.setValue(CONTEXT, target, value); } + /** + * Returns the type of the leaf property of the path. + * + * @return will never be {@literal null}. + */ + public Class getLeafType() { + + return TypedSpelPath.verifyPath(path, type) // + .map(PropertyPath::getLeafProperty) // + .> map(PropertyPath::getType) // + .orElse(type); + } + + public String getExpressionString() { + return expression.getExpressionString(); + } + /** * Returns the type of the expression target based on the given root. * @@ -316,8 +278,7 @@ class SpelPath { * @param source the source object to look the value up from, must not be {@literal null}. * @return */ - - public void copyFrom(SpelPath path, Object source) { + public void copyFrom(UntypedSpelPath path, Object source) { Assert.notNull(path, "Source path must not be null!"); Assert.notNull(source, "Source value must not be null!"); @@ -333,7 +294,7 @@ class SpelPath { * @param source the source object to look the value up from, must not be {@literal null}. * @return */ - public void moveFrom(SpelPath path, Object source) { + public void moveFrom(UntypedSpelPath path, Object source) { Assert.notNull(path, "Source path must not be null!"); Assert.notNull(source, "Source value must not be null!"); @@ -413,8 +374,20 @@ class SpelPath { } } + /* + * (non-Javadoc) + * @see org.springframework.data.rest.webmvc.json.patch.SpelPath#toString() + */ + @Override + public String toString() { + return String.format("%s on %s -> %s", path, type.getName(), getExpressionString()); + } + private TypedSpelPath getParent() { - return TypedSpelPath.of(super.getParent(), type); + + return SpelPath // + .untyped(path.substring(0, path.lastIndexOf('/'))) // + .bindTo(type); } private TypeDescriptor getTypeDescriptor(Object target) { @@ -449,20 +422,153 @@ class SpelPath { Assert.notNull(path, "Path must not be null!"); Assert.notNull(type, "Type must not be null!"); - String pathSource = Arrays.stream(path.split("/"))// - .filter(it -> !it.matches("\\d+")) // no digits - .filter(it -> !it.equals("-")) // no "last element"s - .filter(it -> !it.isEmpty()) // - .collect(Collectors.joining(".")); + // Remove leading digits + String segmentSource = path.replaceAll("^/\\d+", ""); - if (pathSource.isEmpty()) { - return Optional.empty(); - } + Stream segments = Arrays.stream(segmentSource.split("/"))// + .filter(it -> !it.equals("-")) // no "last element"s + .filter(it -> !it.isEmpty()); try { - return Optional.of(PropertyPath.from(pathSource, type)); + + return segments.reduce(Optional. empty(), // + (current, next) -> Optional.of(createOrSkip(current, next, type)), // + (l, r) -> r) // + .map(SkippedPropertyPath::getPath); + } catch (PropertyReferenceException o_O) { - throw new PatchException(String.format(INVALID_PATH_REFERENCE, pathSource, type, path), o_O); + throw new PatchException(String.format(INVALID_PATH_REFERENCE, o_O.getPropertyName(), type), o_O); + } + } + + @Value + @RequiredArgsConstructor(access = AccessLevel.PRIVATE, staticName = "of") + private static class SkippedPropertyPath { + + PropertyPath path; + boolean skipped; + + public static SkippedPropertyPath of(String segment, Class type) { + return of(PropertyPath.from(segment, type), false); + } + + public SkippedPropertyPath nested(String segment) { + + if (skipped) { + return SkippedPropertyPath.of(path.nested(segment), false); + } + + TypeInformation typeInformation = path.getTypeInformation(); + + return typeInformation.isMap() || typeInformation.isCollectionLike() // + ? SkippedPropertyPath.of(path, true) // + : SkippedPropertyPath.of(path.nested(segment), false); + } + } + + private static Expression toSpel(String path, Class type) { + + String expression = Arrays.stream(path.split("/"))// + .filter(it -> !it.isEmpty()) // + .reduce(Optional. empty(), // + (current, next) -> Optional.of(nextOrCreate(current, next, type)), // + (l, r) -> r) // + .map(it -> it.getExpression()) // + .orElse("#this"); + + return SPEL_EXPRESSION_PARSER.parseExpression(expression); + } + + private static SpelExpressionBuilder nextOrCreate(Optional current, String next, + Class type) { + + return current // + .map(it -> it.next(next)) // + .orElseGet(() -> SpelExpressionBuilder.of(type).next(next)); + } + + private static SkippedPropertyPath createOrSkip(Optional current, String next, Class type) { + + return current // + .map(it -> it.nested(next)) // + .orElseGet(() -> SkippedPropertyPath.of(next, type)); + } + + @Value + private static class SpelExpressionBuilder { + + private static final TypeInformation STRING_TYPE = ClassTypeInformation.from(String.class); + + private final @Nullable PropertyPath basePath; + private final Class type; + private final String spelSegment; + private final boolean skipped; + + public String getExpression() { + return StringUtils.hasText(spelSegment) ? spelSegment : null; + } + + public static SpelExpressionBuilder of(Class type) { + return new SpelExpressionBuilder(null, type, "", false); + } + + private SpelExpressionBuilder skipWith(String segment) { + return new SpelExpressionBuilder(basePath, type, spelSegment.concat(segment), true); + } + + private SpelExpressionBuilder nested(String segment) { + + String segmentBase = StringUtils.hasText(spelSegment) // + ? spelSegment.concat(".") // + : spelSegment; + + try { + + PropertyPath path = basePath == null // + ? PropertyPath.from(segment, type) // + : basePath.nested(segment); + + return new SpelExpressionBuilder(path, type, segmentBase.concat(segment), false); + + } catch (PropertyReferenceException o_O) { + throw new PatchException(String.format(INVALID_PATH_REFERENCE, o_O.getPropertyName(), type), o_O); + } + } + + public SpelExpressionBuilder next(String segment) { + + if (basePath == null) { + + if (APPEND_CHARACTER.equals(segment)) { + return skipWith("$[true]"); + } + + if (segment.matches("\\d+")) { + return skipWith(String.format("[%s]", segment)); + } + + return nested(segment); + } + + if (skipped) { + return nested(segment); + } + + TypeInformation typeInformation = basePath.getTypeInformation(); + + if (typeInformation.isMap()) { + + TypeInformation componentType = typeInformation.getComponentType(); + String keyExpression = STRING_TYPE.equals(componentType) ? String.format("'%s'", segment) : segment; + + return skipWith(String.format("[%s]", keyExpression)); + } + + if (typeInformation.isCollectionLike()) { + return skipWith(APPEND_CHARACTER.equals(segment) ? "$[true]" : String.format("[%s]", segment)); + } + + return nested(segment); } } } diff --git a/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/patch/TestOperation.java b/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/patch/TestOperation.java index 4436325aa..d856f4a56 100644 --- a/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/patch/TestOperation.java +++ b/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/patch/TestOperation.java @@ -21,6 +21,7 @@ import lombok.RequiredArgsConstructor; import java.math.BigDecimal; import java.math.BigInteger; +import org.springframework.data.rest.webmvc.json.patch.SpelPath.UntypedSpelPath; import org.springframework.util.ObjectUtils; /** @@ -43,7 +44,7 @@ class TestOperation extends PatchOperation { * @param path The path to test. (e.g., '/foo/bar/4') * @param value The value to test the path against. */ - private TestOperation(SpelPath path, Object value) { + private TestOperation(UntypedSpelPath path, Object value) { super("test", path, value); } @@ -57,7 +58,7 @@ class TestOperation extends PatchOperation { private final String path; public TestOperation hasValue(Object value) { - return new TestOperation(SpelPath.of(path), value); + return new TestOperation(SpelPath.untyped(path), value); } } diff --git a/spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/patch/ReplaceOperationTests.java b/spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/patch/ReplaceOperationTests.java index 034ac1d0e..4ae778bb7 100755 --- a/spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/patch/ReplaceOperationTests.java +++ b/spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/patch/ReplaceOperationTests.java @@ -15,13 +15,17 @@ */ package org.springframework.data.rest.webmvc.json.patch; +import static org.assertj.core.api.Assertions.assertThat; import static org.junit.Assert.*; import java.util.ArrayList; +import java.util.HashMap; import java.util.List; +import java.util.Map; import org.junit.Test; +import com.fasterxml.jackson.databind.JsonNode; import com.fasterxml.jackson.databind.ObjectMapper; public class ReplaceOperationTests { @@ -82,4 +86,30 @@ public class ReplaceOperationTests { assertNotNull(todo.getType().getValue()); assertTrue(todo.getType().getValue().equals("new")); } + + @Test // DATAREST-1338 + public void replacesMapValueCorrectly() throws Exception { + + Book book = new Book(); + book.characters = new HashMap<>(); + book.characters.put("protagonist", "Pinco"); + + ReplaceOperation.valueAt("/characters/protagonist") // + .with(prepareValue("\"Pallo\"")) // + .perform(book, Book.class); + + assertThat(book.characters.get("protagonist")).isEqualTo("Pallo"); + } + + private static Object prepareValue(String json) throws Exception { + + ObjectMapper mapper = new ObjectMapper(); + JsonNode node = mapper.readTree(json); + return new JsonLateObjectEvaluator(mapper, node); + } + + // DATAREST-1338 + class Book { + public Map characters; + } } diff --git a/spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/patch/SpelPathUnitTests.java b/spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/patch/SpelPathUnitTests.java index 625575e1f..c21d67461 100755 --- a/spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/patch/SpelPathUnitTests.java +++ b/spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/patch/SpelPathUnitTests.java @@ -20,9 +20,11 @@ import static org.junit.Assert.*; import java.util.ArrayList; import java.util.List; +import java.util.Map; import org.junit.Test; import org.springframework.data.rest.webmvc.json.patch.SpelPath.TypedSpelPath; +import org.springframework.data.rest.webmvc.json.patch.SpelPath.UntypedSpelPath; /** * Unit tests for {@link SpelPath}. @@ -34,34 +36,34 @@ public class SpelPathUnitTests { @Test public void listIndex() { - SpelPath expr = SpelPath.of("/1/description"); + UntypedSpelPath expr = SpelPath.untyped("/1/description"); List todos = new ArrayList(); todos.add(new Todo(1L, "A", false)); todos.add(new Todo(2L, "B", false)); todos.add(new Todo(3L, "C", false)); - assertEquals("B", (String) expr.bindTo(Todo.class).getValue(todos)); + assertEquals("B", expr.bindTo(Todo.class).getValue(todos)); } @Test public void accessesLastCollectionElementWithDash() { - SpelPath expr = SpelPath.of("/-/description"); + UntypedSpelPath expr = SpelPath.untyped("/-/description"); List todos = new ArrayList(); todos.add(new Todo(1L, "A", false)); todos.add(new Todo(2L, "B", false)); todos.add(new Todo(3L, "C", false)); - assertEquals("C", (String) expr.bindTo(Todo.class).getValue(todos)); + assertEquals("C", expr.bindTo(Todo.class).getValue(todos)); } @Test // DATAREST-1152 public void cachesSpelPath() { - SpelPath left = SpelPath.of("/description"); - SpelPath right = SpelPath.of("/description"); + UntypedSpelPath left = SpelPath.untyped("/description"); + UntypedSpelPath right = SpelPath.untyped("/description"); assertSame(left, right); } @@ -69,7 +71,7 @@ public class SpelPathUnitTests { @Test // DATAREST-1152 public void cachesTypedSpelPath() { - SpelPath source = SpelPath.of("/description"); + UntypedSpelPath source = SpelPath.untyped("/description"); TypedSpelPath left = source.bindTo(Todo.class); TypedSpelPath right = source.bindTo(Todo.class); @@ -78,6 +80,35 @@ public class SpelPathUnitTests { @Test // DATAREST-1274 public void supportsMultiDigitCollectionIndex() { - assertThat(SpelPath.of("/11/description").getLeafType(Todo.class)).isEqualTo(String.class); + assertThat(SpelPath.untyped("/11/description").bindTo(Todo.class).getLeafType()).isEqualTo(String.class); + } + + @Test // DATAREST-1338 + public void handlesStringMapKeysInPathExpressions() { + + TypedSpelPath path = SpelPath.untyped("people/Dave/name").bindTo(MapWrapper.class); + + assertThat(path.getExpressionString()).isEqualTo("people['Dave'].name"); + assertThat(path.getLeafType()).isEqualTo(String.class); + } + + @Test // DATAREST-1338 + public void handlesIntegerMapKeysInPathExpressions() { + + TypedSpelPath path = SpelPath.untyped("peopleByInt/0/name").bindTo(MapWrapper.class); + + assertThat(path.getExpressionString()).isEqualTo("peopleByInt[0].name"); + assertThat(path.getLeafType()).isEqualTo(String.class); + } + + // DATAREST-1338 + + static class Person { + String name; + } + + static class MapWrapper { + Map people; + Map peopleByInt; } }