incubator-sling-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From cziege...@apache.org
Subject svn commit: r1325221 - in /sling/trunk/bundles/jcr/resource/src: main/java/org/apache/sling/jcr/resource/ test/java/org/apache/sling/jcr/resource/internal/
Date Thu, 12 Apr 2012 12:31:38 GMT
Author: cziegeler
Date: Thu Apr 12 12:31:37 2012
New Revision: 1325221

URL: http://svn.apache.org/viewvc?rev=1325221&view=rev
Log:
SLING-2425 : Incorrect and inconsistent escaping of property names used in JcrPropertyMap

Modified:
    sling/trunk/bundles/jcr/resource/src/main/java/org/apache/sling/jcr/resource/JcrModifiablePropertyMap.java
    sling/trunk/bundles/jcr/resource/src/main/java/org/apache/sling/jcr/resource/JcrPropertyMap.java
    sling/trunk/bundles/jcr/resource/src/test/java/org/apache/sling/jcr/resource/internal/JcrPropertyMapTest.java

Modified: sling/trunk/bundles/jcr/resource/src/main/java/org/apache/sling/jcr/resource/JcrModifiablePropertyMap.java
URL: http://svn.apache.org/viewvc/sling/trunk/bundles/jcr/resource/src/main/java/org/apache/sling/jcr/resource/JcrModifiablePropertyMap.java?rev=1325221&r1=1325220&r2=1325221&view=diff
==============================================================================
--- sling/trunk/bundles/jcr/resource/src/main/java/org/apache/sling/jcr/resource/JcrModifiablePropertyMap.java
(original)
+++ sling/trunk/bundles/jcr/resource/src/main/java/org/apache/sling/jcr/resource/JcrModifiablePropertyMap.java
Thu Apr 12 12:31:37 2012
@@ -28,7 +28,7 @@ import javax.jcr.RepositoryException;
 import javax.jcr.Value;
 import javax.jcr.nodetype.NodeType;
 
-import org.apache.jackrabbit.util.ISO9075;
+import org.apache.jackrabbit.util.Text;
 import org.apache.sling.api.resource.PersistableValueMap;
 import org.apache.sling.api.resource.PersistenceException;
 import org.apache.sling.jcr.resource.internal.helper.JcrPropertyMapCacheEntry;
@@ -92,7 +92,7 @@ public final class JcrModifiableProperty
         final Object oldValue = this.get(key);
         try {
             this.cache.put(key, new JcrPropertyMapCacheEntry(value, getNode().getSession()));
-        } catch (RepositoryException re) {
+        } catch (final RepositoryException re) {
             throw new IllegalArgumentException("Value for key " + key + " can't be put into
node: " + value, re);
         }
         this.valueCache.put(key, value);
@@ -129,7 +129,7 @@ public final class JcrModifiableProperty
         if ( this.changedProperties == null ) {
             this.changedProperties = new HashSet<String>();
         }
-        this.changedProperties.add(key.toString());
+        this.changedProperties.add(key);
         return oldValue;
     }
 
