17879 - [FEATURE][LDAP] searching in address book

Status: VERIFIED FIXED

(MailNews Core :: LDAP Integration, defect, P1)

Description

[Phil Peterson]

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

Comment 1

[Biswapesh Chatterjee]

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.

Comment 2

[Phil Peterson]

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

Comment 3

[Biswapesh Chatterjee]

The former, and inside the firewall.

Comment 4

[Phil Peterson]

P4 per m/n staff mtg today

Comment 5

[selmer (gone)]

Moving all P4s out of beta2 to M17.

Comment 6

[selmer (gone)]

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

Comment 7

[selmer (gone)]

[LDAP] in summary

Comment 8

[selmer (gone)]

LDAP to M30, nobody, helpwanted

Comment 9

[pmock]

Changing qa assigned to myself.

Comment 10

[Kathleen :Brade]

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

Comment 11

[scottputterman]

reassigning to chuang.

Comment 12

[Stephen Walker]

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

Comment 13

[fenella]

Assign it to myself.

Comment 14

[fenella]

Oop! It belongs to Hong Kwon's group

Comment 15

[Jussi-Pekka Mantere]

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?

Comment 16

[scottputterman]

reassigning to dmose

Comment 17

[Marek Z. Jeziorek]

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

Comment 18

[Dan Mosedale (:dmosedale, :dmose)]

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.

Comment 19

[Dan Mosedale (:dmosedale, :dmose)]

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?

Comment 20

[Dan Mosedale (:dmosedale, :dmose)]

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;

Comment 21

[John Marmion]

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. 

Comment 22

[Dan Mosedale (:dmosedale, :dmose)]

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

Comment 23

[Dan Mosedale (:dmosedale, :dmose)]

2nd try at reassignment.

Comment 24

[John Marmion]

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.

Comment 25

[Dan Mosedale (:dmosedale, :dmose)]

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.

Comment 26

[John Marmion]

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);

Comment 27

[Dan Mosedale (:dmosedale, :dmose)]

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

Comment 28

[poelzi]

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"

Comment 29

[Keyser Sose]

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

Comment 30

[Blake Ross]

Missed 0.9.3.

Comment 31

[Keyser Sose]

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

Comment 32

[Michael Baffoni]

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

Comment 33

[Brendan Byrd]

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.

Comment 34

[Mark Smith [:mcs]]

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

Comment 35

[Hong Kwon]

It is assigned, and it is being worked on.

Comment 36

[Brendan Byrd]

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

Comment 37

[Keyser Sose]

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

Comment 38

[Dan Mosedale (:dmosedale, :dmose)]

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.

Comment 39

[John Marmion]

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.

Comment 40

[Dan Mosedale (:dmosedale, :dmose)]

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?

Comment 41

[John Marmion]

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.

Comment 42

[John Marmion]

Attachment: Patch File for Review Changes

Comment 43

[Dan Mosedale (:dmosedale, :dmose)]

r=[e-mail] on this last patch

Comment 44

[John Marmion]

Attachment: One line fix to earlier patch

Comment 45

[John Marmion]

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.

Comment 46

[Scott MacGregor]

sr=mscott

Comment 47

[Dan Mosedale (:dmosedale, :dmose)]

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.

Comment 48

[yulian chang]

Verified.