Mailing-List: contact dev-help@directory.apache.org; run by ezmlm
Precedence: bulk
Reply-To: "Apache Directory Developers List" <dev@directory.apache.org>
Subject: Re: LDAPConnection fixes
To: api@directory.apache.org,
 Apache Directory Developers List <dev@directory.apache.org>
References: <CAG8=FRhzGE8iQAqzDxrbvZB9=Xj1_KRqLnPJ_a-YsGvkaf543A@mail.gmail.com>
From: Stefan Seelmann <mail@stefan-seelmann.de>
Message-ID: <9095a1a3-e24c-3fc9-5005-ba925fc5a6c6@stefan-seelmann.de>
Date: Sat, 15 Oct 2016 15:21:54 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101
 Thunderbird/45.4.0
MIME-Version: 1.0
In-Reply-To: <CAG8=FRhzGE8iQAqzDxrbvZB9=Xj1_KRqLnPJ_a-YsGvkaf543A@mail.gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8bit
archived-at: Sat, 15 Oct 2016 13:22:19 -0000

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