From d8d3389fca1f6dd655cad2717b3ed2dfd2cd1ece Mon Sep 17 00:00:00 2001 From: Morten Svanaes Date: Sat, 18 Jul 2026 02:50:38 +0800 Subject: [PATCH 01/12] fix: avoid loading UserRole members during JSON Patch apply JsonPatchManager converted the managed entity to a tree via valueToTree and then re-invoked every collection getter in handleCollectionUpdates. For UserRole, getUsers() initializes all lazy members, causing O(members) SQL and hundreds of MB of allocation on scalar PATCHes. Skip non-owner collection properties that no patch path references, in both serialization passes. Non-owner collections are ignored by metadata import UPDATE, so omitting them is semantically free; owner collections always stay in the tree (omitting them would clear them on import). Non-owner collections referenced by patch paths keep today's behavior. --- .../hisp/dhis/jsonpatch/JsonPatchManager.java | 80 ++++++++++++++++++- .../dhis/jsonpatch/JsonPatchManagerTest.java | 77 ++++++++++++++++++ .../webapi/controller/UserControllerTest.java | 25 ++++++ 3 files changed, 179 insertions(+), 3 deletions(-) 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..0b684dea0361 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,11 +32,17 @@ 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.Set; +import java.util.stream.Collectors; import org.hisp.dhis.common.BaseIdentifiableObject; import org.hisp.dhis.common.EmbeddedObject; import org.hisp.dhis.common.IdentifiableObject; @@ -59,6 +65,8 @@ */ @Service public class JsonPatchManager { + private static final String JSON_PATCH_FILTER_ID = "jsonPatchFilter"; + private final ObjectMapper jsonMapper; private final SchemaService schemaService; @@ -73,6 +81,12 @@ 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. Skipping avoids initializing lazy + * Hibernate collections such as {@code UserRole.members} during scalar PATCH /userRoles (slow + * PATCH /userRoles). + * * @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 +100,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 = findExcludableNonOwnerCollections(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 +121,59 @@ 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 of non-owner collection properties that no patch op references. + * + *

