fix: detect @PropertyTransformer on read-only derived properties [DHIS2-21872] (#24520)

* fix: detect @PropertyTransformer on read-only derived properties [DHIS2-21872]

PropertyPropertyIntrospector.updatePropertyTypes() only read the
@PropertyTransformer annotation inside the isWritable() branch. A
property whose getter is annotated but has no matching setter (e.g.
UserRole.getUsers(), computed from the separately-named `members`
field) is read-only, so its annotation was silently dropped and
Property.hasPropertyTransformer() returned false at runtime even
though the annotation is present in source.

Move the annotation read outside the writability gate, since whether
a property is serialized via a custom transformer is orthogonal to
whether it can be written to.

Found while live-validating DHIS2-21856/PR #24519: with that fix
alone, GET /api/userRoles/{id}?fields=users[*] against an
83,385-member role still took 139.7s / ~583k queries because
hasPropertyTransformer() never saw UserRole.users' annotation. With
this fix applied, the same request drops to 5.76s.

* fix: apply @Property annotation regardless of writability too [DHIS2-21872]

Per review feedback from @jbee: initFromPropertyAnnotation() was also
gated behind isWritable(), but most of @Property's attributes (type
override, required/persisted/owner flags, persistedAs aliasing)
describe the property itself and apply equally to read-only
properties or aliases, not just writable ones.

Snapshot writability before calling initFromPropertyAnnotation(),
since that call can itself flip it via access = READ_ONLY/WRITE_ONLY,
and range/min-max defaults should still be judged against the
pre-annotation writability (matches existing behavior for e.g.
Attribute.getObjectTypes(), which has a real setter overridden to
read-only via the annotation).

Added regression coverage: a read-only property now picks up
persistedAs() (previously silently ignored), and a writable property
whose annotation overrides it to read-only still gets range/min-max
defaults applied (unchanged behavior).

Author: jason-p-pickering

dhis-2/dhis-services/dhis-service-schema/src/main/java/org/hisp/dhis/schema/introspection/PropertyPropertyIntrospector.java

@@ -88,15 +88,29 @@ private static void updatePropertyTypes(Property property) {
       property.setItemPropertyType(getPropertyType(property.getItemKlass()));
     }
 
-    if (property.isWritable()) {
-      initFromPropertyAnnotation(property);
-      initFromPropertyTransformerAnnotation(property);
+    // Snapshot before initFromPropertyAnnotation, which may itself flip writability (via
+    // access = READ_ONLY/WRITE_ONLY) — range/min-max only make sense relative to whether the
+    // property was actually settable prior to that annotation-driven override.
+    boolean writable = property.isWritable();
+
+    // Independent of writability: most of @Property's attributes (type override, required/
+    // persisted/owner flags, persistedAs aliasing) describe the property itself, not whether it
+    // can be written to, so they apply equally to read-only properties and aliases (DHIS2-21872).
+    initFromPropertyAnnotation(property);
+
+    if (writable) {
       initFromPropertyRangeAnnotation(property);
       ensureMinMaxDefaults(property);
     } else {
       property.setMin(null);
       property.setMax(null);
     }
+
+    // Independent of writability: a read-only, computed getter (e.g. UserRole.getUsers(), derived
+    // from a differently-named backing field with its own setter) can still carry
+    // @PropertyTransformer, since it describes the serialized shape, not whether the property can
+    // be written to (DHIS2-21872).
+    initFromPropertyTransformerAnnotation(property);
   }
 
   private static void ensureMinMaxDefaults(Property property) {

dhis-2/dhis-services/dhis-service-schema/src/test/java/org/hisp/dhis/schema/introspection/PropertyPropertyIntrospectorTest.java

@@ -0,0 +1,127 @@
+/*
+ * 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.schema.introspection;
+
+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.assertTrue;
+
+import com.fasterxml.jackson.annotation.JsonProperty;
+import java.util.HashMap;
+import java.util.Map;
+import org.hisp.dhis.schema.Property;
+import org.hisp.dhis.schema.annotation.Property.Access;
+import org.hisp.dhis.user.UserRole;
+import org.junit.jupiter.api.Test;
+
+/**
+ * Uses the real {@link JacksonPropertyIntrospector} pipeline (not a hand-built {@link Property}) to
+ * reproduce the exact reflection-based state that {@link PropertyPropertyIntrospector} sees in
+ * production, since a hand-built {@code Property} with {@code setPropertyTransformer(...)} called
+ * directly would bypass the bug entirely.
+ */
+class PropertyPropertyIntrospectorTest {
+
+  @Test
+  void detectsPropertyTransformerOnReadOnlyDerivedGetter() {
+    Map<String, Property> properties = new HashMap<>();
+    new JacksonPropertyIntrospector().introspect(UserRole.class, properties);
+
+    Property users = properties.get("users");
+
+    // UserRole.getUsers() is computed from the "members" field and has no matching setUsers(),
+    // so it must be read-only.
+    assertFalse(users.isWritable());
+
+    new PropertyPropertyIntrospector().introspect(UserRole.class, properties);
+
+    // ... yet it still carries @PropertyTransformer(UserPropertyTransformer.class) (DHIS2-21872).
+    assertTrue(users.hasPropertyTransformer());
+  }
+
+  @Test
+  void detectsPropertyAnnotationOnReadOnlyProperty() {
+    Map<String, Property> properties = new HashMap<>();
+    new JacksonPropertyIntrospector().introspect(ReadOnlyAliasedFixture.class, properties);
+
+    Property displayAlias = properties.get("displayAlias");
+
+    // No setDisplayAlias(...), so this is read-only.
+    assertFalse(displayAlias.isWritable());
+
+    new PropertyPropertyIntrospector().introspect(ReadOnlyAliasedFixture.class, properties);
+
+    // ... yet persistedAs() still aliases the field name, since it describes the property, not
+    // whether it can be written to (DHIS2-21872).
+    assertEquals("internalName", displayAlias.getFieldName());
+  }
+
+  @Test
+  void rangeDefaultsUnaffectedWhenPropertyAnnotationOverridesWritableProperty() {
+    Map<String, Property> properties = new HashMap<>();
+    new JacksonPropertyIntrospector().introspect(ReadOnlyOverrideFixture.class, properties);
+
+    Property computed = properties.get("computed");
+
+    // Has a real setter, so writable prior to the @Property(access = READ_ONLY) override below.
+    assertTrue(computed.isWritable());
+
+    new PropertyPropertyIntrospector().introspect(ReadOnlyOverrideFixture.class, properties);
+
+    // The annotation flips writability for API purposes ...
+    assertFalse(computed.isWritable());
+    // ... but range/min-max defaults are still applied, since that decision is snapshotted from
+    // before the annotation-driven override, matching pre-existing behavior for properties like
+    // Attribute.getObjectTypes() (real setter + @Property(access = READ_ONLY)).
+    assertNotNull(computed.getMin());
+    assertNotNull(computed.getMax());
+  }
+
+  private static class ReadOnlyAliasedFixture {
+    @JsonProperty
+    @org.hisp.dhis.schema.annotation.Property(persistedAs = "internalName")
+    public String getDisplayAlias() {
+      return "computed";
+    }
+  }
+
+  private static class ReadOnlyOverrideFixture {
+    @JsonProperty
+    @org.hisp.dhis.schema.annotation.Property(access = Access.READ_ONLY)
+    public String getComputed() {
+      return "computed";
+    }
+
+    public void setComputed(String value) {
+      // real setter; @Property(access = READ_ONLY) overrides this for API purposes only
+    }
+  }
+}