commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Henning P. Schmiedehausen" <...@intermeta.de>
Subject Re: Problems and shortcomings of Commons Configuration (with code fixes provided)
Date Mon, 06 Sep 2004 11:50:20 GMT
"Hutchison, Ben" <Ben.Hutchison@sensis.com.au> writes:


>Hi Commons developers,

>I have been using commons configuration fairly extensively, and have
>noticed (and fixed) a number of problems.

Thanks for your work. Next time, please try to provide your patches as
unified diff attachments (using diff -u). Thanks.

>I'll describe the problems and fixes here is the hope that they can be
>incorporated back into the codebase and result in a better framework.

>1. Build file script because it uses hardcode paths "D:/.../etc/etc".
>Fix - use the basedir variable as below.

>  <property name="defaulttargetdir" value="${basedir}/target">
>  </property>
>  <property name="libdir" value="${basedir}/target/lib">
>  </property>
>  <property name="classesdir" value="${basedir}/target/classes">
>  </property>
>  <property name="testclassesdir"
>value="${basedir}/target/test-classes">
>  </property>
>  <property name="testclassesdir"
>value="${basedir}/target/test-classes">
>  </property>
>  <property name="testreportdir" value="${basedir}/target/test-reports">
>  </property>

I applied this patch.

>2. XMLConfiguration fails badly with NPE if an invalid system resource
>name passed to constructor.

>I have added a bit of checking that tells the poor user what happened:

>private static File resourceURLToFile(String resource) {
>        URL confURL =
>XMLConfiguration.class.getClassLoader().getResource(resource);
>        if (confURL == null) {
>            confURL = ClassLoader.getSystemResource(resource);
>        }
>        if (confURL == null)
>        	throw new IllegalArgumentException("Resource:
>"+resource+" not found thru context or system classloaders.");
>        return new File(confURL.getFile());
>    }

Thanks. Applied.


>3. XMLConfiguration.addProperty(key, Object) makes improper downcast of
>Object to String.

>Solution - replace downcast with toString(). This allows you to write
>back Ints, Boolean etc to the configuration:

>private void addXmlProperty(String name, Object value) {
>...
>        if (attName == null) {
>            CharacterData data =
>document.createTextNode(value.toString());
>            child.appendChild(data);
>        } else {
>            child.setAttribute(attName, value.toString());
>...
>}

Fixed by ebourg to use String.valueOf(), which is null-safe.

>4 XMLConfiguration.addProperty(key, Object) doesn't handle nest keys
>properly.

>In this loop logic, the full key name is broken into parts, and the DOM
>tree is walked. However, if a branch mentioned in the key isn't there,
>the loop incorrectly breaks out. Result is that eg addProperty("a.b")
>doesn't actually add at "a.b" unless "a" & "b" already exist. Old
>version shown:
>        String[] nodes = parseElementNames(name);
>       
>        Element element = document.getDocumentElement();
>        Element parent = element;

>        for (int i = 0; i < nodes.length; i++) {
>            if (element == null)
>                break;    //******************** this is not right
>**********************
>            parent = element;
>            String eName = nodes[i];
>            Element child = getChildElementWithName(eName, element);

>            element = child;
>        }

>My tested fix:

>        Element element = document.getDocumentElement();
>        Element child = null;
>       
>        for (int i = 0; i < nodes.length; i++) {
>            child = getChildElementWithName(nodes[i], element);
>            if (child == null) {
>                child = document.createElement(nodes[i]);
>                element.appendChild(child);
>            }
>            element = child;
>        }

>        if (attName == null) {
>            CharacterData data =
>document.createTextNode(value.toString());
>            child.appendChild(data);
>        } else {
>            child.setAttribute(attName, value.toString());
>        }

After applying this patch, the unit tests no longer pass:

--- cut ---
Testsuite: org.apache.commons.configuration.TestXMLConfiguration
Tests run: 22, Failures: 1, Errors: 0, Time elapsed: 0.858 sec
 
Testcase: testSave(org.apache.commons.configuration.TestXMLConfiguration):      FAILED
The saved configuration doesn't contain the key 'test.array'
junit.framework.AssertionFailedError: The saved configuration doesn't contain the key 'test.array'
        at org.apache.commons.configuration.TestXMLConfiguration.testSave(TestXMLConfiguration.java:374)
        at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39)
        at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
--- cut ---

So I -1 it. Please fix and supply an unified diff.


>I am presently running a modified version of the commons configuration
>jar, but would appreciate it if these fixes could be committed to the
>library so that I can pick up improvement in new versions.

>Also, not a bug as such, but I noticed that commons configuration have
>some pretty heavy library requirements. I only use the XML
>configuration, and I found by trial and error that I didn't need more
>over half of the stated dependencies. It would be nice if the
>configuration build or distribution process could split out some of the
>extensions and their dependencies so people looing for a lightweight
>solution can us it.

Do you really need these (runtime) or are these just compile time
requirements?

However, using maven makes resolving the dependencies easy, so this is
fairly low on my prio list.

	Regards
		Henning

-- 
Dipl.-Inf. (Univ.) Henning P. Schmiedehausen          INTERMETA GmbH
hps@intermeta.de        +49 9131 50 654 0   http://www.intermeta.de/

RedHat Certified Engineer -- Jakarta Turbine Development  -- hero for hire
   Linux, Java, perl, Solaris -- Consulting, Training, Development

"Fighting for one's political stand is an honorable action, but re-
 fusing to acknowledge that there might be weaknesses in one's
 position - in order to identify them so that they can be remedied -
 is a large enough problem with the Open Source movement that it
 deserves to be on this list of the top five problems."
                       -- Michelle Levesque, "Fundamental Issues with
                                    Open Source Software Development"

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


Mime
View raw message