Ability to use seneca plugins with express requests and seneca messages

[schermannj]

Changed payload structure of the express requests payload. This allows us to do next things:

• Do request like http://localhost:3000/role/cmd?something=true *inside a seneca plugin: function (msg, resp) { console.log(msg.something); }

This will work for POST and GET requests and we will able to use the same plugin's code for seneca .act('role:arole,cmd:*') method.

[coveralls]

Coverage decreased (-0.2%) to 93.431% when pulling e959db12a6955bf90b77fd295aa802a691777624 on schermannj:master into 271621f84b4df8a18277d58e0d8dceb25bfeceaa on senecajs:master.

[tswaters]

This is definitely a breaking change which will mean a semver major bump.

In terms of parity, the other adapters need to be updated in the same way.

I'd also like to hear what the guys from nearForm think before making breaking changes.

Cursory review -- need to continue to pass all hash keys that are currently passed via args, ie user.

[schermannj]

@tswaters I've added back compatibility, more tests and a few fixed for POST request. So there is no more need to change the major version.

[coveralls]

Coverage decreased (-0.4%) to 93.243% when pulling fcca92c38ec8663ded2ff583d207d0ee991ea1a6 on schermannj:master into 271621f84b4df8a18277d58e0d8dceb25bfeceaa on senecajs:master.

[seneca-bot]

I'll take a look later today

On 9 Feb 2017 9:04 am, "Vladyslav Sheruda" notifications@github.com wrote:

@tswaters https://github.com/tswaters I've added back compatibility, more tests and a few fixed for POST request. So there is no more need to change the major version.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/senecajs/seneca-web-adapter-express/pull/10#issuecomment-278584800, or mute the thread https://github.com/notifications/unsubscribe-auth/ASR4gRCKKk9E4uHYR3A3uW783PM1CLWVks5ratadgaJpZM4L62hz .

[mcdonnelldean]

@schermannj Could you give me more clarity on exactly what this is doing? I'm a little confused looking at the code what benefit it's adding?

[schermannj]

@mcdonnelldean The main problem was that I was not able to use seneca plugin with request and with another seneca microservice. For example:

Suppose we have next seneca plugin: `module.exports = function () {

this.add({role: 'math', cmd: 'product'}, (msg, respond) => {

        let product = msg.left * msg.right;

        respond(null, {answer: product});
    }
);};`

It works fine with seneca messages - I mean if we will call this plugin from another plugin or directly from the seneca, e.g.:

seneca.act({role: 'math', cmd: 'product', left: 2, right: 4});

But if we will call it using seneca-web, express, and seneca-web-adapter-express, we will be not able to reuse the plugin code, because request params will be inside the msg.args.

[mcdonnelldean]

@schermannj Ah ok, I see.

Originally it was this way, but it caused issues. For instance, pushing all the args to the top will mean that your routes could potentially get handled by other pattern matches since the message sent would have n new top level keys.

This also leaves us with two implementations which I don't think makes sense for the totality of the project.

@tswaters What are your thoughts on this?

I'm thinking perhaps a switch / option to namespace to args may be a better way to approach this? On by default but could be turned off if you wanted the reuse?

[mcdonnelldean]

@schermannj Just as an aside on this. I tend to advocate away from reuse at this point in code. Generally we've found having functions that handle, parse, and then hand off to another internal seneca call produced much better results.

[schermannj]

@mcdonnelldean I see, let me check it.

[schermannj]

@mcdonnelldean right now I can't find a place where it can break down, because this plugin uses routes object for routing, which looks like this one { prefix: '/math', pin: 'role:math, cmd:*', map: { product: {GET: true}, sum: {POST: true}, square: {GET: true} } }

And the payload object just passes to seneca as the message object. Could you provide an example where it can fail?

UPD

On by default but could be turned off if you wanted the reuse?

Do you want to add and a flag to options config, e.g. compatible=true, and in this case it will work like I need, and if false, like it works now?

[tswaters]

@mcdonnelldean

In general, I'm OK with the changes. My only concern would be the breaking changes and needing to update all the docs / website / etc... and everyone needing to update everything if they moved to latest version of the module. If we control it with option, these concerns are mitigated.

As for naming / semantics of the option, maybe call it namespace

Default it to args, and set it to whatever your want web stuffs to it to be exposed as.

Blank string or falsy value doesn't use namespace at all and adds {body, query, etc} to root msg.

const payload = Object.assign(
  {request$: request, reply$: reply},
  namespace ? {[namespace]: prepared} : prepared,
)

where prepared is everything that currently resides under args.

edit - actually, this is slightly different than what the PR currently does... it will actually merge in query/params into msg directly -- so requesting /some-route?id=1234 results in payload of {id: 1234} not {query: {id: 1234}} which above will do.

[donnd-t]

Any plans to approve this PR soon? I would like the feature where query/params are merged directly into msg (as referred to above).

[tswaters]

Sorry for not responding. Where did we land on this?

If we merge everything to the root I can see potential for security problems.

With config like this:

{
  "pin": "domain:foo,command:*",
  "map": {
    "some-route": true
  }
}

and domain:foo,command:some-route not defined as an action, you could do something like this:

GET /some-route?role=seneca&cmd=stats

or,

GET /some-route?role=mesh&get=members

or any other defined action that doesn't use domain:*,command:*

When the payload hits the seneca.act call, the message payload could be interpreted for the routing that seneca does.

I could see this cropping up in a few places:

• when using mesh, and the network is not fully meshed, the actions aren't available so during upgrades -- or simply taking out an old microservice could leave stray actions unavailable.

• the way seneca-web-adapter-* works, there's no way to remove old pins from the web app - so it'll happily leave those old pins there on the web application so acting into things that don't exist is possible. Need to restart web service to get around that.

• developer screwing up -- if they provide a pin or mapping that doesn't exist.

Merging user-provided payload to the root payload object seems like a big no-no to me.

Thoughts on this?