diff --git a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/jsonpatch/JsonPatchExcludedPropertyFilter.java b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/jsonpatch/JsonPatchExcludedPropertyFilter.java new file mode 100644 index 000000000000..4a7a8b417b52 --- /dev/null +++ b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/jsonpatch/JsonPatchExcludedPropertyFilter.java @@ -0,0 +1,88 @@ +/* + * Copyright (c) 2004-2022, University of Oslo + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions are met: + * + * 1. Redistributions of source code must retain the above copyright notice, this + * list of conditions and the following disclaimer. + * + * 2. Redistributions in binary form must reproduce the above copyright notice, + * this list of conditions and the following disclaimer in the documentation + * and/or other materials provided with the distribution. + * + * 3. Neither the name of the copyright holder nor the names of its contributors + * may be used to endorse or promote products derived from this software without + * specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED + * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE + * DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR + * ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES + * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; + * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON + * ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS + * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + */ +package org.hisp.dhis.jsonpatch; + +import com.fasterxml.jackson.core.JsonGenerator; +import com.fasterxml.jackson.databind.SerializerProvider; +import com.fasterxml.jackson.databind.ser.BeanPropertyWriter; +import com.fasterxml.jackson.databind.ser.PropertyWriter; +import com.fasterxml.jackson.databind.ser.impl.SimpleBeanPropertyFilter; +import java.util.Set; + +/** + * PropertyFilter that omits properties named in {@code excluded} without invoking their getter, via + * {@link PropertyWriter#serializeAsOmittedField}. Mirrors {@code + * org.hisp.dhis.fieldfiltering.FieldFilterSimpleBeanPropertyFilter}'s shape, simplified to a flat + * excluded-name set (no path-context tracking needed here). + * + * @author Jason Pickering + */ +public class JsonPatchExcludedPropertyFilter extends SimpleBeanPropertyFilter { + + /** + * Single source of truth for the filter id -- referenced by both {@link JsonPatchFilterMixin}'s + * {@code @JsonFilter} annotation and {@link JsonPatchManager}'s {@code addFilter} call. Declared + * here, on a concrete class, rather than on the mixin interface itself: an interface holding only + * a constant is exactly the "constant interface" anti-pattern (SonarQube java:S1214) -- + * implementing classes would inherit the constant into their own namespace for no reason. {@code + * JsonPatchFilterMixin} stays a truly empty marker interface, matching its precedent, {@code + * org.hisp.dhis.fieldfiltering.FieldFilterMixin}. + */ + public static final String ID = "json-patch-collection-filter"; + + private final Set excluded; + + public JsonPatchExcludedPropertyFilter(Set excluded) { + this.excluded = excluded; + } + + @Override + protected boolean include(final BeanPropertyWriter writer) { + return true; + } + + @Override + protected boolean include(final PropertyWriter writer) { + return true; + } + + @Override + public void serializeAsField( + Object pojo, JsonGenerator jgen, SerializerProvider provider, PropertyWriter writer) + throws Exception { + if (excluded.contains(writer.getName())) { + if (!jgen.canOmitFields()) { + writer.serializeAsOmittedField(pojo, jgen, provider); + } + } else { + writer.serializeAsField(pojo, jgen, provider); + } + } +} diff --git a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/jsonpatch/JsonPatchFilterMixin.java b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/jsonpatch/JsonPatchFilterMixin.java new file mode 100644 index 000000000000..9448979cf3bc --- /dev/null +++ b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/jsonpatch/JsonPatchFilterMixin.java @@ -0,0 +1,51 @@ +/* + * Copyright (c) 2004-2022, University of Oslo + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions are met: + * + * 1. Redistributions of source code must retain the above copyright notice, this + * list of conditions and the following disclaimer. + * + * 2. Redistributions in binary form must reproduce the above copyright notice, + * this list of conditions and the following disclaimer in the documentation + * and/or other materials provided with the distribution. + * + * 3. Neither the name of the copyright holder nor the names of its contributors + * may be used to endorse or promote products derived from this software without + * specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED + * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE + * DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR + * ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES + * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; + * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON + * ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS + * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + */ +package org.hisp.dhis.jsonpatch; + +import com.fasterxml.jackson.annotation.JsonFilter; + +/** + * Bound per patched entity type ({@code realClass}), not {@code Object.class} -- see {@link + * JsonPatchManager}'s {@code patchMapperCache} field for why a global binding is wrong: {@code + * JsonPatchExcludedPropertyFilter} matches by property name only, so binding this mixin to {@code + * Object.class} would apply the exclusion filter to every nested object in the graph too (e.g. + * {@code org.hisp.dhis.user.sharing.Sharing.users}, which collides by name with {@code + * UserRole.users}) and silently strip unrelated data. {@code JsonPatchManager} builds one + * mixin-bound {@code ObjectMapper} copy per distinct {@code realClass}, lazily, and caches it. + * + *

Solves the same underlying problem as {@code org.hisp.dhis.fieldfiltering.FieldFilterMixin} + * (skip a getter during Jackson serialization based on per-call criteria, without invoking it) for + * the {@code ?fields=} GET path -- but that mixin's filter is path-aware, which is what makes its + * own {@code Object.class}-wide binding safe; this one is not, so it must stay scoped per-class. + * + * @author Jason Pickering + */ +@JsonFilter(JsonPatchExcludedPropertyFilter.ID) +public interface JsonPatchFilterMixin {} diff --git a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/jsonpatch/JsonPatchManager.java b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/jsonpatch/JsonPatchManager.java index c92f868c941a..7c12691d5ad6 100644 --- a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/jsonpatch/JsonPatchManager.java +++ b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/jsonpatch/JsonPatchManager.java @@ -32,16 +32,16 @@ import static org.hisp.dhis.schema.DefaultSchemaService.safeInvoke; import static org.hisp.dhis.util.JsonUtils.jsonToObject; -import com.fasterxml.jackson.annotation.JsonFilter; import com.fasterxml.jackson.databind.JsonNode; import com.fasterxml.jackson.databind.ObjectMapper; import com.fasterxml.jackson.databind.node.ArrayNode; import com.fasterxml.jackson.databind.node.ObjectNode; -import com.fasterxml.jackson.databind.ser.impl.SimpleBeanPropertyFilter; import com.fasterxml.jackson.databind.ser.impl.SimpleFilterProvider; import java.util.Collection; import java.util.HashSet; +import java.util.Map; import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; import java.util.stream.Collectors; import org.hisp.dhis.common.BaseIdentifiableObject; import org.hisp.dhis.common.EmbeddedObject; @@ -65,10 +65,32 @@ */ @Service public class JsonPatchManager { - private static final String JSON_PATCH_FILTER_ID = "jsonPatchFilter"; private final ObjectMapper jsonMapper; + /** + * Cache of per-entity-type {@code ObjectMapper} copies with {@link JsonPatchFilterMixin} bound to + * that type only -- NOT {@code Object.class}. Binding the mixin globally would apply the + * exclusion filter to every nested object in the serialized graph too: {@code Sharing} (present + * on every {@code IdentifiableObject} via {@code BaseIdentifiableObject.getSharing()}) has its + * own {@code @JsonProperty} field literally named {@code users}, so a global binding would + * silently strip {@code sharing.users}/{@code sharing.userGroups} whenever a same-named top-level + * collection (e.g. {@code UserRole.users}) is excluded -- confirmed as a real bug during review, + * not a hypothetical. Scoping to {@code realClass} makes that specific cross-type collision + * impossible: only {@code realClass} carries {@code @JsonFilter}, so a differently-typed nested + * object (e.g. {@code Sharing}) is never affected by another type's exclusions. (A + * self-referential {@code realClass}, e.g. a nested {@code OrganisationUnit} under another {@code + * OrganisationUnit}, does re-carry the filter -- but the excluded set only ever names {@code + * realClass}'s own non-owner collections, so re-applying it at any depth is safe by the same + * owner/non-owner invariant that makes dropping them at the root safe.) Built lazily, once per + * distinct {@code realClass} ever patched, then reused -- mirrors {@code + * org.hisp.dhis.fieldfiltering.FieldFilterSimpleBeanPropertyFilter}'s own {@code + * ALWAYS_EXPAND_CACHE} pattern for the same "compute once per Class" idiom. Per-call code (see + * {@link #toJsonNode}) only attaches a fresh {@link SimpleFilterProvider}; it does not rebuild + * the cached entry on every patch. + */ + private final Map, ObjectMapper> patchMapperCache = new ConcurrentHashMap<>(); + private final SchemaService schemaService; public JsonPatchManager(ObjectMapper jsonMapper, SchemaService schemaService) { @@ -87,7 +109,9 @@ public JsonPatchManager(ObjectMapper jsonMapper, SchemaService schemaService) { * (for example {@code OrganisationUnit.leaf}) are also omitted when unreferenced, because their * getters can force-initialize inverse lazy collections. Skipping avoids initializing lazy * Hibernate collections such as {@code UserRole.members} during scalar PATCH /userRoles (slow - * PATCH /userRoles). + * PATCH /userRoles). A patch path that explicitly targets an otherwise-excludable property (e.g. + * {@code /users}) removes it from the excluded set, so that property still falls back to full + * (slower, but correct) hydration -- it is never silently dropped. * * @param patch JsonPatch object with the operations it should apply. * @param object Jackson Object to apply the patch to. @@ -165,24 +189,27 @@ private static boolean isReferencedByPatch(Property property, Set patche && patchedPaths.contains(property.getCollectionName())); } + /** + * Builds the JSON tree for {@code object}, skipping getters for properties in {@code excluded}. + * Looks up (or lazily builds) {@code realClass}'s cached mapper from {@link #patchMapperCache}; + * only attaches a fresh, per-call {@link SimpleFilterProvider} -- the expensive + * mixin-binding/{@code copy()} happens at most once per distinct {@code realClass}, not on every + * patch. + */ private JsonNode toJsonNode(Object object, Class realClass, Set excluded) { if (excluded.isEmpty()) { return jsonMapper.valueToTree(object); } - // Per-call mapper copy with a mixin bound to realClass only, so nested - // objects serialize unchanged and excluded getters (lazy collections) are - // never invoked. ObjectMapper patchMapper = - jsonMapper - .copy() - .addMixIn(realClass, JsonPatchFilterMixin.class) - .setFilterProvider( - new SimpleFilterProvider() - .addFilter( - JSON_PATCH_FILTER_ID, - SimpleBeanPropertyFilter.serializeAllExcept(excluded))); - return patchMapper.valueToTree(object); + patchMapperCache.computeIfAbsent( + realClass, cls -> jsonMapper.copy().addMixIn(cls, JsonPatchFilterMixin.class)); + + SimpleFilterProvider filterProvider = + new SimpleFilterProvider() + .addFilter( + JsonPatchExcludedPropertyFilter.ID, new JsonPatchExcludedPropertyFilter(excluded)); + return patchMapper.copy().setFilterProvider(filterProvider).valueToTree(object); } private void handleCollectionUpdates( @@ -248,8 +275,4 @@ private void validatePatchPath(JsonPatch patch, Schema schema) throws JsonPatchE } } } - - /** Mixin that attaches the per-call json-patch property filter to the root entity class only. */ - @JsonFilter(JSON_PATCH_FILTER_ID) - private static final class JsonPatchFilterMixin {} } diff --git a/dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/jsonpatch/JsonPatchExcludedPropertyFilterTest.java b/dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/jsonpatch/JsonPatchExcludedPropertyFilterTest.java new file mode 100644 index 000000000000..de1fe5775cc2 --- /dev/null +++ b/dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/jsonpatch/JsonPatchExcludedPropertyFilterTest.java @@ -0,0 +1,100 @@ +/* + * Copyright (c) 2004-2022, University of Oslo + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions are met: + * + * 1. Redistributions of source code must retain the above copyright notice, this + * list of conditions and the following disclaimer. + * + * 2. Redistributions in binary form must reproduce the above copyright notice, + * this list of conditions and the following disclaimer in the documentation + * and/or other materials provided with the distribution. + * + * 3. Neither the name of the copyright holder nor the names of its contributors + * may be used to endorse or promote products derived from this software without + * specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED + * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE + * DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR + * ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES + * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; + * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON + * ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS + * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + */ +package org.hisp.dhis.jsonpatch; + +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import com.fasterxml.jackson.databind.JsonNode; +import com.fasterxml.jackson.databind.ObjectMapper; +import com.fasterxml.jackson.databind.module.SimpleModule; +import com.fasterxml.jackson.databind.ser.impl.SimpleFilterProvider; +import java.util.List; +import java.util.Set; +import org.junit.jupiter.api.Test; + +/** + * @author Jason Pickering + */ +class JsonPatchExcludedPropertyFilterTest { + + /** Getter throws if invoked, so the test fails loudly if the filter ever calls it. */ + static class SampleBean { + public String getId() { + return "abc"; + } + + public List getUsers() { + throw new UnsupportedOperationException("users getter must not be invoked when excluded"); + } + } + + @Test + void excludedPropertyGetterIsNeverInvokedAndOmittedFromOutput() { + ObjectMapper mapper = new ObjectMapper(); + SimpleModule module = new SimpleModule(); + module.setMixInAnnotation(Object.class, JsonPatchFilterMixin.class); + mapper.registerModule(module); + + SimpleFilterProvider filterProvider = + new SimpleFilterProvider() + .addFilter( + JsonPatchExcludedPropertyFilter.ID, + new JsonPatchExcludedPropertyFilter(Set.of("users"))); + + JsonNode node = mapper.copy().setFilterProvider(filterProvider).valueToTree(new SampleBean()); + + assertTrue(node.has("id"), "non-excluded property must still be serialized"); + assertFalse(node.has("users"), "excluded property must not appear in output"); + } + + @Test + void nonExcludedPropertiesAreUnaffected() { + ObjectMapper mapper = new ObjectMapper(); + SimpleModule module = new SimpleModule(); + module.setMixInAnnotation(Object.class, JsonPatchFilterMixin.class); + mapper.registerModule(module); + + SimpleFilterProvider filterProvider = + new SimpleFilterProvider() + .addFilter( + JsonPatchExcludedPropertyFilter.ID, new JsonPatchExcludedPropertyFilter(Set.of())); + + JsonNode node = mapper.copy().setFilterProvider(filterProvider).valueToTree(new NoUsersBean()); + + assertTrue(node.has("id")); + } + + static class NoUsersBean { + public String getId() { + return "abc"; + } + } +} diff --git a/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/jsonpatch/JsonPatchManagerTest.java b/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/jsonpatch/JsonPatchManagerTest.java index 959dd16c30e0..f9060059e75a 100644 --- a/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/jsonpatch/JsonPatchManagerTest.java +++ b/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/jsonpatch/JsonPatchManagerTest.java @@ -33,6 +33,7 @@ import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; import com.fasterxml.jackson.core.JsonProcessingException; import com.fasterxml.jackson.databind.ObjectMapper; @@ -51,6 +52,7 @@ import org.hisp.dhis.user.User; import org.hisp.dhis.user.UserGroup; import org.hisp.dhis.user.UserRole; +import org.hisp.dhis.user.sharing.UserAccess; import org.junit.jupiter.api.DisplayName; import org.junit.jupiter.api.Test; import org.springframework.beans.factory.annotation.Autowired; @@ -331,8 +333,10 @@ void testUserRoleOwnerAuthoritiesPreservedOnNamePatch() throws Exception { } @Test - @DisplayName("Patch referencing non-owner /users path does not throw") - void testUserRoleUsersPathPatchDoesNotThrow() throws Exception { + @DisplayName( + "Patch referencing non-owner /users path falls back to full hydration, not silently" + + " excluded") + void testUserRoleUsersPathPatchFallsBackToFullHydration() throws Exception { UserRole userRole = createUserRole("roleUsersPath", "AUTH_A"); manager.save(userRole); @@ -342,12 +346,50 @@ void testUserRoleUsersPathPatchDoesNotThrow() throws Exception { manager.update(user); manager.update(userRole); + clearSession(); + + UserRole reloaded = manager.get(UserRole.class, userRole.getUid()); + assertNotNull(reloaded); + assertFalse( + Hibernate.isInitialized(reloaded.getMembers()), + "members should be lazy before patch apply"); + JsonPatch patch = jsonMapper.readValue( "[{\"op\": \"replace\", \"path\": \"/users\", \"value\": []}]", JsonPatch.class); - UserRole patched = jsonPatchManager.apply(patch, userRole); + UserRole patched = jsonPatchManager.apply(patch, reloaded); + assertNotNull(patched); + assertTrue( + Hibernate.isInitialized(reloaded.getMembers()), + "explicit /users patch must fall back to full hydration, not be silently excluded"); + } + + @Test + @DisplayName( + "sharing.users survives a UserRole scalar patch (regression: exclusion filter must not" + + " collide with same-named properties on unrelated nested objects)") + void testUserRoleSharingUsersSurviveScalarPatch() throws Exception { + UserRole userRole = createUserRole("roleSharingCheck", "AUTH_A"); + User userA = makeUser("Q"); + userRole.getSharing().addUserAccess(new UserAccess(userA, "rw------")); + + assertEquals( + 1, userRole.getSharing().getUsers().size(), "precondition: sharing.users populated"); + + JsonPatch patch = + jsonMapper.readValue( + "[{\"op\": \"replace\", \"path\": \"/name\", \"value\": \"roleSharingCheckRenamed\"}]", + JsonPatch.class); + + UserRole patched = jsonPatchManager.apply(patch, userRole); + + assertEquals("roleSharingCheckRenamed", patched.getName()); + assertEquals( + 1, + patched.getSharing().getUsers().size(), + "sharing.users must survive an unrelated scalar patch"); } @Test @@ -376,8 +418,7 @@ void testOrganisationUnitScalarPatchDoesNotInitializeInverseCollections() throws Hibernate.isInitialized(reloaded.getChildren()), "children should be lazy before patch apply"); assertFalse( - Hibernate.isInitialized(reloaded.getUsers()), - "users should be lazy before patch apply"); + Hibernate.isInitialized(reloaded.getUsers()), "users should be lazy before patch apply"); JsonPatch patch = jsonMapper.readValue(