Annotation Tests in deployers-vfs

[adrian.brock]

The annotation tests in deployers-vfs are failing for me under eclipse.

I don't know why they are working under Maven (command line)
probably a difference in classpath issue.

When I look at these tests I see a number of problems

These tests should be using the classes in target/classes compiled from src/main
with the deployment constructed using multiple bases.
i.e. using the AssembledDirectory vfs

Major Problems I see:

1) Use of ClassLoaderSystem.getInstance()
Each test should be constructing its own ClassLoaderSystem not
sharing the default one. Using the default one is bound to introduce leaks across tests.
See for example the ClassLoaderDependenciesTest in deployers-impl
on how to do this.

2) Isolated classloading. It should also be setting the DefaultDomain
in its own ClassLoaderSystem to be BEFORE_BUT_JAVA_ONLY
again see ClassLoaderDependenciesTest
otherwise you're going to load classes from the testsuite classpath
rather than the deployment so there's no real test at all

3) BeanScanningDeployer using setRelativeOrder is a hack
and the component handling is fundamentally broken/brain dead.

 public BeanScanningDeployer(KernelDeploymentDeployer kdd)
 {
 super(AnnotationEnvironment.class);
 setInputs(BeanMetaData.class);
 setOutput(BeanMetaData.class);
 if (kdd != null)
 setRelativeOrder(kdd.getRelativeOrder() + 10);
 else
 setRelativeOrder(getRelativeOrder() + 10);
 }

This breaks the orthogonality of the deployers. One deployer should
not depend upon another. This needs doing in a different way.

This can be seen further with the "mapComponents" stuff.
This deployer should either be a component deployer or a top level deployer
not mixing/matching and hacking in some bizarre way.

It certainly shouldn't be creating components by delegation to a different deployer
that has run earlier.

Iteration and delegation, {insert explitive here :-) }

 for (BeanMetaData bfb : bfBeans)
 {
 KernelDeploymentDeployer.addBeanComponent(unit, bfb);

Who knows what happens when addBeanComponent throws an error?

With these current hacks it is impossible for me to insert some extra behaviour
between these deployers because it is doing a stupid almost indeterminate dance.
Nor can I replace one of these deployers with alternate behaviour because
they are too tied together.

4) Debugging

Even if I do

enableTrace("org.jboss")

I get no inclination why this test is failing.
Where is the debug information?

5) Others

There's probably more wrong? But this is just from a basic look.
e.g. why do these tests depend upon the VFS anyway?

As far as I can tell, these tests would work just as well under the Mock classloading
in deployers-impl if written properly?

[adrian.brock]

My guess is that these deployers needs rewriting such the flow goes something like:

1) Deployer that collects Bean/BeanFactory annotations as attachments
2) KernelDeploymentDeployer is ammended to create components
for any Bean/BeanFactory (if metadata doesn't exist).

The association between the two deployers would be some collection
of annotations or the annotation itself with an associated factory for each.

In pseudo code:

An abstraction that defines how to create a BeanMetaData
from a Bean or collection of Beans (or in fact something more generic)

public interface AnnotationProcessor<Class<A extends A>, Class<B extends Collection<A>>, C>
{
...;
}

With a concrete implementation for BeanMetaData

public class BeanProcessor implements AnnoationProcessor<Bean, Beans, BeanMetaData>
{
...;
}

KernelDeploymentDeployer would take the processor(s)
This could probably be done generically in the abstract component deployer

 public KernelDeploymentDeployer(List<AnnotationProcessor> processors)
 {
 setDeploymentVisitor(new KernelDeploymentVisitor());
 setComponentVisitor(new BeanMetaDataVisitor());

 // Add setInput for Bean/Beans, i.e. processor.getAnnotation/s()
 // Add setOutput for processor.getOutput(), i.e. BeanMetaData
 setProcessors(processors);
 }

The component deployer would then visit the Bean/Beans annotations
invoke run the processor then call the relevant the addComponent method
of the normal visitor.

Of course you need a way to handle duplicate names but that should be
known by the processor.

With this generic processing in place, we are back to a more reasonable flow.

* A proper depdendency between deployers
* You can easily insert between the deployers and understand what is happening
* You can replace deployers to get different behaviour
* Using a visitor pattern for the processors makes error handling easy/less error prone
* Other deployers can reuse the new metadata in an understandable way

e.g. There would be nothing to stop me writing a deployer that does

Bean bean = AnnotationFactory.createAnnotations("@Bean ...");
unit.addAttachment(Bean.class, bean);

and it would get processed naturally.

Although I don't know why you want to do this. :-)

[adrian.brock]

My suggestion also has the advantage that it is extensible.
e.g. I could an annotation processor for some other annotation

[alesj]

These tests should be using the classes in target/classes compiled from src/main
with the deployment constructed using multiple bases.
i.e. using the AssembledDirectory vfs

Major Problems I see:

1) Use of ClassLoaderSystem.getInstance()
Each test should be constructing its own ClassLoaderSystem not
sharing the default one. Using the default one is bound to introduce leaks across tests.
See for example the ClassLoaderDependenciesTest in deployers-impl
on how to do this.

