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

fix #49; implement gin adapter #82

Merged
merged 6 commits into from
Mar 15, 2020
Merged

Conversation

gorexlv
Copy link
Contributor

@gorexlv gorexlv commented Mar 9, 2020

Describe what this PR does / why we need it

Does this pull request fix one issue?

Describe how you did it

Describe how to verify it

Special notes for reviews

@louyuting louyuting added area/integrations Issue related to integrations with open-source components to-review PRs to review labels Mar 9, 2020
adapter/gin/middleware.go Outdated Show resolved Hide resolved
adapter/gin/option.go Outdated Show resolved Hide resolved
@sczyh30 sczyh30 linked an issue Mar 10, 2020 that may be closed by this pull request
@sczyh30 sczyh30 added this to the 0.2.0 milestone Mar 10, 2020
@sczyh30
Copy link
Member

sczyh30 commented Mar 10, 2020

And could you please add the document to https://github.com/sentinel-group/sentinel-website/tree/master/docs/zh-cn/golang?

func SentinelMiddleware(opts ...Option) gin.HandlerFunc {
options := evaluateOptions(opts)
return func(c *gin.Context) {
resourceName := c.Request.Method + ":" + c.Request.URL.Path
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe here we need c.FullPath() to get the matched route path (e.g. /foo/:id) rather than the actual path (to avoid generating too many URL entries, bringing memory footprint). And as the comments say:

FullPath returns a matched route full path. For not found routes returns an empty string.

So when the FullPath() returns the empty string, we could just skip it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually we did this by customizing an extractor and matcher.
But in the latest version, the FullPath method is provided, so we can use it directly.

Thank you for pointing it out :)

"github.com/gin-gonic/gin"
)

// SentinelMiddleware
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please improve the comment here?


_, err = flow.LoadRules([]*flow.FlowRule{
{
Resource: "GET_/ping",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GET:/ping?

Copy link
Member

@sczyh30 sczyh30 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@sczyh30 sczyh30 merged commit a24a42d into alibaba:master Mar 15, 2020
@sczyh30
Copy link
Member

sczyh30 commented Mar 15, 2020

Nice work. Thanks for contributing!

@sczyh30 sczyh30 removed the to-review PRs to review label Mar 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/integrations Issue related to integrations with open-source components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add integration module for Gin web framework
3 participants