Re: LDAPConnection fixes
On Sun, Oct 16, 2016 at 7:41 PM, Radovan Semancik < radovan.seman...@evolveum.com> wrote: > Hi, > > On 10/14/2016 08:06 PM, Emmanuel Lecharny wrote: > >> - 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. >> > > I actually never liked this approach as it makes proper error reporting > difficult. I would prefer to just throw good exception (now there is also > timeout exception). Good exceptions can help new users to use the API > somehow "safer". It is more intuitive. I think that not many people would > think about checking the return value. So they'll just go on and the next > attempt to use the connection would fail with an error which is not very > meaningful. The root cause of the problem is often lost or difficult to > find. > > But I think this is similar at many places of the API and we need to have > a serious look at error handling in general. E.g. I would like to avoid the > API to log every schema error to logs at high log level even if in quirks > mode. That makes working with AD really painful ... > > But I see these issues as something for 2.0. Fixing this properly is very > likely to break compatibility. > Indeed. I was able to rename a method, deprecating the badly named one, but here, the only possible solution would be to throw an exception instead of returning 'false', but we should kep the method returning a boolean until a 2.0 is available. That would change the behavior, but ot in a disruptive way, IMO. > - LDAPException : yes, it would be useful to list specific excpetions >> instead of generic ones >> > > Yes. Yes. Yes. That's another thing that. But that can wait. Let's have > the 1.0 release out so we can start fixing things properly. > Agreed. Thanks Radovan.
Re: LDAPConnection fixes
Hi, On 10/14/2016 08:06 PM, Emmanuel Lecharny wrote: - 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. I actually never liked this approach as it makes proper error reporting difficult. I would prefer to just throw good exception (now there is also timeout exception). Good exceptions can help new users to use the API somehow "safer". It is more intuitive. I think that not many people would think about checking the return value. So they'll just go on and the next attempt to use the connection would fail with an error which is not very meaningful. The root cause of the problem is often lost or difficult to find. But I think this is similar at many places of the API and we need to have a serious look at error handling in general. E.g. I would like to avoid the API to log every schema error to logs at high log level even if in quirks mode. That makes working with AD really painful ... But I see these issues as something for 2.0. Fixing this properly is very likely to break compatibility. - LDAPException : yes, it would be useful to list specific excpetions instead of generic ones Yes. Yes. Yes. That's another thing that. But that can wait. Let's have the 1.0 release out so we can start fixing things properly. -- Radovan Semancik Software Architect evolveum.com
Re: LDAPConnection fixes
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
LDAPConnection fixes
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 : - 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. Regarding the few oher TODOs : - We have to add those SASL methods - LDAPException : yes, it would be useful to list specific excpetions instead of generic ones - 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 ?) Thoughts ? -- Regards, Cordialement, Emmanuel Lécharny www.iktek.com
