126134 - [LDAP Replication] when offline, LDAP autocomplete searches and addressbook searches should use the "replicated" .mab file

Status: VERIFIED FIXED

(SeaMonkey :: MailNews: Address Book & Contacts, defect, P1)

Description

[(not reading, please use [e-mail] instead)]

LDAP replication FE issues

this will just be the bug I check in some of the FE work that I'm doing for 
LDAP replication while rdayal does the BE work.

Comment 1

[scottputterman]

What are some of the FE changes that you will be making?

Comment 2

[(not reading, please use [e-mail] instead)]

Front End Issues that I'll be starting on, and then handing over to rdayal

1) when offline, in the local autocomplete code, we need to determine if we are 
set to do LDAP autocomplete, and if so, see if there is a non bogus .mab file 
for the LDAP server we use for LDAP autocomplete, and autocomplete against it.  
[we need to leave it open for performance, like we do other local abs.  we need 
to close it when going online, and we could close it if the user changes the 
prefs to autocomplete against that LDAP addressbook.] 

2) when offline, and the user selects an LDAP server, we want to use the .mab 
file, but it should behave like an LDAP server (blank until we quick search).  
this is important because for large .mab files, it is expensive to sort and 
generate all the collation keys. 

3) how do we handle LDAP directory deletion?  (we need to delete the .mab file 
also, but see #2, the filename pref may be bogus.  see #99124) 

4) do we need to "re-generate" the filename pref?  (yes, on initial replication 
we might need to regenerate the filename, if it is bogus). 

5) when we change online state, we need to re-root (and re-quick search?) any 
views to LDAP servers. 

6) scenarios where you go offline while a search is in progress.  Yes, in this 
scenario we'll have to first stop the search.  (note, there is no way to do 
this currently, I have a bug to add support for stop.) 

7) are we not able to compact mork? 

I need to implement #1 and #2 soon, so that rdayal can use the AB front end to 
test his replication backend code.

Comment 3

[(not reading, please use [e-mail] instead)]

I have the beginnings of a patch, that when offline LDAP queries turn into 
local queries.

I'll attach it so rdayal and dmose can comment.

Comment 4

[(not reading, please use [e-mail] instead)]

Attachment: patch, could it really be this easy?

if I'm offline, and I'm doing a ldap query, I forward the GetChildCards() to
the directory for the local version of that query.

note, I've hard coded so all offline ldap queries go to Phonebook.mab.

which is my ldap directory.

my prefs, would should help explain:

user_pref("ldap_2.servers.Phonebook.description", "Phonebook");
user_pref("ldap_2.servers.Phonebook.filename", "Phonebook.mab");
user_pref("ldap_2.servers.Phonebook.replication.lastChangeNumber", 0);
user_pref("ldap_2.servers.Phonebook.uri",
"ldap://nsdirectory.netscape.com:389/ou=People,dc=netscape,dc=com??sub");

Comment 5

[(not reading, please use [e-mail] instead)]

that patch is for the second issue:

2) when offline, and the user selects an LDAP server, we want to use the .mab 
file, but it should behave like an LDAP server (blank until we quick search).  
this is important because for large .mab files, it is expensive to sort and 
generate all the collation keys. 

I'll go fix the hard coding to Phonebook.mab, so that rdayal can use this patch 
to test his LDAP replication code.

note, it will assume that your filename pref is valid.  handling edge cases 
where it isn't (and it points to the personal address book) can come later.

then, I'll go look into the #1 issue, getting ldap autocomplete to work when 
offline, if we've replicated.

Comment 6

[(not reading, please use [e-mail] instead)]

Attachment: no more hard coding of the file name, this should be usable by rdayal.

one issue with this patch, is it, like the existing code in
nsAbLDAPDirectory::InitiateConnection() and other code around the tree, goes
through the prefs directly.  this code (like the other code
nsAbLDAPDirectory::InitiateConnection) is "less evil" since it is just reading,
but this still needs to be addressed.  there is an open bug on this issue.

Comment 7

[Rajiv Dayal]

Can we not use DirPrefs for reading the pref as per your suggestion to remove
reading the prefs directly ? I am using that in my code too, something like :

