1572324 - Create a JS/SQLite address book directory provider

Status: RESOLVED FIXED

(MailNews Core :: Address Book, task)

Description

[Geoff Lankow (:darktrojan)]

This is step one of de-mork-ing the address book: creating a provider that can do (almost) everything the mork provider can do. I've chosen to do this with javascript and SQLite as they can operate synchronously (the current interfaces and all the code require this) and asynchronously (future!). Ultimately IndexedDB is probably better but doesn't fit the requirements right now.

Comment 1

[Günter]

With the vContacts (refactor of the VUW project https://neandr.github.io/vContacts) the VUW team decided to use indexedDB and ical.js. Is there a plan to use those also.
ical.js is at least fit for vCard 3.0, not sure about vers.4. AFAIK there is also another bug for working on ical.js (performance issues with Lightning). Should that be a combined effort for calendar and addressbook?

Comment 2

[Günter]

To enable TB/AB supporting CardDAV (vers.3 and 4), this paper is highly recommaned
http://sabre.io/dav/building-a-carddav-client/

Comment 3

[Geoff Lankow (:darktrojan)]

Thanks, but I wasn't looking for input at this stage. This bug is about doing exactly what we do now with a different set of code. (Code which already exists, I'm just cleaning it up.) After that we'll start looking at doing new things.

Comment 4

[Geoff Lankow (:darktrojan)]

Attachment: 1572324-jsaddrbook-1.diff

Part 1/3

Comment 5

[Geoff Lankow (:darktrojan)]

Attachment: 1572324-jsaddrbook-tests-1.diff

Part 2/3

Comment 6

[Geoff Lankow (:darktrojan)]

Attachment: 1572324-jsaddrbook-pref-1.diff

Part 3/3

Comment 7

[Magnus Melin [:mkmelin]]

Comment on attachment 9085400 [details] [diff] [review]
1572324-jsaddrbook-1.diff

Review of attachment 9085400 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/addrbook/jsaddrbook/jsAddrBookCard.jsm
@@ +7,5 @@
> +ChromeUtils.defineModuleGetter(this, "MailServices", "resource:///modules/MailServices.jsm");
> +ChromeUtils.defineModuleGetter(this, "XPCOMUtils", "resource://gre/modules/XPCOMUtils.jsm");
> +ChromeUtils.defineModuleGetter(this, "newUID", "resource:///modules/jsAddrBookUtils.jsm");
> +
> +/* Prototype for nsIAbCard objects that are not mailing lists. */

please use /** */ doc style for this so it would show up in generated documentation

@@ +8,5 @@
> +ChromeUtils.defineModuleGetter(this, "XPCOMUtils", "resource://gre/modules/XPCOMUtils.jsm");
> +ChromeUtils.defineModuleGetter(this, "newUID", "resource:///modules/jsAddrBookUtils.jsm");
> +
> +/* Prototype for nsIAbCard objects that are not mailing lists. */
> +

@implements {nsIAbItem}
 @implements {nsIAbCard}

@@ +9,5 @@
> +ChromeUtils.defineModuleGetter(this, "newUID", "resource:///modules/jsAddrBookUtils.jsm");
> +
> +/* Prototype for nsIAbCard objects that are not mailing lists. */
> +
> +function jsAddrBookCard() {

We should drop the js from the name.
I think modules should also always (almost) be named capitalized. That way you can import them and declare them, and later in the file avoid confusion on what is a variable (lower case) and what is a library function (capitalized)

@@ +23,5 @@
> +
> +  get uuid() {
> +    return MailServices.ab.generateUUID(this._directoryId, this._localId);
> +  },
> +  generateName(generateFormat, bundle) {

bundle seems unused?

@@ +188,5 @@
> +    return this.UID == card.UID;
> +  },
> +};
> +
> +this.NSGetFactory = XPCOMUtils.generateNSGetFactory([jsAddrBookCard]);