Skip when {@code isCollection() && !isOwner()} and the patch paths match neither {@link + * Property#getName()} nor {@link Property#getCollectionName()}. Owner collections must always + * remain serialized (metadata import UPDATE would wipe them if omitted). Non-owner collections + * referenced by a patch path keep today's behavior. + */ + private static Set findExcludableNonOwnerCollections( + Schema schema, Set patchedPaths) { + Set excluded = new HashSet<>(); + for (Property property : schema.getProperties()) { + if (property.isCollection() + && !property.isOwner() + && !patchedPaths.contains(property.getName()) + && !patchedPaths.contains(property.getCollectionName())) { + // collectionName is the JSON name Jackson serializes (e.g. "users") + excluded.add(property.getCollectionName()); + } + } + return excluded; + } + + 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); + } + + 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; @@ -157,4 +227,8 @@ 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-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..3d31025df64e 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,14 @@ 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 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; @@ -46,6 +49,7 @@ import org.hisp.dhis.test.integration.PostgresIntegrationTestBase; 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.beans.factory.annotation.Autowired; import org.springframework.transaction.annotation.Transactional; @@ -270,4 +274,77 @@ 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 does not throw") + void testUserRoleUsersPathPatchDoesNotThrow() 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); + + JsonPatch patch = + jsonMapper.readValue( + "[{\"op\": \"replace\", \"path\": \"/users\", \"value\": []}]", JsonPatch.class); + + UserRole patched = jsonPatchManager.apply(patch, userRole); + assertNotNull(patched); + } } 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") From a69eba901b7f6a4c569b28ef4eb8e8939e68d749 Mon Sep 17 00:00:00 2001 From: Morten Svanaes Date: Sat, 18 Jul 2026 02:50:39 +0800 Subject: [PATCH 02/12] test: add UserRoles PATCH performance simulation Gatling simulation patching a scalar field on an empty control role and on a large-membership role (platform-perf DB), making the O(members) PATCH regression visible. No calibrated thresholds yet; asserts 100% success only. --- .../platform/UserRolesPerformanceTest.java | 277 ++++++++++++++++++ 1 file changed, 277 insertions(+) create mode 100644 dhis-2/dhis-test-performance/src/test/java/org/hisp/dhis/test/platform/UserRolesPerformanceTest.java 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..0babe3cd8543 --- /dev/null +++ b/dhis-2/dhis-test-performance/src/test/java/org/hisp/dhis/test/platform/UserRolesPerformanceTest.java @@ -0,0 +1,277 @@ +/* + * 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}: + * + *

+ * + *

No p95/max threshold assertions yet, only 100% success. Thresholds should be calibrated from + * nightly baselines once this simulation has history (see {@link UsersPerformanceTest} for the + * calibration 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 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).successfulRequests().percent().is(100D), + details(PATCH_LARGE_REQUEST).successfulRequests().percent().is(100D), + details(GET_LARGE_REQUEST).successfulRequests().percent().is(100D)); + } +} From d8ddfb6dbf1e8c0e95b17fa2d1a461089cbbe203 Mon Sep 17 00:00:00 2001 From: Morten Svanaes Date: Thu, 23 Jul 2026 18:59:30 +0800 Subject: [PATCH 03/12] fix: skip non-persisted derived props during JSON Patch serialize OrganisationUnit.leaf calls children.isEmpty(), which initializes the inverse children collection even when JsonPatchManager already omits non-owner collections. Exclude unreferenced non-persisted properties from patch serialization so derived getters cannot force lazy loads. --- .../hisp/dhis/jsonpatch/JsonPatchManager.java | 47 ++++++++++++++----- 1 file changed, 34 insertions(+), 13 deletions(-) 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 0b684dea0361..c92f868c941a 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 @@ -83,7 +83,9 @@ public JsonPatchManager(ObjectMapper jsonMapper, SchemaService schemaService) { * *

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. Skipping avoids initializing lazy + * 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). * @@ -105,7 +107,7 @@ public T apply(JsonPatch patch, T object) throws JsonPatchException { patch.getOperations().stream() .map(op -> op.getPath().getMatchingProperty()) .collect(Collectors.toSet()); - Set excluded = findExcludableNonOwnerCollections(schema, patchedPaths); + Set excluded = findExcludableProperties(schema, patchedPaths); JsonNode node = toJsonNode(object, realClass, excluded); @@ -122,28 +124,47 @@ public T apply(JsonPatch patch, T object) throws JsonPatchException { } /** - * Collect JSON field names of non-owner collection properties that no patch op references. + * Collect JSON field names that are safe to omit during patch serialization. * - *

Skip when {@code isCollection() && !isOwner()} and the patch paths match neither {@link - * Property#getName()} nor {@link Property#getCollectionName()}. Owner collections must always - * remain serialized (metadata import UPDATE would wipe them if omitted). Non-owner collections - * referenced by a patch path keep today's behavior. + *

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 findExcludableNonOwnerCollections( - Schema schema, Set patchedPaths) { + private static Set findExcludableProperties(Schema schema, Set patchedPaths) { Set excluded = new HashSet<>(); for (Property property : schema.getProperties()) { - if (property.isCollection() - && !property.isOwner() - && !patchedPaths.contains(property.getName()) - && !patchedPaths.contains(property.getCollectionName())) { + 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())); + } + private JsonNode toJsonNode(Object object, Class realClass, Set excluded) { if (excluded.isEmpty()) { return jsonMapper.valueToTree(object); From 6ae219f8677be6de7000661a7cfccaf5ef6a7a4f Mon Sep 17 00:00:00 2001 From: Morten Svanaes Date: Thu, 23 Jul 2026 18:59:30 +0800 Subject: [PATCH 04/12] test: OU scalar JSON patch must not init inverse collections --- .../dhis/jsonpatch/JsonPatchManagerTest.java | 46 +++++++++++++++++++ 1 file changed, 46 insertions(+) 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 3d31025df64e..128549d2e580 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 @@ -46,6 +46,7 @@ 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.UserRole; @@ -347,4 +348,49 @@ void testUserRoleUsersPathPatchDoesNotThrow() throws Exception { UserRole patched = jsonPatchManager.apply(patch, userRole); assertNotNull(patched); } + + @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"); + } } From 7d573b70aabc1f60bef8663e3e41c9793806d47f Mon Sep 17 00:00:00 2001 From: Morten Svanaes Date: Thu, 23 Jul 2026 19:05:38 +0800 Subject: [PATCH 05/12] test: User scalar JSON patch skips userGroups, keeps userRoles --- .../dhis/jsonpatch/JsonPatchManagerTest.java | 41 +++++++++++++++++++ 1 file changed, 41 insertions(+) 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 128549d2e580..24c8f8ff8ea2 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 @@ -49,6 +49,7 @@ 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.junit.jupiter.api.DisplayName; import org.junit.jupiter.api.Test; @@ -393,4 +394,44 @@ void testOrganisationUnitScalarPatchDoesNotInitializeInverseCollections() throws 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"); + } } From a043177fca412e7a1c14a8a9a3371e020322d400 Mon Sep 17 00:00:00 2001 From: Morten Svanaes Date: Thu, 23 Jul 2026 19:09:46 +0800 Subject: [PATCH 06/12] test: UserGroup scalar JSON patch keeps owner users --- .../dhis/jsonpatch/JsonPatchManagerTest.java | 56 +++++++++++++++++++ 1 file changed, 56 insertions(+) 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 24c8f8ff8ea2..a744c84e0de8 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 @@ -434,4 +434,60 @@ void testUserScalarPatchDoesNotInitializeUserGroups() throws Exception { 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"); + } } From 80c87e3be5527aa76b53501b69710df84c260871 Mon Sep 17 00:00:00 2001 From: Morten Svanaes Date: Thu, 23 Jul 2026 19:13:03 +0800 Subject: [PATCH 07/12] test: DataElementGroup name patch preserves owner members --- .../dhis/jsonpatch/JsonPatchManagerTest.java | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) 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 a744c84e0de8..959dd16c30e0 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 @@ -490,4 +490,26 @@ void testUserGroupScalarPatchKeepsOwnerUsersAndSkipsManagedBy() throws Exception 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"); + } } From 1f4144d283a0a3c1ec7f37756c60e8818b7c1a9b Mon Sep 17 00:00:00 2001 From: Morten Svanaes Date: Thu, 23 Jul 2026 19:16:20 +0800 Subject: [PATCH 08/12] test: web-api JSON patch side-effect coverage across entities --- .../JsonPatchSideEffectControllerTest.java | 157 ++++++++++++++++++ 1 file changed, 157 insertions(+) create mode 100644 dhis-2/dhis-test-web-api/src/test/java/org/hisp/dhis/webapi/controller/JsonPatchSideEffectControllerTest.java 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..cabda07e8564 --- /dev/null +++ b/dhis-2/dhis-test-web-api/src/test/java/org/hisp/dhis/webapi/controller/JsonPatchSideEffectControllerTest.java @@ -0,0 +1,157 @@ +/* + * 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()); + } +} From 86c2c676fd0b709930cecbe68aeaf5bf1617968c Mon Sep 17 00:00:00 2001 From: Jason Pickering Date: Thu, 23 Jul 2026 15:36:23 +0200 Subject: [PATCH 09/12] refactor: rework JsonPatch collection-exclusion filter to match FieldFilterService pattern [DHIS2-21852] Addresses review feedback on #24489 (Jan Bernitt): the per-call `.addMixIn(realClass, ...)` + inline mixin + filter-provider wiring in JsonPatchManager.apply() was rebuilt from scratch on every call and didn't follow any existing convention. Reworks it to mirror the codebase's established pattern for the same underlying problem -- skipping a getter during Jackson serialization based on per-call criteria, without invoking it -- already used for the `?fields=` GET path (org.hisp.dhis.fieldfiltering.FieldFilterService/FieldFilterMixin). - New `JsonPatchFilterMixin` (`@JsonFilter`) and `JsonPatchExcludedPropertyFilter` (`SimpleBeanPropertyFilter`), shaped like `FieldFilterMixin`/`FieldFilterSimpleBeanPropertyFilter`. - `JsonPatchManager` caches one mixin-bound `ObjectMapper` copy per entity type (`ConcurrentHashMap, ObjectMapper>`, built lazily via `computeIfAbsent`) instead of rebuilding the mixin binding on every `apply()` call. `findExcludableNonOwnerCollections` (the exclusion rule itself) is unchanged -- it was already schema-driven and generic. - The mixin is scoped per-`realClass`, not bound to `Object.class` globally: `FieldFilterMixin`'s own filter is path-aware, so a global binding is safe for it, but `JsonPatchExcludedPropertyFilter` matches by property name only. A global binding was tried first and found, in review, to silently strip `Sharing.users`/`Sharing.userGroups` (present on every `IdentifiableObject`) whenever a same-named top-level collection was excluded, e.g. on a scalar UserRole PATCH. Scoping per-class makes that cross-type collision impossible. Regression test: `testUserRoleSharingUsersSurviveScalarPatch`. - Answers Jan's second question (should an explicit patch to an excluded property throw?): no -- `findExcludableNonOwnerCollections` already un-excludes a property referenced by a patch path, so it falls back to full (slower, correct) hydration rather than being silently dropped. Replaced the previous `testUserRoleUsersPathPatchDoesNotThrow` (asserted only `assertNotNull`, would pass either way) with `testUserRoleUsersPathPatchFallsBackToFullHydration`, which asserts `Hibernate.isInitialized(...)` actually flips to `true`, mutation-tested against the fallback logic. No functional change to PATCH behavior beyond fixing the Object.class regression introduced and caught within this same review cycle (never released). All existing JsonPatchManagerTest coverage (14 tests) plus the new JsonPatchExcludedPropertyFilterTest (2 tests) pass; UserControllerTest (52 tests, dhis-test-web-api) unaffected. Co-Authored-By: Claude Sonnet 5 --- .../JsonPatchExcludedPropertyFilter.java | 88 +++++++++++++++ .../dhis/jsonpatch/JsonPatchFilterMixin.java | 51 +++++++++ .../hisp/dhis/jsonpatch/JsonPatchManager.java | 63 +++++++---- .../JsonPatchExcludedPropertyFilterTest.java | 100 ++++++++++++++++++ .../dhis/jsonpatch/JsonPatchManagerTest.java | 51 ++++++++- 5 files changed, 328 insertions(+), 25 deletions(-) create mode 100644 dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/jsonpatch/JsonPatchExcludedPropertyFilter.java create mode 100644 dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/jsonpatch/JsonPatchFilterMixin.java create mode 100644 dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/jsonpatch/JsonPatchExcludedPropertyFilterTest.java 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( From 04d4ee554cfcf28b617e5e070089d733f6e21958 Mon Sep 17 00:00:00 2001 From: Morten Svanaes Date: Fri, 24 Jul 2026 00:21:49 +0800 Subject: [PATCH 10/12] style: spotless JsonPatchSideEffectControllerTest --- .../JsonPatchSideEffectControllerTest.java | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) 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 index cabda07e8564..41464f74cf7e 100644 --- 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 @@ -12,7 +12,7 @@ * 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 + * 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. * @@ -84,9 +84,7 @@ void testPatchUserGroupNameKeepsUsers() { String groupId = assertStatus( HttpStatus.CREATED, - POST( - "/userGroups/", - "{'name':'GroupSide','users':[{'id':'" + user.getUid() + "'}]}")); + POST("/userGroups/", "{'name':'GroupSide','users':[{'id':'" + user.getUid() + "'}]}")); assertStatus( HttpStatus.OK, @@ -94,8 +92,7 @@ void testPatchUserGroupNameKeepsUsers() { "/userGroups/" + groupId, "[{'op':'replace','path':'/name','value':'GroupSideRenamed'}]")); - JsonObject body = - GET("/userGroups/{id}?fields=name,users[id]", groupId).content(HttpStatus.OK); + 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()); @@ -144,12 +141,9 @@ void testPatchUserRoleUsersPathDoesNotChangeMembership() { 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':[]}]"); + PATCH("/userRoles/" + role.getUid(), "[{'op':'replace','path':'/users','value':[]}]"); - JsonObject body = - GET("/userRoles/{id}?fields=users[id]", role.getUid()).content(HttpStatus.OK); + 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()); From fbe6983ba0d3a532f04c3c04b42ffafaef87085b Mon Sep 17 00:00:00 2001 From: Jason Pickering Date: Sun, 26 Jul 2026 09:48:34 +0200 Subject: [PATCH 11/12] test: calibrate UserRolesPerformanceTest p95/max thresholds Calibrated from the baseline/candidate A/B run on the platform-perf DB used to recalibrate PR #24489 after the rebase onto the JsonPatchFilterMixin refactor. PATCH scenarios share one threshold since the invariant under test is that PATCH latency must not depend on role membership size. Co-Authored-By: Claude Sonnet 5 --- .../platform/UserRolesPerformanceTest.java | 27 ++++++++++++++++--- 1 file changed, 23 insertions(+), 4 deletions(-) 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 index 0babe3cd8543..1f0133d0d45e 100644 --- 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 @@ -77,10 +77,15 @@ *

  • {@code iterations} (default: {@code 10}) * * - *

    No p95/max threshold assertions yet, only 100% success. Thresholds should be calibrated from - * nightly baselines once this simulation has history (see {@link UsersPerformanceTest} for the - * calibration 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}. + *

    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 */ @@ -126,6 +131,14 @@ private static String prop(String key, String defaultValue) { 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"}]\ @@ -270,8 +283,14 @@ public UserRolesPerformanceTest() { .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)); } } From 3b9d8ecd790d6dcbbb941f855cfd889bbcfa84ee Mon Sep 17 00:00:00 2001 From: Morten Svanaes Date: Tue, 28 Jul 2026 02:04:04 +0800 Subject: [PATCH 12/12] refactor: make JsonPatch filter and mixin package-private [DHIS2-21852] Per review: both are internal to JsonPatchManager and not reused outside the package, so drop public visibility. --- .../dhis/jsonpatch/JsonPatchExcludedPropertyFilter.java | 6 +++--- .../java/org/hisp/dhis/jsonpatch/JsonPatchFilterMixin.java | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) 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 index 4a7a8b417b52..6c3f031b2964 100644 --- 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 @@ -44,7 +44,7 @@ * * @author Jason Pickering */ -public class JsonPatchExcludedPropertyFilter extends SimpleBeanPropertyFilter { +class JsonPatchExcludedPropertyFilter extends SimpleBeanPropertyFilter { /** * Single source of truth for the filter id -- referenced by both {@link JsonPatchFilterMixin}'s @@ -55,11 +55,11 @@ public class JsonPatchExcludedPropertyFilter extends SimpleBeanPropertyFilter { * 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"; + static final String ID = "json-patch-collection-filter"; private final Set excluded; - public JsonPatchExcludedPropertyFilter(Set excluded) { + JsonPatchExcludedPropertyFilter(Set excluded) { this.excluded = excluded; } 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 index 9448979cf3bc..4b5b24f64fd5 100644 --- 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 @@ -48,4 +48,4 @@ * @author Jason Pickering */ @JsonFilter(JsonPatchExcludedPropertyFilter.ID) -public interface JsonPatchFilterMixin {} +interface JsonPatchFilterMixin {}