rachavz / reflections

Automatically exported from code.google.com/p/reflections
Do What The F*ck You Want To Public License
0 stars 0 forks source link

Bug in Configuration Mng, addClassLoader does not affect ReflectionUtils.forNames/forName #174

Open GoogleCodeExporter opened 9 years ago

GoogleCodeExporter commented 9 years ago
What steps will reproduce the problem?

1. Setup a ConfigurationBuilder with a custom ClassLoader (use addClassLoader)

2. Use e.g. myreflections.getTypesAnnotatedWith() everytime the scan hits a 
class only resolveable by the custom ClassLoader fires an "could not get type 
for name" exception.

Cause/Fix: In ReflectionUtils.forNames/forName the configured ClassLoaders are 
not used, because the variable list classLoaders is always null. Fixed it 
myself by passing configuration interface to Store and paramterize forNames 
correctly.

In my opinion its a design issue, because the configuration is passed from 
method call to method call. And with your extensive use of variable lists its 
likely to forget to pass the configuration. 

Have a look at getTypesAnnotatedWith() in Refeltions.java for example:

public Set<Class<?>> getTypesAnnotatedWith(final Class<? extends Annotation> 
annotation, boolean honorInherited) {

(1)        Set<String> typesAnnotatedWith = 
store.getTypesAnnotatedWith(annotation.getName(), honorInherited);

(2)        return Sets.newHashSet(forNames(typesAnnotatedWith, 
configuration.getClassLoaders()));

    }

In (2) the setup of configuration parameter is correctly. 

But forName() is already invoked by (1). And your stack looks something like 
this at (1):

(config available) -> () -> () -> () -> isClass(!missing config parameter!) -> 
forNames(!missing config parameter!) -> forName(!missing config parameter!) 

I dont like passing parameters through out the stack..To be honest, i dont like 
the config management in this lib. And your Builder pattern (with all these 
guessing stuff) has a strange taste for me. But okay different developers, 
different approaches...

But i really like the results and easy usage of your lib :o) Thanks for your 
great work.

Original issue reported on code.google.com by tomyma...@googlemail.com on 30 Apr 2014 at 9:34

GoogleCodeExporter commented 9 years ago
To reproduce it easily:

1) Add a custom ClassLoader (addClassLoader)
2) Debug ReflectionUtils.forName or .forNames , look if your configured 
ClassLoader is present. Right its missing.

Original comment by tomyma...@googlemail.com on 30 Apr 2014 at 9:37

GoogleCodeExporter commented 9 years ago
The missing classloader on the stack call to getTypesAnnotatedWith will be 
fixed.
I agree, passing the classloaders all around is a code smell. A workaround is 
to have the context classloader properly set (but that's not always the case), 
or not use static methods such as in ReflectionUtils (which would mean 
instantiate everything with a context - don't like it either...)
As for the ConfigurationBuilder - a patch is more then welcomed!
Thanks for a great feedback, Tomy!

Original comment by ronm...@gmail.com on 5 Jun 2014 at 6:16