I think this should better be
var EXPORTED_SYMBOLS = [jsAddrBookCard];

... and related changes. 
(see bug 1562313, plenty of examples in bug 1524688)

::: mailnews/addrbook/jsaddrbook/jsAddrBookDirectory.jsm
@@ +14,5 @@
> +ChromeUtils.defineModuleGetter(this, "jsAddrBookMailingList", "resource:///modules/jsAddrBookMailingList.jsm");
> +ChromeUtils.defineModuleGetter(this, "newUID", "resource:///modules/jsAddrBookUtils.jsm");
> +ChromeUtils.defineModuleGetter(this, "toXPCOMArray", "resource:///modules/iteratorUtils.jsm");
> +
> +/* This is where the address book manager creates an nsIAbDirectory. We want

/** */ style please

Also for this file, please drop the js prefix

@@ +68,5 @@
> +  },
> +};
> +
> +/* Keep track of all database connections, and close them at shutdown, since
> + * nothing else ever tells us to close them. */

// style please

@@ +90,5 @@
> +    connections.delete(path);
> +  }
> +}
> +
> +/* Prototype for nsIAbDirectory objects that aren't mailing lists. */

/** */

@@ +91,5 @@
> +  }
> +}
> +
> +/* Prototype for nsIAbDirectory objects that aren't mailing lists. */
> +

@implements {nsIAbCollection}
@implements {nsIAbDirectory}

@@ +221,5 @@
> +        insertStatement.reset();
> +      }
> +    }
> +    deleteStatement.finalize();
> +    insertStatement.finalize();

should this be in a transaction?

@@ +324,5 @@
> +  },
> +  get childNodes() {
> +    let lists = Array.from(
> +      this._lists.values(),
> +      l => new jsAddrBookMailingList(l.uid, this, l.localId, l.name, l.nickName, l.description).asDirectory

l is hard to read. if you wnat to make it compact, perhaps v (for value)

@@ +330,5 @@
> +    return new SimpleEnumerator(lists);
> +  },
> +  get childCards() {
> +    let results = [];
> +    for (let l of this._lists.values()) {

here too

@@ +340,5 @@
> +    }
> +    if (this._query) {
> +      if (!this._processedQuery) {
> +        let r = /^\((and|or|not|([^\)]*)(\)+))/;
> +        let i = 1; // Ignore ?

ignore '?' from where?

@@ +377,5 @@
> +          ]);
> +        } else {
> +          properties = this._loadCardProperties(card.UID);
> +        }
> +        function matches(b) {

let matches = function(b)

@@ +620,5 @@
> +    this._prefBranch.setComplexValue(name, Ci.nsIPrefLocalizedString, value);
> +  },
> +};
> +
> +this.NSGetFactory = XPCOMUtils.generateNSGetFactory([jsAddrBookDirectory]);

same for this, use the static registration

::: mailnews/addrbook/jsaddrbook/jsAddrBookFactory.jsm
@@ +8,5 @@
> +ChromeUtils.defineModuleGetter(this, "MailServices", "resource:///modules/MailServices.jsm");
> +ChromeUtils.defineModuleGetter(this, "XPCOMUtils", "resource://gre/modules/XPCOMUtils.jsm");
> +ChromeUtils.defineModuleGetter(this, "closeConnectionTo", "resource:///modules/jsAddrBookDirectory.jsm");
> +
> +/* Address book factory. This looks like it should be a useful for keeping

/** */

