Issue 6621

lib/ioIndexedDB.js

+/*
+ * This file is part of Adblock Plus <https://adblockplus.org/>,
+ * Copyright (C) 2006-present eyeo GmbH
+ *
+ * Adblock Plus is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 3 as
+ * published by the Free Software Foundation.
+ *
+ * Adblock Plus is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+"use strict";
+
+
+// from DefaultConfig https://github.com/localForage/localForage/blob/master/src/localforage.js

[kzar, 2018/06/04 11:06:07]

Please could you improve this comment a bit? For example

// Values taken from the default localforage IndexDB configuration
//
https://github.com/localForage/localForage/blob/2cdbd74/src/localforage.js#L4...

(I added line numbers to the link and then replaced "master" with "2cdbd74" in
case it changes the linked file changes in the future.)

[piscoi.georgiana, 2018/06/05 07:15:54]

I didn't knew you can link to line numbers. Thanks! I've updated as per your
suggestion

+const localForageDefs = {

[kzar, 2018/06/04 11:06:07]

Nit: Would you mind naming this one more consistently with the other? For
example perhaps `localforageDbConfig`?

[piscoi.georgiana, 2018/06/05 07:15:55]

Done.

+  dbName: "localforage",
+  storeName: "keyvaluepairs",
+  version: 2
+};
+
+const dbConfig = {
+  dbName: "adbp",

[Sebastian Noack, 2018/06/01 19:15:46]

This acronym seems somewhat bogus. If we abbreviate "Adblock Plus" we use "ABP",
but genrally it's a good idea to avoid abbreviations as much as possible. So I'd
suggest to use "adblockplus" as database name.

[piscoi.georgiana, 2018/06/05 07:15:55]

Done.

+  storeName: "file",
+  keyPath: "fileName",
+  version: 1
+};
+
+let dbInstances = {};

[Sebastian Noack, 2018/06/01 19:15:46]

I wonder whether it would be more appropriate to just have two variables (e.g.
"db" and "dbLocalForage") rather than an object (used as mapping) here.

[kzar, 2018/06/04 11:06:07]

Yea, I agree.

[piscoi.georgiana, 2018/06/05 07:15:54]

Done.

[piscoi.georgiana, 2018/06/05 07:15:55]

Done.

+/**
+ * @type Promise
+ */
+let migrationStatus = {};
+
+const keyPrefix = "file:";
+
+  /**
+   * Handles migrating a file, if found, from localforage db to the new one

[kzar, 2018/06/04 11:06:07]

Would you mind specifying what "the new one" is here?

[piscoi.georgiana, 2018/06/05 07:15:54]

I've changed it, but I'm not 100% sure

+   * @param {string} fileName
+   *    Name of the file to be migrated
+   * @return {Promise}
+   *    Promise to be resolved or rejected once the operation is completed
+   */
+function migrateFile(fileName)
+{
+  let fileData;
+  if (!migrationStatus[fileName])

[kzar, 2018/06/04 11:06:07]

Nit: When the body of an if statement spans multiple lines we add the { }
braces. Did your changes pass ESLint? (If you've not tried ESLint yet have a
look at the README, there are some instructions there.)

[piscoi.georgiana, 2018/06/05 07:15:54]

It did pass. My editor shows me lint errors, but I've also ran the command, just
in case. 

+    migrationStatus[fileName] = new Promise((resolve, reject) =>

[Sebastian Noack, 2018/06/01 19:15:46]

Perhaps, rather than migrating each file on demand, we can get all filenames and
convert them all together upfront. Something like that:

  let migrationDone = getAllFiles().then(
    filenames => Promise.all(filenames.map(migrateFile))
  );

This has following advantages:

* We make sure all files get migrated. So that if at some point we remove
  the migration code, it cannot happen that some files which haven't been
  accessed in a while are still stored in the old format.
* We start the migration as soon as possible, minimizing any delay when
  the IO API is used the first time.
* It seems to simplify the logic, as we don't need to keep track and check
  for files that have already been migrated.

[piscoi.georgiana, 2018/06/05 07:15:55]

Done.

+    {
+      return openDB(localForageDefs)
+      .then(dbInstance =>
+        getFile(fileName, dbInstance, localForageDefs.storeName))
+      .then(file =>
+      {
+        fileData = file;

[Sebastian Noack, 2018/06/01 19:15:46]

Rather than making fileData a non-local variable, I would do it like that:

  .then(file =>
  {
    openDB(dbConfig).then(db =>
    {
      saveFile(file, db, dbConfig.storeName).then(() =>
      {
        ...
      });
    });
  });

+        return openDB(dbConfig);
+      })
+      .then(dbInstance =>
+        saveFile(fileData, dbInstance, dbConfig.storeName))
+      .then(() =>
+        deleteFile(
+          fileName,
+          dbInstances[localForageDefs.dbName],
+          localForageDefs.storeName)
+        ).then(() => resolve())
+      .catch(error =>
+      {
+        // file or store wasn't found so no migration needed
+        if (error.type == "NoSuchFile" || error == "NotFoundError")
+        {
+          return resolve();
+        }
+      });
+    });
+  return migrationStatus[fileName];
+}
+
+function fileToKey(fileName)
+{
+  return keyPrefix + fileName;
+}
+
+function formatFile(name, data)
+{
+  return {
+    fileName: fileToKey(name),
+    content: Array.from(data),
+    lastModified: Date.now()
+  };
+}
+
+function openDB(config)

