This is a feature tracking bug for searching LDAP directories in the address book window.

Please get it in if possible - this is the only thing which is preventing a lot of us from moving full-time to Mozilla mailnews from Netscape 4.x mailnews. Thanks.

Is the LDAP server you want to use part of a corporate intranet? Or is it a public server out on the internet somewhere?

The former, and inside the firewall.

P4 per m/n staff mtg today

Moving all P4s out of beta2 to M17.

Phil found a way to weasel out of owning this bug. Reassigning.

[LDAP] in summary

LDAP to M30, nobody, helpwanted

Changing qa assigned to myself.

reassign to component owner for re-consideration for mozilla 1.0 (or sooner)... Here are some comments from Steve Harman on 11/16/2000 at http://www.macintouch.com/netscape6.html: I'll keep this one short - WHEN DID NETSCAPE DROP LDAP SUPPORT ?? My guess is somewhere between 4.76 and 6.0. Unless we're all being really, really simple this end, we can't find any mention of setting up a Directory for obtaining e-mail addresses inside Netscape 6.0. 4.76 came with Lycos, BigFoot and several other directories pre-configured but 6.0 seems to have none. As a large corporate with 250+ Macs we RELY on a central [Mac based] LDAP directory system for our users to obtain addresses from. Can you imagine keeping 250+ individual address books up to date? Please please please - someone tell me we've missed the 'tick here for LDAP' option... Regards, Steve

reassigning to chuang.

*** Bug 66631 has been marked as a duplicate of this bug. ***

Assign it to myself.

Oop! It belongs to Hong Kwon's group

This looks like an ideal bug to land any Address Book LDAP enhancements with. Dan, if you want to shepherd the landing do you want to assign this to yourself?

reassigning to dmose

Setting target milestone to 0.9.3 (check it in anytime, even before, when the tree is open for). Per PDT triage.

The code to do the basic work here is in the patch in 78933. According to some mail I have from Martin, the code relevant to this bug are the following new files: addrbook/src/nsAbLDAPDirFactory.cpp addrbook/src/nsAbLDAPDirFactory.h addrbook/src/nsAbLDAPDirectory.cpp addrbook/src/nsAbLDAPDirectory.h addrbook/src/nsAbLDAPCard.cpp addrbook/src/nsAbLDAPCard.h addrbook/src/nsAbLDAPDirectoryQuery.cpp addrbook/src/nsAbLDAPDirectoryQuery.h addrbook/src/nsAbLDAPProperties.cpp addrbook/src/nsAbLDAPProperties.h addrbook/src/nsAbBoolExprToLDAPFilter.cpp addrbook/src/nsAbBoolExprToLDAPFilter.h I'll start reviewing these next week.

Here's a few random reviewer comments to start with; more to follow as I get my head better wrapped around the code: nsAbLDAPDirFactory.cpp ---------------------- * deleteDirectory doesn't actually do anything. Is this intended to be filled in later, or is it not necessary to do anything to make it disappear from the UI? If it's intended to be filled in later, please add an XXX comment about what it's supposed to contain, and open a new bugzilla bug that depends on this one. nsAbLDAPDirectoryQuery.cpp -------------------------- * why not use nsCOMPtrs for url and connection? * in nsAbQueryLDAPMessageListener::OnLDAPMessageSearchResult(), use the error constants from nsILDAPErrors.idl, not from the ldap.h header file. * check for failure from PR_smprintf nsAbLDAPProperties.cpp ---------------------- * for the property table, use a data structure that allows the search to be faster than linear. maybe just initialize the table pre-sorted and use a binary search?

