From 96f8d944f8a0ed4a1b0483db9813d8240a99a696 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Stefan=20Tr=C3=B6ger?= <stefantroeger@gmx.net>
Date: Fri, 4 Aug 2017 07:06:58 +0200
Subject: [PATCH] Make Group searching robust for cyclic dependencies. fixes
 #0002567

---
 src/App/GroupExtension.cpp | 64 +++++++++++++++++++++++++++++---------
 src/App/GroupExtension.h   |  7 +++--
 src/Mod/Test/Document.py   | 14 +++++++--
 3 files changed, 66 insertions(+), 19 deletions(-)

diff --git a/src/App/GroupExtension.cpp b/src/App/GroupExtension.cpp
index 3437683638..8b99a2e574 100644
--- a/src/App/GroupExtension.cpp
+++ b/src/App/GroupExtension.cpp
@@ -167,15 +167,28 @@ DocumentObject *GroupExtension::getObject(const char *Name) const
 
 bool GroupExtension::hasObject(const DocumentObject* obj, bool recursive) const
 {
+    
+    if(obj == getExtendedObject())
+        return false;
+    
     const std::vector<DocumentObject*>& grp = Group.getValues();
-    for (std::vector<DocumentObject*>::const_iterator it = grp.begin(); it != grp.end(); ++it) {
-        if (*it == obj) {
+    for (auto child : grp) {
+        
+        if(!child)
+            continue;
+        
+        if (child == obj) {
             return true;
-        } else if ( recursive && (*it)->hasExtension(GroupExtension::getExtensionClassTypeId()) ) {
+        } else if (child == getExtendedObject()) {
+            Base::Exception("Cyclic dependencies detected: Search cannot be performed");
+        } else if ( recursive && child->hasExtension(GroupExtension::getExtensionClassTypeId()) ) {
+                       
             App::GroupExtension *subGroup = static_cast<App::GroupExtension *> (
-                                    (*it)->getExtension(GroupExtension::getExtensionClassTypeId()));
+                                    child->getExtension(GroupExtension::getExtensionClassTypeId()));
+            std::vector<const GroupExtension*> history;
+            history.push_back(this);
 
-            if (subGroup->hasObject (obj, recursive)) {
+            if (subGroup->recursiveHasObject (obj, subGroup, history)) {
                 return true;
             }
         }
@@ -184,22 +197,45 @@ bool GroupExtension::hasObject(const DocumentObject* obj, bool recursive) const
     return false;
 }
 
-bool GroupExtension::isChildOf(const GroupExtension* group) const
-{
-    const std::vector<DocumentObject*>& grp = group->Group.getValues();
-    for (std::vector<DocumentObject*>::const_iterator it = grp.begin(); it != grp.end(); ++it) {
-        if (*it == getExtendedObject())
-            return true;
-        if ((*it)->hasExtension(GroupExtension::getExtensionClassTypeId())) {
-            if (this->isChildOf(static_cast<GroupExtension*>((*it)->getExtension(GroupExtension::getExtensionClassTypeId()))))
+bool GroupExtension::recursiveHasObject(const DocumentObject* obj, const GroupExtension* group, 
+                                        std::vector< const GroupExtension* > history) const {
 
+    //the purpose is to prevent infinite recursion when groups form a cyclic graph. To do this 
+    //we store every group we processed on the current leave of the tree, and if we reach an 
+    //already processed group we know that it not really is a tree but a cycle.
+    
+    history.push_back(this);
+    
+    for (auto child : group->Group.getValues()) {
+        
+        if(!child)
+            continue;
+        
+        if (child == obj) {
+            return true;
+        } else if ( child->hasExtension(GroupExtension::getExtensionClassTypeId()) ) {
+            
+            auto ext = child->getExtensionByType<GroupExtension>();
+            
+            if(std::find(history.begin(), history.end(), ext) != history.end())
+                Base::Exception("Cyclic dependencies detected: Search cannot be performed");
+            
+            if (recursiveHasObject(obj, ext, history)) {
                 return true;
+            }
         }
     }
-
     return false;
 }
 
+
+bool GroupExtension::isChildOf(const GroupExtension* group, bool recursive) const
+{
+    return group->hasObject(getExtendedObject(), recursive);
+}
+
+
+
 std::vector<DocumentObject*> GroupExtension::getObjects() const
 {
     return Group.getValues();
diff --git a/src/App/GroupExtension.h b/src/App/GroupExtension.h
index 53230e6f12..0e1d27d0b8 100644
--- a/src/App/GroupExtension.h
+++ b/src/App/GroupExtension.h
@@ -80,10 +80,10 @@ public:
      */
     bool hasObject(const DocumentObject* obj, bool recursive=false) const;
     /**
-     * Checks whether this group object is a child (or sub-child)
+     * Checks whether this group object is a child (or sub-child if enabled)
      * of the given group object.
      */
-    bool isChildOf(const GroupExtension*) const;
+    bool isChildOf(const GroupExtension* group, bool recursive = true) const;
     /** Returns a list of all objects this group does have.
      */
     std::vector<DocumentObject*> getObjects() const;
@@ -110,6 +110,9 @@ public:
 
 private:
     void removeObjectFromDocument(DocumentObject*);
+    //this version if has object stores the already searched objects to prevent infinite recursion
+    //in case of a cyclic group graph
+    bool recursiveHasObject(const DocumentObject* obj, const GroupExtension* group, std::vector<const GroupExtension*> history) const;
 };
 
 
diff --git a/src/Mod/Test/Document.py b/src/Mod/Test/Document.py
index e5607582ff..e05b54ec4d 100644
--- a/src/Mod/Test/Document.py
+++ b/src/Mod/Test/Document.py
@@ -933,9 +933,17 @@ class DocumentGroupCases(unittest.TestCase):
     prt1.addObject(prt2)
     grp = prt2.Group
     grp.append(prt1)
-    prt2.Group = grp 
-    prt1.Group = [prt1]
-    prt2.Group = [prt2]
+    prt2.Group = grp
+    self.Doc.recompute()
+    prt2.Group = []
+    try:
+        prt2.Group = [prt2]
+    except:
+        pass
+    else:
+        self.fail("Exception is expected")
+        
+    self.Doc.recompute()
 
   def tearDown(self):
     # closing doc