128087 - implement ldap replication change log protocol

Status: VERIFIED FIXED

(MailNews Core :: LDAP Integration, defect)

Description

[Rajiv Dayal]

This bug tracks the implementation of change log protocol used for updating the
locally replicated AB for a directory with the changes since the last update.
The implementation should find the changes and download the changes to the local
AB DB.

Comment 1

[Rajiv Dayal]

Attachment: initial patch for first cut implementation searching authDN, rootDSE and changelog entries

This is an initial patch with first cut implementation of changelog protocol.
Not ready for review yet. Needs implementation for updating the AB DB
appropriately, using string bundles for nsIPrompt dlg, etc. Working version for
searching authDN for the user, rootDSE to access changelog container and
downloading the changelog entries.

Comment 2

[Rajiv Dayal]

Attachment: updated patch integrated with the final patch for full replication

this patch is created integrating with the final patch for full replication. It
also takes care of comments on bug # 128086 for full replication which are
related and useful here.

Comment 3

[Rajiv Dayal]

I have tested the above patch on Windows. The ChangeLog replication works fine.
However in some cases (1 out 0f 10) I am seeing a problem which is related to
the LDAP SearchExt race condition bug # 131447. The ldap_result call in LDAP
connection thread returns 0 (timeout) and since the existing code sleeps and
then loops again
doing this continously, this results into an infinite loop. And the ldap_result
calls just keeps timing out each time. The fix for bug # 131447 should fix this
problem.

Comment 4

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

>The fix for bug # 131447 should fix this problem.

can you try the last patch in #131447 and report back if that fixes the problem?

Comment 5

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

a first review, looks pretty good.

I'm most concerned about the PRInt64 issues and the comment out code.

1)

+#For getting authDN for replication using dlg box
+AuthDlgTitle=Addressbook LDAP Replication
+AuthDlgDesc=Enter email-id and password for accessing LDAP server
+

please get jglick / robinf to review these strings.  I think robin will advise 
against "email-id".

2)

why is it necessary to have a empty string?

+static char * gLDAPChangeLogRootDSEAttribs[] =
+{ 
+    "changelog", 
+    "firstChangeNumber", 
+    "lastChangeNumber", 
+    "dataVersion", 
+    "nsAddressBookSyncURL", 
+    ""
+};

3)

why is it necessary to have a empty string?

+static char * gLDAPChangeLogEntryAttribs[] =
+{ 
+	"targetdn", 
+	"changetype", 
+    ""
+};

4)
 
+void nsAbLDAPProcessReplicationData::PopulateAuthDataFromDirServer()
+{
+    mAuthDN.AssignWithConversion(mDirServerInfo->authDn);
+    mAuthPswd.AssignWithConversion(mDirServerInfo->password);
+}

Is this safe because auth dn's and passwords always US-ASCII?

5) 

-        // donot call Done here since CreateABForReplicationDir calls it.
+        // donot call done here since it is called by OpenABForReplicationDir

don't or do not, per bienvenu other review.

6)

     // initialize the LDAP connection
-    return mConnection->Init(host.get(), port, NS_ConvertUTF8toUCS2(dn).get(), 
listener);
+    nsString authDN(aAuthDN);
+    rv = mConnection->Init(host.get(), port, authDN.get(), listener);

This copies aAuthDn.  will nsDependentString() work?

7)

atoll returns a PRInt64, these are PRInt32

+            case 1:
+                mRootDSEEntry.firstChangeNumber = nsCRT::atoll
(NS_ConvertUCS2toUTF8(vals[0]).get());
+                break;
+            case 2:
+                mRootDSEEntry.lastChangeNumber = nsCRT::atoll
(NS_ConvertUCS2toUTF8(vals[0]).get());
+                break;

8)

will this code work once ldap SDK 5.0 lands?
what are the effects of not being able to do this?  does it affect the user?
can you log a tracker bug that depends on us landing the 5.0 SDK, to turn this 
code on?

+    /* this one does not work with current LDAP XPCOM SDK, 
+    ** returns only the entry for the first query **
+    for(long i=0; i < mEntriesToDelete.Count(); i++) {
+       rv = Query->QueryChangedEntries(mEntriesToDelete[i]->get());
+       if(NS_FAILED(rv)) 
+           return rv;
+       mChangedEntriesQueryCount ++;
+    }
+
+    for(i=0; i < mEntriesToAdd.Count(); i++) {
+       rv = Query->QueryChangedEntries(mEntriesToAdd[i]->get());
+       if(NS_FAILED(rv)) 
+           return rv;
+       mChangedEntriesQueryCount ++;
+    }
+    */

9)

same as #8

+/* above changes are done since this below code doesnot work 
+** since the commented code in function OnFindingChangesDone doesnot work **
+    mChangedEntriesQueryCount --;  
+    if(mChangedEntriesQueryCount)
+       return NS_OK;  // more data to come
+    if(mReplicationDB && mDBOpen) {
+        mReplicationDB->Close(PR_TRUE); // commit and close
+        mDBOpen = PR_FALSE;
+    }
+    Done(PR_TRUE);  // all data is recieved
+    return NS_OK;
+*/

10)

+nsAbLDAPChangeLogQuery::nsAbLDAPChangeLogQuery()
+ :mReconnected(0)
+{
+  NS_INIT_ISUPPORTS();

mReconnected is a PRBool, not an int.

11)

