refactor: rework JsonPatch collection-exclusion filter to match FieldFilterService pattern [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
+ */
+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<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,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)
+public interface JsonPatchFilterMixin {}

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,27 @@ 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(
+                JsonPatchExcludedPropertyFilter.ID, new JsonPatchExcludedPropertyFilter(excluded));
+    return patchMapper.copy().setFilterProvider(filterProvider).valueToTree(object);
   }
 
   private <T> 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 {}
 }

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(
+                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";
+    }
+  }
+}