Skip to content

Commit

Permalink
Fixes to Provider.save() and tests.
Browse files Browse the repository at this point in the history
Fixed `Provider.save()` to properly ignore stores which do not provide
a saveSync method.  Also, fixed `save()` to properly save asynchronously
when an async `save()` method on a store is provided.

Removed the tests from `nconf-test.js` which expected `save()` to throw
or return an error when a store without `save()` methods was
encountered. Also removed a `console.log` from `provider-test.js`.
  • Loading branch information
Russell Frank committed May 2, 2012
1 parent 29eb5f9 commit 36e061c
Show file tree
Hide file tree
Showing 4 changed files with 35 additions and 39 deletions.
2 changes: 1 addition & 1 deletion lib/nconf/common.js
Original file line number Diff line number Diff line change
Expand Up @@ -108,4 +108,4 @@ common.merge = function (objs) {
//
common.capitalize = function (str) {
return str && str[0].toUpperCase() + str.slice(1);
};
};
52 changes: 32 additions & 20 deletions lib/nconf/provider.js
Original file line number Diff line number Diff line change
Expand Up @@ -373,11 +373,14 @@ Provider.prototype.load = function (callback) {
};

//
// ### function save (value, callback)
// #### @value {Object} **Optional** Config object to set for this instance
// #### @callback {function} Continuation to respond to when complete.
// Removes any existing configuration settings that may exist in this
// instance and then adds all key-value pairs in `value`.
// ### function save (callback)
// #### @callback {function} **optional** Continuation to respond to when
// complete.
// Instructs each provider to save. If a callback is provided, we will attempt
// asynchronous saves on the providers, falling back to synchronous saves if
// this isn't possible. If a provider does not know how to save, it will be
// ignored. Returns an object consisting of all of the data which was
// actually saved.
//
Provider.prototype.save = function (value, callback) {
if (!callback && typeof value === 'function') {
Expand All @@ -388,32 +391,41 @@ Provider.prototype.save = function (value, callback) {
var self = this,
names = Object.keys(this.stores);

function saveStoreSync(name) {
function saveStoreSync(memo, name) {
var store = self.stores[name];

//
// If the `store` doesn't have a `saveSync` method,
// just ignore it and continue.
//
return store.saveSync
? store.saveSync()
: null;
if (store.saveSync) {
var ret = store.saveSync();
if (typeof ret == 'object' && ret !== null) {
memo.push(ret);
}
}
return memo;
}

function saveStore(name, next) {
function saveStore(memo, name, next) {
var store = self.stores[name];

//
// If the `store` doesn't have a `save` or saveSync`
// method(s), just ignore it and continue.
//
if (!store.save && !store.saveSync) {
return next();
}

return store.saveSync
? next(null, store.saveSync())
: store.save(next);
if (store.save) {
store.save(function (err, data) {
if (err) return next(err);
if (typeof data == 'object' && data !== null) {
memo.push(data);
}
});
} else if (store.saveSync) {
memo.push(store.saveSync());
}
next(null, memo);
}

//
Expand All @@ -422,11 +434,11 @@ Provider.prototype.save = function (value, callback) {
// then do so.
//
if (!callback) {
return common.merge(names.map(saveStoreSync));
return common.merge(names.reduce(saveStoreSync, []));
}

async.map(names, saveStore, function (err, objs) {
return err ? callback(err) : callback();
async.reduce(names, [], saveStore, function (err, objs) {
return err ? callback(err) : callback(null, common.merge(objs));
});
};

Expand Down Expand Up @@ -488,4 +500,4 @@ function onError(err, callback) {
}

throw err;
}
}
17 changes: 1 addition & 16 deletions test/nconf-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -101,22 +101,7 @@ vows.describe('nconf').addBatch({
});
}
}
},
"the save() method": {
"without a callback": {
"should throw an exception": function () {
assert.throws(function () { nconf.save() });
}
},
"with a callback": {
topic: function () {
nconf.save(this.callback.bind(null, null));
},
"should respond with an error": function (ign, err) {
assert.isNotNull(err);
}
}
}
}
}
}).export(module);
}).export(module);
3 changes: 1 addition & 2 deletions test/provider-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,6 @@ vows.describe('nconf/provider').addBatch({

helpers.assertMerged(null, merged);
assert.equal(merged.candy.something, 'file1');
console.log(provider.sources);
}
},
"when multiple stores are used": {
Expand All @@ -117,4 +116,4 @@ vows.describe('nconf/provider').addBatch({
}
}
}
}).export(module);
}).export(module);

3 comments on commit 36e061c

@indexzero
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@russfrank This introduced a very serious regression that thankfully was caught before too many upstream versions were affected.

The regression is illustrated in 7e8d9d6 and fixed in d96d254.

When saving stores asynchronously in saveStore() you immediately invoked the next callback instead of waiting for the store.save() to return (if it exists). This causes node.js to exit before the save operation actually completes causing blank files.

@rf
Copy link
Contributor

@rf rf commented on 36e061c Jul 10, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops! sorry about that, glad it was fixed

@indexzero
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Me too .... thanks for the contribution, I liked you changes; except the bug of course :-D

Please sign in to comment.