Fix #82: Fixing tree now handles case when nodes pointing non-existing parent

Author: lazychaser

CHANGELOG.markdown

@@ -1,13 +1,15 @@
 ### 4.1.0
 
-*   Converted to trait
-*   Methods were renamed:
+*   #75: Converted to trait. Following methods were renamed:
     -   `appendTo` to `appendToNode`
     -   `prependTo` to `prependToNode`
     -   `insertBefore` to `insertBeforeNode`
     -   `insertAfter` to `insertAfterNode`
     -   `getNext` to `getNextNode`
     -   `getPrev` to `getPrevNode`
+    
+*   #82: Fixing tree now handles case when nodes pointing non-existing parent
+*   The number of missing parent is now returned when using `countErrors`
 
 ### 3.1.1
 

README.markdown

@@ -420,10 +420,12 @@ $data = Category::countErrors();
 
 It will return an array with following keys:
 
--   `oddness` -- the number of nodes that have wrong set of `lft` and `rgt` values;
--   `duplicates` -- the number of nodes that have same `lft` or `rgt` values;
+-   `oddness` -- the number of nodes that have wrong set of `lft` and `rgt` values
+-   `duplicates` -- the number of nodes that have same `lft` or `rgt` values
 -   `wrong_parent` -- the number of nodes that have invalid `parent_id` value that
-    doesn't correspond to `lft` and `rgt` values.
+    doesn't correspond to `lft` and `rgt` values
+-   `missing_parent` -- the number of nodes that have `parent_id` pointing to
+    node that doesn't exists
 
 #### Fixing tree
 

src/NodeTrait.php