@@ -185,10 +185,9 @@ public final class JcrModifiableProperty
         try {
             final Node node = getNode();
             // check for mixin types
-            final String mixinTypesKey = ISO9075.decode(MIXIN_TYPES);
-            if ( this.changedProperties.contains(mixinTypesKey) ) {
-                if ( cache.containsKey(mixinTypesKey) ) {
-                    final JcrPropertyMapCacheEntry entry = cache.get(mixinTypesKey);
+            if ( this.changedProperties.contains(MIXIN_TYPES) ) {
+                if ( cache.containsKey(MIXIN_TYPES) ) {
+                    final JcrPropertyMapCacheEntry entry = cache.get(MIXIN_TYPES);
                     handleMixinTypes(node, entry.values);
                 } else {
                     // remove all mixin types!
@@ -197,7 +196,7 @@ public final class JcrModifiableProperty
             }
 
             for(final String key : this.changedProperties) {
-                final String name = ISO9075.encodePath(key);
+                final String name = Text.escapeIllegalJcrChars(key);
                 if ( !MIXIN_TYPES.equals(name) ) {
                     if ( cache.containsKey(key) ) {
                         final JcrPropertyMapCacheEntry entry = cache.get(key);
@@ -212,7 +211,7 @@ public final class JcrModifiableProperty
                 }
             }
             node.getSession().save();
-        } catch (RepositoryException re) {
+        } catch (final RepositoryException re) {
             throw new PersistenceException("Unable to persist changes.", re);
         }
         this.reset();

Modified: sling/trunk/bundles/jcr/resource/src/main/java/org/apache/sling/jcr/resource/JcrPropertyMap.java
URL: http://svn.apache.org/viewvc/sling/trunk/bundles/jcr/resource/src/main/java/org/apache/sling/jcr/resource/JcrPropertyMap.java?rev=1325221&r1=1325220&r2=1325221&view=diff
==============================================================================
--- sling/trunk/bundles/jcr/resource/src/main/java/org/apache/sling/jcr/resource/JcrPropertyMap.java
(original)
+++ sling/trunk/bundles/jcr/resource/src/main/java/org/apache/sling/jcr/resource/JcrPropertyMap.java
Thu Apr 12 12:31:37 2012
@@ -42,6 +42,7 @@ import javax.jcr.Value;
 import javax.jcr.ValueFormatException;
 
 import org.apache.jackrabbit.util.ISO9075;
+import org.apache.jackrabbit.util.Text;
 import org.apache.sling.api.resource.ValueMap;
 import org.apache.sling.jcr.resource.internal.helper.JcrPropertyMapCacheEntry;
 import org.slf4j.Logger;
@@ -122,10 +123,7 @@ public class JcrPropertyMap
             return (T) get(key);
         }
 
-        JcrPropertyMapCacheEntry entry = cache.get(key);
-        if (entry == null) {
-            entry = read(key);
-        }
+        final JcrPropertyMapCacheEntry entry = this.read(key);
         if ( entry == null ) {
             return null;
         }
@@ -161,10 +159,7 @@ public class JcrPropertyMap
      */
     public Object get(final Object aKey) {
         final String key = checkKey(aKey.toString());
-        JcrPropertyMapCacheEntry entry = cache.get(key);
-        if (entry == null) {
-            entry = read(key);
-        }
+        final JcrPropertyMapCacheEntry entry = this.read(key);
         final Object value = (entry == null ? null : entry.getDefaultValueOrNull());
         return value;
     }
@@ -172,14 +167,14 @@ public class JcrPropertyMap
     /**
      * @see java.util.Map#containsKey(java.lang.Object)
      */
-    public boolean containsKey(Object key) {
+    public boolean containsKey(final Object key) {
         return get(key) != null;
     }
 
     /**
      * @see java.util.Map#containsValue(java.lang.Object)
      */
-    public boolean containsValue(Object value) {
+    public boolean containsValue(final Object value) {
         readFully();
         return valueCache.containsValue(value);
     }
@@ -245,62 +240,138 @@ public class JcrPropertyMap
     public String getPath() {
         try {
             return node.getPath();
-        } catch (RepositoryException e) {
+        } catch (final RepositoryException e) {
             throw new IllegalStateException(e);
         }
     }
 
     // ---------- Helpers to access the node's property ------------------------
 
-    JcrPropertyMapCacheEntry read(final String key) {
-
-        // if the node has been completely read, we need not check
-        // again if the key does not point to a sub node
-        if (fullyRead && key.indexOf('/') == -1 ) {
-            // except if the key contains
-            return null;
-        }
-
-        final String name = ISO9075.encodePath(key);
+    /**
+     * Put a single property into the cache
+     * @param prop
+     * @return
+     * @throws IllegalArgumentException if a repository exception occurs
+     */
+    private JcrPropertyMapCacheEntry cacheProperty(final Property prop) {
         try {
-            if (node.hasProperty(name)) {
-                final Property prop = node.getProperty(name);
-                final JcrPropertyMapCacheEntry entry = new JcrPropertyMapCacheEntry(prop);
+            // calculate the key
+            final String name = prop.getName();
+            String key = null;
+            if ( name.indexOf("_x") != -1 ) {
+                // for compatiblity with older versions we use the (wrong)
+                // ISO9075 path encoding
+                key = ISO9075.decode(name);
+                if ( key.equals(name) ) {
+                    key = null;
+                }
+            }
+            if ( key == null ) {
+                key = Text.unescapeIllegalJcrChars(name);
+            }
+            JcrPropertyMapCacheEntry entry = cache.get(key);
+            if ( entry == null ) {
+                entry = new JcrPropertyMapCacheEntry(prop);
                 cache.put(key, entry);
-                Object defaultValue = entry.getDefaultValue();
+
+                final Object defaultValue = entry.getDefaultValue();
                 if (defaultValue != null) {
                     valueCache.put(key, entry.getDefaultValue());
                 }
-                return entry;
             }
-        } catch (RepositoryException re) {
-            // TODO: log !!
+            return entry;
+        } catch (final RepositoryException re) {
+            throw new IllegalArgumentException(re);
         }
+    }
 
-        // property not found or some error accessing it
+    /**
+     * Read a single property.
+     * @throws IllegalArgumentException if a repository exception occurs
+     */
+    JcrPropertyMapCacheEntry read(final String name) {
+        // 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 {
+                if ( node.hasProperty(path) ) {
+                    return new JcrPropertyMapCacheEntry(node.getProperty(path));
+                }
+            } 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 {
+                if ( node.hasProperty(newPath) ) {
+                    return new JcrPropertyMapCacheEntry(node.getProperty(newPath));
+                }
+            } catch (final RepositoryException re) {
+                throw new IllegalArgumentException(re);
+            }
+
+            return null;
+        }
+
+        // if the node has been completely read we can directly return
+        if ( fullyRead ) {
+            return cache.get(name);
+        }
+
+        final String key = Text.escapeIllegalJcrChars(name);
+        try {
+            if (node.hasProperty(key)) {
+                final Property prop = node.getProperty(key);
+                return cacheProperty(prop);
+            }
+            // for compatiblity with older versions we use the (wrong) ISO9075 path
+            // encoding
+            final String oldKey = ISO9075.encodePath(name);
+            if (node.hasProperty(oldKey)) {
+                final Property prop = node.getProperty(oldKey);
+                return cacheProperty(prop);
+            }
+        } catch (final RepositoryException re) {
+            throw new IllegalArgumentException(re);
+        }
+
+        // property not found
         return null;
     }
 
+    /**
+     * Read all properties.
+     * @throws IllegalArgumentException if a repository exception occurs
+     */
     void readFully() {
         if (!fullyRead) {
             try {
-                PropertyIterator pi = node.getProperties();
+                final PropertyIterator pi = node.getProperties();
                 while (pi.hasNext()) {
-                    Property prop = pi.nextProperty();
-                    final String name = prop.getName();
-                    final String key = ISO9075.decode(name);
-                    if (!cache.containsKey(key)) {
-                        final JcrPropertyMapCacheEntry entry = new JcrPropertyMapCacheEntry(prop);
-                        cache.put(key, entry);
-                        Object defaultValue = entry.getDefaultValue();
-                        if (defaultValue != null) {
-                            valueCache.put(key, entry.getDefaultValue());
-                        }
-                    }
+                    final Property prop = pi.nextProperty();
+                    this.cacheProperty(prop);
                 }
                 fullyRead = true;
-            } catch (RepositoryException re) {
-                // TODO: log !!
+            } catch (final RepositoryException re) {
+                throw new IllegalArgumentException(re);
             }
         }
     }
@@ -482,13 +553,13 @@ public class JcrPropertyMap
         }
         return type;
     }
-    
+
 	private Map<String, Object> transformEntries( Map<String, JcrPropertyMapCacheEntry>
map) {
-		
+
 		Map<String, Object> transformedEntries = new LinkedHashMap<String, Object>(map.size());
 		for ( Map.Entry<String, JcrPropertyMapCacheEntry> entry : map.entrySet() )
 			transformedEntries.put(entry.getKey(), entry.getValue().getDefaultValueOrNull());
-		
+
 		return transformedEntries;
 	}
 

Modified: sling/trunk/bundles/jcr/resource/src/test/java/org/apache/sling/jcr/resource/internal/JcrPropertyMapTest.java
URL: http://svn.apache.org/viewvc/sling/trunk/bundles/jcr/resource/src/test/java/org/apache/sling/jcr/resource/internal/JcrPropertyMapTest.java?rev=1325221&r1=1325220&r2=1325221&view=diff
==============================================================================
--- sling/trunk/bundles/jcr/resource/src/test/java/org/apache/sling/jcr/resource/internal/JcrPropertyMapTest.java
(original)
+++ sling/trunk/bundles/jcr/resource/src/test/java/org/apache/sling/jcr/resource/internal/JcrPropertyMapTest.java
Thu Apr 12 12:31:37 2012
@@ -33,6 +33,7 @@ import javax.jcr.ValueFactory;
 
 import org.apache.commons.io.IOUtils;
 import org.apache.jackrabbit.util.ISO9075;
+import org.apache.jackrabbit.util.Text;
 import org.apache.sling.api.resource.ValueMap;
 import org.apache.sling.commons.testing.jcr.RepositoryTestBase;
 import org.apache.sling.jcr.resource.JcrPropertyMap;
@@ -166,21 +167,21 @@ public class JcrPropertyMapTest extends 
         result = map.get(PROP_NAME_NIL, defaultValue);
         assertSame(defaultValue, result);
     }
-    
+
     public void testInputStream() throws Exception {
         InputStream instream = new ByteArrayInputStream("this too shall pass".getBytes());
-        
+
         ValueFactory valueFactory = rootNode.getSession().getValueFactory();
 
         rootNode.setProperty("bin", valueFactory.createBinary(instream));
         rootNode.getSession().save();
-        
+
         ValueMap map = new JcrPropertyMap(rootNode);
         instream = map.get("bin", InputStream.class);
         assertNotNull(instream);
         String read = IOUtils.toString(instream);
         assertEquals("Stream read successfully", "this too shall pass", read);
-        
+
         instream = map.get("bin", InputStream.class);
         assertNotNull(instream);
         read = IOUtils.toString(instream);
@@ -281,18 +282,48 @@ public class JcrPropertyMapTest extends 
 
     private static final String TEST_PATH = "a<a";
 
+    private static final String VALUE = "value";
+    private static final String VALUE1 = "value";
+    private static final String VALUE2 = "value";
+    private static final String PROP1 = "-prop";
+    private static final String PROP2 = "1prop";
+
     public void testNames() throws Exception {
-        this.rootNode.setProperty(ISO9075.encodePath(TEST_PATH), "value");
+        this.rootNode.setProperty(Text.escapeIllegalJcrChars(TEST_PATH), VALUE);
+        this.rootNode.setProperty(PROP1, VALUE1);
+        this.rootNode.setProperty(PROP2, VALUE2);
         final ValueMap vm = this.createPropertyMap(this.rootNode);
-        assertEquals("value", vm.get(TEST_PATH));
+        assertEquals(VALUE, vm.get(TEST_PATH));
+        assertEquals(VALUE1, vm.get(PROP1));
+        assertEquals(VALUE2, vm.get(PROP2));
     }
 
     public void testIerators() throws Exception {
-        this.rootNode.setProperty(ISO9075.encodePath(TEST_PATH), "value");
+        this.rootNode.setProperty(Text.escapeIllegalJcrChars(TEST_PATH), VALUE);
+        this.rootNode.setProperty(PROP1, VALUE1);
+        this.rootNode.setProperty(PROP2, VALUE2);
+        final ValueMap vm = this.createPropertyMap(this.rootNode);
+        assertTrue(vm.containsKey(TEST_PATH));
+        search(vm.keySet().iterator(), TEST_PATH);
+        search(vm.keySet().iterator(), PROP1);
+        search(vm.keySet().iterator(), PROP2);
+        search(vm.values().iterator(), VALUE);
+        search(vm.values().iterator(), VALUE1);
+        search(vm.values().iterator(), VALUE2);
+    }
+
+    public void testNamesOld() throws Exception {
+        this.rootNode.setProperty(ISO9075.encodePath(TEST_PATH), VALUE);
+        final ValueMap vm = this.createPropertyMap(this.rootNode);
+        assertEquals(VALUE, vm.get(TEST_PATH));
+    }
+
+    public void testIeratorsOld() throws Exception {
+        this.rootNode.setProperty(ISO9075.encodePath(TEST_PATH), VALUE);
         final ValueMap vm = this.createPropertyMap(this.rootNode);
         assertTrue(vm.containsKey(TEST_PATH));
         search(vm.keySet().iterator(), TEST_PATH);
-        search(vm.values().iterator(), "value");
+        search(vm.values().iterator(), VALUE);
     }
 
     public void testDotSlash() throws Exception {



Mime
View raw message