bdelacretaz commented on a change in pull request #13:
URL:
https://github.com/apache/sling-org-apache-sling-jcr-repoinit/pull/13#discussion_r597564823
##########
File path:
src/main/java/org/apache/sling/jcr/repoinit/impl/NodePropertiesVisitor.java
##########
@@ -70,29 +76,116 @@ private static boolean needToSetProperty(Node n,
PropertyLine line) throws Repos
return(!n.hasProperty(name) || n.getProperty(name) == null);
}
+ /**
+ * True if the property needs to be set - if false, it is not touched. This
+ * handles the "default" repoinit instruction, which means "do not change
the
+ * property if already set"
+ *
+ * @throws RepositoryException
+ * @throws PathNotFoundException
+ */
+ private static boolean needToSetProperty(Authorizable a, String pRelPath,
boolean isDefault) throws RepositoryException {
+ if (!isDefault) {
+ // It's a "set" line -> overwrite existing value if any
+ return true;
+ }
+
+ // Otherwise set the property only if not set yet
+ return(!a.hasProperty(pRelPath) || a.getProperty(pRelPath) == null);
+ }
+
+ /**
+ * Build relative property path from a subtree path and a property name
+ * @param subTreePath the subtree path (may be null or empty)
+ * @param name the property name
+ * @return the relative path of the property
+ */
+ private static String toRelPath(String subTreePath, final String name) {
+ final String pRelPath;
+ if (subTreePath == null || subTreePath.isEmpty()) {
+ pRelPath = name;
+ } else {
+ if (subTreePath.startsWith("/")) {
+ subTreePath = subTreePath.substring(1);
+ }
+ pRelPath = String.format("%s/%s", subTreePath, name);
+ }
+ return pRelPath;
+ }
+
+ /**
+ * Lookup the authorizables for the given ids
+ * @param session the jcr session
+ * @param ids delimited list of authorizable ids
+ * @return iterator over the found authorizables
+ */
+ @NotNull
+ private static Iterable<Authorizable> getAuthorizables(@NotNull Session
session, @NotNull String ids) throws RepositoryException {
+ List<Authorizable> authorizables = new ArrayList<>();
+ for (String id : Text.explode(ids, ID_DELIMINATOR)) {
+ Authorizable a = UserUtil.getAuthorizable(session, id);
+ if (a == null) {
+ throw new PathNotFoundException("Cannot resolve path of
authorizable with id '" + id + "'.");
+ }
+ authorizables.add(a);
+ }
+ return authorizables;
+ }
+
@Override
public void visitSetProperties(SetProperties sp) {
for (String nodePath : sp.getPaths()) {
try {
- log.info("Setting properties on nodePath '{}'", nodePath);
- Node n = session.getNode(nodePath);
- for (PropertyLine pl : sp.getPropertyLines()) {
- final String pName = pl.getPropertyName();
- final PropertyLine.PropertyType pType =
pl.getPropertyType();
- final List<Object> values = pl.getPropertyValues();
- final int type = PropertyType.valueFromName(pType.name());
- if (needToSetProperty(n, pl)) {
- if (values.size() > 1) {
- Value[] pValues = convertToValues(values);
- n.setProperty(pName, pValues, type);
+ if (nodePath.startsWith(PATH_AUTHORIZABLE)) {
+ // special case for setting properties on authorizable
+ int lastHashIndex =
nodePath.lastIndexOf(SUBTREE_DELIMINATOR);
+ if (lastHashIndex == -1) {
+ throw new IllegalStateException("Invalid format of
authorizable path: # deliminator expected.");
+ }
+ String ids =
nodePath.substring(PATH_AUTHORIZABLE.length(), lastHashIndex);
+ String subTreePath = nodePath.substring(lastHashIndex + 1);
+ for (Authorizable a : getAuthorizables(session, ids)) {
+ log.info("Setting properties on authorizable '{}'",
a.getID());
+ for (PropertyLine pl : sp.getPropertyLines()) {
+ final String pName = pl.getPropertyName();
+ final String pRelPath = toRelPath(subTreePath,
pName);
+ final List<Object> values = pl.getPropertyValues();
+ if (needToSetProperty(a, pRelPath,
pl.isDefault())) {
Review comment:
this looks very similar to the code that also uses needToSetProperty a
few lines down, and the many nested if/else are a bit hard to read. Maybe
something can be factored out for readability?
##########
File path:
src/main/java/org/apache/sling/jcr/repoinit/impl/NodePropertiesVisitor.java
##########
@@ -16,31 +16,37 @@
*/
package org.apache.sling.jcr.repoinit.impl;
-import java.util.List;
+import java.util.ArrayList;
import java.util.Calendar;
+import java.util.List;
-import javax.jcr.Session;
import javax.jcr.Node;
import javax.jcr.PathNotFoundException;
-import javax.jcr.Value;
import javax.jcr.PropertyType;
import javax.jcr.RepositoryException;
+import javax.jcr.Session;
+import javax.jcr.Value;
+import org.apache.jackrabbit.api.security.user.Authorizable;
+import org.apache.jackrabbit.util.Text;
import org.apache.jackrabbit.value.BooleanValue;
import org.apache.jackrabbit.value.DateValue;
import org.apache.jackrabbit.value.DoubleValue;
import org.apache.jackrabbit.value.LongValue;
import org.apache.jackrabbit.value.StringValue;
-
import org.apache.sling.repoinit.parser.operations.PropertyLine;
import org.apache.sling.repoinit.parser.operations.SetProperties;
+import org.jetbrains.annotations.NotNull;
/**
* OperationVisitor which processes only operations related to setting node
* properties. Having several such specialized visitors makes it easy to
control
* the execution order.
*/
class NodePropertiesVisitor extends DoNothingVisitor {
+ private static final String PATH_AUTHORIZABLE = ":authorizable:";
Review comment:
Maybe add a comment that the colons are added by the repoinit parser for
an `authorizable(name)` expression, like for `home(name)` and similar notations
which the parser transforms to `:authorizable:name#`.
And I think if we go ahead with this PR we should add a test that uses
`authorizable(name)` to the parser's test suite
([test-34.txt](https://github.com/apache/sling-org-apache-sling-repoinit-parser/blob/master/src/test/resources/testcases/test-34.txt)
and test-99.txt) to expose that syntax there, even if it's transparent for the
parser.
--
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.
For queries about this service, please contact Infrastructure at:
[email protected]
