From bf37590b25a0c066f67547b39fb4d7294e2c7cb7 Mon Sep 17 00:00:00 2001 From: Oliver Drotbohm Date: Thu, 1 Sep 2022 11:09:03 -0500 Subject: [PATCH] Improve JSON Patch implementation. Refactor JSON Patch application implementation to improve the property detection for which values are supposed to be set. Fixes #2177. --- .../config/JsonPatchHandlerUnitTests.java | 64 ++++++- .../rest/webmvc/config/JsonPatchHandler.java | 45 ++--- ...ResourceHandlerMethodArgumentResolver.java | 13 +- .../RepositoryRestMvcConfiguration.java | 6 +- .../rest/webmvc/json/BindContextFactory.java | 36 ++++ .../rest/webmvc/json/DomainObjectReader.java | 2 +- .../rest/webmvc/json/JacksonBindContext.java | 74 ++++++++ .../rest/webmvc/json/MappedProperties.java | 39 ++++- .../PersistentEntitiesBindContextFactory.java | 49 ++++++ .../rest/webmvc/json/patch/AddOperation.java | 13 +- .../rest/webmvc/json/patch/BindContext.java | 44 +++++ .../rest/webmvc/json/patch/CopyOperation.java | 6 +- .../json/patch/JsonPatchPatchConverter.java | 6 +- .../webmvc/json/patch/JsonPointerMapping.java | 127 ++++++++++++++ .../rest/webmvc/json/patch/MoveOperation.java | 4 +- .../data/rest/webmvc/json/patch/Patch.java | 9 +- .../webmvc/json/patch/PatchOperation.java | 6 +- .../webmvc/json/patch/RemoveOperation.java | 4 +- .../webmvc/json/patch/ReplaceOperation.java | 5 +- .../data/rest/webmvc/json/patch/SpelPath.java | 163 ++++++++++-------- .../rest/webmvc/json/patch/TestOperation.java | 6 +- .../rest/webmvc/json/patch/package-info.java | 17 ++ ...andlerMethodArgumentResolverUnitTests.java | 8 +- .../json/MappedPropertiesUnitTests.java | 19 +- .../json/patch/AddOperationUnitTests.java | 20 ++- .../json/patch/CopyOperationUnitTests.java | 55 +++++- .../webmvc/json/patch/JsonPatchUnitTests.java | 2 +- .../json/patch/JsonPointerMappingTests.java | 69 ++++++++ .../json/patch/MoveOperationUnitTests.java | 20 +-- .../json/patch/PatchOperationUnitTests.java | 2 +- .../json/patch/RemoveOperationTests.java | 4 +- .../json/patch/ReplaceOperationTests.java | 10 +- .../webmvc/json/patch/SpelPathUnitTests.java | 84 ++++++++- .../json/patch/TestOperationUnitTests.java | 8 +- .../json/patch/TestPropertyPathContext.java | 41 +++++ 35 files changed, 888 insertions(+), 192 deletions(-) create mode 100644 spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/BindContextFactory.java create mode 100644 spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/JacksonBindContext.java create mode 100644 spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/PersistentEntitiesBindContextFactory.java create mode 100644 spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/patch/BindContext.java create mode 100644 spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/patch/JsonPointerMapping.java create mode 100644 spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/patch/package-info.java create mode 100644 spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/patch/JsonPointerMappingTests.java create mode 100644 spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/patch/TestPropertyPathContext.java 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 0fbc3e6db..fd036b5a1 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 @@ -19,6 +19,8 @@ import static org.mockito.Mockito.*; import static org.springframework.data.rest.tests.mongodb.TestUtils.*; +import lombok.Data; + import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; @@ -36,21 +38,28 @@ import org.springframework.data.rest.tests.mongodb.Address; import org.springframework.data.rest.tests.mongodb.User; import org.springframework.data.rest.webmvc.RestMediaTypes; +import org.springframework.data.rest.webmvc.json.BindContextFactory; import org.springframework.data.rest.webmvc.json.DomainObjectReader; +import org.springframework.data.rest.webmvc.json.PersistentEntitiesBindContextFactory; +import org.springframework.data.rest.webmvc.json.patch.PatchException; import org.springframework.data.rest.webmvc.mapping.Associations; import org.springframework.http.converter.HttpMessageNotReadableException; +import com.fasterxml.jackson.annotation.JsonIgnore; +import com.fasterxml.jackson.annotation.JsonIgnoreProperties; import com.fasterxml.jackson.databind.ObjectMapper; /** * Unit tests for {@link JsonPatchHandler}. * * @author Oliver Gierke + * @author Mark Paluch */ @ExtendWith(MockitoExtension.class) class JsonPatchHandlerUnitTests { JsonPatchHandler handler; + ObjectMapper mapper = new ObjectMapper(); User user; @Mock ResourceMappings mappings; @@ -63,12 +72,13 @@ void setUp() { MongoMappingContext context = new MongoMappingContext(); context.setSimpleTypeHolder(conversions.getSimpleTypeHolder()); context.getPersistentEntity(User.class); + context.getPersistentEntity(WithIgnoredProperties.class); PersistentEntities entities = new PersistentEntities(Arrays.asList(context)); - Associations associations = new Associations(mappings, mock(RepositoryRestConfiguration.class)); + BindContextFactory factory = new PersistentEntitiesBindContextFactory(entities); - this.handler = new JsonPatchHandler(new ObjectMapper(), new DomainObjectReader(entities, associations)); + this.handler = new JsonPatchHandler(factory, new DomainObjectReader(entities, associations)); Address address = new Address(); address.street = "Foo"; @@ -86,7 +96,7 @@ void appliesRemoveOperationCorrectly() throws Exception { String input = "[{ \"op\": \"replace\", \"path\": \"/address/zipCode\", \"value\": \"ZIP\" }," + "{ \"op\": \"remove\", \"path\": \"/lastname\" }]"; - User result = handler.applyPatch(asStream(input), user); + User result = handler.applyPatch(asStream(input), user, mapper); assertThat(result.lastname).isNull(); assertThat(result.address.zipCode).isEqualTo("ZIP"); @@ -97,7 +107,7 @@ void appliesMergePatchCorrectly() throws Exception { String input = "{ \"address\" : { \"zipCode\" : \"ZIP\"}, \"lastname\" : null }"; - User result = handler.applyMergePatch(asStream(input), user); + User result = handler.applyMergePatch(asStream(input), user, mapper); assertThat(result.lastname).isNull(); assertThat(result.address.zipCode).isEqualTo("ZIP"); @@ -119,7 +129,7 @@ void removesArrayItemCorrectly() throws Exception { String input = "[{ \"op\": \"remove\", \"path\": \"/colleagues/0\" }]"; - handler.applyPatch(asStream(input), user); + handler.applyPatch(asStream(input), user, mapper); assertThat(user.colleagues).hasSize(1); assertThat(user.colleagues.get(0).firstname).isEqualTo(christoph.firstname); @@ -129,7 +139,49 @@ void removesArrayItemCorrectly() throws Exception { void hintsToMediaTypeIfBodyCantBeRead() throws Exception { assertThatExceptionOfType(HttpMessageNotReadableException.class) - .isThrownBy(() -> handler.applyPatch(asStream("{ \"foo\" : \"bar\" }"), new User())) + .isThrownBy(() -> handler.applyPatch(asStream("{ \"foo\" : \"bar\" }"), new User(), mapper)) .withMessageContaining(RestMediaTypes.JSON_PATCH_JSON.toString()); } + + @Test + void skipsReplaceConditionally() throws Exception { + + WithIgnoredProperties object = new WithIgnoredProperties(); + assertThatExceptionOfType(PatchException.class).isThrownBy(() -> { + handler.applyPatch(asStream("[{ \"op\": \"replace\", \"path\": \"/password\", \"value\": \"hello\" }]"), object, + mapper); + }); + + WithIgnoredProperties result = handler + .applyPatch(asStream("[{ \"op\": \"replace\", \"path\": \"/name\", \"value\": \"hello\" }]"), object, mapper); + + assertThat(result.name).isEqualTo("hello"); + } + + @Test + void skipsCopyConditionally() throws Exception { + + WithIgnoredProperties object = new WithIgnoredProperties(); + object.setName("hello"); + + assertThatExceptionOfType(PatchException.class).isThrownBy(() -> { + handler.applyPatch(asStream("[{ \"op\": \"copy\", \"path\": \"/password\", \"from\": \"/name\" }]"), object, + mapper); + }); + + WithIgnoredProperties result = handler + .applyPatch(asStream("[{ \"op\": \"copy\", \"path\": \"/lastname\", \"from\": \"/name\" }]"), object, mapper); + + assertThat(result.lastname).isEqualTo("hello"); + } + + @JsonIgnoreProperties("password") + @Data + static class WithIgnoredProperties { + + String name, lastname, password; + + @JsonIgnore String ssn; + + } } diff --git a/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/config/JsonPatchHandler.java b/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/config/JsonPatchHandler.java index f873d624a..c109591eb 100644 --- a/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/config/JsonPatchHandler.java +++ b/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/config/JsonPatchHandler.java @@ -19,14 +19,15 @@ import org.springframework.data.rest.webmvc.IncomingRequest; import org.springframework.data.rest.webmvc.RestMediaTypes; +import org.springframework.data.rest.webmvc.json.BindContextFactory; import org.springframework.data.rest.webmvc.json.DomainObjectReader; +import org.springframework.data.rest.webmvc.json.patch.BindContext; import org.springframework.data.rest.webmvc.json.patch.JsonPatchPatchConverter; import org.springframework.data.rest.webmvc.json.patch.Patch; import org.springframework.data.rest.webmvc.util.InputStreamHttpInputMessage; import org.springframework.http.converter.HttpMessageNotReadableException; import org.springframework.util.Assert; -import com.fasterxml.jackson.annotation.JsonInclude.Include; import com.fasterxml.jackson.databind.ObjectMapper; import com.fasterxml.jackson.databind.node.ObjectNode; @@ -44,26 +45,23 @@ */ class JsonPatchHandler { - private final ObjectMapper mapper; - private final ObjectMapper sourceMapper; + private final BindContextFactory factory; private final DomainObjectReader reader; /** - * Creates a new {@link JsonPatchHandler} with the given {@link ObjectMapper} and {@link DomainObjectReader}. + * Creates a new {@link JsonPatchHandler} with the given {@link JacksonBindContextFactory} and + * {@link DomainObjectReader}. * - * @param mapper must not be {@literal null}. + * @param factory must not be {@literal null}. * @param reader must not be {@literal null}. */ - public JsonPatchHandler(ObjectMapper mapper, DomainObjectReader reader) { + public JsonPatchHandler(BindContextFactory factory, DomainObjectReader reader) { - Assert.notNull(mapper, "ObjectMapper must not be null!"); - Assert.notNull(reader, "DomainObjectReader must not be null!"); + Assert.notNull(factory, "BindContextFactory must not be null"); + Assert.notNull(reader, "DomainObjectReader must not be null"); - this.mapper = mapper; + this.factory = factory; this.reader = reader; - - this.sourceMapper = mapper.copy(); - this.sourceMapper.setSerializationInclusion(Include.NON_NULL); } /** @@ -74,29 +72,33 @@ public JsonPatchHandler(ObjectMapper mapper, DomainObjectReader reader) { * @return * @throws Exception */ - public T apply(IncomingRequest request, T target) throws Exception { + public T apply(IncomingRequest request, T target, ObjectMapper mapper) throws Exception { Assert.notNull(request, "Request must not be null!"); Assert.isTrue(request.isPatchRequest(), "Cannot handle non-PATCH request!"); Assert.notNull(target, "Target must not be null!"); if (request.isJsonPatchRequest()) { - return applyPatch(request.getBody(), target); + return applyPatch(request.getBody(), target, mapper); } else { - return applyMergePatch(request.getBody(), target); + return applyMergePatch(request.getBody(), target, mapper); } } @SuppressWarnings("unchecked") - T applyPatch(InputStream source, T target) throws Exception { - return getPatchOperations(source).apply(target, (Class) target.getClass()); + T applyPatch(InputStream source, T target, ObjectMapper mapper) throws Exception { + + Class type = target.getClass(); + BindContext context = factory.getBindContextFor(mapper); + + return getPatchOperations(source, mapper, context).apply(target, (Class) target.getClass()); } - T applyMergePatch(InputStream source, T existingObject) throws Exception { + T applyMergePatch(InputStream source, T existingObject, ObjectMapper mapper) throws Exception { return reader.read(source, existingObject, mapper); } - T applyPut(ObjectNode source, T existingObject) throws Exception { + T applyPut(ObjectNode source, T existingObject, ObjectMapper mapper) throws Exception { return reader.readPut(source, existingObject, mapper); } @@ -104,13 +106,14 @@ T applyPut(ObjectNode source, T existingObject) throws Exception { * Returns all {@link JsonPatchOperation}s to be applied. * * @param source must not be {@literal null}. + * @param mapper must not be {@literal null}. * @return * @throws HttpMessageNotReadableException in case the payload can't be read. */ - private Patch getPatchOperations(InputStream source) { + private Patch getPatchOperations(InputStream source, ObjectMapper mapper, BindContext context) { try { - return new JsonPatchPatchConverter(mapper).convert(mapper.readTree(source)); + return new JsonPatchPatchConverter(mapper, context).convert(mapper.readTree(source)); } catch (Exception o_O) { throw new HttpMessageNotReadableException( String.format("Could not read PATCH operations! Expected %s!", RestMediaTypes.JSON_PATCH_JSON), o_O, diff --git a/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/config/PersistentEntityResourceHandlerMethodArgumentResolver.java b/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/config/PersistentEntityResourceHandlerMethodArgumentResolver.java index f2de8b290..075964788 100644 --- a/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/config/PersistentEntityResourceHandlerMethodArgumentResolver.java +++ b/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/config/PersistentEntityResourceHandlerMethodArgumentResolver.java @@ -36,6 +36,7 @@ import org.springframework.data.rest.webmvc.PersistentEntityResource.Builder; import org.springframework.data.rest.webmvc.ResourceNotFoundException; import org.springframework.data.rest.webmvc.RootResourceInformation; +import org.springframework.data.rest.webmvc.json.BindContextFactory; import org.springframework.data.rest.webmvc.json.DomainObjectReader; import org.springframework.data.rest.webmvc.support.BackendIdHandlerMethodArgumentResolver; import org.springframework.http.MediaType; @@ -69,15 +70,15 @@ public class PersistentEntityResourceHandlerMethodArgumentResolver implements Ha private final List> messageConverters; private final RootResourceInformationHandlerMethodArgumentResolver resourceInformationResolver; private final BackendIdHandlerMethodArgumentResolver idResolver; - private final DomainObjectReader reader; private final PluginRegistry, Class> lookups; private final ConversionService conversionService = new DefaultConversionService(); + private final JsonPatchHandler jsonPatchHandler; public PersistentEntityResourceHandlerMethodArgumentResolver( List> messageConverters, RootResourceInformationHandlerMethodArgumentResolver resourceInformationResolver, BackendIdHandlerMethodArgumentResolver idResolver, DomainObjectReader reader, - PluginRegistry, Class> lookups) { + PluginRegistry, Class> lookups, BindContextFactory factory) { Assert.notNull(messageConverters, "HttpMessageConverters must not be null!"); Assert.notNull(resourceInformationResolver, "RootResourceInformation resolver must not be null!"); @@ -88,8 +89,8 @@ public PersistentEntityResourceHandlerMethodArgumentResolver( this.messageConverters = messageConverters; this.resourceInformationResolver = resourceInformationResolver; this.idResolver = idResolver; - this.reader = reader; this.lookups = lookups; + this.jsonPatchHandler = new JsonPatchHandler(mapper -> factory.getBindContextFor(mapper), reader); } /* @@ -210,8 +211,7 @@ private Object readPatch(IncomingRequest request, ObjectMapper mapper, Object ex try { - JsonPatchHandler handler = new JsonPatchHandler(mapper, reader); - return handler.apply(request, existingObject); + return jsonPatchHandler.apply(request, existingObject, mapper); } catch (Exception o_O) { @@ -228,10 +228,9 @@ private Object readPutForUpdate(IncomingRequest request, ObjectMapper mapper, Ob try { - JsonPatchHandler handler = new JsonPatchHandler(mapper, reader); JsonNode jsonNode = mapper.readTree(request.getBody()); - return handler.applyPut((ObjectNode) jsonNode, existingObject); + return jsonPatchHandler.applyPut((ObjectNode) jsonNode, existingObject, mapper); } catch (Exception o_O) { throw new HttpMessageNotReadableException(String.format(ERROR_MESSAGE, existingObject.getClass()), o_O, 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 fa2fc24ce..db1250a52 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 @@ -483,13 +483,15 @@ public PersistentEntityResourceHandlerMethodArgumentResolver persistentEntityArg @Qualifier("defaultMessageConverters") List> defaultMessageConverters, RootResourceInformationHandlerMethodArgumentResolver repoRequestArgumentResolver, Associations associationLinks, BackendIdHandlerMethodArgumentResolver backendIdHandlerMethodArgumentResolver, - PersistentEntities persistentEntities) { + PersistentEntities entities) { PluginRegistry, Class> lookups = PluginRegistry.of(getEntityLookups()); + DomainObjectReader reader = new DomainObjectReader(entities, associationLinks); + BindContextFactory factory = new PersistentEntitiesBindContextFactory(entities); return new PersistentEntityResourceHandlerMethodArgumentResolver(defaultMessageConverters, repoRequestArgumentResolver, backendIdHandlerMethodArgumentResolver, - new DomainObjectReader(persistentEntities, associationLinks), lookups); + reader, lookups, factory); } /** diff --git a/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/BindContextFactory.java b/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/BindContextFactory.java new file mode 100644 index 000000000..963505826 --- /dev/null +++ b/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/BindContextFactory.java @@ -0,0 +1,36 @@ +/* + * Copyright 2022 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. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.springframework.data.rest.webmvc.json; + +import org.springframework.data.rest.webmvc.json.patch.BindContext; + +import com.fasterxml.jackson.databind.ObjectMapper; + +/** + * Factory to create {@link BindContext} instances. + * + * @author Oliver Drotbohm + */ +public interface BindContextFactory { + + /** + * Creates a {@link BindContext} for the given {@link ObjectMapper}. + * + * @param mapper must not be {@literal null}. + * @return will never be {@literal null}. + */ + BindContext getBindContextFor(ObjectMapper mapper); +} diff --git a/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/DomainObjectReader.java b/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/DomainObjectReader.java index c8ba04e8f..f0667b51a 100644 --- a/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/DomainObjectReader.java +++ b/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/DomainObjectReader.java @@ -246,7 +246,7 @@ T doMerge(ObjectNode root, T target, ObjectMapper mapper) throws Exception { JsonNode child = entry.getValue(); String fieldName = entry.getKey(); - if (!mappedProperties.isWritableProperty(fieldName)) { + if (!mappedProperties.isWritableField(fieldName)) { i.remove(); continue; 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 new file mode 100644 index 000000000..395884725 --- /dev/null +++ b/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/JacksonBindContext.java @@ -0,0 +1,74 @@ +/* + * Copyright 2022 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. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.springframework.data.rest.webmvc.json; + +import java.util.Optional; + +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.util.Assert; + +import com.fasterxml.jackson.databind.ObjectMapper; + +/** + * A {@link BindContext} that uses a Jackson {@link ObjectMapper} to inspect its metadata to decide whether segments are + * exposed or not. + * + * @author Oliver Drotbohm + */ +class JacksonBindContext implements BindContext { + + private final PersistentEntities entities; + private final ObjectMapper mapper; + + /** + * Creates a new {@link JacksonBindContext} for the given {@link PersistentEntities} and {@link ObjectMapper}. + * + * @param entities must not be {@literal null}. + * @param mapper must not be {@literal null}. + */ + public JacksonBindContext(PersistentEntities entities, ObjectMapper mapper) { + + Assert.notNull(entities, "PersistentEntities must not be null"); + Assert.notNull(mapper, "ObjectMapper must not be null"); + + this.entities = entities; + this.mapper = mapper; + } + + @Override + public Optional getReadableProperty(String segment, Class type) { + + return getProperty(entities.getPersistentEntity(type) + .map(it -> MappedProperties.forSerialization(it, mapper)) + .filter(it -> it.isReadableField(segment)), segment); + } + + @Override + public Optional getWritableProperty(String segment, Class type) { + + return getProperty(entities.getPersistentEntity(type) + .map(it -> MappedProperties.forDeserialization(it, mapper)) + .filter(it -> it.isWritableField(segment)), segment); + } + + private static Optional getProperty(Optional properties, String segment) { + + return properties.map(it -> it.getPersistentProperty(segment)) + .map(PersistentProperty::getName); + } +} diff --git a/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/MappedProperties.java b/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/MappedProperties.java index f8c3f582a..b8226a69b 100644 --- a/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/MappedProperties.java +++ b/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/MappedProperties.java @@ -26,9 +26,11 @@ import org.springframework.data.mapping.PersistentEntity; import org.springframework.data.mapping.PersistentProperty; +import org.springframework.lang.Nullable; import org.springframework.util.Assert; import com.fasterxml.jackson.annotation.JsonAnySetter; +import com.fasterxml.jackson.annotation.JsonIgnoreProperties; import com.fasterxml.jackson.databind.BeanDescription; import com.fasterxml.jackson.databind.DeserializationConfig; import com.fasterxml.jackson.databind.ObjectMapper; @@ -83,7 +85,15 @@ private MappedProperties(PersistentEntity> en // collection of ignored properties in the first place. See // https://github.com/FasterXML/jackson-databind/issues/2531 - this.ignoredPropertyNames = description.getIgnoredPropertyNames(); + this.ignoredPropertyNames = new HashSet<>(description.getIgnoredPropertyNames()); + + JsonIgnoreProperties annotation = entity.findAnnotation(JsonIgnoreProperties.class); + + if (annotation != null) { + for (String property : annotation.value()) { + ignoredPropertyNames.add(property); + } + } for (BeanPropertyDefinition property : description.findProperties()) { @@ -172,6 +182,7 @@ public boolean hasPersistentPropertyForField(String fieldName) { * @param fieldName must not be empty or {@literal null}. * @return the {@link PersistentProperty} backing the field with the field name. */ + @Nullable public PersistentProperty getPersistentProperty(String fieldName) { Assert.hasText(fieldName, "Field name must not be null or empty!"); @@ -229,7 +240,31 @@ public boolean isMappedProperty(PersistentProperty property) { * @param name must not be {@literal null} or empty. * @return */ - public boolean isWritableProperty(String name) { + public boolean isWritableField(String name) { + + Assert.hasText(name, "Property name must not be null or empty"); + + if (ignoredPropertyNames.contains(name)) { + return false; + } + + PersistentProperty property = fieldNameToProperty.get(name); + + return property != null ? property.isWritable() : anySetterFound; + } + + public boolean isReadableField(String name) { + + Assert.hasText(name, "Property name must not be null or empty"); + + if (ignoredPropertyNames.contains(name)) { + return false; + } + + return fieldNameToProperty.get(name) != null; + } + + public boolean isExposedProperty(String name) { Assert.hasText(name, "Property name must not be null or empty!"); 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 new file mode 100644 index 000000000..ac7d71a32 --- /dev/null +++ b/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/PersistentEntitiesBindContextFactory.java @@ -0,0 +1,49 @@ +/* + * Copyright 2022 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. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.springframework.data.rest.webmvc.json; + +import org.springframework.data.mapping.context.PersistentEntities; +import org.springframework.data.rest.webmvc.json.patch.BindContext; +import org.springframework.util.Assert; + +import com.fasterxml.jackson.databind.ObjectMapper; + +/** + * A {@link BindContextFactory} based on {@link PersistentEntities}. + * + * @author Oliver Drotbohm + */ +public class PersistentEntitiesBindContextFactory implements BindContextFactory { + + private final PersistentEntities entities; + + /** + * Creates a new {@link PersistentEntitiesBindContextFactory} for the given {@link PersistentEntities}. + * + * @param entities must not be {@literal null}. + */ + public PersistentEntitiesBindContextFactory(PersistentEntities entities) { + + Assert.notNull(entities, "PersistentEntities must not be null!"); + + this.entities = entities; + } + + @Override + public BindContext getBindContextFor(ObjectMapper mapper) { + return new JacksonBindContext(entities, mapper); + } +} 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 b8344b3e5..0c0f8072a 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 @@ -42,11 +42,12 @@ public static AddOperation of(String path, Object value) { /* * (non-Javadoc) - * @see org.springframework.data.rest.webmvc.json.patch.PatchOperation#perform(java.lang.Object, java.lang.Class) + * @see org.springframework.data.rest.webmvc.json.patch.PatchOperation#perform(java.lang.Object, java.lang.Class, org.springframework.data.rest.webmvc.json.PropertyFilter) */ @Override - void perform(Object targetObject, Class type) { - path.bindTo(type).addValue(targetObject, evaluateValueFromTarget(targetObject, type)); + void perform(Object target, Class type, BindContext context) { + + path.bindForWrite(type, context).addValue(target, evaluateValueFromTarget(target, type, context)); } /* @@ -54,12 +55,12 @@ void perform(Object targetObject, Class type) { * @see org.springframework.data.rest.webmvc.json.patch.PatchOperation#evaluateValueFromTarget(java.lang.Object, java.lang.Class) */ @Override - protected Object evaluateValueFromTarget(Object targetObject, Class entityType) { + protected Object evaluateValueFromTarget(Object targetObject, Class entityType, BindContext context) { if (!path.isAppend()) { - return super.evaluateValueFromTarget(targetObject, entityType); + return super.evaluateValueFromTarget(targetObject, entityType, context); } - return evaluate(path.bindTo(entityType).getLeafType()); + return evaluate(path.bindForWrite(entityType, context).getLeafType()); } } 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 new file mode 100644 index 000000000..df204ccac --- /dev/null +++ b/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/patch/BindContext.java @@ -0,0 +1,44 @@ +/* + * Copyright 2022 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. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.springframework.data.rest.webmvc.json.patch; + +import java.util.Optional; + +/** + * Contextual mapping for he translation of JSON Pointer segments into property references on persistent types. + * + * @author Oliver Drotbohm + */ +public interface BindContext { + + /** + * Returns the name of the writable property for the given JSON pointer segment. + * + * @param segment must not be {@literal null} or empty. + * @param type must not be {@literal null}. + * @return will never be {@literal null}. + */ + Optional getWritableProperty(String segment, Class type); + + /** + * Return the name of the readable property for the given JSON pointer segment. + * + * @param segment must not be {@literal null} or empty. + * @param type must not be {@literal null}. + * @return will never be {@literal null}. + */ + Optional getReadableProperty(String segment, Class type); +} 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 52792f2b3..cedf402ae 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 @@ -82,7 +82,9 @@ CopyOperation to(String to) { * @see org.springframework.data.rest.webmvc.json.patch.PatchOperation#perform(java.lang.Object, java.lang.Class) */ @Override - void perform(Object target, Class type) { - path.bindTo(type).copyFrom(from, target); + void perform(Object target, Class type, BindContext context) { + + path.bindForWrite(type, context) // + .copyFrom(from, target, context); } } diff --git a/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/patch/JsonPatchPatchConverter.java b/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/patch/JsonPatchPatchConverter.java index 7d4378b38..70e283efd 100644 --- a/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/patch/JsonPatchPatchConverter.java +++ b/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/patch/JsonPatchPatchConverter.java @@ -36,12 +36,14 @@ public class JsonPatchPatchConverter implements PatchConverter { private final ObjectMapper mapper; + private final BindContext context; - public JsonPatchPatchConverter(ObjectMapper mapper) { + public JsonPatchPatchConverter(ObjectMapper mapper, BindContext context) { Assert.notNull(mapper, "ObjectMapper must not be null!"); this.mapper = mapper; + this.context = context; } /** @@ -87,7 +89,7 @@ public Patch convert(JsonNode jsonNode) { } } - return new Patch(ops); + return new Patch(ops, context); } private Object valueFromJsonNode(String path, JsonNode valueNode) { diff --git a/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/patch/JsonPointerMapping.java b/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/patch/JsonPointerMapping.java new file mode 100644 index 000000000..14d46a033 --- /dev/null +++ b/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/patch/JsonPointerMapping.java @@ -0,0 +1,127 @@ +/* + * Copyright 2022 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. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.springframework.data.rest.webmvc.json.patch; + +import lombok.RequiredArgsConstructor; + +import java.util.Optional; +import java.util.function.BiFunction; + +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.util.StringUtils; + +/** + * @author Oliver Drotbohm + */ +@RequiredArgsConstructor +class JsonPointerMapping { + + private final BiFunction, Optional> reader, writer; + + public JsonPointerMapping(BindContext context) { + + this.reader = context::getReadableProperty; + this.writer = context::getWritableProperty; + } + + /** + * Maps the given JSON Pointer to the given type to ultimately read the attribute pointed to. + * + * @param pointer must not be {@literal null}. + * @param type must not be {@literal null}. + * @return a JSON Pointer with the segments translated into the matching property references. + */ + public String forRead(String pointer, Class type) { + return verify(pointer, type, reader, "readable"); + } + + /** + * Maps the given JSON Pointer to the given type to ultimately write the attribute pointed to. + * + * @param pointer must not be {@literal null}. + * @param type must not be {@literal null}. + * @return a JSON Pointer with the segments translated into the matching property references. + */ + public String forWrite(String pointer, Class type) { + return verify(pointer, type, writer, "writable"); + } + + private String verify(String pointer, Class type, BiFunction, Optional> filter, + String qualifier) { + + String[] strings = pointer.split("/"); + + if (strings.length == 0) { + return pointer; + } + + PropertyPath base = null; + StringBuilder result = new StringBuilder(); + TypeInformation currentType = ClassTypeInformation.from(type); + + for (int i = 0; i < strings.length; i++) { + + String segment = strings[i]; + + if (!StringUtils.hasText(segment)) { + continue; + } + + if (currentType != null && currentType.isMap()) { + + result.append("/").append(segment); + currentType = currentType.getActualType(); + + continue; + } + + if (segment.equals("-") || segment.matches("\\d+")) { + result.append("/").append(segment); + currentType = currentType.getActualType(); + continue; + } + + TypeInformation rejectType = currentType; + + // Use given filter for final segment, reader otherwise + String property = (i == strings.length - 1 ? filter : reader) // + .apply(segment, currentType.getType()) // + .orElseThrow(() -> reject(segment, rejectType, pointer, qualifier)); + + try { + base = base == null ? PropertyPath.from(property, type) : base.nested(segment); + } catch (PropertyReferenceException o_O) { + throw reject(segment, rejectType, pointer, qualifier); + } + + currentType = base.getTypeInformation(); + + result.append("/").append(property); + } + + return result.toString(); + } + + private static PatchException reject(String segment, TypeInformation type, String pointer, String qualifier) { + + return new PatchException( + String.format("Couldn't find %s property for pointer segment %s on %s in %s", qualifier, segment, + type.getType(), pointer)); + } +} 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 e75a6a201..1a829fe82 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 @@ -75,7 +75,7 @@ public MoveOperation to(String to) { * @see org.springframework.data.rest.webmvc.json.patch.PatchOperation#perform(java.lang.Object, java.lang.Class) */ @Override - void perform(Object target, Class type) { - path.bindTo(type).moveFrom(from, target); + void perform(Object target, Class type, BindContext context) { + path.bindForWrite(type, context).moveFrom(from, target, context); } } 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 ec4fc9425..2c2154359 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 @@ -35,9 +35,12 @@ public class Patch implements Streamable { private final List operations; + private final BindContext context; + + public Patch(List operations, BindContext context) { - public Patch(List operations) { this.operations = operations; + this.context = context; } /** @@ -60,7 +63,7 @@ public int size() { public T apply(T in, Class type) throws PatchException { for (PatchOperation operation : operations) { - operation.perform(in, type); + operation.perform(in, type, context); } return in; @@ -79,7 +82,7 @@ public T apply(T in, Class type) throws PatchException { public List apply(List in, Class type) throws PatchException { for (PatchOperation operation : operations) { - operation.perform(in, type); + operation.perform(in, type, context); } return in; 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 57c4dc795..9efab2cc9 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 @@ -59,8 +59,8 @@ protected PatchOperation(String op, UntypedSpelPath path, Object value) { * @return the result of late-value evaluation if the value is a {@link LateObjectEvaluator}; the value itself * otherwise. */ - protected Object evaluateValueFromTarget(Object targetObject, Class entityType) { - return evaluate(path.bindTo(entityType).getType(targetObject)); + protected Object evaluateValueFromTarget(Object targetObject, Class entityType, BindContext context) { + return evaluate(path.bindForRead(entityType, context).getType(targetObject)); } protected final Object evaluate(Class type) { @@ -73,5 +73,5 @@ protected final Object evaluate(Class type) { * @param target the target of the operation, must not be {@literal null}. * @param type must not be {@literal null}. */ - abstract void perform(Object target, Class type); + abstract void perform(Object target, Class type, BindContext context); } 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 544912b37..d791b4083 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 @@ -44,7 +44,7 @@ public static RemoveOperation valueAt(String path) { * @see org.springframework.data.rest.webmvc.json.patch.PatchOperation#perform(java.lang.Object, java.lang.Class) */ @Override - void perform(Object target, Class type) { - path.bindTo(type).removeFrom(target); + void perform(Object target, Class type, BindContext context) { + path.bindForWrite(type, context).removeFrom(target); } } 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 b9aaf2bcf..5d5274cd8 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 @@ -39,7 +39,6 @@ public static ReplaceOperationBuilder valueAt(String path) { return new ReplaceOperationBuilder(path); } - static class ReplaceOperationBuilder { private final String path; @@ -58,7 +57,7 @@ private ReplaceOperationBuilder(final String path) { * @see org.springframework.data.rest.webmvc.json.patch.PatchOperation#perform(java.lang.Object, java.lang.Class) */ @Override - void perform(Object target, Class type) { - path.bindTo(type).setValue(target, evaluateValueFromTarget(target, type)); + void perform(Object target, Class type, BindContext context) { + path.bindForWrite(type, context).setValue(target, evaluateValueFromTarget(target, type, context)); } } 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 f472073ab..e147f2b00 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 @@ -42,10 +42,14 @@ import org.springframework.util.ConcurrentReferenceHashMap; import org.springframework.util.StringUtils; +import com.fasterxml.jackson.databind.ObjectMapper; + /** * Value object to represent a SpEL-backed patch path. * * @author Oliver Gierke + * @author Mark Paluch + * @author Greg Turnquist */ class SpelPath { @@ -53,6 +57,8 @@ class SpelPath { private static final String APPEND_CHARACTER = "-"; private static final Map UNTYPED_PATHS = new ConcurrentReferenceHashMap<>(32); + private static final ObjectMapper objectMapper = new ObjectMapper(); + protected final String path; private SpelPath(String path) { @@ -62,10 +68,6 @@ private SpelPath(String path) { this.path = path; } - public String getPath() { - return this.path; - } - /** * Returns a {@link UntypedSpelPath} for the given source. * @@ -76,16 +78,6 @@ public static UntypedSpelPath untyped(String source) { return UNTYPED_PATHS.computeIfAbsent(source, UntypedSpelPath::new); } - /** - * Returns a {@link TypedSpelPath} for the given source and type. - * - * @param source must not be {@literal null}. - * @return - */ - public static TypedSpelPath typed(String source, Class type) { - return untyped(source).bindTo(type); - } - /** * Returns whether the current path represents an append path, i.e. is supposed to append to collection. * @@ -135,55 +127,61 @@ public int hashCode() { static class UntypedSpelPath extends SpelPath { + private static final Map READ_PATHS = new ConcurrentReferenceHashMap<>(256); + private static final Map WRITE_PATHS = new ConcurrentReferenceHashMap<>(256); + private UntypedSpelPath(String path) { super(path); } + public ReadingOperations bindForRead(Class type, BindContext context) { + + Assert.notNull(path, "Path must not be null"); + Assert.notNull(type, "Type must not be null"); + + 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); + }); + } + /** * Returns a {@link TypedSpelPath} binding the expression to the given type. * * @param type must not be {@literal null}. * @return */ - public TypedSpelPath bindTo(Class type) { + public WritingOperations bindForWrite(Class type, BindContext context) { - Assert.notNull(type, "Type must not be null!"); + Assert.notNull(context, "BindContext must not be null"); + Assert.notNull(type, "Type must not be null"); - return TypedSpelPath.of(this, type); + 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); + }); } - } - - /** - * A {@link SpelPath} that has typing information tied to it. - * - * @author Oliver Gierke - */ - static class TypedSpelPath extends 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 Map TYPED_PATHS = new ConcurrentReferenceHashMap<>(32); - private static final EvaluationContext CONTEXT = SimpleEvaluationContext.forReadWriteDataBinding().build(); - - private final Expression expression; - private final Class type; private static final class CacheKey { private final Class type; private final UntypedSpelPath path; + private final BindContext context; - private CacheKey(Class type, UntypedSpelPath path) { + private CacheKey(Class type, UntypedSpelPath path, BindContext context) { Assert.notNull(type, "Type must not be null!"); Assert.notNull(path, "UntypedSpelPath must not be null!"); this.type = type; this.path = path; + this.context = context; } - public static CacheKey of(final Class type, final UntypedSpelPath path) { - return new CacheKey(type, path); + public static CacheKey of(Class type, UntypedSpelPath path, BindContext context) { + return new CacheKey(type, path, context); } /* @@ -204,7 +202,8 @@ public boolean equals(Object o) { CacheKey that = (CacheKey) o; return Objects.equals(type, that.type) // - && Objects.equals(path, that.path); + && Objects.equals(path, that.path) // + && Objects.equals(context, that.context); } /* @@ -213,40 +212,58 @@ public boolean equals(Object o) { */ @Override public int hashCode() { - return Objects.hash(type, path); - } - - /* - * (non-Javadoc) - * @see java.lang.Object#toString() - */ - @Override - public java.lang.String toString() { - return "SpelPath.TypedSpelPath.CacheKey(type=" + type + ", path=" + path + ")"; + return Objects.hash(type, path, context); } } + } - private TypedSpelPath(UntypedSpelPath path, Class type) { + interface CommonOperations { - super(path.path); + String getExpressionString(); + } - this.type = type; - this.expression = toSpel(path.path, type); - } + interface ReadingOperations extends CommonOperations { - /** - * Returns the {@link TypedSpelPath} for the given {@link SpelPath} and type. - * - * @param path must not be {@literal null}. - * @param type must not be {@literal null}. - * @return - */ - public static TypedSpelPath of(UntypedSpelPath path, Class type) { + T getValue(Object target); - Assert.notNull(path, "Path must not be null!"); - Assert.notNull(type, "Type must not be null!"); + Class getType(Object root); + } + + interface WritingOperations extends CommonOperations { + + Class getLeafType(); + + Object removeFrom(Object target); + + void addValue(Object target, Object value); + + void setValue(Object target, @Nullable Object value); + + void copyFrom(UntypedSpelPath path, Object source, BindContext context); + + void moveFrom(UntypedSpelPath path, Object source, BindContext context); + } - return TYPED_PATHS.computeIfAbsent(CacheKey.of(type, path), key -> new TypedSpelPath(key.path, key.type)); + /** + * A {@link SpelPath} that has typing information tied to it. + * + * @author Oliver Gierke + */ + static class TypedSpelPath extends SpelPath implements ReadingOperations, WritingOperations { + + 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 TypedSpelPath(String path, Class type) { + + super(path); + + this.type = type; + this.expression = toSpel(path, type); } /** @@ -334,12 +351,12 @@ public Class getType(Object root) { * @param source the source object to look the value up from, must not be {@literal null}. * @return */ - public void copyFrom(UntypedSpelPath path, Object source) { + public void copyFrom(UntypedSpelPath path, Object source, BindContext context) { Assert.notNull(path, "Source path must not be null!"); Assert.notNull(source, "Source value must not be null!"); - addValue(source, path.bindTo(type).getValue(source)); + addValue(source, path.bindForRead(type, context).getValue(source)); } /** @@ -350,12 +367,15 @@ public void copyFrom(UntypedSpelPath path, Object source) { * @param source the source object to look the value up from, must not be {@literal null}. * @return */ - public void moveFrom(UntypedSpelPath path, Object source) { + public void moveFrom(UntypedSpelPath path, Object source, BindContext context) { Assert.notNull(path, "Source path must not be null!"); Assert.notNull(source, "Source value must not be null!"); - addValue(source, path.bindTo(type).removeFrom(source)); + // Verify we are allowed to read the source + path.bindForRead(type, context); + + addValue(source, path.bindForWrite(type, context).removeFrom(source)); } /** @@ -377,7 +397,7 @@ public Object removeFrom(Object target) { setValue(target, null); return value; } catch (SpelEvaluationException o_O) { - throw new PatchException("Path '" + path + "' is not nullable.", o_O); + throw new PatchException("Path '" + path + "' is not nullable", o_O); } } else { @@ -440,10 +460,7 @@ public String toString() { } private TypedSpelPath getParent() { - - return SpelPath // - .untyped(path.substring(0, path.lastIndexOf('/'))) // - .bindTo(type); + return new TypedSpelPath(path.substring(0, path.lastIndexOf('/')), type); } private TypeDescriptor getTypeDescriptor(Object target) { @@ -658,6 +675,8 @@ private SpelExpressionBuilder nested(String segment) { ? spelSegment.concat(".") // : spelSegment; + Class currentType = basePath == null ? type : basePath.getLeafType(); + try { PropertyPath path = basePath == null // 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 43f31a068..c370f9532 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 @@ -71,10 +71,10 @@ public TestOperation hasValue(Object value) { * @see org.springframework.data.rest.webmvc.json.patch.PatchOperation#perform(java.lang.Object, java.lang.Class) */ @Override - void perform(Object target, Class type) { + void perform(Object target, Class type, BindContext context) { - Object expected = normalizeIfNumber(evaluateValueFromTarget(target, type)); - Object actual = normalizeIfNumber(path.bindTo(type).getValue(target)); + Object expected = normalizeIfNumber(evaluateValueFromTarget(target, type, context)); + Object actual = normalizeIfNumber(path.bindForRead(type, context).getValue(target)); if (!ObjectUtils.nullSafeEquals(expected, actual)) { throw new PatchException("Test against path '" + path + "' failed."); diff --git a/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/patch/package-info.java b/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/patch/package-info.java new file mode 100644 index 000000000..1a74dd464 --- /dev/null +++ b/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/patch/package-info.java @@ -0,0 +1,17 @@ +/* + * Copyright 2013-2022 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. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +@org.springframework.lang.NonNullApi +package org.springframework.data.rest.webmvc.json.patch; diff --git a/spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/config/PersistentEntityResourceHandlerMethodArgumentResolverUnitTests.java b/spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/config/PersistentEntityResourceHandlerMethodArgumentResolverUnitTests.java index a03d42398..7823c84c6 100644 --- a/spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/config/PersistentEntityResourceHandlerMethodArgumentResolverUnitTests.java +++ b/spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/config/PersistentEntityResourceHandlerMethodArgumentResolverUnitTests.java @@ -36,6 +36,8 @@ import org.springframework.data.rest.webmvc.PersistentEntityResource; import org.springframework.data.rest.webmvc.RootResourceInformation; import org.springframework.data.rest.webmvc.json.DomainObjectReader; +import org.springframework.data.rest.webmvc.json.BindContextFactory; +import org.springframework.data.rest.webmvc.json.patch.TestPropertyPathContext; import org.springframework.data.rest.webmvc.support.BackendIdHandlerMethodArgumentResolver; import org.springframework.http.HttpInputMessage; import org.springframework.http.MediaType; @@ -55,6 +57,8 @@ */ class PersistentEntityResourceHandlerMethodArgumentResolverUnitTests { + private static final BindContextFactory FACTORY = mapper -> TestPropertyPathContext.INSTANCE; + HttpMessageConverter converter; RootResourceInformationHandlerMethodArgumentResolver rootResourceResolver; BackendIdHandlerMethodArgumentResolver backendIdResolver; @@ -79,7 +83,7 @@ void returnsAggregateInstanceWithIdentifierPopulatedForPutRequests() throws Exce PersistentEntityResourceHandlerMethodArgumentResolver argumentResolver = new PersistentEntityResourceHandlerMethodArgumentResolver( Arrays.> asList(converter), rootResourceResolver, backendIdResolver, reader, - PluginRegistry.empty()); + PluginRegistry.empty(), FACTORY); HttpServletRequest request = new MockHttpServletRequest("PUT", "/foo/4711"); @@ -103,7 +107,7 @@ void setsLookupPropertyForEntitiesWithCustomLookup() throws Exception { PersistentEntityResourceHandlerMethodArgumentResolver argumentResolver = new PersistentEntityResourceHandlerMethodArgumentResolver( Arrays.> asList(converter), rootResourceResolver, backendIdResolver, reader, - PluginRegistry.of(Arrays.asList(lookup))); + PluginRegistry.of(Arrays.asList(lookup)), FACTORY); HttpServletRequest request = new MockHttpServletRequest("PUT", "/foo/someName"); diff --git a/spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/MappedPropertiesUnitTests.java b/spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/MappedPropertiesUnitTests.java index 06361f090..efcd3915a 100755 --- a/spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/MappedPropertiesUnitTests.java +++ b/spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/MappedPropertiesUnitTests.java @@ -25,6 +25,7 @@ import com.fasterxml.jackson.annotation.JsonAnySetter; import com.fasterxml.jackson.annotation.JsonIgnore; +import com.fasterxml.jackson.annotation.JsonIgnoreProperties; import com.fasterxml.jackson.annotation.JsonProperty; import com.fasterxml.jackson.annotation.JsonProperty.Access; import com.fasterxml.jackson.databind.ObjectMapper; @@ -91,7 +92,7 @@ void doesNotRegardReadOnlyPropertyForDeserialization() { MappedProperties properties = MappedProperties.forDeserialization(entity, mapper); - assertThat(properties.isWritableProperty("anotherReadOnlyProperty")).isFalse(); + assertThat(properties.isWritableField("anotherReadOnlyProperty")).isFalse(); assertThat(properties.getPersistentProperty("readOnlyProperty")).isNull(); properties = MappedProperties.forSerialization(entity, mapper); @@ -107,12 +108,12 @@ void exposesExistanceOfCatchAllMethod() { MappedProperties properties = MappedProperties.forDeserialization(entity, mapper); - assertThat(properties.isWritableProperty("someProperty")).isTrue(); - assertThat(properties.isWritableProperty("readOnlyProperty")).isFalse(); - assertThat(properties.isWritableProperty("anotherReadOnlyProperty")).isFalse(); + assertThat(properties.isWritableField("someProperty")).isTrue(); + assertThat(properties.isWritableField("readOnlyProperty")).isFalse(); + assertThat(properties.isWritableField("anotherReadOnlyProperty")).isFalse(); // Due to @JsonAnySetter - assertThat(properties.isWritableProperty("someRandomProperty")).isTrue(); + assertThat(properties.isWritableField("someRandomProperty")).isTrue(); } @Test // #2130 @@ -120,6 +121,13 @@ void exposesIgnoredProperties() { assertThat(properties.getIgnoredProperties()).contains("notExposedByJackson"); } + @Test + void ignoresTypeLevelProperties() { + + assertThat(properties.getIgnoredProperties()).contains("typeLevelIgnored"); + } + + @JsonIgnoreProperties("typeLevelIgnored") static class Sample { public @Transient String notExposedBySpringData; @@ -128,6 +136,7 @@ static class Sample { public @JsonProperty("email") String emailAddress; public @JsonProperty(access = Access.READ_ONLY) String readOnlyProperty; public @ReadOnlyProperty String anotherReadOnlyProperty; + public String typeLevelIgnored; } static class SampleWithJsonAnySetter { diff --git a/spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/patch/AddOperationUnitTests.java b/spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/patch/AddOperationUnitTests.java index 7e29bbca9..2b323268b 100755 --- a/spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/patch/AddOperationUnitTests.java +++ b/spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/patch/AddOperationUnitTests.java @@ -40,7 +40,7 @@ void addBooleanPropertyValue() throws Exception { todos.add(new Todo(3L, "C", false)); AddOperation add = AddOperation.of("/1/complete", true); - add.perform(todos, Todo.class); + add.perform(todos, Todo.class, TestPropertyPathContext.INSTANCE); assertThat(todos.get(1).isComplete()).isTrue(); } @@ -54,7 +54,7 @@ void addStringPropertyValue() throws Exception { todos.add(new Todo(3L, "C", false)); AddOperation add = AddOperation.of("/1/description", "BBB"); - add.perform(todos, Todo.class); + add.perform(todos, Todo.class, TestPropertyPathContext.INSTANCE); assertThat(todos.get(1).getDescription()).isEqualTo("BBB"); } @@ -68,7 +68,7 @@ void addItemToList() throws Exception { todos.add(new Todo(3L, "C", false)); AddOperation add = AddOperation.of("/1", new Todo(null, "D", true)); - add.perform(todos, Todo.class); + add.perform(todos, Todo.class, TestPropertyPathContext.INSTANCE); assertThat(todos.size()).isEqualTo(4); assertThat(todos.get(0).getDescription()).isEqualTo("A"); @@ -86,7 +86,7 @@ void addsItemsToNestedList() { Todo todo = new Todo(1L, "description", false); - AddOperation.of("/items/-", "Some text.").perform(todo, Todo.class); + AddOperation.of("/items/-", "Some text.").perform(todo, Todo.class, TestPropertyPathContext.INSTANCE); assertThat(todo.getItems().get(0)).isEqualTo("Some text."); } @@ -100,7 +100,7 @@ void addsLazilyEvaluatedObjectToList() throws Exception { JsonNode node = mapper.readTree("\"Some text.\""); JsonLateObjectEvaluator evaluator = new JsonLateObjectEvaluator(mapper, node); - AddOperation.of("/items/-", evaluator).perform(todo, Todo.class); + AddOperation.of("/items/-", evaluator).perform(todo, Todo.class, TestPropertyPathContext.INSTANCE); assertThat(todo.getItems().get(0)).isEqualTo("Some text."); } @@ -110,7 +110,7 @@ void initializesNullCollectionsOnAppend() { Todo todo = new Todo(1L, "description", false); - AddOperation.of("/uninitialized/-", "Text").perform(todo, Todo.class); + AddOperation.of("/uninitialized/-", "Text").perform(todo, Todo.class, TestPropertyPathContext.INSTANCE); assertThat(todo.getUninitialized()).containsExactly("Text"); } @@ -122,7 +122,7 @@ void addsItemToTheEndOfACollectionViaIndex() { todos.add(new Todo(1L, "A", false)); Todo todo = new Todo(2L, "B", true); - AddOperation.of("/1", todo).perform(todos, Todo.class); + AddOperation.of("/1", todo).perform(todos, Todo.class, TestPropertyPathContext.INSTANCE); assertThat(todos).element(1).isEqualTo(todo); } @@ -134,7 +134,8 @@ void rejectsAdditionBeyondEndOfList() { todos.add(new Todo(1L, "A", false)); assertThatExceptionOfType(PatchException.class) // - .isThrownBy(() -> AddOperation.of("/2", new Todo(2L, "B", true)).perform(todos, Todo.class)) // + .isThrownBy(() -> AddOperation.of("/2", new Todo(2L, "B", true)).perform(todos, Todo.class, + TestPropertyPathContext.INSTANCE)) // .withMessageContaining("index") // .withMessageContaining("2") // .withMessageContaining("1"); @@ -152,7 +153,8 @@ void manipulatesNestedCollectionProperly() { TodoListWrapper outer = new TodoListWrapper(todoList); Todo newTodo = new Todo(3L, "C", false); - AddOperation.of("/todoList/todos/-", newTodo).perform(outer, TodoListWrapper.class); + AddOperation.of("/todoList/todos/-", newTodo).perform(outer, TodoListWrapper.class, + TestPropertyPathContext.INSTANCE); assertThat(outer.todoList.getTodos()).containsExactly(todos.get(0), todos.get(1), newTodo); } diff --git a/spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/patch/CopyOperationUnitTests.java b/spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/patch/CopyOperationUnitTests.java index 884db243f..f2e135711 100755 --- a/spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/patch/CopyOperationUnitTests.java +++ b/spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/patch/CopyOperationUnitTests.java @@ -19,6 +19,7 @@ import java.util.ArrayList; import java.util.List; +import java.util.Optional; import org.junit.jupiter.api.Test; @@ -33,7 +34,7 @@ void copyBooleanPropertyValue() throws Exception { todos.add(new Todo(3L, "C", false)); CopyOperation copy = CopyOperation.from("/0/complete").to("/1/complete"); - copy.perform(todos, Todo.class); + copy.perform(todos, Todo.class, TestPropertyPathContext.INSTANCE); assertThat(todos.get(1).isComplete()).isTrue(); } @@ -47,7 +48,7 @@ void copyStringPropertyValue() throws Exception { todos.add(new Todo(3L, "C", false)); CopyOperation copy = CopyOperation.from("/0/description").to("/1/description"); - copy.perform(todos, Todo.class); + copy.perform(todos, Todo.class, TestPropertyPathContext.INSTANCE); assertThat(todos.get(1).getDescription()).isEqualTo("A"); } @@ -61,7 +62,7 @@ void copyBooleanPropertyValueIntoStringProperty() throws Exception { todos.add(new Todo(3L, "C", false)); CopyOperation copy = CopyOperation.from("/0/complete").to("/1/description"); - copy.perform(todos, Todo.class); + copy.perform(todos, Todo.class, TestPropertyPathContext.INSTANCE); assertThat(todos.get(1).getDescription()).isEqualTo("true"); } @@ -75,7 +76,7 @@ void copyListElementToBeginningOfList() throws Exception { todos.add(new Todo(3L, "C", false)); CopyOperation copy = CopyOperation.from("/1").to("/0"); - copy.perform(todos, Todo.class); + copy.perform(todos, Todo.class, TestPropertyPathContext.INSTANCE); assertThat(todos.size()).isEqualTo(4); assertThat(todos.get(0).getId().longValue()).isEqualTo(2L); // NOTE: This could be problematic if you try to save it @@ -94,7 +95,7 @@ void copyListElementToMiddleOfList() throws Exception { todos.add(new Todo(3L, "C", false)); CopyOperation copy = CopyOperation.from("/0").to("/2"); - copy.perform(todos, Todo.class); + copy.perform(todos, Todo.class, TestPropertyPathContext.INSTANCE); assertThat(todos.size()).isEqualTo(4); assertThat(todos.get(2).getId().longValue()).isEqualTo(1L); // NOTE: This could be problematic if you try to save it @@ -113,7 +114,7 @@ void copyListElementToEndOfList_usingIndex() throws Exception { todos.add(new Todo(3L, "C", false)); CopyOperation copy = CopyOperation.from("/0").to("/3"); - copy.perform(todos, Todo.class); + copy.perform(todos, Todo.class, TestPropertyPathContext.INSTANCE); assertThat(todos.size()).isEqualTo(4); assertThat(todos.get(3).getId().longValue()).isEqualTo(1L); // NOTE: This could be problematic if you try to save it @@ -132,7 +133,7 @@ void copyListElementToEndOfList_usingDash() throws Exception { todos.add(new Todo(3L, "C", false)); CopyOperation copy = CopyOperation.from("/0").to("/-"); - copy.perform(todos, Todo.class); + copy.perform(todos, Todo.class, TestPropertyPathContext.INSTANCE); assertThat(todos.size()).isEqualTo(4); assertThat(todos.get(3)).isEqualTo(new Todo(1L, "A", true)); // NOTE: This could be problematic if you try to save @@ -148,10 +149,48 @@ void copyListElementFromEndOfList_usingDash() throws Exception { todos.add(new Todo(3L, "C", false)); CopyOperation copy = CopyOperation.from("/-").to("/0"); - copy.perform(todos, Todo.class); + copy.perform(todos, Todo.class, TestPropertyPathContext.INSTANCE); assertThat(todos.size()).isEqualTo(4); assertThat(todos.get(0)).isEqualTo(new Todo(3L, "C", false)); // NOTE: This could be problematic if you try to save // it to a DB because there'll be duplicate IDs } + + @Test + void rejectsCopyingFromHiddenProperty() { + + BindContext context = new TestPropertyPathContext() { + + @Override + public Optional getReadableProperty(String segment, Class type) { + return Optional.of(segment).filter(it -> !"description".equals(it)); + } + }; + + CopyOperation operation = CopyOperation.from("/description").to("/description"); + Todo target = new Todo(1L, "Description", false); + + assertThatExceptionOfType(PatchException.class) + .isThrownBy(() -> operation.perform(target, Todo.class, context)) + .withMessageContaining("readable property"); + } + + @Test + void rejectsCopyingToHiddenProperty() { + + BindContext context = new TestPropertyPathContext() { + + @Override + public Optional getWritableProperty(String segment, Class type) { + return Optional.of(segment).filter(it -> !"description".equals(it)); + } + }; + + CopyOperation operation = CopyOperation.from("/description").to("/description"); + Todo target = new Todo(1L, "Description", false); + + assertThatExceptionOfType(PatchException.class) + .isThrownBy(() -> operation.perform(target, Todo.class, context)) + .withMessageContaining("writable property"); + } } diff --git a/spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/patch/JsonPatchUnitTests.java b/spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/patch/JsonPatchUnitTests.java index b20ce4207..34c02de3c 100755 --- a/spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/patch/JsonPatchUnitTests.java +++ b/spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/patch/JsonPatchUnitTests.java @@ -161,7 +161,7 @@ private Patch readJsonPatch(String jsonPatchFile) throws IOException, JsonParseE ClassPathResource resource = new ClassPathResource(jsonPatchFile, getClass()); JsonNode node = new ObjectMapper().readValue(resource.getInputStream(), JsonNode.class); - Patch patch = new JsonPatchPatchConverter(new ObjectMapper()).convert(node); + Patch patch = new JsonPatchPatchConverter(new ObjectMapper(), TestPropertyPathContext.INSTANCE).convert(node); return patch; } 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 new file mode 100644 index 000000000..1814239c4 --- /dev/null +++ b/spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/patch/JsonPointerMappingTests.java @@ -0,0 +1,69 @@ +/* + * Copyright 2022 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. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.springframework.data.rest.webmvc.json.patch; + +import java.util.Arrays; +import java.util.Collection; + +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.springframework.data.keyvalue.core.mapping.context.KeyValueMappingContext; +import org.springframework.data.mapping.context.PersistentEntities; +import org.springframework.data.rest.webmvc.json.BindContextFactory; +import org.springframework.data.rest.webmvc.json.PersistentEntitiesBindContextFactory; + +import com.fasterxml.jackson.annotation.JsonAutoDetect; +import com.fasterxml.jackson.annotation.JsonAutoDetect.Visibility; +import com.fasterxml.jackson.databind.ObjectMapper; + +/** + * Unit tests for {@link JsonPointerMapping}. + * + * @author Oliver Drotbohm + */ +public class JsonPointerMappingTests { + + JsonPointerMapping verifier; + + @BeforeEach + void setUp() { + + KeyValueMappingContext context = new KeyValueMappingContext<>(); + context.getPersistentEntity(Sample.class); + + PersistentEntities entities = new PersistentEntities(Arrays.asList(context)); + BindContextFactory factory = new PersistentEntitiesBindContextFactory(entities); + + ObjectMapper mapper = new ObjectMapper(); + this.verifier = new JsonPointerMapping(factory.getBindContextFor(mapper)); + } + + @Test + void verifiesSimpleProperty() { + verifier.forRead("/firstname", Sample.class); + } + + @Test + void verifiesPathIntoCollection() { + verifier.forRead("/collection/27/firstname", Sample.class); + } + + @JsonAutoDetect(fieldVisibility = Visibility.ANY) + static class Sample { + String firstname; + Collection collection; + } +} diff --git a/spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/patch/MoveOperationUnitTests.java b/spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/patch/MoveOperationUnitTests.java index d53501e8e..828c8bb71 100755 --- a/spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/patch/MoveOperationUnitTests.java +++ b/spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/patch/MoveOperationUnitTests.java @@ -35,8 +35,8 @@ void moveBooleanPropertyValue() throws Exception { MoveOperation move = MoveOperation.from("/0/complete").to("/1/complete"); assertThatExceptionOfType(PatchException.class) - .isThrownBy(() -> move.perform(todos, Todo.class)) - .withMessage("Path '/0/complete' is not nullable."); + .isThrownBy(() -> move.perform(todos, Todo.class, TestPropertyPathContext.INSTANCE)) + .withMessage("Path '/0/complete' is not nullable"); assertThat(todos.get(1).isComplete()).isFalse(); } @@ -50,7 +50,7 @@ void moveStringPropertyValue() throws Exception { todos.add(new Todo(3L, "C", false)); MoveOperation move = MoveOperation.from("/0/description").to("/1/description"); - move.perform(todos, Todo.class); + move.perform(todos, Todo.class, TestPropertyPathContext.INSTANCE); assertThat(todos.get(1).getDescription()).isEqualTo("A"); } @@ -66,8 +66,8 @@ void moveBooleanPropertyValueIntoStringProperty() throws Exception { MoveOperation move = MoveOperation.from("/0/complete").to("/1/description"); assertThatExceptionOfType(PatchException.class) - .isThrownBy(() -> move.perform(todos, Todo.class)) - .withMessage("Path '/0/complete' is not nullable."); + .isThrownBy(() -> move.perform(todos, Todo.class, TestPropertyPathContext.INSTANCE)) + .withMessage("Path '/0/complete' is not nullable"); assertThat(todos.get(1).getDescription()).isEqualTo("B"); } @@ -89,7 +89,7 @@ void moveListElementToBeginningOfList() throws Exception { todos.add(new Todo(3L, "C", false)); MoveOperation move = MoveOperation.from("/1").to("/0"); - move.perform(todos, Todo.class); + move.perform(todos, Todo.class, TestPropertyPathContext.INSTANCE); assertThat(todos.size()).isEqualTo(3); assertThat(todos.get(0).getId().longValue()).isEqualTo(2L); @@ -106,7 +106,7 @@ void moveListElementToMiddleOfList() throws Exception { todos.add(new Todo(3L, "C", false)); MoveOperation move = MoveOperation.from("/0").to("/2"); - move.perform(todos, Todo.class); + move.perform(todos, Todo.class, TestPropertyPathContext.INSTANCE); assertThat(todos.size()).isEqualTo(3); assertThat(todos.get(2).getId().longValue()).isEqualTo(1L); @@ -123,7 +123,7 @@ void moveListElementToEndOfList_usingIndex() throws Exception { todos.add(new Todo(3L, "C", false)); MoveOperation move = MoveOperation.from("/0").to("/2"); - move.perform(todos, Todo.class); + move.perform(todos, Todo.class, TestPropertyPathContext.INSTANCE); assertThat(todos.size()).isEqualTo(3); assertThat(todos.get(2).getId().longValue()).isEqualTo(1L); @@ -147,7 +147,7 @@ void moveListElementToBeginningOfList_usingDash() throws Exception { expected.add(new Todo(4L, "E", false)); MoveOperation move = MoveOperation.from("/-").to("/1"); - move.perform(todos, Todo.class); + move.perform(todos, Todo.class, TestPropertyPathContext.INSTANCE); assertThat(todos).isEqualTo(expected); } @@ -168,7 +168,7 @@ void moveListElementToEndOfList_usingDash() throws Exception { expected.add(new Todo(2L, "G", false)); MoveOperation move = MoveOperation.from("/1").to("/-"); - move.perform(todos, Todo.class); + move.perform(todos, Todo.class, TestPropertyPathContext.INSTANCE); assertThat(todos).isEqualTo(expected); } diff --git a/spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/patch/PatchOperationUnitTests.java b/spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/patch/PatchOperationUnitTests.java index 420c467fb..ffc44fefa 100644 --- a/spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/patch/PatchOperationUnitTests.java +++ b/spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/patch/PatchOperationUnitTests.java @@ -53,7 +53,7 @@ Stream invalidPathGetsRejected() { Todo todo = new Todo(1L, "A", false); assertThatExceptionOfType(PatchException.class) // - .isThrownBy(() -> it.perform(todo, Todo.class)); + .isThrownBy(() -> it.perform(todo, Todo.class, TestPropertyPathContext.INSTANCE)); }); } } diff --git a/spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/patch/RemoveOperationTests.java b/spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/patch/RemoveOperationTests.java index 473fb5cca..d231a4690 100755 --- a/spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/patch/RemoveOperationTests.java +++ b/spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/patch/RemoveOperationTests.java @@ -32,7 +32,7 @@ void removePropertyFromObject() throws Exception { todos.add(new Todo(2L, "B", false)); todos.add(new Todo(3L, "C", false)); - RemoveOperation.valueAt("/1/description").perform(todos, Todo.class); + RemoveOperation.valueAt("/1/description").perform(todos, Todo.class, TestPropertyPathContext.INSTANCE); assertThat(todos.get(1).getDescription()).isNull(); } @@ -45,7 +45,7 @@ void removeItemFromList() throws Exception { todos.add(new Todo(2L, "B", false)); todos.add(new Todo(3L, "C", false)); - RemoveOperation.valueAt("/1").perform(todos, Todo.class); + RemoveOperation.valueAt("/1").perform(todos, Todo.class, TestPropertyPathContext.INSTANCE); assertThat(todos.size()).isEqualTo(2); assertThat(todos.get(0).getDescription()).isEqualTo("A"); 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 bb30c9411..5976c6d97 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 @@ -38,7 +38,7 @@ void replaceBooleanPropertyValue() throws Exception { todos.add(new Todo(3L, "C", false)); ReplaceOperation replace = ReplaceOperation.valueAt("/1/complete").with(true); - replace.perform(todos, Todo.class); + replace.perform(todos, Todo.class, TestPropertyPathContext.INSTANCE); assertThat(todos.get(1).isComplete()).isTrue(); } @@ -52,7 +52,7 @@ void replaceTextPropertyValue() throws Exception { todos.add(new Todo(3L, "C", false)); ReplaceOperation replace = ReplaceOperation.valueAt("/1/description").with("BBB"); - replace.perform(todos, Todo.class); + replace.perform(todos, Todo.class, TestPropertyPathContext.INSTANCE); assertThat(todos.get(1).getDescription()).isEqualTo("BBB"); } @@ -66,7 +66,7 @@ void replaceTextPropertyValueWithANumber() throws Exception { todos.add(new Todo(3L, "C", false)); ReplaceOperation replace = ReplaceOperation.valueAt("/1/description").with(22); - replace.perform(todos, Todo.class); + replace.perform(todos, Todo.class, TestPropertyPathContext.INSTANCE); assertThat(todos.get(1).getDescription()).isEqualTo("22"); } @@ -79,7 +79,7 @@ void replaceObjectPropertyValue() throws Exception { ObjectMapper mapper = new ObjectMapper(); ReplaceOperation replace = ReplaceOperation.valueAt("/type") .with(new JsonLateObjectEvaluator(mapper, mapper.readTree("{ \"value\" : \"new\" }"))); - replace.perform(todo, Todo.class); + replace.perform(todo, Todo.class, TestPropertyPathContext.INSTANCE); assertThat(todo.getType()).isNotNull(); assertThat(todo.getType().getValue()).isNotNull(); @@ -95,7 +95,7 @@ void replacesMapValueCorrectly() throws Exception { ReplaceOperation.valueAt("/characters/protagonist") // .with(prepareValue("\"Pallo\"")) // - .perform(book, Book.class); + .perform(book, Book.class, TestPropertyPathContext.INSTANCE); assertThat(book.characters.get("protagonist")).isEqualTo("Pallo"); } 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 f0086f0ff..ac7e2d579 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 @@ -17,21 +17,51 @@ import static org.assertj.core.api.Assertions.*; +import lombok.Data; +import lombok.Getter; + import java.util.ArrayList; +import java.util.Arrays; import java.util.List; import java.util.Map; +import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; -import org.springframework.data.rest.webmvc.json.patch.SpelPath.TypedSpelPath; +import org.springframework.data.keyvalue.core.mapping.context.KeyValueMappingContext; +import org.springframework.data.mapping.context.PersistentEntities; +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 com.fasterxml.jackson.annotation.JsonIgnore; +import com.fasterxml.jackson.annotation.JsonProperty; +import com.fasterxml.jackson.databind.ObjectMapper; /** * Unit tests for {@link SpelPath}. * * @author Oliver Gierke + * @author Greg Turnquist */ class SpelPathUnitTests { + BindContext context; + + @BeforeEach + void setUp() { + + KeyValueMappingContext context = new KeyValueMappingContext<>(); + context.getPersistentEntity(MapWrapper.class); + context.getPersistentEntity(Todo.class); + context.getPersistentEntity(Person.class); + + PersistentEntities entities = new PersistentEntities(Arrays.asList(context)); + BindContextFactory factory = new PersistentEntitiesBindContextFactory(entities); + + this.context = factory.getBindContextFor(new ObjectMapper()); + } + @Test void listIndex() { @@ -42,7 +72,7 @@ void listIndex() { todos.add(new Todo(2L, "B", false)); todos.add(new Todo(3L, "C", false)); - Object value = expr.bindTo(Todo.class).getValue(todos); + Object value = expr.bindForRead(Todo.class, context).getValue(todos); assertThat(value).isEqualTo("B"); } @@ -57,7 +87,7 @@ void accessesLastCollectionElementWithDash() { todos.add(new Todo(2L, "B", false)); todos.add(new Todo(3L, "C", false)); - Object value = expr.bindTo(Todo.class).getValue(todos); + Object value = expr.bindForRead(Todo.class, context).getValue(todos); assertThat(value).isEqualTo("C"); } @@ -75,21 +105,22 @@ void cachesSpelPath() { void cachesTypedSpelPath() { UntypedSpelPath source = SpelPath.untyped("/description"); - TypedSpelPath left = source.bindTo(Todo.class); - TypedSpelPath right = source.bindTo(Todo.class); + WritingOperations left = source.bindForWrite(Todo.class, context); + WritingOperations right = source.bindForWrite(Todo.class, context); assertThat(left).isSameAs(right); } @Test // DATAREST-1274 void supportsMultiDigitCollectionIndex() { - assertThat(SpelPath.untyped("/11/description").bindTo(Todo.class).getLeafType()).isEqualTo(String.class); + assertThat(SpelPath.untyped("/11/description").bindForWrite(Todo.class, context).getLeafType()) + .isEqualTo(String.class); } @Test // DATAREST-1338 void handlesStringMapKeysInPathExpressions() { - TypedSpelPath path = SpelPath.untyped("people/Dave/name").bindTo(MapWrapper.class); + WritingOperations path = SpelPath.untyped("people/Dave/name").bindForWrite(MapWrapper.class, context); assertThat(path.getExpressionString()).isEqualTo("people['Dave'].name"); assertThat(path.getLeafType()).isEqualTo(String.class); @@ -98,18 +129,55 @@ void handlesStringMapKeysInPathExpressions() { @Test // DATAREST-1338 void handlesIntegerMapKeysInPathExpressions() { - TypedSpelPath path = SpelPath.untyped("peopleByInt/0/name").bindTo(MapWrapper.class); + WritingOperations path = SpelPath.untyped("peopleByInt/0/name").bindForWrite(MapWrapper.class, context); assertThat(path.getExpressionString()).isEqualTo("peopleByInt[0].name"); assertThat(path.getLeafType()).isEqualTo(String.class); } + @Test + void failsAccessingPropertyIgnoredByJackson() { + + String path = "peopleByInt/0/hiddenProperty"; + + assertThatExceptionOfType(PatchException.class) // + .isThrownBy(() -> SpelPath.untyped(path).bindForWrite(MapWrapper.class, context)) // + .withMessageContaining("hiddenProperty") // + .withMessageContaining(Person.class.getName()) // + .withMessageContaining(path); // + } + + @Test + void failsAccessingGetterIgnoredByJackson() { + + String path = "peopleByInt/0/hiddenGetter"; + + assertThatExceptionOfType(PatchException.class) // + .isThrownBy(() -> SpelPath.untyped(path).bindForWrite(MapWrapper.class, context)) // + .withMessageContaining("hiddenGetter") // + .withMessageContaining(Person.class.getName()) // + .withMessageContaining(path); // + } + + @Test + void mapsRenamedProperty() { + + WritingOperations path = SpelPath.untyped("demaner").bindForWrite(Person.class, context); + + assertThat(path.getExpressionString()).isEqualTo("renamed"); + } + // DATAREST-1338 + @Data static class Person { String name; + @JsonIgnore String hiddenProperty; + @Getter(onMethod = @__(@JsonIgnore)) String hiddenGetter; + @JsonProperty("demaner") String renamed; } + @Data static class MapWrapper { Map people; Map peopleByInt; diff --git a/spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/patch/TestOperationUnitTests.java b/spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/patch/TestOperationUnitTests.java index a61b7addf..a3095eba5 100755 --- a/spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/patch/TestOperationUnitTests.java +++ b/spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/patch/TestOperationUnitTests.java @@ -33,10 +33,10 @@ void testPropertyValueEquals() throws Exception { todos.add(new Todo(3L, "C", false)); TestOperation test = TestOperation.whetherValueAt("/0/complete").hasValue(false); - test.perform(todos, Todo.class); + test.perform(todos, Todo.class, TestPropertyPathContext.INSTANCE); TestOperation test2 = TestOperation.whetherValueAt("/1/complete").hasValue(true); - test2.perform(todos, Todo.class); + test2.perform(todos, Todo.class, TestPropertyPathContext.INSTANCE); } @@ -51,7 +51,7 @@ void testPropertyValueNotEquals() throws Exception { TestOperation test = TestOperation.whetherValueAt("/0/complete").hasValue(true); assertThatExceptionOfType(PatchException.class) // - .isThrownBy(() -> test.perform(todos, Todo.class)); + .isThrownBy(() -> test.perform(todos, Todo.class, TestPropertyPathContext.INSTANCE)); } @Test @@ -63,6 +63,6 @@ void testListElementEquals() throws Exception { todos.add(new Todo(3L, "C", false)); TestOperation test = TestOperation.whetherValueAt("/1").hasValue(new Todo(2L, "B", true)); - test.perform(todos, Todo.class); + test.perform(todos, Todo.class, TestPropertyPathContext.INSTANCE); } } 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 new file mode 100644 index 000000000..9864b159d --- /dev/null +++ b/spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/patch/TestPropertyPathContext.java @@ -0,0 +1,41 @@ +/* + * Copyright 2022 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. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.springframework.data.rest.webmvc.json.patch; + +import java.util.Optional; + +public class TestPropertyPathContext implements BindContext { + + public static final BindContext INSTANCE = new TestPropertyPathContext(); + + /* + * (non-Javadoc) + * @see org.springframework.data.rest.webmvc.json.patch.BindContext#getReadableProperty(java.lang.String, java.lang.Class) + */ + @Override + public Optional getReadableProperty(String segment, Class type) { + return Optional.of(segment); + } + + /* + * (non-Javadoc) + * @see org.springframework.data.rest.webmvc.json.patch.BindContext#getWritableProperty(java.lang.String, java.lang.Class) + */ + @Override + public Optional getWritableProperty(String segment, Class type) { + return Optional.of(segment); + } +}