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

Comments on draft-ietf-ldapext-ldap-java-api-15.txt



Please cc me in the discussion. I'm not on this mailing list.

_____________________
Rosanna Lee
Java Software, Sun Microsystems, Inc
rosanna.lee@sun.com

-----------------------------------------------------------------------

1. Section 1.2  The LDAP classes 

   The LDAPConnection class also provides fields for storing settings 
   that are specific to the LDAP session (such as limits on the number 
   of results returned or timeout limits).

In fact, the LDAPConnection class does not provide fields because none
has been declared. What the class does provide are get/set methods
for updating the state of the connection. 

2. LDAPSearchResult and LDAPSearchResults

A natural conclusion of examining these names is that they should be
related: LDAPSearchResults is probably a collection of
"LDAPSearchResult"s. But these classes are not related at all.  An
LDAPSearchResults is an enumeration of LDAPEntry while
LDAPSearchResult is a result from an async search. One of these
classes should be renamed to avoid confusion. 

3. Section 1.4 Interfaces

   LDAPEntryComparator        An interface to support arbitrary sorting 
                              algorithms for entries returned by a 
                              search operation. The basic Java LDAP 
                              classes include one implementation: 
                              LDAPCompareAttrNames, to sort in 
                              ascending order based on one or more 
                              attribute names. 

Not just ascending, but ascending or descending order.

4. Section 1.4 Interfaces

   LDAPRebind                 A programmer desiring to supply 
                              credentials to the default 
                              reauthentication behavior when 
                              automatically following referrals MUST
				...

Supplying "credentials" to a "behavior" is very strange wording.


5. Section 1.4 Interfaces
   LDAPBind                   This interface allows a programmer to 
                              override the default authentication and 
                              reauthentication behavior when 
                              automatically following referrals and 
                              search references. It is used to specify 
                              the authentication mechanism used on 
                              automatic referral following. 

This description seems too restrictive.  LDAPBind is used to specify
arbitrary behavior--behavior that might be unrelated to
authentication--when following referrals.

6. Section 1.4 Interfaces
   LDAPUnsolicitedNotificationListener  Allows a client to be notified 
                              when unsolicited messages arrive from a 
                              server. Unsolicited messages have a 
                              message ID of 0. An implementation of the 
                              Java LDAP API SHOULD NOT generate 
                              messages with an ID of 0. 

Is the Java LDAP API even allowed to generate messages? Isn't message
generation the responsibility of the server?

7. Section 1.5 Classes

   LDAPConstraints            Defines options controlling all 
                              operations. 

Do you mean "Defines options controlling all operations on the
Directory"? Or perhaps "... on LDAPConnection"?

8. Section 1.5 Classes
   LDAPExtendedResponse       The response returned by an LDAP server 
                              on an asynchronous extended operation 
                              request. It extends LDAPResponse. 

Not async.

9. Section 1.5 Classes

   LDAPDITContentRuleSchema   Represents a specific DIT content rule in 
                              the directory schema. 
    
   LDAPDITStructureRuleSchema Represents a specific DIT structure rule 
                              in the directory schema. 

   LDAPMatchingRuleUseSchema  Represents a specific matching rule use 
                              in the directory schema. 

   LDAPNameFormSchema         Represents a specific name form in the 
                              directory schema. 
    
   LDAPSyntaxSchema           Represents a specific syntax definition 
                              in the directory schema. 

Delete 'specific'. 

10. Section 1.5 Classes

   LDAPAttributeSchema        Represents a schematic definition of a 
                              particular attribute in a particular 
                              Directory Server. 

   LDAPMatchingRuleSchema     Represents the schematic definition of a 
                              particular matching rule in a particular 
                              Directory Server. 

   LDAPObjectClassSchema      Represents the schematic definition of a 
                              particular object class in a particular 
                              Directory Server. 

   LDAPSchema                 Represents the schema controlling one or 
                              more entries held by a particular 
                              Directory Server. 

Delete 'particular'.  Why 'in a particular Directory Server'? In fact,
you can create one of these and it need not be associated with any
Directory Server until you decide to add it to one.

11. Section 1.5 Classes

   LDAPMessage                Base class for LDAP request and response 
                              messages. Subclassed by response messages 
                              used in asynchronous operations. 

   LDAPResponse               Represents a message received from an 
                              LDAP server in response to an 
                              asynchronous request. It extends 
                              LDAPMessage. 

async is incorrect. Also used for non-async extendedOperation.

12. Section 1.5 Classes

   LDAPResponseListener       Low-level mechanism for processing 
                              asynchronous messages received from a 
                              server. 

   LDAPSearchListener         Low-level mechanism for queuing 
                              asynchronous search results received from 
                              a server. 
    
'Low-level'? Compared to what? Is there a high-level mechanism for
doing the same thing? If not, delete 'low-level'.

13. Section 1.5 Classes

   LDAPSchemaElement          Base class for representing LDAP schema 
                              elements. 

Base class for representing an LDAP schema element. (singular).

14. Section 1.5 Classes

   LDAPUrl                    Encapsulates parameters of an LDAP Url 
                              query, as defined in [LDAPURL]. 

Strange wording. Why isn't this simply "Represents an LDAP URL
[LDAPURL]"?

15. Section 1.7 LDAP API use 

   For the asynchronous methods, exceptions are raised only for 
   connection errors.

Are local errors in the "connection errors" category?

16. Section 1.7 LDAP API use 

   To facilitate user feedback during synchronous searches, intermediate 
   search results can be obtained before the entire search operation is 
   completed by specifying the number of entries to return at a time.  
   Standard Java Enumerations are used to parse synchronous search 
   results.  

parse -> obtain

Users of the enumeration don't parse anything, although the underlying
Enumeration implementation might.

17. Section 3.6 Array Return Values 

Section 3.6 belongs here in section 1.7, where similar things are
talked about, instead of being in a section named 'Implementation
Considerations'.

18.  Section 2.2.1 Constructors 

   public LDAPAttributeSchema(String name, 
                              String oid, 
                              String description, 
                              String syntaxString, 
                              boolean single 
                              String superior, 
                              String[] aliases, 
                              boolean obsolete, 
                              String equality, 
                              String ordering, 
                              String substring, 
                              boolean collective, 
                              boolean userMod, 
                              int usage) 

Which ones of these arguments are optional?  The descriptions seem to
say that the following are optional:
	description
	superior
	aliases
	equality
	ordering
	substring

RFC 2252 says that the following are optional:
	name/aliases
	description
	obsolete
	superior
	equality
	ordering
	substring
	usage
	syntaxString
	userMod
	collective
	single

Need to clarify in the descriptions which are optional.  If conforming
to RFC 2252, then need to specify whether the corresponding methods
(such as getSyntaxString()) can return null.

19 Section 2.3 public class LDAPAttributeSet 
                 implements Cloneable 

If LDAPAttributeSet is really a set, then it should implement
java.util.Set so that it benefits from the utilties offered by the
Java Collections Framework.

20.. Section 2.3.5 getAttribute 

   Returns a single best-match attribute, or none if no match is 
   available in the entry. 

entry -> set

21. Section 2.3.2 add 
    
   public void add(LDAPAttribute attr) 
    
   Adds the specified attribute to this attribute set. 

What happens if attr with same name already exists? Overwritten?
Ignored?  Runtime exception? Need to specify.

22. Section 2.3.4 elementAt 
    
   public LDAPAttribute elementAt(int index) 
                               throws ArrayIndexOutOfBoundsException 
    
   Returns the attribute at the position specified by the index. The 
   index is 0-based. 

and Section 2.3.9 removeElementAt 
    
   public void removeElementAt(int index) 
                               throws ArrayIndexOutOfBoundsException 
    
   Removes the attribute at the position specified by the index.


Are the contents of LDAPAttributeSet ordered? A set, by definition, is
unordered. How is the order affected by addition of a new attribute?
Added to the beginning? end? random?  If LDAPAttributeSet is ordered,
then the class should be renamed and the behavior of these methods
specified.

23. Section 2.3.5 getAttribute (subtyping)

         getAttribute( "cn", "lang-en-us" ) returns the "cn;lang-en" 
                                            attribute. 
         getAttribute( "sn", "lang-en" )    returns the "sn" attribute. 
         getAttribute( "cn", "lang-ja" )    returns null. 

These seem to violate subtyping rules and RFC 2596. A lang-en is not a
lang-en-us.  A lang-ja-JP-kanji is a lang-ja. An 'sn' is not an
'sn;lang-en'.

24. Section 2.3.5 getAttribute 

   ... If there are 
   no matching attributes, null is returned. If the results are 
   ambiguous, the exception UnsupportedOperationException is thrown. 

UnsupportedOperationException seems to be the wrong exception to throw
here.  RuntimeExceptions are not intended to be used in this way.  The
nature of the failure seem to indicate the need for a checked
exception and that caller of getAttribute() should expect the
exception if the parameter specification is ambiguous

