ACL supporting dry run mode and batching - #309
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a shared CommitPolicy seam so ACL-related operations (users/groups/permissions) and locking respect repository transaction control (dry-run, batching, and auto-commit) instead of always persisting immediately.
Changes:
- Added
CommitPolicyinterface and wiredRepoto implement it, so auto-commit decisions can be shared. - Updated ACL internals to commit via the provided
CommitPolicyrather than always callingsession.save(). - Updated
Lockerto consultCommitPolicy.isAutoCommit()when deciding whether to persist lock changes.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| core/src/main/java/dev/vml/es/acm/core/repo/Repo.java | Makes Repo the shared commit policy owner and adds batch(...) support. |
| core/src/main/java/dev/vml/es/acm/core/repo/Locker.java | Uses a commit policy (auto-commit flag) when committing lock/unlock operations. |
| core/src/main/java/dev/vml/es/acm/core/repo/CommitPolicy.java | New commit-policy abstraction used by repo/acl/locker. |
| core/src/main/java/dev/vml/es/acm/core/code/CodeContext.java | Passes Repo into Acl so ACL respects repo transaction settings. |
| core/src/main/java/dev/vml/es/acm/core/acl/utils/PermissionsManager.java | Switches persistence to CommitPolicy.commit(context) with contextual commit messages. |
| core/src/main/java/dev/vml/es/acm/core/acl/utils/AuthorizableManager.java | Switches persistence to CommitPolicy.commit(context) and improves one error message. |
| core/src/main/java/dev/vml/es/acm/core/acl/AclContext.java | Plumbs CommitPolicy into ACL managers. |
| core/src/main/java/dev/vml/es/acm/core/acl/Acl.java | Adds constructor overload to accept a CommitPolicy. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (5)
core/src/main/java/dev/vml/es/acm/core/acl/utils/AuthorizableManager.java:241
commitPolicy.commit(...)can throwRepoException(runtime) which will currently escape as a non-AclException, unlike other failure paths in this class that consistently wrap errors inAclException. Consider translating commit failures toAclExceptionto keep the ACL API’s exception surface consistent.
private void save(String context) {
commitPolicy.commit(context);
}
core/src/main/java/dev/vml/es/acm/core/acl/utils/PermissionsManager.java:65
commitPolicy.commit(...)may throwRepoExceptionwhich currently bubbles up as a non-AclException. Wrapping commit failures here would make error handling consistent with the rest ofPermissionsManager(which usesAclExceptionfor failures).
private void save(String context) {
commitPolicy.commit(context);
}
core/src/main/java/dev/vml/es/acm/core/repo/Repo.java:146
Repo.batchrestoresautoCommitinfinallybut does not revert transient changes whenoperation.run()or the finalcommit()fails. That can leave uncommitted changes in the shared session, and subsequent operations (with auto-commit re-enabled) may accidentally persist a partial batch. Revert on failure (and consider suppressing revert failures onto the original exception).
public void batch(Runnable operation) {
if (!autoCommit) {
throw new RepoException("Cannot start a batch: already inside a batch or dry run scope.");
}
this.autoCommit = false;
getLogger().info("Batch started. Changes will be committed once at the end.");
try {
operation.run();
commit();
getLogger().info("Batch completed. Changes committed.");
} finally {
this.autoCommit = true;
}
}
ui.content/src/main/content/jcr_root/conf/acm/settings/snippet/available/core/repo/dry_run.yml:17
- Docs say
autoCommitapplies only torepoandacl, but the PR also wires the policy into the locker (viaRepo.getLocker()), and explicit commits likerepo.commit()/resourceResolver.commit()/session.save()can still persist changes and defeat dry-run. Please clarify to avoid giving a false sense of safety.
The `autoCommit` control applies only to operations performed via the `repo` and `acl` services.
Since everything shares the same session, calling vanilla Sling/AEM/JCR APIs directly (e.g. `session.save()`)
may trigger an accidental commit of the pending changes and defeat the dry run.
ui.content/src/main/content/jcr_root/conf/acm/settings/snippet/available/core/repo/batch.yml:19
- Docs say
autoCommitapplies only torepoandacl, but the PR also shares the commit policy with the locker (viaRepo.getLocker()), and explicit commits likerepo.commit()/resourceResolver.commit()/session.save()can still persist changes mid-batch. Consider clarifying this so the “commit at the end” guarantee isn’t overstated.
The `autoCommit` control applies only to operations performed via the `repo` and `acl` services.
Since everything shares the same session, calling vanilla Sling/AEM/JCR APIs directly (e.g. `session.save()`)
may trigger an accidental commit of the pending changes and break the "commit at the end" guarantee.
jakub-berlinski-wttech
left a comment
There was a problem hiding this comment.
I like this change
🎉 ACL now respects repo transaction control (dryRun / batch / autoCommit)
Until now, ACM's acl operations (users, groups, permissions) always saved to the
repository immediately, ignoring repo transaction settings. That's fixed — acl now
goes through the same commit policy as repo, so you can preview and batch ACL changes.
What you can do now:
• Dry-run your ACL setup — preview everything, commit nothing:
• Batch large ACL scripts into a single commit via repo.batch { ... } instead of a
save after every operation.
• Plain scripts still auto-commit per operation (default), so nothing changes unless you opt in.
Proof (dry-run vs real runs):
# dry run ON -> created + applied, then "Changes reverted" (nothing persisted)
Created user 'alice.doe'
Applied allow permissions for authorizable 'alice.doe' at path '/content'
Dry run completed. Changes reverted.
Under the hood: introduced a small CommitPolicy seam shared by repo, acl and the
locker, so a single auto-commit flag governs the whole session. Backward compatible. ✅