search

apigee-127 / swagger-node-runner

The heart of Swagger-Node
MIT License
102 stars 124 forks source link

resolving controller-interface type #65

Closed osher closed 8 years ago

osher commented 8 years ago

Following the discussion in: https://github.com/theganyo/swagger-node-runner/issues/58

This PR is NOT READY YET, I just wanted you to have an early look and get feedback from you and from the builder.

The code implementation is more or less there However - I cannot guarantee it before I add tests for all the cases we discussed.

I'll get to it in a few days.

osher commented 8 years ago

Peeweee. Haven't seen stack traces this long ever since the darker days of java... 😛 so ok, I broke the tests. need to run mocha with --recursive... I added mocha.opts file

osher commented 8 years ago

baaah. more missing opts. ok.

osher commented 8 years ago

no. not missing opts. just a non-standard behavior. I think i'got'cha now. You don't want me to use mocha, you require me to use npm test The way it's written does not work on windows. But I'll give you what you want.

moment

coveralls commented 8 years ago

Coverage Status

Coverage decreased (-0.3%) to 96.104% when pulling f99c496c13a5ab1a375d29e130df839252b86eba on osher:patch-1 into a44711b21e04d0a989d58392ce076bd6cf9f96ca on theganyo:master.

osher commented 8 years ago

Ok. Build passes

BUT

The PR is not ready, I still have to add more tests.

I just would appreciate if you take a look and say a word

coveralls commented 8 years ago

Coverage Status

Coverage increased (+0.07%) to 96.434% when pulling 94e3ef636e86815699982c8874f12185c4124bac on osher:patch-1 into a44711b21e04d0a989d58392ce076bd6cf9f96ca on theganyo:master.

osher commented 8 years ago

Well, I'll be damned. I could not help myself...

I think we're there. I would like to add one more thing: ~/fittings/swagger_cors

module.exports = require('./cors')

Can I please do it here?

this should allow me to config/default.yaml

swagger:
  fittingsDirs:             [ node_modules, lib/fittings ]
theganyo commented 8 years ago

Nice! I haven't had time to completely review, but this is looking super!

Re: cors... it's unrelated, should probably be a separate PR...

osher commented 8 years ago

Then I suppose the context.output structure is also a different issue. Will be glad to hear what you think though.

Anyway - I admire your response times. I'm off to the weekend. Weekends here are Fri/Sat so I'll be back on Sunday. [edited]: Do you think you'll have time for it by then?

theganyo commented 8 years ago

Actually, on second thought, I'm ok with addressing them all in the same PR. As long as we can ensure all are backward compatible, I don't anticipate the need to accept piecemeal.

theganyo commented 8 years ago

Oh, and enjoy your weekend! Yes, I'll try to review before then.

coveralls commented 8 years ago

Coverage Status

Coverage increased (+0.08%) to 96.44% when pulling 6ec101fd20f34e455e8d8d2b5080aba074870a90 on osher:patch-1 into a44711b21e04d0a989d58392ce076bd6cf9f96ca on theganyo:master.

coveralls commented 8 years ago

Coverage Status

Coverage increased (+0.08%) to 96.44% when pulling 6ec101fd20f34e455e8d8d2b5080aba074870a90 on osher:patch-1 into a44711b21e04d0a989d58392ce076bd6cf9f96ca on theganyo:master.

coveralls commented 8 years ago

Coverage Status

Coverage increased (+0.08%) to 96.44% when pulling 6ec101fd20f34e455e8d8d2b5080aba074870a90 on osher:patch-1 into a44711b21e04d0a989d58392ce076bd6cf9f96ca on theganyo:master.

osher commented 8 years ago

ok - fittings/swagger_cors - is done.

I wanted to update the docs, but I saw the README is rather empty... How do you intend to manage docs? Perhaps I should create a wiki page about x-controller-interface?

theganyo commented 8 years ago

Yes, docs are horrible right now. This project was relying on the swagger project to maintain all the documentation, but that's no longer possible. So, yes, we should start a wiki. Many doc updates have been made only in the release notes until now, but it's not an acceptable way to continue.

theganyo commented 8 years ago

Oops. Forgot to finalize this over the weekend. Thanks for all your work on this!

osher commented 7 years ago

Scott - this is wonderful. When should I expect it to be published on NPM?

theganyo commented 7 years ago

Did you happen to go ahead and write up any release notes?

osher commented 7 years ago

Where should I provide release notes? I thought its some github feature only owners can...

Anyway - for my changes the release note may be in the spirit of:

This PR allows you to chose how you want to implement your controllers. The default is like a middleware: operationMethod(req, res, next)

Alternatively, you can chose to write your controllers as a pipe: operationMethod(ctx, next) In this case, the controller is passed with the pipeworks context, which have a reference to .request, to .response as seen in lib/connect_middleware, line 55. The idea in this case is not to use ctx.response, but leave your response body on the ctx.output, the status on ctx.statusCode, and the headers on ctx.headers. The benefit is improved isolation from web-context. You don't have to run a web server to run your tests. You just pass it a ctx and see what you find on the ctx once the callback is called.

Lastly, you can ask bagpipes to deduct the interface from the controller's method signature. If it names 2 parameters - it's a pipe, if it names 3 parameters - it's a middleware.

This directive may be provided in few levels that cascade each other.

  1. The default for the entire server - provided at config/default.yaml as swagger.bagpipes._router.controllersInterface.
  2. If its more comfortable to you to express it in the swagger - you may use in your swagger.yaml the following directive on the root: x-controller-interface.
  3. You can cascade the default for an entire path, suggesting that all methods of the controller that manages this path are implemented this way, using x-controller-interface on the path level.
  4. lastly - you can cascade the directive per operation, using x-controller-interface on the operation level. The legal values are one of the values middleware, pipe, auto-detect, correspond to the behavior described above.

.

BTW - I think there's yet a small miss which I will be happy to fix before 0.7.1 We did not accomplished isolation from the web. Even if the user does not need to use ctx.response - he still needs ctx.request to access the ctx.request.swagger.params. I think we should feature ctx.params Edit - added line : I think it probably should be done in fittings/swagger_params_parser

.

Oh, I also see you did not reference your release notes for v0.7.0: 😮

readme.md ends with

https://github.com/theganyo/swagger-node-runner/releases/tag/v0.6.4
https://github.com/theganyo/swagger-node-runner/releases/tag/v0.6.10
https://github.com/theganyo/swagger-node-runner/releases/tag/v0.6.11