noodlefrenzy / node-amqp10

amqp10 is a promise-based, AMQP 1.0 compliant node.js client
MIT License
134 stars 56 forks source link

Question: Policy.merge() #246

Open torgeir opened 8 years ago

torgeir commented 8 years ago

Just starting out with node-amqp10 I was trying to create a QpidJava policy with some of the sslOptions overridden.

Browsing through the code I got the impression that something along the lines of this would work. Should this be working, or am I misunderstanding things?

const client = new Amqp.Client(Amqp.Policy.merge({
  connect: {
    options: {
      sslOptions: { 
        keyFile: keyFilePath,
        certFile: certFilePath
      }
    }
  }
}, Amqp.Policy.QpidJava));

It dies with

TypeError: Cannot set property 'defaultSubjects' of undefined
    at helper (/Users/torgeir/Code/amqp-test/node_modules/amqp10/lib/utilities.js:131:18)
    at Object.deepMerge (/Users/torgeir/Code/amqp-test/node_modules/amqp10/lib/utilities.js:143:5)
    at Object.merge (/Users/torgeir/Code/amqp-test/node_modules/amqp10/lib/policies/policy_utilities.js:71:12)
    at Object.module.exports.Policy.merge (/Users/torgeir/Code/amqp-test/node_modules/amqp10/lib/index.js:25:50)
    at Object.<anonymous> (/Users/torgeir/Code/amqp-test/amqp.js:31:28)
    at Module._compile (module.js:409:26)
    at Object.Module._extensions..js (module.js:416:10)
    at Module.load (module.js:343:32)
    at Function.Module._load (module.js:300:12)
    at Function.Module.runMain (module.js:441:10)
    at startup (node.js:139:18)
    at node.js:968:3

Fell back to this, which does seems to work, but is unfortunate as it overrides the defaults.

const QpidJava = Amqp.Policy.QpidJava;
QpidJava.connect.options.sslOptions.keyFile = keyFilePath;
QpidJava.connect.options.sslOptions.certFile = certFilePath;

const client = new Amqp.Client(QpidJava);
mbroadst commented 8 years ago

Hey it looks like this is a bug with our deepMerge implementation, but it also seems to work if you just flip the order of the merged objects:

const client = new Amqp.Client(Amqp.Policy.merge(Amqp.Policy.QpidJava, {
  connect: {
    options: {
      sslOptions: { 
        keyFile: keyFilePath,
        certFile: certFilePath
      }
    }
  }
}));

Sorry for the confusion, maybe @noodlefrenzy can speak to this - otherwise we can change the docs to reflect this form.

torgeir commented 8 years ago

Alright.

it also seems to work if you just flip the order of the merged objects

But that would merge the other way around, no? My extensions into the QpidJava object, which I was trying to avoid.

mbroadst commented 8 years ago

yup totally :) give me a minute maybe I can solve this quickly. Another temporary alternative is that the Policy class itself takes overrides as its first argument, so you can just do let myPolicy = new Policy({ /* all the overrides from the existing qpidjava policy */});

torgeir commented 8 years ago

Hehe, np. An Object.assign will do for now :) Thanks

mbroadst commented 8 years ago

@torgeir oh how I desperately wish to update the codebase to es6... that's sort of on the docket for a v4, but for now we have to deal with all this