Annotation processing

[adrian.brock]

While I was doing the scoping changes I broke the annotation processing
and then had no idea what was going on.
This processing has no error messages so you can't understand it
if it doesn't work as you expect.

I added some simple trace logging so you can at least debug which
annotations are (not) being applied without having to attach a debugger. :-)

Also the BasicBeanAdapter and related classes need fixing such that they use the
MetaData not the MetaDataRetrieval.

The same is true of its usage in the Scoping.

In fact, the getMetaDataRetrieval() on the kernel repository should be removed.

It's only real use at the moment is in AOP to find out if there are instance annotations.
That processing is wrong anyway. There should be a
MetaData.hasInstanceMetaData()
instead.

[alesj]

While I was doing the scoping changes I broke the annotation processing
and then had no idea what was going on.
This processing has no error messages so you can't understand it
if it doesn't work as you expect.

I added some simple trace logging so you can at least debug which
annotations are (not) being applied without having to attach a debugger. :-)

I'll have a look.
Since I still have to add javadocs and definitely improve logging.
But using debugger sounds like fun. :-)

Also the BasicBeanAdapter and related classes need fixing such that they use the MetaData not the MetaDataRetrieval.

The same is true of its usage in the Scoping.

MetaData has all the required functionality - as MetaDataRetrieval?

Scoping as in PreInstall stuff that we did?

In fact, the getMetaDataRetrieval() on the kernel repository should be removed.

It's only real use at the moment is in AOP to find out if there are instance annotations.
That processing is wrong anyway. There should be a
MetaData.hasInstanceMetaData()
instead.

Once I'm done, I'll try to remove it.

[alesj]

It's only real use at the moment is in AOP to find out if there are instance annotations.
That processing is wrong anyway. There should be a
MetaData.hasInstanceMetaData() instead.

What's the difference between local and non-local retrievers (from MetaDataContext):

 /**
 * Get the retrievals
 *
 * @return the retrievals
 */
 List<MetaDataRetrieval> getRetrievals();

 /**
 * Get the local retrievals
 *
 * @return the local retrievals
 */
 List<MetaDataRetrieval> getLocalRetrievals();

I guess this needs removal as well (from KernelControllerContextAction):

 if (joinpoint instanceof KernelControllerContextAware)
 ((KernelControllerContextAware) joinpoint).setKernelControllerContext(context);
...
 if (joinpoint instanceof KernelControllerContextAware)
 ((KernelControllerContextAware) joinpoint).unsetKernelControllerContext(null);

Since all information for AOPConstructorJoinpoint should then be in MetaData instance taken from the stack?

Any quick tips on how MetaData.hasInstanceMetaData() should look like?
Should it still use MetaDataContext as current impl in AOPConstructorJoinpoint does?

[alesj]

"alesj" wrote:

Also the BasicBeanAdapter and related classes need fixing such that they use the MetaData not the MetaDataRetrieval.

The same is true of its usage in the Scoping.

MetaData has all the required functionality - as MetaDataRetrieval?

Scoping as in PreInstall stuff that we did?

Yup, it was easy to move annotations handling to plain MetaData lookup. :-)

I also (re)moved the part in PreInstallAction where MetaDataRetrieval was referenced from KernelMetaDataRepository.

But I guess this code cannot be changed to use plain MetaData since I _need_ to build the whole MetaDataRetrieval for the scope - similar to how we do it for ControllerContext's MetaData (hidden in BasicKernelMetaDataRepository).

 // find scoped controller
 MutableMetaDataRepository mmdr = repository.getMetaDataRepository();
 MetaDataRetrieval mdr = mmdr.getMetaDataRetrieval(scopeKey);
 if (mdr == null)
 {
 mdr = new MemoryMetaDataLoader(scopeKey);
 mmdr.addMetaDataRetrieval(mdr);
 }
 MetaDataItem<ScopedKernelController> controllerItem = mdr.retrieveMetaData(ScopedKernelController.class);
 ScopedKernelController scopedController;
 if (controllerItem != null)
 {
 scopedController = controllerItem.getValue();
 }
 else
 {
 AbstractController parentController = null;
 ScopeKey parentKey = scopeKey.getParent();
 while (parentController == null && parentKey != null)
 {
 MetaDataRetrieval pmdr = mmdr.getMetaDataRetrieval(parentKey);
 if (pmdr != null)
 {
 MetaDataItem<ScopedKernelController> pci = pmdr.retrieveMetaData(ScopedKernelController.class);
 if (pci != null)
 {
 parentController = pci.getValue();
 }
 }
 parentKey = parentKey.getParent();
 }
 if (parentController == null)
 {
 if (controller instanceof AbstractController == false)
 throw new IllegalArgumentException("Underlying controller not AbstractController instance!");
 parentController = (AbstractController) controller;
 }
 scopedController = new ScopedKernelController(controller.getKernel(), parentController);
 ((MutableMetaData)mdr).addMetaData(scopedController, ScopedKernelController.class);
 }
 scopedController.addControllerContext(context);
 }

