fix(metadatastore): #1577: Add parameterized query

addon/MetadataStore.js

@@ -425,25 +425,28 @@ MetadataStore.prototype = {
   /**
    * Get page metadata (including images) for the given cache_keys.
    * For the missing cache_keys, it simply ignores them and will not
-   * raise any exception. Note that if this function gets called on
-   * a closed or unestablished connection, it returns an empty array
+   * raise any exception. Note that this function throws an exception
+   * if it is called with a closed or unestablished connection
    *
    * @param {Array} cacheKeys an cache key array
    *
    * Returns a promise with the array of retrieved metadata records
    */
   asyncGetMetadataByCacheKey: Task.async(function*(cacheKeys) {
-    const quoted = cacheKeys.map(key => `'${key}'`).join(",");
+    const quoted = cacheKeys.map(key => "?").join(",");
     let metaObjects;
     try {
       metaObjects = yield this.asyncExecuteQuery(
         `SELECT * FROM page_metadata WHERE cache_key IN (${quoted})`,
-        {columns: ["id", "cache_key", "places_url", "title", "type", "description", "media_url", "provider_name"]}
+        {
+          params: cacheKeys,
+          columns: ["id", "cache_key", "places_url", "title", "type", "description", "media_url", "provider_name"]
+        }
       );
     }
     catch (e) {
-      // return immediately if there is any exception gets thrown
-      return [];
+      Cu.reportError("Failed to fetch metadata by cacheKey: ${e.message}");
+      throw e;
     }
 
     // fetch the favicons and images for each metadata object
@@ -462,9 +465,8 @@ MetadataStore.prototype = {
            {columns: ["url", "type", "height", "width", "color"]}
         );
       } catch (e) {
-        // return immediately if there is any exception gets thrown, we do not
-        // want to return the partially fetched metadata entries
-        return [];
+        Cu.reportError("Failed to fetch metadata by cacheKey: ${e.message}");
+        throw e;
       }
       for (let image of images) {
         switch (image.type) {
@@ -501,7 +503,10 @@ MetadataStore.prototype = {
    */
   asyncCacheKeyExists: Task.async(function*(key) {
     try {
-      let metadataLink = yield this.asyncExecuteQuery(`SELECT 1 FROM page_metadata WHERE cache_key = '${key}'`);
+      let metadataLink = yield this.asyncExecuteQuery(
+        "SELECT 1 FROM page_metadata WHERE cache_key = :key",
+        {params: {key}}
+      );
       return metadataLink.length > 0;
     } catch (e) {
       return false;

addon/PreviewProvider.js

@@ -232,8 +232,7 @@ PreviewProvider.prototype = {
   }),
 
   /**
-   * Find the metadata for each link in the database. Note that it'll try to fetch items
-   * from the cache before querying the metadata store
+   * Find the metadata for each link in the database
    */
   _asyncFindItemsInDB: Task.async(function*(links) {
     let cacheKeys = [];
@@ -245,7 +244,13 @@ PreviewProvider.prototype = {
     });
 
     // Hit the database for the missing keys
-    const linksMetadata = yield this._metadataStore.asyncGetMetadataByCacheKey(cacheKeys);
+    let linksMetadata;
+    try {
+      linksMetadata = yield this._metadataStore.asyncGetMetadataByCacheKey(cacheKeys);
+    } catch (e) {
+      Cu.reportError(`Failed to fetch metadata: ${e.message}`);
+      return [];
+    }
     return linksMetadata;
   }),
 

test/test-MetadataStore.js

@@ -183,6 +183,45 @@ exports.test_async_get_single_link_by_cache_key = function*(assert) {
   assert.equal(linkExists, false, "It should return an empty array since it doesn't exist");
 };
 
+function _makeFunkyMetadataFixture() {
+  // Deep copy the metadata fixture
+  let funkyFixture = metadataFixture.map(fixture => Object.assign({}, fixture));
+  let fixture = funkyFixture[0];
+  fixture.cache_key = "I'm funky";
+  fixture = funkyFixture[1];
+  fixture.cache_key = "I'm >*funkier*<";
+  fixture = funkyFixture[2];
+  fixture.cache_key = "I'm the >*funkiest*<); DROP TABLE page_metadata;";
+  return funkyFixture;
+}
+
+exports.test_async_get_single_link_by_cache_key_with_special_characters = function*(assert) {
+  let funkyFixture = _makeFunkyMetadataFixture();
+  yield gMetadataStore.asyncInsert(funkyFixture);
+
+  for (let fixture of funkyFixture) {
+    let linkExists = yield gMetadataStore.asyncCacheKeyExists(fixture.cache_key);
+    assert.equal(linkExists, true, "It should be present in the store");
+  }
+};
+
+exports.test_async_get_link_by_cache_key_with_special_characters = function*(assert) {
+  let funkyFixture = _makeFunkyMetadataFixture();
+  yield gMetadataStore.asyncInsert(funkyFixture);
+
+  for (let fixture of funkyFixture) {
+    let metaObjects = yield gMetadataStore.asyncGetMetadataByCacheKey([fixture.cache_key]);
+    assert.equal(metaObjects.length, 1, "It should fetch one metadata record");
+    let metaObject = metaObjects[0];
+    assert.equal(metaObject.favicons.length, 1, "It should fetch one favicon");
+    assert.equal(metaObject.images.length, fixture.images.length, "It should fetch one favicon");
+  }
+
+  let cacheKeys = funkyFixture.map(fixture => fixture.cache_key);
+  let metaObjects = yield gMetadataStore.asyncGetMetadataByCacheKey(cacheKeys);
+  assert.equal(metaObjects.length, funkyFixture.length, "It should fetch all metadata records");
+};
+
 exports.test_async_get_by_cache_key_in_special_cases = function*(assert) {
   yield gMetadataStore.asyncInsert(metadataFixture);
 
@@ -223,8 +262,13 @@ exports.test_on_an_invalid_connection = function*(assert) {
   assert.ok(error, "It should raise exception if the connection is closed or not established");
 
   let cacheKeys = metadataFixture.map(fixture => fixture.cache_key);
-  let metaObjects = yield gMetadataStore.asyncGetMetadataByCacheKey(cacheKeys);
-  assert.equal(metaObjects.length, 0, "It should return an empty array if the connection is closed or not established");
+  error = false;
+  try {
+    yield gMetadataStore.asyncGetMetadataByCacheKey(cacheKeys);
+  } catch (e) {
+    error = true;
+  }
+  assert.ok(error, "It should raise exception if the connection is closed or not established");
 };
 
 exports.test_color_conversions = function(assert) {