Middy: What's the rational behind reversing the after middleware order?

[ossareh]

Hi,

Q: Why do you push before and error middlewares but unshift after middlewares on to their stacks?

Details

In the docs you give the example of middleware being like an onion, where the order of middlewares is:

1. middleware1
2. middleware2
3. middleware3
4. handler
5. middleware3
6. middleware2
7. middleware 1

I have a case where the middlewares depend on the response of the prior middleware.

Specifically I want to reuse your work on the cors() middleware to set various headers, but then I want to overwrite the "Access-Control-Allow-Origin" header with the value of the Origin header if it is present. So in my case middleware 2 depends on 1. Additionally, the AMP specification requires additional headers to be set, as well as a few pre-flight checks.

I've a test case which set up middlewares like this:

.use(cors())
.use(corsByOrigin({
    domains: ["cdn.amp"],
}))
.use(ampCors({
    publisherDomains: ["localhost:3000"],
    validAmpCDNDomains: ["cdn.amp"],
}));

and results in a handler object like this:

        __middlewares:
         { before: [ [Function: bound preflightCheck] ],
           after:
            [ [Function: ampCorsHeader],
              [Function: bound varyCorsByOrigin],
              [Function: bound addCorsHeaders] ],
           onError:
            [ [Function: bound addCorsHeaders],
              [Function: bound varyCorsByOrigin] ] } }

Which is precisely the reverse of what I want, even though the code ordering is exactly the order that I want.

I could change the order that I attach the middlewares but then the order of the after is different than the order of the onError (which are pushed onto their stack, not unshifted like the after ones) - so any relationship between the error cases is lost.

Additionally the API for specifically configuring before and after cannot be used correctly in typescript as there's a mismatch between the javascript and the typescript.

Here you define handler.[before,after,onError] as being a function with two args, instead you want to convey it takes a function, which takes two args.

So in javascript terms it's the difference between:

# the actual javascript
instance.before = (beforeMiddleware) => {
}

and

# what typescript thinks is happening
instance.before = (handler, nextfn) => {

}

I can apply a fix to this, which would allow me to solve my problem with code like:

const ampCorsMiddleware = ampCors(...);
const varyCorsByOrigin = varyByOrigin(...);
const corsMiddleware = cors();

handler
  .before(ampCorsMiddleware.before)
  .after(ampCorsMiddleware.after)
  .after(varyCorsByOrigin.after)
  .after(corsMiddleware.after)
  .onError(corsMiddleware.onError)
  .onError(varyCorsByOrigin.onError);

Which isn't mentally the correct order, but it results in the correct order of execution that I'm after.

So, my main question is why the onion flow?

Other potential solutions

Do not rely on the output of a previous middleware

Pros:

• your destiny is completely in your own hands

Cons:

• lots of code duplications; i.e I have to re-write your cors handler

Wrap middlewares around one another

Pros:

• gets me the precise ordering I want

Cons:

• I can't think of a really strong one.

Change unshift to push

Pros:

• Orders the middlewares as they're in the code / how you logically think of them

Cons:

• Breaking logical-API change.

Example of the output:

        __middlewares:
         { before: [ [Function: bound preflightCheck] ],
           after:
            [ [Function: bound addCorsHeaders],
              [Function: bound varyCorsByOrigin],
              [Function: ampCorsHeader] ],
           onError:
            [ [Function: bound addCorsHeaders],
              [Function: bound varyCorsByOrigin] ] } }

[ossareh]

Hey there, in my middy for I've made a set of changes in line with the thoughts above. This primarily consists of me fixing the index.d.ts file that was incorrectly expecting handler.(before|after|onError) to be a function, instead of a function which takes a callback.

https://github.com/middyjs/middy/compare/master...ossareh:fix_types

There are a couple of other changes to the build, and also a re-packaging of package-lock.json as it seemed to be brining in a broken version of istanbul.

Before submitting a pull request I'm interesting on hearing whether you're OK with:

• changing the node version for tests
• moving the @types definitions to devDependencies
• modernizing the tsconfig

Here's the build of this branch: https://circleci.com/gh/ossareh/middy/6

[lmammino]

Hello @ossareh, sorry for the very late reply (busy busy week!) and thanks for sparking this interesting conversation.

I am going to try to address all your points/questions.

Why the onion flow?

Honestly, this is not easy to answer without saying "This is what many other middleware-based web framework do". I reckon this is not a valid answer, so let me expand a bit on what are the main advantages of this pattern:

• Allows to keep specific pieces of business logic isolated, yet allowing composability
• Middlewares are generally easy to test
• Middlewares are generally high reusable
• Don't need to extend classes or register events to allow code extensibility

Now the middy implementation of the onion flow is slightly different from what you would see in other middleware frameworks like Express or Hapi.

In those frameworks, the implementation resembles more closely the decorator pattern, where a main unit of logic (app or route) is wrapped by different functions and the before, after and error sub-flows are not explicitly defined.

Let's have a look at the following example

const app = createApp(() => ({ message: 'Hello World' }))
app
  .use(middleware1)
  .use(middleware2)
  .use(middleware3)

