docs: design for JSON Patch side-effect regression tests

Spec for multi-entity integration + web-api coverage answering
David's system-wide PATCH concern on PR #24489 / DHIS2-21852.

Author: netroms

docs/superpowers/specs/2026-07-23-jsonpatch-side-effect-tests-design.md

@@ -0,0 +1,185 @@
+# Design: JSON Patch side-effect regression tests (PR #24489 / DHIS2-21852)
+
+Date: 2026-07-23  
+PR: https://github.com/dhis2/dhis2-core/pull/24489  
+Branch: `fix-userrole-jsonpatch-lazy-members`  
+Author: Morten (netroms)
+
+## Problem
+
+PR #24489 changes `JsonPatchManager` system-wide: during `apply`, non-owner collection properties that are not referenced by any patch path are omitted from serialization so lazy Hibernate collections (e.g. `UserRole.members`) are not initialized on scalar PATCH.
+
+Review feedback:
+
+1. **Jan (retracted in weekly meeting)** — filter mechanics complexity. Out of scope for this follow-up.
+2. **David** — worried about unknown side effects because the change applies to all PATCHes, not only `/userRoles`. Asked whether PUT has the same issue.
+
+Existing PR tests are almost entirely UserRole-focused. That is not enough to affirm the generic rule.
+
+## PUT conclusion (no code change)
+
+PUT does **not** share this bug path.
+
+| | PATCH `/{uid}` | PUT `/{uid}` |
+|---|---|---|
+| Entry | `AbstractCrudController.patchObject` → `doPatch` | `AbstractCrudController.putJsonObject` |
+| Core | `jsonPatchManager.apply(patch, persistedObject)` | `deserializeJsonEntity(request)` only |
+| Then | `importMetadata(UPDATE)` | `importMetadata(UPDATE)` |
+
+Evidence: `AbstractCrudController` PUT ~393–422 never calls `JsonPatchManager`. `UserRoleController` does not override PUT/PATCH.
+
+PUT can still be heavy if the client sends a large `users` array; that is payload size, not live lazy-collection hydration via `valueToTree` / `handleCollectionUpdates`.
+
+No PUT regression tests are required for merge. Document the distinction in the PR reply.
+
+## Fix rule under test (unchanged production behavior)
+
+Skip a collection property only when all hold:
+
+1. `property.isCollection()`
+2. `!property.isOwner()`
+3. patch paths contain neither `property.getName()` nor `property.getCollectionName()`
+
+Owner collections must always serialize: metadata import UPDATE would clear them if omitted.
+
+Note: `UserGroup.users` (`members`) is **owner** (hbm has no `inverse`). The PR description implying UserGroup members benefit from the skip is slightly wrong; owner collections are still fully walked. Tests must document that contrast.
+
+## Goal
+
+Add integration + focused web-api tests that lock the skip rule on multiple entity types so David's system-wide side-effect concern is answered with evidence.
+
+## Non-goals
+
+- Production code changes (unless a test exposes a real bug)
+- PUT tests as merge gates
+- Gatling / perf expansion
+- Reworking Jan's filter-mechanics feedback
+- Schema-wide brute-force PATCH of every type
+
+## Invariants
+
+| ID | Invariant | Layer |
+|---|---|---|
+| I1 | Unreferenced **non-owner** collection stays lazy (`!Hibernate.isInitialized`) after scalar `jsonPatchManager.apply` | integration |
+| I2 | **Owner** collections still present on the patched object after scalar apply (not wiped by omit) | integration |
+| I3 | Explicit patch path targeting a non-owner collection still applies without throw (path-aware skip, not a ban) | integration (existing UserRole `/users`) |
+| I4 | HTTP PATCH of a scalar leaves unreferenced membership/children/groups unchanged on GET | web-api |
+| I5 | HTTP PATCH `replace /users` on UserRole does not change DB membership (inverse + no setter) | web-api |
+
+## Representative types
+
+| Entity | Collection | Owner? | Role in matrix |
+|---|---|---|---|
+| UserRole | `users` / field `members` | no | primary bug; already covered |
+| UserRole | `authorities` | yes | owner preserve; already covered |
+| OrganisationUnit | `children`, `users` | no | second fat inverse type |
+| User | `userGroups` (field `groups`) | no | common PATCH target; inverse |
+| User | `userRoles` | yes | owner must remain on patched user |
+| UserGroup | `users` (field `members`) | yes | must **not** be skipped; round-trip |
+| UserGroup | `managedByGroups` | no | skip when unreferenced |
+| DataElementGroup | `dataElements` / members | yes | owner wipe guard outside user domain |
+
+## Test plan
+
+### A. `JsonPatchManagerTest` (integration)
+
+File: `dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/jsonpatch/JsonPatchManagerTest.java`
+
+Keep existing PR tests:
+
+- `testUserRoleScalarPatchDoesNotInitializeMembers` (I1)
+- `testUserRoleOwnerAuthoritiesPreservedOnNamePatch` (I2)
+- `testUserRoleUsersPathPatchDoesNotThrow` (I3)
+
+Add:
+
+1. **`testOrganisationUnitScalarPatchDoesNotInitializeInverseCollections`**
+   - Setup: parent OU, 2 children, 1 user assigned to parent; `clearSession`; reload parent
+   - Assert children/users lazy before apply
+   - Patch: `replace /name`
+   - Assert name updated; `!Hibernate.isInitialized(children)`; `!Hibernate.isInitialized(users)`
+
+2. **`testUserScalarPatchDoesNotInitializeUserGroups`**
+   - Setup: user with a userRole and membership in a userGroup; `clearSession`; reload user
+   - Assert `groups` lazy before apply
+   - Patch: `replace /firstName`
+   - Assert firstName updated; `!isInitialized(groups)`; patched object still has non-empty `userRoles` (I2 owner)
+
+3. **`testUserGroupScalarPatchKeepsOwnerUsersAndSkipsManagedBy`**
+   - Setup: group with 2 users; optional managedByGroups link; `clearSession`; reload
+   - Patch: `replace /name`
+   - Assert patched name; patched `users`/`members` size == 2 (owner not omitted)
+   - Assert `!isInitialized(managedByGroups)` when that collection was left lazy
+
+4. **`testDataElementGroupNamePatchPreservesOwnerMembers`**
+   - Setup: DEG with 2 data elements (same spirit as existing collection test)
+   - Patch: `replace /name`
+   - Assert patched name; member count still 2
+
+Setup pattern: save graph → `clearSession()` → reload → assert lazy where relevant → `jsonPatchManager.apply` → assert. Match style of existing UserRole lazy test (DisplayName, Hibernate.isInitialized).
+
+### B. Web-api: new `JsonPatchSideEffectControllerTest`
+
+File: `dhis-2/dhis-test-web-api/src/test/java/org/hisp/dhis/webapi/controller/JsonPatchSideEffectControllerTest.java`
+
+Base: `H2ControllerIntegrationTestBase`, `@Transactional`, same HTTP helpers as other controller tests.
+
+Author: Morten (netroms) on the new file.
+
+Tests:
+
+1. **`testPatchOrganisationUnitNameKeepsChildren`**
+   - Create parent + child OUs
+   - `PATCH /organisationUnits/{parent}` replace `/name`
+   - GET parent with `fields=name,children[id]` → new name; children size unchanged
+
+2. **`testPatchUserGroupNameKeepsUsers`**
+   - Create group with a user member
+   - `PATCH /userGroups/{id}` replace `/name`
+   - GET `fields=name,users[id]` → new name; users size unchanged
+
+3. **`testPatchUserFirstNameKeepsUserGroups`**
+   - User in a group
+   - `PATCH /users/{id}?importReportMode=ERRORS` replace `/firstName`
+   - GET `fields=firstName,userGroups[id]` → new firstName; userGroups size unchanged
+
+4. **`testPatchUserRoleUsersPathDoesNotChangeMembership`**
+   - Role with one assigned user (via owner side / existing helpers)
+   - `PATCH /userRoles/{id}` `replace /users` with `[]`
+   - GET `fields=users[id]` → still 1 user (I5)
+   - Accept 200 with possible ERRORS_NOT_OWNER reporting; membership must not drop
+
+Keep existing `UserControllerTest.testPatchUserRoleDescriptionKeepsUsers` on the PR (I4 for UserRole).
+
+### C. PR reply (after tests green)
+
+Reply to David covering:
+
+1. System-wide concern addressed by multi-entity integration + web tests (list invariants I1–I5).
+2. PUT: different path; no `JsonPatchManager`; not the same issue.
+3. Owner vs non-owner: UserGroup.users is owner and is not skipped; test proves members retained.
+4. Explicit `/users` on UserRole does not persist membership (owner is `User.userRoles` / membership sub-resource).
+
+## Implementation notes
+
+- Work on branch `fix-userrole-jsonpatch-lazy-members`.
+- Tests only unless a failure reveals a product bug.
+- Prefer entity factories already used in `JsonPatchManagerTest` / web-api bases (`createOrganisationUnit`, `makeUser`, `createUserRole`, `createDataElementGroup`, etc.).
+- For UserGroup managedBy relationship, use existing domain APIs if cheap; if setup is awkward, still assert owner `users` preservation and drop managedBy lazy assert rather than block the suite.
+- Do not use `git add -A`; stage specific paths.
+- Update root `CHANGELOG.md` if the project log is in use for this workstream; otherwise note in PR body only.
+
+## Acceptance criteria
+
+1. New integration tests cover OU, User, UserGroup, DataElementGroup as specified.
+2. New web-api class covers OU, UserGroup, User, UserRole `/users` no-op.
+3. All new tests pass locally (integration + web-api modules as applicable).
+4. Existing PR UserRole tests still pass.
+5. No production code change required for green tests (ideal).
+6. Design/plan docs committed; PR comment ready for David.
+
+## Out of scope follow-ups
+
+- Optional PUT `/userRoles` without `users` body keeps members (reply ammo only).
+- Fixing UserGroup owner lazy cost on PATCH (separate perf ticket if needed).
+- Simplifying Jackson filter mechanics (Jan; retracted for now).