#36936: feat(roles): add PUT /v1/roles/{roleId} for role update by hassandotcms · Pull Request #37012 · dotCMS/core

hassandotcms · 2026-08-11T15:50:11Z

Proposed Changes

PUT /api/v1/roles/{roleId} — update role name, key, description, can-grant flags,
and parent. Replaces DWR RoleAjax#updateRole (Angular portlet migration, Dojo to Angular: Roles and Tools Portlet #36909)
parentRoleId: null → role becomes root (DWR parity)
404 missing role/parent, 403 system/locked, 400 invalid name, 409 duplicate key/name
400 on reparent cycles — new guard, legacy had none server-side
Auth: backend user + roles portlet + CMS admin (same gate as POST /v1/roles; stricter
than DWR's users-portlet check — intentional). Gate extracted, shared with POST
Response = ResponseEntityRoleDetailView, same shape as GET /v1/roles/{roleid}
RoleHelper → @ApplicationScoped CDI; update logic @WrapInTransaction
Regenerated openapi.yaml

Checklist

15 integration tests in RoleResourceIntegrationTest (MainSuite3a): all field updates,
reparent to other/root/self/cycle, 403/404/409 cases, POST create regression
No breaking changes: additive endpoint; DWR and POST behavior unchanged

…eparent * New v1 REST endpoint replacing DWR RoleAjax#updateRole for the Angular Roles & Tools portlet (epic #36909): updates name, key, description, can-grant flags and parent; null parentRoleId reparents to root (DWR parity) * Guards: 404 missing role/parent, 403 system/locked roles, 400 invalid name and reparent cycles (net-new corruption guard), 409 duplicate key/name * Auth: backend user + roles portlet + CMS admin, shared gate extracted and reused by POST /v1/roles (behavior unchanged, regression-tested) * RoleHelper promoted to @ApplicationScoped CDI with the update logic under @WrapInTransaction; response reuses RoleView/ResponseEntityRoleDetailView (same shape as GET /v1/roles/{roleid}) * 15 integration tests (TDD red->green), registered in MainSuite3a * Regenerated openapi.yaml

claude · 2026-08-11T15:55:58Z

Claude finished @hassandotcms's task in 1m 17s —— View job

Review — PR #37012: PUT /v1/roles/{roleId}

Read changed files & diff vs origin/main
Verify prior review findings
Check for new issues
Post review

I re-reviewed the latest commits against origin/main, focused on whether the fixes for the earlier review landed correctly and whether they introduced anything new.

Resolved

✅ RoleHelper.java:114-127 — Cache poisoning fixed. All validation (missing role, system/locked, self-parent, missing parent, cycle) now runs before any mutation, and the update is applied to a detached new Role() populated via BeanUtils.copyProperties, never to the cache-resident instance returned by loadRoleById. A rejected save can no longer leave phantom name/key/description in the cache. The copyProperties failure path is wrapped and surfaced as DotDataException.
✅ RoleHelper.java:104 — Hierarchy API reused. The hand-rolled ancestor walk is gone; the cycle guard is now roleAPI.isParentRole(role, parentRole), and self-parenting is still guarded just above it.
✅ RoleHelper.java:40 — CDI no longer half-applied. The @ApplicationScoped/@Inject annotations were dropped, so RoleResource's new RoleHelper() is no longer misleading. @WrapInTransaction still applies (ByteBuddy package weaving).
✅ RoleResourceIntegrationTest.java:287,362,410 — Coverage gaps closed. testUpdateRole_duplicateKey_conflict, testUpdateRole_invalidName_badRequest, and testUpdateRole_missingParent_notFound now reload the role after the expected exception and assert name/key/description (and parent) survived unchanged. The cache is pre-warmed outside the failing transaction, which is what makes these assertions actually able to detect the old poisoning bug rather than being masked by rollback eviction. Good touch.
✅ RoleResource.java:395 — Path aligned to @Path("/{roleid}") (lowercase), matching the existing GET.
✅ MainSuite3a.java:17 — RoleResourceIntegrationTest is now imported and referenced by simple name, consistent with the rest of the suite.

Existing

🟡 Medium: RoleResource.java:418-420 — N+1 loadRoleById per child when building the response view. Non-blocking and consistent with the existing GET /{roleid}; author noted the GET (and this response-building path) is being reworked in an upcoming version.

New Issues

No new issues found. The detached-copy fix is correct: copyProperties reads the cached instance without mutating it, preserves id (so save performs an update, not an insert) and preserves system/locked as false (so the pre-check remains authoritative), and reparent/FQN recomputation is handled downstream by RoleFactoryImpl.save.

The blocking issue from the prior review is resolved and the new tests pin it. LGTM aside from the acknowledged, non-blocking N+1.

· branch 36936-roles-api-put-role-update

…th a test * OpenAPI description now spells out that PUT overwrites every field: omitted booleans reset to false, omitted roleKey/description are cleared, omitted parentRoleId reparents to root (DWR parity) * New IT testUpdateRole_fullReplace_omittedFieldsAreReset pins the contract so drift to merge/PATCH semantics is a deliberate, test-breaking change * Addresses claude[bot] review finding on PR #37012

fabrizzio-dotCMS

Solid PR overall — good docs, real integration coverage, OpenAPI regenerated, and the auth gate extraction doesn't relax POST (it already required roles portlet + CMS admin) while improving the SecurityLogger call site. I also verified the two things I was most worried about and they're fine: reparenting descendants is handled (RoleFactoryImpl.save recomputes the FQN of children/grandchildren), and @WrapInTransaction still applies even though RoleResource instantiates the helper with new (it's woven by ByteBuddy per package, not a CDI interceptor).

One blocking issue though: the update mutates the cached Role instance before validating, so a rejected request leaves poisoned state in the local cache. Details inline, plus a few smaller notes.

fabrizzio-dotCMS · 2026-08-14T16:17:27Z

+                            role.getName(), role.getId()));
+        }
+
+        role.setName(roleForm.getRoleName());


In-place mutation of the cached Role — poisons the cache when the update is rejected.

roleAPI.loadRoleById() returns the cached instance, not a copy: RoleFactoryImpl.getRoleById does r = rc.get(roleId) and RoleCacheImpl.get returns the reference straight out of the cache region. So these setters mutate the object that every other reader on this node will get.

Failure scenario: PUT a roleKey that already belongs to another role. roleAPI.save throws DuplicateRoleKeyException, the transaction rolls back, the client correctly gets a 409 — but the in-memory Role keeps the rejected name/key, so a subsequent GET /v1/roles/{id} on that node returns phantom values until a cache flush. Worse, RoleCacheImpl.add also indexes the role under keyGroup + roleKey, so the cache ends up holding a role advertising a roleKey that doesn't exist in the DB. Same applies to the 400 (invalid name), 400 (cycle) and 404 (missing parent) paths — note the parent guards below run after name/key/description have already been mutated.

Suggested fix: validate everything first, then mutate a detached copy (new Role() + setters, or BeanUtils.cloneBean) and pass that to save. This is exactly what RoleFactoryImpl.save already does internally — it loads a fresh Hibernate instance and copies properties onto it rather than persisting the caller's object. A CacheLocator.getRoleCache().remove(role) in the catch blocks would be a minimum mitigation, but working on a copy is the clean fix.

this is insightful. fixing it.

fabrizzio-dotCMS · 2026-08-14T16:17:28Z

