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-2357 fallback in JandexDiscoveryStrategy when there is no ClassI… #1633

Merged
merged 1 commit into from Mar 28, 2017

Conversation

tremes
Copy link
Contributor

@tremes tremes commented Mar 24, 2017

…nfo available.

@@ -493,4 +499,36 @@ public static boolean isDefault(Method method) {
return ((method.getModifiers() & (Modifier.ABSTRACT | Modifier.PUBLIC | Modifier.STATIC)) == Modifier.PUBLIC)
&& method.getDeclaringClass().isInterface();
}

public static boolean hasBeanDefiningAnnotation(Class<?> clazz, Set<Class<? extends Annotation>> initialBeanDefiningAnnotations) {
Copy link
Member

Choose a reason for hiding this comment

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

Wrong Reflections class ;-) It should be org.jboss.weld.environment.util.Reflections.

@tremes tremes force-pushed the WELD-2357 branch 2 times, most recently from a63ebb2 to 7c14cfa Compare March 27, 2017 08:55
@@ -33,6 +40,8 @@
*/
public final class Reflections {

public static final List<Class<? extends Annotation>> metaAnnotations = ImmutableList.of(Stereotype.class, NormalScope.class);
Copy link
Member

Choose a reason for hiding this comment

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

The constant name should be META_ANNOTATIONS

@@ -125,6 +134,8 @@ public static boolean isClassLoadable(ResourceLoader resourceLoader, String clas
return cast(Class.forName(className));
} catch (Exception e) {
Copy link
Member

Choose a reason for hiding this comment

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

What about catch (Exception | LinkageError e) {...?

} catch (Exception e) {
throw CommonLogger.LOG.cannotLoadClass(className, e);
} catch (Exception | LinkageError e) {
throw new ResourceLoadingException("Error loading class " + className, e);
Copy link
Member

Choose a reason for hiding this comment

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

I think we should use CommonLogger.LOG.cannotLoadClass() here. org.jboss.weld.resources.spi.ResourceLoader is related to ResourceLoader. Also the cannotLoadClass() method is only used here...

} else {
//if ClassInfo is not available (e.g for WEB-INF/lib/jars) then fallback to reflection
Class<?> clazz = Reflections.loadClass(resourceLoader, className);
if (!Reflections.hasBeanDefiningAnnotation(clazz, initialBeanDefiningAnnotations)) {
Copy link
Member

Choose a reason for hiding this comment

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

One last thing - we should add if (clazz != null) to avoid possible NPE...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then it would make more sense to define it as:
clazz == null || !Reflections.hasBeanDefiningAnnotation(clazz, initialBeanDefiningAnnotations)
or not?

Copy link
Member

Choose a reason for hiding this comment

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

Actually, we should follow ReflectionDiscoveryStrategy.java , ie.:

if (clazz == null || !Reflections.hasBeanDefiningAnnotation(clazz, initialBeanDefiningAnnotations)) {
   classIterator.remove();
}

@mkouba mkouba merged commit 4a270da into weld:master Mar 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants