fix broken init logic when there's a key prefix (launchdarkly#6)

solve-hq · Jan 25, 2019 · a7be2d2 · a7be2d2
1 parent ced995f
commit a7be2d2
Show file tree

Hide file tree

Showing 3 changed files with 103 additions and 24 deletions.
diff --git a/dynamodb_feature_store.js b/dynamodb_feature_store.js
@@ -59,7 +59,7 @@ function dynamoDBFeatureStoreInternal(tableName, options) {
         }
       }
       cb(results);
-    }, function (err) {
+    }).catch(function (err) {
       logger.error('failed to get all ' + kind.namespace +  ': ' + err);
       cb(null);
     });
@@ -70,50 +70,41 @@ function dynamoDBFeatureStoreInternal(tableName, options) {
       .then(function(existingItems) {
         var existingNamespaceKeys = {};
         for (var i = 0; i < existingItems.length; i++) {
-          existingNamespaceKeys[makeNamespaceKey(existingItems[i])] = existingItems[i].version;
+          existingNamespaceKeys[makeNamespaceKey(existingItems[i])] = true;
         }
         delete existingNamespaceKeys[makeNamespaceKey(initializedToken())];
 
         // Write all initial data (without version checks).
         var ops = [];
         allData.forEach(function(collection) {
-          var kindNamespace = collection.kind.namespace;
           collection.items.forEach(function(item) {
             var key = item.key;
-            delete existingNamespaceKeys[kindNamespace + '$' + key];
-            ops.push({ PutRequest: makePutRequest(collection.kind, item) });
+            delete existingNamespaceKeys[namespaceForKind(collection.kind) + '$' + key];
+            ops.push({ PutRequest: { Item: marshalItem(collection.kind, item) } });
           });
         });
 
         // Remove existing data that is not in the new list.
         for (var namespaceKey in existingNamespaceKeys) {
-          var version = existingNamespaceKeys[namespaceKey];
           var namespaceAndKey = namespaceKey.split('$');
-          ops.push({ DeleteRequest: {
-            TableName: tableName,
-            Key: {
-              namespace: namespaceAndKey[0],
-              key: namespaceAndKey[1]
-            },
-            ConditionExpression: 'attribute_not_exists(version) OR version < :new_version',
-            ExpressionAttributeValues: {':new_version': version }
-          }});
+          ops.push({ DeleteRequest: { Key: { namespace: namespaceAndKey[0], key: namespaceAndKey[1] } } });
         }
 
         // Always write the initialized token when we initialize.
-        ops.push({PutRequest: { TableName: tableName, Item: initializedToken() }});
+        ops.push({ PutRequest: { Item: initializedToken() } });
 
         var writePromises = helpers.batchWrite(dynamoDBClient, tableName, ops);
 
-        return Promise.all(writePromises).then(function() { cb && cb(); });
-      },
-      function (err) {
+        return Promise.all(writePromises);
+      })
+      .catch(function (err) {
         logger.error('failed to initialize: ' + err);
-      });
+      })
+      .then(function() { cb && cb(); });
   };
 
   store.upsertInternal = function(kind, item, cb) {
-    var params = makePutRequest(kind, item);
+    var params = makeVersionedPutRequest(kind, item);
 
     // testUpdateHook is instrumentation, used only by the unit tests
     var prepare = store.testUpdateHook || function(prepareCb) { prepareCb(); };
@@ -213,7 +204,7 @@ function dynamoDBFeatureStoreInternal(tableName, options) {
     return null;
   }
 
-  function makePutRequest(kind, item) {
+  function makeVersionedPutRequest(kind, item) {
     return {
       TableName: tableName,
       Item: marshalItem(kind, item),

diff --git a/tests/.eslintrc.js b/tests/.eslintrc.js
@@ -2,6 +2,7 @@ module.exports = {
     "env": {
         "node": true,
         "es6": true,
-        "jasmine": true
+        "jasmine": true,
+        "jest": true
     },
 };
diff --git a/tests/dynamodb_feature_store-test.js b/tests/dynamodb_feature_store-test.js
@@ -1,8 +1,18 @@
 var DynamoDBFeatureStore = require('../dynamodb_feature_store');
 var helpers = require('../dynamodb_helpers');
 var testBase = require('ldclient-node/test/feature_store_test_base');
+var dataKind = require('ldclient-node/versioned_data_kind');
 var AWS = require('aws-sdk');
 
+function stubLogger() {
+  return {
+    debug: jest.fn(),
+    info: jest.fn(),
+    warn: jest.fn(),
+    error: jest.fn()
+  };
+}
+
 describe('DynamoDBFeatureStore', function() {
 
   AWS.config.update({
@@ -13,7 +23,7 @@ describe('DynamoDBFeatureStore', function() {
 
   var dynamodb = new AWS.DynamoDB();
 
-  var table='test-store';
+  var table = 'test-store';
 
   beforeAll(function(done) {
     dynamodb.describeTable({ TableName: table }, function(err) {
@@ -94,6 +104,10 @@ describe('DynamoDBFeatureStore', function() {
     return new DynamoDBFeatureStore(table, {prefix: prefix, cacheTTL: 0});
   }
 
+  function makeStoreWithDefaultPrefix() {
+    return makeStoreWithPrefix('test');
+  }
+
   function makeStoreWithHook(hook) {
     var store = makeStore();
     store.underlyingStore.testUpdateHook = hook;
@@ -108,5 +122,78 @@ describe('DynamoDBFeatureStore', function() {
     testBase.baseFeatureStoreTests(makeStoreWithoutCache, clearTable, false, makeStoreWithPrefix);
   });
 
+  // We run the test suite again here because in the DynamoDB implementation, the prefix is entirely
+  // omitted by default, so we want to make sure all the logic is correct with or without one.
+  describe('uncached with prefix', function() {
+    testBase.baseFeatureStoreTests(makeStoreWithDefaultPrefix, clearTable, false);
+  });
+
   testBase.concurrentModificationTests(makeStore, makeStoreWithHook);
+
+  describe('handling errors from DynamoDB client', function() {
+    var err = new Error('error');
+    var client;
+    var logger;
+    var store;
+
+    beforeEach(() => {
+      client = {};
+      logger = stubLogger();
+      store = new DynamoDBFeatureStore(table, { dynamoDBClient: client, logger: logger });
+    });
+
+    it('error from query in init', done => {
+      var data = { features: { flag: { key: 'flag', version: 1 } } };
+      client.query = (params, cb) => cb(err);
+      store.init(data, function() {
+        expect(logger.error).toHaveBeenCalled();
+        done();
+      });
+    });
+
+    it('error from batchWrite in init', done => {
+      var data = { features: { flag: { key: 'flag', version: 1 } } };
+      client.query = (params, cb) => cb(null, { Items: [] });
+      client.batchWrite = (params, cb) => cb(err);
+      store.init(data, function() {
+        expect(logger.error).toHaveBeenCalled();
+        done();
+      });
+    });
+
+    it('error from get', done => {
+      client.get = (params, cb) => cb(err);
+      store.get(dataKind.features, 'flag', function(result) {
+        expect(result).toBe(null);
+        expect(logger.error).toHaveBeenCalled();
+        done();
+      });
+    });
+
+    it('error from get all', done => {
+      client.query = (params, cb) => cb(err);
+      store.all(dataKind.features, function(result) {
+        expect(result).toBe(null);
+        expect(logger.error).toHaveBeenCalled();
+        done();
+      });
+    });
+
+    it('error from upsert', done => {
+      client.put = (params, cb) => cb(err);
+      store.upsert(dataKind.features, { key: 'flag', version: 1 }, function() {
+        expect(logger.error).toHaveBeenCalled();
+        done();
+      });
+    });
+
+    it('error from initialized', done => {
+      client.get = (params, cb) => cb(err);
+      store.initialized(function(result) {
+        expect(result).toBe(false);
+        expect(logger.error).toHaveBeenCalled();
+        done();
+      });
+    });
+  });
 });