DIR_Server dirServer ;
dirServer.replInfo = nsnull ;
dirServer->prefName = nsCRT::strdup(prefName); // LDAP URI without query
DIR_GetPrefsForOneServer(dirServer, PR_FALSE, PR_FALSE);

// 'dirServer.fileName' is the filename used by replication and required here.

Comment 8

[(not reading, please use [e-mail] instead)]

Attachment: new patch, thanks to rdayal for the suggestion

here's a new patch.  rdayal's suggestion was very useful.  I've fixed that code
to use DIR_GetPrefsForOneServer()

I've also used it to fix the other place I was reading prefs directly, and I've
used it to fix bug #116984 (we no longer have to hard code maxHits to 100)

Comment 9

[(not reading, please use [e-mail] instead)]

Attachment: better patch, less copy and paste.

Comment 10

[(not reading, please use [e-mail] instead)]

Attachment: new patch, does autocompleting against replicated LDAP directories when offline.

note, I'm hard coding to my Phonebook.mab, fixing that now.

Comment 11

[(not reading, please use [e-mail] instead)]

Attachment: patch (no hard coding)

this patch makes is so when you are offline, you can use the replicated LDAP
directory for autocomplete and in the addressbook.

I've removed the hard coding.

Comment 12

[(not reading, please use [e-mail] instead)]

Attachment: same patch, fix a clobber build issue

Comment 13

[(not reading, please use [e-mail] instead)]

Attachment: same patch, fix a clobber build issue

Comment 14

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 72233 [details] [diff] [review]
same patch, fix a clobber build issue

Some review comments:

NeedToSearchLocalDirectories():

a) why pre-initialize *aNeedToSearch and then overwrite it?

NeedToSearchReplicatedDirectories():

b) same as comment a; additionally, I think it should be possible to
get rid of enableLDAPAutocomplete entirely and re-use aNeedToSearch

SearchReplicatedLDAPDirectories():

c) I think calloc() is preferred to PR_Calloc these days, for
performance reasons.  PR_Calloc does a memset() to zero out the
memory.  I recall a discussion where sfraser mentioned that (at least)
Mac OS X provides a calloc() which does the zero automagically at
page-in time using MMU tricks.

Actually, even better might be to change dir_DeleteServerContents to
be to DIR_DeleteServerContents (ie have nsDirPrefs export it) and then
just put the DIR_Server struct itself on the stack.

The same comments apply to GetDIR_Server.

d) How about using strdup() instead of nsCRT::strdup()?

e) If you condense the localDirectoryURI stuff into a single
statement, it'll save an allocation.  The same comments apply to
GetChildCards, only there it'll save a bunch of allocations.

SearchDirectory:

f)

+	 if (enableLocalAutocomplete) {
			  rv = SearchDirectory(kAllDirectoryRoot,
&searchStrings, results, PR_TRUE);
+	   NS_ASSERTION(NS_SUCCEEDED(rv), "searching all local directories
failed");
+	 }
+	 
+	 if (enableReplicatedLDAPAutocomplete) {
+	   rv = SearchReplicatedLDAPDirectories(prefs, &searchStrings, results,
PR_TRUE);
+	   NS_ASSERTION(NS_SUCCEEDED(rv), "searching all replicated LDAP
directories failed");
+	 } 


Looking at the code in context, it appears that if only
replicated-autocomplete fails, nsIAutoCompleteStatus::failed will be
returned.  But if only local-autocomplete fails, a different status
will be returned.  I'd vote for returning one of the success codes if
either one of the Search*Directories() functions succeeds.

g) Having just read through GetPrefForOneServer, it's an incredibly
expensive cost to pay for just getting a single preference (a metric
buttload of string copies, for one thing).  Unless DIR_Server is a
being used to delay disk writes and we have to use it to get the most
current info, I'd really prefer getting rid of DIR_Server entirely
from this patch.

Comment 15

[Rajiv Dayal]

I was looking at nsDirPrefs and see that it does not really give an advantage
for reading or writing directory preferences. It does not either have any
Observor for the related prefs or any notification for its clients if the prefs
changes, there is notifications for the UI when position changes but not really
when prefs are changed and saved. Further for it to be used as a central place
to access directory prefs it should ideally be implemented as a singleton or as
a XPCOM service. I have opened an enhancement bug (milestone future) for clean
up of nsDirPrefs, bug # 129326.

I feel nsDirPrefs can be only useful for code reuse in case if there is a need
to access more than few directory preferences. Seth, your decision here will
also be useful for me. thanks. 

Comment 16

[(not reading, please use [e-mail] instead)]

Attachment: new patch, addresses most of dmose's issues

Comment 17

[(not reading, please use [e-mail] instead)]

a)  addressed
b)  addressed
c1) addressed
d)  addressed
e)  addressed
f)  addressed

