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]>

Reply via email to