25. Section 2.4.1 bind

   ... An implementation may 
   access the host, port, credentials, and other information in the 
   original LDAPConnection object ...

Contrary to this statement, there does not appear to be a way for the
implementation to access the credentials of the original
LDAPConnection.


26. Section 2.5 public class LDAPCompareAttrNames 

Behavior unspecified as to how entries without the specified attribute
name/names are treated for ascending and descending orders. 


27. LDAPCompareAttrNames is a LDAPEntryComparator. Yet the name sounds
like a method instead of a comparator. Better choices might be

LDAPAttrNamesEntryComparator or LDAPAttrNamesComparator


28. Section 2.5.1 Constructors 

      ascendingFlags Array of flags, one for each attrName, where each 
                      one is true to sort in ascending order, false for 
                      descending order. An LDAPException is thrown if 
                      the length of ascendingFlags is not greater than 
                      or equal to the length of attrNames. 

Why is greater than acceptable? Why not just require equality?
Otherwise, you need to add text to say that the extras are ignored.

29. Section 2.6.2 abandon 

   The application is responsible for not calling this method more than 
   once with a particular id or LDAPSearchResults. 

What is the behavior otherwise? Undefined? What is the behavior if
abandon is called on a nonexistent message id? LDAPException? 


30. Section 2.6.3 add

   The application is responsible for not specifying attribute values 
   which are not valid according to the syntax defined for the 
   attributes. It is also responsible for including all attributes which 
   are required for the entry. 

What is the behavior otherwise? LDAPException? Which error codes?  Why
single out attribute values in particular for conformance?  Why not
list requirements on dn, etc. Need to clarify or delete.


31. Section 2.6.5 bind (passwd security)

passwd is explicitly typed to be String. Using String for the password
contradicts Section 4, which says:

   Implementations of this API SHOULD be cautious when handling 
   authentication credentials. In particular, keeping long-lived copies 
   of credentials without the application's knowledge is discouraged. 

The common practice in the JDK is to use char[] for passwords.


32. Section 2.6.5 bind (version)

   public void bind(int version, 
                    String dn, 
                    String passwd) 
                    throws LDAPException 

and related overloads with 'version' parameter.

      version        LDAP protocol version requested: currently 2 or 
                      3. 

What if the user specifies 2 and the server accepts? Then the session
proceeds in v2? Then how does that reconcile with the earlier
statement in Section 1.1 The LDAP model.

   Before the client encodes and sends a string value to a server, the 
   string values are converted from the Java 16-bit Unicode format to 
   UTF-8 format, which many LDAPv3 protocol elements and value 
   encodings use. The integrity of double-byte and other non-ASCII 

v2 doesn't use UTF-8. 

33. Section 2.6.5 bind (more comments on version)

   ... This API is specifically designed 
   for use with LDAPv3. Unless the API provides specific support (as 
   defined in other documents) for other versions of LDAP, version 3 
   should be used. 

How does this reconcile with the version argument description, which
seems to imply that both 2 & 3 are supported.

      version        LDAP protocol version requested: currently 2 or 
                      3. 

34.  Section 2.6.5 bind (dn/passwd parameters)

      dn             If non-null and non-empty, specifies that the 
                      connection and all operations through it SHOULD 
                      be authenticated with dn as the distinguished 
                      name. 
       
      passwd         If non-null and non-empty, specifies that the 
                      connection and all operations through it SHOULD 
                      be authenticated with dn as the distinguished 
                      name and passwd as password. 

Doesn't say what happens if dn is null or empty, or if passwd is null
or empty, or if dn is non-empty and passwd is empty. Need to specify.

Also, why is it only SHOULD? Is the implementation free to ignore dn
and passwd and authenticate using any dn/passwd it chooses?

Doesn't say how to do anonymous bind.

Should explicitly say that non-empty dn and non-empty passwd will be
used to perform simple bind. "simple bind" is mentioned elsewhere in
the class (e.g., isBound()) but not in this section at all.


35. Section 2.6.5 bind (authzId variants)

   public void bind(String dn, 
                    String authzId, 
                    Hashtable props, 
                    javax.security.auth.callback.CallbackHandler cbh) 
                    throws LDAPException 
    
    
   public void bind(String dn, 
                    String authzId, 
                    Hashtable props, 
                    javax.security.auth.callback.CallbackHandler cbh, 
                    LDAPConstraints cons) 
                    throws LDAPException 
    
These appear to be missing the String[] mechanisms parameter, and if
added, aren't these equivalent to the two other definitions that
follow them?  

36. Section 2.6.5 bind (authzId variants)

Should use java.util.Map instead of java.util.Hashtable because
it allows the client to use more efficient implementations.

Similar change to 2.6.24 getSaslBindProperties 
    
   public Hashtable getSaslBindProperties() 
    

37. Section 2.6.5 bind  (SASL bind wording)

   A SASL bind call() may involved 
   multiple protocol requests. An attempt to invoke an operation other 
   than bind between bind requests in a multi-stage bind results in an 
   LDAPException with the result code SASL_BIND_IN_PROGRESS. 

How can someone do this? Isn't the SASL bind method synchronous?  It
returns only when SASL authentication terminates either successfully
or unsuccessfully.

call() -> call

38. Section 2.6.5 bind (authzId variants - parameters)

      authzId        If not null and not empty, an LDAP authzID [AUTH] 
                      to be passed to the SASL layer. 

What if it is null or empty?  Should say that if null or empty, that
the authzId will be treated as an empty string and processed as per
RFC 2222. (Or, instead of referencing 2222, say what RFC 2222 does
with it).

Also, is the dn allowed to be null and/or empty for SASL bind?  Need
to specify.

      cbh            A class which may be called by the Mechanism 
                      Driver to obtain additional information required, 
                      such as additional credentials. 

"Mechanism Driver" not defined in this document.


39. Section 2.6.8 connect (default port)

      port           Contains the TCP port number to connect to or 
                      contact. The default LDAP port is 389. "port" is 
                      ignored for any host identifier which includes a 
                      colon and port number. 

How do you specify the default port? Wording from 2.41.1 seems more
appropriate.

      port           Port number for LDAP server (use 
                      LDAPConnection.DEFAULT_PORT for default port). 

40. Section 2.6.8 connect (host/port specification)

    host              Each host 
                      identifier may include a trailing colon and port 
                      number.  
and
      port           ... "port" is 
                      ignored for any host identifier which includes a 
                      colon and port number. 

This is an odd feature. It is also inconsistent with the Java
platform's networking library. Since both host and port are already
separate arguments, it seems cleaner to leave them separate.

Same comment applies to Section 2.41.1.

41. Section 2.6.8 connect (IPv6)

      host           Contains a host identifier consisting of a 
                      hostname, an IPv4 dotted string, or an IPv6 
                      reference [IPv6] representing the IP address of
                      host running an LDAP server to connect to. 

         "[FEDC:BA98:7654:3210:FEDC:BA98:7654:3210]:4389" 

The format for literal IPv6 addresses is described in RFC 2373.  RFC
2732 describes the format for literal IPv6 addresses in URLs.  'host'
here is being used in the context of a host address, not in the
context of a URL. Therefore, the reference should be to RFC 2373 and
the example shown should be without the '[' and ']' (and port, See 40).

42. Section 2.6.9 delete 

      dn             Distinguished name of the entry to modify. 

Should be 'the entry to delete'.


43. Section 2.6.10 disconnect 

   Disconnects from the LDAP server. The API implementation sends an 
   Unbind request to the server with any controls specified by the 
   LDAPConstraints object before closing the connection. Before the 
   object can perform LDAP operations again, it MUST reconnect to the 
   server by calling either connect or bind (bind will attempt to 
   reconnect). 

This seems to contradict Section 2.6.6 clone :

   The network connection remains 
   open until all clones have disconnected or gone out of scope. Any 
   connection opened after cloning is private to the object making the 
   connection. 

Need to clarify one or the other.

44. Section 2.6.12 finalize 

First, defining finalizers in general degrades performance of the JVM
severely.  Second, the finalize() method is reserved for finalization
and should not be used as a common method name.  Finally, finalize()
should never be declared public.  To do this in a public API forces
poor implementations. If a method is needed for cleaning up the
connection that is different from disconnect, call it something else.

45. Section 2.6.14 getAuthenticationMethod 

      "none"         The current authentication state has not been 
                      established by use of the bind operation.  This 
                      is the initial state upon connect(). 

Perhaps useful to add that the method can be in "none" if the last
bind failed or if last successful bind was anonymous.


46. Section 2.6.16 getHost 

   Returns the host name of the LDAP server to which the object is or 
   was last connected, in the format originally specified. 

and Section 2.6.19 getPort 
    
   public int getPort() 
    
   Returns the port number of the LDAP server to which the object is or 
   was last connected. 

