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

WELD-2501 Add reproducer, first attempt to fix this #1856

Merged
merged 1 commit into from Jul 24, 2018

Conversation

Projects
None yet
2 participants
@manovotn
Copy link
Contributor

commented Jul 2, 2018

No description provided.

@manovotn manovotn added the hold label Jul 2, 2018

@manovotn

This comment has been minimized.

Copy link
Contributor Author

commented Jul 2, 2018

To sum up this 1.0 approach I took...

I first altered Reflections.getInterfaceClosure(javaClass, new HashSet<>()) to perform "deep scanning" make our ATs aware of these default methods we may possibly need. This is required as further on, as we create the subclass, methods we discover are checked against those we know the AT can/should implement.

Then I modified the proxy factory and made it go over whole type hierarchy (gathered in previous step) but only implement 1st level interfaces (as anything else may be "hidden" - package private); all other interfaces are later inspected for default methods which we are adding to subclasses.

@manovotn manovotn requested review from mkouba and nziakova Jul 2, 2018

@manovotn manovotn removed the hold label Jul 2, 2018

@@ -465,14 +465,20 @@ public static Exception unwrapInvocationTargetException(InvocationTargetExceptio
}
}

public static Set<Class<?>> getInterfaceClosure(Class<?> clazz) {
public static Set<Class<?>> getInterfaceClosure(Class<?> clazz, Set<Class<?>> setOfInterfaces) {

This comment has been minimized.

Copy link
@manovotn

manovotn Jul 23, 2018

Author Contributor

We need to add a unit test to org.jboss.weld.tests.unit.util.reflection.ReflectionsTest to have a verification of this new behaviour.

This comment has been minimized.

Copy link
@manovotn

manovotn Jul 23, 2018

Author Contributor

Also, the signature could stay the same (e.g. remove Set param) and create a new set in each method invocation.

This comment has been minimized.

Copy link
@mkouba

mkouba Jul 23, 2018

Member

Something like this should work:

public static Set<Class<?>> getInterfaceClosure(Class<?> clazz) {
   Set<Class<?>> result = new HashSet<>();  
   for (Class<?> classToDiscover = clazz; classToDiscover != null; classToDiscover = classToDiscover.getSuperclass()) {
     addInterfaces(classToDiscover, result);
   }
   return result;
}
@@ -238,6 +254,16 @@ void doReturn(CodeAttribute b, ClassMethod method) {
}
}

@Override
public Set<Class<?>> getAdditionalInterfaces() {

This comment has been minimized.

Copy link
@mkouba

mkouba Jul 23, 2018

Member

I think we should not override the getAdditionalInterfaces() method but use the interfacesToInspect directly where needed. We don't want to extend the additionalInterfaces set as defined in ProxyFactory after all.

WELD-2501 Add reproducer.
While creating interceptor/decorator subclass, we need to inspect the interface hierarchy deeper, but not implement them directly.

@manovotn manovotn force-pushed the manovotn:weld2501 branch from b4676e6 to 0db6b22 Jul 24, 2018

@manovotn

This comment has been minimized.

Copy link
Contributor Author

commented Jul 24, 2018

Agreed via chat that the state of this PR is good to merge.

@manovotn manovotn merged commit 2cabfac into weld:master Jul 24, 2018

5 checks passed

Weld CI - JDK 10 Build finished.
Details
Weld CI - JDK 8 Build finished.
Details
Weld Publish CI - JDK 10 Finished
Details
Weld Publish CI - JDK 8 Finished
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.