fix: avoid loading UserRole members during JSON Patch apply [DHIS2-21852] (#24489)

* 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.

* 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.

* 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.

* test: OU scalar JSON patch must not init inverse collections

* test: User scalar JSON patch skips userGroups, keeps userRoles

* test: UserGroup scalar JSON patch keeps owner users

* test: DataElementGroup name patch preserves owner members

* test: web-api JSON patch side-effect coverage across entities

* 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<Class<?>, 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 <noreply@anthropic.com>

* style: spotless JsonPatchSideEffectControllerTest

* 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 <noreply@anthropic.com>

* 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.

---------

Co-authored-by: Jason Pickering <jason@dhis2.org>
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>

Author: 3 people

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<String> excluded;
+
+  JsonPatchExcludedPropertyFilter(Set<String> 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);
+    }
+  }
+}

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.
+ *
+ * <p>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 {}

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<Class<?>, 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.
    *
+   * <p>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> T apply(JsonPatch patch, T object) throws JsonPatchException {
 
     Class<?> realClass = HibernateProxyUtils.getRealClass(object);
     Schema schema = schemaService.getSchema(realClass);
-    JsonNode node = jsonMapper.valueToTree(object);
+
+    Set<String> patchedPaths =
+        patch.getOperations().stream()
+            .map(op -> op.getPath().getMatchingProperty())
+            .collect(Collectors.toSet());
+    Set<String> 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> T apply(JsonPatch patch, T object) throws JsonPatchException {
         jsonToObject(node, realClass, jsonMapper, ex -> new JsonPatchException(ex.getMessage()));
   }
 
-  private <T> void handleCollectionUpdates(T object, Schema schema, ObjectNode node) {
+  /**
+   * Collect JSON field names that are safe to omit during patch serialization.
+   *
+   * <p>Include when no patch path references {@link Property#getName()} or {@link
+   * Property#getCollectionName()}, and either:
+   *
+   * <ul>
+   *   <li>{@code isCollection() && !isOwner()} - non-owner collections are ignored by metadata
+   *       import UPDATE
+   *   <li>{@code !isPersisted()} - derived getters must not run (e.g. {@code leaf} calling {@code
+   *       children.isEmpty()})
+   * </ul>
+   *
+   * <p>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<String> findExcludableProperties(Schema schema, Set<String> patchedPaths) {
+    Set<String> 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<String> 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<String> 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 <T> void handleCollectionUpdates(
+      T object, Schema schema, ObjectNode node, Set<String> 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;