What if last connection attempt was unsuccessful? What if no
connection attempt was ever made?  For host, do you get null or empty
string? For port, what do you get?

47. Section 2.6.17 getInputStream and Section 2.6.18 getOutputStream 
    
   public InputStream getInputStream() 
    
   Returns the stream used by the connection object for receiving data 
   from the LDAP server. 

What if connect() was not called or the connection has been
disconnected? Do you get null?  Exception? 

48. Section 2.6.20 getProperty 

                      LDAP_PROPERTY_SDK ("version.sdk")  The version of 
                                                    this SDK, as a 
                                                    Float data type. 

A Float seems like an odd type to use for versioning.

                       
                      LDAP_PROPERTY_PROTOCOL ("version.protocol")  The 
                                                    highest supported 
                                                    version of the LDAP 
                                                    protocol, as a 
                                                    Float data type. 

Same comment as above.

                      LDAP_PROPERTY_SECURITY ("version.security")  A 
                                                    comma-separated 
                                                    list of the types 
                                                    of authentication 
                                                    supported, as a 
                                                    String. 

What is meant by authentication types? SASL mechanism names? Or things
like "none", "simple" or "sasl"? Need to clarify.

Are the properties available related at all to those added via
setProperty()?

Why do these property names start with "version."? 

What is the naming convention for the properties and who controls this
namespace?

49. Section 2.6.20 getProperty

                      LDAP_PROPERTY_SDK ("version.sdk")  The version of 
                                                    this SDK, as a 
                                                    Float data type. 
                       
Why not use the versioning facilities in java.lang.Package?


50. Section 2.6.21 getProtocolVersion 

   public int getProtocolVersion () 
    
   Returns the protocol version that the connection is bound to (2 or 
   3). 

How come this returns an int but LDAP_PROPERTY_PROTOCOL returns a
Float?  

What is returned if connection is not bound?


51. Section 2.6.27 isBound 
    
   public boolean isBound() 
    
   Indicates whether the object has authenticated to the connected LDAP 
   server (other than anonymously with simple bind). It returns false 
   initially, false upon a bind request, and true after successful 
   completion of the last outstanding non-anonymous simple bind. 

What about last successful SASL bind?  A better name for this method
might be isAuthenticated(), as indicated by the method description.

52. Section 2.6.30 modify 

   The application is responsible for specifying attribute values which 
   are valid according to the syntax defined for the attributes. 

Otherwise? Why single this out? What about validity of other
arguments?


53. setSearchConstraints still shows up in the following 2 sections.

Section 2.6.6 clone 

Section 2.33 public class LDAPSearchConstraints 
                  extends LDAPConstraints 
    
   A set of options to control a search operation. There is always an 
   LDAPSearchConstraints associated with an LDAPConnection object; its 
   values can be changed with LDAPConnection.setSearchConstraints, or 
   overridden by passing an LDAPConstraints object to the search 
   operation. 

54. Section 2.6.15 getConstraints 
    
   public LDAPConstraints getConstraints() 
    
   Returns a copy of the set of constraints associated with this 
   connection. These constraints apply to all operations performed 
   through this connection (unless a different set of constraints is 
   specified when calling an operation method). 

What if none was set? Do you get null or a copy of a default
LDAPConstraints?

Same comment for Section 2.6.25 getSearchConstraints.

55. Section 2.6.35 setConstraints 
    
   public void setConstraints(LDAPConstraints cons) 
    
   Sets the constraints that apply to all operations performed through 
   this connection (unless a different set of constraints is specified 
   when calling an operation method). An LDAPSearchConstraints object 
   which is passed to this method will override all constraints, while 
   an LDAPConstraints object will only affect the base constraints. 

Can I pass in null to clear any previous set constraints? Or must cons
be nonnull?

56. Section 2.6.11 extendedOperation 

      op             Object which contains an object identifier of the 
                      extended operation, which SHOULD be one 
                      recognized by the particular server this client 
                      is connected to, and  operation-specific sequence 
                      of Octet String or BER-encoded value(s). 

At the API level, there should be no requirement that the operation is
recognized by the server.  Even RFC 2251 doesn't go that far.  


57. Section 2.6.33 rename 

newParentdn -> newParentDn for consistent use of capitalization.

   Renames an existing entry or subtree in the directory, possibly 
   repositioning it in the directory tree. 
    
   Parameters are: 
    
      dn             Current distinguished name of the entry. 

Method description doesn't say what happens when newParentDn is
omitted. Need to specify.

58. Section 2.6.36 setInputStream 
    
   public void setInputStream(InputStream stream) 
    
   Sets the stream used by the connection object for receiving data from 
   the LDAP server. 
    
and Section 2.6.37 setOutputStream 
    
   public void setOutputStream(OutputStream stream) 
    
   Sets the stream used by the connection object to send data to the 
   LDAP server. 


First, these are low-level service-provider methods that should not be
mixed up with user-level methods in the same class.  These are
dangerous methods that should not be used lightly.  Second, you need
to describe the purpose of these methods other than what it does.
There is a brief mention of its use in the change log, but that's not
part of the main spec. Third, you need to describe the implications of
using these methods.  What happens to the async listeners when you
invoke either of these?

Similarly, getOutputStream() and getInputStream() also seem like they
don't belong in a user-level API.


59. Section 2.6.38 setProperty 
    
   public void setProperty(String name, Object value) 
        throws LDAPException 
    
   Sets a property of a connection object. 
    
   No property names have been defined at this time, but the mechanism 
   is in place in order to support revisional as well as dynamic 
   extensions to operation modifiers. 

It seems to be jumping the gun to define an API for which there is
currently no use.  Also, need to explain how are properties are
related to "client controls"? When should you use which to control
client-side behavior?

60. Section 2.6.40 setSocketFactory 

Can this be called more than once? Do you need appropriate permissions to
do this?

61. Section 2.6.41 startTLS 
    
There is no support for the client to stop TLS and downgrade to a
plain unauthenticated LDAP connection. A major advantage of the Start
TLS extension (instead of just LDAP over SSL) is the client's ability
to multiplex encrypted and clear requests on the same
connection. Despite the lack of current server support, the API should
not be deficient.

