119948 - duplicated LDIF import code

Status: RESOLVED FIXED

(MailNews Core :: Address Book, defect)

Description

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

duplicated LDIF import code

we've got duplicated (and messy) LDIF import code.

mozilla\mailnews\import\text\src\nsTextAddress.cpp and 
mozilla\mailnews\addrbook\src\nsAddressBook.cpp

we have to clean this up.

this code is being used in three scenarios:

1) direct LDIF import (nsTextAddress.cpp)
2) 4.x -> 6.x profile migration of .na2 files (we go from .na2 to LDIF, LDIF 
to .mab)
3) in the 6.x commercial bits, when the user migrates .na2 files by hand.
 
what a mess.

Comment 1

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

strike one for this duplicate code.  see
http://bugzilla.mozilla.org/show_bug.cgi?id=149759

Comment 2

[Scott MacGregor]

strike two for this duplicate code:

import from 4.x .mab, "xmozillausehtmlmail: false" would not be imported

in AddressBook.cpp

+        else if (kNotFound != column.Find("false"))
+            mDatabase->AddPreferMailFormat(newRow,
nsIAbPreferMailFormat::plaintext);

fixed with a checkin for bug #44494

Comment 3

[Mark Banner (:standard8)]

I'm looking into this as it will remove some of our bloat and make ldif easier
to maintain. I haven't investigated this fully yet, but here is a possible plan:

1) Combine the following functions, which are in both nsTextAddress and
nsAddressBook, into one new set of files. Named nsIAbLDIFHandler.idl,
nsAbLDIFHandler.h, nsAbLDIFHandler.cpp all stored within the appropriate address
book directories. Note at this stage, the functions only change where required
to merge them, there will be no rework or improvements to the code (this will
address the immediate bloat issue)

str_parse_line
str_getline
GetLdifStringRecord
ParseLdifFile (the only function that would be in nsIAbLDIFHandler.idl to begin
with)
AddLdifRowToDatabase
AddLdifColToDatabase
ClearLdifRecordBuffer

2) Consider moving the export ldif functions to the new LDIF handler from
nsAddressBook (if the previous stage doesn't need it).

3) Remove or tidy the AddressBookParser class defined in nsAddressBook.cpp.

4) Consider centralising and combining any other LDIF related handling functions
from nsTextAddress and nsAddressBook (there may be a few).

Whilst stages 2-4 are still not fully clear, I'd appreciate any comments,
especially on phase 1.

At the moment I'm going to wait on the part 2 nsFileSpec patch change from bug
132180 before doing anything, but comments would be useful before then.

LXR link - AddressBookParser:
http://lxr.mozilla.org/seamonkey/source/mailnews/addrbook/src/nsAddressBook.cpp#387

LXR link - nsTextAddress header:
http://lxr.mozilla.org/seamonkey/source/mailnews/import/text/src/nsTextAddress.h#83

Comment 4

[Mark Banner (:standard8)]

This seems like it will work, except I will probably use nsIAbLDIFService
instead of nsIAbLDIFHandler for the name as I think it's better like that.

Comment 5

[Mark Banner (:standard8)]

Attachment: Merge LDIF code Part 1 v1

Here's the first version of the duplicate LDIF code merging. Note as per my
previous comments, this first patch doesn't do anything to the code apart from
locating it in a common place and renaming a function or two.

There is some more work I need to do, like testing of the migration part of the
code (which should work), and other pre-review checks, but at this stage
general comments would be appreciated.

I envisage there will be more patches and possibly an extension of the new
interface to tidy up the remaining import and address book code.

Comment 6

[Mark Banner (:standard8)]

Attachment: Merge LDIF code Part 1 v2

Changes in this patch (v2):

- Sort out interface code (wrong capitialisation)
- Use nsCOMPtr<nsIAddrDatabase> instead of nsIAddrDatabase* in class
nsAbLDIFService
- Remove old AddressBookParser (with the changes in v1 this ended up as a class
wrapper around one function, this is now just a function in the code).
- Remove debug printfs

Todo:
Re-examine import code for improvements.
Test migration
Finialise patch for review.

Comment 7

[Mark Banner (:standard8)]

Comment on attachment 184265 [details] [diff] [review]
Merge LDIF code Part 1 v2

Opps v2 patch won't compile, undo the following change:

+++ mailnews/addrbook/src/nsAddressBook.cpp	22 May 2005 21:03:49 -0000
-#include "prprf.h" 
+//#include "prprf.h"

Comment 8

[Mark Banner (:standard8)]

Attachment: Merge LDIF code Part 1 v3

Changes in this patch (v3):

- Make opening of stream for reading ldif files consistent
- Remove now unnecessary function nsTextAddress::ImportLDIF
- Finish sorting out file headers
- Improve documentation for new interface file

Todo:
- Test migration
- Sort out old import log messages and thier handling.

Comment 9

[Dan Mosedale (:dmosedale, :dmose)]

Mark: sorry for not commenting sooner.  This plan looks quite reasonable to me,
and I think the phased approach is an excellent choice.  The patch so far looks
well done to me.  I'd love to see this land sooner rather than later, so let me
know what I can do to help.

Comment 10

[Mark Banner (:standard8)]

Dan, thanks for the comment - from just before I went on holiday, netscape 4.x
to suite migration I believe is horked (not sure about to thunderbird) though
not by this patch as far as I can see, but I couldn't find a bug #. If you could
test or find someone to test it on thunderbird it'd be useful.

Apart from that it's just a couple of cleanups that need doing - I should be
able to get round to them sometime in the next couple of days.

Comment 11

[Mark Banner (:standard8)]

Attachment: Merge LDIF code Part 1 v4

This is now ready for review. I've fixed up the import logging, however I can't
test the migration from 4.x as it's only included in commercial builds (ref
http://lxr.mozilla.org/seamonkey/source/mailnews/base/src/nsMessengerMigrator.cpp#2112).


The rest of it appears to be working fine though.

Comment 12

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 186727 [details] [diff] [review]
Merge LDIF code Part 1 v4

In general, this looks great.  A few minor nits...

>+#include "nsISupports.idl"
>+#include "nsIAbDirectory.idl"

Is this include actually necessary?  If so, can it be pushed down to either a
.h (good) or .cpp (better) file?

