You are viewing a plain text version of this content. The canonical link for it is here.

Posted to issues@commons.apache.org by Derek-Ashmore <gi...@git.apache.org> on 2016/05/13 09:50:01 UTC

[GitHub] commons-lang pull request: Lang 1195: Enhance MethodUtils to allow...

GitHub user Derek-Ashmore opened a pull request:

    https://github.com/apache/commons-lang/pull/141

    Lang 1195: Enhance MethodUtils to allow invocation of private methods

    Currently, MethodUtils is restricted to finding and invoking accessible methods. Frequently, developers have a need to test 'private' methods. What I see is that they escalate access to 'protected' in order to more easily provide test coverage for these methods. From a design perspective, this is bad.
    
    I propose to enhance MethodUtils so that it can easily invoke private methods. I'm not suggesting that developers should do this in production code, merely test code. Much as FieldUtils provides access to private fields via the 'forceAccess' overload on many of its methods. I've copied a utility like this around for years. It would be much more convenient to simply include it with Commons Lang. See [LANG-1195](https://issues.apache.org/jira/browse/LANG-1195) for details.
    
    I made sure that I used space indentation and tried to adhere closely to your standards.  I've also signed the Contributor License Agreement.
    
    Assuming you like the enhancement, I'm willing to contribute additional work if you see issues with what I've done.

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/Force66/commons-lang LANG-1195

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/commons-lang/pull/141.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #141
    
----
commit f53fb65535232962ccd35ba9ad66e2e4c248f92e
Author: Derek Ashmore <da...@force66.com>
Date:   2016-01-01T14:31:47Z

    See LANG-1195; Enhance MethodUtils to allow invocation of private
    methods

commit 4a256e62529fbc57dfb55a99e222ec3591cee7ec
Author: Derek Ashmore <da...@force66.com>
Date:   2016-05-13T09:42:32Z

    See LANG-1195; Removed accidental tab indentation

----


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] commons-lang issue #141: Lang 1195: Enhance MethodUtils to allow invocation ...

Posted by PascalSchumacher <gi...@git.apache.org>.

Github user PascalSchumacher commented on the issue:

    https://github.com/apache/commons-lang/pull/141
  
    Hi Derek,
    
    sorry for the delay.
    
    I took another look at the code and compared it with `FieldUitls`. Maybe it is a good idea to change the method parameter order to `Object object, String methodName, boolean forceAccess`. I think `object` and `methodName` belong together and this order is similar to the order of the parameters of `FieldUtils` methods e.g. `FieldUtils#readDeclaredField(Object target, String fieldName, boolean forceAccess)`.
    
    What do you think?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] commons-lang issue #141: Lang 1195: Enhance MethodUtils to allow invocation ...

Posted by PascalSchumacher <gi...@git.apache.org>.

Github user PascalSchumacher commented on the issue:

    https://github.com/apache/commons-lang/pull/141
  
    @Derek-Ashmore Just today I learned that the resetting of the original accessibility of the method I asked you to add is not necessary, because `Method#setAccessible` only modifies the behavior of the `AccessibleObject` not of the actual method. 
    
    See http://stackoverflow.com/questions/10638826/java-reflection-impact-of-setaccessibletrue and http://www.artima.com/weblogs/viewpost.jsp?thread=164042 for details.
    
    Sorry that I caused you extra work. :(


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] commons-lang pull request #141: Lang 1195: Enhance MethodUtils to allow invo...

Posted by Derek-Ashmore <gi...@git.apache.org>.

