Need cleanup of the attachments api

[starksm64]

As I have more metadata being stored off, its clear that the DeploymentUnit api is not sufficient/confusing. There are non-managed, non-serializable attachments showing up in the only used attachments contents. The current attachments api breakdown is:

DeploymentUnit
.addAttachment
.getTransientManagedObjects().addAttachment
.getAttachment
.getTransientManagedObjects().getAttachment

DeploymentContext
.getPredeterminedManagedObjects().addAttachment/getAttachment
.getTransientManagedObjects().addAttachment/getAttachment
.getTransientAttachments().addAttachment/getAttachment

Right now when one does a DeploymentUnit.addAttachment this is going into the DeploymentContext.getTransientAttachments() contents. When one does a DeploymentUnit.getAttachment the DeploymentContext attachments are checked for a match in the order: getPredeterminedManagedObjects(), getTransientManagedObjects(), and getTransientAttachments().

The notion of the various attachments as I understand it and discussed elsewhere is:
getPredeterminedManagedObjects() - attachment overrides for use by things like the profileservice.
getTransientManagedObjects() - attachment overrides for use by things like the runtime aspects.
getTransientAttachments() - the general runtime attachments bucket for use by deployers, aspects.

The TransientManagedObjects is currently unused, and everything is going into TransientAttachments. This is not a sufficient separation of what is managed metadata that should be persisted vs metadata that is used for inter deployer/aspect communication.

I suggest the following change to cleanup the usage and allow for true transient/non-managed attachments:

1. Remove the Attachments interface from DeploymentUnit.
2. Mirror the Attachments api in DeploymentUnit with an enum that indicates what bucket is being accessed. The non-enum version of getAttachment follows the existing order of accessing the PredeterminedManagedObjects, TransientManagedObjects and TransientAttachments. The non-enum version of addAttachment would populate the TransientManagedObjects contents.
3. To populate a non-managed attachment, one would have to use the call that takes the enum targetting the TransientAttachments list.
4. Drop the getTransientManagedObjects access from DeploymentUnit.

[bill.burke]

Good cleanup ideas. I always avoided using DeploymentUnit interface for finding attachments as I didn't like how it merged parent and child attachment data in the lookup.

[adrian.brock]

I suggest the following change to cleanup the usage and allow for true transient/non-managed attachments:

1. Remove the Attachments interface from DeploymentUnit.
2. Mirror the Attachments api in DeploymentUnit with an enum that indicates what bucket is being accessed. The non-enum version of getAttachment follows the existing order of accessing the PredeterminedManagedObjects, TransientManagedObjects and TransientAttachments. The non-enum version of addAttachment would populate the TransientManagedObjects contents.
3. To populate a non-managed attachment, one would have to use the call that takes the enum targetting the TransientAttachments list.
4. Drop the getTransientManagedObjects access from DeploymentUnit.

I don't see the need for the enum?
Better would be like the DeploymentContext, i.e.
have a getTransientManagedObjects() and getTransientAttachments();

You can leave the old methods in place (deprecate them?)
and populate the TransientManagedObjects like you suggest.

[adrian.brock]

Good cleanup ideas. I always avoided using DeploymentUnit interface for finding attachments as I didn't like how it merged parent and child attachment data in the lookup.

That's a bug we identified a while ago.
It should only be doing the merge for components.

[starksm64]

I don't see the need for the enum?
Better would be like the DeploymentContext, i.e.
have a getTransientManagedObjects() and getTransientAttachments();

You can leave the old methods in place (deprecate them?)
and populate the TransientManagedObjects like you suggest.

Enum vs explicit accessors is not a big deal.

[bill.burke]

Enums are only useful if we'll need to expand the types of attachments. This probably won't happen right? Specific methods make it easier to autocomplete what you want in IDEs.

[starksm64]

Good cleanup ideas. I always avoided using DeploymentUnit interface for finding attachments as I didn't like how it merged parent and child attachment data in the lookup.

That's a bug we identified a while ago.
It should only be doing the merge for components.

What was that discussion? If I'm going to be updating this I want to get it all changed for 2.0.0.Beta3.

[starksm64]

Enums are only useful if we'll need to expand the types of attachments. This probably won't happen right? Specific methods make it easier to autocomplete what you want in IDEs.

Right.

[adrian.brock]

Good cleanup ideas. I always avoided using DeploymentUnit interface for finding attachments as I didn't like how it merged parent and child attachment data in the lookup.

That's a bug we identified a while ago.
It should only be doing the merge for components.

What was that discussion? If I'm going to be updating this I want to get it all changed for 2.0.0.Beta3.

It was before Christmas sometime.

I looks like it might already have been fixed?

AbstractDeploymentUnit:
 public Map<String, Object> getAttachments()
 {
 DeploymentContext parent = deploymentContext.getParent();

 // HERE Don't look at the parent when we are not a component
 if (deploymentContext.isComponent() == false)
 parent = null;

 HashMap<String, Object> result = new HashMap<String, Object>();
 if (parent != null)
 result.putAll(parent.getTransientAttachments().getAttachments());

Or maybe the fix missed part of the api?

[starksm64]

That change was way back in Sep. So when requesting an attachment from a component it will pickup its parent. Is this ok or do we need a visitor to allow this to be up to the caller?

I know there are circumstances in which one does need to look across DeploymentUnits and I did add a parent accessor to DeploymentUnit for this.

[adrian.brock]

In most cases, the Component just overrides one piece of metadata
(and usually it is not an override)
e.g. the MC components "overrides" the BeanMetaData.

I think it would be pretty annoying if the deployer needs to always code

Blah result = unit.getAttachment(Blah.class);
if (result == null && unit.isComponent())
 result = unit.getParent().getAtttachment(Blah.class);

IIRC. The case where Bill came across it originally was because he got an invocation
of his deployer for every component when he just wanted to do it once
per deployment (it was after the component deployers).

I told him to add a check:

if (unit.isComponent())
 return;

We might want to add better support for deployers that aren't interested in components?