Empty objects in Config File Merge are assigned, not merged

[shookie]

Empty objects in Config File Merge are assigned, not merged

When multiple config files are merged later empty objects under key are not merged but overwrite/clear earlier non-empty objects under the same key.

It may be possible that this behavior is wanted, but it is highly inconvenient and can be a nasty surprise.

• ActionHero Version: 19.1.1
• Node.js Version: 8.x.x
• Operating System: all

Steps to reproduce your error

file config/A.js

exports.default = {
  key:  { 
      A: 1
  }
}

and they key in file B is an empty object
file config/B.js

exports.default = {
  key:  { 
  }
}

then key will be empty once all configurations are parsed and loaded, because B.js is alphabetically loaded after A.js

• to show this do a console.dir(api.config.key) in any start method

The problem is in the api.utils.hashMerge function where for both arguments the

if (api.utils.isPlainObject(a[i]) && Object.keys(a[i]).length > 0)

condition falls through if the object is empty, and you end up assigning in the last branch.

c[i] = b[i]

the empty object case should be its own branch and just do nothing.

[evantahler]

Can you share more about the use case in your configs in which the difference between undefined and null are useful? I'm facinated!

Either way, the requested change here seems ok! Do you need help creating the pull request and tests for this?

[evantahler]

sorry! I misread your bug... yes, I agree that full empty objects should be "passthroughs" on configs. Do you need help creating the pull request and tests for this?

[evantahler]

After conversations, this is not a bug.

In your example, you are explicitly setting Key = {}, which is a replacement for the original value. Both undefined and null are properly treated as passthroughs. An empty hash is not the same as null

[shookie]

Ok, maybe we misunderstand each other, and I should explain myself better. But maybe it really is the wanted behavior. But I have already heard from others that ran into issues from this merging behavior.

The use case isn't really about null or undefined, for both of which it is completely fine if later values overwrite earlier values. The use case is about empty objects being treated differently than non-empty-objects on merge.

The whole purpose of having the configuration distributed over many files is to allow grouping of configuration values that logically belong together into files, but still have the whole merged configuration available at run time.

So, you define a configuration structure that all the different config files must adhere to, because the code expects the values at the locations it expects them.

Maybe a more instructive example would help, the test case I defined is just the minimal test case, but it doesn't demonstrate why it is needed.

// config/departmentA.js
exports.default = {
  user:  { 
     john: { email: "john@company.com", rights: 0x1a77 },
     billy: { email: "billy@company.com", rights: 0x6a03 },
  }
}

// config/departmentB.js
exports.default = {
  // user is merged with previous file, all users are configured
  user:  { 
     annie: { email: "annie@company.com", rights: 0x5556 },
     george: { email: "george@company.com", rights: 0xFFFF },
  }
}

// config/departmentC.js
// a new department is created, and an predefined config file for it is created too
// there aren't just any users yet
// which means, once actionhero reads all configs, all users from the whole company are gone
exports.default = {
  // all previous users are overwritten by empty user object, no one is configured
  user:  { 
     // add new users here
     // username: { email: "username@company.com", rights: 0x0000 }
  }
}

With the patch I have provided, all empty objects are ignored, which means later empty objects don't overwrite/delete the configuration values in earlier non-empty objects. It also means you can't create preexisting keys with empty objects as their value, which is possible with the existing code.

If both should be possible, i.e. creating empty objects under a new key, and having empty objects keys not overwrite existing keys, then I need to adjust the provided patch a little.

But as it stands, the merging of objects does not happen according to the well defined mathematical set operators.

Current behavior:

{} + {} = {}
{ A, B } = { A, B }
{} + { A, B } = { A, B }
{ A, B } + {} = {}
{ A, B } + { C, D } = { A, B, C, D}
{ A, B } + { C, D } + {} = {}

And the last part is the problem.

My fix makes it so that

{} + {} = undefined
{ A, B } = { A, B }
{} + { A, B } = { A, B }
{ A, B } + {} = { A, B }
{ A, B } + { C, D } = { A, B, C, D}
{ A, B } + { C, D } + {} = { A, B, C, D}

Ideal would probably be

{} + {} = {}
{ A, B } = { A, B }
{} + { A, B } = { A, B }
{ A, B } + {} = { A, B }
{ A, B } + { C, D } = { A, B, C, D}
{ A, B } + { C, D } + {} = { A, B, C, D}

but for that I need to adjust my fix a little.

[evantahler]

Thank you so much for this thoughtful and complete writeup! For me personally, I finally got it with the mathematical proof at the end.

I agree that with configuration, and the name of the method including "merge", the mathematical model you describe at the end is the proper behavior set. I think it also might be important to handle the null and undefined cases as well:

{} + {} = {}
{ A, B } = { A, B }
{} + { A, B } = { A, B }
{ A, B } + {} = { A, B }
{ A, B } + { C, D } = { A, B, C, D}
{ A, B } + { C, D } + {} = { A, B, C, D}
{ A, B } + { C, D } + undefined = { A, B, C, D}
{ A, B } + { C, D } + null = null

I believe there is also a use-case when you want to "erase" all previous data in the set(s), and passing null is probably the way to do it? Maybe?'

If you can adjust your PR to match this modeling, I'll merge it!

[shookie]

Ok, I'll make a new patch ready, which implements the behavior you have described last. Maybe Today, more likely tomorrow. Testing and stuff.