+ /* member initializers and constructor code */

this comment is unnecessary.

12)

+#ifdef USE_AUTHDLG
+    nsString authDN;
+    return ConnectToLDAPServer(mURL, authDN);
+#else
+    ((nsAbLDAPProcessReplicationData *)((nsIAbLDAPProcessReplicationData *) 
mDataProcessor))->PopulateAuthDataFromDirServer();
+    return ConnectToLDAPServer(mURL, NS_ConvertUTF8toUCS2(mDirServer-
>authDn).get());
+#endif

don't do this cast.  instead, make PopulateAuthDataFromDirServer part of the 
nsIAbLDAPProcessReplicationData interface.

or, if that's not necessary, and USE_AUTHDLG will always be defined, can you 
remove this code?

13)

+    nsresult rv = NS_OK;
+
+    nsCAutoString host;
+    rv = mURL->GetHost(host);

a style point, potential optimization.  You don't need to initialize rv in 
these scenarios.

Try this instead:

+    nsCAutoString host;
+    nsresult rv = mURL->GetHost(host);

This is something to fix through out the whole patch.

14)

+    // we need to reconnect to the other server
+    if(!mReconnected && !nsCRT::strcmp(host.get(),syncHost.get()))

strcmp() instead of nsCRT::strcmp()

15)

+    {
+        nsString authDN(aAuthDN);
+        rv = ConnectToLDAPServer(syncURL, authDN);

this nsString will cause a copy.  why can't you just pass aAuthDN through?

16)

+    char *attribs[2];
+    attribs[0] = nsCRT::strdup(DIR_GetFirstAttributeString(mDirServer, cn));
+    attribs[1] = NULL;
+
+    nsAutoString filter;
+    filter.AssignWithConversion(DIR_GetFirstAttributeString(mDirServer, auth));
+    filter.AppendWithConversion("=");
+    filter += aValueUsedToFindDn;
+
+    nsXPIDLCString dn;
+    rv = mURL->GetDn(getter_Copies(dn));
+    if(NS_FAILED(rv)) 
+        return rv;
+    if(dn.IsEmpty())
+        return NS_ERROR_UNEXPECTED;
+
+    rv = mOperation->SearchExt(NS_ConvertUTF8toUCS2(dn).get(), 
LDAP_SCOPE_SUBTREE, 
+                               filter.get(), 
+                               sizeof(attribs),(const char **) &attribs[0],
+                               0, 0);

it looks like you are going to leak attribs[0]

can't attribs be a const char * array?

17)

+NS_IMETHODIMP nsAbLDAPChangeLogQuery::QueryChangeLog(const nsAString & 
aChangeLogDN, PRInt32 aLastChangeNo)
+{
+    if(!mInitialized) 
+        return NS_ERROR_NOT_INITIALIZED;
+    if(aChangeLogDN.IsEmpty()) 
+        return NS_ERROR_NULL_POINTER;

since aChangeLogDN is not a ptr, how about NS_ERROR_UNEXPECTED

18)

+    nsCString filter("(|");
+    for(PRInt64 i=mDirServer->replInfo->lastChangeNumber+1; i<=aLastChangeNo; 
i++)
+    {
+        char * filterForAChangeNo = PR_smprintf("(changenumber=%d)", i);
+        filter += filterForAChangeNo;
+    }
+    filter += (")");

a bunch of issues here.

a) make sure you can do +1 and ++ on PRInt64.  I think there are special macros 
for PRInt64
I don't think you can do simple math like this on PRInt64.  see prlong.h

b) you are leaking filterForAChangeNo and I don't think %d is safe for PRInt64.

+    {
+        filter += NS_LITERAL_"(changenumber=";
+        filter += nsCRT::lltoa(i);  // not sure if this exists
+        filter += ")";
+    }

there should be a clever way to use nsDependString and NS_LITERAL_CSTRING to do 
that on one line.

    
19)

+    // this below filter doesnot work ??!! should find out from mcs.
+    // char * filter = PR_smprintf("(changenumber>=%d)", mDirServer.replInfo-
>lastChangeNumber+1);

what are the effects of not being able to do this?  does it affect the user?

20)

+    nsString changeLogDN(aChangeLogDN);
+    return mOperation->SearchExt(changeLogDN.get(), 
+                               LDAP_SCOPE_ONELEVEL, 
+                               NS_ConvertUTF8toUCS2(filter).get(), 
+                               attributes.GetSize(), attributes.GetArray(),
+                               0, 0);
+}

this will copy aChangeLogDN.  can you use nsDependentString()?

21)


+NS_IMETHODIMP nsAbLDAPChangeLogQuery::QueryChangedEntries(const nsAString & 
aChangedEntryDN)
+{
+    if(!mInitialized) 
+        return NS_ERROR_NOT_INITIALIZED;
+    if(aChangedEntryDN.IsEmpty()) 
+        return NS_ERROR_NULL_POINTER;

since it's not a string, I'd suggest NS_ERROR_UNEXPECTED

22)

+    nsString changedEntryDN(aChangedEntryDN);
+    return mOperation->SearchExt(changedEntryDN.get(), scope, 
+                               NS_ConvertUTF8toUCS2(urlFilter).get(), 
+                               attributes.GetSize(), attributes.GetArray(),
+                               0, 0);