>+interface nsIFileSpec;
>+interface nsIAddrDatabase;
>+
>+[scriptable, uuid(b594baf1-34d4-48ce-891c-66fc2e8bc59d)]    
>+interface nsIAbLDIFService : nsISupports {
>+
>+  /**
>+   * Determine if a file is likely to be an LDIF file based on field
>+   * names that commonly appear in LDIF files.
>+   *
>+   * @param       pSrc   The file to examine
>+   *    
>+   * @return      PR_TRUE if the file appears to be of LDIF type,
>+   *              PR_FALSE otherwise
>+   */
>+  PRBool isLDIFFile(in nsIFileSpec pSrc);

Prefer |boolean| to PRBool in IDL.

Also, if you want to use Hungarian, I've not seen much use of this style of it
in the Mozilla tree.  I think we tend to prefer aArgument; kOnstant; gGlobal.

>+++ mailnews/addrbook/src/nsAbLDIFService.h	2005-06-01 22:31:23.000000000 +0100
>@@ -0,0 +1,70 @@
>+/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */

The tab width above does not match the width used in the file.

> NS_IMETHODIMP nsAddressBook::ConvertLDIFtoMAB(nsIFileSpec *fileSpec, PRBool migrating, nsIAddrDatabase *db, PRBool bStoreLocAsHome, PRBool bImportingComm4x)
> {
>     nsresult rv;
>     if (!fileSpec) return NS_ERROR_FAILURE;
> 
>-    rv = fileSpec->OpenStreamForReading();
>-    NS_ENSURE_SUCCESS(rv, rv);
>-
>-    AddressBookParser abParser(fileSpec, migrating, db, bStoreLocAsHome, bImportingComm4x);
>+    // this is converting 4.x to 6.0
>+    if (bImportingComm4x && db)
>+    {
>+      nsCOMPtr<nsIAbLDIFService> ldifService = do_GetService(NS_ABLDIFSERVICE_CONTRACTID, &rv);
> 
>-    rv = abParser.ParseFile();
>+      if (NS_SUCCEEDED(rv))
>+        rv = ldifService->ImportLDIFFile(db, fileSpec, bStoreLocAsHome, nsnull);
>+    }
>+    else
>+    {
>+      rv = Migrate4xAb(fileSpec, migrating, db, bStoreLocAsHome, bImportingComm4x);
>+    }

The logic in this function (ie why the various clauses would be called) is not
clear to me upon reading.  One question I have is why Migrate4xAb takes a
bImportingComm4x argument at all.  But it would also be helpful if you could
add some explicit commentary about the various clauses here and why you might
need to call Migrate4xAb rather than just ImportLDIFFile.

Comment 13

[Mark Banner (:standard8)]

Attachment: Merge LDIF code Part 1 v5

Changes:
- addressed review comments.
- Removed ConvertLDIFtoMAB, we now call Migrate4xAb direct
- Added some documentation in nsIAddressBook for Migrate4xAb and
ConvertNa2toLDIF functions.
- Changed hungarian style parameters on the interface functions only -
retaining the other ones for now on the rest of the functions to reduce the
amount of changes necessary for the patch. I may address these later if
required.

Comment 14

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 187143 [details] [diff] [review]
Merge LDIF code Part 1 v5

Nice work; r=dmose.

Comment 15

[David :Bienvenu]

Comment on attachment 187143 [details] [diff] [review]
Merge LDIF code Part 1 v5

sr=bienvenu with a couple nits.

+//class nsIAddrDatabase;

can you remove that line? You'll never need it, because you'll need to include
nsIAddrDatabase.h or have the including c++ file include it.

I know you're mostly getting rid of dup code, but might as well clean it up as
we go:

+    if (c == 0xA)
+    {
+      mLFCount++;
+    }
+    else if (c == 0xD)
+    {
+      mCRCount++;
+    }
+    else if ( c != 0xA && c != 0xD)

that last if clause is not needed since we just checked if c was 0xa or 0xd

Comment 16

[Mark Banner (:standard8)]

Attachment: Merge LDIF code Part 1 v6

This patch fixes David's nits (thanks David, I'd forgotten about the first one
of them).

Requesting approval for checkin, this patch combines two sets of ldif import
code, which were virtually the same, into one.

Comment 17

[Mark Banner (:standard8)]

Opps, forgot to mention: carrying forward Dan's r and David's sr.

Comment 18

[Mark Banner (:standard8)]

Comment on attachment 187303 [details] [diff] [review]
Merge LDIF code Part 1 v6

Cancelling approval request - I think we're a bit too close to check this in
now, will try for the next round.

Comment 19

[Mark Banner (:standard8)]

Attachment: Merge LDIF code Part 1 v7 (Checked in)

Updated patch to fix bitrot.

Re-requesting approval on this, as although I cancelled it before, I think I
got the wrong information as to when 1.8b3 would actually happen. It's also
starting to get in the way of some more bugs I'd like to fix.

This is a reorgansiation of existing code with a couple of tidy ups, the merged
functions I checked diffs by hand so they should be ok. Certainly the import
still works ok on my testcases. Affects thunderbird and seamonkey.

Comment 20

[Ian Neal]

Comment on attachment 188124 [details] [diff] [review]
Merge LDIF code Part 1 v7 (Checked in)

Checking in mailnews/addrbook/public/nsIAbLDIFService.idl;
initial revision: 1.1
mailnews/addrbook/public/Makefile.in;
new revision: 1.29; previous revision: 1.28
mailnews/addrbook/public/nsIAddressBook.idl;
new revision: 1.37; previous revision: 1.36
mailnews/addrbook/src/nsAbLDIFService.h;
initial revision: 1.1
mailnews/addrbook/src/nsAbLDIFService.cpp;
initial revision: 1.1
mailnews/addrbook/src/Makefile.in;
new revision: 1.61; previous revision: 1.60
mailnews/addrbook/src/nsAbBaseCID.h;
new revision: 1.7; previous revision: 1.6
mailnews/addrbook/src/nsAddressBook.cpp;
new revision: 1.137; previous revision: 1.136
mailnews/addrbook/build/nsAbFactory.cpp;
new revision: 1.38; previous revision: 1.37
mailnews/base/src/nsMessengerMigrator.cpp;
new revision: 1.114; previous revision: 1.113
mailnews/build/nsMailModule.cpp;
new revision: 1.21; previous revision: 1.20
mailnews/import/text/src/nsTextAddress.cpp;
new revision: 1.46; previous revision: 1.45
mailnews/import/text/src/nsTextAddress.h;
new revision: 1.13; previous revision: 1.12
mailnews/import/text/src/nsTextImport.cpp;
new revision: 1.35; previous revision: 1.34
done

Comment 21

[Mark Banner (:standard8)]

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

Comment 22

[Mark Banner (:standard8)]

Just a status recap (mainly for me):
> 1) Combine the following functions, which are in both nsTextAddress and
> nsAddressBook, into one new set of files. Named nsIAbLDIFHandler.idl,
> nsAbLDIFHandler.h, nsAbLDIFHandler.cpp all stored within the appropriate address
> book directories. 