Github user Derek-Ashmore commented on a diff in the pull request:

    https://github.com/apache/commons-lang/pull/141#discussion_r65728446
  
    --- Diff: src/main/java/org/apache/commons/lang3/reflect/MethodUtils.java ---
    @@ -92,6 +92,29 @@ public static Object invokeMethod(final Object object, final String methodName)
                 IllegalAccessException, InvocationTargetException {
             return invokeMethod(object, methodName, ArrayUtils.EMPTY_OBJECT_ARRAY, null);
         }
    +    
    +    /**
    +     * <p>Invokes a named method without parameters.</p>
    +     *
    +     * <p>This is a convenient wrapper for
    +     * {@link #invokeMethod(Object object,String methodName, Object[] args, Class[] parameterTypes)}.
    --- End diff --
    
    Yeah -- my bad. I'll correct the javadoc link. Thanks for catching that.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] commons-lang issue #141: Lang 1195: Enhance MethodUtils to allow invocation ...

Posted by Derek-Ashmore <gi...@git.apache.org>.

Github user Derek-Ashmore commented on the issue:

    https://github.com/apache/commons-lang/pull/141
  
    Agreed -- in addition to the other changes you mentioned, I'll re-base using the current master so that it's easier to merge in.   Thanks for looking at this.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] commons-lang issue #141: Lang 1195: Enhance MethodUtils to allow invocation ...

Posted by PascalSchumacher <gi...@git.apache.org>.

Github user PascalSchumacher commented on the issue:

    https://github.com/apache/commons-lang/pull/141
  
    You are right. I missed that. It is impossible to use the parameter order I suggested, because it would clash with the existing `invokeMethod` signature. Sorry.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] commons-lang issue #141: Lang 1195: Enhance MethodUtils to allow invocation ...

Posted by Derek-Ashmore <gi...@git.apache.org>.

Github user Derek-Ashmore commented on the issue:

    https://github.com/apache/commons-lang/pull/141
  
    It's the Objects.deepEquals() -- to new for the supported jdk versions -- I'll remove it


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] commons-lang issue #141: Lang 1195: Enhance MethodUtils to allow invocation ...

Posted by Derek-Ashmore <gi...@git.apache.org>.

Github user Derek-Ashmore commented on the issue:

    https://github.com/apache/commons-lang/pull/141
  
    I happened to notice that I inadvertently made unwanted formatting changes in a previous commit. I've take pains to reduce the number of changed lines dramatically. Hopefully, this will make it easier to merge this change into the trunk if it's decided that this is a reasonable enhancement.
    
    I'm also willing to make additional changes to the if you would like; anything I can do to make this contribution easier to merge in. 
    
    Please let me know if you have questions.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] commons-lang issue #141: Lang 1195: Enhance MethodUtils to allow invocation ...

Posted by Derek-Ashmore <gi...@git.apache.org>.

Github user Derek-Ashmore commented on the issue:

    https://github.com/apache/commons-lang/pull/141
  
    Good idea -- the change has been made and checked in.  Any other thoughts?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] commons-lang pull request #141: Lang 1195: Enhance MethodUtils to allow invo...

Posted by PascalSchumacher <gi...@git.apache.org>.

Github user PascalSchumacher commented on a diff in the pull request:

    https://github.com/apache/commons-lang/pull/141#discussion_r65721245
  
    --- Diff: src/main/java/org/apache/commons/lang3/reflect/MethodUtils.java ---
    @@ -142,21 +193,57 @@ public static Object invokeMethod(final Object object, final String methodName,
          * @throws InvocationTargetException wraps an exception thrown by the method invoked
          * @throws IllegalAccessException if the requested method is not accessible via reflection
          */
    -    public static Object invokeMethod(final Object object, final String methodName,
    +    public static Object invokeMethod(final Object object, final boolean forceAccess, final String methodName,
                 Object[] args, Class<?>[] parameterTypes)
                 throws NoSuchMethodException, IllegalAccessException,
                 InvocationTargetException {
             parameterTypes = ArrayUtils.nullToEmpty(parameterTypes);
             args = ArrayUtils.nullToEmpty(args);
    -        final Method method = getMatchingAccessibleMethod(object.getClass(),
    -                methodName, parameterTypes);
    +        
    +        final String messagePrefix;
    +        final Method method;
    +        if (forceAccess) {
    +            messagePrefix = "No such method: ";
    +            method = getMatchingMethod(object.getClass(), methodName, parameterTypes);
    +            if (method != null) {
    +                method.setAccessible(true);
    --- End diff --
    
    For `FieldUtils` there is this jira issue: https://issues.apache.org/jira/browse/LANG-965?jql=project%20%3D%20LANG%20AND%20status%20%3D%20Open%20ORDER%20BY%20priority%20DESC "FieldUtils methods leak accessible flags" with requests that the visibility of a field should be restored after the field is written. Maybe the visibility of the method should also be restored after it is invoked? What do think? 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] commons-lang pull request #141: Lang 1195: Enhance MethodUtils to allow invo...

Posted by PascalSchumacher <gi...@git.apache.org>.

Github user PascalSchumacher commented on a diff in the pull request:

    https://github.com/apache/commons-lang/pull/141#discussion_r65720235
  
    --- Diff: src/main/java/org/apache/commons/lang3/reflect/MethodUtils.java ---
    @@ -126,13 +149,41 @@ public static Object invokeMethod(final Object object, final String methodName,
         /**
          * <p>Invokes a named method whose parameter type matches the object type.</p>
          *
    -     * <p>This method delegates the method search to {@link #getMatchingAccessibleMethod(Class, String, Class[])}.</p>
    +     * <p>This method supports calls to methods taking primitive parameters 
    +     * via passing in wrapping classes. So, for example, a {@code Boolean} object
    +     * would match a {@code boolean} primitive.</p>
    +     *
    +     * <p>This is a convenient wrapper for
    +     * {@link #invokeMethod(Object object,String methodName, Object[] args, Class[] parameterTypes)}.
    --- End diff --
    
    I guess this should be ``invokeMethod(final Object object, final boolean forceAccess, final String methodName, Object[] args, Class[] parameterTypes)`?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] commons-lang pull request #141: Lang 1195: Enhance MethodUtils to allow invo...

Posted by PascalSchumacher <gi...@git.apache.org>.

Github user PascalSchumacher commented on a diff in the pull request:

    https://github.com/apache/commons-lang/pull/141#discussion_r65719370
  
    --- Diff: src/main/java/org/apache/commons/lang3/reflect/MethodUtils.java ---
    @@ -126,13 +149,41 @@ public static Object invokeMethod(final Object object, final String methodName,
         /**
          * <p>Invokes a named method whose parameter type matches the object type.</p>
          *
    -     * <p>This method delegates the method search to {@link #getMatchingAccessibleMethod(Class, String, Class[])}.</p>
    +     * <p>This method supports calls to methods taking primitive parameters 
    +     * via passing in wrapping classes. So, for example, a {@code Boolean} object
    +     * would match a {@code boolean} primitive.</p>
    +     *
    +     * <p>This is a convenient wrapper for
    +     * {@link #invokeMethod(Object object,String methodName, Object[] args, Class[] parameterTypes)}.
    --- End diff --
    
    I guess this should be ``invokeMethod(final Object object, final boolean forceAccess, final String methodName, Object[] args, Class[] parameterTypes)`?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---