[adrian.brock]

"alesj" wrote:

What's the difference between local and non-local retrievers (from MetaDataContext):

 /**
 * Get the retrievals
 *
 * @return the retrievals
 */
 List<MetaDataRetrieval> getRetrievals();

 /**
 * Get the local retrievals
 *
 * @return the local retrievals
 */
 List<MetaDataRetrieval> getLocalRetrievals();

A MetaDataContext is made of local retrievals and a parent context.
retrievals = local + parent retrievals.

Any quick tips on how MetaData.hasInstanceMetaData() should look like?
Should it still use MetaDataContext as current impl in AOPConstructorJoinpoint does?

Just move the AOP alogrithm into the MetaData/bridge.
i.e. see whether the backing MetaDataContext has an INSTANCE scope
and whether that scope has local annotations or metadata.

A more complete api would be to allow
MetaData.getScopeMetaData(ScopeKey)
which would produce a MetaData backed by the local retrievals of that scope
and add an isEmpty() to the MetaData api.

The hasInstanceMetaData() could then be implemented as:

MetaData instance = getScopedMetaData(instanceScope);
return instance.isEmpty() == false;

making it redundant.

But that might be a bit more complicated to implement with the caching, etc.?

[alesj]

and add an isEmpty() to the MetaData api.

What would mean that MetaData.isEmpty() == true?

This?

 public boolean isEmpty()
 {
 MetaDatasItem items = retrieval.retrieveMetaData();
 return items == null || items.getMetaDatas() == null || items.getMetaDatas().length == 0;
 }

[adrian.brock]

"alesj" wrote:

and add an isEmpty() to the MetaData api.

What would mean that MetaData.isEmpty() == true?

This?

 public boolean isEmpty()
 {
 MetaDatasItem items = retrieval.retrieveMetaData();
 return items == null || items.getMetaDatas() == null || items.getMetaDatas().length == 0;
 }

Yes, but I think you should propogate the isEmpty() through the api
such that it can be done in a more efficient manner where possible.
i.e. no need to construct the metadata array from the annotations and plain
metadata just to see if there are any.

[alesj]

Regarding MetaData.getScopeMetaData() I have this next drawback. :-)
Looks like introduction of MetaDataContext into MetaDataRetrievalToMetaDataBridge doesn't really fit:

 public MetaData getScopeMetaData(ScopeKey scopeKey)
 {
 if (retrieval instanceof MetaDataContext)
 {
 MetaDataContext context = (MetaDataContext)retrieval;
 ...
 }
 return null;
 }

Perhaps having a new MetaDataContextToMetaDataBridge would do the trick, but this, at a quick glance, looks like a big propagation through the metadata stuff ...

[adrian.brock]

If you want me to do it then I will, but don't ask to write all the code in the forum. :-)

As for your "drawback"

public interface MetaDataContext extends MetaDataRetrieval

AND

 /**
 * Create a new AbstractMetaDataContext.
 *
 * @param parent the parent
 * @param retrievals the retrievals
 */
 public AbstractMetaDataContext(MetaDataContext parent, List<MetaDataRetrieval> retrievals)

Since you need to reference ALL the local retrievals.

For the INSTANCE scope there's only probably one, but that certainly isn't true for a generic scope.

[adrian.brock]

If you mean it isn't a MetaDataContext in the first place,
that's not going to happen in practice for the real scoped metadata used by applications,
but if it were true then it would represent only be a single local scope.

So the code would be:

if (retrieval instanceof MetaDataContext && scopeFoundInContext())
 createBridgeForNewContextOfLocalRetrievals();
else if (singleRetrievalMatchesScope())
 createBridgeForRetrieval();
else
 // Not found
 return null;

[alesj]

If you want me to do it then I will, but don't ask to write all the code in the forum. :-)

I think this is the first attempt of somebody else then you writing anything to do with metadata. So I guess it's only fair to ask you before. :-)

public interface MetaDataContext extends MetaDataRetrieval

AND

 /**
 * Create a new AbstractMetaDataContext.
 *
 * @param parent the parent
 * @param retrievals the retrievals
 */
 public AbstractMetaDataContext(MetaDataContext parent, List<MetaDataRetrieval> retrievals)

Hmmm, I think I'm able to see the MetaDataContext extends MetaDataRetrieval. And looking at the impl's constructor. ;-)

If you mean it isn't a MetaDataContext in the first place,

