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..6c3f031b2964 --- /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 + */ +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}. + */ + static final String ID = "json-patch-collection-filter"; + + private final Set excluded; + + 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..4b5b24f64fd5 --- /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) +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 9fd94d8a4249..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 @@ -36,7 +36,13 @@ 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.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; import org.hisp.dhis.common.IdentifiableObject; @@ -59,8 +65,32 @@ */ @Service public class JsonPatchManager { + 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) { @@ -73,6 +103,16 @@ public JsonPatchManager(ObjectMapper jsonMapper, SchemaService schemaService) { * object into a tree like node structure, and this is where the patch will be applied. This means * that any property renaming etc will be followed. * + *

Non-owner collection properties that are not referenced by any patch path are omitted from + * serialization. Non-owner collections are ignored by metadata import UPDATE, so omitting them is + * free; omitting owner collections would clear them on import. Non-persisted derived properties + * (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). 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. * @return New instance of the object with the patch applied. @@ -86,12 +126,19 @@ public T apply(JsonPatch patch, T object) throws JsonPatchException { Class realClass = HibernateProxyUtils.getRealClass(object); Schema schema = schemaService.getSchema(realClass); - JsonNode node = jsonMapper.valueToTree(object); + + Set patchedPaths = + patch.getOperations().stream() + .map(op -> op.getPath().getMatchingProperty()) + .collect(Collectors.toSet()); + Set excluded = findExcludableProperties(schema, patchedPaths); + + JsonNode node = toJsonNode(object, realClass, excluded); // since valueToTree does not properly handle our deeply nested classes, // we need to make another trip to make sure all collections are // correctly made into json nodes. - handleCollectionUpdates(object, schema, (ObjectNode) node); + handleCollectionUpdates(object, schema, (ObjectNode) node, excluded); validatePatchPath(patch, schema); @@ -100,10 +147,81 @@ public T apply(JsonPatch patch, T object) throws JsonPatchException { jsonToObject(node, realClass, jsonMapper, ex -> new JsonPatchException(ex.getMessage())); } - private void handleCollectionUpdates(T object, Schema schema, ObjectNode node) { + /** + * Collect JSON field names that are safe to omit during patch serialization. + * + *

Include when no patch path references {@link Property#getName()} or {@link + * Property#getCollectionName()}, and either: + * + *

+ * + *

Owner collections must always remain serialized (metadata import UPDATE would wipe them if + * omitted). Properties referenced by a patch path keep today's behavior. + */ + private static Set findExcludableProperties(Schema schema, Set patchedPaths) { + Set excluded = new HashSet<>(); + for (Property property : schema.getProperties()) { + if (isReferencedByPatch(property, patchedPaths)) { + continue; + } + if (property.isCollection() && !property.isOwner()) { + // collectionName is the JSON name Jackson serializes (e.g. "users") + excluded.add(property.getCollectionName()); + } else if (!property.isPersisted()) { + String jsonName = + property.isCollection() ? property.getCollectionName() : property.getName(); + if (jsonName != null) { + excluded.add(jsonName); + } + } + } + return excluded; + } + + private static boolean isReferencedByPatch(Property property, Set patchedPaths) { + return patchedPaths.contains(property.getName()) + || (property.getCollectionName() != null + && 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); + } + + ObjectMapper patchMapper = + 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( + T object, Schema schema, ObjectNode node, Set excluded) { for (Property property : schema.getProperties()) { if (property.isCollection()) { + // Skip before safeInvoke so excluded lazy collections stay uninitialized. + if (excluded.contains(property.getCollectionName())) { + continue; + } + Object data = safeInvoke(object, property.getGetterMethod()); Collection collection = (Collection) data; 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 576ca3d1c4fb..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 @@ -30,11 +30,15 @@ package org.hisp.dhis.jsonpatch; import static org.junit.jupiter.api.Assertions.assertEquals; +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; +import java.util.Set; +import org.hibernate.Hibernate; import org.hisp.dhis.common.CodeGenerator; import org.hisp.dhis.common.IdentifiableObjectManager; import org.hisp.dhis.commons.jackson.config.JacksonObjectMapperConfig; @@ -43,9 +47,13 @@ import org.hisp.dhis.constant.Constant; import org.hisp.dhis.dataelement.DataElement; import org.hisp.dhis.dataelement.DataElementGroup; +import org.hisp.dhis.organisationunit.OrganisationUnit; import org.hisp.dhis.test.integration.PostgresIntegrationTestBase; 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; import org.springframework.transaction.annotation.Transactional; @@ -270,4 +278,279 @@ void testRemoveByIdNotExistProperty() throws JsonProcessingException { assertNotNull(patch); assertThrows(JsonPatchException.class, () -> jsonPatchManager.apply(patch, userRole)); } + + @Test + @DisplayName( + "Scalar UserRole patch must not initialize lazy members (slow PATCH /userRoles invariant)") + void testUserRoleScalarPatchDoesNotInitializeMembers() throws Exception { + UserRole userRole = createUserRole("roleMembersLazy", "AUTH_A"); + manager.save(userRole); + + for (int i = 0; i < 4; i++) { + User user = makeUser(String.valueOf((char) ('A' + i))); + manager.save(user); + userRole.addUser(user); + 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\": \"/description\", \"value\": \"updated\"}]", + JsonPatch.class); + + UserRole patched = jsonPatchManager.apply(patch, reloaded); + + assertEquals("updated", patched.getDescription()); + assertFalse( + Hibernate.isInitialized(reloaded.getMembers()), + "scalar patch must not initialize UserRole.members"); + } + + @Test + @DisplayName("Owner collection authorities survive a name-only UserRole patch") + void testUserRoleOwnerAuthoritiesPreservedOnNamePatch() throws Exception { + UserRole userRole = createUserRole("roleOwnerAuths", "AUTH_A", "AUTH_B"); + manager.save(userRole); + + JsonPatch patch = + jsonMapper.readValue( + "[{\"op\": \"replace\", \"path\": \"/name\", \"value\": \"roleOwnerAuthsRenamed\"}]", + JsonPatch.class); + + UserRole patched = jsonPatchManager.apply(patch, userRole); + + assertEquals("roleOwnerAuthsRenamed", patched.getName()); + assertEquals(Set.of("AUTH_A", "AUTH_B"), patched.getAuthorities()); + } + + @Test + @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); + + User user = makeUser("Z"); + manager.save(user); + userRole.addUser(user); + 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, 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 + @DisplayName( + "Scalar OrganisationUnit patch must not initialize inverse children/users collections") + void testOrganisationUnitScalarPatchDoesNotInitializeInverseCollections() throws Exception { + OrganisationUnit parent = createOrganisationUnit('P'); + manager.save(parent); + + OrganisationUnit childA = createOrganisationUnit('A', parent); + OrganisationUnit childB = createOrganisationUnit('B', parent); + manager.save(childA); + manager.save(childB); + manager.update(parent); + + User user = makeUser("Z"); + user.addOrganisationUnit(parent); + manager.save(user); + manager.update(parent); + + clearSession(); + + OrganisationUnit reloaded = manager.get(OrganisationUnit.class, parent.getUid()); + assertNotNull(reloaded); + assertFalse( + Hibernate.isInitialized(reloaded.getChildren()), + "children should be lazy before patch apply"); + assertFalse( + Hibernate.isInitialized(reloaded.getUsers()), "users should be lazy before patch apply"); + + JsonPatch patch = + jsonMapper.readValue( + "[{\"op\": \"replace\", \"path\": \"/name\", \"value\": \"ParentRenamed\"}]", + JsonPatch.class); + + OrganisationUnit patched = jsonPatchManager.apply(patch, reloaded); + + assertEquals("ParentRenamed", patched.getName()); + assertFalse( + Hibernate.isInitialized(reloaded.getChildren()), + "scalar patch must not initialize OrganisationUnit.children"); + assertFalse( + Hibernate.isInitialized(reloaded.getUsers()), + "scalar patch must not initialize OrganisationUnit.users"); + } + + @Test + @DisplayName( + "Scalar User patch must not initialize inverse userGroups; owner userRoles stay on patched object") + void testUserScalarPatchDoesNotInitializeUserGroups() throws Exception { + UserRole role = createUserRole("roleUserPatch", "AUTH_X"); + manager.save(role); + + User user = makeUser("G"); + user.getUserRoles().add(role); + manager.save(user); + + UserGroup group = createUserGroup('G', Set.of(user)); + manager.save(group); + // Keep both sides consistent for Hibernate inverse User.groups + user.getGroups().add(group); + manager.update(user); + + clearSession(); + + User reloaded = manager.get(User.class, user.getUid()); + assertNotNull(reloaded); + assertFalse( + Hibernate.isInitialized(reloaded.getGroups()), + "userGroups/groups should be lazy before patch apply"); + + JsonPatch patch = + jsonMapper.readValue( + "[{\"op\": \"replace\", \"path\": \"/firstName\", \"value\": \"PatchedFirst\"}]", + JsonPatch.class); + + User patched = jsonPatchManager.apply(patch, reloaded); + + assertEquals("PatchedFirst", patched.getFirstName()); + assertFalse( + Hibernate.isInitialized(reloaded.getGroups()), + "scalar patch must not initialize User.groups"); + assertNotNull(patched.getUserRoles()); + assertEquals(1, patched.getUserRoles().size(), "owner userRoles must survive scalar patch"); + } + + @Test + @DisplayName( + "Scalar UserGroup patch keeps owner users and does not initialize inverse managedByGroups") + void testUserGroupScalarPatchKeepsOwnerUsersAndSkipsManagedBy() throws Exception { + User userA = makeUser("A"); + User userB = makeUser("B"); + manager.save(userA); + manager.save(userB); + + UserGroup managed = createUserGroup('M', Set.of()); + manager.save(managed); + + UserGroup group = createUserGroup('U', Set.of(userA, userB)); + manager.save(group); + // managedByGroups on `managed` is inverse of managedGroups on `group` + group.addManagedGroup(managed); + manager.update(group); + manager.update(managed); + + clearSession(); + + UserGroup reloaded = manager.get(UserGroup.class, group.getUid()); + assertNotNull(reloaded); + // Owner collection may or may not be initialized depending on load plan; + // after apply, patched object MUST still carry both users. + UserGroup managedReloaded = manager.get(UserGroup.class, managed.getUid()); + assertNotNull(managedReloaded); + assertFalse( + Hibernate.isInitialized(managedReloaded.getManagedByGroups()), + "managedByGroups should be lazy before patch apply on managed group"); + + JsonPatch patchOnOwnerGroup = + jsonMapper.readValue( + "[{\"op\": \"replace\", \"path\": \"/name\", \"value\": \"UserGroupURenamed\"}]", + JsonPatch.class); + + UserGroup patched = jsonPatchManager.apply(patchOnOwnerGroup, reloaded); + + assertEquals("UserGroupURenamed", patched.getName()); + assertNotNull(patched.getMembers()); + assertEquals(2, patched.getMembers().size(), "owner users must not be omitted on scalar patch"); + + // Apply scalar patch on the managed group: inverse managedByGroups must stay lazy + JsonPatch patchOnManaged = + jsonMapper.readValue( + "[{\"op\": \"replace\", \"path\": \"/name\", \"value\": \"UserGroupMRenamed\"}]", + JsonPatch.class); + + UserGroup patchedManaged = jsonPatchManager.apply(patchOnManaged, managedReloaded); + + assertEquals("UserGroupMRenamed", patchedManaged.getName()); + assertFalse( + Hibernate.isInitialized(managedReloaded.getManagedByGroups()), + "scalar patch must not initialize UserGroup.managedByGroups"); + } + + @Test + @DisplayName("Name-only DataElementGroup patch preserves owner dataElements members") + void testDataElementGroupNamePatchPreservesOwnerMembers() throws Exception { + DataElement deA = createDataElement('A'); + DataElement deB = createDataElement('B'); + manager.save(deA); + manager.save(deB); + + DataElementGroup group = createDataElementGroup('G', deA, deB); + manager.save(group); + + JsonPatch patch = + jsonMapper.readValue( + "[{\"op\": \"replace\", \"path\": \"/name\", \"value\": \"DataElementGroupGRenamed\"}]", + JsonPatch.class); + + DataElementGroup patched = jsonPatchManager.apply(patch, group); + + assertEquals("DataElementGroupGRenamed", patched.getName()); + assertEquals(2, patched.getMembers().size(), "owner members must survive name-only patch"); + } } diff --git a/dhis-2/dhis-test-performance/src/test/java/org/hisp/dhis/test/platform/UserRolesPerformanceTest.java b/dhis-2/dhis-test-performance/src/test/java/org/hisp/dhis/test/platform/UserRolesPerformanceTest.java new file mode 100644 index 000000000000..1f0133d0d45e --- /dev/null +++ b/dhis-2/dhis-test-performance/src/test/java/org/hisp/dhis/test/platform/UserRolesPerformanceTest.java @@ -0,0 +1,296 @@ +/* + * Copyright (c) 2004-2025, 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.test.platform; + +import static io.gatling.javaapi.core.CoreDsl.*; +import static io.gatling.javaapi.http.HttpDsl.*; + +import com.fasterxml.jackson.databind.ObjectMapper; +import io.gatling.javaapi.core.*; +import io.gatling.javaapi.http.*; +import java.io.FileInputStream; +import java.io.IOException; +import java.net.URI; +import java.net.http.HttpClient; +import java.net.http.HttpRequest; +import java.net.http.HttpResponse; +import java.nio.charset.StandardCharsets; +import java.util.Base64; +import java.util.Properties; + +/** + * Performance test for scalar JSON Patch updates on {@code /api/userRoles/{uid}}. + * + *

Motivated by a production incident where {@code PATCH /api/userRoles/{uid}} on a role with + * many members hydrated the entire lazy {@code UserRole.members} collection during {@code + * JsonPatchManager.apply}, making a one-column update cost O(members) SQL and hundreds of MB of + * allocation. A scalar PATCH must be independent of role membership size; this simulation makes + * that regression visible by patching both an empty control role and a large-membership role. + * + *

Scenarios (sequential, single virtual user): + * + *

    + *
  1. PATCH empty role: scalar description patch on a role created in {@code before()} + * with zero members (control; establishes the membership-independent baseline) + *
  2. PATCH large role: the same scalar patch on a pre-existing role with large membership + * ({@code userRoleUid}) + *
  3. GET large role: narrow-fields read of the large role (control; verifies reads stay + * cheap and the role is intact) + *
+ * + *

Available properties (with platform-perf DB defaults), settable via {@code -D} flags or a + * {@code -DconfigFile=.properties}: + * + *

+ * + *

Thresholds calibrated 2026-07-26 from a baseline/candidate A/B run on the platform-perf DB + * (see PR #24489): fixed-code p95/max were empty-role PATCH 37/39ms, large-role (83,334 members) + * PATCH 33/35ms, GET 6/6ms — consistent with the invariant this simulation checks, that PATCH + * latency is independent of membership size. Thresholds are set well above that noise floor but far + * below the pre-fix regression (large-role PATCH p95 was 17,304ms), so a reintroduced O(members) + * hydration fails loudly while ordinary CI-runner jitter does not. Recalibrate the same way if + * thresholds start flapping (see {@link UsersPerformanceTest} for the general workflow). On an + * unfixed server, the large-role PATCH may exceed Gatling's default 60s request timeout; raise it + * with {@code -Dgatling.http.requestTimeout=600000}. + * + * @author Morten Svanæs + */ +public class UserRolesPerformanceTest extends Simulation { + + private static final Properties CONFIG = loadConfig(); + + private static Properties loadConfig() { + String path = System.getProperty("configFile"); + Properties props = new Properties(); + if (path != null) { + try (FileInputStream fis = new FileInputStream(path)) { + props.load(fis); + System.out.println("[UserRolesPerformanceTest] Loaded config from: " + path); + } catch (IOException e) { + System.err.println( + "[UserRolesPerformanceTest] Warning: could not load configFile=" + + path + + ": " + + e.getMessage()); + } + } + return props; + } + + private static String prop(String key, String defaultValue) { + String sys = System.getProperty(key); + if (sys != null) return sys; + String file = CONFIG.getProperty(key); + return file != null ? file : defaultValue; + } + + private static final String BASE_URL = prop("baseUrl", "http://localhost:8080"); + private static final String USERNAME = prop("username", "admin"); + private static final String PASSWORD = prop("password", "district"); + private static final String BASIC_AUTH = + Base64.getEncoder() + .encodeToString((USERNAME + ":" + PASSWORD).getBytes(StandardCharsets.UTF_8)); + private static final String LARGE_ROLE_UID = prop("userRoleUid", "MoRvPzDH7lc"); + private static final int ITERATIONS = Integer.parseInt(prop("iterations", "10")); + + private static final String PATCH_EMPTY_REQUEST = "PATCH UserRole - scalar (empty role)"; + private static final String PATCH_LARGE_REQUEST = "PATCH UserRole - scalar (large role)"; + private static final String GET_LARGE_REQUEST = "GET UserRole - narrow fields (large role)"; + + private record Thresholds(int p95, int max) {} + + // Thresholds (p95, max) in ms, calibrated 2026-07-26 (see class javadoc). Both PATCH scenarios + // share one threshold: the point of this simulation is that PATCH latency must not depend on + // membership size, so empty-role and large-role PATCH are held to the same bar. + private static final Thresholds PATCH_THRESH = new Thresholds(100, 150); + private static final Thresholds GET_THRESH = new Thresholds(30, 50); + + private static final String PATCH_BODY_TEMPLATE = + """ + [{"op":"replace","path":"/description","value":"perf-patched %s"}]\ + """; + + /** UID of the empty control role created in {@link #before()}. */ + private static volatile String emptyRoleUid; + + /** + * Creates the zero-member control role and verifies the large role exists. Fails fast if either + * precondition cannot be met, so a misconfigured {@code userRoleUid} does not produce a + * misleadingly green run. + */ + @Override + public void before() { + HttpClient client = HttpClient.newHttpClient(); + ObjectMapper mapper = new ObjectMapper(); + try { + String name = "PerfTest empty role " + System.currentTimeMillis(); + HttpRequest create = + HttpRequest.newBuilder() + .uri(URI.create(BASE_URL + "/api/userRoles")) + .header("Content-Type", "application/json") + .header("Authorization", "Basic " + BASIC_AUTH) + .header("Accept", "application/json") + .POST( + HttpRequest.BodyPublishers.ofString( + "{\"name\":\"%s\",\"description\":\"perf control role\"}".formatted(name))) + .build(); + HttpResponse createResponse = + client.send(create, HttpResponse.BodyHandlers.ofString()); + emptyRoleUid = mapper.readTree(createResponse.body()).path("response").path("uid").asText(); + if (emptyRoleUid.isEmpty()) { + throw new IllegalStateException( + "Could not create control role (HTTP " + + createResponse.statusCode() + + "): " + + createResponse.body()); + } + + HttpRequest verify = + HttpRequest.newBuilder() + .uri(URI.create(BASE_URL + "/api/userRoles/" + LARGE_ROLE_UID + "?fields=id,name")) + .header("Authorization", "Basic " + BASIC_AUTH) + .header("Accept", "application/json") + .GET() + .build(); + HttpResponse verifyResponse = + client.send(verify, HttpResponse.BodyHandlers.ofString()); + if (verifyResponse.statusCode() != 200) { + throw new IllegalStateException( + "Large role %s not found (HTTP %d), set -DuserRoleUid to a role with many members" + .formatted(LARGE_ROLE_UID, verifyResponse.statusCode())); + } + System.out.println( + "[UserRolesPerformanceTest] control role %s, large role %s, %d iterations" + .formatted(emptyRoleUid, LARGE_ROLE_UID, ITERATIONS)); + } catch (IOException | InterruptedException e) { + Thread.currentThread().interrupt(); + throw new IllegalStateException("Setup failed: " + e.getMessage(), e); + } + } + + /** Deletes the control role created in {@link #before()}. */ + @Override + public void after() { + if (emptyRoleUid == null || emptyRoleUid.isEmpty()) { + return; + } + try { + HttpClient.newHttpClient() + .send( + HttpRequest.newBuilder() + .uri(URI.create(BASE_URL + "/api/userRoles/" + emptyRoleUid)) + .header("Authorization", "Basic " + BASIC_AUTH) + .DELETE() + .build(), + HttpResponse.BodyHandlers.discarding()); + } catch (IOException | InterruptedException e) { + Thread.currentThread().interrupt(); + System.err.println("Cleanup of control role failed: " + e.getMessage()); + } + } + + public UserRolesPerformanceTest() { + // Same session strategy as UsersPerformanceTest: authenticate once per virtual user via a + // separately-named request so the one-time bcrypt cost stays out of the measured requests. + HttpProtocolBuilder httpProtocol = + http.baseUrl(BASE_URL).acceptHeader("application/json").disableCaching(); + + ChainBuilder authenticate = + exec(flushCookieJar()) + .exec( + http("Authenticate (session login)") + .get("/api/me") + .header("Authorization", "Basic " + BASIC_AUTH) + .check(status().is(200))); + + ScenarioBuilder patchEmptyScenario = + scenario(PATCH_EMPTY_REQUEST) + .exec(authenticate) + .repeat(ITERATIONS) + .on( + exec( + http(PATCH_EMPTY_REQUEST) + .patch(session -> "/api/userRoles/" + emptyRoleUid) + .header("Content-Type", "application/json-patch+json") + .body( + StringBody(session -> PATCH_BODY_TEMPLATE.formatted(System.nanoTime()))) + .check(status().is(200)))); + + ScenarioBuilder patchLargeScenario = + scenario(PATCH_LARGE_REQUEST) + .exec(authenticate) + .repeat(ITERATIONS) + .on( + exec( + http(PATCH_LARGE_REQUEST) + .patch("/api/userRoles/" + LARGE_ROLE_UID) + .header("Content-Type", "application/json-patch+json") + .body( + StringBody(session -> PATCH_BODY_TEMPLATE.formatted(System.nanoTime()))) + .check(status().is(200)))); + + ScenarioBuilder getLargeScenario = + scenario(GET_LARGE_REQUEST) + .exec(authenticate) + .repeat(ITERATIONS) + .on( + exec( + http(GET_LARGE_REQUEST) + .get("/api/userRoles/" + LARGE_ROLE_UID) + .queryParam("fields", "id,name,description,authorities") + .check(status().is(200)))); + + ClosedInjectionStep singleUser = rampConcurrentUsers(0).to(1).during(1); + + setUp( + patchEmptyScenario + .injectClosed(singleUser) + .andThen(patchLargeScenario.injectClosed(singleUser)) + .andThen(getLargeScenario.injectClosed(singleUser))) + .protocols(httpProtocol) + .assertions( + details(PATCH_EMPTY_REQUEST).responseTime().percentile(95).lt(PATCH_THRESH.p95()), + details(PATCH_EMPTY_REQUEST).responseTime().max().lt(PATCH_THRESH.max()), + details(PATCH_EMPTY_REQUEST).successfulRequests().percent().is(100D), + details(PATCH_LARGE_REQUEST).responseTime().percentile(95).lt(PATCH_THRESH.p95()), + details(PATCH_LARGE_REQUEST).responseTime().max().lt(PATCH_THRESH.max()), + details(PATCH_LARGE_REQUEST).successfulRequests().percent().is(100D), + details(GET_LARGE_REQUEST).responseTime().percentile(95).lt(GET_THRESH.p95()), + details(GET_LARGE_REQUEST).responseTime().max().lt(GET_THRESH.max()), + details(GET_LARGE_REQUEST).successfulRequests().percent().is(100D)); + } +} diff --git a/dhis-2/dhis-test-web-api/src/test/java/org/hisp/dhis/webapi/controller/JsonPatchSideEffectControllerTest.java b/dhis-2/dhis-test-web-api/src/test/java/org/hisp/dhis/webapi/controller/JsonPatchSideEffectControllerTest.java new file mode 100644 index 000000000000..41464f74cf7e --- /dev/null +++ b/dhis-2/dhis-test-web-api/src/test/java/org/hisp/dhis/webapi/controller/JsonPatchSideEffectControllerTest.java @@ -0,0 +1,151 @@ +/* + * Copyright (c) 2004-2026, 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.webapi.controller; + +import static org.hisp.dhis.http.HttpAssertions.assertStatus; +import static org.junit.jupiter.api.Assertions.assertEquals; + +import org.hisp.dhis.http.HttpStatus; +import org.hisp.dhis.jsontree.JsonObject; +import org.hisp.dhis.organisationunit.OrganisationUnit; +import org.hisp.dhis.test.webapi.H2ControllerIntegrationTestBase; +import org.hisp.dhis.user.User; +import org.hisp.dhis.user.UserRole; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; +import org.springframework.transaction.annotation.Transactional; + +/** + * HTTP-level regressions for system-wide JsonPatchManager non-owner collection skipping + * (DHIS2-21852 / PR #24489). Complements {@link org.hisp.dhis.jsonpatch.JsonPatchManagerTest}. + * + * @author Morten (netroms) + */ +@Transactional +class JsonPatchSideEffectControllerTest extends H2ControllerIntegrationTestBase { + + @Test + @DisplayName("PATCH organisationUnit name keeps children") + void testPatchOrganisationUnitNameKeepsChildren() { + OrganisationUnit parent = createOrganisationUnit('P'); + manager.save(parent); + OrganisationUnit child = createOrganisationUnit('C', parent); + manager.save(child); + manager.update(parent); + + assertStatus( + HttpStatus.OK, + PATCH( + "/organisationUnits/" + parent.getUid(), + "[{'op':'replace','path':'/name','value':'ParentPatched'}]")); + + JsonObject body = + GET("/organisationUnits/{id}?fields=name,children[id]", parent.getUid()) + .content(HttpStatus.OK); + + assertEquals("ParentPatched", body.getString("name").string()); + assertEquals(1, body.getArray("children").size()); + assertEquals(child.getUid(), body.getArray("children").getObject(0).getString("id").string()); + } + + @Test + @DisplayName("PATCH userGroup name keeps owner users") + void testPatchUserGroupNameKeepsUsers() { + User user = makeUser("G"); + userService.addUser(user); + + String groupId = + assertStatus( + HttpStatus.CREATED, + POST("/userGroups/", "{'name':'GroupSide','users':[{'id':'" + user.getUid() + "'}]}")); + + assertStatus( + HttpStatus.OK, + PATCH( + "/userGroups/" + groupId, + "[{'op':'replace','path':'/name','value':'GroupSideRenamed'}]")); + + JsonObject body = GET("/userGroups/{id}?fields=name,users[id]", groupId).content(HttpStatus.OK); + + assertEquals("GroupSideRenamed", body.getString("name").string()); + assertEquals(1, body.getArray("users").size()); + assertEquals(user.getUid(), body.getArray("users").getObject(0).getString("id").string()); + } + + @Test + @DisplayName("PATCH user firstName keeps userGroups") + void testPatchUserFirstNameKeepsUserGroups() { + UserRole role = createUserRole('F'); + manager.save(role); + + User user = makeUser("F"); + user.setEmail("first@example.org"); + user.getUserRoles().add(role); + userService.addUser(user); + + String groupId = + assertStatus( + HttpStatus.CREATED, + POST( + "/userGroups/", + "{'name':'UserFirstGroup','users':[{'id':'" + user.getUid() + "'}]}")); + + assertStatus( + HttpStatus.OK, + PATCH( + "/users/" + user.getUid() + "?importReportMode=ERRORS", + "[{'op':'replace','path':'/firstName','value':'FirstPatched'}]")); + + JsonObject body = + GET("/users/{id}?fields=firstName,userGroups[id]", user.getUid()).content(HttpStatus.OK); + + assertEquals("FirstPatched", body.getString("firstName").string()); + assertEquals(1, body.getArray("userGroups").size()); + assertEquals(groupId, body.getArray("userGroups").getObject(0).getString("id").string()); + } + + @Test + @DisplayName("PATCH userRole replace /users does not change membership") + void testPatchUserRoleUsersPathDoesNotChangeMembership() { + UserRole role = createUserRole('S'); + User user = makeUser("R"); + userService.addUser(user); + role.addUser(user); + manager.save(role); + + // Non-owner path: may return OK with ERRORS_NOT_OWNER notes; membership must not drop. + PATCH("/userRoles/" + role.getUid(), "[{'op':'replace','path':'/users','value':[]}]"); + + JsonObject body = GET("/userRoles/{id}?fields=users[id]", role.getUid()).content(HttpStatus.OK); + + assertEquals(1, body.getArray("users").size()); + assertEquals(user.getUid(), body.getArray("users").getObject(0).getString("id").string()); + } +} diff --git a/dhis-2/dhis-test-web-api/src/test/java/org/hisp/dhis/webapi/controller/UserControllerTest.java b/dhis-2/dhis-test-web-api/src/test/java/org/hisp/dhis/webapi/controller/UserControllerTest.java index fcb6780f7a13..1cf2a0e76364 100644 --- a/dhis-2/dhis-test-web-api/src/test/java/org/hisp/dhis/webapi/controller/UserControllerTest.java +++ b/dhis-2/dhis-test-web-api/src/test/java/org/hisp/dhis/webapi/controller/UserControllerTest.java @@ -1450,6 +1450,31 @@ void testGetUserRoleUsersAreTransformed() { assertEquals(user.getUid(), userInRole.getString("id").string()); } + @Test + @DisplayName("PATCH /userRoles/{uid} scalar description keeps assigned users") + void testPatchUserRoleDescriptionKeepsUsers() { + UserRole role = createUserRole('P'); + User user = makeUser("R"); + userService.addUser(user); + role.addUser(user); + manager.save(role); + + assertStatus( + HttpStatus.OK, + PATCH( + "/userRoles/" + role.getUid(), + "[{'op':'replace','path':'/description','value':'patched'}]")); + + JsonObject patchedRole = + GET("/userRoles/{id}?fields=id,description,users[id]", role.getUid()) + .content(HttpStatus.OK); + + assertEquals("patched", patchedRole.getString("description").string()); + assertEquals(1, patchedRole.getArray("users").size()); + assertEquals( + user.getUid(), patchedRole.getArray("users").getObject(0).getString("id").string()); + } + @Test @DisplayName( "GET /users?filter=organisationUnits.id:in:[uid] returns only users in that org unit")