2) Isolated classloading. It should also be setting the DefaultDomain
in its own ClassLoaderSystem to be BEFORE_BUT_JAVA_ONLY
again see ClassLoaderDependenciesTest
otherwise you're going to load classes from the testsuite classpath
rather than the deployment so there's no real test at all

Yup, it's on my todo list.
That's been puzzling me from the beginning.
But since it worked, in both - IDEA and maven - I didn't go and investigate in details. :-)

3) BeanScanningDeployer using setRelativeOrder is a hack
and the component handling is fundamentally broken/brain dead.

 public BeanScanningDeployer(KernelDeploymentDeployer kdd)
 {
 super(AnnotationEnvironment.class);
 setInputs(BeanMetaData.class);
 setOutput(BeanMetaData.class);
 if (kdd != null)
 setRelativeOrder(kdd.getRelativeOrder() + 10);
 else
 setRelativeOrder(getRelativeOrder() + 10);
 }

This breaks the orthogonality of the deployers. One deployer should
not depend upon another. This needs doing in a different way.

This can be seen further with the "mapComponents" stuff.
This deployer should either be a component deployer or a top level deployer
not mixing/matching and hacking in some bizarre way.

It certainly shouldn't be creating components by delegation to a different deployer that has run earlier.

Iteration and delegation, {insert explitive here :-) }

 for (BeanMetaData bfb : bfBeans)
 {
 KernelDeploymentDeployer.addBeanComponent(unit, bfb);

Who knows what happens when addBeanComponent throws an error?

With these current hacks it is impossible for me to insert some extra behaviour
between these deployers because it is doing a stupid almost indeterminate dance.
Nor can I replace one of these deployers with alternate behaviour because
they are too tied together.

(A bit of cardboard posting ... again)
OK, just saw your 2nd post to this issue. :-)

4) Debugging

Even if I do

enableTrace("org.jboss")

I get no inclination why this test is failing.
Where is the debug information?

Coming up. ;-)

5) Others

There's probably more wrong? But this is just from a basic look.
e.g. why do these tests depend upon the VFS anyway?

As far as I can tell, these tests would work just as well under the Mock classloading
in deployers-impl if written properly?

OK, I'll leave the current one's as they are, and add a few there.

[alesj]

My suggestion also has the advantage that it is extensible.
e.g. I could an annotation processor for some other annotation

This should be then there already. ;-)
So there would be no need for my crappy prototype.

[alesj]

