[Date Prev][Date Next] [Chronological] [Thread] [Top]

Re: JLDAP Bug n fixes (ITS#1512)



Dane,

I have reviewed your changes for com.novell.ldap.LDAPAttribute.  I
like much of what I see.  You have done some good work.
It has been a long while since we looked at this code, and it
is obvious it needed looking at.

I was thinking of storing all values as byte[] in way similar
to what you have done, hadn't got around to it yet.

I have some questions:

The existing code stores attribute values in an ArrayList.  The
code you supplied puts the values in an Object[] and must
allocate a new Object[] each time an attribute is added
or removed.  This seems much more expensive than simply
adding a value to a list.   We were trying to avoid object
creation a much as possible. What problem were you trying
to solve when you made that change?

If the problem was that null attributes were being incorrectly
counted, I believe there is a better way to solve that problem.
Attribute values passed in as null should not be allowed or
should be ignored.

It certainly doesn't make sense to do something like
  LDAPAttribute("name", new String[]  {"a", "b", null, "c"});
This case should probably throw an exception.

  LDAPAttribute("name", (String)null)
would act the same as LDAPAttribute("name");

-Steve


>>> <dfoster@equitytg.com> 06-Jan-02 2:07:55 PM >>>
Full_Name: Dane Foster
Version: n/a
OS: WIN2K /Linux
URL: ftp://ftp.openldap.org/incoming/jldapFilez.tar.gz 
Submission from: (NULL) (216.242.111.6)


- There is a NullPointerException being thrown in the copy constructor
of the
<code>org.ietf.ldap.LDAPAttribute</code> class.

- There is ClassCastException being thrown by the
<code>removeValue</code>
method of <code>com.novell.ldap.LDAPAttribute</code> if you set the
value of an
attribute using a byte array instead of a String.

- The <code>size</code> method of
<code>com.novell.ldap.LDAPAttribute</code>
incorrectly reports the number of values for an attribute if any of the
values
are <code>null</code>.  This is open to debate because one may argue
for or
against that <code>null</code> is a valid value.  I've chosen the
latter and
thus consider the behavior a bug.

- I've included a JUnit TestCase class that can reproduce the problem. 
Please
note, the patch that I've made is not simply fixes for the bugs but was
a
rewrite of a large portion of the
<code>com.novell.ldap.LDAPAttribute</code>
class.  The JUnit testcase was written to ensure that my changes would
not
introduce new bugs into the JLDAP API.