middyjs / middy

🛵 The stylish Node.js middleware engine for AWS Lambda 🛵
https://middy.js.org
MIT License
3.72k stars 375 forks source link

Using immutable event and context changes in middleware #300

Closed dbartholomae closed 3 years ago

dbartholomae commented 5 years ago

I might be a little late here, but I recently stumbled upon a similar issue to #275: the jwt-auth middleware injects the decoded jwt into event.auth and ideally I would like to make sure that this is directly reflected in typing without additional burden on the user. Unfortunately this is hard to do since middy requires middleware to mutate event and context instead of returning new ones. I haven't completely thought it through, but since it could be a major change in direction I wanted to bring it up for discussion first: In my opinion it would make a lot of sense if middleware called next with optional event and context parameters that are then used for the following steps (and later in the handler) instead of mutating existing objects. This would allow higher code quality due to less mutability and would allow really neat typing where the middlewares could provide how event and context types change so for the user it "just works" to e. g. access event.auth or event.rawHeaders. This would open up really nice possibilities for the validation middleware as well. I think this might even be possible without a breaking change, since current middlewares should not be using next parameters anyways.

pocesar commented 5 years ago

yeah, this is a tricky one, because the shape of the event / context are the first thing you do, as in:

middy<Handler<Event, Result>>(async () => {})

you'd never be able to know, beforehand, how the event will look like after using a middleware with use(). for the TS perspective, all the mutations would have to be "extracted" from their inner workings and be presented to the user if he wants to "inject" properties on the event object, but it would fall short for the problem you mention: wouldn't be possible to create immutability (and thus expected object shape). one way would to behave more like promises and streams, so each subsequent "use" you'll know what comes in and what comes out, until you call something like collect() at the bottom (instead of passing the handler first hand)... that's one way to do it, IMHO

the whole thing would be downstream, like:

middy()
  .use(something())
  .use(somethingelse())
  .use(anotherone())
  .collect(async (data, event, context) => {
     // the result of this function depend on the middlewares used, like Promise<{ body: object, error: string | null  }>

     // event = original event from API gateway, for example
     // data = current stuff appended by middlewares, namespaced
    data.something.property
    data.somethingelse.property2
    data.anotherone.property
  })

so the "shape" of the data would actually be a concatenation of previous middleware, so something() & somethingelse() & anotherone(), although not many will change the "data" and will have to manipulate the event directly (which is fine)

dbartholomae commented 5 years ago

I've thought some more about this and I think the cleanest solution would be to use decorators:

class Controller {
  @UseMiddleware(httpEventNormalizer())
  public getSomething (event) {
    console.log(event.pathParameters.userId) // TypeScript will know that pathParameters is not undefined
  }
}

Unfortunately this currently won't work as the TypeScript team is waiting for decorators to proceed to Stage 3 before implementing typing there, and decorators will be discussed in the March ECMA meeting, but don't seem close to finished. I might start playing around with the concept, though, but this most likely will be a separate middleware library as it doesn't fit the middy architecture at all.

In the end it isn't actually that different from your proposal, though, only that it would use function chaining instead of method chaining. After thinking about it (and looking up other middleware engines) I also realized that Middy currently is the only middleware engine I know of that asks for the Controller first and for the middleware second.

A bit unsure how to proceed here, though, as indeed this would not be possible without turning away from the current structure.

pocesar commented 5 years ago

the function chaining makes sense, but since their order is irrelevant (because of it can have after, before or both), if two middlewares apply the same mutation to the event object, it would be confusing for people that didn't understand the inner workings of middy, but the convention of middy(middleware, [middleware, middlware], (handler) => {}) has been changed and people coming from express would be really confused if they never used expressApp.use()

eduardomourar commented 4 years ago

@dbartholomae, now that Typescript have added decorators, it would be nice to have your suggestion implemented within middy without changing its architecture. I have created a very simple solution for this here: https://github.com/eduardomourar/serverless-aws-typescript/blob/b0992321b6173e2acb27098e0853be2363df0bd4/src/util/cors.ts

dbartholomae commented 4 years ago

