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