From a7be2d2dc208b5fffd92d3f7060a33598d49a661 Mon Sep 17 00:00:00 2001 From: Eli Bishop Date: Fri, 25 Jan 2019 13:23:55 -0800 Subject: [PATCH] fix broken init logic when there's a key prefix (#6) --- dynamodb_feature_store.js | 35 ++++------- tests/.eslintrc.js | 3 +- tests/dynamodb_feature_store-test.js | 89 +++++++++++++++++++++++++++- 3 files changed, 103 insertions(+), 24 deletions(-) diff --git a/dynamodb_feature_store.js b/dynamodb_feature_store.js index 8d0c352..eaa2be0 100644 --- 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 index 1cc45b6..c26da11 100644 --- 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 index 7c76a1c..397a186 100644 --- 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(); + }); + }); + }); });