+            // findRoleHierarchy walks getParent() up to the root, so it returns the proposed
+            // parent and all its ancestors — if the edited role is among them, the reparent
+            // would create a cycle
+            for (final Role ancestor : this.roleAPI.findRoleHierarchy(parentRole)) {


Reuse the existing hierarchy API. This loop is exactly roleAPI.isParentRole(role, parentRole) (RoleAPIImpl:583) — same findRoleHierarchy walk, already @CloseDBIfOpened. Since self-parenting is guarded a few lines above, isParentRole excluding the child itself is not a problem here.

Also worth knowing about the primitive you're building on: findRoleHierarchy does while(!role.getParent().equals(role.getId()) && i < 100) — it NPEs if any ancestor has a null parent (the factory has if(UtilMethods.isSet(r.getParent())) checks, so unset parents are considered possible), and it silently truncates at 100 levels, which means on an already-corrupted hierarchy this guard passes without detecting anything. Not introduced by this PR, but the new endpoint is now the main caller.

makes sense

fabrizzio-dotCMS · 2026-08-14T16:17:28Z

 * Helper to encapsulate Roles logic
 * @author jsanca
 */
+@ApplicationScoped


Half-applied CDI. RoleHelper becomes @ApplicationScoped with an @Inject constructor, but RoleResource still holds private final RoleHelper roleHelper = new RoleHelper();, so the bean is never resolved through CDI and the annotation is effectively dead.

To be clear, nothing is broken: RoleAPI does have a producer (APILocatorProducers#getRoleAPI) so the bean would be satisfiable, and @WrapInTransaction still takes effect because it's woven by the ByteBuddy agent over the com.dotcms package (ByteBuddyFactory), not by a CDI interceptor. But either inject the helper into the resource or drop the CDI annotations, otherwise the next reader will assume proxying is in play.

fabrizzio-dotCMS · 2026-08-14T16:17:28Z

+		final List<String> roleChildrenIdList = null != updatedRole.getRoleChildren()
+				? updatedRole.getRoleChildren() : new ArrayList<>();
+		for (final String childRoleId : roleChildrenIdList) {
+			childrenRoles.add(new RoleView(this.roleAPI.loadRoleById(childRoleId), new ArrayList<>()));


Minor: N+1 loadRoleById to build the children views. It mirrors what GET /{roleid} already does so it's consistent, and it's cache-backed, but for a role with many children this is one call per child on every update.

we aim to change GET as well, because there is a new version of it coming in.

fabrizzio-dotCMS · 2026-08-14T16:17:28Z

+					content = @Content(mediaType = "application/json"))
+	})
+	@PUT
+	@Path("/{roleId}")


Nit: the existing GET on this resource uses @Path("/{roleid}") (lowercase id) while this uses {roleId}. Harmless for JAX-RS matching, but it makes the two endpoints look like different paths at a glance — worth aligning.

fabrizzio-dotCMS · 2026-08-14T16:17:28Z

