fix: skip getter invocation/recursion for PropertyTransformer fields in field filtering [DHIS2-21867]#24514
Merged
jason-p-pickering merged 4 commits intoJul 22, 2026
Conversation
…in field filtering [DHIS2-21867] FieldPathHelper.visitFieldPath (used by FieldFilterService.applyAccess and applySharingDisplayNames) walked every requested sub-path under a property regardless of whether that property has a PropertyTransformer, invoking the real getter and recursing into every collection element. For properties like OrganisationUnit.users, UserRole.users, UserGroup.members, etc. (all annotated with @PropertyTransformer(UserPropertyTransformer.class)), the final JSON shape is always a fixed 5-field DTO independent of what's requested underneath, so that walk computes Access/sharing-display-name mutations for leaves that can never appear in the response -- on a Hibernate-backed collection this forces full lazy-association materialization (N+1) purely to throw the result away. property.hasPropertyTransformer() is already tracked on the schema; adding a short-circuit in visitFieldPath closes this for every PropertyTransformer site with one change instead of per-entity workarounds. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…es N+1 [DHIS2-21867]
OrganisationUnitUsersFieldFilterPerformanceTest hits GET
/api/organisationUnits/{uid}?fields=id,name,users[id,name,userRoles[id,name]]
against the platform-perf DB's root org unit -- the exact repro from
DHIS2-21867, which forced 250,054 JDBC queries / ~70s before this branch's
fix to FieldPathHelper.visitFieldPath. Targets the same platform-perf DB
UIDs already used by UsersPerformanceTest.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
jbee
approved these changes
Jul 21, 2026
Contributor
There was a problem hiding this comment.
I thought about it. While the visiting of the object tree in general is generic we only use it to supply display name and access fields in some cases. This would never apply to a property that is transformed via the property transformer which this PR uses to short-circuit the visit.
In think we could even exclude paths from the visit that have a transformer attached from fields as the same logic applies that what is rendered is transformed and would not need injecting some transient data.
david-mackessy
approved these changes
Jul 21, 2026
…hold from real CI run 30s was too tight: a real baseline-vs-candidate CI run (perf runner, platform-perf DB, 10 iterations) showed the fixed branch ranging 21,597-42,634ms (p95 42,634ms) depending on 2nd-level-cache warmth, against master's flat ~51-52s. Raise to 48s -- enough headroom above the fixed range's observed max while staying below master's observed floor. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
netroms
approved these changes
Jul 21, 2026
…cAuth Protocol-level basicAuth() re-verifies the bcrypt password on every request, polluting the measured GET's response time with a fixed ~70ms auth cost repeated per iteration. Switch to the UsersPerformanceTest pattern: authenticate once per virtual user via a separately-named request, then reuse the session cookie so the measured endpoint reflects only its own cost. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
This was referenced Jul 21, 2026
Merged
jason-p-pickering
deleted the
fix/DHIS2-21867-user-property-transformer-n1
branch
July 22, 2026 11:02
jason-p-pickering
added a commit
that referenced
this pull request
Jul 22, 2026
… 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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
FieldPathHelper.visitFieldPath(used byFieldFilterService.applyAccess/applySharingDisplayNames) had no short-circuit for properties annotated with@PropertyTransformer(e.g.UserPropertyTransformer, used onOrganisationUnit.users,UserRole.users,UserGroup.members,createdBy/lastUpdatedBy/user, and ~10 other sites).Access/sharing-display-name mutations was pure waste — and on a Hibernate-backed collection it forced full lazy-association materialization (N+1) just to throw the result away.visitFieldPathviaproperty.hasPropertyTransformer(), which is already tracked on the schema. Closes this for every@PropertyTransformersite with one change.UserRole/UserGroupmembers specifically, but only as a side effect of substituting a transientUser— it didn't fix the generalvisitFieldPathbehavior, so sites likeOrganisationUnit.usersremained exposed.Jira: https://dhis2.atlassian.net/browse/DHIS2-21867
Measured impact
Glowroot trace,
GET /api/organisationUnits/{id}?fields=id,name,users[id,name,userRoles[id,name]]on an org unit with many users, before vs. after this fix (same data, same server, only this change applied):The query-count collapse confirms the root cause:
visitFieldPathwas callinguser.getUserRoles()once per user while walking theusers.userRoles.id/users.userRoles.nameleaves looking foraccess/sharingsegments that were never there, triggering one lazy-load per user. Post-fix, 0 CPU profile samples land inFieldPathHelperat all (was 8/684 pre-fix, and the query storm it triggered dominated the rest).Not in scope for this PR: the remaining 18.6s is now dominated by (a) the one legitimate
getUsers()call Jackson still needs to serialize theusersarray —OrganisationUnit.usersnever got theapplyUserSummaries-style raw-SQL projection that #24512 gaveUserRole/UserGroup, so that collection still fully materializes via Hibernate — and (b) Ehcache 2nd-level-cacheputFromLoadpaying a full Java-serialization round-trip per cachedUserentity (ObjectInputStream/SerializingCopier, ~73% of post-fix CPU samples). Both predate this change (visible as a smaller share of the much larger pre-fix profile) and are being tracked as a separate follow-up.CI comparison (Gatling, perf runner)
Independent confirmation on real, isolated CI hardware (not a locally-instrumented trace):
performance-tests-compare.ymlrun,dhis2/core-dev:latest(master) vs.dhis2/core-pr:24514(this branch), platform-perf DB,OrganisationUnitUsersFieldFilterPerformanceTest, 10 iterations each (excluding warmup):Master is a flat ~51–52s on every one of the 10 requests (the N+1 storm is deterministic). The branch ranges 21.6–42.6s, consistent with the remaining, separately-tracked bottleneck above being sensitive to 2nd-level-cache warmth between requests.
Test plan
FieldPathHelperVisitFieldPathTestreproduces the bug (getter invoked despite aPropertyTransformeron the property) against unfixed code, and passes after the fix.dhis-service-field-filteringmodule test suite passes.OrganisationUnitControllerTest,UserControllerTest,FieldFilterServiceTest,SharingControllerTest(dhis-test-web-api) andDefaultFieldFilterServiceTest(dhis-service-dxf2) pass unchanged — output JSON is unaffected since the transformer already discarded that subtree; only the wasted lazy-load work is removed.OrganisationUnitUsersFieldFilterPerformanceTest(dhis-test-performance) added, and used to produce the CI comparison above.🤖 Generated with Claude Code