Bug 619909 - Gloda fromJSON de-persistence needs to filter out undefined returned values; deleted tags on removed messages end up as undefined, break faceting logic. v1 filter out undefined return values from fromJSON, use hasOwnProperty with NounTag. r=bienvenu
authorAndrew Sutherland <asutherland@asutherland.org>
Fri, 17 Dec 2010 10:19:15 -0800
changeset 6856 3a67b40d56f8355a4fca52c3b90a3024bf564223
parent 6855 c6c5290dc2ae37ab11652e1360665151c906dc04
child 6857 1e36326c45e371eefda9ab0e196076bf4aa4808c
push idunknown
push userunknown
push dateunknown
reviewersbienvenu
bugs619909
Bug 619909 - Gloda fromJSON de-persistence needs to filter out undefined returned values; deleted tags on removed messages end up as undefined, break faceting logic. v1 filter out undefined return values from fromJSON, use hasOwnProperty with NounTag. r=bienvenu
mailnews/db/gloda/modules/datastore.js
mailnews/db/gloda/modules/noun_tag.js
mailnews/db/gloda/test/unit/test_index_messages_local.js
--- a/mailnews/db/gloda/modules/datastore.js
+++ b/mailnews/db/gloda/modules/datastore.js
@@ -3455,23 +3455,40 @@ var GlodaDatastore = {
         if (objectNounDef.contributeObjDependencies(jsonValue,
                              aReferencesByNounID, aInverseReferencesByNounID)) {
           deps[attribId] = jsonValue;
           hasDeps = true;
         }
         else // just propagate the value, it's some form of simple sentinel
           aItem[attrib.boundName] = jsonValue;
       }
-      // otherwise, the value just needs to be de-persisted, or not
+      // otherwise, the value just needs to be de-persisted, or...
       else if (objectNounDef.fromJSON) {
-        if (attrib.singular)
-          aItem[attrib.boundName] = objectNounDef.fromJSON(jsonValue);
-        else
-          aItem[attrib.boundName] = [objectNounDef.fromJSON(val) for each
-            ([, val] in Iterator(jsonValue))];
+        if (attrib.singular) {
+          // For consistency with the non-singular case, we don't assign the
+          //  attribute if undefined is returned.
+          let deserialized = objectNounDef.fromJSON(jsonValue);
+          if (deserialized !== undefined)
+            aItem[attrib.boundName] = deserialized;
+        }
+        else {
+          // Convert all the entries in the list filtering out any undefined
+          //  values. (TagNoun will do this if the tag is now dead.)
+          let outList = [];
+          for each (let [, val] in Iterator(jsonValue)) {
+            let deserialized = objectNounDef.fromJSON(val);
+            if (deserialized !== undefined)
+              outList.push(deserialized);
+          }
+          // Note: It's possible if we filtered things out that this is an empty
+          //  list.  This is acceptable because this is somewhat of an unusual
+          //  case and I don't think we want to further complicate our
+          //  semantics.
+          aItem[attrib.boundName] = outList;
+        }
       }
       // it's fine as is
       else
         aItem[attrib.boundName] = jsonValue;
     }
 
     if (hasDeps)
       aItem._deps = deps;
