cors: name collision

[osher]

I'm trying to create some reusable pipe fittings packed as simple npm packages.

So to prove the concept I placed a simple node_modules/required-fitting/index.js (together with a standard package.json) in same folder, as follows:

var debug = require('debug')('fitting:required-fitting');
module.exports = function create(fittingDef) {
    console.log('required-fitting', fittingDef);
    return function stam(ctx, cb) {
        console.log('REQUIRED-FITTING IS CALLED');
        cb()
    }
}

And added to the config:

So:

swagger:
  fittingsDirs:             [ lib/fittings, node_modules ]
  ...

Alas - the pipe explodes. The error is observed on the response as {"message":"undefined is not a function"}, with NO MENTION of it on the server log

It appears to explode on cors, which is now ambiguous between the two paths:

• node_modules/cors
• lib/fitting/cors.js

playing with order of paths in fittingsDir did not help :(

I can PR to rename lib/fitting/cors.js to lib/fitting/swagger_cors.js or anytihng of that sort, however, it looks like a breaking change.

I think the situation points also to an additional problem: I think the fitting builder that calls the fitting factory may be required to make sure that the factory returns a function in the correct arity. (the vanilla cors package is a f(1) factory that returns f(3))

I did not miss the part about type:node-machine that requires some overhead plus pushes me to experimental zone, which I would totally prefer to avoid at this stage... my client wont approve of anything with experimental labelled on it...

Thoughts?

[osher]

baaah. found a workaround.

lib/fittings/cors-patch.js in user-project as:

module.exports = require('swagger-node-runner/fittings/cors');

where config/default.yaml states

swagger:
  fittingsDirs:             [ lib/fittings, node_modules  ]
  ...
  bagpipes:
    ...
    cors:
      name:                 cors-patch    # <==== ssswell....
      origin: 
        - !!js/regexp /^https?:\/\/[0-9a-z-.]+\.mydomain.com(:\d*)?$/i

[theganyo]

Ugh. Yeah, I'd strongly recommend not putting "node_modules" in the fittingsDirs because of the potential name conflicts.

[osher]

Oh... :disappointed: ...but I need fitting to be modular and reusable...

What would you recommend?

[osher]

I'll give you more information.

My task is to provide a repository of modular and reusable infrastructure parts to implement a big set of micro-services. On the surface, out of the premise - Pipes and Fittings look great for this purpose :smile:.

These infrasturcure parts may be:

• session-pool managers for different dbs
• dal/model for different data-sources
• brokers for data-crunchers
• queue msg producers and so on and so forth.

This, on top of the must-for-all basics:

• config-svr agent
• KPI counter agent
• loggers

I thought that each pipe may enrich the context with an initiated module / instance, resulting with a context empowered with all the tools necessary for the job that gets passed to the web controller as declared by the programmer, probably in some x-uses property found on the swagger.yaml. It practically gets to be a sort of DI :smile:

mm. yea. I know - now the context is not passed to the web controller because the swagger_router passes (req,res,next). For this I have two options: start with a pipe that mounts the ctx on the req in some name, or replace swagger_router with a hacked version that passes (ctx, next) instead. I'm not sure which of them is cleaner.

The microservices must be slim: These tools must be cherry-picked into each project, where an initial boiler-plate will provide the barebones basics.

I don't want to wrap every adapter in a node-machine. We already have our own solution for documentation and exploration. We aspire to work with simple node-modules.

...

I'll appreciate any input.

Osher

[osher]

You know what? Here's a rabbit hole I'd like your opinion about: Maybe we can explore it and get something good out of it. It will be fair if you say it's too soon for it, or too demanding for the schedule.

It's a rabbit hole because it rises few design discussions, but I believe it worth's exploring. Anyway:

Ideally, there will be an operation level in the spirit of x-pipe attribute that describes a ...pipe! Not a controller-method...

This pipe should be a pipe defined add-hock for the operation. If I have to switch all the time between swagger.yaml and default.yaml - we lose the elegance. So we may decide to allow be x-pipeon the path level that is **_extended_** with fittings from thex-pipeon the opertion/verb level. We can even considerx-pipeon the document level as a common start point to allx-pipe`s, but this has to be flexible and easy to play with, or we lose our audience.

So what does it give us? In this sense, as far as the web-context is concerned - it's made of 2 adaptation fittings: The first extracts parameters from web-context request, and the last translates result (from ctx.output?) into web-context response. All the fittings in the middle - can be indifferent to the context and decoupled from it - They don't care if the pipe is operated in a web context, a CLI context, a msg-queue consumer - they can just do their work, and pass results.

any thoughts?

[osher]

do I talk to much , or you really need time to conteplate all this?

[theganyo]

I haven't had much of a chance to think about this, but we're already pushing the boundaries of what Swagger (Open API) defines (it should basically just be the REST interface), so I'm not sure it's wise to dive even further down this programming language rabbit hole inside the Open API document.

Note that Pipes and Fittings are already decoupled from web context. It's only the connect_middleware that knows how to translate the simple in/out of the pipe from a request and into a response.

[osher]

that's not quite true. one big difference is:

the controller handlers are written for (req,res,next), and have no access to the ctx, so if I want to collect augmented tools on ctx to be used in the controller handlers - I have to have a pipe fitting with a line like ctx.request.ctx = ctx or write the fitting to specifically augment ctx.request

both alternative feel wrong... do you get to see what I mean?

if operation handlers would get to be written for (ctx, next) - then they get to be just like any other fitting :)

The ctx will have .request, and .response, but they should not be meant to use them. They will for example - use ctx.myCoolDal.get(..., cb), use ctx.myConf.get(..), use ctx.logger(..) - maybe even ctx.params, which will reflect ctx.request.swagger.params, and end with done(null, { ..body-data...} ) or maybe even collect all the data on ctx.result which done() can then use.

Now this will truly decouple them from the web-context :)

On Mon, Sep 19, 2016 at 11:39 PM, Scott Ganyo notifications@github.com wrote:

I haven't had much of a chance to think about this, but we're already pushing the boundaries of what Swagger (Open API) defines (it should basically just be the REST interface), so I'm not sure it's wise to dive even further down this programming language rabbit hole inside the Open API document.

Note that Pipes and Fittings are already decoupled from web context. It's only the connect_middleware that knows how to translate the simple in/out of the pipe from a request and into a response.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/theganyo/swagger-node-runner/issues/58#issuecomment-248118846, or mute the thread https://github.com/notifications/unsubscribe-auth/AAxBHWnloSdps1PwkjAQ1v6HAVAzPjTeks5qrvL1gaJpZM4J0gCx .

[theganyo]

I'm still trying to understand your use-case and I think I'm failing, so forgive me if I over-simplify (or get it completely wrong), but what I'm gathering is that you'd like to do something like this:

swagger-node-runner -> pipe -> [middleware] -> pipe -> swagger-node-runner

Where you'd be plugging in some existing controllers that uses the standard (req, res, next) function as [middleware]. But you also don't want to wrap your controllers in fittings.

Am I completely off-base? Sorry if so... Maybe it would help me to understand if we were discussing one simple, concrete example.

[osher]

mm. let me illustrate.

infra packates:

• dals (db, cfg, cache, whatever)
• loggers (debug, ELK events, distress signals, slack - you name it)
• model brokers
• format converters
• security authenticators / authorizators - whatever.

basically, all you need to keep your controllers slim, logical and free from implementation details And this is why I have to require fittings from node-modules

about contexts:

Decoupling user-code and infra pkgs mean that I can use them in any other context: msg-queue-consumer, TCP server - you name it. It looks like something that is more valuable for infra-pkg only, but for example - consider a design for scale, where POC implements logic in web context, and later replace web handler with msg-queue producer, and moves the data-crunching to msg-queue consumer. This is one case where the context is changed, and I would like the code not to change.

I really adore the idea of bagpipes, I actually started designing one for my own just before I discovered it inside swagger-node-runner, and realizing the potential of this - _my brain exploded!!_

I was so happy that there are other good people that think like me :smile:

[osher]

mmm. there are still open questions re. how decoupled should the execution context and the web context be. For now I think that as an implementation detail, it will be the same context. It will be simpler to start like that.

But I foresee a stage where we'll have to really decouple them - maybe because of name collisions, maybe because of other reasons - but we'll have to be creating a private focused context for the execution. For this we'll have to decide how to indicate what tools that are initiated once in the router pipe = i.e. are put on the router web context, but should be passed to the execution context.

By when that time comes - I trust we'll have few projects running over this structure, and we'll figure the right balance from observing what we got.

[osher]

P.S. I just found that some people answer comments right from the mail, where I often use the comments like an editor: I often "upgrade" my comments - simply because github allows it, and I don't know if github informs participants when a message is edited. I'll try not to do it, but those are very rooted habits, started years back when skype started allowing editing sent messages... :(

Anyway - bottom line - final messages on the github web.

[osher]

drawing updated:

https://docs.google.com/drawings/d/1Gzqd1Ts9hCB0gO1_7iOvQjDzhXvn13_Bx26voSAWW7Q/edit

[osher]

im considering using my own router fitting - a lightly hacked version of the router of the runner - and avoiding the mehhhh... on the bottom left

[theganyo]

Fantastic! A picture is worth ~1k words! Thanks!

I (finally) think I understand where you're going. So to kind of summarize back into prose: Based on your desired state, you'd prefer a direct mapping to pipes you've built for the security and router fittings. Correct?

Let me think on this a bit...

[osher]

yes - this summary is much more on target :) However - while you think of it - don't miss the following stresses

but lets start with the obvious: I saw the x-swagger-pipe directive, which you use to cascade the swagger_controller pipe for serving the swagger document. The problem with using it for custom controller pipes are:

• it supports a pipe per path - it does not work for pipe per operation
• even if I define a pipe per operation - there will be a huge part of repetition between them: operations in the same microservice usually have a lot in common
• it will keep you going back and forth all the time between default.yaml and swagger.yaml if we already have to use default.yaml, let it be the p

So.

Having that said - we can distinct the following following levels:

• base fittings sequence for the entire micro-service
• added fittings per path - common to all operations in the path (verbs)
• added fittings per operation - specific pipes required for this operation only.

In this stage I did not find a use-case for post-operation. post-path, post-all fitting, but I'm sure that if the need comes - we'll find how to feature it in an elegant way :wink: (well, that's not true, there's the request logger, and the response-time reporter - however I'm currently doing it streight over restify).

.

I'm happy you want to give it a thought. I'll be happy to be part of the solution - we could work on it together in a branch or something - it will be a delight

However - I noted the time differences between us. I'll be going for a holiday for 10 days, however, I'm traveling north, so the time-differences will stay as they are... I hope that my lowered availability will not break the dialog...

Cheers!!

[theganyo]

Great!

First off, though, I want to make sure we set a baseline:

1. I'm not sure what you just said is accurate: I'm pretty sure that x-swagger-pipe does already work at the operation level. (If not, that sounds like a bug.)
2. As I'm sure you are aware, pipes can be composed. Again, maybe not exactly what you want, but using this could certainly avoid a lot of duplication.

Assuming those two statements are correct, it seems like the scope of what we'd need to address for your use case could be reduced quite a bit.

That said, I think it might make sense to break the discussion apart into two general areas of potential improvement (or at least we should consider these as related, but not interdependent items):

1. Ease of pipe declaration (possibly in swagger.yaml) and composition (avoiding duplication)
2. Enhance runtime to allow direct pipe access from security and router fittings

Am I still missing something here?

[osher]

I think we're quite on target!

I tried x-swagger-pipe for an operation level - I did not get what I was expected. I did it for fun - and did not hinder on it because of that. I'll double-check myself, and if reproduced - will create a bug for the matter.

Next - Ah - pipe composition is something I did not consider. This opens some possibilities. Having a pipe per operation should reduce the back and forth between default.yaml and swagger.yaml but will not eliminate it completely. Assuming they are all composed with a reusable baseline - it does make things very bearable. We can even add a feature-flag to allow deduction of custom pipe name by some straight-forward naming convention - this puts us almost there in this front, and lowering priority on the requirement to execute prior to the controller fitting named on the swagger.yaml on the path and operation level.

About pt. 2 - I propose to support both functionalities side by side as passing to the controllers (ctx, done), or (ctx.request, ctx.response, next) at the user's choice. It can be an alternative router module, or a fitting configuration flag.

I think I can put up a PR with the configuration flag option, based on something in the spirit of: in default.yaml:

openapi_error: 
   name:                 json_error_handler
   handle500Errors:      true

openapi_cors:
   name:                 cors-patch  #just requires 'swagger-node-runner/fittings/cors'

openapi_validate:
   name:                 swagger_validator
   validateResponse:     false      

openapi_router:
   name: swagger_router
   controllersDirs:      [ lib/web/controllers ]
   controllersType:      middleware # passes (req, res, next) - the default behavior
   #  OR
   controllersType:      pipe  # passes (ctx, done)
                               # done = the layer that expects 
                               # ctx.output and emits it to ctx.response

swagger_endpoint:
   - onError:            openapi_error
   - openapi_cors
   - swagger_params_parser
   - swagger_security
   - openapi_validate
   - express_compatibility
   - openapi_router

thoughts?

[theganyo]

Hey! Sorry for the delay.

So, glad the composition sounds like it may work for you.

So... on part two... I wonder if this could happen automagically using, say, Function.length so we wouldn't have to add configuration for it?

[osher]

I'm still thinking about it, so lets express some thoughts in write, and consider if all of them is a desired effect, and see if it all makes sense.

The biggest implication I can point in this stage is: Using the .length of a function means that you could in fact have pipes written as a middleware for (ctx.req, ctx.res, next) side by side with pipes written for (ctx, done) - where in my original proposition all of them must be of the same type.

I see greater values for this especially if we open the possibilities to use pipes from the node_modules - i.e. - use as a pipe a package that exports a middleware function by naming that package name as a fitting in a compound pipe (and on the same breath we get for free support for packages that exports a mw-factory that accepts a sinlge config object 😄, if you see what I mean here). Currently what stops us is https://github.com/theganyo/swagger-node-runner/issues/58.

But. This freedom comes in trade for adding a not-so-obvious gotcha to an already under-communicated project. This gotcha feels like a detail that will be missed in the docs/readme by many a developer - a hurdle that can be mitigated only to a limit.

It will also mean that there will not ever be any other future controller-type that accepts 3 arguments that is not a middleware, and no other future controller-type that will accept 2 arguments that is not made for (ctx, done) (or we will have to introduce configs over this gotcha). However, since it's by definition a utilization for a web project - I suppose that we should be able to name this as a base assumption.

It also means that the returned functions will have to explicitly name their args to ensure they answer the .length test, no tricks allowed. Despite not being too happy about it I tend to accept this limitation too.

If we're on the same page - I think we have a design 😉

[theganyo]

Yup. I totally, agree. There are potential issues, but...

1. It's not really fair to limit design decisions because of lame documentation.
2. It's very unlikely that there will ever be another type of controller we can't adapt to - and as you said, even if we do, we could add config if/when that happens.

It's the "no tricks" thing is the one that gives me a bit of heartburn - it's hard to put restrictions on things you can't control (and the node_modules issue is actually a good example of this). But, it does feel like the flexibility gained by this could be worth it.

[osher]

Roger. I would have started right away, but I have to catch up a gap with my professional tasks 😢

[theganyo]

So say we all.

[osher]

I'll have a window for this, hopefully in a matter of days.

A colleague architect pointed my attention that many developers write just function(req,res) whenever they don't intend to use the next - and this gives more weight to the discussed gotcha, because their implementations would yield Function#length will be 2, mistakenly taken for (ctx, next).

I suggest to support the following:

• if config is found (see bellow)
  • use controller-type expressed in config
• else - config is not found
  • use .length to distinct between (req,res,next) to (ctx,next)

config search stops on first find according to the following fallback chain:

1. sway.Operation
2. sway.Path
3. sway.SwaggerApi
4. not found

Thoughts?

[theganyo]

Oh curse those lazy developers! ;)

So... I'm going to reverse myself, and say that we should rely on the config annotations instead of auto-detect because it could be a breaking-change for some folks. (It's what I should've said in the first place anyway.)

That said.. if you'd still like that mix-and-match auto-detect feature, we could have that if we simply add an "auto" config option. So we'd have: "middleware", "pipe", and "auto"... currently, of course, it's always "middleware" and we continue to default to that so as not to break anything. "pipe" sets it to pipe, natch. And "auto" sets it to use its best judgement with .length... and it's up to the developer to ensure that things don't break. Do you think this is worthwhile?

The config search chain looks good.

[osher]

I feel much safer with your new decision. That makes me happy 😄

[osher]

Oups. Forgot something. fittingDef.

So, update to config search. Still stops on first find. But following fallback chain is:

1. operation.definition['x-controller-interface']
2. operation.pathObject.definition['x-controller-interface']
3. operation.pathObject.api.definition['x-controller-interface']
4. fittingDef.controllersInterface,
5. not found - defaults to 'middeware'

How would you suggest to handle illegal values?

We can validate point 4 during start-up. That's very easy. But I don't see an easy reference to the sway.SwaggerApi to preamptively validate all the rest on start up. Maybe I'm not looking in the right place?

If there is an easy reference to the sway.SwaggerApi object that you can point me to use - then great. I think I'll prefer to do it on start-up, and eliminate errors already on dev time.

Otherwise - I can offer the following, relaying on runtime behavior:

1. emit warning to the log - only the first time it is met for this concrete level in the current execution (by deleting the attribute from the relevant .definition object)
2. ignore it and carry on to next fallback level

A more harsh approach is emit an error and passing an error to the callback.

What is it going to be?

[osher]

mmm. Found it

bagpipes.config.swaggerNodeRunner.api It will be on load time :)

In this case I can optimize and load each operation with it's final value instead of searching the value for every operation execution

[theganyo]

I'm not sure where you are working, but in most places in swagger-node-runners's index.js, it's just self.api.

[osher]

The PR is not ready, but you can have an early look. Feedback is always appreciated :)

[osher]

Ok. the PR is on a good state, although I'd rather do 2 more things there. please have a look.