noodlefrenzy / node-amqp10

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

Export ForcedType and AMQPArray #276

Open princjef opened 8 years ago

princjef commented 8 years ago

Sorry for the blitz of issues. I was working through a long scenario and wanted to make sure it worked end-to-end before raising issues for the things I needed to get it working 😄

I am trying to send a message whose body is an AMQP value with the following format:

{
    "lock-tokens": ["ec59a901-4643-4980-8aae-62db992610c8"/*uuid*/]
}

The problem is that the service I'm talking to specifically expects a map with an array as the value for key "lock-tokens" and uuids as entries in the array. the default encoding from JSON will produce a map, but arrays get turned into lists by default and strings are turned into strings, not uuids.

While I could specify a custom encoder to perform the correct encoding for the body, it seems more straightforward to make use of the type classes that are already available in node-amqp10. Would it be reasonable to export these classes so that I can send something like this instead and guarantee the encoding of my body is what I'm expecting?

new ForcedType("map", {
    "lock-tokens": new AMQPArray(["ec59a901-4643-4980-8aae-62db992610c8"], 0x98)
})
mbroadst commented 8 years ago

@princjef could something like this help here: https://github.com/noodlefrenzy/node-amqp10/blob/master/lib/index.js#L37, you should be able to define types like:

[ 'map', { some: 'map' } ]

or

[ 'map', [ 'symbol', 'Key 1' ], [ 'ulong', 1 ], [ 'symbol', 'Key 2' ], [ 'boolean', false ] ]

I'd really prefer to figure out how to make this thing more natural, and complete than expose the internal classes if possible.

princjef commented 8 years ago

@mbroadst Yeah that's reasonable. I had seen node-amqp-encoder but not the translator.

I'll play around with it a bit and see if I can get it to work. At the very least extending that would probably be preferable to exporting the classes

mbroadst commented 8 years ago

@princjef yeah I mean, if you have the time it would be really appreciated. I'd like to make that (or whatever replaces it) the default method for explicit typing, so if we can think up a great interface together that would be fantastic.

mbroadst commented 8 years ago

@princjef for instance, I kind of like the idea of exporting something like translator as type instead, maybe with properties for each type. e.g.:

const type = require('amqp10').type;
let message = type.map({ some: type.array([10, 20, 30]) });

just spitballing

princjef commented 8 years ago

@mbroadst yeah something like that would be incredibly useful. I should have some time here and there and would be happy to do some brainstorming/coding. A good explicit type format would be very helpful in resolving some of the encoding issues people have.

I do like wrapping values with a type because it still keeps the general structure, just with some extra annotations. The big question with that approach is whether having it be serializable to JSON is important (which is the main benefit of the node-amqp-encoder format to me).

As for the specific use case from this issue, I ran into a couple of problems with the array logic:

  1. uuid is not included in node-amqp-encoder, so it fails when the array entries are encoded. I can submit a PR there to fix that
  2. Even if the type is one of the supported values, it doesn't work. The processor expects the type to be specified as a string, but AMQPArray expects the type as a number. If a string is specified, an extra 0x00 byte is inserted where the code should be (since it's trying to put a string into a byte) and the code is added onto the beginning of each entry. Seems like arrayBuilder needs a string mode.
mbroadst commented 8 years ago

@princjef sorry I let this one get away from me. Can you provide a small example of how the translator is failing you here? It actually occurs to me that the biggest gap in our code coverage is around the translator, so I'd like to get some unit tests in there for that 😄

princjef commented 8 years ago

@mbroadst no worries. I had actually given some thought to a new translation/type API like you mentioned but haven't had time to do much with it.

If I remember correctly, it was anything involving an array. I think it was something like this, but you should run into the same problem for any array filled with anything (uuid won't work because of a gap in node-amqp-encoder):

['map',
    ['string', 'lock-tokens'], ['array', 'uuid', '88f54527-e090-4315-a3d3-88649300ca78', 'eafaa684-b7d5-49e5-ba3d-80f07321694e']
]