meteran / gnext

Web Framework extension for Gin. Offers the API structuring, automates validation and generates documentation. It's fully compatible with the current Gin usages and Gin's middlewares.
https://pkg.go.dev/github.com/meteran/gnext
MIT License
69 stars 5 forks source link

What are the best ways to wrap handlers? #32

Open busf4ctor opened 1 year ago

busf4ctor commented 1 year ago

GNext is impressive but uses interface{} as the handler type. The good part is that we're flexible on how to write handlers. The bad part is how hard it is to write any function that wraps handlers in general.

The code below is an example of what I want to achieve:

router.GET("/endpoint", WithAuth(myHandler, roleAdmin, roleUser))
router.POST("/endpoint", WithAuth(myHandler, roleAdmin))

Writing the WithAuth wrapper is more challenging than I thought because there's no handler type. It would require several runtime checks and reflect uses, making the code very unclear.

Here's how I am doing it:

authMiddlewareAdmin := AuthMiddleware{}
authMiddlewareAdmin.AddAllowedRoles(roleAdmin)

adminOnlyGroup := router.Group("")
adminOnlyGroup.Use(gnext.Middleware{
    Before: authMiddlewareAdmin.Before,
}

adminOnlyGroup.POST("/endpoint", myHandler)

authMiddlewareAdminUser := AuthMiddleware{}
authMiddlewareAdminUser.AddAllowedRoles(roleAdmin, roleUser)

userAdminGroup := router.Group("")
userAdminGroup.Use(gnext.Middleware{
    Before: authMiddlewareAdminUser.Before,
}

userAdminGroup.GET("/endpoint", myHandler)

This needs to be more scalable. If you have five roles, you need 5! objects of AuthMiddleware to represent every combination.

I have opened this issue to gather opinions from the contributors on how to improve this code. It would be relatively easy with pure Gin, but I'd lose all the benefits from GNext. Please let me know if you have any questions or suggestions.


Here's one idea I have in mind: building a permissions matrix. Instead of declaring allowed roles on a handler level, I'd do it in another package. It's a matrix of endpoints and roles.

/endpoint /anotherEndpoint
Admin Allow Allow
User Deny Allow

I'd add another dimension for the HTTP method. However, it takes a lot of work to represent 3D in markdown. It's a scalable solution, as far as I know. We could even have a standard authorization module on GNext.

kruczekrobert commented 1 year ago

Hey,

I've already given exactly the same idea in the issue: https://github.com/meteran/gnext/issues/14

meteran commented 1 year ago

Definitely, there should be a way to define permissions per endpoint and reflect them in the documentation.

Having a separate matrix in other package is not a good idea. If you are reviewing the code, you need to look into 2 places instead of one. It also causes the risk of mistakes like typo in the endpoint path (different in router and different in permission matrix). Or another block of consts with paths - boilerplate.

I would suggest to have something like:

r.GET(path, handler, middleware1, middleware2, ...)

However, current interface already has an optional parameter in route endpoint which points to the additional docs. We would like to keep both functionalities, so we can switch to options-like interface:

r.GET(path, handler, gnext.WithDocs(...), gnext.WithPermisions(...))

Means... breaking changes, which we would like to avoid now. It sounds like a good idea for the gNext v1.0.0. Alternatively we could change route method interface to get additional parameters as interface{} and recognize if they are middlewares or docs.

busf4ctor commented 1 year ago

Alternatively we could change route method interface to get additional parameters as interface{} and recognize if they are middlewares or docs.

I don't think it's a good solution. It'll make the method unclear, a basic functionality harder to use, and it will decrease readability.

Is it a breaking change? Yes. However, what's the main feature we'd like to provide? Docs, or a basic essential thing like endpoint specific middleware?

I've been using Gnext for the last few weeks and I find it confusing because it is too flexible. Reading the variable type of an endpoint and seeing interface{} for the handler makes the code writing slower because I always need the docs. The reflections increase runtime errors, so I never feel safe deploying it into production.