More reviewer comments; still some more code to go through, however... nsAbLDAPDirFactory.cpp ---------------------- at the end of CreateDirectory, we see: nsSingletonEnumerator* cursor = new nsSingletonEnumerator(directory); if(!cursor) return NS_ERROR_NULL_POINTER; *_retval = cursor; NS_IF_ADDREF(*_retval); NS_ERROR_NULL_POINTER is probably the wrong error here; if new fails, this presumably means that you're out of memory, so I think NS_ERROR_OUT_OF_MEMORY would be more appropriate. Also, you probably don't really need to use the cursor temporary variable at all. How about: NS_IF_ADDREF(*_retval = new nsSingletonEnumerator(directory)); return *_retval ? NS_OK : NS_ERROR_OUT_OF_MEMORY; nsAbLDAPDirectory.cpp --------------------- * In the destructor, should probably only release mLock if it's non-zero, since it's conceivable that Initiate could have failed to allocate it. * Since the "default port" semantics of nsLDAPURL and nsILDAPConnection now match, the following lines in Initiate() should no longer be necessary: if (port == -1) port = 389; * Just an FYI: mConnection->Init() currently has a potential hang in if DNS is wonky. You'll probably want to add yourselves to the CC of http://bugzilla.mozilla.org/show_bug.cgi?id=82412 and make sure that whatever solution ends up being proposed works for you. * The comments in the file claim the GetOperations, GetChildNodes, GetChildCards are methods from nsIAbDirectoryQuery; when they are actually from nsIAbDirectory. * GetChildNodes() and GetChildCards() need to handle possible failure in NS_NewISupportsArray(). * if HasCard() is called while the search is returning results, it may fail. GetTotalCards() would return the wrong number. Since this failure mode isn't documented in nsIAbDirectory.idl, I assume it's probably not the right thing. Should nsIAbDirectroy::hasCard and ::getTotalCards be defined such that callers need to be able to cope with an NS_ERROR_NOT_AVAILABLE exception by trying again later? In that case these methods could return that if the search hasn't completed. * in CreateCard(), I don't think the c temporary var and the final NS_ENSURE_SUCCESS at the end of the function are really necessary. How about instead: rv = res->QueryInterface(nsIAbCard, NS_STATIC_CAST(card, nsIAbCard**)); NS_IF_ADDREF(*card); return rv;

Responded to the issues raised in the review by posting a new patch file to #78933. See also the comments in the same bug which deals with some of the issues raised.

Reassigning bugs related to LDAP/MAPI addressbook change landing to John Marmion, as he'll be driving this landing going forward.

2nd try at reassignment.

updated the patch file at #78933: Updated the license boilerplate for all new source files submitted with this patch. I have also updated the experimental builds at <http://abzilla.mozdev.org> with this patch against today's head for Linux, Windows and the Mac. Note to enable the LDAP Address Book the uri entry in the preferences file must be set to "moz-abldapdirectory://...." as requested.

Looks like the last two review comments I made have yet to be addressed, but the most recent changes look good. I'll try and get through another file or two tommorrow, if I get the chance.

Working on all the issues outstanding both here and at #78933. On the very last issue in the CreateCard(), I cannot get that code to compile. But we have come up with the following: rv = res->QueryInterface(NS_GET_IID(nsIAbCard), NS_REINTERPRET_CAST(void **,card)); NS_IF_ADDREF(*card);

* I'm surprised a static cast is required for the CreateCards thing, but if that's what it takes, go for it.

Can you add better options for custom definition of search dn and bind dn please. It is much easier to organize and save a ldap server bind "cn=myname, o=aorg.org" root dn "ou=myname, ou=abook, o=aorg.org"

*** Bug 85845 has been marked as a duplicate of this bug. ***

Missed 0.9.3.

*** Bug 90483 has been marked as a duplicate of this bug. ***

*** Bug 91028 has been marked as a duplicate of this bug. ***

This is an urgently needed feature, and it definately needs to be assigned to somebody. It seems relatively simple, considering the code for LDAP-based address completion is already implemented. Just get the base code implemented and work on stuff like the directory address tree structure (and adding/editing/modifying entries) later. Sorry. Not trying to be an ass, but I'd thought this feature would have been in here way before now (like 0.8 or 0.9). I would code it in myself, but C++ isn't my forte.

*** Bug 94331 has been marked as a duplicate of this bug. ***

It is assigned, and it is being worked on.

Okay. I was just wondering why it was still set to NEW.

*** Bug 94126 has been marked as a duplicate of this bug. ***

