I have a junit test to reproduce the NPE :-)
My problem was how it could be solved. I understand why you've added the
for-loop, but I'm not sure we should keep it:
- I don't think similar checks are made in other Ivy methods which accept a
collection of some type
- it gives you a false feeling of safety: the following code will cause a CCE
as well later in the code:
EvictionData data = new EvictionData("default", null, null, new ArrayList());
data.getSelected().add(new Object());
I would prefer removing the for-loop and adding javadoc to the constructor (and
the markEvicted methods, where this EvictionData is used). And maybe we should
implement a consistent way of verifying the content of collections of all
public methods (and also a way to make sure they can never contain objects of
the wrong type, like the example above illustrates, for instance by using
something similar like
org.apache.commons.collections.collection.TypedCollection as fields).
Maarten
----- Original Message ----
From: Xavier Hanin <[EMAIL PROTECTED]>
To: [email protected]
Sent: Thursday, August 30, 2007 2:39:18 PM
Subject: Re: IVY-590
On 8/30/07, Maarten Coene <[EMAIL PROTECTED]> wrote:
>
> Hi,
>
> jira issue IVY-590 reports a NullPointerException which is caused by the
> following code (selected can be null):
>
> public EvictionData(String rootModuleConf, IvyNode parent,
> ConflictManager conflictManager,
> Collection selected) {
> this.rootModuleConf = rootModuleConf;
> this.parent = parent;
> this.conflictManager = conflictManager;
> this.selected = selected;
> for (Iterator iter = selected.iterator(); iter.hasNext();) {
> Object o = (Object) iter.next();
> if (!(o instanceof IvyNode)) {
> throw new IllegalArgumentException(
> "selected nodes must be instance of IvyNode.
> Found: "
> + o.getClass().getName());
> }
> }
> }
>
>
> Several patches have been contributed to fix this NPE by checking the
> selected parameter for null before entering the for-loop. However, I would
> like to remove this for loop from the code, since the collection always
> contains IvyNodes.
>
> Does someone has an idea why this instanceof-check has been added there?
I think I introduced this check because it happened once that we had a non
IvyNode instance in the collection, causing a CCE later which was difficult
to diagnose. Since this constructor is public, making a check seems like a
not too bad idea (if it was done correctly, ie by checking null before :-)).
But if you experience a performance issue with this check, we can remove the
check, and add a comment in the constructor saying that the collection must
contain only IvyNode, otherwise a CCE may be cast later in other sections of
the code.
To fix the bug, I thought it would be better to first find the case when the
collection is null and add a junit test for it. But it may take more time
than what we have.
WDYT?
Xavier
regards,
> Maarten
>
>
>
>
>
>
> ____________________________________________________________________________________
> Be a better Globetrotter. Get better travel answers from someone who
> knows. Yahoo! Answers - Check it out.
> http://answers.yahoo.com/dir/?link=list&sid=396545469
>
--
Xavier Hanin - Independent Java Consultant
http://xhab.blogspot.com/
http://incubator.apache.org/ivy/
http://www.xoocode.org/
____________________________________________________________________________________
Yahoo! oneSearch: Finally, mobile search
that gives answers, not web links.
http://mobile.yahoo.com/mobileweb/onesearch?refer=1ONXIC
