[GitHub] struts pull request #124: WW-4744: AnnotationUtils supports non-public metho...

classic Classic list List threaded Threaded
20 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] struts pull request #124: WW-4744: AnnotationUtils supports non-public metho...

sdutry
GitHub user yasserzamani opened a pull request:

    https://github.com/apache/struts/pull/124

    WW-4744: AnnotationUtils supports non-public methods

    With these changes, AnnotationUtils also has equipped with an internal cache. Mainly, getAnnotatedMethods and all it's usages have improved and equipped with enough unit tests (also all findAnnotation usages have equipped).
   
    NOTES:
    To avoid reinventing the wheel, I copied from Spring framework, So, I copied their license and authors as well where needed :)

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

    $ git pull https://github.com/yasserzamani/struts WW-4744

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

    https://github.com/apache/struts/pull/124.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 #124
   
----
commit e1bb1a70c089b80b3924391da9d0268b5bcc29f3
Author: Yasser Zamani <[hidden email]>
Date:   2017-03-19T15:32:45Z

    WW-4744: AnnotationUtils supports non-public methods

----


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] struts pull request #124: WW-4744: AnnotationUtils supports non-public metho...

sdutry
Github user lukaszlenart commented on a diff in the pull request:

    https://github.com/apache/struts/pull/124#discussion_r107079345
 
    --- Diff: core/src/main/java/com/opensymphony/xwork2/util/AnnotationUtils.java ---
    @@ -37,13 +40,25 @@
      * @author Rainer Hermanns
      * @author Zsolt Szasz, zsolt at lorecraft dot com
      * @author Dan Oxlade, dan d0t oxlade at gmail d0t c0m
    + * @author Rob Harrop
    + * @author Juergen Hoeller
    + * @author Sam Brannen
    + * @author Mark Fisher
    + * @author Chris Beams
    + * @author Phillip Webb
    --- End diff --
   
    I'm not sure if adding those authors here makes any sense, in most cases we (at ASF) trying to remove the `@author` tag also asking those users about this class can confuse them.


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] struts pull request #124: WW-4744: AnnotationUtils supports non-public metho...