@@ +14,5 @@
> + * methods of accessing a directory, and nsAbManager has a cache anyway. */
> +
> +function jsAddrBookFactory() {
> +}
> +jsAddrBookFactory.prototype = {

add the @implements

::: mailnews/addrbook/public/nsIAbDirectory.idl
@@ +26,5 @@
>  #define kABFileName_PreviousSuffixLen 4
> +#define kABFileName_CurrentSuffix  ".mab" /* v3 address book extension */
> +
> +#define kJSDirectoryRoot           "jsaddrbook://"
> +#define kJSAddressBook             "jsbook.sqlite"

addrbook.sqlite?

Comment 8

[Magnus Melin [:mkmelin]]

Comment on attachment 9085401 [details] [diff] [review]
1572324-jsaddrbook-tests-1.diff

Review of attachment 9085401 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/addrbook/test/unit/head_jsaddrbook.js
@@ +4,5 @@
> +
> +// Ensure the profile directory is set up
> +do_get_profile();
> +
> +/* What follows is a copy of abSetup.js modified for tests with the

// style I guess

Comment 9

[Magnus Melin [:mkmelin]]

Comment on attachment 9085402 [details] [diff] [review]
1572324-jsaddrbook-pref-1.diff

Review of attachment 9085402 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/addrbook/content/abDragDrop.js
@@ +75,5 @@
> +        aXferData.data.addDataForFlavour("application/x-moz-file-promise-dest-filename", card.displayName + ".vcf");
> +        aXferData.data.addDataForFlavour("application/x-moz-file-promise-url", "data:text/vcard," + vCard);
> +        aXferData.data.addDataForFlavour("application/x-moz-file-promise", abFlavorDataProvider);
> +      } catch (ex) {
> +        Cu.reportError(ex);

why this?

Comment 10

[Geoff Lankow (:darktrojan)]

Attachment: 1572324-jsaddrbook-2.diff

Comment 11

[Geoff Lankow (:darktrojan)]

Comment on attachment 9086298 [details] [diff] [review]
1572324-jsaddrbook-2.diff

Oh oops, I haven't finished with this yet.

Comment 12

[Geoff Lankow (:darktrojan)]

Attachment: 1572324-jsaddrbook-3.diff

Comment 13

[Geoff Lankow (:darktrojan)]

Attachment: 1572324-jsaddrbook-tests-2.diff

Comment 14

[Geoff Lankow (:darktrojan)]

Attachment: 1572324-jsaddrbook-4.diff

Comment 15

[Geoff Lankow (:darktrojan)]

(In reply to Magnus Melin [:mkmelin] from comment #9)

::: mailnews/addrbook/content/abDragDrop.js

> +        aXferData.data.addDataForFlavour("application/x-moz-file-promise-dest-filename", card.displayName + ".vcf");
> +        aXferData.data.addDataForFlavour("application/x-moz-file-promise-url", "data:text/vcard," + vCard);
> +        aXferData.data.addDataForFlavour("application/x-moz-file-promise", abFlavorDataProvider);
> +      } catch (ex) {
> +        Cu.reportError(ex);```

why this?

Because at this point, translateTo throws NS_ERROR_NOT_IMPLEMENTED (that's a whole other bug in itself) but I don't want to break drag and drop.

Comment 16

[Magnus Melin [:mkmelin]]

(In reply to Geoff Lankow (:darktrojan) from comment #15)

why this?

Because at this point, translateTo throws NS_ERROR_NOT_IMPLEMENTED (that's a whole other bug in itself) but I don't want to break drag and drop.

Please add a comment into the code, so we'll remember why it's there

Comment 17

[Magnus Melin [:mkmelin]]

Comment on attachment 9086302 [details] [diff] [review]
1572324-jsaddrbook-4.diff

Review of attachment 9086302 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM, r=mkmelin

Comment 18

[Geoff Lankow (:darktrojan)]

Attachment: 1572324-jsaddrbook-pref-2.diff

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/b0bac431a4e7
Create a JS/SQLite address book directory provider; r=mkmelin
https://hg.mozilla.org/comm-central/rev/7c2568875848
Enable JS address book provider for new address books behind a pref; r=mkmelin
https://hg.mozilla.org/comm-central/rev/9c50784bfcdc
Run the address book tests for both Mork and JS/SQLite providers; r=mkmelin

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/fa465d49d7bf
follow-up - Replace mistaken uint with uint32_t; rs=bustage-fix

Comment 21

[Philippe VIGNEAU]

Hi Geoff

one thing that seems important to me on this bug : it would be good that this provider includes a way to encrypt data.

and something I haven't understood, why sqllite and not indexedDB ?

Comment 22

[Geoff Lankow (:darktrojan)]

(In reply to Philippe VIGNEAU from comment #21)

one thing that seems important to me on this bug : it would be good that this provider includes a way to encrypt data.

Not in this bug, please file another one.

and something I haven't understood, why sqllite and not indexedDB ?

We're using SQLite at this point because we can access it synchronously. Almost all of our address book code is synchronous, but we plan to change that. SQLite can also be accessed asynchronously, so this new provider can be easily upgraded as we make changes. I do want to shift to IndexedDB, but it's not possible at this point.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/e1d87f5ee220
Follow-up: Reformat. rs=reformat DONTBUILD

Comment 24

[Joshua Cranmer [:jcranmer]]

Okay, this is the drive-by comment that no one here is going to want to hear, and I apologize for not having taken the time to make this kind of comment earlier:

The performance of the database schema you've specified for the address book is unlikely to be acceptable. The database is going to be relatively wasteful in space usage, and it's likely to exhibit O(N) performance in the important bottleneck situations. And you should stop pushing use of this address book until the performance issues are fixed. Releasing bad performance code induces a stigma that can be hard to erase, and even for all of Mork's ills, sacrificing performance to be rid of it is not a useful trade-off. I speak from experience here--I submitted bug 418551 which had exactly this sort of I-did-it-naively-and-screwed-performance-in-doing-so side effects.

To be constructive, this is what needs to be done:

First, you need to establish a benchmark to evaluate performance. Scraping a public LDAP database of a large university is a good start. I have some ideas for how to actually randomly generate a dataset if you don't want to try public data. For the target size, I'd aim for no fewer than 10,000 entries, probably closer to 100,000 (larger being a better way of highlighting scalability issues).

The second thing is the set of tasks to benchmark. The obvious tasks are:

• Bulk addition and removal (say 10% of the database)
• The time it takes to open the address book window and open up the card modification dialog
• The time it takes to query the address book for the details to show in the message header (i.e., get card from email address)
• The time it takes to return autocomplete results for a given string

The third and fourth tasks are by far the most important tasks; the second task should at least feel snappy to the user, and it's okay to be slower on the first task so long as you don't freeze the UI.

My gut instinct is that you're looking at roughly a 10-40× slowdown on code with the sqlite address book over the mork address book, but I haven't attempted to measure it--I'm extrapolating from my failed attempt in bug 418551. That's really unacceptable performance numbers.

Okay, so on to how to fix the database schema. I'm not terribly great with database optimizations, so I'll defer to someone else who is more knowledgeable here. But you absolutely want to uniquify the property names, so that querying by (card, property) is querying by two integers. Primary keys should always be integers (ideally AUTOINCREMENT NOT NULL)--a single integer not null primary key generally implies O(1) access to any data on that field.

Entity-attribute-value is usually the worst way to implement any sort of database structure. I've seen some suggestions to implement multiple fields as "extend the table dynamically," which might be a feasible approach. Alternatively, promote the important fields--popularity, first/last name, and display name--to the card table, emails to a separate table, and store the rest of the fields as a JSON blob in the card table. Realistically, we only query cards by email (GetCardForEmailAddress!) or display names (autocomplete), so losing SQL support for querying by, say, age, isn't going to be a big loss. That search is already O(N) in today's Mork world.

I'm not sure how to handle the indexes for email and autocomplete fields. Experimentation is useful here--this is why having a benchmark to assess the performance is necessary to move forward. SQLite appears to let you construct indexes on expressions such that you can build an index on LOWER(field) instead of just field.