@@ -45,37 +45,37 @@ public static function bootNodeTrait()
     {
         static::saving(function (self $model) {
             $model->getConnection()->beginTransaction();
-            
+
             return $model->callPendingAction();
         });
-        
+
         static::saved(function (self $model) {
             $model->getConnection()->commit();
         });
 
         static::deleting(function (self $model) {
             $model->getConnection()->beginTransaction();
-            
+
             // We will need fresh data to delete node safely
             $model->refreshNode();
         });
 
         static::deleted(function (self $model) {
             $model->deleteDescendants();
-            
+
             $model->getConnection()->commit();
         });
 
         if (static::usesSoftDelete()) {
             static::restoring(function (self $model) {
                 $model->getConnection()->beginTransaction();
-                
+
                 static::$deletedAt = $model->{$model->getDeletedAtColumn()};
             });
 
             static::restored(function (self $model) {
                 $model->restoreDescendants(static::$deletedAt);
-                
+
                 $model->getConnection()->commit();
             });
         }
@@ -197,8 +197,8 @@ protected function actionAppendOrPrepend(self $parent, $prepend = false)
      */
     protected function setParent($value)
     {
-        $this->attributes[$this->getParentIdName()] = $value 
-            ? $value->getKey() 
+        $this->attributes[$this->getParentIdName()] = $value
+            ? $value->getKey()
             : null;
 
         $this->setRelation('parent', $value);
@@ -805,9 +805,7 @@ public function getParentIdName()
      */
     public function getLft()
     {
-        return isset($this->attributes[$this->getLftName()])
-            ? $this->attributes[$this->getLftName()]
-            : null;
+        return $this->getAttributeValue($this->getLftName());
     }
 
     /**
@@ -817,9 +815,7 @@ public function getLft()
      */
     public function getRgt()
     {
-        return isset($this->attributes[$this->getRgtName()])
-            ? $this->attributes[$this->getRgtName()]
-            : null;
+        return $this->getAttributeValue($this->getRgtName());
     }
 
     /**
@@ -829,14 +825,14 @@ public function getRgt()
      */
     public function getParentId()
     {
-        return $this->getAttribute($this->getParentIdName());
+        return $this->getAttributeValue($this->getParentIdName());
     }
 
     /**
      * Returns node that is next to current node without constraining to siblings.
-     * 
+     *
      * This can be either a next sibling or a next sibling of the parent node.
-     * 
+     *
      * @param array $columns
      *
      * @return self
@@ -850,7 +846,7 @@ public function getNextNode(array $columns = array( '*' ))
      * Returns node that is before current node without constraining to siblings.
      *
      * This can be either a prev sibling or parent node.
-     * 
+     *
      * @param array $columns
      *
      * @return self
@@ -943,7 +939,7 @@ public function getPrevSibling(array $columns = array( '*' ))
      */
     public function isDescendantOf(self $other)
     {
-        return $this->getLft() > $other->getLft() && 
+        return $this->getLft() > $other->getLft() &&
                $this->getLft() < $other->getRgt();
     }
 
@@ -1029,15 +1025,23 @@ public function getBounds()
      */
     public function setLft($value)
     {
-        $this->setAttribute($this->getLftName(), $value);
+        $this->attributes[$this->getLftName()] = $value;
     }
 
     /**
      * @param $value
      */
     public function setRgt($value)
     {
-        $this->setAttribute($this->getRgtName(), $value);
+        $this->attributes[$this->getRgtName()] = $value;
+    }
+
+    /**
+     * @param $value
+     */
+    public function setParentId($value)
+    {
+        $this->attributes[$this->getParentIdName()] = $value;
     }
 
 }

src/QueryBuilder.php

@@ -587,7 +587,10 @@ public function countErrors()
         // Check if parent_id is set correctly
         $checks['wrong_parent'] = $this->getWrongParentQuery();
 
-        $query = $this->toBase();
+        // Check for nodes that have missing parent
+        $checks['missing_parent' ] = $this->getMissingParentQuery();
+
+        $query = $this->query->newQuery();
 
         foreach ($checks as $key => $inner) {
             $inner->selectRaw('count(1)');
@@ -625,7 +628,7 @@ protected function getDuplicatesQuery()
             ->newServiceQuery()
             ->toBase()
             ->from($this->query->raw("{$table} c1, {$table} c2"))
-            ->whereRaw("c1.id <> c2.id")
+            ->whereRaw("c1.id < c2.id")
             ->whereNested(function (BaseQueryBuilder $inner) {
                 list($lft, $rgt) = $this->wrappedColumns();
 
@@ -662,6 +665,32 @@ protected function getWrongParentQuery()
 
     }
 
+    /**
+     * @return $this
+     */
+    protected function getMissingParentQuery()
+    {
+        return $this->model
+            ->newServiceQuery()
+            ->toBase()
+            ->whereNested(function (BaseQueryBuilder $inner) {
+                $table = $this->wrappedTable();
+                $keyName = $this->wrappedKey();
+                $parentIdName = $this->query->raw($this->model->getParentIdName());
+
+                $existsCheck = $this->model
+                    ->newServiceQuery()
+                    ->toBase()
+                    ->selectRaw('1')
+                    ->from($this->query->raw("{$table} p"))
+                    ->whereRaw("{$table}.{$parentIdName} = p.{$keyName}")
+                    ->limit(1);
+
+                $inner->whereRaw("{$parentIdName} is not null")
+                      ->addWhereExistsQuery($existsCheck, 'and', true);
+            });
+    }
+
     /**
      * Get the number of total errors of the tree.
      *
@@ -710,7 +739,18 @@ public function fixTree()
 
         $fixed = 0;
 
-        self::reorderNodes($nodes, $fixed);
+        $cut = self::reorderNodes($nodes, $fixed);
+
+        // Saved nodes that have invalid parent as roots
+        while ( ! $nodes->isEmpty()) {
+            $parentId = $nodes->keys()->first();
+
+            foreach ($nodes[$parentId] as $model) {
+                $model->setParentId(null);
+            }
+
+            $cut = self::reorderNodes($nodes, $fixed, $parentId, $cut);
+        }
 
         return $fixed;
     }
@@ -726,8 +766,12 @@ public function fixTree()
     protected static function reorderNodes(Collection $models, &$fixed,
                                            $parentId = null, $cut = 1
     ) {
+        if ( ! isset($models[$parentId])) {
+            return $cut;
+        }
+
         /** @var Model|self $model */
-        foreach ($models->get($parentId, [ ]) as $model) {
+        foreach ($models[$parentId] as $model) {
             $model->setLft($cut);
 
             $cut = self::reorderNodes($models, $fixed, $model->getKey(), $cut + 1);
@@ -743,6 +787,8 @@ protected static function reorderNodes(Collection $models, &$fixed,
             ++$cut;
         }
 
+        unset($models[$parentId]);
+
         return $cut;
     }
 }

tests/NodeTest.php

@@ -78,9 +78,8 @@ public function dumpTree($items = null)
     {
         if ( ! $items) $items = Category::withTrashed()->defaultOrder()->get();
 
-        foreach ($items as $item)
-        {
-            echo PHP_EOL.($item->trashed() ? '-' : '+').' '.$item->name." ".$item->getLft()." ".$item->getRgt();
+        foreach ($items as $item) {
+            echo PHP_EOL.($item->trashed() ? '-' : '+').' '.$item->name." ".$item->getKey().' '.$item->getLft()." ".$item->getRgt().' '.$item->getParentId();
         }
     }
 
@@ -386,8 +385,6 @@ public function testSoftDeletedNodeisDeletedWhenParentIsDeleted()
 
         $this->assertTreeNotBroken();
 
-        $this->dumpTree();
-
         $this->assertNull($this->findCategory('samsung', true));
         $this->assertNull($this->findCategory('sony'));
     }
@@ -566,7 +563,21 @@ public function testCountsTreeErrors()
     {
         $errors = Category::countErrors();
 
-        $this->assertEquals([ 'oddness' => 0, 'duplicates' => 0, 'wrong_parent' => 0 ], $errors);
+        $this->assertEquals([ 'oddness' => 0,
+                              'duplicates' => 0,
+                              'wrong_parent' => 0,
+                              'missing_parent' => 0 ], $errors);
+
+        Category::where('id', '=', 5)->update([ '_lft' => 14 ]);
+        Category::where('id', '=', 8)->update([ 'parent_id' => 2 ]);
+        Category::where('id', '=', 11)->update([ '_lft' => 20 ]);
+        Category::where('id', '=', 4)->update([ 'parent_id' => 24 ]);
+
+        $errors = Category::countErrors();
+
+        $this->assertEquals(1, $errors['oddness']);
+        $this->assertEquals(2, $errors['duplicates']);
+        $this->assertEquals(1, $errors['missing_parent']);
     }
 
     public function testCreatesNode()
@@ -649,6 +660,8 @@ public function testTreeIsFixed()
     {
         Category::where('id', '=', 5)->update([ '_lft' => 14 ]);
         Category::where('id', '=', 8)->update([ 'parent_id' => 2 ]);
+        Category::where('id', '=', 11)->update([ '_lft' => 20 ]);
+        Category::where('id', '=', 2)->update([ 'parent_id' => 24 ]);
 
         $fixed = Category::fixTree();
 

tests/data/categories.php

@@ -11,5 +11,5 @@
                 array('id' => 8, 'name' => 'galaxy', '_lft' => 12, '_rgt' => 13, 'parent_id' => 7),
             array('id' => 9, 'name' => 'sony', '_lft' => 15, '_rgt' => 16, 'parent_id' => 5),
             array('id' => 10, 'name' => 'lenovo', '_lft' => 17, '_rgt' => 18, 'parent_id' => 5),
-    array('id' => 11, 'name' => 'store 2', '_lft' => 21, '_rgt' => 22, 'parent_id' => null),
+    array('id' => 11, 'name' => 'store_2', '_lft' => 21, '_rgt' => 22, 'parent_id' => null),
 );