465882 - gloda getMessagesByMessageID's MessagesByMessageIdCallback getting undesired notifications [caught exception from listener in onItemsAdded] [this.results[this.msgIDToIndex[message.headerMessageID]] is undefined]

Status: RESOLVED FIXED

(MailNews Core :: Database, defect)

Description

[Andrew Sutherland [:asuth] (he/him)]

Attachment: v1 add and use a null query class

Error messages of the following form:

2008-11-19 11:49:29	gloda.collection	ERROR	caught exception from listener in onItemsAdded: file:///path/to/file/datastore.js:81: TypeError: this.results[this.msgIDToIndex[message.headerMessageID]] is undefined

are the result of getMessagesByMessageID using an explicit query to characterize its collection.  Since the purpose of getMessagesByMessageID is to return messages that they be ghosts or deleted, there can be messages in the collection which do not yet exist and may result in a call to GlodaCollectionManager.itemsAdded.  Since the message id is the same as the message in getMessagesByMessageID's collection, it will pass the test and trigger an undesired onItemsAdded notification on the callback.

The solution is to introduce a null query type to cover this case where we do not desire updates, but we potentially benefit from the caching.  This should complete our set of query types: I want nothing (null), I want things I already have (explicit), I want things that match some set of constraints (general query), and I want everything (wildcard, for debugging purposes only).

This blocks bug 463860 about re-enabling tests because that bug improves a deficiency in the unit test which catches this bug.

Comment 1

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 349121 [details] [diff] [review]
v1 add and use a null query class

Looks good; r=dmose with a few nits tweaked:

>diff --git a/mailnews/db/gloda/modules/datastore.js b/mailnews/db/gloda/modules/datastore.js
>--- a/mailnews/db/gloda/modules/datastore.js
>+++ b/mailnews/db/gloda/modules/datastore.js
>@@ -2007,17 +2007,17 @@ var GlodaDatastore = {
>     let quotedIDs = ["'" + msgID.replace("'", "''", "g") + "'" for each
>                      ([i, msgID] in Iterator(aMessageIDs))]
>     let sqlString = "SELECT * FROM messages WHERE headerMessageID IN (" +
>                     quotedIDs + ")";
>     
>     let nounDef = GlodaMessage.prototype.NOUN_DEF;
>     let listener = new MessagesByMessageIdCallback(msgIDToIndex, results,
>         aCallback, aCallbackThis);
>-    let query = new nounDef.explicitQueryClass();
>+    let query = new nounDef.nullQueryClass();

Please add a brief comment explaining why you've picked a nullQueryClass here.  Could even just be a pointer to the comment you already have at the head of the nullQueryClass definition.

>diff --git a/mailnews/db/gloda/modules/query.js b/mailnews/db/gloda/modules/query.js
>--- a/mailnews/db/gloda/modules/query.js
>+++ b/mailnews/db/gloda/modules/query.js
>@@ -290,16 +290,51 @@ GlodaQueryClass.prototype = {
>         return true;
>     }
>     
>     return false;
>   },
> };
> 
> /**
>+ * @class A query that never matches anything.
>+ * 
>+ * Collections corresponding to this query are intentionally frozen in time and
>+ *  do not want to be notified of any updates.  Conceptually, the collection
>+ *  does not even need to be registered with the collection manager, but it is
>+ *  arguably beneficial to do so, as it provides positive cache benefits until
>+ *  it is collected.

I might be nice to explain what these positive cache benefits are, unless you think that's likely to change over time.

>+GlodaNullQueryClass.prototype = {
>+  // don't let people try and mess with us
>+  or: function() { return null; },
>+  // don't let people try and query on us (until we have a real use case for
>+  //  that...)
>+  getCollection: function() { return null; },
>+  /**
>+   * Never matches anything.
>+   */
>+  test: function gloda_query_explicit_test(aObj) {
>+    return false;
>+  }
>+};
>+

Please use doxygen comments for the methods above that don't have them also put a line of whitespace between methods for readability.

Comment 2

[Andrew Sutherland [:asuth] (he/him)]

Attachment: v2 add and use a null query class

all demands have been met ;), carrying forward r=dmose

As always, your thorough reviews and insistence on good documentation are much appreciated.

Comment 3

[Andrew Sutherland [:asuth] (he/him)]

So, in my haste to address this issue, I overlooked the fact that the query mechanism is used to determine whether or not the item should actually be removed from the collection when a modification event occurs.  As such the null query does not make a lot of sense unless it affects the collection manager in ways other than just claiming nothing matches it.

I'll need to think on this use case a bit more; the problematic use case of getting messages by message id using the existing query mechanism is somewhat special, and its reuse of the query mechanism wasn't fully thought out.

Comment 4

[Andrew Sutherland [:asuth] (he/him)]

Attachment: v3 add and use a null query class, make collection manager aware of frozen attrib

This addresses the aforementioned problem by adding a 'frozen' attribute to the null query class so that the collection manager can specialize for this one case.  I think I've concluded that we need the collection registered, which is why we did this, and also why I also updated some documentation to that effect.

Requesting review again because there is a design decision newly made.

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 349370 [details] [diff] [review]
v3 add and use a null query class, make collection manager aware of frozen attrib

Looks good; r=dmose.

Comment 6

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/comm-central/rev/acbc55ca72ee