10 Replies Latest reply on Feb 23, 2007 1:54 PM by Adrian Brock

    Need cleanup of the attachments api

    Scott Stark Master

      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.

        • 1. Re: Need cleanup of the attachments api
          Bill Burke Master

          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.

          • 2. Re: Need cleanup of the attachments api
            Adrian Brock Master

             

            "scott.stark@jboss.org" wrote:

            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.

            • 3. Re: Need cleanup of the attachments api
              Adrian Brock Master

               

              "bill.burke@jboss.com" wrote:
              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.

              • 4. Re: Need cleanup of the attachments api
                Scott Stark Master

                 

                "adrian@jboss.org" wrote:

                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.


                • 5. Re: Need cleanup of the attachments api
                  Bill Burke Master

                  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.

                  • 6. Re: Need cleanup of the attachments api
                    Scott Stark Master

                     

                    "adrian@jboss.org" wrote:
                    "bill.burke@jboss.com" wrote:
                    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.


                    • 7. Re: Need cleanup of the attachments api
                      Scott Stark Master

                       

                      "bill.burke@jboss.com" wrote:
                      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.


                      • 8. Re: Need cleanup of the attachments api
                        Adrian Brock Master

                         

                        "scott.stark@jboss.org" wrote:
                        "adrian@jboss.org" wrote:
                        "bill.burke@jboss.com" wrote:
                        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?

                        • 9. Re: Need cleanup of the attachments api
                          Scott Stark Master

                          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.

                          • 10. Re: Need cleanup of the attachments api
                            Adrian Brock Master

                            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?