#36936: feat(roles): add PUT /v1/roles/{roleId} for role update - #37012
#36936: feat(roles): add PUT /v1/roles/{roleId} for role update#37012hassandotcms wants to merge 3 commits into
Conversation
…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 finished @hassandotcms's task in 3m 1s —— View job Code Review: PUT /v1/roles/{roleId}I reviewed the diff ( Verified correct:
New Issues
Existing
Overall this is a clean, well-tested addition. No blocking issues — the two items above are non-blocking polish. |
…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
left a comment
fabrizzio-dotCMS
left a comment
There was a problem hiding this comment.
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.
| role.getName(), role.getId())); | ||
| } | ||
|
|
||
| role.setName(roleForm.getRoleName()); |
There was a problem hiding this comment.
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.
| // 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)) { |
There was a problem hiding this comment.
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.
| * Helper to encapsulate Roles logic | ||
| * @author jsanca | ||
| */ | ||
| @ApplicationScoped |
There was a problem hiding this comment.
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.
| 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<>())); |
There was a problem hiding this comment.
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.
| content = @Content(mediaType = "application/json")) | ||
| }) | ||
| @PUT | ||
| @Path("/{roleId}") |
There was a problem hiding this comment.
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.
| * Expected Result: 409 ConflictException (DuplicateRoleKeyException from RoleAPIImpl.save). | ||
| */ | ||
| @Test(expected = ConflictException.class) | ||
| public void testUpdateRole_duplicateKey_conflict() throws Exception { |
There was a problem hiding this comment.
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.
| ContentToStringUtilTest.class, | ||
| CacheResourceIntegrationTest.class, | ||
| InodeExistenceCheckIntegrationTest.class, | ||
| com.dotcms.rest.api.v1.system.role.RoleResourceIntegrationTest.class, |
There was a problem hiding this comment.
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.
2 participants
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)POST /v1/roles; stricterthan DWR's users-portlet check — intentional). Gate extracted, shared with POST
ResponseEntityRoleDetailView, same shape asGET /v1/roles/{roleid}RoleHelper→@ApplicationScopedCDI; update logic@WrapInTransactionopenapi.yamlChecklist
RoleResourceIntegrationTest(MainSuite3a): all field updates,reparent to other/root/self/cycle, 403/404/409 cases, POST create regression
Fixes #36936