FELIX-890 Configuration.getProperties() must return a full copy of the
dictionary, that is also arrays and collections must be copied
git-svn-id: https://svn.apache.org/repos/asf/felix/trunk@735651 13f79535-47bb-0310-9956-ffa450edef68
diff --git a/configadmin/src/main/java/org/apache/felix/cm/impl/CaseInsensitiveDictionary.java b/configadmin/src/main/java/org/apache/felix/cm/impl/CaseInsensitiveDictionary.java
index 2cc491a..ee8b6c0 100644
--- a/configadmin/src/main/java/org/apache/felix/cm/impl/CaseInsensitiveDictionary.java
+++ b/configadmin/src/main/java/org/apache/felix/cm/impl/CaseInsensitiveDictionary.java
@@ -19,6 +19,7 @@
package org.apache.felix.cm.impl;
+import java.lang.reflect.Array;
import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
@@ -26,6 +27,8 @@
import java.util.Enumeration;
import java.util.Hashtable;
import java.util.Iterator;
+import java.util.Map;
+import java.util.Vector;
/**
@@ -88,9 +91,43 @@
}
- CaseInsensitiveDictionary( CaseInsensitiveDictionary props )
+ CaseInsensitiveDictionary( CaseInsensitiveDictionary props, boolean deepCopy )
{
- internalMap = new Hashtable( props.internalMap );
+ Hashtable tmp = new Hashtable( Math.max( 2 * props.internalMap.size(), 11 ), 0.75f );
+ if ( deepCopy )
+ {
+ Iterator entries = props.internalMap.entrySet().iterator();
+ while ( entries.hasNext() )
+ {
+ Map.Entry entry = ( Map.Entry ) entries.next();
+ Object value = entry.getValue();
+ if ( value.getClass().isArray() )
+ {
+ // copy array
+ int length = Array.getLength( value );
+ Object newValue = Array.newInstance( value.getClass().getComponentType(), length );
+ System.arraycopy( value, 0, newValue, 0, length );
+ value = newValue;
+ }
+ else if ( value instanceof Collection )
+ {
+ // copy collection, create Vector
+ // a Vector is created because the R4 and R4.1 specs
+ // state that the values must be simple, array or
+ // Vector. And even though we accept Collection nowadays
+ // there might be clients out there still written against
+ // R4 and R4.1 spec expecting Vector
+ value = new Vector( ( Collection ) value );
+ }
+ tmp.put( entry.getKey(), value );
+ }
+ }
+ else
+ {
+ tmp.putAll( props.internalMap );
+ }
+
+ internalMap = tmp;
originalKeys = new Hashtable( props.originalKeys );
}
diff --git a/configadmin/src/main/java/org/apache/felix/cm/impl/ConfigurationAdapter.java b/configadmin/src/main/java/org/apache/felix/cm/impl/ConfigurationAdapter.java
index 6b89a8c..7a4ae55 100644
--- a/configadmin/src/main/java/org/apache/felix/cm/impl/ConfigurationAdapter.java
+++ b/configadmin/src/main/java/org/apache/felix/cm/impl/ConfigurationAdapter.java
@@ -29,7 +29,7 @@
* The <code>ConfigurationAdapter</code> TODO
*
* @author fmeschbe
- * @version $Rev: 527592 $, $Date$
+ * @version $Rev:616219 $, $Date$
*/
public class ConfigurationAdapter implements Configuration
{
@@ -117,7 +117,10 @@
public Dictionary getProperties()
{
checkDeleted();
- return delegatee.getProperties();
+
+ // return a deep copy since the spec says, that modification of
+ // any value should not modify the internal, stored value
+ return delegatee.getProperties( true );
}
diff --git a/configadmin/src/main/java/org/apache/felix/cm/impl/ConfigurationImpl.java b/configadmin/src/main/java/org/apache/felix/cm/impl/ConfigurationImpl.java
index 4b04282..e15f162 100644
--- a/configadmin/src/main/java/org/apache/felix/cm/impl/ConfigurationImpl.java
+++ b/configadmin/src/main/java/org/apache/felix/cm/impl/ConfigurationImpl.java
@@ -223,10 +223,20 @@
}
- /* (non-Javadoc)
- * @see org.osgi.service.cm.Configuration#getProperties()
+ /**
+ * Returns an optionally deep copy of the properties of this configuration
+ * instance.
+ * <p>
+ * This method returns a copy of the internal dictionary. If the
+ * <code>deepCopy</code> parameter is true array and collection values are
+ * copied into new arrays or collections. Otherwise just a new dictionary
+ * referring to the same objects is returned.
+ *
+ * @param deepCopy
+ * <code>true</code> if a deep copy is to be returned.
+ * @return
*/
- public Dictionary getProperties()
+ public Dictionary getProperties( boolean deepCopy )
{
// no properties yet
if ( properties == null )
@@ -234,7 +244,7 @@
return null;
}
- CaseInsensitiveDictionary props = new CaseInsensitiveDictionary( properties );
+ CaseInsensitiveDictionary props = new CaseInsensitiveDictionary( properties, deepCopy );
// fix special properties (pid, factory PID, bundle location)
setAutoProperties( props, false );
@@ -375,7 +385,10 @@
void store() throws IOException
{
- Dictionary props = getProperties();
+ // we don't need a deep copy, since we are not modifying
+ // any value in the dictionary itself. we are just adding
+ // properties to it, which are required for storing
+ Dictionary props = getProperties( false );
// if this is a new configuration, we just use an empty Dictionary
if ( props == null )
diff --git a/configadmin/src/main/java/org/apache/felix/cm/impl/ConfigurationManager.java b/configadmin/src/main/java/org/apache/felix/cm/impl/ConfigurationManager.java
index af74954..fe89c14 100644
--- a/configadmin/src/main/java/org/apache/felix/cm/impl/ConfigurationManager.java
+++ b/configadmin/src/main/java/org/apache/felix/cm/impl/ConfigurationManager.java
@@ -680,7 +680,9 @@
*/
private Dictionary callPlugins( ServiceReference sr, ConfigurationImpl cfg )
{
- Dictionary props = cfg.getProperties();
+ // return a deep copy, since the plugins may tamper with the array
+ // and collection elements, which should not modify the internal data
+ Dictionary props = cfg.getProperties( true );
// guard against NPE for new configuration never updated
if (props == null) {
diff --git a/configadmin/src/test/java/org/apache/felix/cm/MockPersistenceManager.java b/configadmin/src/test/java/org/apache/felix/cm/MockPersistenceManager.java
new file mode 100644
index 0000000..95a4ca1
--- /dev/null
+++ b/configadmin/src/test/java/org/apache/felix/cm/MockPersistenceManager.java
@@ -0,0 +1,71 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.felix.cm;
+
+
+import java.io.IOException;
+import java.util.Collections;
+import java.util.Dictionary;
+import java.util.Enumeration;
+import java.util.HashMap;
+import java.util.Map;
+
+
+public class MockPersistenceManager implements PersistenceManager
+{
+
+ private final Map configs = new HashMap();
+
+
+ public void delete( String pid )
+ {
+ configs.remove( pid );
+ }
+
+
+ public boolean exists( String pid )
+ {
+ return configs.containsKey( pid );
+ }
+
+
+ public Enumeration getDictionaries()
+ {
+ return Collections.enumeration( configs.values() );
+ }
+
+
+ public Dictionary load( String pid ) throws IOException
+ {
+ Dictionary config = ( Dictionary ) configs.get( pid );
+ if ( config != null )
+ {
+ return config;
+ }
+
+ throw new IOException( "No such configuration: " + pid );
+ }
+
+
+ public void store( String pid, Dictionary properties )
+ {
+ configs.put( pid, properties );
+ }
+
+}
diff --git a/configadmin/src/test/java/org/apache/felix/cm/impl/ConfigurationAdapterTest.java b/configadmin/src/test/java/org/apache/felix/cm/impl/ConfigurationAdapterTest.java
new file mode 100644
index 0000000..5d5bb99
--- /dev/null
+++ b/configadmin/src/test/java/org/apache/felix/cm/impl/ConfigurationAdapterTest.java
@@ -0,0 +1,160 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.felix.cm.impl;
+
+
+import java.io.IOException;
+import java.lang.reflect.Array;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Dictionary;
+import java.util.Hashtable;
+
+import junit.framework.TestCase;
+
+import org.apache.felix.cm.MockPersistenceManager;
+import org.apache.felix.cm.PersistenceManager;
+import org.osgi.framework.Constants;
+import org.osgi.service.cm.Configuration;
+
+
+public class ConfigurationAdapterTest extends TestCase
+{
+
+ private static final String SCALAR = "scalar";
+ private static final String STRING_VALUE = "String Value";
+ private static final String STRING_VALUE2 = "Another String Value";
+
+ private static final String ARRAY = "array";
+ private final String[] ARRAY_VALUE;
+
+ private static final String COLLECTION = "collection";
+ private final Collection COLLECTION_VALUE;
+
+ private static final String TEST_PID = "test.pid";
+ private static final String TEST_LOCATION = "test:location";
+
+ private final PersistenceManager pm = new MockPersistenceManager();
+ private final MockConfigurationManager configMgr = new MockConfigurationManager();
+
+ {
+ ARRAY_VALUE = new String[]
+ { STRING_VALUE };
+ COLLECTION_VALUE = new ArrayList();
+ COLLECTION_VALUE.add( STRING_VALUE );
+ }
+
+
+ private Configuration getConfiguration() throws IOException
+ {
+ ConfigurationImpl cimpl = new ConfigurationImpl( configMgr, pm, TEST_PID, null, TEST_LOCATION );
+ return new ConfigurationAdapter( null, cimpl );
+ }
+
+
+ public void testScalar() throws IOException
+ {
+ Configuration cimpl = getConfiguration();
+ Dictionary props = cimpl.getProperties();
+ assertNull( "Configuration is fresh", props );
+
+ props = new Hashtable();
+ props.put( SCALAR, STRING_VALUE );
+ cimpl.update( props );
+
+ Dictionary newProps = cimpl.getProperties();
+ assertNotNull( "Configuration is not fresh", newProps );
+ assertEquals( "Expect 2 elements", 2, newProps.size() );
+ assertEquals( "Service.pid must match", TEST_PID, newProps.get( Constants.SERVICE_PID ) );
+ assertEquals( "Scalar value must match", STRING_VALUE, newProps.get( SCALAR ) );
+ }
+
+
+ public void testArray() throws IOException
+ {
+ Configuration cimpl = getConfiguration();
+
+ Dictionary props = cimpl.getProperties();
+ assertNull( "Configuration is fresh", props );
+
+ props = new Hashtable();
+ props.put( ARRAY, ARRAY_VALUE );
+ cimpl.update( props );
+
+ Dictionary newProps = cimpl.getProperties();
+ assertNotNull( "Configuration is not fresh", newProps );
+ assertEquals( "Expect 2 elements", 2, newProps.size() );
+ assertEquals( "Service.pid must match", TEST_PID, newProps.get( Constants.SERVICE_PID ) );
+
+ Object testProp = newProps.get( ARRAY );
+ assertNotNull( testProp );
+ assertTrue( testProp.getClass().isArray() );
+ assertEquals( 1, Array.getLength( testProp ) );
+ assertEquals( STRING_VALUE, Array.get( testProp, 0 ) );
+
+ // modify the array property
+ Array.set( testProp, 0, STRING_VALUE2 );
+
+ // the array element change must not be reflected in the configuration
+ Dictionary newProps2 = cimpl.getProperties();
+ Object testProp2 = newProps2.get( ARRAY );
+ assertNotNull( testProp2 );
+ assertTrue( testProp2.getClass().isArray() );
+ assertEquals( 1, Array.getLength( testProp2 ) );
+ assertEquals( STRING_VALUE, Array.get( testProp2, 0 ) );
+ }
+
+
+ public void testCollection() throws IOException
+ {
+ Configuration cimpl = getConfiguration();
+
+ Dictionary props = cimpl.getProperties();
+ assertNull( "Configuration is fresh", props );
+
+ props = new Hashtable();
+ props.put( COLLECTION, COLLECTION_VALUE );
+ cimpl.update( props );
+
+ Dictionary newProps = cimpl.getProperties();
+ assertNotNull( "Configuration is not fresh", newProps );
+ assertEquals( "Expect 2 elements", 2, newProps.size() );
+ assertEquals( "Service.pid must match", TEST_PID, newProps.get( Constants.SERVICE_PID ) );
+
+ Object testProp = newProps.get( COLLECTION );
+ assertNotNull( testProp );
+ assertTrue( testProp instanceof Collection );
+ Collection coll = ( Collection ) testProp;
+ assertEquals( 1, coll.size() );
+ assertEquals( STRING_VALUE, coll.iterator().next() );
+
+ // modify the array property
+ coll.clear();
+ coll.add( STRING_VALUE2 );
+
+ // the array element change must not be reflected in the configuration
+ Dictionary newProps2 = cimpl.getProperties();
+ Object testProp2 = newProps2.get( COLLECTION );
+ assertNotNull( testProp2 );
+ assertTrue( testProp2 instanceof Collection );
+ Collection coll2 = ( Collection ) testProp2;
+ assertEquals( 1, coll2.size() );
+ assertEquals( STRING_VALUE, coll2.iterator().next() );
+ }
+}
diff --git a/configadmin/src/test/java/org/apache/felix/cm/impl/MockConfigurationManager.java b/configadmin/src/test/java/org/apache/felix/cm/impl/MockConfigurationManager.java
new file mode 100644
index 0000000..3ec434e
--- /dev/null
+++ b/configadmin/src/test/java/org/apache/felix/cm/impl/MockConfigurationManager.java
@@ -0,0 +1,41 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.felix.cm.impl;
+
+
+public class MockConfigurationManager extends ConfigurationManager
+{
+
+ void updated( ConfigurationImpl config )
+ {
+ // do nothing, might register the update call
+ }
+
+
+ void deleted( ConfigurationImpl config )
+ {
+ // do nothing, might register the update call
+ }
+
+
+ void log( int level, String message, Throwable t )
+ {
+ // no logging for now
+ }
+}