Return-Path: 
 <commons-dev-return-58214-apmail-jakarta-commons-dev-archive=jakarta.apache.org@jakarta.apache.org>
Delivered-To: apmail-jakarta-commons-dev-archive@www.apache.org
Received: (qmail 87449 invoked from network); 6 Sep 2004 11:50:31 -0000
Received: from hermes.apache.org (HELO mail.apache.org) (209.237.227.199)
  by minotaur-2.apache.org with SMTP; 6 Sep 2004 11:50:31 -0000
Received: (qmail 95040 invoked by uid 500); 6 Sep 2004 11:50:28 -0000
Delivered-To: apmail-jakarta-commons-dev-archive@jakarta.apache.org
Received: (qmail 94945 invoked by uid 500); 6 Sep 2004 11:50:27 -0000
Mailing-List: contact commons-dev-help@jakarta.apache.org; run by ezmlm
Precedence: bulk
List-Unsubscribe: <mailto:commons-dev-unsubscribe@jakarta.apache.org>
List-Subscribe: <mailto:commons-dev-subscribe@jakarta.apache.org>
List-Help: <mailto:commons-dev-help@jakarta.apache.org>
List-Post: <mailto:commons-dev@jakarta.apache.org>
List-Id: "Jakarta Commons Developers List" <commons-dev.jakarta.apache.org>
Reply-To: "Jakarta Commons Developers List" <commons-dev@jakarta.apache.org>
Delivered-To: mailing list commons-dev@jakarta.apache.org
Received: (qmail 94932 invoked by uid 99); 6 Sep 2004 11:50:27 -0000
X-ASF-Spam-Status: No, hits=0.0 required=10.0
	tests=
X-Spam-Check-By: apache.org
Received-SPF: pass (hermes.apache.org: local policy)
Received: from [194.77.152.181] (HELO mail.hometree.net) (194.77.152.181)
  by apache.org (qpsmtpd/0.28) with ESMTP; Mon, 06 Sep 2004 04:50:23 -0700
Received: from tangens.hometree.net (IDENT:news@mail.hometree.net
 [194.77.152.181])
	by mail.hometree.net (8.12.10/8.12.10) with ESMTP id i86BoKNx018852
	for <commons-dev@jakarta.apache.org>; Mon, 6 Sep 2004 13:50:20 +0200
Received: (from news@localhost)
	by tangens.hometree.net (8.12.10/8.12.8/Submit) id i86BoKhu018851
	for commons-dev@jakarta.apache.org; Mon, 6 Sep 2004 13:50:20 +0200
To: commons-dev@jakarta.apache.org
Path: not-for-mail
From: "Henning P. Schmiedehausen" <hps@intermeta.de>
Newsgroups: hometree.jakarta.commons.dev
Subject: Re: Problems and shortcomings of Commons Configuration (with code
 fixes provided)
Date: Mon, 6 Sep 2004 11:50:20 +0000 (UTC)
Organization: INTERMETA - Gesellschaft fuer Mehrwertdienste mbH
Lines: 172
Message-ID: <chhits$h89$1@tangens.hometree.net>
References: <472AAF63A310604C99AA54DDCEBEADBA19269B@C1-EXG-VS1.corp.org.local>
Reply-To: hps@intermeta.de
NNTP-Posting-Host: forge.intermeta.de
X-Trace: tangens.hometree.net 1094471420 17673 194.77.152.164 (6 Sep 2004
 11:50:20 GMT)
X-Complaints-To: news@intermeta.de
NNTP-Posting-Date: Mon, 6 Sep 2004 11:50:20 +0000 (UTC)
X-Copyright: (C) 1996-2003 Henning Schmiedehausen
User-Agent: nn/6.6.5
X-Virus-Checked: Checked
X-Spam-Rating: minotaur-2.apache.org 1.6.2 0/1000/N

"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