From a71ffd65c788ca2668696f7e84ddf8b7f3a3b31a Mon Sep 17 00:00:00 2001 From: Oliver Drotbohm Date: Tue, 21 Feb 2023 18:40:31 +0100 Subject: [PATCH] Use FormattingConversionService in JSON Patch binding. We no pipe the Spring MVC ConversionService into the JSON Patch path binding. That usually is a FormattingConversionService at runtime and also supports the conversion of dates. The ConversionServices is configured into the BindContext(Factory) we use for binding. The context then exposes the EvaluationContext set up with it. Fixes #2233. --- .../config/JsonPatchHandlerUnitTests.java | 4 +++- .../RepositoryRestMvcConfiguration.java | 2 +- .../rest/webmvc/json/JacksonBindContext.java | 18 ++++++++++++++++- .../PersistentEntitiesBindContextFactory.java | 7 +++++-- .../rest/webmvc/json/patch/BindContext.java | 10 ++++++++++ .../data/rest/webmvc/json/patch/SpelPath.java | 20 +++++++++---------- .../json/patch/JsonPointerMappingTests.java | 4 +++- .../webmvc/json/patch/SpelPathUnitTests.java | 20 ++++++++++++++++++- .../json/patch/TestPropertyPathContext.java | 12 +++++++++++ 9 files changed, 80 insertions(+), 17 deletions(-) diff --git a/spring-data-rest-tests/spring-data-rest-tests-mongodb/src/test/java/org/springframework/data/rest/webmvc/config/JsonPatchHandlerUnitTests.java b/spring-data-rest-tests/spring-data-rest-tests-mongodb/src/test/java/org/springframework/data/rest/webmvc/config/JsonPatchHandlerUnitTests.java index e761484dd..cdc16692a 100755 --- a/spring-data-rest-tests/spring-data-rest-tests-mongodb/src/test/java/org/springframework/data/rest/webmvc/config/JsonPatchHandlerUnitTests.java +++ b/spring-data-rest-tests/spring-data-rest-tests-mongodb/src/test/java/org/springframework/data/rest/webmvc/config/JsonPatchHandlerUnitTests.java @@ -30,6 +30,7 @@ import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; import org.mockito.Mock; import org.mockito.junit.jupiter.MockitoExtension; +import org.springframework.core.convert.support.DefaultConversionService; import org.springframework.data.mapping.context.PersistentEntities; import org.springframework.data.mongodb.core.convert.MongoCustomConversions; import org.springframework.data.mongodb.core.mapping.MongoMappingContext; @@ -76,7 +77,8 @@ class JsonPatchHandlerUnitTests { PersistentEntities entities = new PersistentEntities(Arrays.asList(context)); Associations associations = new Associations(mappings, mock(RepositoryRestConfiguration.class)); - BindContextFactory factory = new PersistentEntitiesBindContextFactory(entities); + BindContextFactory factory = new PersistentEntitiesBindContextFactory(entities, + DefaultConversionService.getSharedInstance()); this.handler = new JsonPatchHandler(factory, new DomainObjectReader(entities, associations)); diff --git a/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/config/RepositoryRestMvcConfiguration.java b/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/config/RepositoryRestMvcConfiguration.java index 58089be9f..d168d1572 100644 --- a/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/config/RepositoryRestMvcConfiguration.java +++ b/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/config/RepositoryRestMvcConfiguration.java @@ -500,7 +500,7 @@ public class RepositoryRestMvcConfiguration extends HateoasAwareSpringDataWebCon PluginRegistry, Class> lookups = PluginRegistry.of(getEntityLookups()); DomainObjectReader reader = new DomainObjectReader(entities, associationLinks); - BindContextFactory factory = new PersistentEntitiesBindContextFactory(entities); + BindContextFactory factory = new PersistentEntitiesBindContextFactory(entities, defaultConversionService); return new PersistentEntityResourceHandlerMethodArgumentResolver(defaultMessageConverters, repoRequestArgumentResolver, backendIdHandlerMethodArgumentResolver, diff --git a/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/JacksonBindContext.java b/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/JacksonBindContext.java index 539b5a393..8b0e920be 100644 --- a/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/JacksonBindContext.java +++ b/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/JacksonBindContext.java @@ -17,9 +17,12 @@ package org.springframework.data.rest.webmvc.json; import java.util.Optional; +import org.springframework.core.convert.ConversionService; import org.springframework.data.mapping.PersistentProperty; import org.springframework.data.mapping.context.PersistentEntities; import org.springframework.data.rest.webmvc.json.patch.BindContext; +import org.springframework.expression.EvaluationContext; +import org.springframework.expression.spel.support.SimpleEvaluationContext; import org.springframework.util.Assert; import com.fasterxml.jackson.databind.ObjectMapper; @@ -34,20 +37,24 @@ class JacksonBindContext implements BindContext { private final PersistentEntities entities; private final ObjectMapper mapper; + private final EvaluationContext context; /** * Creates a new {@link JacksonBindContext} for the given {@link PersistentEntities} and {@link ObjectMapper}. * * @param entities must not be {@literal null}. + * @param conversionService must not be {@literal null}. * @param mapper must not be {@literal null}. */ - public JacksonBindContext(PersistentEntities entities, ObjectMapper mapper) { + public JacksonBindContext(PersistentEntities entities, ConversionService conversionService, ObjectMapper mapper) { Assert.notNull(entities, "PersistentEntities must not be null"); Assert.notNull(mapper, "ObjectMapper must not be null"); + Assert.notNull(conversionService, "ConversionService must not be null!"); this.entities = entities; this.mapper = mapper; + this.context = SimpleEvaluationContext.forReadWriteDataBinding().withConversionService(conversionService).build(); } @Override @@ -66,6 +73,15 @@ class JacksonBindContext implements BindContext { .filter(it -> it.isWritableField(segment)), segment); } + /* + * (non-Javadoc) + * @see org.springframework.data.rest.webmvc.json.patch.BindContext#getEvaluationContext() + */ + @Override + public EvaluationContext getEvaluationContext() { + return context; + } + private static Optional getProperty(Optional properties, String segment) { return properties.map(it -> it.getPersistentProperty(segment)) diff --git a/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/PersistentEntitiesBindContextFactory.java b/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/PersistentEntitiesBindContextFactory.java index 94f9fa87d..51a0e6ff7 100644 --- a/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/PersistentEntitiesBindContextFactory.java +++ b/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/PersistentEntitiesBindContextFactory.java @@ -15,6 +15,7 @@ */ package org.springframework.data.rest.webmvc.json; +import org.springframework.core.convert.ConversionService; import org.springframework.data.mapping.context.PersistentEntities; import org.springframework.data.rest.webmvc.json.patch.BindContext; import org.springframework.util.Assert; @@ -29,21 +30,23 @@ import com.fasterxml.jackson.databind.ObjectMapper; public class PersistentEntitiesBindContextFactory implements BindContextFactory { private final PersistentEntities entities; + private final ConversionService conversionService; /** * Creates a new {@link PersistentEntitiesBindContextFactory} for the given {@link PersistentEntities}. * * @param entities must not be {@literal null}. */ - public PersistentEntitiesBindContextFactory(PersistentEntities entities) { + public PersistentEntitiesBindContextFactory(PersistentEntities entities, ConversionService conversionService) { Assert.notNull(entities, "PersistentEntities must not be null!"); this.entities = entities; + this.conversionService = conversionService; } @Override public BindContext getBindContextFor(ObjectMapper mapper) { - return new JacksonBindContext(entities, mapper); + return new JacksonBindContext(entities, conversionService, mapper); } } diff --git a/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/patch/BindContext.java b/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/patch/BindContext.java index 5ae0e24cb..89097b1db 100644 --- a/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/patch/BindContext.java +++ b/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/patch/BindContext.java @@ -17,6 +17,8 @@ package org.springframework.data.rest.webmvc.json.patch; import java.util.Optional; +import org.springframework.expression.EvaluationContext; + /** * Contextual mapping for he translation of JSON Pointer segments into property references on persistent types. * @@ -41,4 +43,12 @@ public interface BindContext { * @return will never be {@literal null}. */ Optional getReadableProperty(String segment, Class type); + + /** + * Returns the {@link EvaluationContext} to be used for evaluating the underlying SpEL expression. + * + * @return will never be {@literal null}. + * @since 3.7.9 + */ + EvaluationContext getEvaluationContext(); } 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 b396a1e13..20707643d 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 @@ -35,7 +35,6 @@ import org.springframework.expression.ExpressionException; 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; @@ -142,7 +141,7 @@ class SpelPath { return READ_PATHS.computeIfAbsent(CacheKey.of(type, this, context), key -> { String mapped = new JsonPointerMapping(context).forRead(key.path.path, type); - return new TypedSpelPath(mapped, key.type); + return new TypedSpelPath(mapped, key.type, context.getEvaluationContext()); }); } @@ -160,7 +159,7 @@ class SpelPath { return WRITE_PATHS.computeIfAbsent(CacheKey.of(type, this, context), key -> { String mapped = new JsonPointerMapping(context).forWrite(key.path.path, type); - return new TypedSpelPath(mapped, key.type); + return new TypedSpelPath(mapped, key.type, context.getEvaluationContext()); }); } @@ -253,17 +252,18 @@ class SpelPath { 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 EvaluationContext CONTEXT = SimpleEvaluationContext.forReadWriteDataBinding().build(); private final Expression expression; private final Class type; + private final EvaluationContext context; - private TypedSpelPath(String path, Class type) { + private TypedSpelPath(String path, Class type, EvaluationContext context) { super(path); this.type = type; this.expression = toSpel(path, type); + this.context = context; } /** @@ -278,7 +278,7 @@ class SpelPath { Assert.notNull(target, "Target must not be null!"); try { - return (T) expression.getValue(CONTEXT, target); + return (T) expression.getValue(context, target); } catch (ExpressionException o_O) { throw new PatchException("Unable to get value from target", o_O); } @@ -294,7 +294,7 @@ class SpelPath { Assert.notNull(target, "Target must not be null!"); - expression.setValue(CONTEXT, target, value); + expression.setValue(context, target, value); } /** @@ -326,7 +326,7 @@ class SpelPath { try { - return expression.getValueType(CONTEXT, root); + return expression.getValueType(context, root); } catch (SpelEvaluationException o_O) { @@ -460,11 +460,11 @@ class SpelPath { } private TypedSpelPath getParent() { - return new TypedSpelPath(path.substring(0, path.lastIndexOf('/')), type); + return new TypedSpelPath(path.substring(0, path.lastIndexOf('/')), type, context); } private TypeDescriptor getTypeDescriptor(Object target) { - return expression.getValueTypeDescriptor(CONTEXT, target); + return expression.getValueTypeDescriptor(context, target); } private Integer getTargetListIndex() { diff --git a/spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/patch/JsonPointerMappingTests.java b/spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/patch/JsonPointerMappingTests.java index 3bc5f77ae..0d4d0845a 100644 --- a/spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/patch/JsonPointerMappingTests.java +++ b/spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/patch/JsonPointerMappingTests.java @@ -20,6 +20,7 @@ import java.util.Collection; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import org.springframework.core.convert.support.DefaultConversionService; import org.springframework.data.keyvalue.core.mapping.context.KeyValueMappingContext; import org.springframework.data.mapping.context.PersistentEntities; import org.springframework.data.rest.webmvc.json.BindContextFactory; @@ -45,7 +46,8 @@ public class JsonPointerMappingTests { context.getPersistentEntity(Sample.class); PersistentEntities entities = new PersistentEntities(Arrays.asList(context)); - BindContextFactory factory = new PersistentEntitiesBindContextFactory(entities); + BindContextFactory factory = new PersistentEntitiesBindContextFactory(entities, + DefaultConversionService.getSharedInstance()); ObjectMapper mapper = new ObjectMapper(); this.verifier = new JsonPointerMapping(factory.getBindContextFor(mapper)); 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 8a9520195..ec2fdbf4f 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,6 +20,7 @@ import static org.assertj.core.api.Assertions.*; import lombok.Data; import lombok.Getter; +import java.time.LocalDate; import java.util.ArrayList; import java.util.Arrays; import java.util.List; @@ -33,6 +34,9 @@ import org.springframework.data.rest.webmvc.json.BindContextFactory; import org.springframework.data.rest.webmvc.json.PersistentEntitiesBindContextFactory; import org.springframework.data.rest.webmvc.json.patch.SpelPath.UntypedSpelPath; import org.springframework.data.rest.webmvc.json.patch.SpelPath.WritingOperations; +import org.springframework.format.annotation.DateTimeFormat; +import org.springframework.format.annotation.DateTimeFormat.ISO; +import org.springframework.format.support.DefaultFormattingConversionService; import com.fasterxml.jackson.annotation.JsonIgnore; import com.fasterxml.jackson.annotation.JsonProperty; @@ -57,7 +61,8 @@ class SpelPathUnitTests { context.getPersistentEntity(Person.class); PersistentEntities entities = new PersistentEntities(Arrays.asList(context)); - BindContextFactory factory = new PersistentEntitiesBindContextFactory(entities); + BindContextFactory factory = new PersistentEntitiesBindContextFactory(entities, + new DefaultFormattingConversionService()); this.context = factory.getBindContextFor(new ObjectMapper()); } @@ -167,6 +172,18 @@ class SpelPathUnitTests { assertThat(path.getExpressionString()).isEqualTo("renamed"); } + @Test // #2233 + void bindsDatesProperly() { + + Person person = new Person(); + + SpelPath.untyped("/birthday") + .bindForWrite(Person.class, context) + .setValue(person, "2000-01-01"); + + assertThat(person.birthday).isEqualTo(LocalDate.of(2000, 1, 1)); + } + // DATAREST-1338 @Data @@ -175,6 +192,7 @@ class SpelPathUnitTests { @JsonIgnore String hiddenProperty; @Getter(onMethod = @__(@JsonIgnore)) String hiddenGetter; @JsonProperty("demaner") String renamed; + @DateTimeFormat(iso = ISO.DATE) LocalDate birthday; } @Data diff --git a/spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/patch/TestPropertyPathContext.java b/spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/patch/TestPropertyPathContext.java index 2e0755b2c..4057e1be4 100644 --- a/spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/patch/TestPropertyPathContext.java +++ b/spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/patch/TestPropertyPathContext.java @@ -17,6 +17,9 @@ package org.springframework.data.rest.webmvc.json.patch; import java.util.Optional; +import org.springframework.expression.EvaluationContext; +import org.springframework.expression.spel.support.SimpleEvaluationContext; + public class TestPropertyPathContext implements BindContext { public static final BindContext INSTANCE = new TestPropertyPathContext(); @@ -38,4 +41,13 @@ public class TestPropertyPathContext implements BindContext { public Optional getWritableProperty(String segment, Class type) { return Optional.of(segment); } + + /* + * (non-Javadoc) + * @see org.springframework.data.rest.webmvc.json.patch.BindContext#getEvaluationContext() + */ + @Override + public EvaluationContext getEvaluationContext() { + return SimpleEvaluationContext.forReadWriteDataBinding().build(); + } }