Author: bobtarling
Date: 2008-08-09 10:06:52-0700
New Revision: 15526
Modified:
trunk/src/argouml-core-model-mdr/src/org/argouml/model/mdr/CollaborationsHelperMDRImpl.java
trunk/src/argouml-core-model-mdr/src/org/argouml/model/mdr/CoreHelperMDRImpl.java
trunk/src/argouml-core-model-mdr/src/org/argouml/model/mdr/ModelManagementHelperMDRImpl.java
Log:
Issue 5288: Prevent constant loop when cyclic generalizations found. Includes some performance improvements.
Modified: trunk/src/argouml-core-model-mdr/src/org/argouml/model/mdr/CollaborationsHelperMDRImpl.java
Url: http://argouml.tigris.org/source/browse/argouml/trunk/src/argouml-core-model-mdr/src/org/argouml/model/mdr/CollaborationsHelperMDRImpl.java?view=diff&rev=15526&p1=trunk/src/argouml-core-model-mdr/src/org/argouml/model/mdr/CollaborationsHelperMDRImpl.java&p2=trunk/src/argouml-core-model-mdr/src/org/argouml/model/mdr/CollaborationsHelperMDRImpl.java&r1=15525&r2=15526
==============================================================================
--- trunk/src/argouml-core-model-mdr/src/org/argouml/model/mdr/CollaborationsHelperMDRImpl.java (original)
+++ trunk/src/argouml-core-model-mdr/src/org/argouml/model/mdr/CollaborationsHelperMDRImpl.java 2008-08-09 10:06:52-0700
@@ -34,6 +34,7 @@
import javax.jmi.reflect.InvalidObjectException;
+import org.apache.log4j.Logger;
import org.argouml.model.CollaborationsHelper;
import org.argouml.model.CoreHelper;
import org.argouml.model.InvalidElementException;
@@ -73,6 +74,8 @@
*/
private MDRModelImplementation modelImpl;
+ private static final Logger LOG = Logger.getLogger(CollaborationsHelperMDRImpl.class);
+
/**
* Constructor.
*
@@ -429,6 +432,7 @@
public Collection<Feature> allAvailableFeatures(Object arole) {
+ LOG.info("allAvailableFeatures start");
if (arole instanceof ClassifierRole) {
try {
@@ -443,6 +447,7 @@
for (Classifier classifier : role.getBase()) {
returnList.addAll(classifier.getFeature());
}
+ LOG.info("allAvailableFeatures " + returnList.size());
return returnList;
} catch (InvalidObjectException e) {
throw new InvalidElementException(e);
@@ -454,6 +459,7 @@
public Collection allAvailableContents(Object arole) {
+ LOG.info("allAvailableContents start");
try {
if (arole instanceof ClassifierRole) {
List returnList = new ArrayList();
@@ -467,6 +473,7 @@
for (Classifier baseClassifier : role.getBase()) {
returnList.addAll(baseClassifier.getOwnedElement());
}
+ LOG.info("allAvailableContents " + returnList.size());
return returnList;
}
} catch (InvalidObjectException e) {
Modified: trunk/src/argouml-core-model-mdr/src/org/argouml/model/mdr/CoreHelperMDRImpl.java
Url: http://argouml.tigris.org/source/browse/argouml/trunk/src/argouml-core-model-mdr/src/org/argouml/model/mdr/CoreHelperMDRImpl.java?view=diff&rev=15526&p1=trunk/src/argouml-core-model-mdr/src/org/argouml/model/mdr/CoreHelperMDRImpl.java&p2=trunk/src/argouml-core-model-mdr/src/org/argouml/model/mdr/CoreHelperMDRImpl.java&r1=15525&r2=15526
==============================================================================
--- trunk/src/argouml-core-model-mdr/src/org/argouml/model/mdr/CoreHelperMDRImpl.java (original)
+++ trunk/src/argouml-core-model-mdr/src/org/argouml/model/mdr/CoreHelperMDRImpl.java 2008-08-09 10:06:52-0700
@@ -34,6 +34,7 @@
import java.util.Set;
import javax.jmi.reflect.InvalidObjectException;
+import javax.swing.text.html.HTMLDocument.HTMLReader.IsindexAction;
import org.apache.log4j.Logger;
import org.argouml.model.CoreFactory;
@@ -414,12 +415,76 @@
static Collection<GeneralizableElement> getAllParents(
GeneralizableElement ge) {
Collection<GeneralizableElement> result =
- new HashSet<GeneralizableElement>();
- for (GeneralizableElement parent : getParents(ge)) {
+ new HashSet<GeneralizableElement>(2000);
+ getAllParents(result, ge);
+ return result;
+ }
+
+ private static void getAllParents(
+ final Collection<GeneralizableElement> result,
+ final GeneralizableElement ge) {
+ for (Generalization g : ge.getGeneralization()) {
+ GeneralizableElement parent = g.getParent();
result.add(parent);
- result.addAll(getAllParents(parent));
+ getAllParents(result, parent);
}
- return result;
+ }
+
+ /**
+ * A recursive method that iterates through generalizable elements to find
+ * if the model element is visible from any super namespace.
+ *
+ * @param element the model element to test
+ * @param ge the namespace which is a generalizable element
+ * @param dupCheck Used to prevent recursion continuing endlessly due to
+ * cyclic generalizations in the model. This should be an empty Set
+ * except when this method calls itself recursively.
+ * @return true if the model element is visible from any part of the
+ * generalization hierarchy.
+ */
+ private boolean isVisiblyOwned(
+ final ModelElement element,
+ final GeneralizableElement ge,
+ final Set<ModelElement> dupCheck) {
+
+ assert dupCheck != null;
+ assert ge != null;
+ assert element != null;
+
+ final boolean alreadyChecked = !dupCheck.add(ge);
+ if (alreadyChecked) {
+ if (LOG.isInfoEnabled()) {
+ LOG.info(
+ "Cyclic generalization found "
+ + getFullName(ge));
+ }
+ return false;
+ }
+
+ for (final Generalization g : ge.getGeneralization()) {
+ final GeneralizableElement parent = g.getParent();
+ if (parent instanceof Namespace
+ && isVisiblyOwned(element, (Namespace) parent)) {
+ return true;
+ }
+ }
+
+ for (final Generalization g : ge.getGeneralization()) {
+ // Recurse into ourself for each parent
+ if (isVisiblyOwned(element, g.getParent(), dupCheck)) {
+ return true;
+ }
+ }
+ return false;
+ }
+
+ private String getFullName(ModelElement elem) {
+ String name = elem.getName();
+ while (elem.getNamespace() != null) {
+ elem = elem.getNamespace();
+ name = elem.getName() + "." + name;
+ }
+ return name;
}
public List<Parameter> getReturnParameters(Object operation) {
@@ -1498,7 +1563,9 @@
return false;
}
- private boolean isValidNamespace(UmlAssociation assoc, Namespace ns) {
+ private boolean isValidNamespace(
+ final UmlAssociation assoc,
+ final Namespace ns) {
for (AssociationEnd end : assoc.getConnection()) {
if (!isVisible(end.getParticipant(), ns)) {
return false;
@@ -1565,9 +1632,9 @@
// of both <<access>> and <<import>> when the spec calls for
// only the former, but that seems to give different semantics
// to the way package imports work. Review to see which is wrong.
- Collection<Permission> permissions = getPackageImports(ns);
+ final Collection<Permission> permissions = getPackageImports(ns);
for (Permission imp : permissions) {
- Collection<ModelElement> suppliers = imp.getSupplier();
+ final Collection<ModelElement> suppliers = imp.getSupplier();
for (ModelElement me : suppliers) {
// d.supplier.oclAsType(Namespace).ownedElement->select (e |
// e.elementOwnership.visibility =
@@ -1581,21 +1648,25 @@
// e. elementOwnership.visibility =
// #public)->includes (r.participant) or
if (me instanceof GeneralizableElement) {
- Collection<GeneralizableElement> allParents =
- getAllParents((GeneralizableElement) me);
- for (GeneralizableElement parent : allParents) {
- if (parent instanceof Namespace
- && isVisiblyOwned(element,
- (Namespace) parent)) {
- return true;
- }
+
+ // TODO: Performance. Consider instantiating this just
+ // once outside the for loops and clear at this point
+ // instead.
+ final Set<ModelElement> dupCheck =
+ new HashSet<ModelElement>(10);
+
+ if (isVisiblyOwned(
+ element,
+ (GeneralizableElement) me,
+ dupCheck)) {
+ return true;
}
}
// d.supplier.oclAsType(Package).allImportedElements->select (
// e | e. elementImport.visibility =
// #public) ->includes (r.participant) ) )
if (me instanceof UmlPackage) {
- Collection<ElementImport> imports =
+ final Collection<ElementImport> imports =
((UmlPackage) me).getElementImport();
for (ElementImport ei : imports) {
if (element.equals(ei.getImportedElement())
@@ -1631,10 +1702,15 @@
Collection<Generalization> generalizations =
generalizableElement.getGeneralization();
+ ModelManagementHelperMDRImpl modelManagementHelper =
+ (ModelManagementHelperMDRImpl) modelImpl.getModelManagementHelper();
+
for (Generalization generalization : generalizations) {
- GeneralizableElement parent = generalization.getParent();
- if (!modelImpl.getModelManagementHelper().getAllContents(namespace)
- .contains(parent)) {
+ final GeneralizableElement parent = generalization.getParent();
+ final Set<ModelElement> results = new HashSet<ModelElement>(2000);
+ final Set<ModelElement> dupCheck = new HashSet<ModelElement>(2000);
+ modelManagementHelper.getAllContents(results, namespace, dupCheck);
+ if (!results.contains(parent)) {
return false;
}
}
@@ -1699,9 +1775,11 @@
public Collection<Namespace> getAllPossibleNamespaces(Object modelElement,
Object model) {
+ LOG.info("getAllPossibleNamespaces start");
ModelElement m = (ModelElement) modelElement;
Collection<Namespace> ret = new HashSet<Namespace> ();
if (m == null) {
+ LOG.info("getAllPossibleNamespaces end");
return ret;
}
@@ -1721,6 +1799,13 @@
throw new InvalidElementException(e);
}
+ if (LOG.isInfoEnabled()) {
+ // This is an expensive method that we should ensure is called
+ // rarely. Hence info level to track easily.
+ LOG.info(
+ "getAllPossibleNamespaces returns "
+ + ret.size() + " items");
+ }
return ret;
}
Modified: trunk/src/argouml-core-model-mdr/src/org/argouml/model/mdr/ModelManagementHelperMDRImpl.java
Url: http://argouml.tigris.org/source/browse/argouml/trunk/src/argouml-core-model-mdr/src/org/argouml/model/mdr/ModelManagementHelperMDRImpl.java?view=diff&rev=15526&p1=trunk/src/argouml-core-model-mdr/src/org/argouml/model/mdr/ModelManagementHelperMDRImpl.java&p2=trunk/src/argouml-core-model-mdr/src/org/argouml/model/mdr/ModelManagementHelperMDRImpl.java&r1=15525&r2=15526
==============================================================================
--- trunk/src/argouml-core-model-mdr/src/org/argouml/model/mdr/ModelManagementHelperMDRImpl.java (original)
+++ trunk/src/argouml-core-model-mdr/src/org/argouml/model/mdr/ModelManagementHelperMDRImpl.java 2008-08-09 10:06:52-0700
@@ -572,6 +572,34 @@
}
/**
+ * <p>A helper method to make {@link #getContents(Object)} as
+ * efficient as possible.</p>
+ * <p>This is called passing in a collection to place the result
+ * rather than creating a new instance of a collection to return</p>
+ * @param results a collection of model elements to which the contents
+ * are to be added
+ * @param modelelement the model element to get the contents from
+ */
+ private void getContents(
+ final Collection<ModelElement> results,
+ final Object modelelement) {
+ if (modelelement instanceof UmlPackage) {
+ getContents(results, (UmlPackage) modelelement);
+ return;
+ } else if (modelelement instanceof Namespace) {
+ getContents(results, (Namespace) modelelement);
+ return;
+ } else if (modelelement instanceof Instance) {
+ getContents(results, (Instance) modelelement);
+ return;
+ } else if (modelelement == null) {
+ return;
+ }
+ throw new IllegalArgumentException("Unsupported element type "
+ + modelelement);
+ }
+
+ /**
* Get the contents of a Package.
* <p>
* For a Package: <pre>
@@ -600,6 +628,21 @@
}
/**
+ * Adds the contents of a package to the given collection.
+ * Same as {@link #getContents(UmlPackage)} but adding to results to an
+ * existing collection instead of returning a new collection.
+ */
+ private static void getContents(
+ final Collection<ModelElement> results,
+ final UmlPackage pkg) {
+ Collection<ElementImport> c = pkg.getElementImport();
+ for (ElementImport ei : c) {
+ results.add(ei.getImportedElement());
+ }
+ getContents(results, (Namespace) pkg);
+ }
+
+ /**
* Get the contents of a Namespace (which includes the contents of
* all owning namespaces).
* <p>
@@ -622,6 +665,22 @@
// TODO: Should we handle <<access>> and <<import>> here?
return results;
}
+
+ /**
+ * Adds the contents of a namespace to the given collection.
+ * Same as {@link #getContents(Namespace)} but adding to results to an
+ * existing collection instead of returning a new collection.
+ */
+ private static void getContents(
+ final Collection<ModelElement> results,
+ final Namespace namespace) {
+ results.addAll(namespace.getOwnedElement());
+ Namespace owner = namespace.getNamespace();
+ if (owner != null) {
+ getContents(results, owner);
+ }
+ // TODO: Should we handle <<access>> and <<import>>?
+ }
/**
* Return the contents of an Instance.
@@ -641,12 +700,43 @@
return results;
}
+ /**
+ * Adds the contents of an instance to the given collection.
+ * Same as {@link #getContents(Instance)} but adding to results to an
+ * existing collection instead of returning a new collection.
+ */
+ private static void getContents(
+ final Collection<ModelElement> results,
+ final Instance instance) {
+ results.addAll(instance.getOwnedInstance());
+ results.addAll(instance.getOwnedLink());
+ }
+
public Collection<ModelElement> getAllImportedElements(Object pack) {
if (!(pack instanceof Namespace)) {
return Collections.emptyList();
}
- Namespace ns = ((Namespace) pack);
Collection<ModelElement> ret = new ArrayList<ModelElement>();
+ getAllImportedElements(ret, pack);
+ return ret;
+ }
+
+ /**
+ * <p>A helper method to make {@link #getAllImportedElements(Object)} as
+ * efficient as possible.</p>
+ * <p>This is called passing in a collection to place the result
+ * rather than creating a new instance of a collection to return</p>
+ * @param results a collection of modelelements to which the imported
+ * elements are to be added
+ * @param pack the package to get the imported elements from
+ */
+ private void getAllImportedElements(
+ final Collection<ModelElement> results,
+ final Object pack) {
+ if (!(pack instanceof Namespace)) {
+ return;
+ }
+ Namespace ns = ((Namespace) pack);
try {
/* TODO: This is not according the contract for this function, but
* it is used in several places, and I (MVW) presume that
@@ -661,7 +751,7 @@
.hasStereotype(dep, FRIEND_STEREOTYPE)) {
for (ModelElement o : dep.getSupplier()) {
if (o instanceof Namespace) {
- ret.addAll(((Namespace) o).getOwnedElement());
+ results.addAll(((Namespace) o).getOwnedElement());
}
}
} else if (modelImpl.getExtensionMechanismsHelper()
@@ -670,7 +760,7 @@
.hasStereotype(dep, ACCESS_STEREOTYPE)) {
for (ModelElement o : dep.getSupplier()) {
if (o instanceof Namespace) {
- ret.addAll(CoreHelperMDRImpl
+ results.addAll(CoreHelperMDRImpl
.getAllVisibleElements((Namespace) o));
}
}
@@ -679,19 +769,40 @@
}
/* TODO: This is the 2nd part of this method: */
Collection imports = modelImpl.getFacade().getImportedElements(ns);
- ret.addAll(imports);
+ results.addAll(imports);
} catch (InvalidObjectException e) {
throw new InvalidElementException(e);
}
- return ret;
}
public Collection<ModelElement> getAllContents(Object pack) {
- Set<ModelElement> results = new HashSet<ModelElement>();
- if (pack == null) {
- return results;
+ // TODO: Is there anyway we can determine this size at runtime?
+ Set<ModelElement> results = new HashSet<ModelElement>(2000);
+ Set<ModelElement> dupCheck = new HashSet<ModelElement>(2000);
+ getAllContents(results, (ModelElement) pack, dupCheck);
+ return results;
+ }
+
+ /**
+ * <p>A helper method to make {@link #getAllContents(Object)} as efficient
+ * as possible. This is called recursively.</p>
+ * <p>
+ * @param results a collection that will be populated with model elements
+ * @param pack the namespace to get the contents from
+ * @param dupCheck a collection of model elements that have already been
+ * checked on a previous recursive call. Checking this saves duplicating
+ * effort on already processed elements.
+ */
+ void getAllContents(
+ final Collection<ModelElement> results,
+ final ModelElement pack,
+ final Collection<ModelElement> dupCheck) {
+ if (pack == null || dupCheck.contains(pack)) {
+ return;
}
+
+ dupCheck.add(pack);
try {
/*
@@ -706,7 +817,7 @@
* </pre><p>
*/
if (pack instanceof Namespace) {
- results.addAll(getContents(pack));
+ getContents(results, pack);
}
/*
@@ -759,18 +870,19 @@
if (pack instanceof Classifier || pack instanceof UmlPackage) {
Collection<GeneralizableElement> parents =
CoreHelperMDRImpl.getParents((GeneralizableElement) pack);
- Set<ModelElement> allContents = new HashSet<ModelElement>();
+ // TODO: Try reusing the same set on every recursion
+ Set<ModelElement> allContents = new HashSet<ModelElement>(2000);
for (GeneralizableElement parent : parents) {
- allContents.addAll(getAllContents(parent));
+ getAllContents(allContents, parent, dupCheck);
}
if (pack instanceof UmlPackage) {
- allContents.addAll(getAllImportedElements(pack));
+ getAllImportedElements(allContents, pack);
for (GeneralizableElement parent : parents) {
- allContents.addAll(getAllImportedElements(parent));
+ getAllImportedElements(allContents, parent);
}
}
-
+
for (ModelElement element : allContents) {
if (VisibilityKindEnum.VK_PUBLIC.equals(element
.getVisibility())
@@ -807,8 +919,6 @@
} catch (InvalidObjectException e) {
throw new InvalidElementException(e);
}
- return results;
-
}
public boolean isReadOnly(Object element) {
lmpx.com only provides a reader for public news (NNTP) servers. It is not
affiliated with the servers or forums shown here and is not responsible for
the content of articles, which is written by their respective authors.