Hi Stefan,

thank you for your insights.

Am 03.12.2011 08:25, schrieb Stefan Bodewig:
Hi Oliver,

On 2011-12-02,<ohe...@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-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org



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

Reply via email to