Iteration and delegation, {insert explitive here :-) }

 for (BeanMetaData bfb : bfBeans)
 {
 KernelDeploymentDeployer.addBeanComponent(unit, bfb);

Who knows what happens when addBeanComponent throws an error?

Why do we treat component failure on deploy (AbstractComponentDeployer)

 try
 {
 deployComponents(unit);
 }
 catch (Throwable t)
 {
 undeployComponents(unit);
 throw DeploymentException.rethrowAsDeploymentException("Error deploying: " + unit.getName(), t);
 }

differently then in AbstractRealDeployerWithInput

 List<T> visited = new ArrayList();
 try
 {
 Set<? extends T> deployments = unit.getAllMetaData(getInput());
 for (T deployment : deployments)
 {
 visitor.deploy(unit, deployment);
 visited.add(deployment);
 }
 }
 catch (Throwable t)
 {
 for (int i = visited.size()-1; i >= 0; --i)
 {
 try
 {
 visitor.undeploy(unit, visited.get(i));
 }
 catch (Throwable ignored)
 {
 log.warn("Error during undeploy: " + unit.getName(), ignored);
 }
 }
 throw DeploymentException.rethrowAsDeploymentException("Error deploying: " + unit.getName(), t);
 }

[alesj]

Of course you need a way to handle duplicate names but that should be
known by the processor.

Can I add?

DeploymentUnit::getComponent(String name) --> DeploymentUnit

To ease up on component iteration regarding code duplication. ;-)

[adrian.brock]

"alesj" wrote:

Why do we treat component failure on deploy (AbstractComponentDeployer)
...
differently then in AbstractRealDeployerWithInput

I didn't know we did :-)

Looks like I never did the unwind of the components properly, instead it tries
to undeploy all the components whether they got created or not.

[alesj]

Looks like I never did the unwind of the components properly, instead it tries
to undeploy all the components whether they got created or not.

I fixed it here:
- http://www.jboss.com/index.html?module=bb&op=viewtopic&t=136534
;-)

[adrian.brock]

"alesj" wrote:

Can I add?

DeploymentUnit::getComponent(String name) --> DeploymentUnit

To ease up on component iteration regarding code duplication. ;-)

I guarantee somebody will misuse it. :-)

[alesj]

I guarantee somebody will misuse it. :-)

They can already do that.
It's only that it will require more code for them to do it.
Which might prevent them from trying. :-)

[alesj]

This is what I came up with:

public abstract class AbstractAnnotationDeployer<D, C> extends AbstractComponentDeployer<D, C>
{
 /** The annotation processors */
 private AnnotationProcessor<?, C>[] processors;

 public AbstractAnnotationDeployer(AnnotationProcessor<?, C>... processors)
 {
 super();
 if (processors != null && processors.length > 0)
 {
 addInput(AnnotationEnvironment.class);
 this.processors = processors;
 for (AnnotationProcessor processor : processors)
 {
 addInput(processor.getAnnotation());
 }
 }
 }

 public void internalDeploy(DeploymentUnit unit) throws DeploymentException
 {
 super.internalDeploy(unit);

 if (processors != null && compVisitor != null)
 {
 try
 {
 AnnotationEnvironment env = unit.getAttachment(AnnotationEnvironment.class);
 for (AnnotationProcessor<?, C> processor : processors)
 {
 List<C> components = new ArrayList<C>();

 if (env != null)
 {
 // from classes
 Set<Class<?>> classes = env.classIsAnnotatedWith(processor.getAnnotation());
 for (Class clazz : classes)
 {
 C component = processor.createComponent(clazz);
 if (component != null)
 components.add(component);
 }
 }

 // from attachments
 Annotation annotation = unit.getAttachment(processor.getAnnotation());
 if (annotation != null)
 {
 C component = processor.createComponent(annotation);
 if (component != null)
 components.add(component);
 }

 int i = 0;
 try
 {
 for (i = 0; i < components.size(); i++)
 {
 deployComponent(unit, processor, components.get(i));
 }
 }
 catch (Throwable t)
 {
 for (; i >= 0; i--)
 {
 try
 {
 compVisitor.undeploy(unit, components.get(i));
 }
 catch (Throwable ignored)
 {
 }
 }
 throw DeploymentException.rethrowAsDeploymentException("Error deploying: " + unit.getName(), t);
 }
 }
 }
 catch (Throwable t)
 {
 super.internalUndeploy(unit);
 throw DeploymentException.rethrowAsDeploymentException("Error deploying: " + unit.getName(), t);
 }
 }
 }

