Tightening namespace contracts, and bugfixes

[Jeff Turner <je...@socialchange.net.au>]

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

Re: Tightening namespace contracts, and bugfixes

[Jeff Turner <je...@socialchange.net.au>]

On Mon, Dec 10, 2001 at 10:31:02AM +1100, Peter Donald wrote:
> On Mon, 10 Dec 2001 00:09, Jeff Turner wrote:
> > Since Avalon is in codefreeze, I thought I'd ask first..
> 
> I would consider these show stopper bugs that need to be fixed before we 
> release. So just go ahead and fix them .. unless someone else has objections?

Righto, done.

--Jeff

--
To unsubscribe, e-mail:   <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>

Re: Tightening namespace contracts, and bugfixes

[Peter Donald <pe...@apache.org>]

On Mon, 10 Dec 2001 00:09, Jeff Turner wrote:
> Since Avalon is in codefreeze, I thought I'd ask first..

I would consider these show stopper bugs that need to be fixed before we 
release. So just go ahead and fix them .. unless someone else has objections?

Anyways they sound good to me but I don't actually use that bit of code much 
so .. ;)

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

-- 
Cheers,

Pete

--------------------------------------------------------------
"Science is like sex: sometimes something useful comes out, 
but that is not the reason we are doing it" -- Richard Feynman
--------------------------------------------------------------

--
To unsubscribe, e-mail:   <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>