Re: LDAPConnection fixes

[Stefan Seelmann <ma...@stefan-seelmann.de>]

On 10/14/2016 08:06 PM, Emmanuel Lecharny wrote:
> Hi !
> 
> Stefan has added some TODO in the LdapConnection interface :
> 
> // TODO: all the SASL bind methods are not declared in this interface, but
> implemented in LdapNetworkConnection. Is that intended?
> // TODO: why does connect() return a boolean? What is the difference
> between false and an Exception?
> // TODO: think about usage of abbrevisions (Dn/Rdn) vs. spelled out
> (relative distinguished name) in javadoc
> // TODO: describe better which type of LdapException are thrown in which
> case?
> // TODO: remove the "we" language in javadoc
> // TODO: does method getCodecService() belong into the interface? It
> returns a LdapApiService, should it be renamed?
> // TODO: does method doesFutureExistFor() belong into the interface? Move
> to LdapAsyncConnection?
> 
> I have fixed some of them :

Great, thanks Emmanuel!

> - Replaced teh Dn/Rdn abbreviations by the full name
> - Removed the 'we' all over the javadoc
> - Replaced the doesFutureExistFor() method by a more explicit
> isRequestCompleted() method. It's not a pure async method, it's also useful
> when one want to abandon a request that takes too long.
> - The connect() method returns a boolean to be able to distinguish between
> a problem while establishing a connection, and a simple timeout because teh
> remote host does not answer. We do have a retry mechanism in this case, and
> when we reach the number of possible retries, we simply return 'false'. Of
> course, when we have some more problematic problem, an exception is thrown.

Hm, but as a user of the API I just want to connect, I have to handle
both, the return value and the exception. When I get an Exception I can
see the reason (host not found, invalid credetials, etc). But If I just
get a "false" I don't know any details. I'd prefer to get an exception.

> Regarding the few oher TODOs :
> 
> - We have to add those SASL methods

Agreed

> - LDAPException : yes, it would be useful to list specific excpetions
> instead of generic ones

I'd say it's optional to refine them.

> - getCodecService() : this is a problem. the LdapApiService means nothing
> for those who haven't wrote the API? CodecService is slightly more
> significant. This method returns the container that exposes the supported
> codec and extended operation. Renaming it to getServerFeatures() would be
> better, but we should also rename the LdapApiService (something like
> ServerFeatures would probably be clearer ?)

CodecService sounds ok to me.

But, I wonder if this service needs to be exposed via the LdapConnection
interface at all? Does a user of the LDAP API ever need it? I guess only
if she want to register new controls or extended operations
programatically, but that can also be done by setting system properties

Kind Regards,
Stefan