Skip to content

Commit

Permalink
multiGet breaking test and fix
Browse files Browse the repository at this point in the history
Summary:
the flush + optimized multiGet result in an obscure bug that results when two multiGet requests with overlapping key sets get issued. The result array for both requests ends up bigger than the key array (because it has duplicates)
Closes #5514

Reviewed By: svcscm

Differential Revision: D2908264

Pulled By: nicklockwood

fb-gh-sync-id: 60be1bce4acfc47083e4ae28bb8b63f9dfa56039
  • Loading branch information
mvayngrib authored and facebook-github-bot-0 committed Feb 6, 2016
1 parent 375abc3 commit 52755fd
Show file tree
Hide file tree
Showing 2 changed files with 31 additions and 5 deletions.
18 changes: 18 additions & 0 deletions IntegrationTests/AsyncStorageTest.js
Original file line number Diff line number Diff line change
Expand Up @@ -142,12 +142,30 @@ function testMerge() {
expectAsyncNoError('testMerge/setItem', err3);
expectEqual(JSON.parse(result), VAL_MERGE_EXPECT, 'testMerge');
updateMessage('objects deeply merged\nDone!');
runTestCase('multi set and get', testOptimizedMultiGet);
});
});
});
}

function testOptimizedMultiGet() {
let batch = [[KEY_1, VAL_1], [KEY_2, VAL_2]];
let keys = batch.map(([key, value]) => key);
AsyncStorage.multiSet(batch, (err1) => {
// yes, twice on purpose
;[1, 2].forEach((i) => {
expectAsyncNoError(`${i} testOptimizedMultiGet/multiSet`, err1);
AsyncStorage.multiGet(keys, (err2, result) => {
expectAsyncNoError(`${i} testOptimizedMultiGet/multiGet`, err2);
expectEqual(result, batch, `${i} testOptimizedMultiGet multiGet`);
updateMessage('multiGet([key_1, key_2]) correctly returned ' + JSON.stringify(result));
done();
});
});
});
}


var AsyncStorageTest = React.createClass({
getInitialState() {
return {
Expand Down
18 changes: 13 additions & 5 deletions Libraries/Storage/AsyncStorage.js
Original file line number Diff line number Diff line change
Expand Up @@ -180,14 +180,16 @@ var AsyncStorage = {
// Even though the runtime complexity of this is theoretically worse vs if we used a map,
// it's much, much faster in practice for the data sets we deal with (we avoid
// allocating result pair arrays). This was heavily benchmarked.
//
// Is there a way to avoid using the map but fix the bug in this breaking test?
// https://github.com/facebook/react-native/commit/8dd8ad76579d7feef34c014d387bf02065692264
let map = {};
result.forEach(([key, value]) => map[key] = value);
const reqLength = getRequests.length;
for (let i = 0; i < reqLength; i++) {
const request = getRequests[i];
const requestKeys = request.keys;
var requestResult = result.filter(function(resultPair) {
return requestKeys.indexOf(resultPair[0]) !== -1;
});

let requestResult = requestKeys.map(key => [key, map[key]]);
request.callback && request.callback(null, requestResult);
request.resolve && request.resolve(requestResult);
}
Expand All @@ -214,6 +216,7 @@ var AsyncStorage = {
var getRequest = {
keys: keys,
callback: callback,
// do we need this?
keyIndex: this._getKeys.length,
resolve: null,
reject: null,
Expand All @@ -225,7 +228,12 @@ var AsyncStorage = {
});

this._getRequests.push(getRequest);
this._getKeys.push.apply(this._getKeys, keys);
// avoid fetching duplicates
keys.forEach(key => {
if (this._getKeys.indexOf(key) === -1) {
this._getKeys.push(key);
}
});

return promiseResult;
},
Expand Down

0 comments on commit 52755fd

Please sign in to comment.