do you have to use a nsString here?  That will cause us to copy the string to 
the heap.
What about a nsDependentString?   

Comment 6

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

when we last met, there were some open issues with authenticating and 
changelogging, and then with autoconfig (and PSM/wallet?)

can you summarize any open issues with these two things?

Comment 7

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

one other things, can you summarize the impact of not having LDAP SDK 5.0?

Comment 8

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

don't we want to store the username and password in wallet?

In think you want to use nsIAuthPrompt, which you'd get from nsIWindowWatcher.

for example, see 
http://lxr.mozilla.org/mozilla/source/mailnews/compose/src/nsSmtpServer.cpp#385

If I remember right, if the username and password are in wallet, we won't 
prompt the user.  this will prevent us from prompting the user from every time 
they replicate (for a give account).

the other half of this bug would be the auth checkbox in the ldap directory 
properties.  has that landed?  I think srilatha has started the fix for that.

Comment 9

[Rajiv Dayal]

Yep that is why I had the #ifdef USE_AUTHDLG, i thought there would be that
another part of code from the UI which will ask the user if he wants to save
password and throw up the auth dlg and then store the authDn and pswd in Wallet.
However, I will change the code to use nsIAuthPrompt.

Comment 10

[Rajiv Dayal]

Also the reason we had this as separate is since we wanted to authenticate the
user using the supplied id and password over SSL. Please correct me if i am wrong. 

Thus if we have that hook to authenticate the user and store his/her authDN and
password the replication would not really throw the AuthDlg and just get the
stored authDn and password. The authDn and password fields are part of
nsDirPrefs (i think since 4.x munged/encrypted it and stored it in the prefs) so
in future once that authentication stuff lands, nsDirPrefs would change to get
it from the wallet. Or maybe we can just change the replication code to directly
get it from wallet since these would not really be coming from prefs anymore.

Hence i had this dialog for now to just get the authDN and pswd everytime till
we land the fix to integrate the wallet.

Comment 11

[Rajiv Dayal]

Hi Jeniffer,

Can u please review the below strings related to the authentication dialog
displayed to the user to get his/her email id, that is used by the program to
get his/her unique record (actually Distinguished Name(DN)) from LDAP server,
and his/her password that is used by the program to authenticate him/her with
the LDAP server:

+AuthDlgTitle=Addressbook LDAP Replication
+AuthDlgDesc=Enter email-id and password for accessing LDAP server

thanks, - Rajiv.

Comment 12

[robinf]

+AuthDlgDesc=Enter email-id and password for accessing LDAP server

Is email-id the same as user name? If so, how about "To access the directory
server, enter your user name and password."

Bug 120432 contains a screenshot that includes a checkbox in the Directory
Server Properties (General tab) for "Log in with user name and password." Is
this authentication dialog the result of checking that box?

Comment 13

[Rajiv Dayal]

No not really, this dialog is thrown up to the user when the user tries to
update his locally replicated Address Book for changed entries in the LDAP
server. We use the user's email to retrieve his/her authDN in here.

Comment 14

[Rajiv Dayal]

For comments above:

2) and 3) we do this since the last element of the attributes array for LDAP
search should be either null or point to a null string.

6) 20) and 22) Since nsDependentString cannot initialize using ns'A'String i was
creating this nsString, however using nsPromiseFlatString can avoid the copy in
case if the original string is not fragmented. So changed to use it which would
be better in most cases, dmose suggests this.

7) and 18a) the value of lastChangeNo and firstChangeNo is in fact PRInt32 but
now i found out that using standard C 'atol' function is perfectly fine, hence i
have changed the code to not use nsCRT:atoll and also we donot need to deal with
PRInt64 at all.

8) and 9) I donot know if SDK 5 would take care of this situation, dmose can
enlighten us more here, I have filed another bug for this anyway, bug #
Also this may not really have any performance implication, instead of doing all
queries in a loop and then retrieving the results for them one by one, we do a
query and retrieve its result and then do the next one. Also not sure if firing
many queries at once can have negative effect, depends on how the connection
thread would handle the results, etc, etc.

18b)i found out we can use nsPrintfCString !

19) this would not directly effect the user, a very minor performance impact
could be at the server end since it could take longer to parse the query if we
cannot do this commented code. However this query is done only once.

Comment 15

[Rajiv Dayal]

Attachment: updated patch

Comment 16

[Rajiv Dayal]

In the above patch the PopulateAuthData and the #else part of the #ifdef
USE_AUTHDLG will be implemented once we go ahead with bug # 120432. In that case
the PopulateAuthData will first check the prefs.js for authDN and
wallet/password manager for the stored password. If it cannot find either of the
two, it will call GetAuthData. We need to wait for #120432 since if the user
wants to change what is stored in the password manager the only way (if
PopulateAuthData is implemented) would be using the UI proposed in #120432.

For now we just display the username/pswd dialog everytime as in the current patch.

Comment 17

[jglick]

>+AuthDlgTitle=Addressbook LDAP Replication

"Address Book" should be two words. :-) Did Robin's comment take care of dialog 
text for you?

Comment 18

[Rajiv Dayal]

Attachment: updated patch

This patch implements the PopulateAuthData mentioned above and removes handling
syncURI since underlying LDAP SDK handles referrels.

