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
Feature/dubbo32. resteasy filter&intercept support #12492
Feature/dubbo32. resteasy filter&intercept support #12492
Conversation
Please add the newly added SPI extension to shade configuration in dubbo-all/pom.xml and fix the conflicts.
|
…to feature/dubbo32._resteasy_filter&intercept_support # Conflicts: # dubbo-rpc/dubbo-rpc-rest/src/main/java/org/apache/dubbo/rpc/protocol/rest/handler/NettyHttpHandler.java # dubbo-rpc/dubbo-rpc-rest/src/test/java/org/apache/dubbo/rpc/protocol/rest/JaxrsRestProtocolTest.java
@@ -58,7 +62,7 @@ protected void decode(ChannelHandlerContext ctx, io.netty.handler.codec.http.Ful | |||
|
|||
// business handler | |||
try { | |||
handler.handle(requestFacade, nettyHttpResponse); | |||
new NettyHttpHandler(serviceDeployer, url).handle(requestFacade, nettyHttpResponse); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can result in constant creation of NettyHttpHandler
instances
new RestFilter() { | ||
@Override | ||
public void filter(URL url, RequestFacade requestFacade, NettyHttpResponse response, RestFilterChain restFilterChain) throws Exception { | ||
|
||
restFilterChain.filter(url, requestFacade, response, restFilterChain); | ||
} | ||
}.filter(url, requestFacade, nettyHttpResponse, new RestFilterChain(restFilters)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like RestFilterChain is fixed, because restFilters are fixed, I think the RestFilterChain here is not unnecessary, looking at the RestFilterChain logic, should just iterate through the execution of these restfilter
. I don't see much logic at the moment.
public class ServiceInvokeRestResponseInterceptor implements RestResponseInterceptor { | ||
|
||
@Override | ||
public void intercept(URL url, RequestFacade request, NettyHttpResponse nettyHttpResponse, Object result, RpcInvocation rpcInvocation, RestResponseInterceptorChain interceptorChain, ServiceDeployer serviceDeployer) throws Exception { | ||
|
||
writeResult(nettyHttpResponse, request, url, result, rpcInvocation.getReturnType()); | ||
nettyHttpResponse.setStatus(200); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does not need to exist and may skew other non-200 results.
...pc-rest/src/main/java/org/apache/dubbo/rpc/protocol/rest/filter/ServiceInvokeRestFilter.java
Outdated
Show resolved
Hide resolved
.../apache/dubbo/rpc/protocol/rest/extension/resteay/filter/ResteasyContainerFilterAdapter.java
Outdated
Show resolved
Hide resolved
…intercept_support' into feature/dubbo32._resteasy_filter&intercept_support # Conflicts: # dubbo-rpc/dubbo-rpc-rest/src/main/java/org/apache/dubbo/rpc/protocol/rest/RestProtocol.java
Codecov Report
@@ Coverage Diff @@
## 3.2 #12492 +/- ##
=============================================
+ Coverage 68.73% 69.35% +0.62%
+ Complexity 341 2 -339
=============================================
Files 3619 1647 -1972
Lines 169138 68122 -101016
Branches 28109 9961 -18148
=============================================
- Hits 116249 47248 -69001
+ Misses 42799 16320 -26479
+ Partials 10090 4554 -5536 see 2047 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
@CrazyHZM change request & response filter |
|
||
@Override | ||
public void proceed() throws IOException, WebApplicationException { | ||
LogMessages.LOGGER.debugf("Interceptor Context: %s, Method : proceed", getClass().getName()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replace with dubbo's own logger.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ping @suncairong163
import java.util.ArrayList; | ||
import java.util.List; | ||
|
||
public class RestFilterManager { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Each method should go back to where it was originally used and static classes are not a very good approach.
For example, executeRequestFilters and executeResponseFilters should revert to NettyHttpHandler, and restRequestFilters should be fetched from the FrameworkModel in the context. Such as the url FrameworkModel.
@CrazyHZM fix some change request |
Kudos, SonarCloud Quality Gate passed! |
What is the purpose of the change
add resteasy filter&intercept support
Brief changelog
Verifying this change
Checklist