[GitHub] [cxf] andymc12 opened a new pull request #656: Consolidate getTargetMethod calls

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

[GitHub] [cxf] andymc12 opened a new pull request #656: Consolidate getTargetMethod calls

GitBox
andymc12 opened a new pull request #656: Consolidate getTargetMethod calls
URL: https://github.com/apache/cxf/pull/656
 
 
   I was looking at getting the target method in a new interceptor and realized that there are a few different places in the code that is essentially doing the same thing.  So I thought it would be better for maintenance to consolidate that function in one place.
   
   Feedback appreciated.  If nobody objects, I'd like to back port this to 3.3.x too.
   
   Thanks!

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
[hidden email]


With regards,
Apache Git Services
Reply | Threaded
Open this post in threaded view
|

[GitHub] [cxf] reta commented on a change in pull request #656: Consolidate getTargetMethod calls

GitBox
reta commented on a change in pull request #656: Consolidate getTargetMethod calls
URL: https://github.com/apache/cxf/pull/656#discussion_r398959852
 
 

 ##########
 File path: core/src/main/java/org/apache/cxf/validation/AbstractValidationInterceptor.java
 ##########
 @@ -108,15 +107,7 @@ protected Method getServiceMethod(Message message) {
         Message inMessage = message.getExchange().getInMessage();
         Method method = null;
         if (inMessage != null) {
-            method = (Method)inMessage.get("org.apache.cxf.resource.method");
-            if (method == null) {
-                BindingOperationInfo bop = inMessage.getExchange().getBindingOperationInfo();
-                if (bop != null) {
-                    MethodDispatcher md = (MethodDispatcher)
-                        inMessage.getExchange().getService().get(MethodDispatcher.class.getName());
-                    method = md.getMethod(bop);
-                }
-            }
+            method = MessageUtils.getTargetMethod(inMessage, null);
 
 Review comment:
   Minor, may be introducing the overloaded method `MessageUtils.getTargetMethod(inMessage)` (which calls the original one with `null` supplier) would be a bit more cleaner, no need to guess what second argument is. Alternatively, could be `Optional<Method>` so the caller can raise exceptions or get along with `null`. What do you think?

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
[hidden email]


With regards,
Apache Git Services
Reply | Threaded
Open this post in threaded view
|

[GitHub] [cxf] reta commented on issue #656: Consolidate getTargetMethod calls

GitBox
In reply to this post by GitBox
reta commented on issue #656: Consolidate getTargetMethod calls
URL: https://github.com/apache/cxf/pull/656#issuecomment-604745241
 
 
   Thanks @andymc12 !

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
[hidden email]


With regards,
Apache Git Services
Reply | Threaded
Open this post in threaded view
|

[GitHub] [cxf] andymc12 commented on a change in pull request #656: Consolidate getTargetMethod calls

GitBox
In reply to this post by GitBox
andymc12 commented on a change in pull request #656: Consolidate getTargetMethod calls
URL: https://github.com/apache/cxf/pull/656#discussion_r399315562
 
 

 ##########
 File path: core/src/main/java/org/apache/cxf/validation/AbstractValidationInterceptor.java
 ##########
 @@ -108,15 +107,7 @@ protected Method getServiceMethod(Message message) {
         Message inMessage = message.getExchange().getInMessage();
         Method method = null;
         if (inMessage != null) {
-            method = (Method)inMessage.get("org.apache.cxf.resource.method");
-            if (method == null) {
-                BindingOperationInfo bop = inMessage.getExchange().getBindingOperationInfo();
-                if (bop != null) {
-                    MethodDispatcher md = (MethodDispatcher)
-                        inMessage.getExchange().getService().get(MethodDispatcher.class.getName());
-                    method = md.getMethod(bop);
-                }
-            }
+            method = MessageUtils.getTargetMethod(inMessage, null);
 
 Review comment:
   Good idea!  I'll give the `Optional<Method>` approach a try.  Thanks!

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
[hidden email]


With regards,
Apache Git Services
Reply | Threaded
Open this post in threaded view
|

[GitHub] [cxf] andymc12 merged pull request #656: Consolidate getTargetMethod calls

GitBox
In reply to this post by GitBox
andymc12 merged pull request #656: Consolidate getTargetMethod calls
URL: https://github.com/apache/cxf/pull/656
 
 
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
[hidden email]


With regards,
Apache Git Services