[Discussion] Introduce Guards/Pipes/Middleware as a chain before the controller endpoint

[adrien2p]

Hello there, here is the new subject I wanted to write up.

Why now

I have been able to do quite some reviews and walk along the code base for a good time now.
What I have seen so far, is that when create a new endpoint, everything that is related to

• request manipulation/transformation
• security check on entity ownership
• entity fetching

Is done by the endpoint itself.

How it can solve the problem

From my point of view, moving the points I cited above to an higher level would allow to decoupled those step from the endpoint itself which will result in the following

• endpoint become simpler and act more as a passthrough to the business logic layer
• avoid forgetting to check that an entity belongs to a parent entity based on the endpoint target
• simplify security check
• and centralise validation/transformation of the input data in some cases

Story

From my background with nestjs I've seen how that approach can be powerful when well used and it can solve a lot of problem about code decoupled as well as centralisation of common tasks across the all app endpoints.
That approach is not new and already exists, so there is only the needs to implement it if that makes enough sense

and that's why we would like your feedback and opinions on that matter

Actual schema

Proposal schema

Examples

Let see some example on how that can be translated into code
for the following example lets take the following endpoint (imaginary exndpoint)

GET /admin/files/:file_id

export async function fetchRequestedFile(req, res, next) {
  const fileId = req.params.file_id
  const fileService = req.scope.resolve('fileService')
  const file = await fileService.retrieve(fileId)
  if (!file) {
    throw new MedusaError(MedusaError.Types.NOT_FOUND, `file with id ${fileId} not found`)
  }
  req.file = file
  next()
}

Now let see a guard to check the security on the file

export async function canAccessFile(req, res, next) {
  const file = req.file
  const loggedInUser = req.user
  const canAccess = await file.canAccess(user)
  if (!canAccess) {
    throw new MedusaError(MedusaError.Types.NOT_ALLOWED, `Unauthorized to access the file with the id ${file.id}`)
  }
  next()
}

If we wanted now to create a file on POST /admin/files

let see some pipes to manipulate the data and query

export function transformBodyTo(targetClass, targetPropOnReq) {
  return async (req, res, next) => {
    const rawData = req.body
    const validated = await validator(targetClass, req.body)
    req[targetPropOnReq] = validated
    next()
  }
}

export function transformQueryTo<T>(targetClass, targetPropOnReq, defaultFields = [], defaultRelations = []) {
  return async (req, res, next) => {
    const { expand, fields, limit, offset } = await validator(
      targetClass,
      req.query
    )

    const queryConfig = getRetrieveConfig<T>(
      defaultFields,
      defaultRelations,
      fields?.split(",") as (keyof T)[],
      expand ? [...(expand ?? []).split(",")] : undefined
    )

    req[targetPropOnReq] = {
      ...queryConfig,
      skip: offset,
      take: limit,
    }
    next()
  }
}

In the end, we can compose a route as follow

route.get(path, fetchRequestedFile, canAccessFile, handler)

All the common middleware that are applied on the same sub routes could be applied on a parent /admin/files/:file_id and then all the routes under that path would work as usual and they would already be prepared and validated. in the handler we would already have access to the entity and now that it is validated and sure. also, for path where there is some inter dependance that must be checked, it could be done the same way and in the end in the handler we would know that the relations between inter dependent entity would have been checked already and that we are in a valid context once we are in the handler.

for the post routes we would have something like

route.post(path, transformBodyTo(PlainToClass, "file"), transformQueryTo(plainToClass, "queryConfig"), handler)

Conclusion

Hope that it is comprehensible enough, if there is any question or feedback do not hesitate to post it here :)

Example

https://github.com/medusajs/medusa/pull/1396/files

[octalpixel]

This makes sense! This enables to stick to the SoC principle in terms of development and will keep the core a bit more easier to read and extend.

Got one question though, Is this going be optional for anyone extending medusa or developing a plugin ? Since it's a matter of chaining express middleware it alright for it to be an optional way of developing yeah ? Raising this concern, because Medusa is getting a lot traction, and new developers might find it too overwhelming catch everthing.

[adrien2p]

Indeed, when a user create plugins or custom handler. It is his choice to use the provided middlewares or not or even create new one or use another approach. It really depends on the needs.

Basically that approach will mostly serve the core but can serve the community if needed. Wdyt?

[juanzgc]

This helps decouple the services and the routes in general and seems like a great step forward! Allows for greater code reusability as well as readability.

[dwene]

I think it's a great idea! I love all the Nestjs patterns

[adrien2p]

Thanks. And in the end it can be handled by the decorator as well when i will write up on that subject too 🤓