c2)

> Actually, even better might be to change dir_DeleteServerContents to
> be to DIR_DeleteServerContents (ie have nsDirPrefs export it) and then
> just put the DIR_Server struct itself on the stack.
> The same comments apply to GetDIR_Server.

I'm not sure I want to expose more of internals of the DIR_Server.


> g) Having just read through GetPrefForOneServer, it's an incredibly
> expensive cost to pay for just getting a single preference (a metric
> buttload of string copies, for one thing).  Unless DIR_Server is a
> being used to delay disk writes and we have to use it to get the most
> current info, I'd really prefer getting rid of DIR_Server entirely
> from this patch.

I agree, the existing DIR_Server code is heavy weight.
On one of the things it provides is an abstraction on top of prefs
so that when we delete servers, all the necessary prefs are delete.
It also provides an good place to put the code that generates 
a unique .mab file name

I need to go study the existing code.  

I'm not against us replacing it with something simpler, scriptable, and lighter 
weight.  now that we have XPCOM, we could do a lot of clean up.  (the 
DIR_Server code, from 4.x, has its own ref counting stuff.)

That said, I don't think now is the time to rip it out.  When we do come up 
with a clean interface and implementation, it should be easy to replace this 
code.  I'd rather not write [more] code that just reads prefs directly.  I'd 
rather use DIR_Server code until it is replaced.

this is covered by rdayal's new bug: #129326.

cc srilatha, as a scriptable DIR_Server like service will affect her, and allow 
her to clean up a lot of the existing js that reads / writes prefs directly.

comments?

Comment 18

[(not reading, please use [e-mail] instead)]

talking with dmose privately, he convinced me why it is better to add new 
(attribute getting) code that uses prefs directly.

see http://bugzilla.mozilla.org/show_bug.cgi?id=129326#c2

the way this affects this patch is I'll fix the code to go back to reading 
prefs directly, and that code will be fixed when 129326 is fixed.

note to srilatha and rdayal, if you are writing code to get addressbook 
attributes, use prefs directly, until 129326.  I'm not sure about setting 
addressbook values, that probably depends on where from (C++ or JS) and what 
values.  it might make sense to still use DIR_Server in certain C++ cases.

new patch coming soon.

Comment 19

[(not reading, please use [e-mail] instead)]

Attachment: patch to use read prefs directly.

Comment 20

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 73282 [details] [diff] [review]
patch to use read prefs directly.

r=dmose

Comment 21

[David :Bienvenu]

Comment on attachment 73282 [details] [diff] [review]
patch to use read prefs directly.

sr=bienvenu - there are some tabs, I think, that need cleaning up, and as a
style point, return result(s) should be the last parameter(s) in methods
(+nsAbAutoCompleteSession::SearchReplicatedLDAPDirectories)

Comment 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 73282 [details] [diff] [review]
patch to use read prefs directly.

a=dbaron for trunk checkin

Comment 23

[(not reading, please use [e-mail] instead)]

fixed.

I fixed the tabs and fixed bienvenu's style point for the code I added, and the 
existing code that I modeled my code after.

this blocks #59038 (the LDAP replication feature) so noting that.

Comment 24

[yulian chang]

Add Gary Chan and myself to CC list

Comment 25

[esther]

Yulian, doesn't this bug fall into your area for testing?  It's testing a
replicated LDAP directory in both Online and Offline state.  If so, please
reassign QA to yourself. Thanks

Comment 26

[yulian chang]

Reassign QA Contact to myself.

Comment 27

[yulian chang]

Verified with 20020410 trunk builds on various platforms.