From f0836719c9a0f483fd49af64da5d909d42c72653 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miroslav=20Bajto=C5=A1?= Date: Wed, 8 Oct 2014 17:12:02 +0200 Subject: [PATCH] compiler: improve merging of Arrays and Objects Add more unit-tests to cover various edge cases. Fix issues discovered by these new tests. --- lib/config-loader.js | 84 ++++++++++++++++------- test/compiler.test.js | 150 ++++++++++++++++++++++++++++++++--------- test/helpers/appdir.js | 10 --- 3 files changed, 176 insertions(+), 68 deletions(-) diff --git a/lib/config-loader.js b/lib/config-loader.js index f43fac7..17834eb 100644 --- a/lib/config-loader.js +++ b/lib/config-loader.js @@ -112,7 +112,7 @@ function mergeConfigurations(configObjects, mergeFn) { function mergeDataSourceConfig(target, config, fileName) { for (var ds in target) { - var err = applyCustomConfig(target[ds], config[ds]); + var err = mergeObjects(target[ds], config[ds]); if (err) { throw new Error('Cannot apply ' + fileName + ' to `' + ds + '`: ' + err); } @@ -120,41 +120,73 @@ function mergeDataSourceConfig(target, config, fileName) { } function mergeAppConfig(target, config, fileName) { - var err = applyCustomConfig(target, config); + var err = mergeObjects(target, config); if (err) { throw new Error('Cannot apply ' + fileName + ': ' + err); } } -function applyCustomConfig(target, config) { +function mergeObjects(target, config, keyPrefix) { for (var key in config) { - var value = config[key]; - if (target[key]) { - if (Array.isArray(target[key]) && Array.isArray(value)) { - if (target[key].length == value.length) { - for (var valueIdx in value) { - if (typeof value[valueIdx] === 'object') { - applyCustomConfig(target[key][valueIdx], value[valueIdx]); - } else { - target[key][valueIdx] = value[valueIdx]; - } - } - } else { - return 'override for the option `' + key + - '` is an array and lengths mismatch.'; - } - } else if (typeof target[key] === 'object' && typeof value === 'object') { - applyCustomConfig(target[key], value); - } else { - target[key] = value; - } - } else { - target[key] = value; - } + var fullKey = keyPrefix ? keyPrefix + '.' + key : key; + var err = mergeSingleItemOrProperty(target, config, key, fullKey); + if (err) return err; } return null; // no error } +function mergeSingleItemOrProperty(target, config, key, fullKey) { + var origValue = target[key]; + var newValue = config[key]; + + if (!hasCompatibleType(origValue, newValue)) { + return 'Cannot merge values of incompatible types for the option `' + + fullKey + '`.'; + } + + if (Array.isArray(origValue)) { + return mergeArrays(origValue, newValue, fullKey); + } + + if (typeof origValue === 'object') { + return mergeObjects(origValue, newValue, fullKey); + } + + target[key] = newValue; + return null; // no error +} + +function mergeArrays(target, config, keyPrefix) { + if (target.length !== config.length) { + return 'Cannot merge array values of different length' + + ' for the option `' + keyPrefix + '`.'; + } + + // Use for(;;) to iterate over undefined items, for(in) would skip them. + for (var ix=0; ix < target.length; ix++) { + var fullKey = keyPrefix + '[' + ix + ']'; + var err = mergeSingleItemOrProperty(target, config, ix, fullKey); + if (err) return err; + } + + return null; // no error +} + +function hasCompatibleType(origValue, newValue) { + if (origValue === null || origValue === undefined) + return true; + + if (Array.isArray(origValue)) + return Array.isArray(newValue); + + if (typeof origValue === 'object') + return typeof newValue === 'object'; + + // Note: typeof Array() is 'object' too, + // we don't need to explicitly check array types + return typeof newValue !== 'object'; +} + /** * Try to read a config file with .json extension * @param cwd Dirname of the file diff --git a/test/compiler.test.js b/test/compiler.test.js index d5c18cc..73d812b 100644 --- a/test/compiler.test.js +++ b/test/compiler.test.js @@ -125,29 +125,62 @@ describe('compiler', function() { expect(db).to.have.property('fromJs', true); }); - it('merges Object properties', function() { - var nestedValue = { key: 'value' }; + it('merges new Object values', function() { + var objectValue = { key: 'value' }; appdir.createConfigFilesSync(); appdir.writeConfigFileSync('datasources.local.json', { - db: { nested: nestedValue } + db: { nested: objectValue } }); var instructions = boot.compile(appdir.PATH); var db = instructions.dataSources.db; expect(db).to.have.property('nested'); - expect(db.nested).to.eql(nestedValue); + expect(db.nested).to.eql(objectValue); }); - it('merges nested Object properties', function() { - var nestedValue = 'http://api.test.com'; - appdir.createConfigFilesSync(); + it('deeply merges Object values', function() { + appdir.createConfigFilesSync({}, { + email: { + transport: { + host: 'localhost' + } + } + }); + + appdir.writeConfigFileSync('datasources.local.json', { + email: { + transport: { + host: 'mail.example.com' + } + } + }); + + var instructions = boot.compile(appdir.PATH); + var email = instructions.dataSources.email; + expect(email.transport.host).to.equal('mail.example.com'); + }); + + it('deeply merges Array values of the same length', function() { + appdir.createConfigFilesSync({}, { + rest: { + operations: [ + { + template: { + method: 'POST', + url: 'http://localhost:12345' + } + } + ] + } + + }); appdir.writeConfigFileSync('datasources.local.json', { rest: { operations: [ { template: { - url: nestedValue + url: 'http://api.example.com' } } ] @@ -157,50 +190,103 @@ describe('compiler', function() { var instructions = boot.compile(appdir.PATH); var rest = instructions.dataSources.rest; - expect(rest).to.have.property('operations'); - expect(rest.operations[0]).to.have.property('template'); - expect(rest.operations[0].template).to.have.property('url'); - expect(rest.operations[0].template.method).to.eql('POST'); - expect(rest.operations[0].template.url).to.eql(nestedValue); + expect(rest.operations[0].template).to.eql({ + method: 'POST', // the value from datasources.json + url: 'http://api.example.com' // overriden in datasources.local.json + }); }); it('merges Array properties', function() { - var nestedValue = ['value']; + var arrayValue = ['value']; appdir.createConfigFilesSync(); appdir.writeConfigFileSync('datasources.local.json', { - db: { nested: nestedValue } + db: { nested: arrayValue } }); var instructions = boot.compile(appdir.PATH); var db = instructions.dataSources.db; expect(db).to.have.property('nested'); - expect(db.nested).to.eql(nestedValue); + expect(db.nested).to.eql(arrayValue); }); - it('errors on mismatched arrays', function() { - var nestedValue = 'http://api.test.com'; - appdir.createConfigFilesSync(); - appdir.writeConfigFileSync('datasources.local.json', { - rest: { - operations: [ + it('refuses to merge Array properties of different length', function() { + appdir.createConfigFilesSync({ + nest: { + array: [] + } + }); + + appdir.writeConfigFileSync('config.local.json', { + nest: { + array: [ { - template: { - url: nestedValue - } - }, - { - template: { - method: 'GET', - url: nestedValue - } + key: 'value' } ] } }); expect(function() { boot.compile(appdir.PATH); }) - .to.throw(/an array and lengths mismatch/); + .to.throw(/array values of different length.*nest\.array/); + }); + + it('refuses to merge Array of different length in Array', function() { + appdir.createConfigFilesSync({ + key: [[]] + }); + + appdir.writeConfigFileSync('config.local.json', { + key: [['value']] + }); + + expect(function() { boot.compile(appdir.PATH); }) + .to.throw(/array values of different length.*key\[0\]/); + }); + + it('returns full key of an incorrect Array value', function() { + appdir.createConfigFilesSync({ + toplevel: [ + { + nested: [] + } + ] + }); + + appdir.writeConfigFileSync('config.local.json', { + toplevel: [ + { + nested: [ 'value' ] + } + ] + }); + + expect(function() { boot.compile(appdir.PATH); }) + .to.throw(/array values of different length.*toplevel\[0\]\.nested/); + }); + + it('refuses to merge incompatible object properties', function() { + appdir.createConfigFilesSync({ + key: [] + }); + appdir.writeConfigFileSync('config.local.json', { + key: {} + }); + + expect(function() { boot.compile(appdir.PATH); }) + .to.throw(/incompatible types.*key/); + }); + + it('refuses to merge incompatible array items', function() { + appdir.createConfigFilesSync({ + key: [[]] + }); + appdir.writeConfigFileSync('config.local.json', { + key: [{}] + }); + + expect(function() { boot.compile(appdir.PATH); }) + .to.throw(/incompatible types.*key\[0\]/); }); it('merges app configs from multiple files', function() { diff --git a/test/helpers/appdir.js b/test/helpers/appdir.js index 579e675..1a31be5 100644 --- a/test/helpers/appdir.js +++ b/test/helpers/appdir.js @@ -28,16 +28,6 @@ appdir.createConfigFilesSync = function(appConfig, dataSources, models) { db: { connector: 'memory', defaultForType: 'db' - }, - rest: { - connector: 'rest', - operations: [ - { - template: { - method: 'POST' - } - } - ] } }, dataSources); appdir.writeConfigFileSync ('datasources.json', dataSources);