With the decorator-like implementation this code is more or less equivalent to writing this (assuming a syncronous flow for the sake of simplicity):

const request = {}
const response = {}
(function app(request, response) {
  (function middleware1(request, response) {
    // do stuff before middleware 2 (edit request)
    (function middleware2(request, response){
      // do stuff before middleware 3 (edit request)
        (function middleware3(request, response){
          // do stuff before main app logic (edit request)
          (function mainAppLogic(request, response){
            response.message = 'hello world'
          })(request, response)
          // do stuff after main app logic (edit response)
        })(request, response)
      // do stuff after middleware 3 (edit response)
    })(request, response)
    // do stuff after middleware 2 (edit response)
  })(request, response)
})(request, response)

// response is now { message: 'hello world' }

Which means that, with this approach, a realisitic middleware code will look like this (also introducing the next callback for dealing with asynchronicity):

function middleware(request, response, next) {
  // do stuff with request, response shouldn't have been touched yet
  next() // invokes the next middleware (and indirectly the full chain of middlewares)
  // response should now be available and you can change it if needed
}

The approach we decided to take with middy is to make very clear what are the different execution phases (before, after and error) so that writing middlewares and understanding what's going on should be simpler compared with the decorators-like approach. Also we believe the code of the middleware runtime results much simpler to read, understand and to work with.

I am not sure this dissertation really answer to your question, but I hope it gives a more detailed context on the internals (which might be useful in the next part of my response).

Many competing middlewares

Let's move to your specific use case, which I believe it's very interesting and potentially very common.

In your use case you have three competing middlewares, where by competing I mean that they are trying to address a common responsability, specifically adding/manipulating CORS headers.

I haven't tried to deal with similar use cases yet, but it seems to me that the middleware composability model as you described in your implementation doesn't fit well this specific use case and that it would probably make more sense to have a single bespoke middleware that deals with the entire business logic around your CORS headers.

Rewriting all of it from scratch means you lose the advantage of being able to re-use the logic in the current implementation of the CORS middleware, which is not ideal (and against the DRY principal).

On a first though I believe a solution here might be to implement (in middy or as an external helper utility) some function that allows to compose/wrap middlewares and returns a new middleware that can be attached to a middy instance as a single middleware. I believe this is what you are suggesting in the potential solution titled "Wrap middlewares around one another". I haven't got much time to experiment with this yet, but it seems the most favorable (less invasive) solution at the moment.

Your PR and typescript issues

Regarding our TypeScript definitions, I am very happy to keep supporting them as many users seems to love it, but unfortunately I don't have much experience with TypeScript and I am not always sure the implementation is correct.

I think what you are saying is correct and that we might need some update. What seems wrong to me is this part of the definition:

  interface IMiddy extends Handler {
    use: IMiddyUseFunction;
    before: IMiddyMiddlewareFunction; // <--- should instead be a functiont that receives an IMiddyMiddlewareFunction as argument
    after: IMiddyMiddlewareFunction; // <--- should instead be a functiont that receives an IMiddyMiddlewareFunction as argument
    onError: IMiddyMiddlewareFunction; // <--- should instead be a functiont that receives an IMiddyMiddlewareFunction as argument
  }

Feel free to submit a PR directly to middy if you think you can provide improvements in this space, but please argument it with some kind of tests so that it would be easy to review even for people with small TypeScript knowledge.

I believe that's all.
Sorry for the lengthy answer and please let me know if something is missing.

[ossareh]

Hey there @lmammino, thanks for your detailed response. Also, absolutely no stress about the time it took to respond, this is an OSS project and I expect you are all volunteers, so I have no expectation around quick responses. I can always fork and do my own thing ;)

I am not sure this dissertation really answer to your question, but I hope it gives a more detailed context on the internals (which might be useful in the next part of my response).

... and ...

Sorry for the lengthy answer

It's a great answer! I now far better understand why you've chosen this pattern, I also I understand why you're explicit about the phases and their ordering. I'll go ahead and submit my type PR to fix the before/after/error methods and continue to use my current strategy.

related thoughts

When it comes to being the "best middleware for AWS Lambda" I think taking security seriously is really beneficial as the model for AWS Lambda very aggressively exposes a wide range of web security concerns; if you don't know web security it's really easy with AWS Lambda to create an insecure setup. So to "be the best" we need a really great set of CORS and CSP middlewares.

Thinking along those lines, and wanting to move more toward a functional approach I can see two approaches:

1. individuals can wrap the middleware
2. middy/cors can be improved.

The first option allows people to shoot themselves in the foot easily. The second option would require the cors middleware to change to support setting the cors header values based on the origin header. Additionally it emit a specific error type if it cannot do that. I think these should be the default, so if you want to create a very permissible Lambda endpoint you need to actively disable these options.

This change would mean the additional headers for AMP are really easy to implement with the onion layout.

I'm happy to continue to discuss using this issue. However I feel as though my question is answered so I'll go ahead and close this. As per #176 I'm happy to continue to discuss this if you guys have a formalized place that you gather to discuss such design thoughts 👍