--- a/mailnews/db/gloda/modules/noun_tag.js
+++ b/mailnews/db/gloda/modules/noun_tag.js
@@ -98,17 +98,18 @@ var TagNoun = {
 
   toParamAndValue: function gloda_noun_tag_toParamAndValue(aTag) {
     return [aTag.key, null];
   },
   toJSON: function gloda_noun_tag_toJSON(aTag) {
     return aTag.key;
   },
   fromJSON: function gloda_noun_tag_fromJSON(aTagKey, aIgnored) {
-    let tag = this._tagMap[aTagKey];
+    let tag = this._tagMap.hasOwnProperty(aTagKey) ? this._tagMap[aTagKey]
+                : undefined;
     // you will note that if a tag is removed, we are unable to aggressively
     //  deal with this.  we are okay with this, but it would be nice to be able
     //  to listen to the message tag service to know when we should rebuild.
     if ((tag === undefined) && this._msgTagService.isValidKey(aTagKey)) {
       this._updateTagMap();
       tag = this._tagMap[aTagKey];
     }
     // we intentionally are returning undefined if the tag doesn't exist
--- a/mailnews/db/gloda/test/unit/test_index_messages_local.js
+++ b/mailnews/db/gloda/test/unit/test_index_messages_local.js
@@ -21,15 +21,78 @@ function test_reparse_of_local_folder_wo
   folder.msgDatabase.summaryValid = false;
   // clear the database so next time we have to reparse
   folder.msgDatabase.ForceClosed();
 
   // force gloda to re-parse the folder again...
   GlodaMsgIndexer.indexFolder(folder);
   yield wait_for_gloda_indexer();
 }
+tests.unshift(test_reparse_of_local_folder_works);
 
-tests.unshift(test_reparse_of_local_folder_works);
+/**
+ * Ensure that fromJSON for a non-singular attribute properly filters out
+ *  "undefined" return values, specifically as it relates to tags.  When the
+ *  user removes them Gloda doesn't actually re-index the messages so the
+ *  values will still be there when we next load the message.
+ *
+ * We directly monkey with the state of NounTag for no really good reason, but
+ *  maybe it cuts down on disk I/O because we don't have to touch prefs.
+ */
+function test_fromjson_of_removed_tag() {
+  // -- inject
+  let [folder, msgSet] = make_folder_with_sets([{count: 1}]);
+  yield wait_for_message_injection();
+  yield wait_for_gloda_indexer(msgSet, {augment: true});
+  let gmsg = msgSet.glodaMessages[0];
+
+  // -- tag
+  let tag = TagNoun.getTag("$label4");
+  msgSet.addTag(tag.key);
+  yield wait_for_gloda_indexer(msgSet);
+  do_check_eq(gmsg.tags.length, 1);
+  do_check_eq(gmsg.tags[0].key, tag.key);
+
+  // -- forget about the tag, TagNoun!
+  delete TagNoun._tagMap[tag.key];
+  // this also means we have to replace the tag service with a liar.
+  let realTagService = TagNoun._msgTagService;
+  TagNoun._msgTagService = {
+    isValidKey: function() {return false;} // lies!
+  };
+
+  // -- forget about the message, gloda!
+  let glodaId = gmsg.id;
+  nukeGlodaCachesAndCollections();
+
+  // -- re-load the message
+  let query = Gloda.newQuery(Gloda.NOUN_MESSAGE);
+  query.id(glodaId);
+  let coll = queryExpect(query, msgSet);
+  yield false; // queryExpect is async
+
+  // -- put the tag back in TagNoun before we check and possibly explode
+  TagNoun._tagMap[tag.key] = tag;
+  TagNoun._msgTagService = realTagService;
+
+  // -- verify the message apparently has no tags (despite no reindex)
+  gmsg = coll.items[0];
+  do_check_eq(gmsg.tags.length, 0);
+}
+tests.unshift(test_fromjson_of_removed_tag);
+
+/**
+ * Test that we are using hasOwnProperty or a properly guarding dict for
+ *  NounTag so that if someone created a tag called "watch" and then deleted
+ *  it, we don't end up exposing the watch function as the tag.
+ *
+ * Strictly speaking, this does not really belong here, but it's a matched set
+ *  with the previous test.
+ */
+function test_nountag_does_not_think_it_has_watch_tag_when_it_does_not() {
+  do_check_eq(TagNoun.fromJSON("watch"), undefined);
+}
+tests.unshift(test_nountag_does_not_think_it_has_watch_tag_when_it_does_not);
 
 function run_test() {
   configure_message_injection({mode: "local"});
   glodaHelperRunTests(tests);
 }