Hi, Since Avalon is in codefreeze, I thought I'd ask first..
I'd like to make explicit two contracts in the Configuration: 1) getNamespace() will never return null. If no namespace is set, getNamespace() will return "". 2) getPrefix should be exactly the same. Never null, "" as the default. I think we agreed on 1). 2) was never formally defined, but it makes sense. By "make explicit", I mean: - make getNamespace() and getPrefix() throw ConfigurationExceptions if the builder did not set the internal fields. This follows the existing behaviour of getValue(), getValueAs*() and getAttributeAs*() methods. - Document in the javadocs that those methods will never return null. And now the *real* reason for these changes.. :) Currently, if you ask DefaultConfigurationSerializer to serialize a Configuration without namespaces (eg <foo/>), it will throw a NPE. Ie, it's *very* broken.. I think this bug is a symptom of the loose contract around getPrefix. Here is the relevant code (~line 121): String nsPrefix = ""; if ( element instanceof AbstractConfiguration ) { nsPrefix = ((AbstractConfiguration) element).getPrefix(); >>> nsPrefix is null here } boolean nsWasDeclared = false; final String existingURI = m_namespaceSupport.getURI( nsPrefix ); >>> getURI(null) will cause a NPE So tightening the contract so nsPrefix is never null is IMHO a more correct fix than adding a "if (nsPrefix != null)" check. The attached patch does the following: - tightens the getNamespace() and getPrefix() contracts as proposed above - Fixes the Serializer NPE bug when parsing non-namespace XML - Fixes a Serializer bug where no xmlns declarations are made. Ie, <x:foo xmlns:x="http://bar"/> will be serialized into <x:foo/>. And yes, I know there should be unit tests for all of this.. they're next on my list. --Jeff
Index: src/java/org/apache/avalon/framework/configuration/AbstractConfiguration.java =================================================================== RCS file: /home/cvspublic/jakarta-avalon/src/java/org/apache/avalon/framework/configuration/AbstractConfiguration.java,v retrieving revision 1.10 diff -u -r1.10 AbstractConfiguration.java --- src/java/org/apache/avalon/framework/configuration/AbstractConfiguration.java 2001/11/19 16:59:15 1.10 +++ src/java/org/apache/avalon/framework/configuration/AbstractConfiguration.java 2001/12/09 12:00:47 @@ -24,8 +24,11 @@ * Returns the prefix of the namespace. This is only used as a serialization * hint, therefore is not part of the client API. It should be included in * all Configuration implementations though. + * @return A non-null String (defaults to "") + * @throws ConfigurationException if no prefix was defined (prefix is + * <code>null</code>. */ - protected abstract String getPrefix(); + protected abstract String getPrefix() throws ConfigurationException; /** * Returns the value of the configuration element as an <code>int</code>. Index: src/java/org/apache/avalon/framework/configuration/Configuration.java =================================================================== RCS file: /home/cvspublic/jakarta-avalon/src/java/org/apache/avalon/framework/configuration/Configuration.java,v retrieving revision 1.8 diff -u -r1.8 Configuration.java --- src/java/org/apache/avalon/framework/configuration/Configuration.java 2001/12/09 04:53:33 1.8 +++ src/java/org/apache/avalon/framework/configuration/Configuration.java 2001/12/09 12:00:50 @@ -134,7 +134,7 @@ * @since 4.1 * @return a String identifying the namespace of this Configuration. */ - String getNamespace(); + String getNamespace() throws ConfigurationException; /** * Return a new <code>Configuration</code> instance encapsulating the Index: src/java/org/apache/avalon/framework/configuration/DefaultConfiguration.java =================================================================== RCS file: /home/cvspublic/jakarta-avalon/src/java/org/apache/avalon/framework/configuration/DefaultConfiguration.java,v retrieving revision 1.11 diff -u -r1.11 DefaultConfiguration.java --- src/java/org/apache/avalon/framework/configuration/DefaultConfiguration.java 2001/11/26 09:28:43 1.11 +++ src/java/org/apache/avalon/framework/configuration/DefaultConfiguration.java 2001/12/09 12:00:52 @@ -44,6 +44,13 @@ /** * Create a new <code>DefaultConfiguration</code> instance. + * @param name config node name + * @param location Builder-specific locator string + * @param ns Namespace string (typically a URI). Should not be null; use "" + * if no namespace. + * @param prefix A short string prefixed to element names, associating + * elements with a longer namespace string. Should not be null; use "" if no + * namespace. */ public DefaultConfiguration( final String name, final String location, @@ -53,7 +60,7 @@ m_name = name; m_location = location; m_namespace = ns; - m_prefix = prefix; // only used as a serialization hint + m_prefix = prefix; // only used as a serialization hint. Cannot be null } /** @@ -67,17 +74,37 @@ /** * Returns the namespace of this configuration element */ - public String getNamespace() + public String getNamespace() throws ConfigurationException { - return m_namespace; - } + if( null != m_namespace ) + { + return m_namespace; + } + else + { + throw new ConfigurationException( "No namespace (not even default \"\") is associated with the "+ + "configuration element \"" + getName() + + "\" at " + getLocation() ); + } + } /** * Returns the prefix of the namespace + * @throws ConfigurationException if prefix is not present (<code>null</code>). */ - protected String getPrefix() + protected String getPrefix() throws ConfigurationException { - return m_prefix; + if( null != m_prefix ) + { + return m_prefix; + } + else + { + throw new ConfigurationException( "No prefix (not even default \"\") is associated with the "+ + "configuration element \"" + getName() + + "\" at " + getLocation() ); + } + } /** Index: src/java/org/apache/avalon/framework/configuration/DefaultConfigurationSerializer.java =================================================================== RCS file: /home/cvspublic/jakarta-avalon/src/java/org/apache/avalon/framework/configuration/DefaultConfigurationSerializer.java,v retrieving revision 1.6 diff -u -r1.6 DefaultConfigurationSerializer.java --- src/java/org/apache/avalon/framework/configuration/DefaultConfigurationSerializer.java 2001/11/19 16:59:15 1.6 +++ src/java/org/apache/avalon/framework/configuration/DefaultConfigurationSerializer.java 2001/12/09 12:00:56 @@ -83,7 +83,7 @@ * be set before calling this method. */ protected void serialize( final Configuration source ) - throws SAXException + throws SAXException, ConfigurationException { m_namespaceSupport.reset(); m_handler.startDocument(); @@ -95,7 +95,7 @@ * Serialize each Configuration element. This method is called recursively. */ protected void serializeElement( final Configuration element ) - throws SAXException + throws SAXException, ConfigurationException { m_namespaceSupport.pushContext(); @@ -106,8 +106,12 @@ { for (int i = 0; i < attrNames.length; i++) { - attr.addAttribute("", attrNames[i], attrNames[i], "CDATA", - element.getAttribute(attrNames[i], "")); + attr.addAttribute("", // namespace URI + attrNames[i], // local name + attrNames[i], // qName + "CDATA", // type + element.getAttribute(attrNames[i], "") // value + ); } } @@ -118,14 +122,31 @@ { nsPrefix = ((AbstractConfiguration) element).getPrefix(); } + // nsPrefix is guaranteed to be non-null at this point. boolean nsWasDeclared = false; - // Is this namespace already declared? final String existingURI = m_namespaceSupport.getURI( nsPrefix ); + + // ie, there is no existing URI declared for this prefix or we're + // remapping the prefix to a different URI if ( existingURI == null || !existingURI.equals( nsURI ) ) { nsWasDeclared = true; + if (nsPrefix.equals("") && nsURI.equals("")) + { + // implicit mapping; don't need to declare + } + else if (nsPrefix.equals("")) + { + // (re)declare the default namespace + attr.addAttribute("", "xmlns", "xmlns", "CDATA", nsURI); + } + else + { + // (re)declare a mapping from nsPrefix to nsURI + attr.addAttribute("", "xmlns:"+nsPrefix, "xmlns:"+nsPrefix, "CDATA", nsURI); + } m_handler.startPrefixMapping( nsPrefix, nsURI ); m_namespaceSupport.declarePrefix( nsPrefix, nsURI ); } Index: src/java/org/apache/avalon/framework/configuration/NamespacedSAXConfigurationHandler.java =================================================================== RCS file: /home/cvspublic/jakarta-avalon/src/java/org/apache/avalon/framework/configuration/NamespacedSAXConfigurationHandler.java,v retrieving revision 1.1 diff -u -r1.1 NamespacedSAXConfigurationHandler.java --- src/java/org/apache/avalon/framework/configuration/NamespacedSAXConfigurationHandler.java 2001/12/07 13:44:54 1.1 +++ src/java/org/apache/avalon/framework/configuration/NamespacedSAXConfigurationHandler.java 2001/12/09 12:00:56 @@ -122,7 +122,8 @@ final String namespaceURI, final String location ) { - final String prefix = m_namespaceSupport.getPrefix( namespaceURI ); + String prefix = m_namespaceSupport.getPrefix( namespaceURI ); + if (prefix == null) prefix = ""; return new DefaultConfiguration( localName, location, namespaceURI, prefix ); } Index: tools/ext/avalon-framework.jar =================================================================== RCS file: /home/cvspublic/jakarta-avalon/tools/ext/avalon-framework.jar,v retrieving revision 1.9 diff -u -r1.9 avalon-framework.jar Binary files /tmp/cvsULUegfhsxh and avalon-framework.jar differ
-- To unsubscribe, e-mail: <mailto:[EMAIL PROTECTED]> For additional commands, e-mail: <mailto:[EMAIL PROTECTED]>