 protected void deployComponent(DeploymentUnit unit, AnnotationProcessor<?, C> processor, C component) throws DeploymentException
 {
 String name = processor.getComponentName(component);
 DeploymentUnit componentUnit = unit.getComponent(name);
 C previous = null;
 if (componentUnit != null)
 previous = componentUnit.getAttachment(getOutput());

 if (previous != null)
 {
 processor.merge(previous, component);
 }
 else if (processor.isValidComponent(component))
 {
 compVisitor.deploy(unit, component);
 }
 }

 public void internalUndeploy(DeploymentUnit unit)
 {
 if (processors != null && compVisitor != null)
 {
 try
 {
 AnnotationEnvironment env = unit.getAttachment(AnnotationEnvironment.class);
 for (AnnotationProcessor<?, C> processor : processors)
 {
 List<C> components = new ArrayList<C>();

 if (env != null)
 {
 // from classes
 Set<Class<?>> classes = env.classIsAnnotatedWith(processor.getAnnotation());
 for (Class clazz : classes)
 {
 C component = processor.createUndeployableComponent(clazz);
 if (component != null)
 components.add(component);
 }
 }

 // from attachments
 Annotation annotation = unit.getAttachment(processor.getAnnotation());
 if (annotation != null)
 {
 C component = processor.createUndeployableComponent(annotation);
 if (component != null)
 components.add(component);
 }

 for (int i = components.size() - 1; i >= 0; i--)
 {
 try
 {
 compVisitor.undeploy(unit, components.get(i));
 }
 catch (Throwable ignored)
 {
 log.warn("Error during undeploy: " + unit.getName(), ignored);
 }
 }
 }
 }
 catch (Throwable ignored)
 {
 log.warn("Error during undeploy: " + unit.getName(), ignored);
 }
 }

 super.internalUndeploy(unit);
 }
}

The two issues that I see here are
- I'm not able to differ between previous (merged) component while undeploying
- I have to re-create component for undeploy (that's why I use that createUndeployableComponent method)

Does this somehow fit into the picture you have/had?

[adrian.brock]

"alesj" wrote:

Does this somehow fit into the picture you have/had?

No not really.

I wanted you to seperate the annotation scanning from the component creation.
i.e the annotation @Bean or a "collection" of them @Beans?
becomes an attachment in its own right.

The component deployer should not care where the annotation(s) come from.
One possible source is the annoation scanning.

Schematically:
BeanAnnotationScanner: class scan -> annotation attachments
Component deployer: metadata or annotations -> BeanMetaData components

That way if I dont want annotation scanning for beans or I want to use
some other logic I can remove, replace or insert a new deployer to do what I want.

e.g. Hypothetically: suppose I want all beans scanned from classes to be "on-demand"
I can add a deployer inbetween that modifies the annotations but doesn't touch
those created using normal metadata.

Like I said before, I don't really think anybody will do this kind of thing,
but we should do it properly just in case.

Also it keeps the processing to "TO DO ONE THING WELL"
i.e. one deployer knows how to scan for bean annotations
one knows how to create components from a set of such annotations
(mod those that already have metadata).

[adrian.brock]

I wanted you to seperate the annotation scanning from the component creation.
i.e the annotation @Bean or a "collection" of them @Beans?
becomes an attachment in its own right.

The attachment doesn't actually have to be the annotation if you need other
information from the class that is annotated. e.g. It could be a collection of classes.

[alesj]

The attachment doesn't actually have to be the annotation if you need other
information from the class that is annotated. e.g. It could be a collection of classes.

Yup, exactly what I wanted to point out.
Since in my @Bean case, I need the actual class that is annoatted.
Otherwise there is no point in having @Bean if you could define the class there. ;-)