whitlockjc / sway-connect

Connect compatible Swagger integration middleware using the sway API.
MIT License
12 stars 4 forks source link

Design the metadata middleware #2

Open whitlockjc opened 8 years ago

whitlockjc commented 8 years ago

The swagger-metadata middleware in swagger-tools was the entry point to Swagger integrations using swagger-tools. The reason it is its own middleware is because there is a lot going implemented in the middleware and it would allow server authors to not reinvent the wheel but still get the basic Swagger integrations. All other swagger-tools middleware was written assuming that swagger-metadata was used previously in the middleware chain.

With Sway providing an actual API, some of which are already req aware, this middleware could become less useful because we no longer have to extra work to support Swagger 1.2, to do parameter processing, etc. In fact, the first prototype of a Sway-based swagger-metadata is currently in the repository. (I renamed "swagger-metadata" to "matcher", as in "middleware to match Sway requests".) As you can see, there isn't nearly as much to it as there was in swagger-tools. In fact, apart from the logging it could become a helper method...if said helper method could be written to avoid multiple separate middlewares running the same function.

All of this being said, I would love some feedback on what you think about this middleware. If it stands as-is, it will likely be like swagger-metadata in swagger-tools where other Sway-based middlewares depend on it. I also have some ideas for configuring this middleware:

The reason these would be here and not in the potential router and/or validator middlewares is they could short circuit the request middleware chain based on information already available.

peterbsmyth commented 8 years ago

My first thought about swagger-metadata is that it took me a tremendous amount of time to realize that for a request to be to given the .swagger the corresponding routes must also be included in the initializeMiddleware function.

I was doing routing with express when I had this issue. I eventually saw this line in the documentation:

// Initialize the remaining server components

and that's all i can see in the documentation that would lead me to conclude that app routes must be in the initializeMiddleware function.

In the updated documentation for sway-metadata it would be great to see clearer documentation on how to use it with various routers, like Express...

whitlockjc commented 8 years ago

I don't understand the first issue you brought up. The way the swagger-tools/sway middlewares work for Connect/Express is that they listen to all requests (this is configurable of course using mount points) and will only do work for requests matching paths described in your Swagger document. So if you want to have these middlewares do anything, you'll likely have them registered before your manually registered route handlers. I definitely think we can describe things better.

peterbsmyth commented 8 years ago

This attaches the req.swagger:

swagger(swaggerDoc, function(middleware) {
    app.use(middleware.swaggerMetadata());

    // Load routing files
    require('../server/routes/auth.server.routes')(app);
    require('../server/routes/books.server.routes')(app);
  });

This doesn't attach the req.swagger:

swagger(swaggerDoc, function(middleware) {
    app.use(middleware.swaggerMetadata());

  });
  // Load routing files
  require('../server/routes/auth.server.routes')(app);
  require('../server/routes/books.server.routes')(app);

And it took me a bit to figure that.

whitlockjc commented 8 years ago

Yes. That's the nature of async. In the first version, you're wiring your route handler after the swagger-tools middleware gets wired up. In the second version, you're wiring your route handlers before the swagger-tools middleware gets wired up. We won't have this issue with sway-connect.