Comment 19

[robinf]

+AuthDlgDesc=Enter email and password for accessing LDAP server

Suggested text: "To access the directory server, enter your user name and
password." 

Comment 20

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

a few nits, a few suggestions, some duplicate code and concern about leaks.

Index: build/nsAbBaseCID.h
===================================================================
RCS file: /cvsroot/mozilla/mailnews/addrbook/build/nsAbBaseCID.h,v
1)

+nsAbLDAPProcessChangeLogData::nsAbLDAPProcessChangeLogData()
+: mUseChangeLog(PR_FALSE),
+  mEntriesDeletedQueryCount(0),
+  mEntriesAddedQueryCount(0),
+  mChangeLogEntriesCount(0),
+  mEntriesUpdatedQueryCount(0)
+{
+  mEntriesToAdd.Clear();
+  mEntriesToDelete.Clear();
+  mEntriesToUpdate.Clear();
+}

is Clear() necessary in the ctor?  I doubt it.  I bet nsStringArrays are 
initialized as empty.

2)


+    *attributes.GetArrayAddr() = NS_STATIC_CAST(char **, nsMemory::Alloc
((*attributes.GetSizeAddr()) * sizeof(char *)));
+    attributes.GetArray()[0] = ToNewCString(nsDependentCString
(DIR_GetFirstAttributeString(mDirServer, cn)));

do these get leaked?

3)

+    filter.AppendWithConversion("=");

instead do something like:

filter.Append(NS_LITERAL_STRING("="));

3)


+        filter += nsPrintfCString(128,"%d)", i);

this might be faster, but I'm not 100% certain:

   filter += "(changenumber="
   filter.AppendInt(i);
   filter += ")";

Personally, I'd rather do this than use nsPrintfCstring.

4)

should these be static const char *?
 
+static char * gLDAPChangeLogRootDSEAttribs[] =
+{ 
+    "changelog", 
+    "firstChangeNumber", 
+    "lastChangeNumber", 
+    "dataVersion", 
+    "nsAddressBookSyncURL", 
+    ""
+};
+
+#define CHANGELOG_ATTRIBS_COUNT 6 
+
+static char * gLDAPChangeLogEntryAttribs[] =
+{ 
+	"targetdn", 
+	"changetype", 
+    ""
+};
+
+#define CHANGELOGENTRY_ATTRIBS_COUNT 3

5)

+nsresult MozillaLdapPropertyRelator::getLDAPChangeLogAttribs(PRUint32 *aCount, 
char ***_retval)
+nsresult MozillaLdapPropertyRelator::getLDAPChangeLogEntryAttribs(PRUint32 
*aCount, char ***_retval)

Those two methods are the same code, with two minor differences.

You just need one method, and pass in the two differences:

nsresult MozillaLdapPropertyRelator::getLDAPAttribs(PRUint32 aTotal, const char 
**aAttribTable, PRUint32 *aCount, char ***_retval)
{
}

and you'd call it with CHANGELOG_ATTRIBS_COUNT, gLDAPChangeLogRootDSEAttribs
and CHANGELOGENTRY_ATTRIBS_COUNT, gLDAPChangeLogEntryAttribs


6)

I'd include a comment here that this call to DeleteCard() has to happen before 
the call
to CreateNewCardAndAddToDb() for DB_REPLACE to work properly.
 
+   if((aDBOperation == DB_DELETE) || (aDBOperation == DB_DELETEANDADD)) {
+        rv = mReplicationDB->DeleteCard(newCard, PR_FALSE);
+        if(NS_FAILED(rv)) {
+            Abort();
+        }
+    }
+
+    if((aDBOperation == DB_ADD) || (aDBOperation == DB_DELETEANDADD)) {
     rv = mReplicationDB->CreateNewCardAndAddToDB(newCard, PR_FALSE);
     if(NS_FAILED(rv)) {
         Abort();

7)

+#define DB_DELETEANDADD 2

how about:

+#define DB_REPLACE 2

8)

To your exisitng interface, consider adding:
 
const long kUnknown       = -1;

and change your code to:

+    // this could be a rebind call
+    PRInt32 replicationState = nsIAbLDAPProcessReplicationData::kUnknown;
+    rv = mDataProcessor->GetReplicationState(&replicationState);
+    if(NS_FAILED(rv)) 
+        return rv;
+    if((replicationState != nsIAbLDAPProcessReplicationData::kUnknown) && 
(replicationState != nsIAbLDAPProcessReplicationData::kQueryNotStarted))
+    {

9)

you don't need to null out mConnection or mOperation.

+        mConnection = nsnull;
+        mConnection = do_CreateInstance(NS_LDAPCONNECTION_CONTRACTID, &rv);
+        if(NS_FAILED(rv)) 
+            return rv;
+
+        mOperation = nsnull;
+        mOperation = do_CreateInstance(NS_LDAPOPERATION_CONTRACTID, &rv);


10)

is this comment still correct?

     // currently only Replicate All is implemented
-    return nsIAbLDAPProcessReplicationData::kDefaultDownloadAll;
+    return nsIAbLDAPProcessReplicationData::kChangeLogProtocol;
 }
 

Comment 21

[Rajiv Dayal]

Attachment: updated patch to take care of comments

2) CharPtrArrayGuard in its destructor frees the array so these would not get
leaked.