Done

> 3) Remove or tidy the AddressBookParser class defined in nsAddressBook.cpp.

Done

> 4) Consider centralising and combining any other LDIF related handling functions
> from nsTextAddress and nsAddressBook (there may be a few).

Done.

> 2) Consider moving the export ldif functions to the new LDIF handler from
> nsAddressBook (if the previous stage doesn't need it).

To do - note that it looks like we can replace the export table with the nsILDAPAttributeMap - and hence save redefining that table.

I also think it is still a good idea to move this from nsAddressBook to the LDIF Service.

Comment 23

[Mark Banner (:standard8)]

Attachment: Use LDIF Attribute map in export code.

This patch removes the ldap property name column from the export table and reuses the ldap attribute map class (when exporting ldif files).

The table is still used for the ldif as it gives us the order of elements to export. If anyone can think of a better way, then I'll implement that in a future patch.

There are also some tidy ups - removal of a redundant function in nsIAddrDatabase and using NS_LINEBREAK as opposed to the duplicated  {LDIF_,MSG_}LINEBREAK definitions.

The actual move of the ldif export functions to nsILDIFService I'll do in the next patch.

Comment 24

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 201553 [details] [diff] [review]
Use LDIF Attribute map in export code.

>@@ -705,6 +690,16 @@ nsAddressBook::ExportDirectoryToLDIF(nsI
>   if (NS_FAILED(rv))
>     return rv;
> 
>+  // Get the default attribute map
>+  nsCOMPtr<nsIAbLDAPAttributeMapService> mapSrv = 
>+    do_GetService("@mozilla.org/addressbook/ldap-attribute-map-service;1", &rv);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  nsCOMPtr<nsIAbLDAPAttributeMap> attrMap;
>+  rv = mapSrv->GetMapForPrefBranch(NS_LITERAL_CSTRING("ldap_2.servers.default.attrmap"),
>+                                   getter_AddRefs(attrMap));
>+  NS_ENSURE_SUCCESS(rv, rv);
>+

Can you add a comment here about why the default map is chosen rather than the server specific map?  ie that we are doing an export of a Mozilla addressbook and the fact that it happens to be backed by a server using a possibly different schema is irrelevant: if people want an ldif using the server's schema, they can just use ldapsearch(1).

I still need to do some more thinking about how we choose and map dns (a la bug 302364 and the bug it spun off of) before I sign off on this patch.  I'll look at it some more tomorrow.

Comment 25

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 201553 [details] [diff] [review]
Use LDIF Attribute map in export code.

After giving this some thought, I think it's reasonable to deal with the dn mapping stuff in bug 302364.

r=dmose

Comment 26

[Mark Banner (:standard8)]

Attachment: Use LDIF Attribute map in export code v2 (part checked in, comment 30)

Added comment as per dmose's request, carrying forward his r, requesting sr.

Comment 27

[Mark Banner (:standard8)]

Comment on attachment 201784 [details] [diff] [review]
Use LDIF Attribute map in export code v2 (part checked in, comment 30)

I checked this into trunk earlier today.

Comment 28

[Andrew Schultz]

Comment on attachment 201784 [details] [diff] [review]
Use LDIF Attribute map in export code v2 (part checked in, comment 30)

this broke building with LDAP disabled.

Comment 29

[Mark Banner (:standard8)]

(In reply to comment #28)
> (From update of attachment 201784 [details] [diff] [review] [edit])
> this broke building with LDAP disabled.
> 

The problem is we are now using the ldap attribute map to get the attribute names for ldif export - Dan could we make the ldap attribute map to build always not just when MOZ_LDAP_XPCOM is defined?

I had a quick look and it seems the only function we will have problems with is setCardPropertiesFromLDAPMessage. Any suggestions?

Comment 30

[Mark Banner (:standard8)]

Comment on attachment 201784 [details] [diff] [review]
Use LDIF Attribute map in export code v2 (part checked in, comment 30)

I've just backed out the nsAddressBook.h & nsAddressBook.cpp changes from the part 2 patch due to the --disable-ldap breakage.

The other parts just removed a unused function so they should be ok left in.

I'll revise the patch once I've been able to discuss some ideas on irc.

Comment 31

[Mark Banner (:standard8)]

Attachment: Use LDIF Attribute map in export code v3 (checked in)

This patch still has the same nsAddressBook changes as v2, but also puts nsIAbLDAPAttribute map into all builds (not just ones with MOZ_LDAP_XPCOM defined).

If you try and call the setCardPropertiesFromLDAPMessage(in nsILDAPMessage ...) from js for instance, then you get the following (in this case I was calling it from addressbook.js):

Error: Expected ERROR: [Exception... "Cannot find interface information for parameter arg 0 [nsIAbLDAPAttributeMap.setCardPropertiesFromLDAPMessage]"  nsresult: "0x80570006 (NS_ERROR_XPC_CANT_GET_PARAM_IFACE_INFO)"  location: "JS frame :: chrome://messenger/content/addressbook/addressbook.js :: OnLoadAddressBook :: line 191"  data: no]

So I think we're safe to include it in this manner.

Comment 32

[Worcester12345]

footprint, mail1, mail2 keywords needed?

Comment 33

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 202889 [details] [diff] [review]
Use LDIF Attribute map in export code v3 (checked in)

Re-routing review for more timely service (thanks David!)

Comment 34

[David :Bienvenu]

Comment on attachment 202889 [details] [diff] [review]
Use LDIF Attribute map in export code v3 (checked in)

looks good, thx, Mark. This would seem to require testing that we're only going to get in the field...

Comment 35

[Scott MacGregor]

Comment on attachment 202889 [details] [diff] [review]
Use LDIF Attribute map in export code v3 (checked in)

looks good mark.

Comment 36

[Mark Banner (:standard8)]

All Patches checked in -> fixed.

Comment 37

[Mark Banner (:standard8)]

Try remembering to actually mark fixed ;-)