fix: avoid loading UserRole members during JSON Patch apply [DHIS2-21852]

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;