[kzar, 2018/06/04 11:06:07]

I guess we could destructure `config` here?

  function openDb({dbName, storeName, version})

[piscoi.georgiana, 2018/06/05 07:15:54]

Done.

+{
+  return new Promise((resolve, reject) =>
+  {
+    const {dbName, storeName, version} = config;

[Sebastian Noack, 2018/06/01 19:15:46]

Sorry, this is matter of taste, but at least for the matter of consistency with
the rest of our code, we don't use const wherever possible, but merely as an
assertion in cases where reassigning a variable would inherently break things. 

If you wonder why, you might want to read this article:
https://madhatted.com/2016/1/25/let-it-be

[piscoi.georgiana, 2018/06/05 07:15:55]

For me, I guess it's mostly reflex from working on projects that were
"constantly const", I didn't even notice. Thank you! I've updated the rest of
the code as well.

+
+    if (dbInstances[dbName])
+      return resolve(dbInstances[dbName]);
+
+    const req = indexedDB.open(dbName, version);
+
+    req.onsuccess = (event) =>
+    {
+      dbInstances[dbName] = event.currentTarget.result;
+      return resolve(dbInstances[dbName]);
+    };
+
+    req.onerror = reject;
+
+    req.onupgradeneeded = (event) =>
+    {
+      event
+      .currentTarget
+      .result
+      .createObjectStore(storeName,
+        {
+          keyPath: dbConfig.keyPath,
+          autoIncrement: true
+        });
+    };
+  });
+}
+
+function getObjectStore(dbInstance, storeName)
+{
+  return dbInstance
+    .transaction([storeName], IDBTransaction.READ_WRITE)
+    .objectStore(storeName);
+}
+
+function getFile(fileName, dbInstance, storeName, from)
+{
+  return new Promise((resolve, reject) =>
+  {
+    const store = getObjectStore(dbInstance, storeName);
+    const req = store.get(fileToKey(fileName));
+
+    req.onsuccess = (event) =>

[kzar, 2018/06/04 11:06:07]

Nit: While we don't have a rule about adding / omitting the parenthesis for
arrow function arguments, please could you use them consistently? Here you added
them but elsewhere you omitted them.

[piscoi.georgiana, 2018/06/05 07:15:55]

Done.

+    {
+      const result = event.currentTarget.result;
+      if (result)
+        resolve(result);
+      else
+      reject({type: "NoSuchFile"});

[kzar, 2018/06/04 11:06:07]

Nit: This line isn't indented correctly.

[piscoi.georgiana, 2018/06/05 07:15:54]

Done.

+    };
+    req.onerror = reject;
+  });
+}
+
+function saveFile(data, dbInstance, storeName, from)
+{
+  return new Promise((resolve, reject) =>
+  {
+    const store = getObjectStore(dbInstance, storeName);
+    const req = store.put(data);
+
+    req.onsuccess = resolve;
+    req.onerror = reject;
+  });
+}
+
+function deleteFile(fileName, dbInstance, storeName, from)
+{
+  return new Promise((resolve, reject) =>
+  {
+    const store = getObjectStore(dbInstance, storeName);
+    const req = store.delete(fileToKey(fileName));
+
+    req.onsuccess = resolve;
+    req.onerror = reject;
+  });
+}
+
+exports.IO =
+{
+  /**
+   * Writes text lines to a file.
+   * @param {string} fileName
+   *    Name of the file to be written
+   * @param {Iterable.<string>} data
+   *    An array-like or iterable object containing the lines (without line
+   *    endings)
+   * @return {Promise}
+   *    Promise to be resolved or rejected once the operation is completed
+   */
+

[kzar, 2018/06/04 11:06:07]

Nit: Please could you remove this newline?

[piscoi.georgiana, 2018/06/05 07:15:54]

Done.

+  writeToFile(fileName, data)
+  {
+    return migrateFile(fileName)
+    .then(() => openDB(dbConfig))
+    .then(dbInstance =>
+      saveFile(
+        formatFile(fileName, data),
+        dbInstance,
+        dbConfig.storeName)
+    );
+  },
+
+  /**
+   * Reads text lines from a file.
+   * @param {string} fileName
+   *    Name of the file to be read
+   * @param {TextSink} listener
+   *    Function that will be called for each line in the file
+   * @return {Promise}
+   *    Promise to be resolved or rejected once the operation is completed
+   */
+  readFromFile(fileName, listener)
+  {
+    return migrateFile(fileName)
+      .then(() => openDB(dbConfig))
+      .then(dbInstance =>
+        getFile(
+          fileName,
+          dbInstance,
+          dbConfig.storeName)
+      ).then(entry =>
+      {
+        for (let line of entry.content)
+          listener(line);
+      });
+  },
+
+  /**
+   * Retrieves file metadata.
+   * @param {string} fileName
+   *    Name of the file to be looked up
+   * @return {Promise.<StatData>}
+   *    Promise to be resolved with file metadata once the operation is
+   *    completed
+   */
+  statFile(fileName)
+  {
+    return migrateFile(fileName)
+      .then(() => openDB(dbConfig))
+      .then(dbInstance =>
+        getFile(
+          fileName,
+          dbInstance,
+          dbConfig.storeName)
+      ).then(entry =>
+      {
+        return {
+          exists: true,
+          lastModified: entry.lastModified
+        };
+      })
+      .catch(error =>
+      {
+        if (error.type == "NoSuchFile")
+          return {exists: false};
+        throw error;
+      });
+  },
+
+  /**
+   * Renames a file.
+   * @param {string} fromFile
+   *    Name of the file to be renamed
+   * @param {string} newName
+   *    New file name, will be overwritten if exists
+   * @return {Promise}
+   *    Promise to be resolved or rejected once the operation is completed
+   */
+

[kzar, 2018/06/04 11:06:07]

Nit: Please could you remove this newline?

[piscoi.georgiana, 2018/06/05 07:15:55]

Done.

+  renameFile(fromFile, newName)
+  {
+    return migrateFile(fromFile)
+      .then(() => openDB(dbConfig))
+      .then(dbInstance => getFile(fromFile, dbInstance, dbConfig.storeName))
+      .then(fileData =>
+        saveFile(
+          {
+            fileName: fileToKey(newName),
+            content: fileData.content,
+            lastModified: fileData.lastModified
+          },
+          dbInstances[dbConfig.dbName],
+          dbConfig.storeName)
+      ).then(() =>
+        deleteFile(fromFile, dbInstances[dbConfig.dbName], dbConfig.storeName));
+  }
+};
+