Re: NullPointerException in Element.setAttribute(final String name, final String value, final Namespace ns)

Hi Gisella.

I pushed through a change for 2.0.0.... 
Issue: https://github.com/hunterhacker/jdom/issues/71
Commit:
https://github.com/hunterhacker/jdom/commit/587efe408670d112f6e1af7094373e20f5a8d0aa

In JDOM 2 I have more flexibility with changing the API, and this one
makes it all somewhat more consistent....

So, the fix is already in for 2.0.0, and released.

I don't expect this to be changed in 1.1.4. It is possible it could break
existing behaviour for other people....

Rolf

On Mon, 9 Apr 2012 18:59:54 +0300, <[email protected]> wrote:
> thank you, Rolf. 
> I had patched our code using JDOM 1.1.3, to check for null.
> 
> A solution for the NPE in  JDOM 1.1.3/2.x could be:
> 
> As in Element.java:
> 
>     public Attribute getAttribute(final String name) {
>         return getAttribute(name, Namespace.NO_NAMESPACE);
>     }
> 
> then:
> 
>     public Attribute getAttribute(final String name, final Namespace ns)
{
>         return (Attribute) attributes.get(name, (ns == null)?
>         Namespace.NO_NAMESPACE : ns);
>     }
> 
> Thanks.
> 
> -----Original Message-----
> From: Rolf Lear [mailto:[email protected]] 
> Sent: Friday, April 06, 2012 7:09 PM
> To: Saavedra Gisella
> Cc: [email protected]; Jason Hunter
> Subject: Re: [jdom-interest] NullPointerException in
> Element.setAttribute(final String name, final String value, final
Namespace
> ns)
> 
> Hi again Gisella.
> 
> I think your examples are great, in the sense that they perfectly 
> illustrate the inconsistencies. On the other hand, they do not help 
> solve the problem of which one is right or wrong. It does make me want 
> to make sure that new Element(String, null) throws NPE though!
> 
> Currently the Attribute handling is 'consistent'... get, remove, set all

> throw NPE for null Namespace.
> 
> I can't think of a nice way to put it, but the easiest thing to do would

> be to change your code to use a conditional:
>    emt.setAttribute(String, String,
>         ns == null ? Namespace.NO_NAMESPACE : ns);
> 
> That's for 1.1.3 at least.
> 
> I still think that maybe I could change the logic on JDOM 2.0.0...
> 
> For all the get/set/remove Attribute I can maybe put the if-logic in.
> 
> Yes, I have played with it, and it's a reasonable thing to do for 2.0.0.