62. Section 2.7.9 setReferralFollowing 

   ... Referrals of any type 
   other than to an LDAP server (i.e. a referral URL other than 
   ldap://something) are ignored on automatic referral following. The 
   default is false. 

Is this true also if you setReferralHandler(LDAPBind)?  When you use
LDAPBind, aren't you given all of the URLs regardless of their scheme?


63. Section 2.7.2 getClientControls 
    
   public LDAPControl[] getClientControls()  
    
   Returns controls to be used by the interface.

and Section 2.7.7 setClientControls 
    
   Sets controls for use by the interface.  

Not clear what 'used by the interface' means. An interface is a
declaration.  Perhaps you mean "the implementation of the Java LDAP
API".  The whole concept of "client controls" is confusing. See 64.

64. Section 3.1 Client and Server Controls 
    
   LDAPv3 operations can be extended through the use of controls. 
   Controls MAY be sent to a server or returned to the client with any 
   LDAP message. These controls are referred to as server controls. The 
   LDAP API also supports a client-side extension mechanism through the 
   use of client controls (these controls affect the behavior of the 
   LDAP API only and are never sent to a server). A common class is used 
   to represent both types of controls - LDAPControl. 

The API is inventing new terms and concepts that don't mesh well with
those already defined in RFC 2251. "Control" has a very specific
meaning in RFC 2251. It maps to server controls as described in this
document. The API invents "client controls", using the same structure
to modify behavior of the client library. There are several problems
with this. First, it has a very clumsy structure (Section 2.8.1)

   public LDAPControl(String id, 
                      boolean critical, 
                      byte[] value) 

This structure is appropriate for server controls because it fits well
within the RFC 2251 framework. For client controls, why force control
values to fit within a byte[]? If byte[] won't' be used for client
controls, then why use LDAPControl? Is there a common encoding for
byte[] value that makes it useful for client controls?

Second, what is the relationship between a property and a client control?
Section 2.6.38 setProperty says:

   No property names have been defined at this time, but the mechanism 
   is in place in order to support revisional as well as dynamic 
   extensions to operation modifiers. 

It seems the properties could be used as operation modifiers.  Is that
what client controls are for too? When should you use which?

Third, are there examples of client controls? Are they standard?

65. Section 2.7.7 setClientControls 
    
   public void setClientControls( LDAPControl control )  
    
   public void setClientControls( LDAPControl[] controls )  

   Sets controls for use by the interface.  

What if the client control is unsupported?  Does this method throw an
exception? Or does LDAPConnection throw an exception somewhere down
the line when it tries to use the client controls?

Why is the first method name plural when the argument is singular?

66. Section 2.7.11 setServerControls 
    
   public void setServerControls( LDAPControl control )  

Why is the method name plural when the argument is singular?

67. LDAPConstraints.getReferralHandler() was removed.

How can the LDAPConnection implementation get the LDAPReferralHandler
from the LDAPConstraints/LDAPSearchContraints argument to handle
referrals?

68. Section 2.7.10 setReferralHandler 
    
   public void setReferralHandler (LDAPReferralHandler binder) 
    
   Specifies the object that will process authentication requests. The 
   default is null. 

and Section 2.7.1 Constructors

   public LDAPConstraints(int msLimit, 
                          boolean doReferrals, 
                          LDAPReferralHandler binder, 
                          int hop_limit) 


Poor choice of name for LDAPReferralHandler interface leads to
confusion about exactly what it is. 'binder' is a handler?

69. Poor descriptions of LDAPReferralHandler.

Section 1.4:

   LDAPReferralHandler        Interface that is a shared ancestor to 
                              LDAPBind and LDAPRebind.  

And in 2.7.10 setReferralHandler 
    
   public void setReferralHandler (LDAPReferralHandler binder) 
    
   Specifies the object that will process authentication requests. The 
   default is null. 
    
   Parameters are: 
    
      binder         An object that implements LDAPBind or LDAPRebind. 


These descriptions need to be made more useful instead of just
describing the class hierarchy. What is its exact purpose? To
customize referral handling? To customize authentication done during
referral handling?  Describing its intended use would be more useful.
If its purpose is to process authentication requests, why is it called
LDAPReferralHandler? Probably both the interface name and the
descriptions need work.

70. LDAPBind and LDAPRebind, LDAPRebindAuth

Section 2.4 public interface LDAPBind
Section 2.25 public interface LDAPRebind
Section 2.26 public class LDAPRebindAuth

These interface and class names give a poor indication of their
function.

LDAPRebindAuth: Why not use java.net.PasswordAuthentication?  The
PasswordAuthentication class is a {String, char[]} pair. Password is
best represented as a char[] so that you can zero it out, instead of
being stored in an immutable entity like a String. Using String for
the password contradicts Section 4, which says:

   Implementations of this API SHOULD be cautious when handling 
   authentication credentials. In particular, keeping long-lived copies 
   of credentials without the application's knowledge is discouraged. 

The name LDAPRebind is not at all indicative of the purpose of this
interface. How about:
	
LDAPRebind -> LdapAuthenticator

Similarly, LDAPBind is a poor choice of name. Seems like it is related
to LDAPRebind except performing the bind the first time. But that's
not what the interface is for. How about:

LDAPBind -> LdapConnectionReferralHandler


71. Section 2.8.1 Constructors 

      critical       If true, the LDAP operation will fail with 
                      UNAVAILABLE_CRITICAL_EXTENSION if the server does 
                      not support this Control. 

      value          Control-specific data. 

The description of the 'critical' parameter cannot be used if you want
LDAPControl to represent both client and server controls.

What is the format of value? Same comment as 73.


72. Section 2.8.6 register  
    
   public static void register(String oid, Class controlClass) 

Do you need appropriate permissions to do this?  Explicit use of Class
means that you're forcing the caller to deal with class loading
issues.

      controlClass   A class which can instantiate an LDAPControl. 

A factory can also instantiate an LDAPControl, but you had a stricter
requirement earlier:

      The controlClass MUST be an extension of LDAPControl. 

Need to clarify argument description.

73. Section 2.8.4 getValue 
    
   public byte[] getValue() 
    
   Returns the control-specific data of the object. 

and Section 2.8.7 setValue 
    
   protected void setValue(byte[] value) 

   Sets the control-specific data of the object. This method is for use 
   by extensions of LDAPControl. 

What is the format of value? Is it in ASN.1 BER encoding, ready to
be sent as is to the server? Or is it an octet string that the Java
LDAP API implementation must encode using ASN.1 BER, by adding in the
tag and length.

This needs to be specified because it is the contract between the
developer of the control and the developer of the LDAP client library,
which might be different entities.

74. Section 3.1 Client and Server Controls 

   Support for specific controls is defined in a package "controls" 
   subordinate to the main LDAP package. 

Support for controls may be defined in any package (user-defined,
vendor-supplied, etc), not just those in the "controls"
subpackage. Perhaps standard controls might be defined in a "controls"
subpackage but why even mention it here, especially since the other
package hasn't been standardized?

75. Section 2.1.2 addValue (both forms): How are duplicates handled?


76. Section 2.1.4 getByteValues 
    
   public Enumeration getByteValues() 
    
   Returns an enumerator for the values of the attribute in byte[] 
   format. 

and Section 2.1.5 getByteValueArray 
 
   public byte[][] getByteValueArray() 
    
   Returns the values of the attribute as an array of byte[]. 

What if the values are of type String? What encoding is used for the
conversion?  UTF-8 won't work for v2.

77. Section 2.1.13 removeValue 
    
   public void removeValue(String attrString) 
    
   Removes a string value from the attribute. 

What if attribute values are byte[]? How is equality of attribute
values determined? What if the value does not exist?  Need to specify.

   public void removeValue(byte[] attrBytes) 
    
   Removes a byte[]-formatted value from the attribute. 

What if attribute value is a String? How is equality of attribute
values determined? What if the value does not exist? Need to specify.

78. Section 2.1.8 getStringValueArray 
    
   public String[] getStringValueArray() 
    
   Returns the values of the attribute as an array of Strings. This 
   method should only be called if the attribute values are known to be 
   strings with values restricted to UTF-8. The returned Strings have 
   undefined values if the attribute contains values not restricted to 
   UTF-8. 
    
and Section 2.1.9 getStringValues 
    
   public Enumeration getStringValues() 
    
   Returns an enumerator for the string values of an attribute. This 
   method should only be called if the attribute values are known to be 
   strings with values restricted to UTF-8. The returned Strings have 
   undefined values if the attribute contains values not restricted to 
   UTF-8. 

How do I know whether a string has an undefined value?  Do you mean
the String is null or does it contain funny characters (by inspection)?

79. Section 2.1 public class LDAPAttribute 

This class allows you to get the attr values back as either String or
byte[] but there appears to be no way of finding out what was the
intended presentation format (String or binary). Suppose I'm writing a
generic browser and reads some attributes from the directory. What is
the recommended approach to display as much of the attribute values as
strings as possible and indicate that others are binary?

80. None of the classes are Serializable. Some data types, such as
LdapAttribute, LdapAttributeSet, and the schema classes seem like they
would benefit from being Serializable.

81. Why isn't LDAPAttribute Cloneable? It has a constructor that
accepts an LDAPAttribute. Why do that instead of making it cloneable?
Why use a different pattern than LDAPAttributeSet?

82. Incorrect/unresolved citations

Section 2.6.41 startTLS 
   Begin using the Transport Layer Security (TLS) protocol for session 
   privacy [10].

Section 2.29.4 getResultCode 
   Returns the result code in a server response, as defined in [5]. 

Section 2.11.2 escapeRDN 
   Returns the RDN after escaping the characters requiring escaping [4]. 

83. 'raw' descriptions

Section 2.2.1 Constructors 

   public LDAPAttributeSchema(String raw) 
    
   Constructs an attribute definition from the raw String value returned 
   on a Directory query for "attributetypes". 

You probably mean a single attribute value from the "attributetypes"
attribute.  But the string value might not have been gotten from a
directory query.  Better to just say that 'raw' is a string encoded
using the AttributeTypeDescription syntax [ATTR].

Similar comments apply to the following constructors:

Section 2.9.1 Constructors 
   public LDAPDITContentRuleSchema(String raw) 

   Constructs a DIT content rule from the raw String value returned on a 
   schema query for "dITContentRules". 

Section 2.10.1 Constructors 
   public LDAPDITStructureRuleSchema(String raw) 
    
   Constructs a DIT structure rule from the raw String value returned on 
   a schema query for "dITStructureRules". 

Section 2.18.1 Constructors 
   public LDAPMatchingRuleSchema(String rawMatchingRule, 
                                 String rawMatchingRuleUse) 
    
   Constructs a matching rule definition from the raw String values 
   returned on a Directory query for "matchingRule" and for 
   "matchingRuleUse" for the same rule. 

Section 2.19.1 Constructors 
   public LDAPMatchingRuleUseSchema(String raw) 
    
   Constructs a matching rule use definition from the raw String value 
   returned on a schema query for "matchingRuleUse". 

Section 2.23.1 Constructors 
   public LDAPNameFormSchema(String raw) 
    
   Constructs a DIT content rule from the raw String value returned on a 
   schema query for "nameForms". 

Section 2.24.1 Constructors 
   public LDAPObjectClassSchema(String raw) 
    
   Constructs an object class definition from the raw String value 
   returned on a Directory query for "objectclasses". 

Section 2.39.1 Constructors 
   public LDAPSyntaxSchema(String raw) 
    
   Constructs a syntax from the raw String value returned on a schema 
   query for "LDAPSyntaxes". 

84. 2.9.1 Constructors 
    
   public LDAPDITContentRuleSchema(String name, 
                                   String oid, 
                                   String description, 
                                   boolean obsolete, 
                                   String[] auxiliary, 
                                   String[] required, 
                                   String[] optional, 
                                   String[] precluded, 
                                   String[] aliases) 

Which ones of these arguments are optional?  The descriptions seem to
say that the following are optional:
   description
   aliases

RFC 2252 says that the following are optional:
   name/aliases
   description, 
   obsolete, 
   auxiliary, 
   required, 
   optional, 
   precluded, 

Need to clarify in the descriptions which are optional.  If conforming
to RFC 2252, then need to specify whether the corresponding methods
(such as getName()) can return null.

85. Section 2.10.1 Constructors 
    
   public LDAPDITStructureRuleSchema(String name, 
                                     int ruleID, 
                                     String description, 
                                     boolean obsolete, 
                                     String nameForm, 
                                     String[] superiorIDs, 
                                     String[] aliases) 

Which ones of these arguments are optional?  The descriptions seem to
say that the following are optional:
     description
     superiorIDs
     aliases

RFC 2252 says that the following are optional:
	name/aliases
	description
        superiorIDs
	obsolete

Need to clarify in the descriptions which are optional.  If conforming
to RFC 2252, then need to specify whether the corresponding methods
(such as getName()) can return null.

86. Section 2.10.2 getNameForm 
    
   public String getNameForm() 
    
   Returns the NameForm that this structure rule controls. You can get 
   the actual object class that this structure rule controls by calling 
   getNameForm().getObjectClass(). 

The method returns a String, not the NameForm. The last sentence needs
correction, such as

	ldapSchema.getNameForm(ditStructRule.getNameForm()).getObjectClass().

87. Section 2.11 public class LDAPDN 
    
   A utility class representing a distinguished name (DN). 

First, poor choice of capitalization.  Second, this class does not
represent a distinguished name. It is a utility class for manipulating
distinguished names.

88. Section 2.12.2 getAttribute 

   public LDAPAttribute getAttribute(String attrName) 

Section 2.12.3 getAttributeSet 

   public LDAPAttributeSet getAttributeSet(String subtype) 

These are redundant with 2.3.5 getAttribute and 2.3.7 getSubset.
LDAPAtrributeSet has a superset of similar methods for doing this. Why
duplicate a subset here?

89. Section 2.13 public interface LDAPEntryComparator

Why doesn't this method extend from java.util.Comparator?  In fact,
why is this interface necessary at all? Why doesn't
LDAPCompareAttrNames just implement Comparator? Using Comparator would
make available all of the sorting and searching utilities available in
the Java Collections framework. Also, having

   public int compare(Object o1, Object o2)

is a lot more flexible than 

   public boolean isGreater(LDAPEntry entry1, LDAPEntry entry2) 

90. Section 2.14 public class LDAPException 
                  extends Exception 
    
   Thrown to indicate that an error has occurred. An LDAPException can 
   result from physical problems (such as network errors) as well as 
   problems with LDAP operations (for example, if the LDAP add operation 
   fails because an entry already exists with the DN of the entry to be 
   added). 

And also due to "local errors"?

91. Section 2.14.2 errorCodeToString 
    
Section 2.14.5 getLDAPResultCode 
    
One method says "errorCode", the other says "LDAPResultCode".  Do you
mean the same thing? If so, use the same names.


92. Section 2.14.4 getLDAPErrorMessage 
    
   public String getLDAPErrorMessage() 
    
   Returns the error message, if this message is available (that is, if 
   this message was set). If the message was not set, this method 
   returns null. 

How is this set? via the 'message' argument to the constructor?  How
is this different from the result of Throwable.getMessage()?  The
description doesn't mention anything LDAP-specific, so why is the
method named getLDAPErrorMessage()? 

93. Section 2.14.7 toString 
    
   public String toString() 

   Overrides the default toString implementation. It expands all the 
   nested exceptions. 

Don't quite know what "expanding the nested exceptions" mean. Does it
mean showing the class names of the nested exception, if any? In which
order?

94. Section 2.14.2 errorCodeToString 

   public static String errorCodeToString( int code ) 
    
   Returns a String representing an arbitrary result code in the default 
   Locale, or null if there is no such code known to the API. 

   public static String errorCodeToString( int code, Locale locale ) 
    
   Returns a String representing an arbitrary result code in the 
   specified Locale, or null if there is no such code or if a String 
   representation is not available for the requested Locale. 

Not an arbitrary result code, but the specified result code.

95. Section 2.14.5 getLDAPResultCode 
    
   public int getLDAPResultCode() 

Why 'LDAPResultCode' and not simply 'ResultCode'?  Some error codes
(such as NO_MEMORY, USER_CANCELLED) might not even be LDAP related.


96. Section 2.15.1 Constructors 
    
   public LDAPExtendedOperation(String oid, 
                                byte[] value) 
    
      value           The operation-specific data of the operation. 

Section 2.15.3 getValue 
    
   public byte[] getValue() 
    
   Returns the operation-specific data (not a copy, a reference). 

and Section 2.15.4 setValue 
    
   protected void setValue(byte[] value) 

Same comment as 73.

97. Section 2.16.2 getValue 
    
   public byte[] getValue() 
    
   Returns the raw bytes of the value part of the response. 

Similar comment as 73.

98. Section 2.16 public class LDAPExtendedResponse 
                  extends LDAPResponse 
    
   An LDAPExtendedResponse object encapsulates a server response to an 
   asynchronous extended operation request. 

and Section 1.5 Classes

   LDAPExtendedResponse       The response returned by an LDAP server 
                              on an asynchronous extended operation 
                              request. It extends LDAPResponse. 

The extended operation need not be asynchronous.  For example, it is
returned by (Section 2.6.11)
    
   public LDAPExtendedResponse extendedOperation( 
                                   LDAPExtendedOperation op ) 
                                   throws LDAPException 
    
   public LDAPExtendedResponse extendedOperation( 
                                   LDAPExtendedOperation op, 
                                   LDAPConstraints cons ) 
                                   throws LDAPException 

which are synchronous forms of extendedOperation.


99. Section 2.17 public interface LDAPListener 
    
   Represents the message queue associated with a particular 
   asynchronous LDAP operation or operations. 

If it is a message queue, why is it called a listener?

100. Inappropriate use of the "Listener" pattern for

LDAPListener
LDAPResponseListener
LDAPSearchListener

Throughout the Java platform, interfaces that are called "Listener"
are callback interfaces that contain methods that are invoked when an
asynchronous event occurs.  The LDAP listeners, however, are not
callback interfaces but instead have an entirely different usage
pattern. Furthermore, there is an interface in this package,
LDAPUnsolicitedNotificationListener, that is a callback interface.
Having two different types of "listeners" in the same package is
confusing enough, not to mention the confusion created by one type
being different from those in the Java platform.

As 98 points out, the description of the LDAP listeners indicates the
poor name choice right away. In Section 1.3, the following sentence
appears:

   The listener is a message queue 
   associated with the request, and it is the responsibility of the 
   client to read messages out of the queue and process them. 

If the listener is a message queue, why call it a listener at all?
This incorrect use of "listener" makes the API confusing to use.


101. Section 2.17.3 isResponseReceived 

   Reports true if a response has been received from the server for a 
   particular message ID but not retrieved with getResponse. If there is 
   no outstanding operation for the message ID (or if it is zero or a 
   negative number), IllegalArgumentException is thrown. 
    
and Section 2.17.2 getResponse 

   Blocks until a response is available for a particular message ID, or 
   until all operations associated with the message ID have completed or 
   been canceled, and returns the response. If there is no outstanding 
   operation for the message ID (or if it is zero or a negative number), 
   IllegalArgumentException is thrown. 

Perhaps a better name for these methods might be hasResponse() and
nextResponse(), respectively, because they fit the descriptions and
usage pattern better.

102. Section 2.17.1 getMessageIDs 
    
   public int[] getMessageIDs() 

Didn't explain what is a message ID. Perhaps can be resolved as simply
as making a reference to Section 4.1.1.1 of RFC 2251. Similar comment
for methods in LDAPListener, LDAPMessage and the
LDAPConnection.abandon() method.

103. Section 2.17.4 merge 
    
   public void merge(LDAPListener listener2) 
    
   Merges two listeners. Moves/appends the content from another            
   listener to this one. 

and Section 2.30.4 merge 
    
   public void merge(LDAPListener listener2) 

Can you merge different types of listeners (LDAPResponseListener with
LDAPSearchListener and vice versa)? What about a user-defined
LDAPListener implementation that is neither LDAPResponseListener nor
LDAPSearchListener?

Also, is this operation destructive in terms of listener2?  After the
operation, would listener2.getMessageIDs() return an empty array and
its outstanding responses be removed? Need to be more explicit.


104. Section 2.2.6 getSyntaxString 

   public String getSyntaxString() 
    
   Returns the Object Identifier of the syntax of the attribute in 
   dotted numerical format. 

Section 2.2.1 Constructors 
    
   public LDAPAttributeSchema(String name, 
                              String oid, 
                              String description, 
                              String syntaxString, ...

      syntaxString   Object Identifier of the syntax of the attribute 
                      - in dotted-decimal format. 
    
Section 2.18.1 Constructors 

      syntaxString       Object Identifier of the syntax that this 
                         matching rule is valid for - in dotted-decimal 
                         format. 

Section 2.18.3 getSyntaxString 
    
   public String getSyntaxString() 
    
   Returns the OID of the syntax that this matching rule is valid for. 

Why aren't these methods called getSyntaxOid() and the argument to the
constructors called syntaxOid instead of syntaxString?

105. Section 2.18.1 Constructors 
    
   public LDAPMatchingRuleSchema(String name, 
                                 String oid, 
                                 String description, 
                                 String[] attributes, 
                                 String syntaxString, 
                                 String[] aliases) 
    
   public LDAPMatchingRuleSchema(String name, 
                                 String oid, 
                                 String description, 
                                 boolean obsolete, 
                                 String syntaxString, 
                                 String[] aliases) 

For all of the classes that are subclasses of LDAPSchemaElement, each
has a single constructor with some optional arguments and a
constructor that accepts a raw string. Why does this class need two
similar constructors, one with 'obsolete' and the other 'attributes'.

106. 2.18.1 Constructors 

Which ones of these arguments are optional?  The descriptions seem to say that
the following are optional:
        description
	attributes
	aliases

RFC 2252 says that the following are optional:
	name/aliases,
	description,
	obsolete
	attributes

Need to clarify in the descriptions which are optional.  If conforming
to RFC 2252, then need to specify whether the corresponding methods
(such as getName()) can return null.

107. 2.19.1 Constructors 

   public LDAPMatchingRuleUseSchema(String name, 
                                    String oid, 
                                    String description, 
                                    boolean obsolete, 
                                    String[] attributes, 
                                    String[] aliases) 


Which ones of these arguments are optional?  The descriptions seem to say that
the following are optional:
	description
	aliases

RFC 2252 says that the following are optional:
	name/aliases
	description
	obsolete

Need to clarify in the descriptions which are optional.  If conforming
to RFC 2252, then need to specify whether the corresponding methods
(such as getName()) can return null.


108. Section 2.20 public class LDAPMessage
    
   Base class for asynchronous LDAP request and response messages. 

Not always async. Also used for LDAPExtendedResponse, which may be
gotten as a synchronous response.

109. Section 2.21.1 Constructors 

               ADD                      The value should be added to 
                                        the attribute 
          
               DELETE                   The value should be removed 
                                        from the attribute 
          
               REPLACE                  The value should replace all 
                                        existing values of the 
                                        attribute 

If you're going to explain these here, then make the descriptions
complete or refer to RFC 2251. For example, ADD doesn't just add a
value, it might affect the entire attribute.

110. Section 2.21.4 Constants of LDAPModification 

Same comment as 109.


111. Section 2.22 public class LDAPModificationSet 

A "set", by definition, is unordered.  Why does a set have the
following methods?

2.22.3 elementAt 
2.22.5 removeElementAt 

Also, in Section 2.22.4 remove 
    
   public void remove(String name) 
    
   Removes the first attribute with the specified name in the set of 
   modifications.

'the first' is meaningless in a set. Replace with 'an'.

If it is really a set, then it should implement the java.util.Set
interface.

Furthermore, RFC 2251 requires the modifications to be a sequence:

        ModifyRequest ::= [APPLICATION 6] SEQUENCE {
                object          LDAPDN,
                modification    SEQUENCE OF SEQUENCE {
                        operation       ENUMERATED {
                                                add     (0),
                                                delete  (1),
                                                replace (2) },
                        modification    AttributeTypeAndValues } }

It might be more efficient and simpler to eliminate
LDAPModificationSet and just use LDAPModification[].

112. Section 2.23.1 Constructors

   public LDAPNameFormSchema(String name, 
                             String oid, 
                             String description, 
                             boolean obsolete, 
                             String objectClass, 
                             String[] required, 
                             String[] optional, 
                             String[] aliases) 

Which ones of these arguments are optional?  The descriptions seem to
say that the following are optional:
	description
	aliases

RFC 2252 says that the following are optional:
	name/aliases
	description
	obsolete
	optional

Need to clarify in the descriptions which are optional.  If conforming
to RFC 2252, then need to specify whether the corresponding methods
(such as getName()) can return null.


113. Section 2.24.1 Constructors 
    
   public LDAPObjectClassSchema(String name, 
                                String oid, 
                                String[] superiors, 
                                String description, 
                                String[] required, 
                                String[] optional, 
                                int type, 
                                String[] aliases, 
                                boolean obsolete) 

Which ones of these arguments are optional?  The descriptions seem to
say that the following are optional:
	description
	aliases

RFC 2252 says that the following are optional:
	name/aliases
	desc
	obsolete
	superiors
	required
	optional
	type

Need to clarify in the descriptions which are optional.  If conforming
to RFC 2252, then need to specify whether the corresponding methods
(such as getName()) can return null.

114. Section 2.26.3 getPassword 
    
   public byte[] getPassword() 
    
   Returns the password to be used for reauthentication on automatic 
   referral following. 

What if passwd was of type String when passed into the constructor?
Is UTF-8 encoding used for the conversion always? What if you're using
LDAP v2?

115. Section 2.27.3 getReferrals 
    
   public String[] getReferrals() 
    
   Gets the list of referral URLs (LDAP URLs to other servers) returned ...

   The referral list MAY include URLs of a type other than ones for an 
   LDAP server (i.e. a referral URL other than ldap://something). 

Delete "LDAP" from the first sentence because it contradicts RFC 2251
and the last sentence in the section.

116. Section 2.28 public interface LDAPReferralHandler
    
   Shared ancestor to the two types of referal objects - LDAPBind and 
   LDAPRebind. 

Typo. referal -> referral.


117. Section 2.29 public class LDAPResponse 
                  extends LDAPMessage 
    
   Represents the response to a particular asynchronous LDAP operation. 

Not always asynchronous.

118. Section 2.31 public class LDAPSchema  (method naming)

The methods in this class has names that are confusing.  For example,
in Section 2.31.3 getAttribute:
    
   public LDAPAttributeSchema getAttribute( String name ) 

The result returned is actually an attribute schema, not an attribute.
Similarly for Section 2.31.24 getSyntax
    
   public LDAPSyntaxSchema getSyntax( String oid ) 

and Section 2.31.25 getSyntaxes 
    
   public Enumeration getSyntaxes() 
    
   Returns an enumeration of LDAPSyntaxSchema objects. 

getSyntaxSchema() and getSyntaxSchemas() seem more appropriate names.

All of the get* method would benefit by employing more appropriate
names.

119. Section 2.31 public class LDAPSchema (method parameters)
    
LDAPSchema defines several get methods based on the schema
definition's name.

   public LDAPAttributeSchema getAttribute( String name ) 
   public Enumeration getAttributeNames() 

   public LDAPDITContentRuleSchema getDITContentRule( String name ) 
   public Enumeration getDITContentRuleNames () 

   public Enumeration getDITStructureRuleNames () 

   public LDAPMatchingRuleSchema getMatchingRule( String name ) 
   public Enumeration getMatchingRuleNames() 

   public LDAPMatchingRuleUseSchema getMatchingRuleUse( String name ) 
   public Enumeration getMatchingRuleUseNames() 

   public LDAPNameFormSchema getNameForm( String name ) 
   public Enumeration getNameFormNames () 

   public LDAPObjectClassSchema getObjectClass( String name ) 
   public Enumeration getObjectClassNames() 

Yet, according to RFC 2252, the corresponding schema definitions have
the oid as mandatory and name as optional.

      AttributeTypeDescription = "(" whsp
            numericoid whsp              ; AttributeType identifier
          [ "NAME" qdescrs ]             ; name used in AttributeType
	...

      ObjectClassDescription = "(" whsp
          numericoid whsp      ; ObjectClass identifier
          [ "NAME" qdescrs ]
	...

      MatchingRuleDescription = "(" whsp
          numericoid whsp  ; MatchingRule identifier
          [ "NAME" qdescrs ]
	...

      MatchingRuleUseDescription = "(" whsp
          numericoid whsp  ; MatchingRule identifier
          [ "NAME" qdescrs ]
	...

      DITContentRuleDescription = "("
          numericoid   ; Structural ObjectClass identifier
          [ "NAME" qdescrs ]
	...

      NameFormDescription = "(" whsp
          numericoid whsp  ; NameForm identifier
          [ "NAME" qdescrs ]
	...

      DITStructureRuleDescription = "(" whsp
          ruleidentifier whsp            ; DITStructureRule identifier
          [ "NAME" qdescrs ]

Aren't the methods in LDAPSchema out of sync with RFC 2252?  Is the
only way to find a definition by OID to do a linear search?

120. Section 2.31.2 fetchSchema 

Seeing that there is a fetchSchema method in LDAPSchema, a user might
expect to find a storeSchema method in the same class but the store
methods are actually found in LDAPSchemaElement and has entirely
different names.  The LDAPSchema description should describe this, or
better yet, be re-structured so that the store methods are in the same
class.

Here's one suggestion of how to make things symmetric and more
intuitive.

Redefine the constructors as follows.

   public LDAPSchema(LDAPConnection ld) throws LDAPException 
    
   public LDAPSchema(LDAPConnection ld, String dn) throws LDAPException

Remove LDAPSchemaElement.add/LDAPSchemaElement.modify/
LDAPSchemaElement.remove/ and add the following to LDAPSchema.

   public void add(LDAPSchemaElement) throws LDAPException 
   public void modify(LDAPSchemaElement) throws LDAPException 
   public void remove(LDAPSchemaElement) throws LDAPException 

If you need a method to refresh the schema, then add

   public void refresh() throws LDAPException;

The schema from ld need not be loaded immediately when the constructor
is called. In fact, the implementation might load it completely or
even partially on demand, as required by the get* methods. The get
methods would need a throws LDAPException clause.

The result is a much cleaner design in which all of the physical
schema access and update methods are localized in one class instead of
being spread over two.


121. Section 2.31.2 fetchSchema 

   The fetchSchema methods are the only methods that interact with a 
   Directory Server. The other methods access information acquired 
   through fetchSchema. An LDAPException is thrown as for 
   LDAPConnection.search() (2.6.34) if the schema cannot be retrieved 
   with the specified connection, or if the subschemaSubentry attribute 
   used to locate the schema contains multiple values. 

The first two sentences are implementation details and should be
omitted.  The 3rd sentence describes how the schema is fetched (i.e.,
via a search) -- again, implementation detail that should be omitted.
Delete "as for LDAPConnection.search() (2.6.34)".

122. Section 2.3.2  public abstract class LDAPSchemaElement 

2.32.1 add
2.32.10 modify 
2.32.11 remove 

See 120. Should move functionality to LDAPSchema, which already deals
with the physical schema tree directly.

123. Why isn't LDAPSchemaElement Cloneable or Serializable? It would
be useful to be able to clone and then modify the copy. It would be
useful to be able to serialize schema elements to a file or other
storage medium.

124. Can these return null?

Section 2.32.2 getAliases 
    
   public String[] getAliases() 

Section 2.32.3 getDescription 
    
   public String getDescription() 

Section 2.32.4 getName 
    
   public String getName() 

Section 2.32.5 getID 
    
   public String getID() 

(LDAPDITStructureRuleSchema says getID() returns null).


125. Section 2.32.6 getQualifier 
    
   public String[] getQualifier(String name) 
    
   Returns an array of all values of a specified optional or 
   experimental qualifier of the element. This method may be used to 
   access the values of vendor-specific qualifiers (which begin with "X-
   " [ATTR]). 
    
and Section 2.32.7 getQualifierNames 
    
   public Enumeration getQualifierNames() 
    
   Returns an enumeration of all qualifiers of the element which are not 
   defined in [ATTR]. 

Does getQualifier and getQualifierNames see the same namespace? Their
method names imply so but their descriptions don't.

getQualifier is a very general name and one would expect to get any
qualifier, not just optional and experimental ones. Also, which are
optional?  Perhaps need to say that the list of optional qualifiers
varies for each type of LDAPSchemaElement and is defined in RFC 2252.
Also, if the method description is correct, then should choose a
different name for the method.

Same comments apply to Section 2.32.12 setQualifier.

The method description for getQualifierNames suggests that the
namespace is more restrictive than getQualifier's. If so, then
the method name should reflect this.


126. Section 2.32.8 getValue  (method name)
    
   public String getValue() 
    
   Returns a String in a format suitable for directly adding to a 
   Directory, as a value of the particular schema element attribute. 

Why not call this method toString()? The "value" of a schema element
is what? It seems that this method is returning the string
representation, and the typical way to do that is to use toString().
The name getValue() seems like it is equivalent to the other get
methods (such as getName(), getID()). If toString() seems too generic,
then how about toRfc2252Syntax()?


127. Section 2.32.8 getValue (method description)
    
   public String getValue() 

   Returns a String in a format suitable for directly adding to a 
   Directory, as a value of the particular schema element attribute. 

The description is too vague. In fact, this method should be abstract
and a getValue() method should be added to each subclass of
LDAPSchemaElement that clearly states the expected output of the
method. It should use the language suggested for the 'raw' parameter
descriptions. (See 83).

For example, for LDAPAttributeSchema, getValue() must return
a string encoded using the AttributeTypeDescription syntax [ATTR].


128. Section 2.7 public class LDAPConstraints 

Why isn't LDAPConstraints cloneable? That would make it easier to make
a copy of an existing LDAPConstraints and modify one or two
parameters.  This is especially useful for LDAPSearchConstraints.

129. Section 2.33.1 Constructors 

It might be useful to have an LDAPSearchConstraints constructor that
accepts a LDAPConstraints argument.

130. Section 2.33.1 Constructors 

      dereference     Specifies when aliases should be dereferenced. 
                      MUST be either DEREF_NEVER, DEREF_FINDING, 
                      DEREF_SEARCHING, or DEREF_ALWAYS (DEREF_NEVER by 
                      default). 

and Section 2.33.3 getDereference 
    
   public int getDereference() 
    
   Specifies when aliases should be dereferenced. Returns either 
   DEREF_NEVER, DEREF_FINDING, DEREF_SEARCHING, or 
   DEREF_ALWAYS. 

either -> one of

131. Section 2.34.3 isComplete 
    
   public boolean isComplete(int msgid) 
    
   Reports true if all results have been received for a particular 
   message ID. For search, it is true if a searchResultDone response has 
   been received from the server for the id. ...

LDAPSearchListener is used only for searches.  'For search' is
redundant.

132. These concrete classes do not have any explicit constructors and
no set methods to update their state. A public no-argument constructor
will be made available by default.  The user should not be creating
instances of these. These are objects returned by the Java LDAP API
implementation. They should be probably be interfaces instead of
concrete classes.

2.16 public class LDAPExtendedResponse extends LDAPResponse 

2.20 public class LDAPMessage 
    
2.29 public class LDAPResponse extends LDAPMessage 

2.35 public class LDAPSearchResult extends LDAPMessage 

2.36 public class LDAPSearchResultReference extends LDAPMessage 

2.37 public class LDAPSearchResults implements Enumeration

133. Section 2.37.5 nextElement 

   public Object nextElement() 
    
   Returns the next result in the enumeration as an Object. This the 
   default implementation of Enumeration.nextElement(). The returned 
   value may be an LDAPEntry or an LDAPException. 

Returning an exception is poor design and nonstandard.  No other API
in the Java platform uses enumerations in this way.  This design
forces the caller to check whether each element is a real answer or an
exception.  Look at the Appendix A, 8.1 example.

               LDAPSearchResults res = ld.search( MY_SEARCHBASE, ...);
    
               /* Loop on results until finished */ 
               while ( res.hasMoreElements() ) { 
    
                   /* Next directory entry */ 
                   LDAPEntry findEntry = (LDAPEntry)res.nextElement(); 

With nextElement() defined the way it is, if you follow this example,
then you'll get a ClassCastException if nextElement() returns an
LDAPException. To avoid the exception, you'd need to write something
like

                   /* Next directory entry */ 
		   Object answer = res.nextElement(); 
		   if (answer instanceof LDAPEntry) {
                       findEntry = (LDAPEntry) answer;
		   } else if (answer instanceof LDAPException) {
		       throw (LDAPException) answer;
		   }

As illustrated by this example, this API definition forces the user to
write very awkward code. 

134. Section 2.37.5 nextElement 
    
   public Object nextElement() 
    
   Returns the next result in the enumeration as an Object. This the 
   default implementation of Enumeration.nextElement(). The returned 
   value may be an LDAPEntry or an LDAPException. 

Can an LDAPException be returned midstream or is it always the last
element in the enumeration? Are some exception fatal while others not?


135. Section 2.37.1 getCount 
    
   public int getCount() 
    
   Returns a count of the entries and exceptions in the search result. 
   If the search is asynchronous (batch size not 0), this reports the 
   number of results received so far. 

LDAPSearchResults is returned only for synchronous searches.  A batch
size of nonzero is not defined in this document as making a search
'asynchronous'. Probably want to use different wording to avoid
confusion with real async searches.

Exceptions are also included in this count so the caller has no real
way of knowing how many entries there are.


136. Section 2.37.6 sort 

   This implies that 
   LDAPSearchResults.nextElement() will always block until all results 
   have been retrieved, after a sort operation. 

This also means batch size will be ignored (be effectively 0) if you
invoke sort. This method seems rather awkward. There is no
corresponding functionality in the LDAP C API.  It would seem more
flexible and appropriate to just delete this method and use the
sort/search facilities available in the Java Collections
framework. That way, you have greater control over the data structures
and algorithm.

137. Section 2.37.2 getResponseControls 
    
   public LDAPControl[] getResponseControls() 
    
   Returns the latest Server Controls returned by a Directory Server 
   in the context of this search request. 

Is this supposed to return the controls associated with each LDAPEntry
that got returned? Should it be invoked in tandem with each call to
nextElement()? Does it block in the same way nextElement() does?  Or
does it return the controls returned by the server since the last time
getResponseControls() was invoked.

138. Section 2.37.3 hasMoreElements 
    
   public boolean hasMoreElements() 
    
   Specifies whether or not there are more search results in the 
   enumeration. If true, there are more search results. 

What about exceptions? Does an exception in the enumeration count as a
"search result"?

139. Section 2.38 public interface LDAPSocketFactory.  

There is already a standard abstract class for socket factory:
javax.net.SocketFactory. javax.net.SocketFactory is available as part
of the Java Secure Socket Extension (JSSE) and also as part of the
Java 2 Platform, Standard Edition, v 1.4.  It is a general socket
factory mechanism and encompasses the functionality of
LDAPSocketFactory.

There is nothing LDAP-specific about LDAPSocketFactory.

   public Socket makeSocket(String host, int port) 
                            throws IOException, UnknownHostException 
    
Even if you decide not to use javax.net.SocketFactory, at least change
the name of this method to "createSocket" so that it is consistent
with javax.net.SocketFactory.


140. Section  2.39.1 Constructors 

   ... Note 
   that adding and removing syntaxes is not typically a supported 
   feature of LDAP servers. 

Don't know why this is here in the constructor class. Calling the
constructor won't add or remove syntaxes from LDAP servers. It might
belong in the class description or even maybe the LDAPSchema
class. This note might be applicable for other types of schema
elements as well and to schema updates in general.

141. Section 2.41.3 encode 

   Encodes an arbitrary string. Any illegal characters are encoded as 
   %HH. 

"illegal" is not the correct term to use here. This method could
benefit from a better description.

Also, not an arbitrary string, but the specified string.

142. Section 2.41.5 getAttributes 
    
   public Enumeration getAttributes() 
    
   Returns an Enumerator for the attribute names specified in the URL. 

Null OK? Or will empty enumeration be returned if there are none.

143. Section 2.41.12 getUrl 
    
   public String getUrl() 
    
   Returns a valid string representation of this LDAP URL. 

Why not call this method toString() instead of getUrl()?  The method
description seems to indicate that toString() is appropriate.

144. Section 2.41.1 Constructors 

   public LDAPUrl(String host, 
                  int port, 
                  String dn, 
                  String[] attrNames, 
                  int scope,  ...

      attrNames      Names or OIDs of attributes to retrieve.  Passing 
                      a null array signifies that all user attributes 
                      are to be retrieved. Passing a value of "*" 
                      allows you to specify that all user attributes as 
                      well as any specified operational attributes are 
                      to be retrieved. 

Can't pass a value of "*" because constructor wants a String[].

The description of "*" contradicts ALL_USER_ATTRS (Section 2.6.41).

      ALL_USER_ATTRS ("*")     Used with search in an attribute list to 
                      indicate that all attributes (other than 
                      operational attributes) are to be returned. 

145. Section 2.41.7 getExtensions 
    
   public String[] getExtensions () 
    
   Returns any LDAP URL extensions specified, or null if none are 
   specified. Each extension is a type=value expression. The =value part 
   MAY be omitted. The expression MAY be prefixed with '!' if it is 
   mandatory for evaluation of the URL. 

The only constructor that allows extensions to be specified is the one
that takes a raw URL String. Is that intended or is a String[]
extensions argument missing from the 3rd constructor.

146. Section 3.1 Client and Server Controls 

   Server controls returned to a client as part of the response to a 
   synchronous operation can be obtained with 
   LDAPConnection.getResponseControls()

And LDAPSearchResults.getResponseControls().

147. Section 3.2 Referral handling and exceptions,    

The "Synchronous requests" section often talks about exceptions being
"thrown". However, those exceptions are also just returned via
LDAPSearchResults.nextElement(), not necessarily thrown.


148. Section 3.2 Referral handling and exceptions 

   When a referral is received, the application receives an LDAPResponse 
   object with a result code of REFERRAL. If a continuation reference is 
   received the application receives an LDAPSearchResultReference 
   object. 

Not true for v2.

149. Section 3.2 Referral handling and exceptions 

Throughout this section, the document uses "the API" when "the
implementation of the Java LDAP API" would be more appropriate.

Two occurrences of "an LDAPReferral exception" should be changed to
"an LDAPReferralException".


150. Section 3.5 Invalid responses 
    
   If a message is received by an API implementation from the server and 
   the message cannot be interpreted as an LDAP PDU, an LDAPException 
   MUST be thrown with a result code of INVALID_RESPONSE. 

Same comment as 147.

151 Section 3.4 Level of compatibility  (package name)
    
   Implementations ... and MUST be 
   implemented with the package name org.ietf.ldap.

The package name should be specified in Section 1 or 2, not in Section
3, under Implementation Considerations.  The package name is not an
implementation consideration, it is part of the API specification,
just like a class name or a method name.

152. Section 3.4 Level of compatibility
    
   Implementations of the API are to be binary compatible, and ...

Binary compatibility of Java programs is defined by the Java
Programming Language and the Java Virtual Machine
Specifications. Don't know the value added by the first sentence.

Might be more useful to specify which version of the Java Platform and
which Optional Packages this API depends on. There is a dependency on
JAAS (javax.security.auth.callback.*).


153. Section 8 Appendix A - Sample java LDAP programs

All code samples should follow the Code Conventions for the Java
Programming Language, available at

http://java.sun.com/docs/codeconv/


154. Section 8.1

               /* Fetch the schema (anonymous is okay for reading) */ 

This comment is not true for Active Directory: you need to be
authenticated to read the schema. 


155. Section 8.1, ShowSchema example

               ); 

               /* What are the possible attributes for "person"? */ 
               LDAPObjectClassSchema o = 
                                 schema.getObjectClass( "person" ); 
               if ( o != null ) { 
                  Enumeration required = o.getRequiredAttributes(); 
                  Enumeration optional = o.getOptionalAttributes(); 
                  System.out.println( 
                              "Required attributes for person:" ); 
                  while( required.hasMoreElements() ) 
                      System.out.println( "  " + 
                              (String)required.nextElement() ); 
                  System.out.println( 
                              "Optional attributes for person:" ); 
                  while(optional.hasMoreElements() ) 
                      System.out.println( "  " + 
                              (String) optional.nextElement() ); 
               ); 

LDAPObjectClassSchema.getRequiredAttributes() and
LDAPObjectClassSchema.getOptionalAttributes() return String[], not
Enumeration, and the subsequent while loops need to be changed
accordingly.

The first and last line of code sample shown should be "}", not ");".

156. Section 8 Appendix

The ModifyEmail example on page 124 is supposed to be asynchronous
but it shows a synchronous usage of "modify" and "bind".

157. Section 2.11.5 isValid 

   Returns true if the string conforms to distinguished name syntax 
   (section 3 of [DN]. It MAY return true also if the string conforms to 
   section 4 of [DN]. 

Missing closing parens.

158. Section 2.14.8 Result codes 

The meanings of the result codes not covered by RFC 2251 need to be
stated explicitly.

        AMBIGUOUS_RESPONSE (0x65) 
        AUTH_UNKNOWN (0x56) 
        CLIENT_LOOP  (0x60) 
        CONNECT_ERROR (0x5b) 
        CONTROL_NOT_FOUND (0x5d) 
        DECODING_ERROR (0x54) 
        ENCODING_ERROR (0x53) 
        FILTER_ERROR (0x57) 
        INVALID_RESPONSE (0x64) 
        LOCAL_ERROR (0x52) 
        LDAP_NOT_SUPPORTED (0x5c) 
        LDAP_TIMEOUT (0x55) 
        MORE_RESULTS_TO_RETURN (0x5f) 
        NO_MEMORY (0x5a) 
        NO_RESULTS_RETURNED (0x5e) 
        REFERRAL_LIMIT_EXCEEDED (0x61) 
        SERVER_DOWN (0x51) 
        USER_CANCELLED (0x58) 
        TLS_NOT_SUPPORTED (0x70) 


Also, isn't NO_MEMORY already covered by the VM error
OutOfMemoryError?