FELIX-3087 Fix wrong Character/Char type conversion; add unit tests; refactor value conversion to do it during validation, which allows to fail value conversion on a component-level instead of a descriptor file level.
git-svn-id: https://svn.apache.org/repos/asf/felix/trunk@1180605 13f79535-47bb-0310-9956-ffa450edef68
diff --git a/scr/src/main/java/org/apache/felix/scr/impl/metadata/PropertyMetadata.java b/scr/src/main/java/org/apache/felix/scr/impl/metadata/PropertyMetadata.java
index a127d70..bf4c5b4 100644
--- a/scr/src/main/java/org/apache/felix/scr/impl/metadata/PropertyMetadata.java
+++ b/scr/src/main/java/org/apache/felix/scr/impl/metadata/PropertyMetadata.java
@@ -33,9 +33,11 @@
private String m_name;
// Type of the property (optional)
- private String m_type = "String";
+ private String m_type;
// Value of the type (optional)
+ // - before validate: raw value from XML (String or String[])
+ // - after validate: converted value provided to component
private Object m_value;
// Flag that indicates if this PropertyMetadata has been validated and thus has become immutable
@@ -76,8 +78,7 @@
if (m_validated == true) {
return;
}
-
- m_value = toType( value );
+ m_value = value;
}
/**
@@ -87,81 +88,19 @@
* @param values
*/
public void setValues(String values) {
- // splite the values and convert to boxed objects
+ if (m_validated == true) {
+ return;
+ }
+ // splite th values
List valueList = new ArrayList();
StringTokenizer tokener = new StringTokenizer(values, "\r\n");
while (tokener.hasMoreTokens()) {
String value = tokener.nextToken().trim();
if (value.length() > 0) {
- valueList.add(toType( value ));
+ valueList.add( value );
}
}
-
- // 112.4.5 Except for String objects, the result will be translated to an array of primitive types.
- if(m_type.equals("String")) {
- m_value = valueList.toArray( new String[valueList.size()] );
- }
- else if(m_type.equals("Long")) {
- long[] array = new long[valueList.size()];
- for (int i=0; i < array.length; i++) {
- array[i] = ((Long) valueList.get(i)).longValue();
- }
- m_value = array;
- }
- else if(m_type.equals("Double")) {
- double[] array = new double[valueList.size()];
- for (int i=0; i < array.length; i++) {
- array[i] = ((Double) valueList.get(i)).doubleValue();
- }
- m_value = array;
- }
- else if(m_type.equals("Float")) {
- float[] array = new float[valueList.size()];
- for (int i=0; i < array.length; i++) {
- array[i] = ((Float) valueList.get(i)).floatValue();
- }
- m_value = array;
- }
- else if(m_type.equals("Integer")) {
- int[] array = new int[valueList.size()];
- for (int i=0; i < array.length; i++) {
- array[i] = ((Integer) valueList.get(i)).intValue();
- }
- m_value = array;
- }
- else if(m_type.equals("Byte")) {
- byte[] array = new byte[valueList.size()];
- for (int i=0; i < array.length; i++) {
- array[i] = ((Byte) valueList.get(i)).byteValue();
- }
- m_value = array;
- }
- else if ( m_type.equals( "Char" ) || m_type.equals( "Character" ) ) {
- // DS 1.1 changes the "Char" type to "Character", here we support both
- //TODO: verify if this is adequate for Characters
- char[] array = new char[valueList.size()];
- for (int i=0; i < array.length; i++) {
- array[i] = ((Character) valueList.get(i)).charValue();
- }
- m_value = array;
- }
- else if(m_type.equals("Boolean")) {
- boolean[] array = new boolean[valueList.size()];
- for (int i=0; i < array.length; i++) {
- array[i] = ((Boolean) valueList.get(i)).booleanValue();
- }
- m_value = array;
- }
- else if(m_type.equals("Short")) {
- short[] array = new short[valueList.size()];
- for (int i=0; i < array.length; i++) {
- array[i] = ((Short) valueList.get(i)).shortValue();
- }
- m_value = array;
- }
- else {
- throw new IllegalArgumentException("Undefined property type '"+m_type+"'");
- }
+ m_value = valueList.toArray( new String[valueList.size()] );
}
/**
@@ -202,7 +141,11 @@
}
// check character type name
- if ( componentMetadata.isDS11() && m_type.equals( "Char" ) )
+ if ( m_type == null )
+ {
+ m_type = "String";
+ }
+ else if ( componentMetadata.isDS11() && m_type.equals( "Char" ) )
{
throw componentMetadata
.validationFailure( "Illegal property type 'Char' used for DS 1.1 descriptor, use 'Character' instead" );
@@ -212,16 +155,49 @@
throw componentMetadata
.validationFailure( "Illegal property type 'Character' used for DS 1.0 descriptor, use 'Char' instead" );
}
+
+ // validate and covert value
+ if ( m_value != null )
+ {
+ try
+ {
+ if ( m_value instanceof String )
+ {
+ m_value = toType( ( String ) m_value );
+ }
+ else
+ {
+ m_value = toTypeArray( ( String[] ) m_value );
+ }
+ }
+ catch ( NumberFormatException nfe )
+ {
+ throw componentMetadata.validationFailure( getName() + ": Cannot convert property value to "
+ + getType() );
+ }
+ catch ( IllegalArgumentException e )
+ {
+ throw componentMetadata.validationFailure( getName() + ": " + e.getMessage() );
+ }
+ }
+
+ m_validated = true;
}
+ /**
+ * @throws IllegalArgumentException if the property type is not valid
+ * according to the spec
+ * @throws NumberFormatException if the string value cannot be converted
+ * to the numeric type indicated by the property type
+ */
private Object toType( String value )
{
// 112.4.5 Parsing of the value is done by the valueOf(String) method (P. 291)
// Should the type accept lowercase too?
if ( m_type.equals( "String" ) )
{
- return String.valueOf( value );
+ return value;
}
else if ( m_type.equals( "Long" ) )
{
@@ -246,8 +222,10 @@
else if ( m_type.equals( "Char" ) || m_type.equals( "Character" ) )
{
// DS 1.1 changes the "Char" type to "Character", here we support both
- char c = ( value.length() > 0 ) ? value.charAt( 0 ) : 0;
- return new Character( c );
+ // For Character types, the conversion is handled by Integer.valueOf method.
+ // (since valueOf is defined in terms of parseInt we directly call
+ // parseInt to prevent unneeded Object creation)
+ return new Character( ( char ) Integer.parseInt( value ) );
}
else if ( m_type.equals( "Boolean" ) )
{
@@ -262,4 +240,97 @@
throw new IllegalArgumentException( "Undefined property type '" + m_type + "'" );
}
}
+
+
+ /**
+ * @throws IllegalArgumentException if the property type is not valid
+ * according to the spec
+ * @throws NumberFormatException if the string value cannot be converted
+ * to the numeric type indicated by the property type
+ */
+ private Object toTypeArray( String[] valueList )
+ {
+ // 112.4.5 Except for String objects, the result will be translated to an array of primitive types.
+ if ( m_type.equals( "String" ) )
+ {
+ return valueList;
+ }
+ else if ( m_type.equals( "Double" ) )
+ {
+ double[] array = new double[valueList.length];
+ for ( int i = 0; i < array.length; i++ )
+ {
+ array[i] = Double.parseDouble( valueList[i] );
+ }
+ return array;
+ }
+ else if ( m_type.equals( "Float" ) )
+ {
+ float[] array = new float[valueList.length];
+ for ( int i = 0; i < array.length; i++ )
+ {
+ array[i] = Float.parseFloat( valueList[i] );
+ }
+ return array;
+ }
+ else if ( m_type.equals( "Long" ) )
+ {
+ long[] array = new long[valueList.length];
+ for ( int i = 0; i < array.length; i++ )
+ {
+ array[i] = Long.parseLong( valueList[i] );
+ }
+ return array;
+ }
+ else if ( m_type.equals( "Integer" ) )
+ {
+ int[] array = new int[valueList.length];
+ for ( int i = 0; i < array.length; i++ )
+ {
+ array[i] = Integer.parseInt( valueList[i] );
+ }
+ return array;
+ }
+ else if ( m_type.equals( "Short" ) )
+ {
+ short[] array = new short[valueList.length];
+ for ( int i = 0; i < array.length; i++ )
+ {
+ array[i] = Short.parseShort( valueList[i] );
+ }
+ return array;
+ }
+ else if ( m_type.equals( "Byte" ) )
+ {
+ byte[] array = new byte[valueList.length];
+ for ( int i = 0; i < array.length; i++ )
+ {
+ array[i] = Byte.parseByte( valueList[i] );
+ }
+ return array;
+ }
+ else if ( m_type.equals( "Char" ) || m_type.equals( "Character" ) )
+ {
+ // DS 1.1 changes the "Char" type to "Character", here we support both
+ char[] array = new char[valueList.length];
+ for ( int i = 0; i < array.length; i++ )
+ {
+ array[i] = ( char ) Integer.parseInt( valueList[i] );
+ }
+ return array;
+ }
+ else if ( m_type.equals( "Boolean" ) )
+ {
+ boolean[] array = new boolean[valueList.length];
+ for ( int i = 0; i < array.length; i++ )
+ {
+ array[i] = Boolean.parseBoolean( valueList[i] );
+ }
+ return array;
+ }
+ else
+ {
+ throw new IllegalArgumentException( "Undefined property type '" + m_type + "'" );
+ }
+ }
}
diff --git a/scr/src/test/java/org/apache/felix/scr/impl/metadata/ComponentMetadataTest.java b/scr/src/test/java/org/apache/felix/scr/impl/metadata/ComponentMetadataTest.java
index f708538..3cd7ff3 100644
--- a/scr/src/test/java/org/apache/felix/scr/impl/metadata/ComponentMetadataTest.java
+++ b/scr/src/test/java/org/apache/felix/scr/impl/metadata/ComponentMetadataTest.java
@@ -19,6 +19,7 @@
package org.apache.felix.scr.impl.metadata;
+import java.lang.reflect.Array;
import junit.framework.TestCase;
import org.apache.felix.scr.impl.MockLogger;
@@ -265,8 +266,8 @@
final ComponentMetadata cm1 = createComponentMetadata11( Boolean.TRUE, null );
cm1.setName( null );
cm1.validate( logger );
- assertEquals( "Expected name to equal implementation class name", cm1.getImplementationClassName(), cm1
- .getName() );
+ assertEquals( "Expected name to equal implementation class name", cm1.getImplementationClassName(),
+ cm1.getName() );
}
@@ -355,8 +356,8 @@
{
final ComponentMetadata cm1 = createComponentMetadata( Boolean.TRUE, null );
cm1.validate( logger );
- assertEquals( "Configuration policy", ComponentMetadata.CONFIGURATION_POLICY_OPTIONAL, cm1
- .getConfigurationPolicy() );
+ assertEquals( "Configuration policy", ComponentMetadata.CONFIGURATION_POLICY_OPTIONAL,
+ cm1.getConfigurationPolicy() );
final ComponentMetadata cm2 = createComponentMetadata( Boolean.TRUE, null );
cm2.setConfigurationPolicy( ComponentMetadata.CONFIGURATION_POLICY_IGNORE );
@@ -380,26 +381,26 @@
{
final ComponentMetadata cm1 = createComponentMetadata11( Boolean.TRUE, null );
cm1.validate( logger );
- assertEquals( "Configuration policy", ComponentMetadata.CONFIGURATION_POLICY_OPTIONAL, cm1
- .getConfigurationPolicy() );
+ assertEquals( "Configuration policy", ComponentMetadata.CONFIGURATION_POLICY_OPTIONAL,
+ cm1.getConfigurationPolicy() );
final ComponentMetadata cm2 = createComponentMetadata11( Boolean.TRUE, null );
cm2.setConfigurationPolicy( ComponentMetadata.CONFIGURATION_POLICY_IGNORE );
cm2.validate( logger );
- assertEquals( "Configuration policy", ComponentMetadata.CONFIGURATION_POLICY_IGNORE, cm2
- .getConfigurationPolicy() );
+ assertEquals( "Configuration policy", ComponentMetadata.CONFIGURATION_POLICY_IGNORE,
+ cm2.getConfigurationPolicy() );
final ComponentMetadata cm3 = createComponentMetadata11( Boolean.TRUE, null );
cm3.setConfigurationPolicy( ComponentMetadata.CONFIGURATION_POLICY_OPTIONAL );
cm3.validate( logger );
- assertEquals( "Configuration policy", ComponentMetadata.CONFIGURATION_POLICY_OPTIONAL, cm3
- .getConfigurationPolicy() );
+ assertEquals( "Configuration policy", ComponentMetadata.CONFIGURATION_POLICY_OPTIONAL,
+ cm3.getConfigurationPolicy() );
final ComponentMetadata cm4 = createComponentMetadata11( Boolean.TRUE, null );
cm4.setConfigurationPolicy( ComponentMetadata.CONFIGURATION_POLICY_REQUIRE );
cm4.validate( logger );
- assertEquals( "Configuration policy", ComponentMetadata.CONFIGURATION_POLICY_REQUIRE, cm4
- .getConfigurationPolicy() );
+ assertEquals( "Configuration policy", ComponentMetadata.CONFIGURATION_POLICY_REQUIRE,
+ cm4.getConfigurationPolicy() );
final ComponentMetadata cm5 = createComponentMetadata11( Boolean.TRUE, null );
cm5.setConfigurationPolicy( "undefined" );
@@ -432,8 +433,8 @@
cm2.addDependency( createReferenceMetadata( "name1" ) );
cm2.addDependency( createReferenceMetadata( "name1" ) );
cm2.validate( logger );
- assertTrue( "Expected warning for duplicate reference name", logger
- .messageContains( "Detected duplicate reference name" ) );
+ assertTrue( "Expected warning for duplicate reference name",
+ logger.messageContains( "Detected duplicate reference name" ) );
}
@@ -476,8 +477,8 @@
// validates fine (though logging a warning) and sets field to null
cm3.validate( logger );
- assertTrue( "Expected warning for unsupported updated method name", logger
- .messageContains( "Ignoring updated method definition" ) );
+ assertTrue( "Expected warning for unsupported updated method name",
+ logger.messageContains( "Ignoring updated method definition" ) );
assertNull( rm3.getUpdated() );
}
@@ -493,8 +494,8 @@
// validates fine (though logging a warning) and sets field to null
cm3.validate( logger );
- assertTrue( "Expected warning for unsupported updated method name", logger
- .messageContains( "Ignoring updated method definition" ) );
+ assertTrue( "Expected warning for unsupported updated method name",
+ logger.messageContains( "Ignoring updated method definition" ) );
assertNull( rm3.getUpdated() );
}
@@ -615,7 +616,7 @@
public void test_property_char_ds10() throws ComponentException
{
final ComponentMetadata cm = createComponentMetadata( null, null );
- PropertyMetadata prop = createPropertyMetadata( "x", "Char", "x" );
+ PropertyMetadata prop = createPropertyMetadata( "x", "Char", Integer.toString( 'x' ) );
cm.addProperty( prop );
cm.validate( logger );
assertTrue( prop.getValue() instanceof Character );
@@ -639,13 +640,55 @@
}
+ public void test_property_non_character()
+ {
+ final ComponentMetadata cm = createComponentMetadata( null, null );
+
+ assertProperty( "String", "Ein String", cm );
+ assertProperty( "Double", new Double( 2.5 ), cm );
+ assertProperty( "Float", new Float( 2.5 ), cm );
+ assertProperty( "Long", new Long( 2 ), cm );
+ assertProperty( "Integer", new Integer( 2 ), cm );
+ assertProperty( "Short", new Short( ( short ) 2 ), cm );
+ assertProperty( "Byte", new Byte( ( byte ) 2 ), cm );
+ assertProperty( "Boolean", Boolean.TRUE, cm );
+
+ assertPropertyFail( "Double", "x", cm );
+ assertPropertyFail( "Float", "x", cm );
+ assertPropertyFail( "Long", "x", cm );
+ assertPropertyFail( "Integer", "x", cm );
+ assertPropertyFail( "Short", "x", cm );
+ assertPropertyFail( "Byte", "x", cm );
+ }
+
+
+ public void test_property_array_non_character()
+ {
+ final ComponentMetadata cm = createComponentMetadata( null, null );
+ assertPropertyArray( "String", "Ein String", cm );
+ assertPropertyArray( "Double", new Double( 2.5 ), cm );
+ assertPropertyArray( "Float", new Float( 2.5 ), cm );
+ assertPropertyArray( "Long", new Long( 2 ), cm );
+ assertPropertyArray( "Integer", new Integer( 2 ), cm );
+ assertPropertyArray( "Short", new Short( ( short ) 2 ), cm );
+ assertPropertyArray( "Byte", new Byte( ( byte ) 2 ), cm );
+ assertPropertyArray( "Boolean", Boolean.TRUE, cm );
+
+ assertPropertyArrayFail( "Double", "x", cm );
+ assertPropertyArrayFail( "Float", "x", cm );
+ assertPropertyArrayFail( "Long", "x", cm );
+ assertPropertyArrayFail( "Integer", "x", cm );
+ assertPropertyArrayFail( "Short", "x", cm );
+ assertPropertyArrayFail( "Byte", "x", cm );
+ }
+
+
public void test_property_character_ds10()
{
final ComponentMetadata cm = createComponentMetadata( null, null );
- cm.addProperty( createPropertyMetadata( "x", "Character", "x" ) );
try
{
- cm.validate( logger );
+ createPropertyMetadata( "x", "Character", Integer.toString( 'x' ) ).validate( cm );
fail( "Expect validation failure for illegal property type Character" );
}
catch ( ComponentException ce )
@@ -658,7 +701,7 @@
public void test_property_character_ds11() throws ComponentException
{
final ComponentMetadata cm = createComponentMetadata11( null, null );
- PropertyMetadata prop = createPropertyMetadata( "x", "Character", "x" );
+ PropertyMetadata prop = createPropertyMetadata( "x", "Character", Integer.toString( 'x' ) );
cm.addProperty( prop );
cm.validate( logger );
assertTrue( prop.getValue() instanceof Character );
@@ -680,11 +723,13 @@
}
catch ( ComponentException ce )
{
- assertTrue( "Expected validation reason to contain '" + expectedValidationReason + "': actual: "
- + ce.getMessage(), ce.getMessage().indexOf( expectedValidationReason ) >= 0 );
+ assertTrue(
+ "Expected validation reason to contain '" + expectedValidationReason + "': actual: " + ce.getMessage(),
+ ce.getMessage().indexOf( expectedValidationReason ) >= 0 );
}
}
+
// Creates Component Metadata for the given namespace
private ComponentMetadata createComponentMetadata( int nameSpaceCode, Boolean immediate, String factory )
{
@@ -702,6 +747,7 @@
return meta;
}
+
// Creates DS 1.0 Component Metadata
private ComponentMetadata createComponentMetadata( Boolean immediate, String factory )
{
@@ -754,4 +800,99 @@
}
return meta;
}
+
+
+ private void assertProperty( String type, Object value, ComponentMetadata cmeta )
+ {
+ PropertyMetadata meta = createPropertyMetadata( "dummy", type, String.valueOf( value ) );
+ meta.validate( cmeta );
+ assertSame( value.getClass(), meta.getValue().getClass() );
+ assertEquals( value, meta.getValue() );
+ }
+
+
+ private void assertPropertyArray( String type, Object value, ComponentMetadata cmeta )
+ {
+ PropertyMetadata meta = createPropertyMetadata( "dummy", type, null );
+ meta.setValues( String.valueOf( value ) );
+ meta.validate( cmeta );
+
+ Object propVal = meta.getValue();
+ assertTrue( propVal.getClass().isArray() );
+ assertPrimitiveType( value.getClass(), propVal.getClass().getComponentType() );
+ assertEquals( 1, Array.getLength( propVal ) );
+ assertEquals( value, Array.get( propVal, 0 ) );
+ }
+
+
+ private void assertPropertyFail( String type, String value, ComponentMetadata cmeta )
+ {
+ try
+ {
+ PropertyMetadata meta = createPropertyMetadata( "dummy", type, value );
+ meta.validate( cmeta );
+ fail( "Expected validation failure for " + type + "=" + value );
+ }
+ catch ( ComponentException ce )
+ {
+ // expected
+ }
+ }
+
+
+ private void assertPropertyArrayFail( String type, String value, ComponentMetadata cmeta )
+ {
+ try
+ {
+ PropertyMetadata meta = createPropertyMetadata( "dummy", type, null );
+ meta.setValues( value );
+ meta.validate( cmeta );
+ fail( "Expected validation failure for " + type + "=" + value );
+ }
+ catch ( ComponentException ce )
+ {
+ // expected
+ }
+ }
+
+
+ private void assertPrimitiveType( final Class expectedBoxClass, final Class actualClass )
+ {
+ if ( expectedBoxClass == String.class )
+ {
+ assertEquals( expectedBoxClass, actualClass );
+ }
+ else if ( expectedBoxClass == Double.class )
+ {
+ assertEquals( Double.TYPE, actualClass );
+ }
+ else if ( expectedBoxClass == Float.class )
+ {
+ assertEquals( Float.TYPE, actualClass );
+ }
+ else if ( expectedBoxClass == Long.class )
+ {
+ assertEquals( Long.TYPE, actualClass );
+ }
+ else if ( expectedBoxClass == Integer.class )
+ {
+ assertEquals( Integer.TYPE, actualClass );
+ }
+ else if ( expectedBoxClass == Short.class )
+ {
+ assertEquals( Short.TYPE, actualClass );
+ }
+ else if ( expectedBoxClass == Byte.class )
+ {
+ assertEquals( Byte.TYPE, actualClass );
+ }
+ else if ( expectedBoxClass == Boolean.class )
+ {
+ assertEquals( Boolean.TYPE, actualClass );
+ }
+ else
+ {
+ fail( "Unexpected box class " + expectedBoxClass );
+ }
+ }
}
diff --git a/scr/src/test/java/org/apache/felix/scr/impl/metadata/XmlHandlerTest.java b/scr/src/test/java/org/apache/felix/scr/impl/metadata/XmlHandlerTest.java
index bc307cd..9fa9864 100644
--- a/scr/src/test/java/org/apache/felix/scr/impl/metadata/XmlHandlerTest.java
+++ b/scr/src/test/java/org/apache/felix/scr/impl/metadata/XmlHandlerTest.java
@@ -239,11 +239,13 @@
// property setting
final PropertyMetadata prop = getPropertyMetadata( cm10, "prop" );
+ prop.validate( cm10 ); // property value requires validation
assertNotNull( "prop exists", prop );
assertEquals( "prop type", "Integer", prop.getType() );
assertEquals( "prop value", 1234, ( ( Integer ) prop.getValue() ).intValue() );
final PropertyMetadata file_property = getPropertyMetadata( cm10, "file.property" );
+ file_property.validate( cm10 ); // property value requires validation
assertNotNull( "file.property exists", file_property );
assertEquals( "file.property type", "String", file_property.getType() );
assertEquals( "file.property value", "Property from File", file_property.getValue() );
@@ -447,12 +449,13 @@
final PropertyMetadata prop = getPropertyMetadata( cm11, "char_array_property" );
assertNotNull( "prop exists", prop );
assertEquals( "prop type", "Character", prop.getType() );
+ prop.validate( cm11 ); // property value conversion requires validation
Object value = prop.getValue();
- assertTrue( "prop arry", value instanceof char[] );
- char[] chars = (char[]) value;
- assertEquals( "prop number of values", 2, chars.length);
- assertEquals( "prop value 0", 'a', chars[0] );
- assertEquals( "prop value 1", 'b', chars[1] );
+ assertTrue( "prop array", value instanceof char[] );
+ char[] chars = ( char[] ) value;
+ assertEquals( "prop number of values", 2, chars.length );
+ assertEquals( "prop value 0", 'A', chars[0] );
+ assertEquals( "prop value 1", 'B', chars[1] );
}
}
diff --git a/scr/src/test/resources/components_properties_11.xml b/scr/src/test/resources/components_properties_11.xml
index f4de438..d16d49c 100644
--- a/scr/src/test/resources/components_properties_11.xml
+++ b/scr/src/test/resources/components_properties_11.xml
@@ -20,7 +20,7 @@
<scr:component xmlns:scr="http://www.osgi.org/xmlns/scr/v1.1.0">
<implementation class="DummyClass" />
<property name="char_array_property" type="Character">
- a
- b
+ 65
+ 66
</property>
</scr:component>