Merge pull request #88 from brave/fix/87

change limitResponse to a maxRecords number
brave · May 12, 2017 · 66b5b22 · 66b5b22
2 parents 5017451 + 69936ab
commit 66b5b22
Show file tree

Hide file tree

Showing 4 changed files with 52 additions and 12 deletions.
diff --git a/client/constants/messages.js b/client/constants/messages.js
@@ -51,7 +51,7 @@ const messages = {
    * with new records, do
    * GET_EXISTING_OBJECTS -> RESOLVE_SYNC_RECORDS -> RESOLVED_SYNC_RECORDS
    */
-  FETCH_SYNC_RECORDS: _, /* @param Array.<string> categoryNames, @param {number} startAt (in seconds or milliseconds), @param {boolean=} limitResponse true to limit response to 1000 records */
+  FETCH_SYNC_RECORDS: _, /* @param Array.<string> categoryNames, @param {number} startAt (in seconds or milliseconds), @param {number=} maxRecords limit response to a given max number of records. set to 0 or falsey to not limit the response. */
   /**
    * browser -> webview
    * sent to fetch all sync devices. webview responds with RESOLVED_SYNC_RECORDS.
@@ -61,8 +61,8 @@ const messages = {
    * webview -> browser
    * after sync gets records, it requests the browser's existing objects so sync
    * can perform conflict resolution.
-   * isTruncated is true if limitResponse was used and the total number of
-   * records exceeds the limit (1000).
+   * isTruncated is true if maxRecords was used and the total number of
+   * records exceeds the limit.
    */
   GET_EXISTING_OBJECTS: _, /* @param {string} categoryName, @param {Array.<Object>} records, @param {lastRecordTimeStamp} number, @param {boolean} isTruncated */
   /**

diff --git a/client/requestUtil.js b/client/requestUtil.js
@@ -150,19 +150,19 @@ RequestUtil.prototype.parseAWSResponse = function (bytes) {
  * Get S3 objects in a category.
  * @param {string} category - the category ID
  * @param {number=} startAt return objects with timestamp >= startAt (e.g. 1482435340)
- * @param {boolean=} limitResponse Limit response to 1000 keys, which is AWS's max response for a listObjects request. By default the Sync lib will fetch all matching records, which might take a long time.
+ * @param {number=} maxRecords Limit response to a given number of recods. By default the Sync lib will fetch all matching records, which might take a long time. If falsey, fetch all records.
  * @returns {Promise(Array.<Object>)}
  */
-RequestUtil.prototype.list = function (category, startAt, limitResponse) {
+RequestUtil.prototype.list = function (category, startAt, maxRecords) {
   const prefix = `${this.apiVersion}/${this.userId}/${category}`
   let options = {
-    MaxKeys: 1000,
+    MaxKeys: maxRecords || 1000,
     Bucket: this.bucket,
     Prefix: prefix
   }
   if (startAt) { options.StartAfter = `${prefix}/${startAt}` }
   return this.withRetry(() => {
-    return s3Helper.listObjects(this.s3, options, limitResponse)
+    return s3Helper.listObjects(this.s3, options, !!maxRecords)
   })
 }
 

diff --git a/lib/s3Helper.js b/lib/s3Helper.js
@@ -81,7 +81,7 @@ module.exports.parseS3Key = function (key) {
  * listObjectsV2 plus follow all cursors.
  * @param {AwsSdk.S3} s3
  * @param {Object} options
- * @param {boolean=} limitResponse Limit response to 1000 keys, which is AWS's max response for a listObjects request. By default the Sync lib will fetch all matching records, which might take a long time.
+ * @param {boolean=} limitResponse Limit response to a given number. By default the Sync lib will fetch all matching records, which might take a long time.
  * @returns {Promise}
  */
 module.exports.listObjects = function (s3, options, limitResponse) {

diff --git a/test/client/requestUtil.js b/test/client/requestUtil.js
@@ -259,10 +259,50 @@ test('client RequestUtil', (t) => {
     }
 
     const testCanLimitResponse = (t) => {
-      t.test('limitResponse true', (t) => {
-        t.plan(1)
-        requestUtil.list(proto.categories.PREFERENCES, 0, true)
-          .then(s3Objects => t.assert(s3Objects.isTruncated === false, `${t.name} has boolean isTruncated value`))
+      t.test('limitResponse undefined', (t) => {
+        t.plan(3)
+        const siteSettingRecord = {
+          action: 'CREATE',
+          deviceId: new Uint8Array([0]),
+          objectId: testHelper.newUuid(),
+          siteSetting: {hostPattern: 'https://google.com', shieldsUp: true}
+        }
+
+        requestUtil.put(proto.categories.PREFERENCES, encrypt(siteSettingRecord))
+          .then(() => {
+            requestUtil.list(proto.categories.PREFERENCES, 0)
+              .then((s3Objects) => {
+                t.assert(s3Objects.isTruncated === false, `${t.name} has false isTruncated value`)
+                t.assert(s3Objects.contents.length === 2, `${t.name} has two records`)
+                testCanLimitResponseToZero(t)
+              })
+              .catch((error) => t.fail(error))
+          })
+          .catch((error) => t.fail(error))
+      })
+    }
+
+    const testCanLimitResponseToZero = (t) => {
+      t.test('limitResponse to 0', (t) => {
+        t.plan(3)
+        requestUtil.list(proto.categories.PREFERENCES, 0, 0)
+          .then((s3Objects) => {
+            t.assert(s3Objects.isTruncated === false, `${t.name} has false isTruncated value`)
+            t.assert(s3Objects.contents.length === 2, `${t.name} has two records`)
+            testCanLimitResponseToOne(t)
+          })
+          .catch((error) => t.fail(error))
+      })
+    }
+
+    const testCanLimitResponseToOne = (t) => {
+      t.test('limitResponse to 1', (t) => {
+        t.plan(2)
+        requestUtil.list(proto.categories.PREFERENCES, 0, 1)
+          .then((s3Objects) => {
+            t.assert(s3Objects.isTruncated === true, `${t.name} has true isTruncated value`)
+            t.assert(s3Objects.contents.length === 1, `${t.name} has one record`)
+          })
           .catch((error) => t.fail(error))
       })
     }