Merge pull request #29 from apache/SLING-11461

SLING-11461 : Reduce complexity
diff --git a/src/main/java/org/apache/sling/jcr/resource/internal/JcrListenerBaseConfig.java b/src/main/java/org/apache/sling/jcr/resource/internal/JcrListenerBaseConfig.java
index e2040b2..88f7978 100644
--- a/src/main/java/org/apache/sling/jcr/resource/internal/JcrListenerBaseConfig.java
+++ b/src/main/java/org/apache/sling/jcr/resource/internal/JcrListenerBaseConfig.java
@@ -74,43 +74,12 @@
      * @param config The configuration
      * @throws RepositoryException If registration fails.
      */
-    public void register(final EventListener listener, final ObserverConfiguration config) throws RepositoryException {
+    public void register(final @NotNull EventListener listener, final @NotNull ObserverConfiguration config) throws RepositoryException {
         final ObservationManager mgr = this.session.getWorkspace().getObservationManager();
         if (mgr instanceof JackrabbitObservationManager) {
             final OakEventFilter filter = FilterFactory.wrap(new JackrabbitEventFilter());
             // paths
-            final Set<String> paths = config.getPaths().toStringSet();
-            int globCount = 0;
-            int pathCount = 0;
-            for (final String p : paths) {
-                if (p.startsWith(Path.GLOB_PREFIX)) {
-                    globCount++;
-                } else {
-                    pathCount++;
-                }
-            }
-            final String[] pathArray = pathCount > 0 ? new String[pathCount] : null;
-            final String[] globArray = globCount > 0 ? new String[globCount] : null;
-            pathCount = 0;
-            globCount = 0;
-
-            // create arrays and remove global prefix
-            for (final String p : paths) {
-                if (p.startsWith(Path.GLOB_PREFIX)) {
-                    globArray[globCount] = p.substring(Path.GLOB_PREFIX.length());
-                    globCount++;
-                } else {
-                    pathArray[pathCount] = p;
-                    pathCount++;
-                }
-            }
-            if (globArray != null) {
-                filter.withIncludeGlobPaths(globArray);
-            }
-            if (pathArray != null) {
-                filter.setAdditionalPaths(pathArray);
-            }
-            filter.setIsDeep(true);
+            setFilterPaths(filter, config);
 
             // exclude paths
             final Set<String> excludePaths = config.getExcludedPaths().toStringSet();
@@ -118,6 +87,8 @@
                 filter.setExcludedPaths(excludePaths.toArray(new String[0]));
             }
 
+            filter.setIsDeep(true);
+
             // external
             filter.setNoExternal(!config.includeExternal());
 
@@ -136,13 +107,47 @@
         }
 
     }
+    
+    private static void setFilterPaths(@NotNull OakEventFilter filter, @NotNull ObserverConfiguration config) {
+        final Set<String> paths = config.getPaths().toStringSet();
+        int globCount = 0;
+        int pathCount = 0;
+        for (final String p : paths) {
+            if (p.startsWith(Path.GLOB_PREFIX)) {
+                globCount++;
+            } else {
+                pathCount++;
+            }
+        }
+        final String[] pathArray = pathCount > 0 ? new String[pathCount] : null;
+        final String[] globArray = globCount > 0 ? new String[globCount] : null;
+        pathCount = 0;
+        globCount = 0;
+
+        // create arrays and remove global prefix
+        for (final String p : paths) {
+            if (p.startsWith(Path.GLOB_PREFIX) && globArray != null) {
+                globArray[globCount] = p.substring(Path.GLOB_PREFIX.length());
+                globCount++;
+            } else if (pathArray != null) {
+                pathArray[pathCount] = p;
+                pathCount++;
+            }
+        }
+        if (globArray != null) {
+            filter.withIncludeGlobPaths(globArray);
+        }
+        if (pathArray != null) {
+            filter.setAdditionalPaths(pathArray);
+        }
+    }
 
     /**
      * Get the event types based on the configuration
      * @param c The configuration
      * @return The event type mask
      */
