anchela commented on code in PR #45:
URL:
https://github.com/apache/sling-org-apache-sling-jcr-repoinit/pull/45#discussion_r1387009917
##########
src/main/java/org/apache/sling/jcr/repoinit/impl/AclUtil.java:
##########
@@ -152,7 +154,9 @@ private static void setAcl(Session session, List<String>
principals, String jcrP
boolean changed = false;
for (String name : principals) {
- Principal principal = AccessControlUtils.getPrincipal(session,
name);
+ Principal principal = options.contains("ignoreMissingPrincipal")
Review Comment:
i would recommend to create a constant in the jcr-repoinit space such that
it is not just hidden somewhere in the code
also, i would love this option to be documented in the repo-init
documentation. but since that is in a separate git repo, you probably have to
do it separately. but please don't forget it.
##########
src/main/java/org/apache/sling/jcr/repoinit/impl/AclVisitor.java:
##########
Review Comment:
sorry too many changes to review with reasonable effort :(
can you avoid any renaming of methods that are unrelated to the change
required here?
i am all in favor of cleaning up the code base, but please use a separate
ticket for that.
##########
src/main/java/org/apache/sling/jcr/repoinit/impl/AclUtil.java:
##########
@@ -152,7 +154,9 @@ private static void setAcl(Session session, List<String>
principals, String jcrP
boolean changed = false;
for (String name : principals) {
- Principal principal = AccessControlUtils.getPrincipal(session,
name);
+ Principal principal = options.contains("ignoreMissingPrincipal")
+ ? () -> name
Review Comment:
i would slightly alter the logic here, as the fallback below is
significant..... if you just use the specified name now, you might break that
contract below.
so, i would suggest:
- always try to retrieve the principal first
- if it does not exist -> try backwards compatible fallback to original code
- if it does not exist -> create the anonymous principal in case option is
set as you do here, otherwise use old behavior.
i believe this way we run the least risk of introducing subtle regressions
that are super hard to troubleshoot.
##########
src/main/java/org/apache/sling/jcr/repoinit/impl/DoNothingVisitor.java:
##########
@@ -66,7 +66,7 @@ protected DoNothingVisitor(Session s) {
session = s;
}
- protected void report(Exception e, String message) {
+ protected static void report(Exception e, String message) {
Review Comment:
unrelated to the task at hand. please move that to a general
cleanup.improvmeent PR.
##########
src/test/java/org/apache/sling/jcr/repoinit/GeneralAclTest.java:
##########
@@ -270,9 +270,9 @@ private void verifyRegisterNamespace(String username,
String prefix, String uri,
try {
userSession = U.loginService(username);
userSession.getWorkspace().getNamespaceRegistry().registerNamespace(prefix,
uri);
- assertTrue("Register namespace succeeded " + prefix + uri,
successExpected);
+ assertTrue("Register namespace succeeded " + prefix + " " + uri,
successExpected);
} catch (RepositoryException e) {
- assertTrue("Error registering namespace " + prefix + uri + " " +
e.getMessage(), !successExpected);
+ assertFalse("Error registering namespace " + prefix + " " + uri +
" " + e.getMessage(), successExpected);
Review Comment:
???? what does the changes for SLING-12115 lead to a changed assertion in a
namespace related test? that doesn't look good at all.
##########
src/main/java/org/apache/sling/jcr/repoinit/impl/AclUtil.java:
##########
@@ -420,7 +419,7 @@ private static boolean isValidPath(@NotNull Session
session, @Nullable String jc
/**
*
* @param acMgr the access control manager
- * @param principal the principal
+ * @param path the path
Review Comment:
is that change related?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