+     * Expected Result: 409 ConflictException (DuplicateRoleKeyException from RoleAPIImpl.save).
+     */
+    @Test(expected = ConflictException.class)
+    public void testUpdateRole_duplicateKey_conflict() throws Exception {


Coverage gap that maps to the cache issue above. The rejection tests assert the exception type only (or, for the cycle test, only parent — which happens to be mutated after the throw, so it passes). None of them re-read the role and assert the other fields survived a rejected update, which is precisely how the cached-instance mutation slips through.

Suggest adding to testUpdateRole_duplicateKey_conflict / testUpdateRole_invalidName_badRequest / testUpdateRole_missingParent_notFound: after the expected exception, roleAPI.loadRoleById(role.getId()) and assert name, key and description still match the original. Those assertions should fail on the current implementation.

resolved all the feedback.

fabrizzio-dotCMS · 2026-08-14T16:17:28Z

        ContentToStringUtilTest.class,
        CacheResourceIntegrationTest.class,
        InodeExistenceCheckIntegrationTest.class,
+        com.dotcms.rest.api.v1.system.role.RoleResourceIntegrationTest.class,


Nit: every other entry in this suite is referenced by simple name with an import at the top; this one is a fully-qualified inline reference. Add the import for consistency.

… saves cannot poison the role cache Review fixes for PR #37012 (fabrizzio-dotCMS): * loadRoleById returns the cache-resident Role instance; the update previously mutated it in place before validation, so a rejected save (duplicate key/name, invalid name, cycle, missing parent) left phantom values in the local cache. Now: validate first, then copy the role (BeanUtils.copyProperties — the same mechanism RoleFactoryImpl.save uses) and save the copy * Rejection tests now pre-warm the cache (production-warm case: entries cached inside the failing transaction are rollback-evicted by CommitListenerCacheWrapper and mask the bug) and assert post-rejection that name/key/description survived — all five failed before the fix, 16/16 after * Replace hand-rolled ancestor walk with roleAPI.isParentRole * Drop dead CDI annotations on RoleHelper (nothing injects it; transactions are ByteBuddy-woven, not CDI-intercepted) * Align path template with sibling GET: {roleId} -> {roleid}; regenerated openapi * MainSuite3a: import instead of fully-qualified suite entry

…-role-update # Conflicts: # dotcms-integration/src/test/java/com/dotcms/MainSuite3a.java

github-project-automation Bot added this to dotCMS - Product Planning Aug 11, 2026

github-actions Bot added the Area : Backend PR changes Java/Maven backend code label Aug 11, 2026

hassandotcms requested review from fabrizzio-dotCMS and jcastro-dotcms August 11, 2026 15:57

hassandotcms marked this pull request as ready for review August 11, 2026 15:57

Merge branch 'main' into 36936-roles-api-put-role-update

d2b55cf

github-actions Bot mentioned this pull request Aug 11, 2026

[Task] Roles API: add PUT /v1/roles/{roleId} for role update + reparent #36936

Open

6 tasks

hassandotcms mentioned this pull request Aug 13, 2026

#36939: feat(roles): add DELETE /v1/roles/{roleId} for role deletion #37042

Open

fabrizzio-dotCMS reviewed Aug 14, 2026

View reviewed changes

hassandotcms requested a review from fabrizzio-dotCMS August 16, 2026 13:01

Merge remote-tracking branch 'upstream/main' into 36936-roles-api-put…

44970ef

…-role-update # Conflicts: # dotcms-integration/src/test/java/com/dotcms/MainSuite3a.java

claude Bot added the AI: Safe To Rollback label Aug 16, 2026

Navigation Menu

Uh oh!

#36936: feat(roles): add PUT /v1/roles/{roleId} for role update - #37012

#36936: feat(roles): add PUT /v1/roles/{roleId} for role update#37012
hassandotcms wants to merge 5 commits into
mainfrom
36936-roles-api-put-role-update

hassandotcms commented Aug 11, 2026

Uh oh!

claude Bot commented Aug 11, 2026 •

edited

Loading

Uh oh!

fabrizzio-dotCMS left a comment

Uh oh!

fabrizzio-dotCMS Aug 14, 2026

Uh oh!

hassandotcms Aug 16, 2026

Uh oh!

fabrizzio-dotCMS Aug 14, 2026

Uh oh!

hassandotcms Aug 16, 2026

Uh oh!

fabrizzio-dotCMS Aug 14, 2026

Uh oh!

fabrizzio-dotCMS Aug 14, 2026

Uh oh!

hassandotcms Aug 16, 2026

Uh oh!

fabrizzio-dotCMS Aug 14, 2026

Uh oh!

fabrizzio-dotCMS Aug 14, 2026

Uh oh!

hassandotcms Aug 16, 2026

Uh oh!

fabrizzio-dotCMS Aug 14, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

hassandotcms commented Aug 11, 2026

Proposed Changes

Checklist

Uh oh!

claude Bot commented Aug 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review — PR #37012: PUT /v1/roles/{roleId}

Resolved

Existing

New Issues

Uh oh!

fabrizzio-dotCMS left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

claude Bot commented Aug 11, 2026 •

edited

Loading