-    private static int getTypes(final ObserverConfiguration c) {
+    private static int getTypes(final @NotNull ObserverConfiguration c) {
         int result = 0;
         for (ChangeType t : c.getChangeTypes()) {
             switch (t) {
diff --git a/src/main/java/org/apache/sling/jcr/resource/internal/JcrValueMap.java b/src/main/java/org/apache/sling/jcr/resource/internal/JcrValueMap.java
index ba9abc9..f435770 100644
--- a/src/main/java/org/apache/sling/jcr/resource/internal/JcrValueMap.java
+++ b/src/main/java/org/apache/sling/jcr/resource/internal/JcrValueMap.java
@@ -274,45 +274,7 @@
         }
         // if the name is a path, we should handle this differently
         if (name.indexOf('/') != -1) {
-            // first a compatibility check with the old (wrong) ISO9075
-            // encoding
-            final String path = ISO9075.encodePath(name);
-            try {
-                Property property = NodeUtil.getPropertyOrNull(node, path);
-                if (property != null) {
-                    return new JcrPropertyMapCacheEntry(property);
-                }
-            } catch (final RepositoryException re) {
-                throw new IllegalArgumentException(re);
-            }
-            // now we do a proper segment by segment encoding
-            final StringBuilder sb = new StringBuilder();
-            int pos = 0;
-            int lastPos = -1;
-            while (pos < name.length()) {
-                if (name.charAt(pos) == '/') {
-                    if (lastPos + 1 < pos) {
-                        sb.append(Text.escapeIllegalJcrChars(name.substring(lastPos + 1, pos)));
-                    }
-                    sb.append('/');
-                    lastPos = pos;
-                }
-                pos++;
-            }
-            if (lastPos + 1 < pos) {
-                sb.append(Text.escapeIllegalJcrChars(name.substring(lastPos + 1)));
-            }
-            final String newPath = sb.toString();
-            try {
-                Property property = NodeUtil.getPropertyOrNull(node,newPath);
-                if (property != null) {
-                    return new JcrPropertyMapCacheEntry(property);
-                }
-            } catch (final RepositoryException re) {
-                throw new IllegalArgumentException(re);
-            }
-
-            return null;
+            return readPath(name);
         }
 
         // check cache
@@ -330,20 +292,47 @@
         } catch (final RepositoryException re) {
             throw new IllegalArgumentException(re);
         }
+        return null;
+    }
 
+    private @Nullable JcrPropertyMapCacheEntry readPath(@NotNull String name) {
+        // first a compatibility check with the old (wrong) ISO9075 encoding
+        final String path = ISO9075.encodePath(name);
         try {
-            // for compatibility with older versions we use the (wrong) ISO9075 path
-            // encoding
-            final String oldKey = ISO9075.encodePath(name);
-            Property property = NodeUtil.getPropertyOrNull(node,oldKey);
+            Property property = NodeUtil.getPropertyOrNull(node, path);
             if (property != null) {
-                return cacheProperty(property);
+                return new JcrPropertyMapCacheEntry(property);
             }
         } catch (final RepositoryException re) {
-            // we ignore this
+            throw new IllegalArgumentException(re);
+        }
+        // now we do a proper segment by segment encoding
+        final StringBuilder sb = new StringBuilder();
+        int pos = 0;
+        int lastPos = -1;
+        while (pos < name.length()) {
+            if (name.charAt(pos) == '/') {
+                if (lastPos + 1 < pos) {
+                    sb.append(Text.escapeIllegalJcrChars(name.substring(lastPos + 1, pos)));
+                }
+                sb.append('/');
+                lastPos = pos;
+            }
+            pos++;
+        }
+        if (lastPos + 1 < pos) {
+            sb.append(Text.escapeIllegalJcrChars(name.substring(lastPos + 1)));
+        }
+        final String newPath = sb.toString();
+        try {
+            Property property = NodeUtil.getPropertyOrNull(node,newPath);
+            if (property != null) {
+                return new JcrPropertyMapCacheEntry(property);
+            }
+        } catch (final RepositoryException re) {
+            throw new IllegalArgumentException(re);
         }
 
-        // property not found
         return null;
     }
 
diff --git a/src/main/java/org/apache/sling/jcr/resource/internal/helper/JcrResourceUtil.java b/src/main/java/org/apache/sling/jcr/resource/internal/helper/JcrResourceUtil.java
index 23fbda2..976336d 100644
--- a/src/main/java/org/apache/sling/jcr/resource/internal/helper/JcrResourceUtil.java
+++ b/src/main/java/org/apache/sling/jcr/resource/internal/helper/JcrResourceUtil.java
@@ -82,12 +82,8 @@
                 return value.getDouble();
             case PropertyType.LONG:
                 return value.getLong();
-            case PropertyType.NAME: // fall through
-            case PropertyType.PATH: // fall through
-            case PropertyType.REFERENCE: // fall through
-            case PropertyType.STRING: // fall through
-            case PropertyType.UNDEFINED: // not actually expected
-            default: // not actually expected
+            // all other types (NAME, PATH, REFERENCE, WEAKREFERENCE, STRING, URI, UNDEFINED) fallback to string representation
+            default:
                 return value.getString();
         }
     }
