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>

Author: jason-p-pickering

dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/jsonpatch/JsonPatchExcludedPropertyFilter.java

@@ -0,0 +1,76 @@
+/*
+ * 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 {
+  private final Set<String> excluded;
+
+  public 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,54 @@
+/*
+ * 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(JsonPatchFilterMixin.ID)
+public interface JsonPatchFilterMixin {
+  /** Single source of truth for the filter id -- see {@link JsonPatchManager}'s usage. */
+  String ID = "json-patch-collection-filter";
+}

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<Class<?>, 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,26 @@ private static boolean isReferencedByPatch(Property property, Set<String> 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<String> 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(JsonPatchFilterMixin.ID, new JsonPatchExcludedPropertyFilter(excluded));
+    return patchMapper.copy().setFilterProvider(filterProvider).valueToTree(object);
   }
 
   private <T> void handleCollectionUpdates(
@@ -248,8 +274,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 {}
 }

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<String> 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(
+                "json-patch-collection-filter",
+                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(
+                "json-patch-collection-filter", 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";
+    }
+  }
+}