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.

Sign up for GitHub

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

Jump to bottom

一点关于UrlCleaner的改进建议和意见 #1287

Open
eacdy opened this issue Feb 14, 2020 · 4 comments
Open

一点关于UrlCleaner的改进建议和意见 #1287

eacdy opened this issue Feb 14, 2020 · 4 comments
Labels
area/integrations Issues or PRs related to integrations with open-source components good first issue Good for newcomers kind/enhancement Category issues or prs related to enhancement.

Comments

@eacdy
Copy link

eacdy commented Feb 14, 2020

这里，UrlCleaner特指sentinel-spring-webmvc-adapter中的UrlCleaner：
https://github.com/alibaba/Sentinel/blob/master/sentinel-adapter/sentinel-spring-webmvc-adapter/src/main/java/com/alibaba/csp/sentinel/adapter/spring/webmvc/callback/UrlCleaner.java

个人认为有两点值得改进：

  • UrlCleaner名称词不达意。UrlCleaner重写的不是Url，而是资源名称，叫ResourceNameRewiter或许更加贴切。

  • 目前UrlCleaner提供的参数较少，能定制的空间很小。目前只能在原资源名称的基础上改写资源。但实际业务中，往往需要根据具体的请求重写资源名称。因此，加上 HttpServletRequest 的参数灵活性会好很多。
@sczyh30 sczyh30 added area/integrations Issues or PRs related to integrations with open-source components kind/enhancement Category issues or prs related to enhancement. good first issue Good for newcomers labels Feb 14, 2020
@sczyh30
Copy link
Member

sczyh30 commented Feb 14, 2020 •
edited
Loading

Thanks for your suggestions! For sentinel-spring-webmvc-adapter I think it's a good idea to add a HttpServletRequest arg to the clean method, but we need to think of the compatibility.

Discussions and contributions are welcomed!

增加 request 参数确实比较有用，但是需要考虑兼容性问题（sentinel-spring-webmvc-adapter 模块发布不久，影响有限；但是 sentinel-web-servlet 模块使用者较多，需要特别注意）。

@vansee
Copy link

vansee commented Feb 21, 2020

I'd like to work on this issue

@sczyh30
Copy link
Member

sczyh30 commented Feb 21, 2020

👍 Looking forward to your PR!

vansee pushed a commit to vansee/Sentinel that referenced this issue Feb 21, 2020
Describe what this PR does / why we need it
83bb43f 
    Add HttpServletRequest arg to clean method on UrlCleaner, for get attribute or header value from the web request
Does this pull request fix one issue?
Related to alibaba#1287
Describe how you did it

Describe how to verify it

Special notes for reviews
@vansee vansee mentioned this issue Feb 21, 2020
Open
@zhaoyuguang
Copy link
Collaborator

建议 UrlCleaner 改成抽象类 增加多态方法 来得到更高的兼容性
还有是否可以将sentinel 编译版本改为1.8 使用default接口
或者当然 使用新的ResourceNameRewiter 也是如此，需要考虑多态支持已有的扩展

@zhaoyuguang zhaoyuguang mentioned this issue May 8, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
area/integrations Issues or PRs related to integrations with open-source components good first issue Good for newcomers kind/enhancement Category issues or prs related to enhancement.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@vansee @eacdy @sczyh30 @zhaoyuguang