Fixed issue with recursion and passing strings into objects.

[1313]

I stumbled upon a bug in when using this merge method directly to merge two query param objects with each other containing only string values.

e.g, target as {userId: "1234"} source as {userId: "64321"}.

This recursively caused the logic for passing strings into objects kick in as follows:
Target = "1234", Source = "64321".
"1234"["64321"] = true;

Hence the modified else clause to be sure that target is an object and not a string.

Regards
Per

[nlf]

Can you provide a test for this?

[1313]

The issue is related to the react-router project which uses this module's merge method directly here. Which might not be a common use case and a bit against encapsulation.

As you only use the merge method within the parse module with one specific use case i don't think i will be able to reproduce the bug through the parse module.

I could however provide you with a test for the merge method itself if you like.

[1313]

Test case to trigger the bug.

var Code = require('code');
var Lab = require('lab');
var merge = require('../lib/utils').merge;

var lab = exports.lab = Lab.script();
var expect = Code.expect;
var describe = lab.experiment;
var it = lab.test;

describe('merge()', function () {
    it('merge two query objects', function (done) {
        var exisitingQuery = {userId: "12345"};
        var newUserQuery = {userId: "76432"};
        expect(merge(exisitingQuery, newUserQuery)).to.deep.equal(newUserQuery);
        done();
    });
});

[nlf]

I think adding additional tests would be fine. The most important thing is that we have test coverage for this, otherwise it's all too easy for this to get broken again in the future. Go ahead and make a test/utils.js module and put the above test in it as part of this pull request. We can flesh it out further later.

[sdemjanenko]

I would like to see this accepted.

[nlf]

I'm going to write some additional tests and get this published soon

[nlf]

Just FYI this wasn't correct behavior either, the expected result of merging { a: 'b' } and { a: 'c' } should be { a: ['b', 'c'] } which is what will happen in version 3.0.0 of this module