@eduardomourar Unless I missed something, there is only support for an outdated decorator proposal, which also doesn't include strict typing and therefore unfortunately wouldn't solve this problem :-( I will most likely set up a new middleware engine which is written in TypeScript and uses the functional style. I will try to add an adaptor which allows to use middy middleware, without the strict typing :)

dbartholomae commented 4 years ago

The new structure will basically look like this: https://www.npmjs.com/package/middytohof

Unfortunately I currently don't see a way to achieve type security and immutability without this structural change :(

lmammino commented 4 years ago

I don't have the necessary Typescript expertise to influence any conversation here, but I would just like to mention that now that we are close to ship v1 stable it makes sense to start considering what do we want to achieve with v2. I am definitely not against revisiting the shape of the framework and structural decision. I am more than happy to introduce breaking changes in v2 if they contribute to making the framework a lot more usable and friendly.

Any way you would recommand to proceed to start experimenting with this?

eduardomourar commented 4 years ago

@dbartholomae , you are completely right and I meant more from a maintainability perspective not for solving this particular problem. I do agree that a structural change would be required, but I would really wait for the ECMA proposal to be finalized (or at least Typescript implements features such as function decorators, etc).

@lmammino , my suggestion for v2 would be to have TypeScript support as a first-class citizen. There are those two libraries that try to achieve something very similar on top of Express: Ts.ED and OvernightJS.

dbartholomae commented 4 years ago

What I would suggest: I will set up a framework which will use a new structure that I think will allow better typing and immutability (see example below) and test it with the current company I am working with. If that's ok I will copy some of the existing middleware from middy and build it in the new structure. If it works out in production, we can think about how to potentially bring it together wirh middy in the future. The way I currently imagine the structure, there could just be adapters for using middleware from one structure with the other structure.

Middleware most likely will look like this

const errorHandlerMiddleware = (options) => (handler) => async (event, context) => {
  try {
    return await handler(event, context)
  } catch (err) {
    return {
      body: err.message,
      statusCode: 500
    }
  }
}

and will be used like this

export const handler = errorHandlerMiddleware()(
  async (event, context) => {
    if (event.body === '') {
      throw new Error('body cannot be empty')
    }
    return {
      body: 'Success!',
      statusCode: 200
    }
  }
)

and multiple middlewares could be chained with any compose or pipe function (and hopefully at some point in time with |>):

import { compose } from 'ramda'

export const handler = compose(
  errorHandlerMiddleware(),
  anotherMiddleware(withOptions),
  andYetAnotherMiddleware(withOtherOptions)
)(
  async (event, context) => {
    if (event.body === '') {
      throw new Error('body cannot be empty')
    }
    return {
      body: 'Success!',
      statusCode: 200
    }
  }
)

An adaptor for middy middleware would then look something like this:

const useMiddyMiddleware = (middyMiddleware) => (handler) => async (event, context) => {
  const instance = { context: { ...context }, event: { ...event }, response: null, error: null }
  await middyMiddleware.before(instance)
  try {
    instance.response = await handler(instance.event, context)
  } catch (error) {
    instance.error = error
    const newError = await middyMiddleware.onError(instance)
    if (newError) {
      throw newError
    }
  }
  return instance.response
}
pocesar commented 4 years ago

@dbartholomae I'm all in for functional approach, but JS programmers usually have a hard time dealing with functors, even if they use it all the time without realizing. they usually expect either passing everything as an object as one parameter or many parameters. also pipe and compose might give different results since it's reduce / reduceRight, unless the middlewares can actually be composable

dbartholomae commented 4 years ago

I have started setting up the middleware. You can follow along with the process in this repo: https://github.com/dbartholomae/lambda-middleware

dbartholomae commented 4 years ago

@lmammino After my focus had wandered a bit, I took this weekend to finally get the project out of the door. Here's the function-based middleware: https://github.com/dbartholomae/lambda-middleware/

I've also added an adaptor so it is possible to use middy middleware with this. The other way around unfortunately is not as easy as the functional middleware is a bit more powerful and can also cover cases that cannot be covered by middy, if I'm not mistaken.

The benefit of type-safety played out quite nicely. Best examples are the class-validator middleware which makes sure you only rely on validated input and jwt-auth which only gives you access to the auth property if the user is actually verified.

Would love to hear feedback around it!