enapps-enorman commented on a change in pull request #13:
URL:
https://github.com/apache/sling-org-apache-sling-jcr-repoinit/pull/13#discussion_r599001729
##########
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:
Well, yes, the logic for determining if a property should be set and
setting that value is similar for authorizable vs node. However, the
differences appear to be in all the important places. Maybe we could try to
extract some common handling by passing around a few callback functions to
handle the those slight differences, but to me that abstraction makes the code
less readable, not more readable.
I never found nested loops and conditional statements difficult to read, but
I will extract those blocks into methods to address your concerns. That should
also resolve the last "code smell" reported by sonar as well.
It seems that the alternative to nested loops and conditions is a extracting
a bunch of small methods that you must jump around the source code to read
rather than having all the logic in one place, so I'm not sold that those
"Cognitive Complexity" calculations from sonar have much real world value.
--
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]
