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.

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.

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.

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.

>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?

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?

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?

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

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.

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.

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.

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.

+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?

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.

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.

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.

>+AuthDlgTitle=Addressbook LDAP Replication "Address Book" should be two words. :-) Did Robin's comment take care of dialog text for you?

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

+AuthDlgDesc=Enter email and password for accessing LDAP server Suggested text: "To access the directory server, enter your user name and password."

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

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

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

>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.

>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.

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.

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".

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

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

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

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)

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?

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

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 on attachment 77797 [details] [diff] [review] updated patch sr=sspitzer

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."

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

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

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

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

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

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."

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 on attachment 77967 [details] [diff] [review] macbuild xml project file changes r=dmose

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

Comment on attachment 77797 [details] [diff] [review] updated patch a=asa (on behalf of drivers) for checkin to the 1.0 trunk

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

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

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

20020423 branch build on Wins Add and Delete works fine on Branch build.

20020423 branch build on Win2K and MacOS X 10.1.2 Verified Modify.