Skip to content
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

fix method search in SecurityActions.findMethod() #1045

Merged
merged 1 commit into from
Aug 5, 2024

Conversation

Ladicek
Copy link
Contributor

@Ladicek Ladicek commented Aug 2, 2024

The previous implementation considered all superclasses, but only implemented interfaces of the bean class. The new implementation also considers implemented interfaces of all superclasses and superinterfaces.

The previous implementation considered all superclasses, but only
implemented interfaces of the bean class. The new implementation also
considers implemented interfaces of all superclasses and superinterfaces.
Copy link

@zakkak zakkak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since SmallRye is looking for a specific method with specific parameters I think it would be better to use Class.getMethod(...) on each class/interface instead of getting all methods and then filtering them by name.

Comment on lines -192 to +210
Class<?> clazz = declaringClass;
while (clazz != null) {
Deque<Class<?>> worklist = new ArrayDeque<>();
worklist.add(declaringClass);
while (!worklist.isEmpty()) {
Class<?> clazz = worklist.removeFirst();
for (Method m : clazz.getDeclaredMethods()) {
result.add(m.getName());
}
clazz = clazz.getSuperclass();
}

for (Class<?> iface : declaringClass.getInterfaces()) {
for (Method m : iface.getMethods()) {
result.add(m.getName());
if (clazz.getSuperclass() != null) {
worklist.add(clazz.getSuperclass());
}
Collections.addAll(worklist, clazz.getInterfaces());
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these really needed?

The comment above says:

// if we find a matching method on the bean class or one of its superclasses or superinterfaces,
// then we have to check that the method is either identical to or an override of a method that:
// - is declared on a class which is a superclass of the declaring class, or
// - is declared on an interface which implemented by the declaring class

which according to my understanding (and assuming it's correct) means that we only need to get the methods declared on the declaring class and its superclasses, plus the methods declared on the interfaces implemented by the declaring class only.

I believe the previous version of the code was fine and perhaps it would also suffice to replace iface.getMethods() with iface.getDeclaredMethod() (so that we don't need the whole hierarchy).

@Ladicek
Copy link
Contributor Author

Ladicek commented Aug 2, 2024

Since SmallRye is looking for a specific method with specific parameters I think it would be better to use Class.getMethod(...) on each class/interface instead of getting all methods and then filtering them by name.

Yeah, that unfortunately only works for public methods. And getDeclaredMethod() doesn't return inherited methods. That's why we need to traverse the inheritance hierarchy manually.

@zakkak
Copy link

zakkak commented Aug 2, 2024

Since SmallRye is looking for a specific method with specific parameters I think it would be better to use Class.getMethod(...) on each class/interface instead of getting all methods and then filtering them by name.

Yeah, that unfortunately only works for public methods. And getDeclaredMethod() doesn't return inherited methods. That's why we need to traverse the inheritance hierarchy manually.

Ooops, right. What about Class.getDeclaredMethod(...)?

@Ladicek Ladicek merged commit 2393d05 into smallrye:main Aug 5, 2024
13 checks passed
@Ladicek Ladicek deleted the fix-method-search branch August 5, 2024 08:52
zakkak added a commit to zakkak/smallrye-fault-tolerance that referenced this pull request Aug 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants