Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reduce HandlerID cardinality by default in gin middleware #93

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

arwineap
Copy link

Here's the doc for FullPath()

FullPath returns a matched route full path. For not found routes
returns an empty string.
    router.GET("/user/:id", func(c *gin.Context) {
        c.FullPath() == "/user/:id" // true
    })

IMO it's better to do the correct thing by default when possible; existing users would benefit from performance increases but it may break some dashboards.

Let me know what you think,
Thanks!

Here's the doc for FullPath()
```
FullPath returns a matched route full path. For not found routes
returns an empty string.
    router.GET("/user/:id", func(c *gin.Context) {
        c.FullPath() == "/user/:id" // true
    })
```
@arwineap arwineap requested a review from slok as a code owner November 18, 2021 04:35
@slok
Copy link
Owner

slok commented Dec 3, 2021

Hi @arwineap!

Thanks for the contribution, You are right, safety should be a default. However, this will have unexpected behaviour for users at this moment, and that could be worse. Maybe would be better to have a flag on middleware creation and break the API so when people upgrade, they are aware of this safer behavior change that will be by default. e.g:

func Handler(handlerID string, m middleware.Middleware, rawPaths bool) gin.HandlerFunc

Let me think about it!

@ImVexed
Copy link

ImVexed commented Jan 23, 2022

Just a bump on this as I found myself wondering why there wasn't an option for this, so I ended up just implementing it myself locally.

+1 for FullPath()

@slok
Copy link
Owner

slok commented Feb 11, 2022

@arwineap @ImVexed Ok, let's do this, add an option on middleware creation so it breaks the API and users are aware of this change. Similar to what is above, by default would use FullPath and if the flag of RawPaths is used then, we use the current way of measuring it.

WDYT?

@tekkamanendless
Copy link

I'm doing something similar for gorestful in #150. Instead of adding a rawPaths bool to Handler, why not consider adding a Config object specific for gin so that we can easily add more options later if that ends up being important.

In #150, I also introduced HandlerWithConfig and left Handler alone (Handler would then be backward compatible and just call HandlerWithConfig with an empty Config{}).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants