LANG-1317 Adds MethodUtils#findAnnotation and extend MethodUtils#getMethodsWithAnnotation for non-public, super-class and interface methods

[yasserzamani]

In order to fix WW-4744 ,

findAnnotation will be an extension for Method.getAnnotation that also searches interfaces and super classes.

findMethodsWithAnnotation will be an extension for getMethodsWithAnnotation that also supports non public methods, super class and interface methods.

NOTES:
To keep changes simple, currently no cache provided. Let's do that when needed 😃

[coveralls]

Coverage decreased (-0.002%) to 94.603% when pulling e49a3a2 on yasserzamani:LANG-1317 into 4a300fe on apache:master.

[PascalSchumacher]

Thanks for the pull request!

It is really unfortunate that the existing method getMethodsWithAnnotation is not called getAccessibleMethodsWithAnnotation : (, because then the new method could be called getMethodsWithAnnotation.

I would not have been able to tell the difference between getMethodsWithAnnotation and findMethodsWithAnnotation without reading the javadoc.

I believe we need a more intention revealing method name or maybe we should just add an overload of getMethodsWithAnnotation e.g. getMethodsWithAnnotation(Class cls, annotationCls, boolean ignoreAccessiblitiy).

What do you think?

[yasserzamani]

@PascalSchumacher , thank you! I was overloaded by another job and forgot that I also can use method overloads. Yes, strongly I agree. Just I have some doubts about name ignoreAccessiblitiy because the new method is not only about accessibility, but also about traverse super classes and implemented interface and super interfaces while the annotation maybe is not present on the given class but is present on lower layers. Maybe we should also add another param named searchSupers for example.

[PascalSchumacher]

You are right. The method signature I suggested omits the important super part. I agree it should be something like getMethodsWithAnnotation(Class cls, annotationCls, boolean searchSupers, boolean ignoreAccess). (I'm not a fan of boolean parameters (especially multiple ones), so if somebody has an idea for a good method name, that is welcome.)

[yasserzamani]

Another bad thing is that getMethodsWithAnnotation(cls, annotationCls, false, false) will be a duplicate for getMethodsWithAnnotation(cls, annotationCls).

Another solution is to keep my current changes not modified but add following javadoc:

 *
 * <p>To lookup annotations on the given class level only choose {@code get*()} methods
 * and to lookup in the entire inheritance hierarchy of the given class and ignore 
 * accessibility, choose {@code find*()} methods</p>
 *
 * @since 2.5
 */
public class MethodUtils {

With these, for (searchSupers,igonreAccess) pair, we have both (false,false) and (true,true) available. In future, if somebody need other possibilities, new methods can be added e.g. findAccessible* for (true,false) and getAll* for (false,true) 🤔

[PascalSchumacher]

Well we have to keep getMethodsWithAnnotation(cls, annotationCls) for compatibility reasons. In my opinion it is not problem if is equal to getMethodsWithAnnotation(cls, annotationCls, false, false). It can be refactored to call getMethodsWithAnnotation(cls, annotationCls, false, false) and then it is just some sugar for the most? common use case.

[yasserzamani]

👌 I am working on a new commit. thanks!

[coveralls]

Coverage decreased (-0.03%) to 94.571% when pulling 7390433 on yasserzamani:LANG-1317 into 4a300fe on apache:master.

[yasserzamani]

@PascalSchumacher , I made it better 😃 thanks a lot!

Please feel free to make any recommendation if you think I can make it better.

Coverage decreased (-0.03%) to 94.571%

😞 Do you know why? 👆

[aaabramov]

@yasserzamani I thinks because you added more conditional branches in the latest commit and that is why less LOC are covered

[coveralls]

Coverage decreased (-0.03%) to 94.578% when pulling b79fe6f on yasserzamani:LANG-1317 into 4a300fe on apache:master.

[yasserzamani]

Thank you @Abrasha ,

I signed up with coveralls.io and see that all of this PR's changes are covered with unit tests: MethodUtils , ClassUtils.

Also these files show enhancement in coverage at COVERAGE CHANGED 🤔

[PascalSchumacher]

Concerning the coverage: I think it's because the coverage differs between java versions. The build used to determine coverage depends on the order in which the travis builds finish.

[PascalSchumacher]

@yasserzamani Sorry for the delay, other open source projects and vacations interfered.

I'm not sure if the priority parameter of getAllSuperclassesAndInterfaces is necessary? Are you aware of any use-cases where it is essential? Maybe we should keep things simple and remove it?

Otherwise the pull request is fine. :=) 👍

[coveralls]

Coverage increased (+0.07%) to 94.675% when pulling 90e252f on yasserzamani:LANG-1317 into 4a300fe on apache:master.

[yasserzamani]

Sorry for the delay, other open source projects and vacations interfered.

@PascalSchumacher , You're welcome, I understand and as an open source fun contributor, I'm patient enough :)

I'm not sure if the priority parameter is necessary?

No, is not. I just thought I should make it more general with more options to prevent future needs for other user's PRs. Now, with thanks to your recommendations I removed it :)

[PascalSchumacher]

This should also mention Interfaces.

Can you add a since tag to this method?

Thanks!

[PascalSchumacher]

Created https://issues.apache.org/jira/browse/LANG-1321 for the ClassUtils#getAllSuperclassesAndInterfaces addition.

[PascalSchumacher]

If I'm not mistaken the list returned by getAllSuperclassesAndInterfaces can contain the same interface multiple times if it is implemented by more than one super class. To prevent this the result should be collected in LinkedHashSet first (similar to https://github.com/yasserzamani/commons-lang/blob/90e252f571377cac39f5e2a3fc73749f589c24de/src/main/java/org/apache/commons/lang3/ClassUtils.java#L448). What do you think?

Edit: I misunderstood what getAllSuperclassesAndInterfaces does. Please ignore the paragraph above. I think we should remove ClassUtils#getAllSuperclassesAndInterfaces and replace it by a private method in MethodUtils which just combines the result of ClassUtils#getAllSuperclasses and ClassUtils#getAllInterfaces (as discussed above the order does not matter, so no need for a new method which just reorders the results). What do you think?

[yasserzamani]

@PascalSchumacher , sorry for my bad decisions. Again I thought I should keep it public to provide same functionality for someone who needs in future :)

Now, I removed it to MethodUtils as a private method. I also added @since tag and mentioned interfaces in @return java doc.

Thanks a lot!

[coveralls]

Coverage increased (+0.09%) to 94.693% when pulling 6aee420 on yasserzamani:LANG-1317 into 4a300fe on apache:master.

[coveralls]

Coverage increased (+0.05%) to 94.655% when pulling 6aee420 on yasserzamani:LANG-1317 into 4a300fe on apache:master.

[PascalSchumacher]

@yasserzamani Thanks!

No need to apologize. It is just that we try to be conservative with new additions (If we add a public method we have to support it for a long time and can not do any incompatible changes.). If the method is deemed generally useful it can always be moved and made public later on.

[PascalSchumacher]

Merged. Thank you very much.