I spent most of today reviewing the remaining LDAP stuff. Here are my comments: nsAbLDAPDirFactory.cpp ---------------------- 1 deleteDirectory doesn't actually do anything. Is this intended to be filled in later, or is it not necessary to do anything to make it disappear from the UI? If it's intended to be filled in later, please add an XXX comment about what it's supposed to contain, and open a new bugzilla bug that depends on this one. nsAbLDAPProperties.cpp ---------------------- 2 in the MozillaLdapPropertyRelator::findXPropertyFromY() both do linear searches on the table. Consider using a data structure that allows the search to be faster than linear. At the very least, initialize the table to be alphabetically pre-sorted by one or the other property type (whichever one is likely to be looked up more often) and do a binary search on the table for that associated finder. 3 Also, if these strings aren't ever actually exposed to the UI, why spend cycles doing strcasecmp rather than just strcmp? Even if you have to downcase one or both properties at the beginning of the functions, at least it doesn't end up happening for every comparison. nsAbBoolExprToLDAPFilter.cpp ---------------------------- 4 Doing multiple .Appends in a row (like in the switch statements inside FilterCondition) has the potential to occasionally cause unnecessary reallocs() if you hit the top end of the buffer more than once. Instead, do stuff like this: case nsIAbBooleanConditionTypes::DoesNotExist: filter.Append ("(!("); filter.Append (ldapProperty); filter.Append ("=*"); filter.Append ("))"); break; can become case nsIAbBooleanConditionTypes::DoesNotExist: filter += NS_LITERAL_CSTRING("(!(") + nsDependentCString(ldapProperty) + NS_LITERAL_CSTRING("=*))") break; nsAbLDAPDirectoryQuery.cpp -------------------------- 5 Just so you know, the timeout parameter to SearchExt is currently ignored; there's an open bug on this assigned to leif, I think. I presume means that a search, in some error conditions, could continue running indefinitely, right? 6 Can't Query{Complete,Match,Error,Stopped} all be coalesced into a single function which takes the result status as an additional argumen In general, in the future, please do significantly more commenting, especially when doing stuff that might have subtle side effects, such as locking.

I will reply to each point raised by Dan. 1. It is true that the deleteDirectory() does nothing but it still needs to exist and return NS_OK. Typically deleting an address book involves removing it from the left pane and physically removing the corresponding mab file. In the case of LDAP and Outlook as no creation of an address book happens, there can be no deletion. A comment in this like the one in Outlook would be sufficient to get this across. 2/3. Yes, I believe a hash table would suffice here and provide a solution to both valid points raised employing a lower case hash table key. The strcasecmp was necessary I remember as we could not guarantee how LDAP servers might hold the names of their attributes. 4. Yes, we should change this. 5. Yes, there are conditions when the search will run indefinitely. I don't know what we can do until this bug is fixed. 6. Yes, the Query should be coalesced into a single function. 7. Some effort was made to document the interfaces inline and I take your point about the dearth of comments in the code. I need to know how we will proceed. I presume I now need to raise a patch against the existing code which covers the areas above.

Yeah, a patch is the thing to do. As far as point 1 goes, does anything need to be done to free the resources and objects associated with the LDAP addressbook, or will that all happen automagically as the core addressbook code releases appropriate references?

On the question of freeing resources and objects. The CreateDirectory() in Directory Factory does not hold any memory references. Yes we do ask RDF to create a resource. This will be freed on the destruction of the object. We did a quick test here to verify that. I don't see any memory leeks here. Obviously any issues here will also be reflected in the nsAbOutlookDirFactory where there is the same synchronization between the CreateDirectory() and DeleteDirectory() methods.

r=dmose@netscape.com on this last patch

In making the review change to create a hash table, I changed the original functionality. We need to traverse the table upwards to keep the original functionality. Thus for(i=0; i < tableSize; ++i)... should become for(i=tableSize-1; i >=0 ; --i)........ This table is used to match LDAP attributes to Mozilla Address Book attributes. This behaviour will occur where one Mozilla attribute can match against more than one LDAP attribute.

sr=mscott

Fix checked in. Note that the UI that makes this stuff usable without having to hand-edit prefs.js is being tracked in bug 83023 and friends.

Verified.