krakenjs / swaggerize-express

Design-driven apis with swagger 2.0 and express.
Other
355 stars 81 forks source link

Allow multiple Schemas / API versions using a router in place of app #49

Closed duncanhall closed 9 years ago

duncanhall commented 9 years ago

Currently, the module can only be applied to a top level Express app object.

This prevents the option of having multiple instances of swaggerize-express running in the same Express instance. Versioning of APIs is therefore not currently possible in the same application.

For example, I might have 2 schemas, describing different versions of the same API:

swagger-v1.json swagger-v2.json

Each schema describes it's own operations / vaildations, but wants to be run from the same application, for example exposing /api/v1/ and /api/v2/ as swagger base paths.

Additionally, applying the module at the app level makes it difficult to apply conditional middleware for different paths.

For example, I might want to pass all /api/ routes through an aunthentication() middleware, but allow calls to /something/public/ to bypass it. Because swaggerize-express gets applied at an app level, if I want to call some middleware before hitting the Swagger routes, I have to declare that middleware at the app level also:

app.use(customMiddleware());
app.use(swaggerize({/* options */}));

//Everything here now has to pass through customMiddleware();

router.use('/something/public/', customHandler);

Both of these use-cases can be handled by allowing swaggerize-express to be applied as a more generic router-level middleware.

I'm tempted to create a separate project, based on swaggerize-builder that would work in this way, but if this makes sense and seems useful, I'd be happy to do this work under a fork of this project. It would almost certainly not be be backwards compatible though.

duncanhall commented 9 years ago

Having looked further into this (it's a mandatory requirement for me), it could be made to fit in the existing module with only a small change to the public API.

To allow for backwards compatibility, the module index.js file could export 2 methods:

var swaggerize = require('swaggerize-express');

app.use(swaggerize.app(/* options */));

router.use('/path', swaggerize.route(/* options */));

The first method, swaggerize.app() would behave in exactly the same way as the current method.

The second method, swaggerize.route() would return a router object and allow for the behaviour described above.

It would be good to get feedback on whether this is desired in the current project. I'd like to avoid duplicating effort across projects if possible.

duncanhall commented 9 years ago

I've started a branch for this feature, which can be tracked here: https://github.com/duncanhall/swaggerize-express/compare/feature/swaggerize-router

The work so far aims to break up the existing functionality into smaller, reusable modules which can be used for both swaggerize.app and swaggerize.route, while keeping functionality and API the same as that on the master branch.

duncanhall commented 9 years ago

A very simple option could actually just be to keep a single method but have it call expressroutes with an express.Router() object, and then return that router.

What exactly is the need / purpose of setting the api and setHost properties on the app?

tlivings commented 9 years ago

Those are convenience methods for updating the host/port info in the document for swagger-ui.

tlivings commented 9 years ago

So what's stopping you from creating two express sub apps yourself, using swaggerize on each of those, and adding them to a parent app?

duncanhall commented 9 years ago

I'm not sure I follow exactly? Isn't the conventional method of composing an express application in 'sub apps' done via use of routers?

The expressroutes() method already expects a router, it's just than index.js explicitly decides which router that should be, and then doesn't give it back to you.

Other than some re-shuffling of things to enable testing, all I'm really suggesting is handing an express.Router() to expressroutes(), and then returning it to the user to do with what they please:

https://github.com/duncanhall/swaggerize-express/blob/a20751e6f8e62e75a703f3a5fbab4bb22d0d2573/lib/index.js#L18-L23

tlivings commented 9 years ago

Afaik it can be done either way. For example: http://expressjs.com/api.html#app.mountpath

I'll take a look at your changes as well.

tlivings commented 9 years ago

Closing due to inactivity.