Skip to content

Commit 894a102

Browse files
jason-p-pickeringclaude
jason-p-pickering
and
claude
authored
fix: prune PropertyTransformer-controlled subtrees at field-expansion time [DHIS2-21856] (#24519)
* fix: prune PropertyTransformer-controlled subtrees at field-expansion time [DHIS2-21856] A property annotated @PropertyTransformer (e.g. UserPropertyTransformer, used on OrganisationUnit.users, UserRole.users, UserGroup.members, and ~12 other sites) always serializes a fixed shape regardless of what's requested underneath it -- verified empirically against a live server: fields=users, fields=users[href] and fields=users[*] all produce byte-identical responses, one in 6.4s and the other in 138.5s / 583,334 JDBC queries. Three independent subsystems were each rediscovering (or failing to discover) this fact on their own: - FieldFilterService.applyAccess/applySharingDisplayNames, fixed narrowly by #24514 (DHIS2-21867) via a check inside FieldPathHelper.visitFieldPath. - DefaultLinkService.generateLink, via AbstractFullReadOnlyController's hasHref/fieldsContains -- a raw substring check on the unparsed fields string (fields.contains("*")) that can't tell a top-level wildcard from one nested four levels deep inside users[*], causing LinkService to reflectively invoke every IdentifiableObject getter on the schema (including UserRole.getUsers()) before field-filtering ever runs. - Jackson's actual serialization, which legitimately needs to enumerate the collection once -- not addressed here. Root cause, generalized: FieldPathHelper.apply() generates descendant field paths under a transformer-controlled property in the first place (confirmed via FieldFilterParser.parse("users[*]") -> "users", "users.:all", which then expands into one leaf per property on the target schema, recursively). Every consumer of the expanded list has to independently re-derive "am I under a PropertyTransformer" to defensively skip it. Fix: - FieldPathHelper.apply() now prunes any path beneath a PropertyTransformer-annotated property as a final step, before any consumer sees the list. The property's own bare path always survives (Jackson only needs it present to include the field at all; its transformer's DTO has no @jsonfilter, so descendant paths were already a no-op for serialization -- confirmed by tracing UserPropertyTransformer's UserDto). - AbstractFullReadOnlyController.hasHref now asks the real, now-pruned field-path expansion instead of substring-matching -- correct for every phrasing (href, id,href, *, :all, :simple, users[href], users[*]) with no special-casing, and fieldsContains is deleted. This makes #24514's visitFieldPath check redundant in practice (the paths it guards against no longer exist to visit) but harmless to keep as defense-in-depth. Supersedes #24512 (DHIS2-21860), which the team judged too specific/hard to maintain (per-entity raw-SQL projection plus a beforeLinkGeneration hook) -- this fixes the shared root for every @PropertyTransformer site with one change instead. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * test: update skipSharingFieldsExcludeCorrectFieldsTest for PropertyTransformer pruning [DHIS2-21856] createdBy/lastUpdatedBy use UserPropertyTransformer, so FieldPathHelper.apply()'s new subtree pruning correctly drops their now-dead createdBy.id/lastUpdatedBy.id descendant paths generated by the :owner preset expansion, while keeping the properties' own bare paths. Expected count drops from 58 to 56 accordingly; added explicit assertions on the surviving/pruned paths so this doesn't regress to a bare count check again. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * test: collapse PropertyTransformer pruning tests into one parameterized test [DHIS2-21856] Addresses SonarCloud MAJOR finding on the PR (three near-identical @test methods differing only in the input fields expression). Mutation-tested: reverting the pruning logic makes 2 of the 3 parameterized cases fail for the right reason (the bare "members" case is unaffected either way, since it has no descendants to prune). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
1 parent 2c241e8 commit 894a102

5 files changed

Lines changed: 280 additions & 11 deletions

File tree

‎dhis-2/dhis-services/dhis-service-field-filtering/src/main/java/org/hisp/dhis/fieldfiltering/FieldPathHelper.java‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -91,9 +91,36 @@ public List<FieldPath> apply(List<FieldPath> paths, Class<?> rootKlass) {
9191
List<FieldPath> exclusions = paths.stream().filter(FieldPath::isExclude).toList();
9292
applyExclusions(exclusions, inclusions);
9393

94+
pruneTransformerControlledSubtrees(inclusions, rootKlass);
95+
9496
return List.copyOf(inclusions.values());
9597
}
9698

99+
/**
100+
* A property with a {@code @PropertyTransformer} (e.g. {@code UserPropertyTransformer}) always
101+
* serializes a fixed shape independent of what's requested underneath it -- verified empirically
102+
* (DHIS2-21856): {@code fields=users}, {@code fields=users[href]} and {@code fields=users[*]} all
103+
* produce byte-identical responses. So any path beneath such a property is dead on arrival and is
104+
* dropped here, before any consumer (ACL/sharing computation, href generation, ...) has a reason
105+
* to walk into it -- rather than leaving every consumer to independently rediscover and skip the
106+
* same pointless subtree.
107+
*/
108+
private void pruneTransformerControlledSubtrees(
109+
Map<PropertyPath, FieldPath> inclusions, Class<?> rootKlass) {
110+
inclusions.keySet().removeIf(path -> isBeneathPropertyTransformer(path, rootKlass));
111+
}
112+
113+
private boolean isBeneathPropertyTransformer(PropertyPath path, Class<?> rootKlass) {
114+
for (PropertyPath ancestor = path.parent(); ancestor != null; ancestor = ancestor.parent()) {
115+
Schema schema = getSchemaByPath(ancestor.parent(), rootKlass);
116+
Property property = schema == null ? null : schema.getProperty(ancestor.segment().toString());
117+
if (property != null && property.hasPropertyTransformer()) {
118+
return true;
119+
}
120+
}
121+
return false;
122+
}
123+
97124
/**
98125
* Applies (recursively) default expansion on the remaining field paths, for example the path
99126
* 'dataElements.dataElementGroups' would get expanded to 'dataElements.dataElementGroups.id' so

‎dhis-2/dhis-services/dhis-service-field-filtering/src/test/java/org/hisp/dhis/fieldfiltering/FieldPathHelperPropertyTransformerPruningTest.java‎

Lines changed: 159 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,159 @@
1+
/*
2+
* Copyright (c) 2004-2026, University of Oslo
3+
* All rights reserved.
4+
*
5+
* Redistribution and use in source and binary forms, with or without
6+
* modification, are permitted provided that the following conditions are met:
7+
*
8+
* 1. Redistributions of source code must retain the above copyright notice, this
9+
* list of conditions and the following disclaimer.
10+
*
11+
* 2. Redistributions in binary form must reproduce the above copyright notice,
12+
* this list of conditions and the following disclaimer in the documentation
13+
* and/or other materials provided with the distribution.
14+
*
15+
* 3. Neither the name of the copyright holder nor the names of its contributors
16+
* may be used to endorse or promote products derived from this software without
17+
* specific prior written permission.
18+
*
19+
* THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND
20+
* ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
21+
* WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
22+
* DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR
23+
* ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
24+
* (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
25+
* LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON
26+
* ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
27+
* (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
28+
* SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
29+
*/
30+
package org.hisp.dhis.fieldfiltering;
31+
32+
import static org.junit.jupiter.api.Assertions.assertEquals;
33+
34+
import java.util.List;
35+
import java.util.Set;
36+
import org.hisp.dhis.common.BaseIdentifiableObject;
37+
import org.hisp.dhis.query.planner.PropertyPath;
38+
import org.hisp.dhis.schema.Property;
39+
import org.hisp.dhis.schema.Schema;
40+
import org.hisp.dhis.schema.SchemaDescriptor;
41+
import org.hisp.dhis.schema.SchemaService;
42+
import org.hisp.dhis.schema.transformer.UserPropertyTransformer;
43+
import org.junit.jupiter.api.BeforeEach;
44+
import org.junit.jupiter.params.ParameterizedTest;
45+
import org.junit.jupiter.params.provider.ValueSource;
46+
47+
/**
48+
* Covers DHIS2-21856: a property annotated with {@code @PropertyTransformer} always serializes a
49+
* fixed shape regardless of what's requested underneath it (verified empirically: `fields=users`,
50+
* `fields=users[href]` and `fields=users[*]` all produce byte-identical responses against a live
51+
* server), so {@link FieldPathHelper#apply} should prune every descendant path of such a property
52+
* rather than expanding presets/defaults into it -- the earlier, narrower fix (DHIS2-21867) only
53+
* stopped {@code visitFieldPath} from *walking* those descendants; this stops them from being
54+
* generated at all, which is what starves every consumer (not just the ACL/sharing visitor) of a
55+
* reason to touch the real collection.
56+
*/
57+
class FieldPathHelperPropertyTransformerPruningTest {
58+
59+
/** Stands in for a Hibernate-backed member type with its own nested collection. */
60+
public static class Member extends BaseIdentifiableObject {
61+
public String getExtra() {
62+
return "extra";
63+
}
64+
}
65+
66+
public static class Root extends BaseIdentifiableObject {
67+
public Set<Member> getMembers() {
68+
return Set.of();
69+
}
70+
}
71+
72+
private FieldPathHelper fieldPathHelper;
73+
74+
@BeforeEach
75+
void setUp() throws NoSuchMethodException {
76+
Property idProperty = new Property(String.class, Member.class.getMethod("getUid"), null);
77+
idProperty.setName("id");
78+
idProperty.setSimple(true);
79+
idProperty.setPersisted(true);
80+
81+
Property extraProperty = new Property(String.class, Member.class.getMethod("getExtra"), null);
82+
extraProperty.setName("extra");
83+
extraProperty.setSimple(true);
84+
85+
Schema memberSchema = new Schema(Member.class, "member", "members");
86+
memberSchema.addProperty(idProperty);
87+
memberSchema.addProperty(extraProperty);
88+
89+
Property membersProperty = new Property(Set.class, Root.class.getMethod("getMembers"), null);
90+
membersProperty.setName("members");
91+
membersProperty.setCollection(true);
92+
membersProperty.setItemKlass(Member.class);
93+
membersProperty.setPropertyTransformer(UserPropertyTransformer.class);
94+
95+
Schema rootSchema = new Schema(Root.class, "root", "roots");
96+
rootSchema.addProperty(membersProperty);
97+
98+
SchemaService schemaService = new StubSchemaService(rootSchema, memberSchema);
99+
fieldPathHelper = new FieldPathHelper(schemaService);
100+
}
101+
102+
@ParameterizedTest
103+
@ValueSource(strings = {"members[*]", "members[id,extra]", "members"})
104+
void fieldsUnderPropertyTransformerCollapseToTheBarePath(String fields) {
105+
List<FieldPath> result = fieldPathHelper.apply(FieldFilterParser.parse(fields), Root.class);
106+
107+
assertEquals(List.of("members"), result.stream().map(FieldPath::toString).toList());
108+
}
109+
110+
private record StubSchemaService(Schema rootSchema, Schema memberSchema)
111+
implements SchemaService {
112+
@Override
113+
public void register(SchemaDescriptor descriptor) {
114+
throw new UnsupportedOperationException();
115+
}
116+
117+
@Override
118+
public Schema getSchema(Class<?> klass) {
119+
if (klass == Root.class) return rootSchema;
120+
if (klass == Member.class) return memberSchema;
121+
return null;
122+
}
123+
124+
@Override
125+
public Schema getSchemaBySingularName(String name) {
126+
throw new UnsupportedOperationException();
127+
}
128+
129+
@Override
130+
public Schema getSchemaByPluralName(String name) {
131+
throw new UnsupportedOperationException();
132+
}
133+
134+
@Override
135+
public List<Schema> getSchemas() {
136+
throw new UnsupportedOperationException();
137+
}
138+
139+
@Override
140+
public List<Schema> getSortedSchemas() {
141+
throw new UnsupportedOperationException();
142+
}
143+
144+
@Override
145+
public List<Schema> getMetadataSchemas() {
146+
throw new UnsupportedOperationException();
147+
}
148+
149+
@Override
150+
public Set<String> collectAuthorities() {
151+
throw new UnsupportedOperationException();
152+
}
153+
154+
@Override
155+
public PropertyPath getPropertyPath(Class<?> klass, String path) {
156+
throw new UnsupportedOperationException();
157+
}
158+
}
159+
}

‎dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/fieldfiltering/FieldPathHelperTest.java‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -120,12 +120,22 @@ void skipSharingFieldsExcludeCorrectFieldsTest() {
120120

121121
// then only matching exclusions should have been applied
122122
// and fields starting with 'user' should still be present
123-
assertEquals(58, result.size()); // all user properties
123+
assertEquals(56, result.size()); // all user properties
124124
assertTrue(
125125
result.stream()
126126
.map(FieldPath::getPropertyName)
127127
.toList()
128128
.containsAll(List.of("username", "userRoles")));
129+
130+
// and @PropertyTransformer-controlled properties (createdBy/lastUpdatedBy use
131+
// UserPropertyTransformer) keep their own bare path but not pruned descendant paths
132+
// such as createdBy.id/lastUpdatedBy.id, since the transformer always serializes a
133+
// fixed shape regardless of what's requested underneath it (DHIS2-21856)
134+
List<String> paths = result.stream().map(fp -> fp.getPath().toString()).toList();
135+
assertTrue(paths.contains("createdBy"));
136+
assertTrue(paths.contains("lastUpdatedBy"));
137+
assertFalse(paths.contains("createdBy.id"));
138+
assertFalse(paths.contains("lastUpdatedBy.id"));
129139
}
130140

131141
@Test

‎dhis-2/dhis-test-web-api/src/test/java/org/hisp/dhis/webapi/controller/AbstractCrudControllerTest.java‎

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,7 @@
7373
import org.hisp.dhis.test.webapi.json.domain.JsonWebMessage;
7474
import org.hisp.dhis.user.User;
7575
import org.hisp.dhis.user.UserGroup;
76+
import org.hisp.dhis.user.UserRole;
7677
import org.junit.jupiter.api.DisplayName;
7778
import org.junit.jupiter.api.Test;
7879
import org.springframework.http.MediaType;
@@ -105,6 +106,66 @@ void testGetObject() {
105106
assertEquals(id, userById.getId());
106107
}
107108

109+
@Test
110+
@DisplayName(
111+
"GET single object: fields under a PropertyTransformer-controlled nested collection never"
112+
+ " change the response, however they're requested (DHIS2-21856)")
113+
void testGetObjectPropertyTransformerFieldsAreInvariant() {
114+
User user = makeUser("H");
115+
user.setEmail("h@h.org");
116+
userService.addUser(user);
117+
UserGroup group = createUserGroup('H', Set.of(user));
118+
manager.save(group);
119+
manager.flush();
120+
121+
// UserPropertyTransformer always serializes id/code/name/displayName/username and nothing
122+
// else -- verified against a live server to be byte-identical for "users", "users[href]" and
123+
// "users[*]". This only pins output correctness; it can't detect the N+1 this ticket actually
124+
// fixes (LinkService.generateLinks reflectively touching the real Hibernate collection before
125+
// field-filtering runs), because that bug is invisible in the JSON response by construction --
126+
// see FieldPathHelperPropertyTransformerPruningTest for the real regression guard, which
127+
// asserts on the field-path list itself rather than on JSON content.
128+
JsonObject baseline =
129+
GET("/userGroups/{id}?fields=id,users", group.getUid())
130+
.content(HttpStatus.OK)
131+
.getArray("users")
132+
.getObject(0);
133+
134+
for (String fields : List.of("users[href]", "users[*]", "users[id,name]")) {
135+
JsonObject userInGroup =
136+
GET("/userGroups/{id}?fields=id,{fields}", group.getUid(), fields)
137+
.content(HttpStatus.OK)
138+
.getArray("users")
139+
.getObject(0);
140+
assertEquals(baseline.toJson(), userInGroup.toJson());
141+
}
142+
}
143+
144+
@Test
145+
@DisplayName(
146+
"GET single object: href is still generated for a nested collection without a"
147+
+ " PropertyTransformer when explicitly requested (regression guard for the"
148+
+ " DHIS2-21856 hasHref rewrite)")
149+
void testGetObjectHrefGeneratedForNonTransformerNestedCollection() {
150+
UserRole role = createUserRole('H', "ALL");
151+
manager.save(role);
152+
User user = makeUser("H");
153+
user.setEmail("h@h.org");
154+
user.getUserRoles().add(role);
155+
userService.addUser(user);
156+
manager.flush();
157+
158+
JsonObject userRole =
159+
GET("/users/{id}?fields=userRoles[*]", user.getUid())
160+
.content(HttpStatus.OK)
161+
.getArray("userRoles")
162+
.getObject(0);
163+
164+
String href = userRole.getString("href").string();
165+
assertNotNull(href);
166+
assertTrue(href.endsWith("/userRoles/" + role.getUid()));
167+
}
168+
108169
@Test
109170
void testGetObjectProperty() {
110171
// response will look like: { "surname": <name> }

‎dhis-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/controller/AbstractFullReadOnlyController.java‎

Lines changed: 22 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -71,6 +71,8 @@
7171
import org.hisp.dhis.feedback.NotFoundException;
7272
import org.hisp.dhis.fieldfilter.FieldFilterService;
7373
import org.hisp.dhis.fieldfiltering.FieldFilterParams;
74+
import org.hisp.dhis.fieldfiltering.FieldFilterParser;
75+
import org.hisp.dhis.fieldfiltering.FieldPathHelper;
7476
import org.hisp.dhis.gist.GistBridge;
7577
import org.hisp.dhis.query.Filter;
7678
import org.hisp.dhis.query.Filters;
@@ -127,6 +129,8 @@ public abstract class AbstractFullReadOnlyController<
127129

128130
@Autowired protected org.hisp.dhis.fieldfiltering.FieldFilterService fieldFilterService;
129131

132+
@Autowired protected FieldPathHelper fieldPathHelper;
133+
130134
@Autowired protected LinkService linkService;
131135

132136
@Autowired protected AclService aclService;
@@ -545,8 +549,25 @@ private void cachePrivate(HttpServletResponse response) {
545549
ContextUtils.HEADER_CACHE_CONTROL, noCache().cachePrivate().getHeaderValue());
546550
}
547551

552+
/**
553+
* Whether {@code href} would actually appear in the response for {@code fields}, at any depth --
554+
* {@link LinkService#generateLinks} stamps {@code href} on every {@code IdentifiableObject}
555+
* property it finds when {@code deep=true} (root object and nested collections alike), then
556+
* field-filtering decides per-object whether it survives into the JSON; this is the coarse gate
557+
* deciding whether that walk is worth attempting at all. Delegates to the real field-path
558+
* expansion ({@link FieldPathHelper#apply}) instead of substring-matching the raw {@code fields}
559+
* string: a property with a {@code @PropertyTransformer} (e.g. {@code UserPropertyTransformer})
560+
* never has an {@code href} no matter how deeply nested or with what wildcard it's requested
561+
* (DHIS2-21856), and {@code apply()} already prunes everything beneath such a property -- so any
562+
* {@code href} path surviving here is one that could genuinely appear in the response. A raw
563+
* {@code fields.contains("*")} check can't tell a top-level {@code *} from one four levels deep
564+
* inside {@code users[*]}, which is what forced {@code generateLinks} to reflectively touch every
565+
* {@code IdentifiableObject} getter on the schema, including large Hibernate-backed collections,
566+
* purely to compute an {@code href} that field-filtering would discard anyway.
567+
*/
548568
private boolean hasHref(String fields) {
549-
return fieldsContains("href", fields);
569+
return fieldPathHelper.apply(FieldFilterParser.parse(fields), getEntityClass()).stream()
570+
.anyMatch(fieldPath -> "href".contentEquals(fieldPath.getPath().segment()));
550571
}
551572

552573
private void handleLinksAndAccess(List<T> entityList, String fields, boolean deep) {
@@ -555,15 +576,6 @@ private void handleLinksAndAccess(List<T> entityList, String fields, boolean dee
555576
}
556577
}
557578

558-
private boolean fieldsContains(String match, String fields) {
559-
return fields.contains("*")
560-
|| fields.contains(":all")
561-
|| fields.equals(match)
562-
|| fields.startsWith(match + ",")
563-
|| fields.endsWith("," + match)
564-
|| fields.contains("," + match + ",");
565-
}
566-
567579
// --------------------------------------------------------------------------
568580
// Reflection helpers
569581
// --------------------------------------------------------------------------

0 commit comments

Comments
 (0)