Add Sentinel SpringMVC adapter module by kaizi2009 · Pull Request #1104 · alibaba/Sentinel

kaizi2009 · 2019-10-21T18:22:32Z

Why we need it?

We expect Sentinel's target is '/foo/{id}'

Issue

#1074

How to verify it

Implements org.springframework.web.servlet.HandlerInterceptor

Junit:

module: sentinel-spring-webmvc-adapter
com.alibaba.csp.sentinel.adapter.spring.webmvc.TestInterceptor

codecov-io · 2019-10-21T18:29:06Z

Codecov Report

Merging #1104 into master will increase coverage by 0.21%.
The diff coverage is 64.03%.

@@             Coverage Diff              @@
##             master    #1104      +/-   ##
============================================
+ Coverage     42.92%   43.13%   +0.21%     
- Complexity     1502     1535      +33     
============================================
  Files           320      327       +7     
  Lines          9400     9514     +114     
  Branches       1283     1294      +11     
============================================
+ Hits           4035     4104      +69     
- Misses         4870     4902      +32     
- Partials        495      508      +13

Impacted Files	Coverage Δ	Complexity Δ
.../webmvc/callback/DefaultBlockExceptionHandler.java	`0% <0%> (ø)`	`0 <0> (?)`
...pring/webmvc/config/SentinelWebMvcTotalConfig.java	`100% <100%> (ø)`	`3 <3> (?)`
...adapter/spring/webmvc/config/BaseWebMvcConfig.java	`100% <100%> (ø)`	`7 <7> (?)`
...nel/adapter/spring/webmvc/SentinelInterceptor.java	`50% <50%> (ø)`	`3 <3> (?)`
...dapter/spring/webmvc/SentinelTotalInterceptor.java	`61.53% <61.53%> (ø)`	`3 <3> (?)`
...ter/spring/webmvc/AbstractSentinelInterceptor.java	`68.18% <68.18%> (ø)`	`13 <13> (?)`
...ter/spring/webmvc/config/SentinelWebMvcConfig.java	`77.77% <77.77%> (ø)`	`4 <4> (?)`
...a/csp/sentinel/slots/statistic/base/LeapArray.java	`67.32% <0%> (-2.98%)`	`33% <0%> (-1%)`
...tinel/slots/block/flow/param/ParamFlowChecker.java	`52.7% <0%> (-2.71%)`	`28% <0%> (-1%)`
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f6dff5a...2309973. Read the comment docs.

zhaoyuguang · 2019-10-22T02:56:48Z

        <module>sentinel-web-servlet</module>
        <module>sentinel-dubbo-adapter</module>
        <module>sentinel-apache-dubbo-adapter</module>
-        <module>sentinel-grpc-adapter</module>


What is the reason for deleting this sentinel-grpc-adapter module?

Miss class in module: sentinel-grpc-adapter

Has been modified

zhaoyuguang · 2019-10-22T03:11:37Z