Comment 22

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

Comment on attachment 76668 [details] [diff] [review]
updated patch to take care of comments

sr=sspitzer, on the condition that dmose does the r=

Comment 23

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

Comment on attachment 76668 [details] [diff] [review]
updated patch to take care of comments

talking with dmose over AIM, he's got a suggestion on how to improve this.

Comment 24

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 76668 [details] [diff] [review]
updated patch to take care of comments

general
-------
a) instead of having all these stubbed out functions bloating
nsAbLDAPReplicationQuery(), how about just making a second interface
in the existing IDL file which inherits from nsIAbLDAPReplicationQuery
called nsIAbLDAPPartialReplicationQuery if it can suitably generalized (or
nsIAbLDAPChangeLogReplicationQuery if not).  We're already paying the
penalty for using XPCOM, I don't think this will add to that penalty.

b) I _think_ you just wanna use PromiseFlatString, not
nsPromiseFlatString, but check to be sure.

A large number of the following things are fairly minor; in general
the patch is looking pretty good.

nsIAbLDAPReplicationQuery.idl: 
------------------------------
c) there are tabs here; please convert them to spaces 

nsAbChangeLogData.h
-------------------
d) RootDSEChangeLogEntry: don't use nsCAutoStrings for member variables
which are going to be allocated off the heap.

e) There's a lot of signed PRInt's here.  Can any of this stuff
actually contain -1 or other negative numbers, or should these be PRUint?


nsAbChangeLogData.cpp
---------------------
f) ::OnLDAPSearchResult(): When we hit a sizelimit exceeded, we need
to somehow notify the user that he doesn't have all the data
requested, perhaps by popping up a prompt.  Also, we need to do this
on all errors which call Abort(), probably inside that function.  If
you want to just file another bug on yourself to implement this later,
that's ok.

g) ::OnSearchAuthDNDone

+	 nsCAutoString authDN;
+	 authDN.AssignWithConversion(mAuthDN);
+	 PR_FREEIF(mDirServerInfo->authDn);
+	 mDirServerInfo->authDn=nsCRT::strdup(authDN.get());

It shouldn't be necessary to allocate, copy, then copy again here.

h) ::ParseRootDSEEntry():    for(PRUint32 i = 0; i < attrs.GetSize(); i++)

Initialize to attrs.GetSize() and countdown to zero.  You avoid
unnecessary method calls, and the comparison is likely to be faster
because many processors have instructions specifically for comparing
with 0.

i) For all the numeric conversions here, use NS_LossyConvertUCS2toASCII.

j) ::OnSearch RootDSEDone():

+    if (!dbPath->Exists())
+	 mUseChangeLog = PR_FALSE;
+    else if (!dbPath->GetFileSize())
+	 mUseChangeLog = PR_FALSE;

Can't this be simplified to a single |if (cond1 || cond2)|?

k) There are two if (mUserChangeLog) {foo} else {bar} statements in a
row.  Can't these be consolidated?

l) There are a couple places in this function where nsCRT::strdup is being
   used on new-style strings.  ToNewCString() is preferred.

m) ::ParseChangeLogEntries()  see previous comment about iterating down to 0.

n) Instead of converting and using nsCRT::strcasecmp, call Compare()
with the CaseInsensitiveComparator against NS_LITERAL_STRINGs.	This
way the conversion happens at compile-time rather than run-time.

o) ::OnFindingChangesDone(): why are these |else if| rather than just
|if|?  Is it not possible to get additions, changes, and deletes done
simultaneously?


p) Can you be more specific about the problem you're seeing here?  Also,
declare i as PRsomething and count down to 0 rather than up

