You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sling.apache.org by "jsedding (via GitHub)" <gi...@apache.org> on 2024/05/28 08:42:24 UTC
[PR] SLING-12329 - Backwards compatibility for legacy repoinit statement reordering [sling-org-apache-sling-jcr-repoinit]
jsedding opened a new pull request, #54:
URL: https://github.com/apache/sling-org-apache-sling-jcr-repoinit/pull/54
(no comment)
--
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] SLING-12329 - Backwards compatibility for legacy repoinit statement reordering [sling-org-apache-sling-jcr-repoinit]
Posted by "jsedding (via GitHub)" <gi...@apache.org>.
jsedding merged PR #54:
URL: https://github.com/apache/sling-org-apache-sling-jcr-repoinit/pull/54
--
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] SLING-12329 - Backwards compatibility for legacy repoinit statement reordering [sling-org-apache-sling-jcr-repoinit]
Posted by "jsedding (via GitHub)" <gi...@apache.org>.
jsedding commented on code in PR #54:
URL: https://github.com/apache/sling-org-apache-sling-jcr-repoinit/pull/54#discussion_r1616876096
##########
src/main/java/org/apache/sling/jcr/repoinit/impl/JcrRepoInitOpsProcessorImpl.java:
##########
@@ -38,28 +44,70 @@
})
public class JcrRepoInitOpsProcessorImpl implements JcrRepoInitOpsProcessor {
+ private static final Logger log = LoggerFactory.getLogger(JcrRepoInitOpsProcessorImpl.class);
+
/**
* Apply the supplied operations: first the namespaces and nodetypes
* registrations, then the service users, paths and ACLs.
*/
@Override
public void apply(Session session, List<Operation> ops) {
- Stream.of(
- // register namespaces first
- singleton(new NamespacesVisitor(session)),
- // then create node types and privileges, both use namespaces
- asList(
+ AtomicReference<Operation> lastAttemptedOperation = new AtomicReference<>();
+ try {
+ Stream.of(
+ // register namespaces first
+ singleton(new NamespacesVisitor(session)),
+ // then create node types and privileges, both use namespaces
+ asList(
+ new NodetypesVisitor(session),
+ new PrivilegeVisitor(session)),
+ // finally apply everything else
+ asList(
+ new UserVisitor(session),
+ new NodeVisitor(session),
+ new AclVisitor(session),
+ new GroupMembershipVisitor(session),
+ new NodePropertiesVisitor(session))
+ ).forEach(visitorGroup -> {
+ ops.forEach(op -> {
+ lastAttemptedOperation.set(op);
+ visitorGroup.forEach(op::accept);
+ });
+ });
+ } catch (RepoInitException originalFailure) {
+ // support legacy statement reordering for backwards compatibility
+ try {
+ session.refresh(false); // drop transient changes
+
+ final OperationVisitor[] visitors = {
+ new NamespacesVisitor(session),
new NodetypesVisitor(session),
- new PrivilegeVisitor(session)),
- // finally apply everything else
- asList(
+ new PrivilegeVisitor(session),
new UserVisitor(session),
new NodeVisitor(session),
new AclVisitor(session),
new GroupMembershipVisitor(session),
- new NodePropertiesVisitor(session))
- ).forEach(visitorGroup -> {
- ops.forEach(op -> visitorGroup.forEach(op::accept));
- });
+ new NodePropertiesVisitor(session)
+ };
+
+ for (OperationVisitor v : visitors) {
+ for (Operation op : ops) {
+ op.accept(v);
+ }
+ }
+
+ log.warn("DEPRECATION - The repoinit script being executed relies on a bug causing repoinit " +
Review Comment:
Not sure if SLING-12107 helps. That is the ticket where the "new" ordering was introduced.
We also don't have enough context to explain what the issue is. However, it may be worthwhile including the `originalFailure` exception, or at least its message, in the log message.
--
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] SLING-12329 - Backwards compatibility for legacy repoinit statement reordering [sling-org-apache-sling-jcr-repoinit]
Posted by "jsedding (via GitHub)" <gi...@apache.org>.
jsedding commented on code in PR #54:
URL: https://github.com/apache/sling-org-apache-sling-jcr-repoinit/pull/54#discussion_r1616872112
##########
src/main/java/org/apache/sling/jcr/repoinit/impl/AclUtil.java:
##########
@@ -567,7 +567,7 @@ private static String toString(JackrabbitAccessControlEntry entry) throws Reposi
private static void checkState(boolean expression, String msgPattern, Object... args) {
if (!expression) {
- if (args != null) {
+ if (args == null) {
Review Comment:
Because it was wrong. I noticed the messages with the unsubstituted message patterns while investigating issues caused by the re-ordering. I didn't think this small fix was worth its own JIRA ticket.
--
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] SLING-12329 - Backwards compatibility for legacy repoinit statement reordering [sling-org-apache-sling-jcr-repoinit]
Posted by "joerghoh (via GitHub)" <gi...@apache.org>.
joerghoh commented on code in PR #54:
URL: https://github.com/apache/sling-org-apache-sling-jcr-repoinit/pull/54#discussion_r1616855141
##########
src/main/java/org/apache/sling/jcr/repoinit/impl/AclUtil.java:
##########
@@ -567,7 +567,7 @@ private static String toString(JackrabbitAccessControlEntry entry) throws Reposi
private static void checkState(boolean expression, String msgPattern, Object... args) {
if (!expression) {
- if (args != null) {
+ if (args == null) {
Review Comment:
I don't get why this is changed. Can you please explain?
--
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] SLING-12329 - Backwards compatibility for legacy repoinit statement reordering [sling-org-apache-sling-jcr-repoinit]
Posted by "joerghoh (via GitHub)" <gi...@apache.org>.
joerghoh commented on code in PR #54:
URL: https://github.com/apache/sling-org-apache-sling-jcr-repoinit/pull/54#discussion_r1616847657
##########
src/main/java/org/apache/sling/jcr/repoinit/impl/JcrRepoInitOpsProcessorImpl.java:
##########
@@ -38,28 +44,70 @@
})
public class JcrRepoInitOpsProcessorImpl implements JcrRepoInitOpsProcessor {
+ private static final Logger log = LoggerFactory.getLogger(JcrRepoInitOpsProcessorImpl.class);
+
/**
* Apply the supplied operations: first the namespaces and nodetypes
* registrations, then the service users, paths and ACLs.
*/
@Override
public void apply(Session session, List<Operation> ops) {
- Stream.of(
- // register namespaces first
- singleton(new NamespacesVisitor(session)),
- // then create node types and privileges, both use namespaces
- asList(
+ AtomicReference<Operation> lastAttemptedOperation = new AtomicReference<>();
+ try {
+ Stream.of(
+ // register namespaces first
+ singleton(new NamespacesVisitor(session)),
+ // then create node types and privileges, both use namespaces
+ asList(
+ new NodetypesVisitor(session),
+ new PrivilegeVisitor(session)),
+ // finally apply everything else
+ asList(
+ new UserVisitor(session),
+ new NodeVisitor(session),
+ new AclVisitor(session),
+ new GroupMembershipVisitor(session),
+ new NodePropertiesVisitor(session))
+ ).forEach(visitorGroup -> {
+ ops.forEach(op -> {
+ lastAttemptedOperation.set(op);
+ visitorGroup.forEach(op::accept);
+ });
+ });
+ } catch (RepoInitException originalFailure) {
+ // support legacy statement reordering for backwards compatibility
+ try {
Review Comment:
can you please extract this complete section into a dedicated method?
##########
src/main/java/org/apache/sling/jcr/repoinit/impl/JcrRepoInitOpsProcessorImpl.java:
##########
@@ -38,28 +44,70 @@
})
public class JcrRepoInitOpsProcessorImpl implements JcrRepoInitOpsProcessor {
+ private static final Logger log = LoggerFactory.getLogger(JcrRepoInitOpsProcessorImpl.class);
+
/**
* Apply the supplied operations: first the namespaces and nodetypes
* registrations, then the service users, paths and ACLs.
*/
@Override
public void apply(Session session, List<Operation> ops) {
- Stream.of(
- // register namespaces first
- singleton(new NamespacesVisitor(session)),
- // then create node types and privileges, both use namespaces
- asList(
+ AtomicReference<Operation> lastAttemptedOperation = new AtomicReference<>();
+ try {
+ Stream.of(
+ // register namespaces first
+ singleton(new NamespacesVisitor(session)),
+ // then create node types and privileges, both use namespaces
+ asList(
+ new NodetypesVisitor(session),
+ new PrivilegeVisitor(session)),
+ // finally apply everything else
+ asList(
+ new UserVisitor(session),
+ new NodeVisitor(session),
+ new AclVisitor(session),
+ new GroupMembershipVisitor(session),
+ new NodePropertiesVisitor(session))
+ ).forEach(visitorGroup -> {
+ ops.forEach(op -> {
+ lastAttemptedOperation.set(op);
+ visitorGroup.forEach(op::accept);
+ });
+ });
+ } catch (RepoInitException originalFailure) {
+ // support legacy statement reordering for backwards compatibility
+ try {
+ session.refresh(false); // drop transient changes
+
+ final OperationVisitor[] visitors = {
+ new NamespacesVisitor(session),
new NodetypesVisitor(session),
- new PrivilegeVisitor(session)),
- // finally apply everything else
- asList(
+ new PrivilegeVisitor(session),
new UserVisitor(session),
new NodeVisitor(session),
new AclVisitor(session),
new GroupMembershipVisitor(session),
- new NodePropertiesVisitor(session))
- ).forEach(visitorGroup -> {
- ops.forEach(op -> visitorGroup.forEach(op::accept));
- });
+ new NodePropertiesVisitor(session)
+ };
+
+ for (OperationVisitor v : visitors) {
+ for (Operation op : ops) {
+ op.accept(v);
+ }
+ }
+
+ log.warn("DEPRECATION - The repoinit script being executed relies on a bug causing repoinit " +
Review Comment:
Do we have a Jira ID or something we can reference when it comes to fixing? This warning is hardly actionable for anyone not familiar with repoinit.
--
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] SLING-12329 - Backwards compatibility for legacy repoinit statement reordering [sling-org-apache-sling-jcr-repoinit]
Posted by "jsedding (via GitHub)" <gi...@apache.org>.
jsedding commented on code in PR #54:
URL: https://github.com/apache/sling-org-apache-sling-jcr-repoinit/pull/54#discussion_r1616872388
##########
src/main/java/org/apache/sling/jcr/repoinit/impl/JcrRepoInitOpsProcessorImpl.java:
##########
@@ -38,28 +44,70 @@
})
public class JcrRepoInitOpsProcessorImpl implements JcrRepoInitOpsProcessor {
+ private static final Logger log = LoggerFactory.getLogger(JcrRepoInitOpsProcessorImpl.class);
+
/**
* Apply the supplied operations: first the namespaces and nodetypes
* registrations, then the service users, paths and ACLs.
*/
@Override
public void apply(Session session, List<Operation> ops) {
- Stream.of(
- // register namespaces first
- singleton(new NamespacesVisitor(session)),
- // then create node types and privileges, both use namespaces
- asList(
+ AtomicReference<Operation> lastAttemptedOperation = new AtomicReference<>();
+ try {
+ Stream.of(
+ // register namespaces first
+ singleton(new NamespacesVisitor(session)),
+ // then create node types and privileges, both use namespaces
+ asList(
+ new NodetypesVisitor(session),
+ new PrivilegeVisitor(session)),
+ // finally apply everything else
+ asList(
+ new UserVisitor(session),
+ new NodeVisitor(session),
+ new AclVisitor(session),
+ new GroupMembershipVisitor(session),
+ new NodePropertiesVisitor(session))
+ ).forEach(visitorGroup -> {
+ ops.forEach(op -> {
+ lastAttemptedOperation.set(op);
+ visitorGroup.forEach(op::accept);
+ });
+ });
+ } catch (RepoInitException originalFailure) {
+ // support legacy statement reordering for backwards compatibility
+ try {
Review Comment:
Will do.
--
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org