You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@sling.apache.org by cz...@apache.org on 2012/04/12 14:31:38 UTC
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/
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 {