sdutry
In reply to this post by sdutry
Github user lukaszlenart commented on a diff in the pull request:

    https://github.com/apache/struts/pull/124#discussion_r107079461
 
    --- Diff: core/src/main/java/com/opensymphony/xwork2/util/AnnotationUtils.java ---
    @@ -150,25 +166,32 @@ public static void addAllInterfaces(Class clazz, List<Class> allInterfaces) {
          * @return the annotation found, or {@code null} if none
          */
         public static <A extends Annotation> A findAnnotation(Method method, Class<A> annotationType) {
    -        A result = getAnnotation(method, annotationType);
    -        Class<?> clazz = method.getDeclaringClass();
    +        AnnotationCacheKey cacheKey = new AnnotationCacheKey(method, annotationType);
    +        A result = (A) findAnnotationCache.get(cacheKey);
             if (result == null) {
    -            result = searchOnInterfaces(method, annotationType, clazz.getInterfaces());
    -        }
    -        while (result == null) {
    -            clazz = clazz.getSuperclass();
    -            if (clazz == null || clazz.equals(Object.class)) {
    -                break;
    -            }
    -            try {
    -                Method equivalentMethod = clazz.getDeclaredMethod(method.getName(), method.getParameterTypes());
    -                result = getAnnotation(equivalentMethod, annotationType);
    -            } catch (NoSuchMethodException ex) {
    -                // No equivalent method found
    -            }
    +            result = getAnnotation(method, annotationType);
    +            Class<?> clazz = method.getDeclaringClass();
                 if (result == null) {
                     result = searchOnInterfaces(method, annotationType, clazz.getInterfaces());
                 }
    +            while (result == null) {
    +                clazz = clazz.getSuperclass();
    +                if (clazz == null || clazz.equals(Object.class)) {
    +                    break;
    +                }
    +                try {
    +                    Method equivalentMethod = clazz.getDeclaredMethod(method.getName(), method.getParameterTypes());
    +                    result = getAnnotation(equivalentMethod, annotationType);
    +                } catch (NoSuchMethodException ex) {
    +                    // No equivalent method found
    --- End diff --
   
    no need to LOG this exception?


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] struts issue #124: WW-4744: AnnotationUtils supports non-public methods

sdutry
In reply to this post by sdutry
Github user lukaszlenart commented on the issue:

    https://github.com/apache/struts/pull/124
 
    Looks good, just have some doubts about putting `@author` tag


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [GitHub] struts pull request #124: WW-4744: AnnotationUtils supports non-public metho...

Lukasz Lenart
In reply to this post by sdutry
2017-03-21 7:04 GMT+01:00 lukaszlenart <[hidden email]>:

> Github user lukaszlenart commented on a diff in the pull request:
>
>     https://github.com/apache/struts/pull/124#discussion_r107079345
>
>     --- Diff: core/src/main/java/com/opensymphony/xwork2/util/AnnotationUtils.java ---
>     @@ -37,13 +40,25 @@
>       * @author Rainer Hermanns
>       * @author Zsolt Szasz, zsolt at lorecraft dot com
>       * @author Dan Oxlade, dan d0t oxlade at gmail d0t c0m
>     + * @author Rob Harrop
>     + * @author Juergen Hoeller
>     + * @author Sam Brannen
>     + * @author Mark Fisher
>     + * @author Chris Beams
>     + * @author Phillip Webb
>     --- End diff --
>
>     I'm not sure if adding those authors here makes any sense, in most cases we (at ASF) trying to remove the `@author` tag also asking those users about this class can confuse them.

Should I ask legal [1] about that? What do you think?

[1] https://issues.apache.org/jira/browse/LEGAL/


Regards
--
Łukasz
+ 48 606 323 122 http://www.lenart.org.pl/

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] struts issue #124: WW-4744: AnnotationUtils supports non-public methods

sdutry
In reply to this post by sdutry
Github user cnenning commented on the issue:

    https://github.com/apache/struts/pull/124
 
    > Should I ask legal [1] about that? What do you think?
   
    That's the class that has been copied from spring. Yes, I think it's better to ask how to handle 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 [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] struts issue #124: WW-4744: AnnotationUtils supports non-public methods

sdutry
In reply to this post by sdutry
Github user lukaszlenart commented on the issue:

    https://github.com/apache/struts/pull/124
 
    > That's the class that has been copied from spring
   
    I meant, coping classes is ok if the license allows that (Spring uses AL 2.0) but my questions was about adding authors to some other class (not copied from Spring), maybe let's allow Yasser explain why he did 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 [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] struts issue #124: WW-4744: AnnotationUtils supports non-public methods

sdutry
In reply to this post by sdutry
Github user aleksandr-m commented on the issue:

    https://github.com/apache/struts/pull/124
 
    Do we need custom annotation utility at all? Can't we just use existing libraries e.g. Apache Commons [MethodUtils](https://commons.apache.org/proper/commons-lang/apidocs/org/apache/commons/lang3/reflect/MethodUtils.html)?


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] struts issue #124: WW-4744: AnnotationUtils supports non-public methods

sdutry
In reply to this post by sdutry
Github user yasserzamani commented on the issue:

    https://github.com/apache/struts/pull/124
 
    > Yasser explain why he did that
   
    Thank you for your review.
   
    `AnnotationUtils` is not a new class but to avoid reinventing the wheel, I copied and merged some codes from Spring into that. While Spring has `Copyright 2002-2014 the original author or authors.` in his License header, I was not sure what to do with `@author`s :(
   
    Currently, I am searching Internet for same to see what people do. Finally, I also may ask Spring.


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] struts issue #124: WW-4744: AnnotationUtils supports non-public methods

sdutry
In reply to this post by sdutry
Github user lukaszlenart commented on the issue:

    https://github.com/apache/struts/pull/124
 
    We must rather ask ASF legal body, you can post an issue in JIRA (see above)


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] struts issue #124: WW-4744: AnnotationUtils supports non-public methods

sdutry
In reply to this post by sdutry
Github user yasserzamani commented on the issue:

    https://github.com/apache/struts/pull/124
 
    > We must rather ask ASF legal body, you can post an issue in JIRA (see above)
   
    Thank you. As I could not find similar issue, I created [Merge others codes having same license but different copyrights](https://issues.apache.org/jira/browse/LEGAL-295).


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] struts pull request #124: WW-4744: AnnotationUtils supports non-public metho...

sdutry
In reply to this post by sdutry
Github user yasserzamani commented on a diff in the pull request:

    https://github.com/apache/struts/pull/124#discussion_r107110525
 
    --- Diff: core/src/main/java/com/opensymphony/xwork2/util/AnnotationUtils.java ---
    @@ -150,25 +166,32 @@ public static void addAllInterfaces(Class clazz, List<Class> allInterfaces) {
          * @return the annotation found, or {@code null} if none
          */
         public static <A extends Annotation> A findAnnotation(Method method, Class<A> annotationType) {
    -        A result = getAnnotation(method, annotationType);
    -        Class<?> clazz = method.getDeclaringClass();
    +        AnnotationCacheKey cacheKey = new AnnotationCacheKey(method, annotationType);
    +        A result = (A) findAnnotationCache.get(cacheKey);
             if (result == null) {
    -            result = searchOnInterfaces(method, annotationType, clazz.getInterfaces());
    -        }
    -        while (result == null) {
    -            clazz = clazz.getSuperclass();
    -            if (clazz == null || clazz.equals(Object.class)) {
    -                break;
    -            }
    -            try {
    -                Method equivalentMethod = clazz.getDeclaredMethod(method.getName(), method.getParameterTypes());
    -                result = getAnnotation(equivalentMethod, annotationType);
    -            } catch (NoSuchMethodException ex) {
    -                // No equivalent method found
    -            }
    +            result = getAnnotation(method, annotationType);
    +            Class<?> clazz = method.getDeclaringClass();
                 if (result == null) {
                     result = searchOnInterfaces(method, annotationType, clazz.getInterfaces());
                 }
    +            while (result == null) {
    +                clazz = clazz.getSuperclass();
    +                if (clazz == null || clazz.equals(Object.class)) {
    +                    break;
    +                }
    +                try {
    +                    Method equivalentMethod = clazz.getDeclaredMethod(method.getName(), method.getParameterTypes());
    +                    result = getAnnotation(equivalentMethod, annotationType);
    +                } catch (NoSuchMethodException ex) {
    +                    // No equivalent method found
    --- End diff --
   
    No, it is common to not found the method in one of supercleasses during our hierarchically search.


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] struts issue #124: WW-4744: AnnotationUtils supports non-public methods

sdutry
In reply to this post by sdutry
Github user yasserzamani commented on the issue:

    https://github.com/apache/struts/pull/124
 
    I also wait for ASF LEGAL part's answer but after some study, it seems I was not allowed to do such work 😞  If so, I think I have to write my own utils at where @aleksandr-m mentioned; re-inventing the wheel :(
   
    Below are references; what do you think, please?
   
    References:
    [1] [https://www.apache.org/licenses/LICENSE-2.0](https://www.apache.org/licenses/LICENSE-2.0)
   
    > 4. Redistribution.
    > c. You must retain, in the Source form of any Derivative Works that You distribute, all copyright, patent, trademark, and attribution notices from the Source form of the Work, excluding those notices that do not pertain to any part of the Derivative Works; and
   
    [2] [http://www.apache.org/legal/src-headers.html](http://www.apache.org/legal/src-headers.html)
   
    > TREATMENT OF THIRD-PARTY WORKS
    > 1. Do not modify or remove any copyright notices or licenses within third-party works.
   
    [3] [http://www.apache.org/dev/apply-license.html](http://www.apache.org/dev/apply-license.html)
   
    > CAN/SHOULD INDIVIDUAL COMMITTERS ADDED COPYRIGHT STATEMENTS TO THE NOTICE OR SOURCE CODE FILES?
    >
    > No.
    >
    > Though committers retain copyright, Apache asks that they do not add copyright statements. See the policy for more details.


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] struts issue #124: WW-4744: AnnotationUtils supports non-public methods

sdutry
In reply to this post by sdutry
Github user yasserzamani commented on the issue:

    https://github.com/apache/struts/pull/124
 
    > Do we need custom annotation utility at all? Can't we just use existing libraries e.g. Apache Commons MethodUtils?
   
    Thank you @aleksandr-m !
   
    I am working on it. I started by creating [LANG-1317](https://issues.apache.org/jira/browse/LANG-1317)


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] struts issue #124: WW-4744: AnnotationUtils supports non-public methods

sdutry
In reply to this post by sdutry
Github user yasserzamani commented on the issue:

    https://github.com/apache/struts/pull/124
 
    ✅ All copied Spring's code including what I was imported in previously merged [PR#117](https://github.com/apache/struts/pull/117) removed 👌
   
    After merge of [LANG-1317 PR](https://github.com/apache/commons-lang/pull/261) and S2's upgrade to use commons lang 3.6, I'll remove deprecated `getAnnotatedMethods` and `findAnnotation` to Apache Commons Lang 3.6 👌


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] struts issue #124: WW-4744: AnnotationUtils supports non-public methods

sdutry
In reply to this post by sdutry
Github user lukaszlenart commented on the issue:

    https://github.com/apache/struts/pull/124
 
    So as I understand this is ready to be merged with a future notice to use Commons Lang 3.6 when available?


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] struts issue #124: WW-4744: AnnotationUtils supports non-public methods

sdutry
In reply to this post by sdutry
Github user yasserzamani commented on the issue:

    https://github.com/apache/struts/pull/124
 
    @lukaszlenart , Yes in my opinion, Thank you!


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] struts issue #124: WW-4744: AnnotationUtils supports non-public methods

sdutry
In reply to this post by sdutry
Github user lukaszlenart commented on the issue:

    https://github.com/apache/struts/pull/124
 
    Cool, I will review this tomorrow just to double check and LGTM! Great 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 [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] struts issue #124: WW-4744: AnnotationUtils supports non-public methods

sdutry
In reply to this post by sdutry
Github user lukaszlenart commented on the issue:

    https://github.com/apache/struts/pull/124
 
    Great work, LGTM! 👍


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] struts pull request #124: WW-4744: AnnotationUtils supports non-public metho...

sdutry
In reply to this post by sdutry
Github user asfgit closed the pull request at:

    https://github.com/apache/struts/pull/124


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Loading...