commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Oliver Heger <oliver.he...@oliver-heger.de>
Subject Re: svn commit: r1209685 - /commons/proper/configuration/trunk/src/main/java/org/apache/commons/configuration/ConfigurationFactory.java
Date Sat, 03 Dec 2011 10:27:44 GMT
Hi Stefan,

thank you for your insights.

Am 03.12.2011 08:25, schrieb Stefan Bodewig:
> Hi Oliver,
>
> On 2011-12-02,<oheger@apache.org>  wrote:
>
>> Another attempt to fix the GUMP build using an ugly cast.
>
> Seeing you jumping through these hoops I wonder whether it wouldn't be
> better to look at the core issue.  If configuration's compilation only
> fails in Gump this means there is an API broken between the component
> version you have specified (Digester 1.8.1) and the version provided by
> Gump (Digester from DIGESTER_2_X branch).
>
> The change in digester happened about three years ago
> <http://svn.apache.org/viewvc/commons/proper/digester/branches/DIGESTER_2_X/src/main/java/org/apache/commons/digester/substitution/MultiVariableExpander.java?r1=560660&r2=729129>
>
> Is configuration's dependency on Digester new or why have we started to
> see this error just now?  You may want to upgrade to Digester 2.0 or 2.1
> to start with (or even Digester3?) if it is new.  If it isn't then
> obviously 2.x isn't compatible enough to 1.x for commons-configuration
> and we could think about a different solution.

Configuration now targets Java 1.5 so we try to incorporate generics in 
the API without breaking binary compatibility in the first draft. In 
this special case this caused the problem that the plain Map we had 
before is now no more compatible with a Map<String, Object> expected by 
the digester class.

What makes things a bit difficult to decide is the fact that the class 
affected in the Configuration code base (ConfigurationFactory) is 
actually deprecated, and it is the only one which requires the 
dependency to digester. Therefore there is not much motivation to do 
some major changes.

>
> Apart from that
>
>> -        Map<Object, Object>  systemProperties = System.getProperties();
>> +        // This is ugly, but it is safe because the Properties object returned
>> +        // by System.getProperties() (which is actually a Map<Object, Object>)
>> +        // contains only String keys.
>> +        @SuppressWarnings("unchecked")
>> +        Map<String, Object>  systemProperties =
>> +                (Map<String, Object>) (Object) System.getProperties();
>>           MultiVariableExpander expander = new MultiVariableExpander();
>>           expander.addSource("$", systemProperties);
>
> The assumption you make here may not always hold true.  We've had
> several bug reports against Ant when we assumed the system properties
> would only hold string keys (or even values) and this simply was not
> true in cases where Ant code was used embedded in a larger application
> that was doing strange things.  java.util.Properties will not prevent
> you from putting objects into it.  I assume a commons component is at a
> bigger risk of such scenarios than an application like Ant.
>
> Even if it takes a bit longer it may be cleaner to create a new Map that
> contained a filtered view of only those properties that actually have
> string keys.

I also thought about the copying approach, but then hoped that for 
system properties it would be safe to rely on String keys.

Oliver

>
> Stefan
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org


Mime
View raw message