apigee-127 / bagpipes Goto Github PK

Hi,
I have to say that the piping system is probably a great idea, it is quiet complicated to stay on top of what's going on though. I'm trying to upgrade an expressed base setup and I'm having various issues with error handling. First thing is that when my application crashes I don't want the error message to go to the client. I would seriously consider only enabling this in debug mode as I don't see that you would ever want that to leak to a client.

To avoid this I previously added an extra error handler at the end which caught all the errors. Worked fine, problem solved. However, if I do that now with bagpipes in place the handler doesn't trigger. I tried to implement a custom error handler through a fitting, but I don't now if this will actually catch all errors (wouldn't in express) and I even want the json_error_handler to be in place. Doing that I figured though that my extra error handler (called within the SwaggerExpress.create callback after swaggerExpress.register) triggers if I use a custom error handler fitting. It even triggers when I copy over the exact same code from json_error_handler. But I got it to work that way.
I disabled CORS for testing and send a request from cross origin to see that security and validator handlers return Will process: no. I expected this to throw an error, which it might do, but my custom fittings afterwards try to process.

So this is basically what I mean, before with express I kind of knew what was going on at the top level, now I don' have a clue anymore what's being processed where. I maybe wouldn't mind if that was on a lower level, but as said before the API will face the public and is therefore a major security risk.

Another example for the new complexity is e.g. passing something from a fitting to a controller. Before I loaded data after the validation passed in a custom handler, added it to res.locals and had it in the controllers. Not sure if that is possible with the new piping system, I got it to work by attaching data to the response as before, but it took time.

Don't get me wrong, I love swagger and it might be I'm just doing it all wrong or missing documentation, but it seems to be a lot more complicated then before. Right now my only option seems to be to downgrade.

1. Clarify that "context.output" should not be set directly as it is overwritten by the callback.
2. Document how "output" target values work.
3. Parallel pipes (especially the shortcut approach) should be introduced as a more advanced concept well after standard serial pipes have been completely explained.

I am trying to use the geocoding example shown in the documentation. Geocoding works fine, but I cannot find a way to get the final results from the context.output property. It seems to be undefined.
I can see it in the debug output, but console.log(context.output) returns undefined.

config/default.yaml

#1. Define a http callout we'll use in our pipe
 google_geocode:
   name: http
   input:
     url: http://maps.googleapis.com/maps/api/geocode/json?sensor=true  
     params:
       address: .request.parameters.address.value[0]

 #2. Defined the pipe flow we'll play
 getAddressLocation:
   - google_geocode            # call the fitting defined in this swagger
   - path: body                # system fitting: get body from output
   - parse: json               # body is a json string, parse to object
   - path: results             # get results from body
   - first                     # get first result
   - path: geometry.location   # output = { lat: n, lng: n }

app.js

'use strict';

var fs = require('fs');
var bagpipes = require('bagpipes');
var yaml = require('js-yaml');

var pipesDefs = yaml.safeLoad(fs.readFileSync('config/default.yaml'));
var pipesConfig = {};
var pipes = bagpipes.create(pipesDefs, pipesConfig);
var pipe = pipes.getPipe('getAddressLocation');

var context = {
    request: { 
        parameters: { 
            address: { 
                value: [ 'Frisco TX. 75034' ]
            }
        }
    }
};
pipes.play(pipe, context);

console.log(context.output);

Thanks for upgrading lodash in this PR to fix the prototype pollution vulnerability

However, it looks like the bagpipes dependency machinepack-http still requires a pre-patched version of lodash, even in its latest release - https://github.com/sailshq/machinepack-http/blob/master/package.json .

This should be remedied per - https://nodesecurity.io/advisories/577 & https://bugzilla.redhat.com/show_bug.cgi?id=CVE-2018-16487

Can this library switch to using a different HTTP dependency such as request (which machinepack-http uses under the hood anyway) or axios (fewer dependencies) that doesn't have any known vulnerabilities? It doesn't look too complicated to replace - https://github.com/apigee-127/bagpipes/blob/master/lib/fittings/http.js

Would you consider connecting to a CI service?

There's so much one can do running tests on his own machine, where on CI you can define different agents running different node versions and so on.

Personally, I use Travis whenever I connect an open-source project to CI, although it covers _n_x platforms - it's rather enough, because it covers all node versions.

the current way forces us to "rename" a pipe in order to pass fittings-config to it.
e.g.

bagpipes:
  my-customized-fitting:
    name:   my-fitting
    custom: setting1
    tuned:  value
  
  my-pipe:
    - my-customized-fitting

Why couldn't it be just:

bagpipes:
  my-fitting:
    custom: setting
    tuned:  value

  my-pipe:
    - my-fitting

I have a problem adding my own fitting.

in my default.yaml config file I have:

fiitingDirs: [ api/fittings]

and I added my fitting file under api/fittings.

as I start my server I get an error saying:
cannot find the module: *******\bagpipes\lib\fittings\FITTING_NAME

it seems to not search for the fitting under api/fitting (where my fitting is placed).

so I've looked at the code and I think that the problem is inside the file bagpipes\lib\fittingTypes\system.js:12
and that this:
pipes.config.fittingsDir
should be changed to
pipes.config.userFittingsDirs
like it have been used in other places in the project

just for info, I'm using bagpipes throw swagger-express

Hi,

There's a bug fix for release 0.1.0 in bagpipes/lib/fittingTypes/user.js when using bagpipes with Jest, however, this hasn't been released?

Would that be possible?

Thanks,

Hi.

The snippet bellow describes a fitting that allows using in parameters a custom attribute x-alias that helps support API changes concerning parameter names.

The use cases which I get to met a lot, are described as:

Having an API that changed a parameter name
I would like to name in the spec only the official new name
so new users will adhere to the new way

Having an API that changed a parameter name
I would like to be able to decide to support the old name
to help backward compatibility

And the question to you:

Now that we can officially support loading user fittings from node_modules, I may wrap it as a package such as bagpipes-fitting-params-alias with it's own tests and all, and let users cherry-pick it into their projects by adding this fitting one step before swagger_validation
Yes, we're ready, lets rock.

OR - alternatively -

No, its too early from your point of view to start diverging modularity, you'd rather it as a PR into bagpipes if at all. In this case, it may not even need to be a separate step. It's a useful feature and same work can be done in fittings/swagger_params_parser.jsaround lines31~41and save a loop iteration overswagger.params` per served request.

What are your thoughts about it?

P.S:
the snippet is expressed only as a thought, I did not ran it yet, but I hope the spirit is clear.
it's written in ES6, but it can very easily be put in ES5 if need be, (just replace const for var and handle the destruction and arrows the old way...

const assert = require('assert');
const debug  = require('debug')('fitting:openapi_params_alias');

module.exports = function create(fittingDef) {
    debug('created');
    return openapi_params_alias;
    
    function openapi_params_alias(ctx, cb) {
        debug('exec');
        const req = ctx.request;
        const { params, operation } = req.swagger;
        
        operation.parameterObjects.forEach( parameter  => {
            const name = parameter.name;
            if (parameter.value) {
                debug('value found on official name [%s]', name);
                return;
            }
            
            const alias = parameter.definition['x-alias'];
            if (!alias) {
                debug('no value and no alias for [%s]', name); //if its required - validator will reject it
                return;
            }
            
            const aliasParam = Object.create(parameter, { name: alias });
            const aliasValue = aliasParam.getValue(req).value;
            
            params[name].value = aliasValue;
            debug('value of parameter [%s] was tried and %sfound under alias [%s]', name, aliasValue ? "" : "NOT ", alias, aliasValue);
        });
        
        cb()
    }
}

Have a look here:
http://npm.anvaka.com/#/view/2d/bagpipes

I bet that most users won't use machine-pack.
Using machine-pack should be a user's choice.

User Story

As a user who wants to manage flow with bagpipes
I want to be able to put pipe definition on top of the file and fitting config bellow
So that I can hold the important parts on top, and the implementation details bellow

More info

Current implementation won't recognize fittings that are yet to be loaded but defined bellow.
That makes the bagpipes list start with the petty details instead of the important story.

e.g. the following does not work, and I very much like it to

bagpipes:
   my-pipe: 
     - fitting1
     - fitting2

   fitting1: 
      name: my-fitting1
      config: value
   
   fitting2:
      name: my-fitting2
      config: value

Hi,

First of all thanks for the great job on swagger-pipes. It is very friendly to used and make apigee 127 even more pleasant.

Then - I have a small concern regarding one of my fitting. Below the very simple code:

module.exports = function create(pipeDef) {

return function searchRetailTransactions(context, cb) {

            return cb(null , JSON.stringify({test: "hey"}));
        }
}

}

this return me a a plain text but I would like it to be json so I added:

context.headers["Content-Type"] = "application/json"

before the callback but it still doesn't work.

This is how is it setup in my swagger doc:

x-swagger-pipes:

searchRetailTransaction:
name: searchRetailTransactions
type: user

searchTransactionFlow:

• searchRetailTransaction

  Could you help me with that ?

Thanks

I have a simple pipe:

  getTruncatedThing:
    - emit:
        fields:
          - id  # tells the "getThing" service to truncate the response to only the id field
    - getThing
  
  getThing:
    name: getThing
    type: user
    input:
      name:
        value: .request.swagger.params.name.value[0]
        default: 'foo'

When the getTruncatedThing pipe is played, the "name" input is omitted from the getThing context. I expect this behavior for amend, but emit doesn't seem like it should also override the input of the next fitting.

Using a list of user defined fitting directories, with one directory that is empty, and a custom fitting with a name exactly 20 characters in length (and potentially other lengths), the control logic determining whether to throw an error or to do nothing and continue to check the next user defined fitting directory is no longer valid.

The problem is here:

Using a user defined fitting directory that doesn't exist, the catch block is executed, and err.message is similar to Cannot find module '/Users/johnsmith/dev/bagpipes/src/fittings/this_is_20_character'. The control logic then checks that there is a path separator before the fitting name (err.message[fittingIndex - 1] === path.sep). This passes. It then checks if the error message character at the index of the fitting name length, which in this case is 20, is not a path separator (err.message[fittingDef.name.length] !== path.sep), which it is, because that character is the beginning of our URL string in the error message. Checking for this arbitrary length in the error message makes no sense.

If I understood what the control logic was attempting to control, I would submit a PR for a fix, but I honestly have no idea what checking the error message index at the length of the fitting name could possibly be doing.

The following test, which demonstrates the above problem, fails whether or not the actual fitting exists in ./fixtures/fittings.

// test/bagpipes.js

it('should attempt to load a fitting from all user defined fitting directories', function(done) {
  // Using 'src/fittings' throws an error, and ensures catch logic is ran
  var userFittingsDirs = [ 'src/fittings', path.resolve(__dirname, './fixtures/fittings') ];
  var pipe = [ 'this_is_20_character' ]; // The name of the fitting is 20 characters, the exact index in which the control logic is invalid
  var bagpipes = Bagpipes.create({ pipe: pipe }, { userFittingsDirs: userFittingsDirs });
  var context = {};
  bagpipes.play(bagpipes.getPipe('pipe'), context);
  context.output.should.eql('test');
  done();
});

Issue #10 suggests that Bagpipes needs to have more flexibility in control and that we should consider at least some kind of notification / callback / event for pipe complete.

Going beyond that, it would be extremely useful to extend this beyond simple notifications to management functions that allow control of the process for things like stepwise control and value inspection for tracing and debugging as a pipe executes.

FYI you should update loadash to >=4.17.5 due to a prototype pollution vulnerability.

See: https://nodesecurity.io/advisories/577

sounds like out of the concern of this package: rendering engines should be a user's choice, and so is the control of what he finds in his node_modules directory...

For your consideration 😉

https://github.com/apigee-127/bagpipes/blob/master/lib/bagpipes.js#L219

perhaps you mean
debugContent('output (context[%s]): %j', target, context[target]); ?

Consider this:

bagpipes:
   some-flow:
    - some-fitting:
         key: false

When I play the pipe - I get the false of the ctx.input as undefined 😢

bug reproduction:

const bagpipes = require('bagpipes');
const pipes = bagpipes.create({ 'foo' : [ { emit: { bar: false } } ] } );
pipes.play( pipes.getPipe( 'foo' ) , ctx = {} );

if( ctx.output.bar !== false) throw new Error('oups. expected false, but got something else');

Thanks

I'd like to push a PR with security patch for review, but get a permissions error.
Is it restricted for some reason?

I cannot figure how to include functions in the configuration for fittings. Using CORS as example I'd like to provide the origin as a function and the only way to do this now is to duplicate the cors fitting in my project's fittings folder like this:

'use strict';

const debug = require('debug')('swagger:cors');
const CORS = require('cors');

// config options: https://www.npmjs.com/package/cors
const options = {
  origin: function() {
    ....
  }
};

module.exports = function create(fittingDef, bagpipes) {
  debug('config: %j', fittingDef);
  const middleware = CORS(Object.assign({}, fittingDef, options));

  return function cors(context, cb) {
    debug('exec');
    middleware(context.request, context.response, cb);
  };
};

What I'd love to be able to just specify the function in Swagger's config.xml :

  bagpipes:
   ...
    cors:
      name: cors
      origin: <Some way to reference the function>

Thank you.

Our default.yaml config file is becoming very large, we'd like to be able to define fittings and pipes in smaller files without using grunt to compile a master config.

Ideally it would be similar to how swagger allows $ref to load an external file in swagger.yaml

Is this an issue others have encountered as well?

Hi,

Me again ! When I try to save data on the context doing context.output = "something", I can't get it from the following user type fitting.

1st fitting code:

   context.output = "something"
   return cb(null, context.output);  // Not sure why we need to pass the response again

The following user fitting does context.input but this is empty.

(I tried adding the below in the swagger file but same story
transformRetailOrder: # <---- this is the 2nd fitting invoked
name: transformRetailOrder
input:
retailOrders:
name: '*' # wildcard to pick entire object
default: NOT_FOUND
)

Currently, the fittings are produced by a synchronous factory.
The factory is provided with fcty(fittingDef, bagpipes).

I would like to be able to introduce fittings that employ asynchronous initiation, and should be able to fail fast in the meaning of stop the process gracefully if any of their data-sources are not available.

A simplified example:

my-paramount-must-be-init-dal initiates itself with a map of values, and subscribes to a channel to be notified whenever they change. outside of it, any access to this map of values is synchronous.
The data-source and the subscription channels, are provided in fittingDef.dalCfg

const dal = require('./my-paramount-must-be-init-dal');
module.exports = myFittingFactory(fittingDef, bagpipes, cb) {
     dal.init(fittingDef.dalCfg, function(err) {
         if (err) return cb(err);
         cb( null, {  get: (id,cb) => cb(null, dal.repo[id]) } ) //not a mistake. sync access to object key
     })
}

To do this without asycn loading of the fitting I have to implement a deferring mechanism to each of the featured dal methods - and that's annoying... :/

since 1.1.1 for some fittings we get this error,
it depends on the length of a fitting.

I commented on it in:
c14ffd2?_pjax=%23js-repo-pjax-container

Hi,

I want to define multiple fittings with parameters in a single pipe.

This works (multiple fittings without parameters):

    # pipe to serve swagger (endpoint is in swagger.yaml)
    swagger_raw:
      - swagger_raw
      - another_fitting

This also works (one fitting with parameters):

    # pipe to serve swagger (endpoint is in swagger.yaml)
    swagger_raw:
      name: swagger_raw
      filter: "^(?!x--.*)"
      privateTags:
        - x-swagger-router-controller
        - x-swagger-pipe

This does not work (array syntax):

    # pipe to serve swagger (endpoint is in swagger.yaml)
    swagger_raw:
      - name: swagger_raw
        filter: "^(?!x--.*)"
        privateTags:
          - x-swagger-router-controller
          - x-swagger-pipe

Error message:

Error: Pipe not found: ^(?!x--.*)
    at Bagpipes.getPipe (/.../node_modules/bagpipes/lib/bagpipes.js:46:11)
    at /.../node_modules/bagpipes/lib/fittings/parallel.js:20:30
    at /.../node_modules/lodash/index.js:3073:15
    at baseForOwn (/.../node_modules/lodash/index.js:2046:14)
    at /.../node_modules/lodash/index.js:3043:18
    at Function.<anonymous> (/.../node_modules/lodash/index.js:3346:13)
    at parallel (/.../node_modules/bagpipes/lib/fittings/parallel.js:18:7)
    at Runner.<anonymous> (/.../node_modules/bagpipes/lib/bagpipes.js:171:7)
    at bound (domain.js:280:14)
    at Runner.runBound (domain.js:293:12)

What I want to do actually:

    # pipe to serve swagger (endpoint is in swagger.yaml)
    swagger_raw:
      - name: swagger_raw
        filter: "^(?!x--.*)"
        privateTags:
          - x-swagger-router-controller
          - x-swagger-pipe
      - name: another_fitting

So, what is the correct syntax to add multiple fittings with parameters to a single pipe?

Best regards,
Norman

Hi.

I got the idea of

var bpCfg = require('config');
var bagpipes = require('bagpipes').create( bpCfg.pipes, bpCfg.options );
bagpipes.play(bagpipes.getPipe(pipeName), ctx)

//wait
ctx.output

But I'm having trouble to figure where do I deliver the final-result callback...

I saw the project in use in swagger-node-runner, and I saw there something with _finish - this doesn't look like a finished API. Is the project immature? I hope not ... ..I mean I hope it is mature...

Help?