+    protected void setEntryContainerInReqeust(HttpServletRequest request, String name, EntryContainer entryContainer) {
+        Object attrVal = request.getAttribute(name);
+        if (attrVal != null) {
+            throw new SentinelSpringMvcException("Already exist attribute name '" + name + "' in request");


It seems to me that throwing an exception directly is not very friendly.

Use RecordLog.warn

zhaoyuguang · 2019-10-22T03:11:55Z

+    protected EntryContainer getEntryContainerInReqeust(HttpServletRequest request, String attrKey) {
+        Object entityContainerObject = request.getAttribute(attrKey);
+        if (entityContainerObject == null) {
+            throw new SentinelSpringMvcException("EntryContainer is null in request");


same above.

RecordLog.warn

zhaoyuguang · 2019-10-22T03:19:21Z

+            if (entryContainer.getUrlEntry() != null) {
+                entryContainer.getUrlEntry().exit();
+            }
+            removeEntryContainerInReqeust(request, config.getRequestAttributeName());


First exit httpMethodUrlEntry and then exit urlEntry.
And Trace the Exception ex at here may be better.

I will modify it

zhaoyuguang · 2019-10-22T03:21:44Z

+            ContextUtil.enter(SPRING_MVC_CONTEXT_NAME);
+            Entry entry = SphU.entry(config.getTotalResourceName());
+            final EntryContainer entryContainer = new EntryContainer().setUrlEntry(entry);
+            setEntryContainerInReqeust(request, config.getRequestAttributeName(), entryContainer);


It might be better to use a new property such as totalUrlEntry

Good suggestion, will modify it

zhaoyuguang · 2019-10-22T03:22:09Z

+            }
+            removeEntryContainerInReqeust(request, config.getRequestAttributeName());
+        }
+        ContextUtil.exit();


Trace the Exception ex at here may be better.

I will modify it

zhaoyuguang · 2019-10-22T03:24:10Z

+/**
+ * @Author kaizi2009
+ */
+public class DefaultUrlCleaner implements UrlCleaner {


DefaultUrlCleaner is not necessary, in my opinion

Good suggestion.

kaizi2009

There is one question left to discuss: Order of 'SentinelInterceptor' and 'SentinelTotalInterceptor'.
I don't think the order matters, what is your detailed analysis?

kaizi2009 · 2019-10-22T17:05:09Z

+    protected EntryContainer getEntryContainerInReqeust(HttpServletRequest request, String attrKey) {
+        Object entityContainerObject = request.getAttribute(attrKey);
+        if (entityContainerObject == null) {
+            throw new SentinelSpringMvcException("EntryContainer is null in request");


RecordLog.warn

kaizi2009 · 2019-10-22T17:42:53Z

+            if (entryContainer.getUrlEntry() != null) {
+                entryContainer.getUrlEntry().exit();
+            }
+            removeEntryContainerInReqeust(request, config.getRequestAttributeName());


kaizi2009 · 2019-10-22T17:43:09Z

+            }
+            removeEntryContainerInReqeust(request, config.getRequestAttributeName());
+        }
+        ContextUtil.exit();


kaizi2009 · 2019-10-22T17:43:23Z

+/**
+ * @Author kaizi2009
+ */
+public class DefaultUrlCleaner implements UrlCleaner {


kaizi2009

Dear @zhaoyuguang ! Please review changes, I have two different views:

The order of the interceptors does not matter.
Handle BlockException in spring MVC, because spring MVC's exception handling is more customized

zhaoyuguang · 2019-10-23T08:28:26Z

Dear @zhaoyuguang ! Please review changes, I have two different views:

The order of the interceptors does not matter.

Handle BlockException in spring MVC, because spring MVC's exception handling is more customized

make sense

sczyh30 · 2019-10-25T18:08:54Z

👍 I'll take a look these days.

kaizi2009

Optimization configuration class.
Optimization AbstractSentinelInterceptor

kaizi2009

viewed

sczyh30 · 2019-10-31T08:38:26Z

+    @Override
+    public boolean preHandle(HttpServletRequest request, HttpServletResponse response, Object handler)
+            throws Exception {
+        Entry urlEntry = null;


Maybe here only one entry is needed. If the httpMethodSpecify is enabled, then the resource name could be METHOD:path, or the resource name could be path.

I agree with your suggestion. I will modify.

sczyh30 · 2019-10-31T08:38:57Z

+                    httpMethodUrlEntry = SphU.entry(request.getMethod().toUpperCase() + COLON + resourceName,
+                            EntryType.IN);
+                }
+                final EntryContainer entryContainer = new EntryContainer().setUrlEntry(urlEntry)


Since only one entry is needed, maybe we could directly carry the Entry in the request attribute.

I agree with your suggestion. I will modify.

kaizi2009

@sczyh30

Use project default java.source.version.
2.Use only on Entry
3.Remove configuration item httpMethodSpecify in SentinelWebMvcTotalConfig

sczyh30 · 2019-11-26T01:34:53Z

+ */
+@ControllerAdvice
+@Order(0)
+public class SentinelSringMvcBlockHandlerConfig {


Modification completed.

sczyh30 · 2019-11-26T01:35:37Z

+import org.springframework.web.servlet.ModelAndView;
+
+/**
+ * @Author kaizi2009


Please format all your file headers: @Author -> @author

Modification completed.

sczyh30 · 2019-11-26T01:58:56Z

+        }
+    }
+
+    protected Entry getEntryInReqeust(HttpServletRequest request, String attrKey) {


typo: Reqeust -> Request

Modification completed.

…into sentinel-spring-webmvc-adapter

kaizi2009

Modify naming.

sczyh30

LGTM

sczyh30 · 2019-11-27T06:45:09Z

Nice work! I'll improve the code later.

Maybe we could replace the sentinel-web-servlet in Spring Cloud Alibaba Sentinel with the sentinel-spring-webmvc-adapter later. @fangjian0423

sczyh30 · 2019-11-27T06:47:13Z

Thanks for contributing!

kaizi2009 · 2019-11-27T10:09:35Z

@sczyh30 com.alibaba.csp.sentinel.adapter.spring.webmvc.config.BaseWebMvcConfig#blockExceptionHandler
Is it necessary to set the DefaultBlockExceptionHandler?

My original idea was:
In most cases, users need to customize BlockExceptionHandler, the DefaultBlockExceptionHandler is not what users want.

My idea now is:
For friendship, set default BlockExceptionHandler

BaseWebMvcConfig
Modify:
BlockExceptionHandler blockExceptionHandler = new DefaultBlockExceptionHandler().
README.md
Delete:
2. Use DefaultBlockExceptionHandler
//SentinelWebMvcTotalConfig config = new SentinelWebMvcTotalConfig();
SentinelWebMvcConfig config = new SentinelWebMvcConfig();
config.setBlockExceptionHandler(new DefaultBlockExceptionHandler());

kaizi2009 · 2019-11-27T10:11:21Z

Need to create new PR?

kaizi2009 · 2019-11-27T10:18:30Z

@sczyh30 What is your suggestion?

sczyh30 · 2019-11-27T11:35:16Z

Need to create new PR?

Feel free to submit a new PR (don't forget to update the relevant part of the document :)

kaizi2009 · 2019-11-28T11:09:24Z

It is better to throw.

- Add sentinel-spring-webmvc-adapter module and demo

kaizi2009 force-pushed the sentinel-spring-webmvc-adapter branch from 8d0f81c to b0dc5a1 Compare October 21, 2019 19:01

sczyh30 added area/integrations Issues or PRs related to integrations with open-source components to-review To review labels Oct 22, 2019

zhaoyuguang reviewed Oct 22, 2019

View reviewed changes

kaizi2009 force-pushed the sentinel-spring-webmvc-adapter branch from b0dc5a1 to 2b406f0 Compare October 22, 2019 17:33

kaizi2009 commented Oct 22, 2019

View reviewed changes

kaizi2009 force-pushed the sentinel-spring-webmvc-adapter branch 2 times, most recently from 011fda0 to 58177bc Compare October 23, 2019 13:52

kaizi2009 force-pushed the sentinel-spring-webmvc-adapter branch 3 times, most recently from 181ad0f to 16a4d67 Compare October 26, 2019 18:02

kaizi2009 commented Oct 26, 2019

View reviewed changes

kaizi2009 force-pushed the sentinel-spring-webmvc-adapter branch 2 times, most recently from 0e5c6fb to a57ef22 Compare October 27, 2019 07:55

spring mvc adapter

d9a6a42

kaizi2009 force-pushed the sentinel-spring-webmvc-adapter branch from a57ef22 to d9a6a42 Compare October 27, 2019 08:00

kaizi2009 commented Oct 30, 2019

View reviewed changes

sczyh30 reviewed Oct 31, 2019

View reviewed changes

kaizi2009 commented Nov 3, 2019

View reviewed changes

Use only one entry, java.source.version 1.7

2309973

kaizi2009 force-pushed the sentinel-spring-webmvc-adapter branch from a2947e7 to 2309973 Compare November 3, 2019 16:47

kaizi2009 changed the title ~~sentinel spring mvc adapter~~ Add Sentinel spring mvc adapter module Nov 7, 2019

sczyh30 reviewed Nov 26, 2019

View reviewed changes

kaizi2009 added 2 commits November 26, 2019 12:04

Modify naming

5dc64c5

Modify naming

cb3503a

Merge remote-tracking branch 'origin/sentinel-spring-webmvc-adapter' …

b3cce1b

…into sentinel-spring-webmvc-adapter

kaizi2009 changed the title ~~Add Sentinel spring mvc adapter module~~ Add sentinel spring mvc adapter module Nov 26, 2019

kaizi2009 changed the title ~~Add sentinel spring mvc adapter module~~ Add Sentinel SpringMVC adapter module Nov 26, 2019

kaizi2009 commented Nov 26, 2019

View reviewed changes

sczyh30 approved these changes Nov 27, 2019

View reviewed changes

sczyh30 added kind/feature Category issues or prs related to feature request. and removed to-review To review labels Nov 27, 2019

sczyh30 added this to the 1.7.1 milestone Nov 27, 2019

sczyh30 merged commit b14534f into alibaba:master Nov 27, 2019

sczyh30 mentioned this pull request Nov 27, 2019

provide sentinel-spring-webmvc-adapter #1093

Closed

This was referenced Nov 28, 2019

Can Spring MVC adapters be provided? #1074

Closed

REST API pattern UrlCleaner同一处理 #286

Closed

hughpearse pushed a commit to hughpearse/Sentinel that referenced this pull request Jun 2, 2021

Add Sentinel Spring Web MVC adapter module (alibaba#1104)

774db8f

- Add sentinel-spring-webmvc-adapter module and demo

Uh oh!

Conversation

kaizi2009 commented Oct 21, 2019

Why we need it?

Issue

How to verify it

Junit:

Uh oh!

codecov-io commented Oct 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kaizi2009 Oct 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kaizi2009 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kaizi2009 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhaoyuguang commented Oct 23, 2019

Uh oh!

sczyh30 commented Oct 25, 2019

codecov-io commented Oct 21, 2019 •

edited

Loading

kaizi2009 Oct 22, 2019 •

edited

Loading

kaizi2009 left a comment •

edited

Loading