+    /* this one does not work with current LDAP XPCOM SDK, 
+    ** returns only the entry for the first query **
+    for(long i=0; i < mEntriesToDelete.Count(); i++) {
+	rv = Query->QueryChangedEntries(mEntriesToDelete[i]->get());
+	if(NS_FAILED(rv)) 
+	    return rv;
+	mChangedEntriesQueryCount ++;
+    }

q) ::ParseChangedEntries: Shouldn't this really be "ParseChangedEntry"
since you've only got one nsILDAPMessage here?	And as a result, shouldn't
these ifs be separated by elses, perhaps even with an assertion if 
the default case gets hit?

+    PRInt16 dbOperation = DB_ADD;
+    if(mEntriesToDelete.IndexOf(targetDN) > 0) 
+	 dbOperation = DB_DELETE;
+    if(mEntriesToAdd.IndexOf(targetDN) > 0) 
+	     dbOperation = DB_ADD;
+    if(mEntriesToUpdate.IndexOf(targetDN) > 0) 
+	 dbOperation = DB_UPDATE;


nsAbLDAPChangeLogQuery.cpp
--------------------------
r) here, and in the make files, you're including "ldap.h" and adding a
dependency on the ldap module.	Neither of these things should be
necessary; you should be able to do everything with pure mozldap
stuff.	One example: use nsILDAPURL::SCOPE_SUBTREE instead of 
LDAP_SCOPE_SUBTREE from ldap.h.  Same for BASE and ONELEVEL.

s) ::DoReplicationQuery() Since you're using an automatic on the
stack, why not use nsAutoString here?

t) ::QueryAuthDN()

+    filter.AppendWithConversion(NS_LITERAL_CSTRING("=").get());
+    filter += aValueUsedToFindDn;

Instead, how about 

+    filter += PRUnichar('=') + aValueUsedToFindDn;

u) If the server is configured appropriately, it should be possible to
bind anonymously and replicate, which I assume would mean an empty DN is ok.
Is that correct?

+    rv = mURL->GetDn(getter_Copies(dn));
+    if(NS_FAILED(rv)) 
+	 return rv;
+    if(dn.IsEmpty())
+	 return NS_ERROR_UNEXPECTED;


v) ::QueryChangeLog(): Please figure out what's going on with the >=
search filter that you have commented out.  The giant list of
changelog numbers is bloaty and inefficient.


nsAbLDAPProperties.cpp
----------------------
w) in the static arrays, g is use to indicate "global".  Since these are
static, so name them starting with s instead.

x) What's the "nsAddressBookSyncURL" in gLDAPChangeLog?  Can you add a
comment explaining where it came from and why it's there, please?

y) Why is it necessary to have getLDAPAttribs to make converted copies
of the arrays in question?  How about just having a single copy of
each of the xpcom arrays, and having everyone use those?  They'll even
be stored in the TEXT segment of the executable avoiding the heap
altogether.

nsAbLDAPReplicationData.cpp
----------------------------
z) kUnknown doesn't seem like it really describes the state correctly.
How about kIdle?  Or even the old name was OK, I think.

A) ::OpenABForReplicatedDir(): bCreate should actually be named
aCreate, since this is an argument, and we do hungarian by scope, not
type.

nsAbLDAPReplicationQuery.cpp
----------------------------
B) ::DoReplicationQuery(): If this

+    nsString authDN;  // pass on an empty string for anonymous binding
+    return ConnectToLDAPServer(mURL, authDN);

is not just stub code waiting for more auth code to be written, how about 

return ConnectToLDAPServer(mURL, NS_LITERAL_STRING(""));

Comment 25

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

>instead of having all these stubbed out functions bloating
>nsAbLDAPReplicationQuery(), how about just making a second interface
>in the existing IDL file which inherits from nsIAbLDAPReplicationQuery
>called nsIAbLDAPPartialReplicationQuery if it can suitably generalized (or
>nsIAbLDAPChangeLogReplicationQuery if not).  We're already paying the
>penalty for using XPCOM, I don't think this will add to that penalty.

Yes, this is a good catch.  To make your life easier, definitely add this new 
interface in the existing .idl file (so that no build changes are needed.)
a single idl file can have multiple interfaces

The shared methods can go in the base interface, and the unique methods can go 
in the inheritied interface, and you can always override.

This change should stay, and should be in the base:

-   [noscript] readonly attribute DIR_ReplicationInfo replicationInfo;
+   [noscript] readonly attribute DIR_Server replicationServerInfo;
 
> kUnknown

Using a constant was my suggestion.  I just made a poor suggestion for the name.

excellent review, solid suggestions and comments.  I should have waited quietly 
for dan's r= before offering up my sr=.  That will teach me.

Comment 26

[Rajiv Dayal]

>instead of having all these stubbed out functions bloating
>nsAbLDAPReplicationQuery(), how about just making a second interface
>in the existing IDL file which inherits from nsIAbLDAPReplicationQuery
>called nsIAbLDAPPartialReplicationQuery if it can suitably generalized (or
>nsIAbLDAPChangeLogReplicationQuery if not).  We're already paying the
>penalty for using XPCOM, I don't think this will add to that penalty.

>Yes, this is a good catch.  To make your life easier, definitely add this new 
>interface in the existing .idl file (so that no build changes are needed.)
>a single idl file can have multiple interfaces

>The shared methods can go in the base interface, and the unique methods can go 
>in the inheritied interface, and you can always override.

If I do this above it will lead to the standard problem of multiple inheritance.
I already thought about it and went with this way because :

a) nsIAbLDAPReplicationQuery is the base interface,
b) nsIAbLDAPChangeLogQuery : nsIAbLDAPReplicationQuery is the derived interface
c) nsAbLDAPReplication : nsIAbLDAPReplicationQuery is the base interface impl
d) nsAbLDAPChangeLogQuery : nsIAbLDAPChangeLogQuery, nsAbLDAPReplicationQuery is
the derived interface impl.

Thus d) above will lead to naming conflicts and ambiguity for
nsIAbLDAPReplicationQuery references and methods and will cause the standard
multiple inheritance problem.

Comment 27

[Rajiv Dayal]

Well on taking a second look at the suggestion of defining another interface,
maybe since nsIAbLDAPReplicationQuery here is nothing but an abstract class by
using scope resolution operators and type casts I can try to avoid the
amibuities. But making these changes for this and testing for all the workaround
will take some time, I will also have to take a look at the XPCOM ISUPPORT_IMPL
macros since there are multiple references of nsISupports too in the derived
interface implementation besides for nsIAbLDAPReplicationQuery.

Comment 28

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

met with rajiv, and I think we've got the interface / class issues all worked out.

also, we discussed how to implement delete during changelogging.

the idea:

store the dn on the nsIAbMDBCard as "_dn", remember _ is the prefix we've been
using for generic columns.

then, when it is time to delete, find card by attribute, looking for the card
with the right "_dn".

Comment 29

[Rajiv Dayal]

Attachment: patch with new interface, delete using SetAttribute, comments, etc.

I cleaned up several more things in the code specially the code to handle
delete and update eliminating the use of string arrays for them and query /
processing for them besides the changes as per the description of the patch.

Comment 30

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 77014 [details] [diff] [review]
patch with new interface, delete using SetAttribute, comments, etc.

a) No need for a temporary or a cast here; use do_QueryInterface instead.

+NS_IMETHODIMP nsAbLDAPProcessChangeLogData::Init(nsIAbLDAPReplicationQuery *
query, nsIWebProgressListener *progressListener)
+{
+   NS_ENSURE_ARG_POINTER(query);
+
+   // here we are assuming that the caller will pass a nsAbLDAPChangeLogQuery
object,
+   // an implementation derived from the implementation of
nsIAbLDAPReplicationQuery.
+   nsAbLDAPChangeLogQuery * tempQuery = NS_STATIC_CAST(nsAbLDAPChangeLogQuery
*, query);
+
+   mChangeLogQuery = tempQuery;


b) Use NS_ConvertUTF8toUCS2 here, since the base DN may not be ASCII.

+	rv = dialog->PromptUsernameAndPassword(title, desc, 
+					    
NS_ConvertASCIItoUCS2(serverUri).get(), 
+					    
nsIAuthPrompt::SAVE_PASSWORD_PERMANENTLY,
+					     getter_Copies(username),
getter_Copies(password), 
+					     &btnResult);

c) I talked to mcs, and it turns out we can't depend on attributes
coming back in the same order they were requested.  So we'll need to
use string compares when looping through these attributes.

d) Use NS_LITERAL_STRING() instead of NS_ConvertUTF8toUCS2 here:

+    return mOperation->SearchExt(nsnull, nsILDAPURL::SCOPE_BASE, 
+				NS_ConvertUTF8toUCS2("objectclass=*").get(), 
+				attribCount, NS_CONST_CAST(const char **,
attributes),
+				0, 0);

e) Still need to get this sorted out:

   // this below filter doesnot work ??!! should find out from mcs.
   // char * filter = PR_smprintf("(changenumber>=%d)",
mDirServer.replInfo->lastChangeNumber+1);

f) There still needs to be a comment about what nsAddressBookSyncURL
is and why we need it

g) In MozillaLdapPropertyRelator, why of use public getters instead
of just making the tables themselves public and const?	Also, tables
should not have a trailing empty string; that's only necessary for the
C SDK, not the XPCOM SDK.  Also, can you switch CHANGELOG_ATTRIBS_COUNT
from a #define to a const so that it plays more nicely with the debugger?

h) is it worth adding an NS_WARNING here, since it might be interesting
while debugging?

     if(hasSetCardProperty == PR_FALSE)
     {
-	 Abort();
-	 return NS_ERROR_FAILURE;
+	 // if some entries are bogus for us, continue with next one
+	 return NS_OK;
     }

Comment 31

[Michael Buckland]

Discussed at Mail News bug meeting.  Decided to make ADT2 and plus this bug.

Comment 32

[Rajiv Dayal]

Attachment: patch to take care of comments above

Comment 33

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 77525 [details] [diff] [review]
patch to take care of comments above

A couple of tiny things:

For the loops through the attribute arrays:


+	 if(vals.GetSize()) {
+	     if (!Compare(nsDependentCString(attrs[i]),
NS_LITERAL_CSTRING("changelog"), nsCaseInsensitiveCStringComparator()))
+		 mRootDSEEntry.changeLogDN = vals[0];
+	     if (!Compare(nsDependentCString(attrs[i]),
NS_LITERAL_CSTRING("firstChangeNumber"), nsCaseInsensitiveCStringComparator()))
+		 mRootDSEEntry.firstChangeNumber =
atol(NS_LossyConvertUCS2toASCII(vals[0]).get());
+
+ [...]

Since both sides of the comparison end up getting wrapped, this is
probably a reasonable place to just use strcasecmp (assuming that's
portable ANSI C, which I _think_ it is).  Also, how about making these
"else if" statements, since if one of the if clauses succeeds, the
rest are guaranteed to fail.

Also, can you add comments by the NS_IMPL_THREADSAFE_* uses that say that if
the 101252 gets fixed, they can be reverted to the non-threadsafe impls?  In
fact, does the service itself even need to be threadsafe still?

Nice work!

r=dmose

Comment 34

[timeless]

F:\2002\m099\mozilla-org\html\releases>type a.cpp
#include "string.h"
int main (void){return strcasecmp(0,0);}
F:\2002\m099\mozilla-org\html\releases>cl a.cpp
Microsoft (R) 32-bit C/C++ Optimizing Compiler Version 12.00.8168 for 80x86
Copyright (C) Microsoft Corp 1984-1998. All rights reserved.

a.cpp
a.cpp(2) : error C2065: 'strcasecmp' : undeclared identifier

F:\2002\m099\mozilla-org\html\releases>type a.cpp
#include "string.h"
int main (void){return stricmp(0,0);}
F:\2002\m099\mozilla-org\html\releases>cl a.cpp
Microsoft (R) 32-bit C/C++ Optimizing Compiler Version 12.00.8168 for 80x86
Copyright (C) Microsoft Corp 1984-1998. All rights reserved.

a.cpp
Microsoft (R) Incremental Linker Version 6.00.8168
Copyright (C) Microsoft Corp 1992-1998. All rights reserved.

/out:a.exe
a.obj

strcasecmp is *not* the name used by msvc. don't use it unwrapped, thank you, 
have a nice day (i'm unlikely to but hopefully i'll have an ok day)

Comment 35

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

looks good.

three minor comments / questions, address these (and dmose's comments) then 
sr=sspitzer

1)

+            if (!Compare(nsDependentCString(attrs[i]), NS_LITERAL_CSTRING
("changelog"), nsCaseInsensitiveCStringComparator()))
+                mRootDSEEntry.changeLogDN = vals[0];

just do:

if (!PL_strcasecmp(attrs[i], "changelog)) 
  mRootDSEEntry.changeLogDN = vals[0];

Same for all the other uses  (there's about 8 total, I think.)

2)

+    PRInt32 replicationState = -1;
+    rv = mDataProcessor->GetReplicationState(&replicationState);
+    if(NS_FAILED(rv)) 
+        return rv;
+    if((replicationState != -1) && (replicationState != 
nsIAbLDAPProcessReplicationData::kIdle))

will GetReplicationState() ever return NS_OK, with the out param == -1?

If not, I think you can leave replicationState un-initialized, and not check 
for != -1.

If it will do that, then add a kUnknown and use it instead of -1.

My problem with -1 that you're mixing it in with the well defined 
nsIAbLDAPProcessReplicationData states, like kIdle.
which is defined to be 0, but there's no agreement that -1 is not going to be 
used by a state.

3)  does this patch include the fix for the dataloss bug 
http://bugzilla.mozilla.org/show_bug.cgi?id=134994?

Comment 36

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

I'm confused why we get auth data when we are in the anonymous binding state:

+    switch(mState) {
+    case kAnonymousBinding :
+        rv = GetAuthData();
+        if(NS_SUCCEEDED(rv)) 
+            rv = mChangeLogQuery->QueryAuthDN(mAuthUserID);
+        if(NS_SUCCEEDED(rv)) 
+            mState = kSearchingAuthDN;
+        break;
+    case kAuthenticatedBinding :
+        rv = mChangeLogQuery->QueryRootDSE();
+        if(NS_SUCCEEDED(rv)) 
+            mState = kSearchingRootDSE;
+        break;
+    } //end of switch

Comment 37

[Rajiv Dayal]

We do GetAuthData in anonymous binding state because we need to get the AuthDN
for the user for authenticated binding, hence we first bind anonymous and use it
to get the DN for the user id entered by the user, once we have that we
re-initialize our connection and rebind using the AuthDN and password.

For 2) i will change it to initialize using kIdle.

For 3) yes this patch has the fix for 134994. If we land this we donot need to
land the patch there.

Comment 38

[Rajiv Dayal]

Attachment: updated patch

Comment 39

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

Comment on attachment 77797 [details] [diff] [review]
updated patch 

sr=sspitzer

Comment 40

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

one request, can you add your bugzilla comment to the code, before checking in:

"We do GetAuthData in anonymous binding state because we need to get the AuthDN
for the user for authenticated binding, hence we first bind anonymous and use it
to get the DN for the user id entered by the user, once we have that we
re-initialize our connection and rebind using the AuthDN and password."

Comment 41

[yulian chang]

Tested Rajiv's replication change log builds on Win2K and Linux.
Add, Edit and Delete are tested and the builds works fine.

Comment 42

[scottputterman]

nominating adt1.0.0.  cc'ing mcarlson because of localization changes.

Comment 43

[Jaime Rodriguez, Jr.]

adt1.0.0+ (on ADT's behalf) approval for checkin into 1.0, pending approval by
michele, a=.

Comment 44

[Jaime Rodriguez, Jr.]

Ray/Montse - Can we get L10N approval for the string change, on this one ASAP.

Comment 45

[msanz]

approved by l10n, I only see a couple of string added, I hope I'm not wrong!

Comment 46

[Rajiv Dayal]

You are right Montse, there are only couple of strings added here :

AuthDlgTitle="Address Book LDAP Replication"
AuthDlgDesc="To access the directory server, enter your user name and password." 

Comment 47

[Rajiv Dayal]

Attachment: macbuild xml project file changes

Oops i forgot to add the xml project file changes in the implementation patch,
here it is, two .cpp files added to the existing addrbook project.

Comment 48

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 77967 [details] [diff] [review]
macbuild xml project file changes

r=dmose

Comment 49

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

Comment on attachment 77967 [details] [diff] [review]
macbuild xml project file changes

sr=sspitzer

Comment 50

[Asa Dotzler [:asa]]

Comment on attachment 77797 [details] [diff] [review]
updated patch 

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 51

[Asa Dotzler [:asa]]

Comment on attachment 77967 [details] [diff] [review]
macbuild xml project file changes

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 52

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

the fix for this has landed.  marking fixed for rdayal.

Comment 53

[yulian chang]

Verified with 20020410 trunk builds on Win2K, Linux and MacOS.

Comment 54

[yulian chang]

20020423 branch build on Wins

Add and Delete works fine on Branch build.

Comment 55

[yulian chang]

20020423 branch build on Win2K and MacOS X 10.1.2

Verified Modify.