No.
I mean that referencing MetaDataContext in MetaDataRetrievalToMetaDataBridge via 'instance of' is not what I would consider good OO. Specially with a name yelling out the purpose - Retrieval bridge ... not Context bridge. :-)

[adrian.brock]

"alesj" wrote:

No.
I mean that referencing MetaDataContext in MetaDataRetrievalToMetaDataBridge via 'instance of' is not what I would consider good OO. Specially with a name yelling out the purpose - Retrieval bridge ... not Context bridge. :-)

I agree, but the MetaData was never originally designed to retrieve structure
which is what MetaDataContext adds to the retrievals.

i.e. MetaDataRetrieval instanceof MetaDataContext == isRetrievalStructured()

The MetaData is actually designed to hide the structure so the client doesn't have to know
about it, i.e. it does not (need to) know what hierarchical scopes are linking retrievals together.

Its only this AOP requirement/optimization to ask if there is any instance metadata
(and only create a proxy if there is) that introduces the need to know structure.

Maybe we should leave it as the simple hasInstanceMetaData() on the MetaData api
and forget about revealing structure through an api that wasn't designed for it?
But you're still going to need to do the ugliness inside the bridge either way.

It's still better to have the ugliness hidden inside MetaData than
have AOP + others trying to play with things they almost certainly don't understand. :-)

I have similar thoughts about some of the code I did for the
ScopeInfo/ScopeBuilder which virtually duplicate the same code
to initialize non-existant scopes.
It would be much better to hide the ugliness inside the metadata api.

[alesj]

Maybe we should leave it as the simple hasInstanceMetaData() on the MetaData api
and forget about revealing structure through an api that wasn't designed for it?
But you're still going to need to do the ugliness inside the bridge either way.

OK, I'll then put the getScopeMetaData/isEmpty ugliness into RetrievalBridge. :-)

[alesj]

"alesj" wrote:

OK, I'll then put the getScopeMetaData/isEmpty ugliness into RetrievalBridge. :-)

 public MetaData getScopeMetaData(ScopeKey scopeKey)
 {
 if (scopeKey == null)
 throw new IllegalArgumentException("Null scope key");

 if (retrieval instanceof MetaDataContext)
 {
 MetaDataContext context = (MetaDataContext)retrieval;
 List<MetaDataRetrieval> matchingRetrievals = new ArrayList<MetaDataRetrieval>();
 List<MetaDataRetrieval> localRetrievals = context.getLocalRetrievals();
 for (MetaDataRetrieval localRetrieval : localRetrievals)
 {
 if (scopeKey.equals(localRetrieval.getScope()))
 matchingRetrievals.add(localRetrieval);
 }
 if (matchingRetrievals.isEmpty() == false)
 return new MetaDataRetrievalToMetaDataBridge(new AbstractMetaDataContext(context, matchingRetrievals));
 }
 else if (scopeKey.equals(retrieval.getScope()))
 return new MetaDataRetrievalToMetaDataBridge(new AbstractMetaDataContext(retrieval));
 return null;
 }

 public boolean isEmpty()
 {
 return retrieval.isEmpty();
 }

This is what I've pushed in (also added MetaDataRetrieval.isEmpty()). I think it should do the trick.

But if I want to remove the hack with joinpoint being ControllerContext aware, I need to get the context's name from somewhere.

 ScopeKey instanceKey = new ScopeKey(CommonLevels.INSTANCE, "<context_name>");
 MetaData instanceMetaData = metaData.getScopeMetaData(instanceKey);
 if (instanceMetaData != null && instanceMetaData.isEmpty() == false)
 {
 return true;
 }

Perhaps put in in the MetaData under 'ControllerContext.name' name?
And push in/out this info in KernelControllerContextAction.dispatchJoinPoint invocation?
Or just simply put in in for good when we first create MetaData for context - in PreInstallAction?

[alesj]

"alesj" wrote:

Perhaps put in in the MetaData under 'ControllerContext.name' name?
And push in/out this info in KernelControllerContextAction.dispatchJoinPoint invocation?
Or just simply put in in for good when we first create MetaData for context - in PreInstallAction?

Is doing this too hacky?

 protected void addMetaData(MutableMetaDataRepository repository, ControllerContext context, MemoryMetaDataLoader mutable)
 {
 mutable.addMetaData(ControllerContext.class.getSimpleName() + ".name", context.getName(), Object.class);
 }

 ...

 Object name = metaData.getMetaData("ControllerContext.name");
 if (name == null)
 return false;

 ScopeKey instanceKey = new ScopeKey(CommonLevels.INSTANCE, name.toString());

I'm passing all tests with this new getScopeMetaData/isEmpty impl. :-)