From 7197b6c6ee2399447424d5f92b224a8c95a618c5 Mon Sep 17 00:00:00 2001 From: Stephen Sawchuk Date: Thu, 31 Jul 2014 18:58:25 -0400 Subject: [PATCH 1/2] fixes #38 datastore.get returns key/prop combined object. --- README.md | 4 ++-- lib/datastore/index.js | 17 +++++++++-------- regression/datastore.js | 26 +++++++++++++------------- test/datastore.js | 31 +++++++++++++++++-------------- 4 files changed, 41 insertions(+), 37 deletions(-) diff --git a/README.md b/README.md index 5cd5472dd5e..44ab7440571 100644 --- a/README.md +++ b/README.md @@ -101,11 +101,11 @@ TODO Get operations require a valid key to retrieve the key identified entity from Datastore. Skip to the "Querying" section if you'd like to learn more about querying against Datastore. ~~~~ js -ds.get(['Company', 123], function(err, key, obj) { +ds.get(['Company', 123], function(err, entities) { }); // alternatively, you can retrieve multiple entities at once. -ds.getAll([key1, key2, ...], function(err, keys, objs) { +ds.getAll([key1, key2, ...], function(err, entities) { }); ~~~~ diff --git a/lib/datastore/index.js b/lib/datastore/index.js index 7d54d38ea1a..a8ec67d7dae 100644 --- a/lib/datastore/index.js +++ b/lib/datastore/index.js @@ -99,11 +99,11 @@ Transaction.prototype.finalize = function(callback) { * @param {Function} callback */ Transaction.prototype.get = function(key, callback) { - this.getAll([key], function(err, keys, objs) { - if (err || objs.length < 0) { + this.getAll([key], function(err, results) { + if (err) { return callback(err); } - return callback(null, keys[0], objs[0]); + return callback(null, results[0]); }); }; @@ -127,12 +127,13 @@ Transaction.prototype.getAll = function(keys, callback) { if (err) { return callback(err); } - var results = [], keys = []; - resp.found.forEach(function(f) { - keys.push(entity.keyFromKeyProto(f.entity.key)); - results.push(entity.entityFromEntityProto(f.entity)); + var results = resp.found.map(function(f) { + return { + key: entity.keyFromKeyProto(f.entity.key), + data: entity.entityFromEntityProto(f.entity) + }; }); - callback && callback(null, keys, results); + callback && callback(null, results); }); }; diff --git a/regression/datastore.js b/regression/datastore.js index eb7c50ea656..75857090d28 100644 --- a/regression/datastore.js +++ b/regression/datastore.js @@ -39,9 +39,9 @@ describe('datastore', function() { ds.save(['Post', postKeyName], post, function(err, key) { if (err) return done(err); assert.equal(key[1], postKeyName); - ds.get(['Post', postKeyName], function(err, key, obj) { + ds.get(['Post', postKeyName], function(err, entity) { if (err) return done(err); - assert.deepEqual(obj, post); + assert.deepEqual(entity.data, post); ds.del(['Post', postKeyName], function(err) { if (err) return done(err); done(); @@ -65,9 +65,9 @@ describe('datastore', function() { ds.save(['Post', postKeyId], post, function(err, key) { if (err) return done(err); assert.equal(key[1], postKeyId); - ds.get(['Post', postKeyId], function(err, key, obj) { + ds.get(['Post', postKeyId], function(err, entity) { if (err) return done(err); - assert.deepEqual(obj, post); + assert.deepEqual(entity.data, post); ds.del(['Post', postKeyId], function(err) { if (err) return done(err); done(); @@ -90,9 +90,9 @@ describe('datastore', function() { if (err) return done(err); assert(key[1]); var assignedId = key[1]; - ds.get(['Post', assignedId], function(err, key, obj) { + ds.get(['Post', assignedId], function(err, entity) { if (err) return done(err); - assert.deepEqual(obj, post); + assert.deepEqual(entity.data, post); ds.del(['Post', assignedId], function(err) { if (err) return done(err); done(); @@ -126,9 +126,9 @@ describe('datastore', function() { assert.equal(keys.length,2); var firstKey = ['Post', keys[0][1]], secondKey = ['Post', keys[1][1]]; - ds.getAll([firstKey, secondKey], function(err, keys, objs) { + ds.getAll([firstKey, secondKey], function(err, entities) { if (err) return done(err); - assert.equal(objs.length, 2); + assert.equal(entities.length, 2); ds.delAll([firstKey, secondKey], function(err) { if (err) return done(err); done(); @@ -350,9 +350,9 @@ describe('datastore', function() { 'url': 'www.google.com' }; ds.runInTransaction(function(t, tDone) { - ds.get(key, function(err, keyRes, objRes) { + ds.get(key, function(err, entity) { if (err) return done(err); - if (objRes) { + if (entity) { tDone(); return; } else { @@ -365,10 +365,10 @@ describe('datastore', function() { }); }, function(err) { if (err) throw (err); - ds.get(key, function(err, keyRes, objRes) { + ds.get(key, function(err, entity) { if (err) return done(err); - assert.deepEqual(objRes, obj); - ds.del(keyRes, function(err) { + assert.deepEqual(entity.data, obj); + ds.del(entity.key, function(err) { if (err) return done(err); done(); }) diff --git a/test/datastore.js b/test/datastore.js index 444b71d9ef4..82e2e1474f4 100644 --- a/test/datastore.js +++ b/test/datastore.js @@ -89,13 +89,14 @@ describe('Dataset', function() { assert.equal(proto.keys.length, 1); callback(null, mockResp_get); }; - ds.get(['Kind', 123], function(err, key, obj) { - assert.deepEqual(key, ['Kind', 5732568548769792]); - assert.strictEqual(obj.name, 'Burcu'); - assert.deepEqual(obj.bytes, new Buffer('hello')); - assert.strictEqual(obj.done, false); - assert.deepEqual(obj.total, 6.7); - assert.strictEqual(obj.createdat.getTime(), 978307200000); + ds.get(['Kind', 123], function(err, entity) { + var properties = entity.data; + assert.deepEqual(entity.key, ['Kind', 5732568548769792]); + assert.strictEqual(properties.name, 'Burcu'); + assert.deepEqual(properties.bytes, new Buffer('hello')); + assert.strictEqual(properties.done, false); + assert.deepEqual(properties.total, 6.7); + assert.strictEqual(properties.createdat.getTime(), 978307200000); done(); }); }); @@ -108,13 +109,15 @@ describe('Dataset', function() { callback(null, mockResp_get); }; ds.getAll([ - ['Kind', 123]], function(err, keys, objs) { - assert.deepEqual(keys[0], ['Kind', 5732568548769792]); - assert.strictEqual(objs[0].name, 'Burcu'); - assert.deepEqual(objs[0].bytes, new Buffer('hello')); - assert.strictEqual(objs[0].done, false); - assert.deepEqual(objs[0].total, 6.7); - assert.strictEqual(objs[0].createdat.getTime(), 978307200000); + ['Kind', 123]], function(err, entities) { + var entity = entities[0]; + var properties = entity.data; + assert.deepEqual(entity.key, ['Kind', 5732568548769792]); + assert.strictEqual(properties.name, 'Burcu'); + assert.deepEqual(properties.bytes, new Buffer('hello')); + assert.strictEqual(properties.done, false); + assert.deepEqual(properties.total, 6.7); + assert.strictEqual(properties.createdat.getTime(), 978307200000); done(); }); }); From 28432751da43e35464ebf8e5be1a9aea70681d5d Mon Sep 17 00:00:00 2001 From: Stephen Sawchuk Date: Fri, 1 Aug 2014 13:45:21 -0400 Subject: [PATCH 2/2] datastore.runQuery returns key/prop combined object. --- README.md | 2 +- lib/datastore/entity.js | 9 +++++ lib/datastore/index.js | 23 ++++--------- regression/datastore.js | 64 ++++++++++++++++++------------------ test/datastore.js | 73 +++++++++++++++++++++++++++++++++++++++++ 5 files changed, 122 insertions(+), 49 deletions(-) diff --git a/README.md b/README.md index 44ab7440571..fe18f9fd535 100644 --- a/README.md +++ b/README.md @@ -150,7 +150,7 @@ also supported. ~~~~ js // retrieves 5 companies var q = ds.createQuery('Company').limit(5); -ds.runQuery(q, function(err, keys, objs, nextQuery) { +ds.runQuery(q, function(err, entities, nextQuery) { // nextQuery is not null if there are more results. if (nextQuery) { ds.runQuery(nextQuery, callback); diff --git a/lib/datastore/entity.js b/lib/datastore/entity.js index f1c2e7e06d9..350df990a92 100644 --- a/lib/datastore/entity.js +++ b/lib/datastore/entity.js @@ -110,6 +110,15 @@ var keyToKeyProto = function(datasetId, key) { module.exports.keyToKeyProto = keyToKeyProto; +module.exports.formatArray = function(results) { + return results.map(function (result) { + return { + key: keyFromKeyProto(result.entity.key), + data: entityFromEntityProto(result.entity) + }; + }); +}; + module.exports.isKeyComplete = function(key) { var proto = keyToKeyProto(null, key); for (var i = 0; i < proto.path.length; i++) { diff --git a/lib/datastore/index.js b/lib/datastore/index.js index a8ec67d7dae..6a204bf3622 100644 --- a/lib/datastore/index.js +++ b/lib/datastore/index.js @@ -114,6 +114,7 @@ Transaction.prototype.get = function(key, callback) { * @param {Function} callback */ Transaction.prototype.getAll = function(keys, callback) { + callback = callback || util.noop; keysProto = []; keys.forEach(function(k) { keysProto.push(entity.keyToKeyProto(this.id, k)); @@ -127,13 +128,8 @@ Transaction.prototype.getAll = function(keys, callback) { if (err) { return callback(err); } - var results = resp.found.map(function(f) { - return { - key: entity.keyFromKeyProto(f.entity.key), - data: entity.entityFromEntityProto(f.entity) - }; - }); - callback && callback(null, results); + + callback(null, entity.formatArray(resp.found)); }); }; @@ -246,7 +242,7 @@ Transaction.prototype.delAll = function(keys, callback) { * available, a query to retrieve the next page is provided to * the callback function. Example: * - * t.runQuery(q, function(err, keys, objs, nextQuery) { + * t.runQuery(q, function(err, entities, nextQuery) { * if (err) return; * // if next page is available, retrieve more results * if (nextQuery) { @@ -257,6 +253,7 @@ Transaction.prototype.delAll = function(keys, callback) { * @param {Function} opt_callback */ Transaction.prototype.runQuery = function(q, opt_callback) { + opt_callback = opt_callback || util.noop; var req = { readOptions: { transaction: this.id @@ -270,19 +267,13 @@ Transaction.prototype.runQuery = function(q, opt_callback) { } this.makeReq('runQuery', req, function(err, resp) { if (err || !resp.batch || !resp.batch.entityResults) { - return opt_callback && opt_callback(err); + return opt_callback(err); } - var results = resp.batch.entityResults, - keys = [], objs = []; - results.forEach(function(r) { - keys.push(entity.keyFromKeyProto(r.entity.key)); - objs.push(entity.entityFromEntityProto(r.entity)); - }); var nextQuery = null; if (resp.batch.endCursor && resp.batch.endCursor != q.startVal) { nextQuery = q.start(resp.batch.endCursor); } - opt_callback && opt_callback(err, keys, objs, nextQuery); + opt_callback(null, entity.formatArray(resp.batch.entityResults), nextQuery); }); }; diff --git a/regression/datastore.js b/regression/datastore.js index 75857090d28..f189e01e8fa 100644 --- a/regression/datastore.js +++ b/regression/datastore.js @@ -205,17 +205,17 @@ describe('datastore', function() { it('should limit queries', function(done) { var q = ds.createQuery('Character').limit(5); - ds.runQuery(q, function(err, keys, objs, secondQuery) { + ds.runQuery(q, function(err, firstEntities, secondQuery) { if (err) return done(err); - assert.equal(objs.length, 5); + assert.equal(firstEntities.length, 5); assert(secondQuery); - ds.runQuery(secondQuery, function(err, keys, objs, thirdQuery) { + ds.runQuery(secondQuery, function(err, secondEntities, thirdQuery) { if (err) return done(err); - assert.equal(objs.length, 3); + assert.equal(secondEntities.length, 3); // TODO(silvano): it currently requires an additional request that brings // an empty page and a null query //assert.equal(thirdQuery, null) - ds.runQuery(thirdQuery, function(err, keys, objs, fourthQuery) { + ds.runQuery(thirdQuery, function(err, thirdEntities, fourthQuery) { if (err) return done(err); assert.equal(fourthQuery, null); done(); @@ -227,9 +227,9 @@ describe('datastore', function() { it('should filter queries with simple indexes', function(done) { var q = ds.createQuery('Character') .filter('appearances >=', 20); - ds.runQuery(q, function(err, keys, objs, nextQuery) { + ds.runQuery(q, function(err, entities, nextQuery) { if (err) return done(err); - assert.equal(objs.length, 6); + assert.equal(entities.length, 6); done(); }); }); @@ -238,28 +238,28 @@ describe('datastore', function() { var q = ds.createQuery('Character') .filter('family =', 'Stark') .filter('appearances >=', 20); - ds.runQuery(q, function(err, keys, objs, nextQuery) { + ds.runQuery(q, function(err, entities, nextQuery) { if (err) return done(err); - assert.equal(objs.length, 6); + assert.equal(entities.length, 6); done(); }); }); it('should filter by ancestor', function(done) { var q = ds.createQuery('Character').hasAncestor(['Character', 'Eddard']); - ds.runQuery(q, function(err, keys, objs, nextQuery) { + ds.runQuery(q, function(err, entities, nextQuery) { if (err) return done(err); - assert.equal(objs.length, 5); + assert.equal(entities.length, 5); done(); }); }); it('should order queries', function(done) { var q = ds.createQuery('Character').order('+appearances'); - ds.runQuery(q, function(err, keys, objs, nextQuery) { + ds.runQuery(q, function(err, entities, nextQuery) { if (err) return done(err); - assert.equal(objs[0].name, characters[0].name); - assert.equal(objs[7].name, characters[3].name); + assert.equal(entities[0].data.name, characters[0].name); + assert.equal(entities[7].data.name, characters[3].name); done(); }); }); @@ -267,13 +267,13 @@ describe('datastore', function() { it('should select projections', function(done) { var q = ds.createQuery('Character') .select(['name', 'family']); - ds.runQuery(q, function(err, keys, objs, nextQuery) { + ds.runQuery(q, function(err, entities, nextQuery) { if (err) return done(err); - assert.deepEqual(objs[0], { + assert.deepEqual(entities[0].data, { name: 'Arya', family: 'Stark' }); - assert.deepEqual(objs[8], { + assert.deepEqual(entities[8].data, { name: 'Sansa', family: 'Stark' }); @@ -286,15 +286,15 @@ describe('datastore', function() { .offset(2) .limit(3) .order('+appearances'); - ds.runQuery(q, function(err, keys, objs, secondQuery) { + ds.runQuery(q, function(err, entities, secondQuery) { if (err) return done(err); - assert.equal(objs.length, 3); - assert.equal(objs[0].name, 'Robb'); - assert.equal(objs[2].name, 'Catelyn'); - ds.runQuery(secondQuery, function(err, keys, objs, thirdQuery) { - assert.equal(objs.length, 3); - assert.equal(objs[0].name, 'Sansa'); - assert.equal(objs[2].name, 'Arya'); + assert.equal(entities.length, 3); + assert.equal(entities[0].data.name, 'Robb'); + assert.equal(entities[2].data.name, 'Catelyn'); + ds.runQuery(secondQuery, function(err, secondEntities, thirdQuery) { + assert.equal(secondEntities.length, 3); + assert.equal(secondEntities[0].data.name, 'Sansa'); + assert.equal(secondEntities[2].data.name, 'Arya'); done(); }); }); @@ -305,17 +305,17 @@ describe('datastore', function() { .offset(2) .limit(2) .order('+appearances'); - ds.runQuery(q, function(err, keys, objs, nextQuery) { + ds.runQuery(q, function(err, entities, nextQuery) { if (err) return done(err); var startCursor = nextQuery.startVal; var cursorQuery = ds.createQuery('Character') .order('+appearances') .start(startCursor); - ds.runQuery(cursorQuery, function(err, keys, objs, nextQuery) { + ds.runQuery(cursorQuery, function(err, secondEntities, nextQuery) { if (err) return done(err); - assert.equal(objs.length, 4); - assert.equal(objs[0].name, 'Catelyn'); - assert.equal(objs[3].name, 'Arya'); + assert.equal(secondEntities.length, 4); + assert.equal(secondEntities[0].data.name, 'Catelyn'); + assert.equal(secondEntities[3].data.name, 'Arya'); done(); }); }); @@ -324,9 +324,9 @@ describe('datastore', function() { it('should group queries', function(done) { var q = ds.createQuery('Character') .groupBy('alive'); - ds.runQuery(q, function(err, keys, objs, nextQuery) { + ds.runQuery(q, function(err, entities, nextQuery) { if (err) return done(err); - assert.equal(objs.length, 2); + assert.equal(entities.length, 2); done(); }); }) diff --git a/test/datastore.js b/test/datastore.js index 82e2e1474f4..5e496979b45 100644 --- a/test/datastore.js +++ b/test/datastore.js @@ -200,4 +200,77 @@ describe('Dataset', function() { }); }); + describe('runQuery', function() { + var ds; + var query; + var mockResponse = { + withResults: { + batch: { entityResults: mockResp_get.found } + }, + withResultsAndEndCursor: { + batch: { entityResults: mockResp_get.found, endCursor: 'cursor' } + }, + withoutResults: mockResp_get + }; + + beforeEach(function () { + ds = new datastore.Dataset({ projectId: 'test' }); + query = ds.createQuery('Kind'); + }); + + describe('errors', function() { + it('should handle upstream errors', function() { + var upstreamError = new Error('upstream error.'); + ds.transaction.makeReq = function(method, proto, callback) { + assert.equal(method, 'runQuery'); + callback(upstreamError); + }; + + ds.runQuery(query, function(err) { + assert.equal(err, upstreamError); + }); + }); + + it('should handle missing results error', function() { + ds.transaction.makeReq = function(method, proto, callback) { + assert.equal(method, 'runQuery'); + callback('simulated-error', mockResponse.withoutResults); + }; + + ds.runQuery(query, function(err) { + assert.equal(err, 'simulated-error'); + }); + }); + }); + + it('should execute callback with results', function() { + ds.transaction.makeReq = function(method, proto, callback) { + assert.equal(method, 'runQuery'); + callback(null, mockResponse.withResults); + }; + + ds.runQuery(query, function (err, entities) { + assert.ifError(err); + + var properties = entities[0].data; + assert.deepEqual(entities[0].key, ['Kind', 5732568548769792]); + assert.strictEqual(properties.name, 'Burcu'); + assert.deepEqual(properties.bytes, new Buffer('hello')); + assert.strictEqual(properties.done, false); + assert.deepEqual(properties.total, 6.7); + }); + }); + + it('should return a new query if results remain', function() { + ds.transaction.makeReq = function(method, proto, callback) { + assert.equal(method, 'runQuery'); + callback(null, mockResponse.withResultsAndEndCursor); + }; + + ds.runQuery(query, function(err, entities, nextQuery) { + assert.ifError(err); + assert.equal(nextQuery.constructor.name, 'Query'); + }); + }); + }); });