@@ -107,22 +103,7 @@
         if (property.isMultiple()) {
             Value[] values = property.getValues();
             final Object firstValue = values.length > 0 ? toJavaObject(values[0]) : null;
-            final Object[] result;
-            if (firstValue instanceof Boolean) {
-                result = new Boolean[values.length];
-            } else if (firstValue instanceof Calendar) {
-                result = new Calendar[values.length];
-            } else if (firstValue instanceof Double) {
-                result = new Double[values.length];
-            } else if (firstValue instanceof Long) {
-                result = new Long[values.length];
-            } else if (firstValue instanceof BigDecimal) {
-                result = new BigDecimal[values.length];
-            } else if (firstValue instanceof InputStream) {
-                result = new Object[values.length];
-            } else {
-                result = new String[values.length];
-            }
+            final Object[] result = createArray(firstValue, values.length);
             for (int i = 0; i < values.length; i++) {
                 Value value = values[i];
                 if (value != null) {
@@ -135,6 +116,26 @@
         // single value property
         return toJavaObject(property.getValue());
     }
+    
+    private static @NotNull Object[] createArray(@Nullable Object firstValue, int length) {
+        Object[] result;
+        if (firstValue instanceof Boolean) {
+            result = new Boolean[length];
+        } else if (firstValue instanceof Calendar) {
+            result = new Calendar[length];
+        } else if (firstValue instanceof Double) {
+            result = new Double[length];
+        } else if (firstValue instanceof Long) {
+            result = new Long[length];
+        } else if (firstValue instanceof BigDecimal) {
+            result = new BigDecimal[length];
+        } else if (firstValue instanceof InputStream) {
+            result = new Object[length];
+        } else {
+            result = new String[length];
+        }
+        return result;
+    }
 
     /**
      * Creates a {@link javax.jcr.Value JCR Value} for the given object with
diff --git a/src/main/java/org/apache/sling/jcr/resource/internal/helper/jcr/JcrItemResourceFactory.java b/src/main/java/org/apache/sling/jcr/resource/internal/helper/jcr/JcrItemResourceFactory.java
index b2d97f5..4e4c3e7 100644
--- a/src/main/java/org/apache/sling/jcr/resource/internal/helper/jcr/JcrItemResourceFactory.java
+++ b/src/main/java/org/apache/sling/jcr/resource/internal/helper/jcr/JcrItemResourceFactory.java
@@ -64,7 +64,10 @@
      * read-access for the session of this resolver, <code>null</code> is
      * returned.
      *
+     * @param resourceResolver The resource resolver
      * @param resourcePath The absolute path
+     * @param parent The parent resource or {@code null}
+     * @param parameters The parameters or{@code null}
      * @return The <code>Resource</code> for the item at the given path.
      * @throws RepositoryException If an error occurrs accessing checking the
      *             item in the repository.
@@ -82,29 +85,9 @@
         } else {
             version = null;
         }
-
-        Node parentNode = null;
-        String parentResourcePath = null;
-        if (parent != null) {
-            parentNode = parent.adaptTo(Node.class);
-            parentResourcePath = parent.getPath();
-        }
-
-        Item item;
-        if (parentNode != null && resourcePath.startsWith(parentResourcePath)) {
-            String subPath = resourcePath.substring(parentResourcePath.length());
-            if (!subPath.isEmpty() && subPath.charAt(0) == '/') {
-                subPath = subPath.substring(1);
-            }
-            item = getSubitem(parentNode, subPath);
-        } else {
-            item = getItemOrNull(resourcePath);
-        }
-
-        if (item != null && version != null) {
-            item = getHistoricItem(item, version);
-        }
-
+        
+        Item item = getItem(resourcePath, parent, version);
+        
         if (item == null) {
             log.debug("createResource: No JCR Item exists at path '{}'", resourcePath);
             return null;
@@ -121,8 +104,33 @@
             return resource;
         }
     }
+    
+    private @Nullable Item getItem(@NotNull String resourcePath, @Nullable Resource parent, @Nullable String versionSpecifier) throws RepositoryException {
+        Node parentNode = null;
+        String parentResourcePath = null;
+        if (parent != null) {
+            parentNode = parent.adaptTo(Node.class);
+            parentResourcePath = parent.getPath();
+        }
+        
+        Item item;
+        if (parentNode != null && resourcePath.startsWith(parentResourcePath)) {
+            String subPath = resourcePath.substring(parentResourcePath.length());
+            if (!subPath.isEmpty() && subPath.charAt(0) == '/') {
+                subPath = subPath.substring(1);
+            }
+            item = getSubitem(parentNode, subPath);
+        } else {
+            item = getItemOrNull(resourcePath);
+        }
 
-    private Item getHistoricItem(Item item, String versionSpecifier) throws RepositoryException {
+        if (item != null && versionSpecifier != null) {
+            item = getHistoricItem(item, versionSpecifier);
+        }
+        return item;
+    }
+    
+    private @Nullable Item getHistoricItem(Item item, String versionSpecifier) throws RepositoryException {
         Item currentItem = item;
         LinkedList<String> relPath = new LinkedList<>();
         Node version = null;
diff --git a/src/main/java/org/apache/sling/jcr/resource/internal/helper/jcr/JcrNodeResourceMetadata.java b/src/main/java/org/apache/sling/jcr/resource/internal/helper/jcr/JcrNodeResourceMetadata.java
index 0ad1e08..1f52965 100644
--- a/src/main/java/org/apache/sling/jcr/resource/internal/helper/jcr/JcrNodeResourceMetadata.java
+++ b/src/main/java/org/apache/sling/jcr/resource/internal/helper/jcr/JcrNodeResourceMetadata.java
@@ -41,6 +41,7 @@
 import org.apache.sling.api.resource.ResourceMetadata;
 import org.apache.sling.jcr.resource.internal.NodeUtil;
 import org.jetbrains.annotations.NotNull;
+import org.jetbrains.annotations.Nullable;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -106,85 +107,83 @@
             internalPut(CREATION_TIME, creationTime);
             return creationTime;
         } else if (CONTENT_TYPE.equals(key)) {
-            String contentType = null;
-            final Node targetNode = promoteNode();
-            try {
-                Property property = NodeUtil.getPropertyOrNull(targetNode, JCR_MIMETYPE);
-                if (property != null) {
-                    contentType = property.getString();
-                }
-            } catch (final RepositoryException re) {
-                report(re);
-            }
-
+            String contentType = getPropertyString(promoteNode(), JCR_MIMETYPE);
             internalPut(CONTENT_TYPE, contentType);
             return contentType;
         } else if (CHARACTER_ENCODING.equals(key)) {
-            String characterEncoding = null;
-            final Node targetNode = promoteNode();
-            try {
-                Property property = NodeUtil.getPropertyOrNull(targetNode, JCR_ENCODING);
-                if (property != null) {
-                    characterEncoding = property.getString();
-                }
-            } catch (final RepositoryException re) {
-                report(re);
-            }
+            String characterEncoding = getPropertyString(promoteNode(), JCR_ENCODING);
             internalPut(CHARACTER_ENCODING, characterEncoding);
             return characterEncoding;
         } else if (MODIFICATION_TIME.equals(key)) {
-            long modificationTime = -1;
-            final Node targetNode = promoteNode();
-            try {
-                Property prop = NodeUtil.getPropertyOrNull(targetNode, JCR_LAST_MODIFIED);
-                if (prop != null) {
-                    // We don't check node type, so JCR_LASTMODIFIED might not be a long
-                    try {
-                        modificationTime = prop.getLong();
-                    } catch (final ValueFormatException vfe) {
-                        LOGGER.debug("Property {} cannot be converted to a long, ignored ({})",
-                                prop.getPath(), vfe);
-                    }
-                }
-            } catch (final RepositoryException re) {
-                report(re);
-            }
+            long modificationTime = getPropertyLong(promoteNode(), JCR_LAST_MODIFIED);
             internalPut(MODIFICATION_TIME, modificationTime);
             return modificationTime;
         } else if (CONTENT_LENGTH.equals(key)) {
-            long contentLength = -1;
-            final Node targetNode = promoteNode();
-            try {
-                // if the node has a jcr:data property, use that property
-                Property prop = NodeUtil.getPropertyOrNull(targetNode, JCR_DATA);
-                if (prop != null) {
-                    contentLength = JcrItemResource.getContentLength(prop);
-                } else {
-                    // otherwise try to follow default item trail
-                    Item item = getPrimaryItem(targetNode);
-                    while (item != null && item.isNode()) {
-                        item = getPrimaryItem((Node) item);
-                    }
-                    if (item != null) {
-                        final Property data = (Property) item;
-
-                        // set the content length property as a side effect
-                        // for resources which are not nt:file based and whose
-                        // data is not in jcr:content/jcr:data this will lazily
-                        // set the correct content length
-                        contentLength = JcrItemResource.getContentLength(data);
-                    }
-                }
-            } catch (final RepositoryException re) {
-                report(re);
-            }
+            long contentLength = getContentLength(promoteNode());
             internalPut(CONTENT_LENGTH, contentLength);
             return contentLength;
         }
         return null;
     }
 
-    private static Item getPrimaryItem(final Node node) throws RepositoryException {
+    private @Nullable String getPropertyString(@NotNull Node node, @NotNull String propertyName) {
+        try {
+            Property property = NodeUtil.getPropertyOrNull(node, propertyName);
+            if (property != null) {
+                return property.getString();
+            }
+        } catch (final RepositoryException re) {
+            report(re);
+        }
+        return null;
+    }
+
+    private long getPropertyLong(@NotNull Node node, @NotNull String propertyName) {
+        try {
+            Property prop = NodeUtil.getPropertyOrNull(node, propertyName);
+            if (prop != null) {
+                // We don't check node type, so the property might not be of type PropertyType.LONG
+                try {
+                    return prop.getLong();
+                } catch (final ValueFormatException vfe) {
+                    LOGGER.debug("Property {} cannot be converted to a long, ignored ({})", prop.getPath(), vfe);
+                }
+            }
+        } catch (final RepositoryException re) {
+            report(re);
+        }
+        return -1;
+    }
+    
+    private long getContentLength(@NotNull Node node) {
+        try {
+            // if the node has a jcr:data property, use that property
+            Property prop = NodeUtil.getPropertyOrNull(node, JCR_DATA);
+            if (prop != null) {
+                return JcrItemResource.getContentLength(prop);
+            } else {
+                // otherwise try to follow default item trail
+                Item item = getPrimaryItem(node);
+                while (item != null && item.isNode()) {
+                    item = getPrimaryItem((Node) item);
+                }
+                if (item != null) {
+                    final Property data = (Property) item;
+
+                    // set the content length property as a side effect
+                    // for resources which are not nt:file based and whose
+                    // data is not in jcr:content/jcr:data this will lazily
+                    // set the correct content length
+                    return JcrItemResource.getContentLength(data);
+                }
+            }
+        } catch (final RepositoryException re) {
+            report(re);
+        }
+        return -1;
+    }
+    
+    private static @Nullable Item getPrimaryItem(final @NotNull Node node) throws RepositoryException {
         String name = node.getPrimaryNodeType().getPrimaryItemName();
         if (name == null) {
             return null;
diff --git a/src/main/java/org/apache/sling/jcr/resource/internal/helper/jcr/JcrResourceProvider.java b/src/main/java/org/apache/sling/jcr/resource/internal/helper/jcr/JcrResourceProvider.java
index 60b6b5a..2fbe40b 100644
--- a/src/main/java/org/apache/sling/jcr/resource/internal/helper/jcr/JcrResourceProvider.java
+++ b/src/main/java/org/apache/sling/jcr/resource/internal/helper/jcr/JcrResourceProvider.java
@@ -415,33 +415,12 @@
         // check for node type
         final Object nodeObj = (properties != null ? properties.get(JcrConstants.JCR_PRIMARYTYPE) : null);
         // check for sling:resourcetype
-        final String nodeType;
-        if (nodeObj != null) {
-            nodeType = nodeObj.toString();
-        } else {
-            final Object rtObj = (properties != null ? properties.get(JcrResourceConstants.SLING_RESOURCE_TYPE_PROPERTY) : null);
-            boolean isNodeType = false;
-            if (rtObj != null) {
-                final String resourceType = rtObj.toString();
-                if (resourceType.indexOf(':') != -1 && resourceType.indexOf('/') == -1) {
-                    try {
-                        getSession(ctx).getWorkspace().getNodeTypeManager().getNodeType(resourceType);
-                        isNodeType = true;
-                    } catch (final RepositoryException ignore) {
-                        // we expect this, if this isn't a valid node type, therefore ignoring
-                    }
-                }
-            }
-            if (isNodeType) {
-                nodeType = rtObj.toString();
-            } else {
-                nodeType = null;
-            }
-        }
+        final String nodeType = getType(nodeObj, properties, ctx);
+        
         if (path == null) {
             throw new PersistenceException("Unable to create node at " + path, null, path, null);
         }
-        Node node = null;
+        Node node;
         try {
             final int lastPos = path.lastIndexOf('/');
             final Node parent;
@@ -458,27 +437,7 @@
             }
 
             if (properties != null) {
-                // create modifiable map
-                final JcrModifiableValueMap jcrMap = new JcrModifiableValueMap(node, getHelperData(ctx));
-                // check mixin types first
-                final Object value = properties.get(JcrConstants.JCR_MIXINTYPES);
-                if (value != null) {
-                    jcrMap.put(JcrConstants.JCR_MIXINTYPES, value);
-                }
-                for (final Map.Entry<String, Object> entry : properties.entrySet()) {
-                    if (!IGNORED_PROPERTIES.contains(entry.getKey())) {
-                        try {
-                            jcrMap.put(entry.getKey(), entry.getValue());
-                        } catch (final IllegalArgumentException iae) {
-                            try {
-                                node.remove();
-                            } catch (final RepositoryException re) {
-                                // we ignore this
-                            }
-                            throw new PersistenceException(iae.getMessage(), iae, path, entry.getKey());
-                        }
-                    }
-                }
+                populateProperties(node, properties, ctx, path);
             }
 
             return new JcrNodeResource(ctx.getResourceResolver(), path, null, node, getHelperData(ctx));
@@ -486,6 +445,55 @@
             throw new PersistenceException("Unable to create node at " + path, e, path, null);
         }
     }
+    
+    private static @Nullable String getType(@Nullable Object nodeObj, @Nullable Map<String, Object> properties, @NotNull ResolveContext<JcrProviderState> ctx) {
+        if (nodeObj != null) {
+            return nodeObj.toString();
+        }
+
+        final Object rtObj = (properties != null ? properties.get(JcrResourceConstants.SLING_RESOURCE_TYPE_PROPERTY) : null);
+        boolean isNodeType = false;
+        if (rtObj != null) {
+            final String resourceType = rtObj.toString();
+            if (resourceType.indexOf(':') != -1 && resourceType.indexOf('/') == -1) {
+                try {
+                    getSession(ctx).getWorkspace().getNodeTypeManager().getNodeType(resourceType);
+                    isNodeType = true;
+                } catch (final RepositoryException ignore) {
+                    // we expect this, if this isn't a valid node type, therefore ignoring
+                }
+            }
+        }
+        if (isNodeType) {
+            return rtObj.toString();
+        } else {
+            return null;
+        }
+    }
+
+    private static void populateProperties(@NotNull Node node, @NotNull Map<String, Object> properties, @NotNull ResolveContext<JcrProviderState> ctx, @NotNull String path) throws PersistenceException {
+        // create modifiable map
+        final JcrModifiableValueMap jcrMap = new JcrModifiableValueMap(node, getHelperData(ctx));
+        // check mixin types first
+        final Object value = properties.get(JcrConstants.JCR_MIXINTYPES);
+        if (value != null) {
+            jcrMap.put(JcrConstants.JCR_MIXINTYPES, value);
+        }
+        for (final Map.Entry<String, Object> entry : properties.entrySet()) {
+            if (!IGNORED_PROPERTIES.contains(entry.getKey())) {
+                try {
+                    jcrMap.put(entry.getKey(), entry.getValue());
+                } catch (final IllegalArgumentException iae) {
+                    try {
+                        node.remove();
+                    } catch (final RepositoryException re) {
+                        // we ignore this
+                    }
+                    throw new PersistenceException(iae.getMessage(), iae, path, entry.getKey());
+                }
+            }
+        }
+    }
 
     @Override
     public boolean orderBefore(@NotNull ResolveContext<JcrProviderState> ctx, @NotNull Resource parent, @NotNull String name,
@@ -496,34 +504,41 @@
         }
         try {
             // check if reordering necessary
-            NodeIterator nodeIterator = node.getNodes();
-            long existingNodePosition = -1;
-            long index = 0;
-            while (nodeIterator.hasNext()) {
-                Node childNode = nodeIterator.nextNode();
-                if (childNode.getName().equals(name)) {
-                    existingNodePosition = index;
-                }
-                if (existingNodePosition >= 0) {
-                    // is existing resource already at the desired position?
-                    if (childNode.getName().equals(followingSiblingName)) {
-                        if (existingNodePosition == index - 1) {
-                            return false;
-                        }
-                    }
-                    // is the existing node already the last one in the list?
-                    else if (followingSiblingName == null && existingNodePosition == nodeIterator.getSize() - 1) {
-                        return false;
-                    }
-                }
-                index++;
+            if (requiresReorder(node, name, followingSiblingName)) {
+                node.orderBefore(name, followingSiblingName);
+                return true;
             }
-            node.orderBefore(name, followingSiblingName);
-            return true;
+            return false;
         } catch (final RepositoryException e) {
             throw new PersistenceException("Unable to reorder children below " + parent.getPath(), e, parent.getPath(), null);
         }
     }
+    
+    private static boolean requiresReorder(@NotNull Node node, @NotNull String name, @Nullable String followingSiblingName) throws RepositoryException {
+        NodeIterator nodeIterator = node.getNodes();
+        long existingNodePosition = -1;
+        long index = 0;
+        while (nodeIterator.hasNext()) {
+            Node childNode = nodeIterator.nextNode();
+            if (childNode.getName().equals(name)) {
+                existingNodePosition = index;
+            }
+            if (existingNodePosition >= 0) {
+                // is existing resource already at the desired position?
+                if (childNode.getName().equals(followingSiblingName)) {
+                    if (existingNodePosition == index - 1) {
+                        return false;
+                    }
+                }
+                // is the existing node already the last one in the list?
+                else if (followingSiblingName == null && existingNodePosition == nodeIterator.getSize() - 1) {
+                    return false;
+                }
+            }
+            index++;
+        }
+        return true;
+    }
 
     @Override
     public void delete(final @NotNull ResolveContext<JcrProviderState> ctx, final @NotNull Resource resource) throws PersistenceException {