diff --git a/addon/MetadataStore.js b/addon/MetadataStore.js index ae606349cb..3329849cb1 100644 --- a/addon/MetadataStore.js +++ b/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; diff --git a/addon/PreviewProvider.js b/addon/PreviewProvider.js index 29a3569907..5ab74f2c89 100644 --- a/addon/PreviewProvider.js +++ b/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; }), diff --git a/test/test-MetadataStore.js b/test/test-MetadataStore.js index aa590dd8a3..da12472d7e 100644 --- a/test/test-MetadataStore.js +++ b/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) {