> It is also consistent with 'new Attribute(String, String, null).
> 
> On the whole, I would agree with you, it should not throw NPE, and get 
> and remove should not throw NPE either.
> 
> I just changed to code to 'work', and, surprisingly, all the tests pass.
> 
> That's the wonders of wvwn with 100% code coverage you still miss some 
> things.
> 
> Maybe that will inspire you to try JDOM 2.0.0 ;-)
> 
> Rolf
> 
> 
> On 06/04/2012 8:28 PM, [email protected] wrote:
>> I see the inconsistencies
>> I would say that when removing the nullpointerexception is OK. But when
>> creating, you can see for example
>> new Element(string name)
>> new Element (String name, null)
>> both set the value of Namespace to NO_NAMESPACE.
>> And Element.setAttribute(final String name, final String value) does
not
>> throw a NPE
>> but Element.setAttribute(final String name, final String value, null) 
>> does.
>>
>>
>> -----Original Message-----
>> From: Rolf Lear [mailto:[email protected]]
>> Sent: Friday, April 06, 2012 4:39 PM
>> To: Saavedra Gisella
>> Cc: [email protected]
>> Subject: Re: [jdom-interest] NullPointerException in
>> Element.setAttribute(final String name, final String value, final
>> Namespace ns)
>>
>> Hi Gisella
>>
>> Yes, I can see now where the change is. It happened between JDOM 1.0
and
>> 1.1, back in 2006 ...
>>
https://github.com/hunterhacker/jdom/commit/b48373d95cd91986f83ac5345566258d80e88027#L0R1139
>>
>> In fact, this issue has been reported before:
>> http://markmail.org/message/atybnwqqvlxk7ut4 and there was never any
>> follow-up...
>>
>> So, I can see the problem, and it still exists in JDOM 2.x.
>>
>> But, I can't see an easy fix.
>>
>> I am of the opinion that null input values deserve a
>> NullPointerException unless documented that nulls are allowed... but I
>> also recognize that compatibility is important, and JDOM has some
>> consistency in it when treating null Namespace values
>> as Namespace.NO_NAMESPACE.
>>
>> So, I am going to think about it for a moment.... but, I would not
>> expect a fix in the short term for JDOM 1.x. Even in the best
>> circumstances it would take a month or so to get a 1.1.4 out...
>>
>> For JDOM 2.x, it is also very late in the game.... too late, in fact.
>>
>> In order to fix it properly I would have to decide on a strategy for
>> dealing with null Namespace input. A quick 'poll' of the JDOM 2.x code
>> suggests:
>>
>>
>> Change null to NO_NAMESPACE:
>> ----------------------------
>> Element Constructor.
>> Element.setNamespace(Namespace)
>>
>>
>> Treats null as 'wildcard':
>> --------------------------
>> Element.getChild(String,Namespace)
>> Element.getChildren(String,Namespace)
>> Element.getChildText*(String,Namespace)
>> Element.removeChild(String,Namespace)
>> Element.removeChildren(String,Namespace)
>>
>>
>> Just ignores it.
>> ----------------
>> Element.removeNamespaceDeclaration(Namespace)
>>
>>
>> Throws NPE:
>> -----------
>> Element.addNamespaceDeclaration(Namespace)
>> Element.getAttribute(String,Namespace)
>> Element.getAttributeValue(String,Namespace);
>> Element.getAttributeValue(String,Namespace, String);
>> Element.removeAttribute(String,Namespace);
>> Element.setAttribute(String,String,Namespace)
>>
>>
>> I think, having looked at the 'poll' results, when it comes to
>> Attributes, the NPE is the 'right' thing.
>>
>> Given that getAttribute(String,Namespace) throws NPE (and
>> removeAttribute), and they have 'always' thrown NPE, I think it is
>> logical that setAttribute(String,String,Namespace) throws NPE too.
>>
>> Bottom line is that I am not convinced that it is a bug in 1.1.3.
>> Perhaps it was a bug in 1.0 to allow the null.
>>
>> So, this all comes around to the 'push-back' option. How difficult is
it
>> for you to do the null-check yourself before you call setAttribute()?
>>
>> That is the obvious workaround, and it is the 'right thing'....
>>
>> Rolf
>>
>>
>>
>>
>> On 06/04/2012 5:53 PM, [email protected] wrote:
>>> the stack trace belongs to JDOM 1.1.3
>>>
>>> -----Original Message-----
>>> From: Saavedra Gisella
>>> Sent: Friday, April 06, 2012 2:49 PM
>>> To: 'Rolf Lear'
>>> Cc: [email protected]
>>> Subject: RE: [jdom-interest] NullPointerException in
>>> Element.setAttribute(final String name, final String value, final
>>> Namespace ns)
>>>
>>> Rolf,
>>>
>>> I believe you might be looking at the wrong method:
>>>
>>> This is JDOM 1.0  (ns is passed as null)
>>>
>>>      public Element setAttribute(String name, String value, Namespace
>>>      ns) {
>>>           return setAttribute(new Attribute(name, value, ns));
>>>       }
>>>
>>> Inside this code, the constructor new Attribute(name, value, ns)
calls:
>>>
>>> public Attribute(String name, String value, Namespace namespace) {
>>>           setName(name);
>>>           setValue(value);
>>>           setNamespace(namespace);
>>>       }
>>>
>>> Here setNamespace(namespace) calls:
>>> public Attribute setNamespace(Namespace namespace) {
>>>           if (namespace == null) {
>>>               namespace = Namespace.NO_NAMESPACE;
>>>           }
>>>
>>>           // Verify the attribute isn't trying to be in a default
>>>           namespace
>>>           // Attributes can't be in a default namespace
>>>           if (namespace != Namespace.NO_NAMESPACE&&
>>>               namespace.getPrefix().equals("")) {
>>>               throw new IllegalNameException("", "attribute
namespace",
>>>                   "An attribute namespace without a prefix can only be
>>>                   the " +
>>>                   "NO_NAMESPACE namespace");
>>>           }
>>>           this.namespace = namespace;
>>>           return this;
>>>       }
>>>
>>> where namespace being null is set to NO_NAMESPACE.
>>>
>>>
>>>
>>>
>>> Stack trace (JODM 1.1.3):
>>>
>>> java.lang.NullPointerException
>>>       at org.jdom.AttributeList.indexOf(AttributeList.java:378)
>>>       at org.jdom.AttributeList.get(AttributeList.java:366)
>>>       at org.jdom.Element.getAttribute(Element.java:1004)
>>>       at org.jdom.Element.setAttribute(Element.java:1178)
>>>       at
>>>      
com.navis.argo.business.snx.AbstractXmlExporter.setAttribute(AbstractXmlExporter.java:40)
>>>
>>>
>>> --------------
>>>
>>>
>>> -----Original Message-----
>>> From: Rolf Lear [mailto:[email protected]]
>>> Sent: Friday, April 06, 2012 2:40 PM
>>> To: Saavedra Gisella
>>> Cc: [email protected]
>>> Subject: Re: [jdom-interest] NullPointerException in
>>> Element.setAttribute(final String name, final String value, final
>>> Namespace ns)
>>>
>>> Hi Gisella.
>>>
>>> I have looked through the JDOM 1.0 code. JDOM 1.0 was released in
>>> September 2004. Here's the code as it was at the time:
>>>
>>> Here's the getAttribute method:
>>>
https://github.com/hunterhacker/jdom/blob/jdom-1.0/core/src/java/org/jdom/Element.java#L980
>>>
>>> Here's the attributes.get(String,Namespace) method:
>>>
https://github.com/hunterhacker/jdom/blob/jdom-1.0/core/src/java/org/jdom/AttributeList.java#L365
>>>
>>> And that calls the indexOf(String,Namespace) method:
>>>
https://github.com/hunterhacker/jdom/blob/jdom-1.0/core/src/java/org/jdom/AttributeList.java#L377
>>>
>>> Which calls namespace.getURI().
>>>
>>> The way I see it is that JDOM 1.0 would always throw a
>>> NullPointerException if you gave it a null Namespace.
>>>
>>> I don't think it would have ever worked in JDOM 1.0...
>>>
>>> Did you have your own custom build of JDOM 1.0?
>>>
>>> Rolf
>>>
>>> On 06/04/2012 5:08 PM, Rolf Lear wrote:
>>>> Hi Gisella.
>>>>
>>>> Is it possible to send us a stack trace, at least the parts including
>>>> the org.jdom.* code.
>>>>
>>>> Thanks
>>>>
>>>> Rolf
>>>>
>>>>
>>>> On 06/04/2012 1:43 PM, [email protected] wrote:
>>>>> I just upgraded from jdom 1.0 to jdom 1.1.3
>>>>>
>>>>> and I was setting an attribute where the namespace was NULL.
>>>>>
>>>>> The old code was different, it was NOT calling get(...), so it was
>>>>> setting
>>>>> the namespace to namespace.NO_NAMESPACE when the namespace was NULL,
>>>>>
>>>>> before calling this get(...)
>>>>>
>>>>> but the new code, when it calls getAttribute(name, ns) (first line
of
>>>>> method described)
>>>>>
>>>>> is getting a null pointer on the namespace.
>>>>>
>>>>> Probably the namespace needs to be set to NO_NAMESPACE before
calling
>>>>> get(..) when it is NULL.
>>>>>
>>>>> *Gisella Saavedra**
>>>>> *Framework Team
>>>>> [email protected]<mailto:[email protected]>_
>>>>>
>>>>>
>>>>>
>>>>> http://www.navis.com/images/spacer.gif
>>>>>
>>>>> 1000 Broadway, Suite 150, Oakland, CA 94607 | T+1 510 267 5123 T
>>>>> Main+1
>>>>> 510 267 5000 F+1 510 267 5100 | _http://www.navis.com
>>>>> <http://www.navis.com/>_
>>>>>
>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> To control your jdom-interest membership:
>>>>>
http://www.jdom.org/mailman/options/jdom-interest/[email protected]
>>>>
>>>> _______________________________________________
>>>> To control your jdom-interest membership:
>>>>
http://www.jdom.org/mailman/options/jdom-interest/[email protected]
>>>>
>>>
>>
_______________________________________________
To control your jdom-interest membership:
http://www.jdom.org/mailman/options/jdom-interest/[email protected]