Improve CompositeImpl.clone so that it clones both included and nested composites

Move the CompositeCloneBuilder to be called before the compositeIncludeBuilder so that the include builder doesn't have to mix with clone

git-svn-id: http://svn.us.apache.org/repos/asf/tuscany@818657 13f79535-47bb-0310-9956-ffa450edef68

8 files changed, 106 insertions, 198 deletions

diff --git a/java/sca/modules/assembly/src/main/java/org/apache/tuscany/sca/assembly/builder/impl/CompositeBuilderImpl.java b/java/sca/modules/assembly/src/main/java/org/apache/tuscany/sca/assembly/builder/impl/CompositeBuilderImpl.java
index 0e5bd2edb7..5fa9c9db17 100644
--- a/java/sca/modules/assembly/src/main/java/org/apache/tuscany/sca/assembly/builder/impl/CompositeBuilderImpl.java
+++ b/java/sca/modules/assembly/src/main/java/org/apache/tuscany/sca/assembly/builder/impl/CompositeBuilderImpl.java
@@ -115,12 +115,12 @@ public class CompositeBuilderImpl implements CompositeBuilder, DeployedComposite
 
         try {
 
+            // Clone nested and included  composites
+            composite = compositeCloneBuilder.build(composite, definitions, monitor);
+
             // Collect and fuse includes
             composite = compositeIncludeBuilder.build(composite, definitions, monitor);
 
-            // Expand nested composites
-            composite = compositeCloneBuilder.build(composite, definitions, monitor);
-
             // Configure all components
             composite = componentConfigurationBuilder.build(composite, definitions, monitor);
 
diff --git a/java/sca/modules/assembly/src/main/java/org/apache/tuscany/sca/assembly/builder/impl/CompositeCloneBuilderImpl.java b/java/sca/modules/assembly/src/main/java/org/apache/tuscany/sca/assembly/builder/impl/CompositeCloneBuilderImpl.java
index 07c1c0c7fc..808ecf1993 100644
--- a/java/sca/modules/assembly/src/main/java/org/apache/tuscany/sca/assembly/builder/impl/CompositeCloneBuilderImpl.java
+++ b/java/sca/modules/assembly/src/main/java/org/apache/tuscany/sca/assembly/builder/impl/CompositeCloneBuilderImpl.java
@@ -20,7 +20,6 @@
 package org.apache.tuscany.sca.assembly.builder.impl;
 
 import java.util.ArrayList;
-import java.util.Iterator;
 import java.util.List;
 
 import org.apache.tuscany.sca.assembly.Component;
@@ -43,7 +42,19 @@ public class CompositeCloneBuilderImpl implements CompositeBuilder {
 
     public Composite build(Composite composite, Definitions definitions, Monitor monitor)
         throws CompositeBuilderException {
-        expandCompositeImplementations(composite);
+        // Clone the includes 
+        List<Composite> includes = new ArrayList<Composite>();
+        for (Composite included : composite.getIncludes()) {
+            try {
+                includes.add((Composite)included.clone());
+            } catch (CloneNotSupportedException e) {
+                throw new UnsupportedOperationException(e);
+            }
+        }
+        composite.getIncludes().clear();
+        composite.getIncludes().addAll(includes);
+
+        cloneCompositeImplementations(composite);
         return composite;
     }
 
@@ -52,74 +63,26 @@ public class CompositeCloneBuilderImpl implements CompositeBuilder {
     }
 
     /**
-     * Expand composite component implementations.
+     * Clone composite component implementations
      * 
      * @param composite
      * @param problems
      */
-    private void expandCompositeImplementations(Composite composite) {
+    private void cloneCompositeImplementations(Composite composite) {
         for (Component component : composite.getComponents()) {
             Implementation implementation = component.getImplementation();
             if (implementation instanceof Composite) {
 
                 Composite compositeImplementation = (Composite)implementation;
-                Composite clone;
                 try {
-                    clone = (Composite)compositeImplementation.clone();
+                    // Please note the clone method is recursive
+                    Composite clone = (Composite)compositeImplementation.clone();
+                    component.setImplementation(clone);
                 } catch (CloneNotSupportedException e) {
-                    throw new RuntimeException(e);
-                }
-                component.setImplementation(clone);
-                expandCompositeImplementations(clone);
-            }
-        }
-    }
-
-    /**
-     * Collect all nested composite implementations in a graph of composites.
-     * 
-     * @param composite
-     * @param nested
-     */
-    private void collectNestedComposites(Composite composite, List<Composite> nested) {
-        for (Component component : composite.getComponents()) {
-            Implementation implementation = component.getImplementation();
-            if (implementation instanceof Composite) {
-                Composite nestedComposite = (Composite)implementation;
-                nested.add(nestedComposite);
-                collectNestedComposites(nestedComposite, nested);
-            }
-        }
-    }
-
-    /**
-     * Fuse nested composites into a top level composite.
-     * 
-     * @param composite
-     */
-    private void fuseCompositeImplementations(Composite composite) {
-
-        // First collect all nested composites
-        List<Composite> nested = new ArrayList<Composite>();
-        collectNestedComposites(composite, nested);
-
-        // Then add all the non-composite components they contain 
-        for (Composite nestedComposite : nested) {
-            for (Component component : nestedComposite.getComponents()) {
-                Implementation implementation = component.getImplementation();
-                if (!(implementation instanceof Composite)) {
-                    composite.getComponents().add(component);
+                    throw new UnsupportedOperationException(e);
                 }
             }
         }
-
-        // Clear the initial list of composite components
-        for (Iterator<Component> i = composite.getComponents().iterator(); i.hasNext();) {
-            Component component = i.next();
-            if (component.getImplementation() instanceof Composite) {
-                i.remove();
-            }
-        }
     }
 
 }
diff --git a/java/sca/modules/assembly/src/main/java/org/apache/tuscany/sca/assembly/builder/impl/CompositeIncludeBuilderImpl.java b/java/sca/modules/assembly/src/main/java/org/apache/tuscany/sca/assembly/builder/impl/CompositeIncludeBuilderImpl.java
index 52dd769f8a..2a28c48905 100644
--- a/java/sca/modules/assembly/src/main/java/org/apache/tuscany/sca/assembly/builder/impl/CompositeIncludeBuilderImpl.java
+++ b/java/sca/modules/assembly/src/main/java/org/apache/tuscany/sca/assembly/builder/impl/CompositeIncludeBuilderImpl.java
@@ -19,9 +19,6 @@
 
 package org.apache.tuscany.sca.assembly.builder.impl;
 
-import java.util.HashSet;
-import java.util.Set;
-
 import org.apache.tuscany.sca.assembly.Component;
 import org.apache.tuscany.sca.assembly.Composite;
 import org.apache.tuscany.sca.assembly.Implementation;
@@ -48,151 +45,81 @@ public class CompositeIncludeBuilderImpl implements CompositeBuilder {
 
     public Composite build(Composite composite, Definitions definitions, Monitor monitor)
         throws CompositeBuilderException {
-        fuseIncludes(composite, monitor);
-        return composite;
+        return processIncludes(composite, monitor);
     }
 
     /**
-     * Copy a list of includes into a composite.
-     * 
+     * Merge the elements from the included composites into the including composite
      * @param composite
+     * @param monitor
+     * @return
      */
-    private void fuseIncludes(Composite composite, Monitor monitor) {
-
+    private Composite processIncludes(Composite composite, Monitor monitor) {
         monitor.pushContext("Composite: " + composite.getName().toString());
 
         try {
-            Set<Composite> visited = new HashSet<Composite>();
-            visited.add(composite);
+            // process any composites referenced through implementation.composite 
+            for (Component component : composite.getComponents()) {
+
+                // recurse for composite implementations
+                Implementation implementation = component.getImplementation();
+                if (implementation instanceof Composite) {
+                    processIncludes((Composite)implementation, monitor);
+                }
+            }
 
+            // get the components etc. from any included composites
             for (Composite included : composite.getIncludes()) {
                 if (included.isLocal() && !composite.isLocal()) {
                     // ASM60041
                     Monitor.error(monitor, this, "assembly-validation-messages", "IllegalCompositeIncusion", composite
                         .getName().toString(), included.getName().toString());
+                    return null;
                 }
-                Composite fusedComposite = fuseInclude(included, visited, monitor);
-                if (fusedComposite != null) {
-                    composite.getComponents().addAll(fusedComposite.getComponents());
-                    composite.getServices().addAll(fusedComposite.getServices());
-                    composite.getReferences().addAll(fusedComposite.getReferences());
-                    composite.getProperties().addAll(fusedComposite.getProperties());
-                    composite.getWires().addAll(fusedComposite.getWires());
-                }
-            }
 
-            // Clear the list of includes as all of the included components 
-            // have now been added into the top level composite
-            composite.getIncludes().clear();
+                // The included has been cloned during composite.clone()
+                Composite merged = processIncludes(included, monitor);
+                if (merged != null) {
+                    for (Component component : merged.getComponents()) {
+                        // apply the autowire flag on this composite to any inline 
+                        // components - Assembly 5.6 point 4
+                        if (component.getAutowire() == null && merged.getAutowire() == Boolean.TRUE) {
+                            component.setAutowire(Boolean.TRUE);
+                        }
+                        // Merge the intents and policySets from the included composite into 
+                        // component/service/reference elements under the composite
+                        component.getRequiredIntents().addAll(merged.getRequiredIntents());
+                        component.getPolicySets().addAll(merged.getPolicySets());
+                    }
 
-            // process any composites referenced through implementation.composite 
-            for (Component component : composite.getComponents()) {
-                monitor.pushContext("Component: " + component.getName());
+                    for (Service service : merged.getServices()) {
+                        service.getRequiredIntents().addAll(merged.getRequiredIntents());
+                        service.getPolicySets().addAll(merged.getPolicySets());
+                    }
 
-                try {
-                    // recurse for composite implementations
-                    Implementation implementation = component.getImplementation();
-                    if (implementation instanceof Composite) {
-                        fuseIncludes((Composite)implementation, monitor);
+                    for (Reference reference : merged.getReferences()) {
+                        reference.getRequiredIntents().addAll(merged.getRequiredIntents());
+                        reference.getPolicySets().addAll(merged.getPolicySets());
                     }
-                } finally {
-                    monitor.popContext();
+                    composite.getComponents().addAll(merged.getComponents());
+                    composite.getServices().addAll(merged.getServices());
+                    composite.getReferences().addAll(merged.getReferences());
+                    composite.getProperties().addAll(merged.getProperties());
+                    composite.getWires().addAll(merged.getWires());
+                    // FIXME: What should we do for the extensions
+                    /*
+                    clone.getExtensions().addAll(fusedComposite.getExtensions());
+                    clone.getAttributeExtensions().addAll(fusedComposite.getAttributeExtensions());
+                    */
                 }
             }
 
+            composite.getIncludes().clear();
+
+            // return the fused composite we have built up so far
+            return composite;
         } finally {
             monitor.popContext();
         }
     }
-
-    private Composite fuseInclude(Composite include, Set<Composite> visited, Monitor monitor) {
-
-        if (visited.contains(include)) {
-            // FIXME: [rfeng] Do we need to have a warning? I think it is fine to have Composite A 
-            // include B and C while both B and C include D.
-            Monitor.warning(monitor, this, "assembly-validation-messages", "CompositeAlreadyIncluded", include
-                .getName().toString());
-            return null;
-        }
-
-        visited.add(include);
-
-        Composite clone;
-        try {
-            clone = (Composite)include.clone();
-        } catch (CloneNotSupportedException e) {
-            throw new RuntimeException(e);
-        }
-
-        // get the components etc. from any included composites
-        for (Composite included : include.getIncludes()) {
-            if (included.isLocal() && !include.isLocal()) {
-                // ASM60041
-                Monitor.error(monitor, this, "assembly-validation-messages", "IllegalCompositeIncusion", include
-                    .getName().toString(), included.getName().toString());
-                return null;
-            }
-
-            Composite fusedComposite = fuseInclude(included, visited, monitor);
-            if (fusedComposite != null) {
-                clone.getComponents().addAll(fusedComposite.getComponents());
-                clone.getServices().addAll(fusedComposite.getServices());
-                clone.getReferences().addAll(fusedComposite.getReferences());
-                clone.getProperties().addAll(fusedComposite.getProperties());
-                clone.getWires().addAll(fusedComposite.getWires());
-                // FIXME: What should we do for the extensions
-                /*
-                clone.getExtensions().addAll(fusedComposite.getExtensions());
-                clone.getAttributeExtensions().addAll(fusedComposite.getAttributeExtensions());
-                */
-            }
-        }
-
-        // apply the autowire flag on this composite to any inline 
-        // components - Assembly 5.6 point 4
-        if (include.getAutowire() == Boolean.TRUE) {
-            for (Component component : clone.getComponents()) {
-                if (component.getAutowire() == null) {
-                    component.setAutowire(Boolean.TRUE);
-                }
-            }
-        }
-
-        // Merge the intents and policySets from the included composite into 
-        // component/service/reference elements under the composite
-        for (Component component : clone.getComponents()) {
-            component.getRequiredIntents().addAll(include.getRequiredIntents());
-            component.getPolicySets().addAll(include.getPolicySets());
-        }
-
-        for (Service service : clone.getServices()) {
-            service.getRequiredIntents().addAll(include.getRequiredIntents());
-            service.getPolicySets().addAll(include.getPolicySets());
-        }
-
-        for (Reference reference : clone.getReferences()) {
-            reference.getRequiredIntents().addAll(include.getRequiredIntents());
-            reference.getPolicySets().addAll(include.getPolicySets());
-        }
-
-        // process any composites referenced through implementation.composite 
-        for (Component component : clone.getComponents()) {
-
-            // recurse for composite implementations
-            Implementation implementation = component.getImplementation();
-            if (implementation instanceof Composite) {
-                try {
-                    monitor.pushContext("Component: " + component.getName());
-                    fuseIncludes((Composite)implementation, monitor);
-                } finally {
-                    monitor.popContext();
-                }
-            }
-        }
-
-        clone.getIncludes().clear();
-
-        // return the fused composite we have built up so far
-        return clone;
-    }
 }
diff --git a/java/sca/modules/assembly/src/main/java/org/apache/tuscany/sca/assembly/builder/impl/ModelBuilderImpl.java b/java/sca/modules/assembly/src/main/java/org/apache/tuscany/sca/assembly/builder/impl/ModelBuilderImpl.java
index c7693e135e..99c5947ab3 100644
--- a/java/sca/modules/assembly/src/main/java/org/apache/tuscany/sca/assembly/builder/impl/ModelBuilderImpl.java
+++ b/java/sca/modules/assembly/src/main/java/org/apache/tuscany/sca/assembly/builder/impl/ModelBuilderImpl.java
@@ -88,6 +88,9 @@ public class ModelBuilderImpl implements CompositeBuilder, DeployedCompositeBuil
                            Monitor monitor) throws CompositeBuilderException {
 
         try {
+            // Clone the composites that are included or referenced in implementation.composite
+            composite = compositeCloneBuilder.build(composite, definitions, monitor);
+
             // Collect and fuse includes. Copy all of the components
             // out of the included composite into the including composite
             // and discards the included composite
@@ -95,10 +98,6 @@ public class ModelBuilderImpl implements CompositeBuilder, DeployedCompositeBuil
 
             // need to apply policy external attachment
 
-            // Expand nested composites. Clone any composite model that
-            // is acting as a component implementation and connects the cloned
-            // model to the component implementation in question
-            composite = compositeCloneBuilder.build(composite, definitions, monitor);
 
             // Process the implementation hierarchy by calculating the component type 
             // for the top level implementation (composite). This has the effect of
diff --git a/java/sca/modules/assembly/src/main/java/org/apache/tuscany/sca/assembly/impl/ComponentImpl.java b/java/sca/modules/assembly/src/main/java/org/apache/tuscany/sca/assembly/impl/ComponentImpl.java
index 9b4db8a8e0..0227ab3c65 100644
--- a/java/sca/modules/assembly/src/main/java/org/apache/tuscany/sca/assembly/impl/ComponentImpl.java
+++ b/java/sca/modules/assembly/src/main/java/org/apache/tuscany/sca/assembly/impl/ComponentImpl.java
@@ -25,6 +25,7 @@ import org.apache.tuscany.sca.assembly.Component;
 import org.apache.tuscany.sca.assembly.ComponentProperty;
 import org.apache.tuscany.sca.assembly.ComponentReference;
 import org.apache.tuscany.sca.assembly.ComponentService;
+import org.apache.tuscany.sca.assembly.Composite;
 import org.apache.tuscany.sca.assembly.ConstrainingType;
 import org.apache.tuscany.sca.assembly.Implementation;
 import org.apache.tuscany.sca.assembly.Property;
@@ -74,6 +75,16 @@ public class ComponentImpl extends ExtensibleImpl implements Component, Cloneabl
         for (ComponentService service : getServices()) {
             clone.services.add((ComponentService)service.clone());
         }
+        
+        // Clone the implementation.composite
+        if(implementation instanceof Composite) {
+            clone.implementation = (Composite) ((Composite) implementation).clone();
+        }
+        
+        // Clone the Lists for intents and policySets
+        clone.requiredIntents = new ArrayList<Intent>(getRequiredIntents());
+        clone.policySets = new ArrayList<PolicySet>(getPolicySets());
+        
         return clone;
     }
 
diff --git a/java/sca/modules/assembly/src/main/java/org/apache/tuscany/sca/assembly/impl/CompositeImpl.java b/java/sca/modules/assembly/src/main/java/org/apache/tuscany/sca/assembly/impl/CompositeImpl.java
index e93d5ebc25..440bd340fc 100644
--- a/java/sca/modules/assembly/src/main/java/org/apache/tuscany/sca/assembly/impl/CompositeImpl.java
+++ b/java/sca/modules/assembly/src/main/java/org/apache/tuscany/sca/assembly/impl/CompositeImpl.java
@@ -89,6 +89,12 @@ public class CompositeImpl extends ImplementationImpl implements Composite, Clon
         for (Wire wire : getWires()) {
             clone.wires.add((Wire)wire.clone());
         }
+        
+        // Clone the includes 
+        clone.includes = new ArrayList<Composite>(includes);
+        for (Composite included : getIncludes()) {
+            clone.includes.add((Composite)included.clone());
+        }
         return clone;
     }
 
diff --git a/java/sca/modules/node-impl/src/main/java/org/apache/tuscany/sca/node/impl/NodeFactoryImpl.java b/java/sca/modules/node-impl/src/main/java/org/apache/tuscany/sca/node/impl/NodeFactoryImpl.java
index b9d1f7d2c2..54277a5fc6 100644
--- a/java/sca/modules/node-impl/src/main/java/org/apache/tuscany/sca/node/impl/NodeFactoryImpl.java
+++ b/java/sca/modules/node-impl/src/main/java/org/apache/tuscany/sca/node/impl/NodeFactoryImpl.java
@@ -52,6 +52,7 @@ import javax.xml.stream.XMLStreamException;
 import javax.xml.stream.XMLStreamReader;
 
 import org.apache.tuscany.sca.assembly.AssemblyFactory;
+import org.apache.tuscany.sca.assembly.Base;
 import org.apache.tuscany.sca.assembly.Composite;
 import org.apache.tuscany.sca.assembly.builder.BuilderExtensionPoint;
 import org.apache.tuscany.sca.assembly.builder.CompositeBuilder;
@@ -539,14 +540,14 @@ public class NodeFactoryImpl extends NodeFactory {
 
         // Create a top level composite to host our composite
         // This is temporary to make the activator happy
-        Composite tempComposite = assemblyFactory.createComposite();
-        tempComposite.setName(new QName(SCA11_TUSCANY_NS, "_tempComposite"));
-        tempComposite.setURI(SCA11_TUSCANY_NS);
+        Composite domainComposite = assemblyFactory.createComposite();
+        domainComposite.setName(new QName(Base.SCA11_NS, ""));
+        domainComposite.setURI(Base.SCA11_NS);
 
         for (Contribution contribution : contributions) {
             for (Composite composite : contribution.getDeployables()) {
                 // Include the node composite in the top-level composite
-                tempComposite.getIncludes().add(composite);
+                domainComposite.getIncludes().add(composite);
                 logger.log(Level.INFO, "Adding composite: " + composite.getName() + " to domain " + getDomainURI());
             }
         }
@@ -559,13 +560,13 @@ public class NodeFactoryImpl extends NodeFactory {
         }
 
         // build the top level composite
-        ((DeployedCompositeBuilder)compositeBuilder).build(tempComposite, systemDefinitions, bindingMap, monitor);
+        ((DeployedCompositeBuilder)compositeBuilder).build(domainComposite, systemDefinitions, bindingMap, monitor);
         analyzeProblems();
 
-        endpointReferenceBuilder.buildtimeBuild(tempComposite);
+        endpointReferenceBuilder.buildtimeBuild(domainComposite);
         analyzeProblems();
 
-        return tempComposite;
+        return domainComposite;
     }
 
     private List<Contribution> loadContributions(NodeConfiguration configuration) throws MalformedURLException,
diff --git a/java/sca/modules/policy-builder/src/test/java/org/apache/tuscany/sca/policy/builder/impl/PolicyAttachmentTestCase.java b/java/sca/modules/policy-builder/src/test/java/org/apache/tuscany/sca/policy/builder/impl/PolicyAttachmentTestCase.java
index b7ea3d8fa2..5c66f44b9c 100644
--- a/java/sca/modules/policy-builder/src/test/java/org/apache/tuscany/sca/policy/builder/impl/PolicyAttachmentTestCase.java
+++ b/java/sca/modules/policy-builder/src/test/java/org/apache/tuscany/sca/policy/builder/impl/PolicyAttachmentTestCase.java
@@ -122,6 +122,7 @@ public class PolicyAttachmentTestCase {
 
         Composite domainComposite = assemblyFactory.createComposite();
         domainComposite.setName(new QName(Base.SCA11_NS, ""));
+        domainComposite.setLocal(false);
         domainComposite.getIncludes().add(composite1);
         domainComposite.getIncludes().add(composite2);
 
@@ -136,12 +137,12 @@ public class PolicyAttachmentTestCase {
             builders.getCompositeBuilder("org.apache.tuscany.sca.assembly.builder.CompositeCloneBuilder");
             */
 
-        includeBuilder.build(domainComposite, definitions, monitor);
-        cloneBuilder.build(domainComposite, definitions, monitor);
-        uriBuilder.build(domainComposite, definitions, monitor);
+        domainComposite = cloneBuilder.build(domainComposite, definitions, monitor);
+        domainComposite = includeBuilder.build(domainComposite, definitions, monitor);
+        domainComposite = uriBuilder.build(domainComposite, definitions, monitor);
 
         PolicyAttachmentBuilderImpl builder = new PolicyAttachmentBuilderImpl(extensionPoints);
-        builder.build(domainComposite, definitions, monitor);
+        domainComposite = builder.build(domainComposite, definitions, monitor);
 
     }