diff --git a/CHANGELOG.md b/CHANGELOG.md index f4fb3fc..fb5aef3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,10 +10,26 @@ based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). - `NestedSet::isNode()` always returned `false` for genuine nodes because it inspected object properties instead of used traits. It now uses `class_uses_recursive()`. +- Moving a node no longer breaks when the model defines a global scope that + adds columns (e.g. via `addSelect`). `getNodeData()` now selects only `lft` + and `rgt` and returns them in a known order, so the `[$lft, $rgt]` read can + no longer pick up an extra column (upstream issue #513). +- `columnPatch()` now renders a zero offset as `+0` rather than concatenating + it onto the column (`"_rgt"0`), which produced invalid SQL on stricter + drivers such as SQL Server (upstream PR #595). ### Added - Native PHP type declarations across the library (PHP 8.3+). +- `isBroken()`, `countErrors()`, `getTotalErrors()`, `fixTree()` and + `fixSubtree()` accept an optional callback to customise the underlying query + — most usefully to drop a global scope that would otherwise interfere with + the integrity checks, e.g. + `Category::fixTree(null, fn ($q) => $q->withoutGlobalScopes())` + (upstream PR #536). +- `withDepth()` now honours global scopes removed from the outer query, so + `Model::withoutGlobalScope(...)->withDepth()` counts the same ancestors in + the depth subquery (upstream PR #300). - Laravel Pint for code style. - Larastan static analysis (level 5) with a baseline for pre-existing findings. - Test suite now runs against SQLite, MySQL and PostgreSQL in CI, plus a code diff --git a/README.md b/README.md index bc1ba98..496999e 100644 --- a/README.md +++ b/README.md @@ -567,6 +567,20 @@ Category::fixTree(); Category::fixSubtree($root); ``` +#### Customising the query + +If your model defines a global scope that interferes with the integrity checks +(for example one that adds columns or self-join-unsafe `where` clauses), pass a +callback to customise the query used by `isBroken()`, `countErrors()`, +`getTotalErrors()`, `fixTree()` and `fixSubtree()`. The most common use is +dropping a global scope: + +```php +Category::isBroken(fn ($query) => $query->withoutGlobalScopes()); + +Category::fixTree(null, fn ($query) => $query->withoutGlobalScopes()); +``` + ### Scoping Imagine you have `Menu` and `MenuItem` models with a one-to-many relationship, where diff --git a/src/QueryBuilder.php b/src/QueryBuilder.php index 48cea75..88bf363 100644 --- a/src/QueryBuilder.php +++ b/src/QueryBuilder.php @@ -2,6 +2,7 @@ namespace Lunar\Nestedset; +use Closure; use Illuminate\Database\Eloquent\Builder; use Illuminate\Database\Eloquent\Model; use Illuminate\Database\Eloquent\ModelNotFoundException; @@ -25,18 +26,28 @@ class QueryBuilder extends Builder */ public function getNodeData(mixed $id, bool $required = false): array { - $query = $this->toBase(); + $lftName = $this->model->getLftName(); + $rgtName = $this->model->getRgtName(); - $query->where($this->model->getKeyName(), '=', $id); - - $data = $query->first([$this->model->getLftName(), - $this->model->getRgtName()]); + // Select lft/rgt explicitly rather than passing them to first(), which + // is ignored when a global scope has already set the query columns + // (e.g. via addSelect). select() resets the columns so we always get + // exactly these two, regardless of any scope on the model. + $data = (array) $this->toBase() + ->select([$lftName, $rgtName]) + ->where($this->model->getKeyName(), '=', $id) + ->first(); if (! $data && $required) { throw new ModelNotFoundException; } - return (array) $data; + // Return lft/rgt in a known order and nothing else. A global scope may + // otherwise reorder or add columns, breaking getPlainNodeData()'s + // positional [$lft, $rgt] destructuring (see issue #513). + return $data + ? [$lftName => $data[$lftName], $rgtName => $data[$rgtName]] + : []; } /** @@ -301,6 +312,9 @@ public function withDepth(string $as = 'depth'): static $query = $this->model ->newScopedQuery('_d') + // Carry over any global scopes removed from the outer query so the + // depth subquery counts the same set of ancestors. + ->withoutGlobalScopes($this->removedScopes) ->toBase() ->selectRaw('count(1) - 1') ->from($this->model->getTable().' as '.$alias) @@ -494,7 +508,10 @@ protected function columnPatch(string $col, array $params): Expression extract($params); /** @var int $height */ - if ($height > 0) { + // Use >= 0 so that a height of 0 renders as "+0" rather than being + // concatenated onto the column (e.g. `"_rgt"0`), which is invalid SQL + // on stricter drivers such as SQL Server. + if ($height >= 0) { $height = '+'.$height; } @@ -507,7 +524,7 @@ protected function columnPatch(string $col, array $params): Expression /** @var int $rgt */ /** @var int $from */ /** @var int $to */ - if ($distance > 0) { + if ($distance >= 0) { $distance = '+'.$distance; } @@ -521,23 +538,27 @@ protected function columnPatch(string $col, array $params): Expression /** * Get statistics of errors of the tree. * + * The optional callback receives each check's query builder, allowing the + * caller to customise it — for example to drop a global scope that would + * otherwise interfere with the integrity checks. + * * @since 2.0 */ - public function countErrors(): array + public function countErrors(?Closure $callback = null): array { $checks = []; // Check if lft and rgt values are ok - $checks['oddness'] = $this->getOdnessQuery(); + $checks['oddness'] = $this->getOdnessQuery($callback); // Check if lft and rgt values are unique - $checks['duplicates'] = $this->getDuplicatesQuery(); + $checks['duplicates'] = $this->getDuplicatesQuery($callback); // Check if parent_id is set correctly - $checks['wrong_parent'] = $this->getWrongParentQuery(); + $checks['wrong_parent'] = $this->getWrongParentQuery($callback); // Check for nodes that have missing parent - $checks['missing_parent'] = $this->getMissingParentQuery(); + $checks['missing_parent'] = $this->getMissingParentQuery($callback); $query = $this->query->newQuery(); @@ -550,10 +571,11 @@ public function countErrors(): array return (array) $query->first(); } - protected function getOdnessQuery(): BaseQueryBuilder + protected function getOdnessQuery(?Closure $callback = null): BaseQueryBuilder { return $this->model ->newNestedSetQuery() + ->when($callback, $callback) ->toBase() ->whereNested(function (BaseQueryBuilder $inner) { [$lft, $rgt] = $this->wrappedColumns(); @@ -563,7 +585,7 @@ protected function getOdnessQuery(): BaseQueryBuilder }); } - protected function getDuplicatesQuery(): BaseQueryBuilder + protected function getDuplicatesQuery(?Closure $callback = null): BaseQueryBuilder { $table = $this->wrappedTable(); $keyName = $this->wrappedKey(); @@ -576,6 +598,7 @@ protected function getDuplicatesQuery(): BaseQueryBuilder $query = $this->model ->newNestedSetQuery($firstAlias) + ->when($callback, $callback) ->toBase() ->from($this->query->raw("{$table} as {$waFirst}, {$table} {$waSecond}")) ->whereRaw("{$waFirst}.{$keyName} < {$waSecond}.{$keyName}") @@ -591,7 +614,7 @@ protected function getDuplicatesQuery(): BaseQueryBuilder return $this->model->applyNestedSetScope($query, $secondAlias); } - protected function getWrongParentQuery(): BaseQueryBuilder + protected function getWrongParentQuery(?Closure $callback = null): BaseQueryBuilder { $table = $this->wrappedTable(); $keyName = $this->wrappedKey(); @@ -610,6 +633,7 @@ protected function getWrongParentQuery(): BaseQueryBuilder $query = $this->model ->newNestedSetQuery('c') + ->when($callback, $callback) ->toBase() ->from($this->query->raw("{$table} as {$waChild}, {$table} as {$waParent}, $table as {$waInterm}")) ->whereRaw("{$waChild}.{$parentIdName}={$waParent}.{$keyName}") @@ -629,12 +653,13 @@ protected function getWrongParentQuery(): BaseQueryBuilder return $query; } - protected function getMissingParentQuery(): BaseQueryBuilder + protected function getMissingParentQuery(?Closure $callback = null): BaseQueryBuilder { return $this->model ->newNestedSetQuery() + ->when($callback, $callback) ->toBase() - ->whereNested(function (BaseQueryBuilder $inner) { + ->whereNested(function (BaseQueryBuilder $inner) use ($callback) { $grammar = $this->query->getGrammar(); $table = $this->wrappedTable(); @@ -645,6 +670,7 @@ protected function getMissingParentQuery(): BaseQueryBuilder $existsCheck = $this->model ->newNestedSetQuery() + ->when($callback, $callback) ->toBase() ->selectRaw('1') ->from($this->query->raw("{$table} as {$wrappedAlias}")) @@ -663,9 +689,9 @@ protected function getMissingParentQuery(): BaseQueryBuilder * * @since 2.0 */ - public function getTotalErrors(): int + public function getTotalErrors(?Closure $callback = null): int { - return array_sum($this->countErrors()); + return array_sum($this->countErrors($callback)); } /** @@ -673,9 +699,9 @@ public function getTotalErrors(): int * * @since 2.0 */ - public function isBroken(): bool + public function isBroken(?Closure $callback = null): bool { - return $this->getTotalErrors() > 0; + return $this->getTotalErrors($callback) > 0; } /** @@ -684,9 +710,11 @@ public function isBroken(): bool * Nodes with invalid parent are saved as roots. * * @param null|NodeTrait|Model $root + * @param null|Closure $callback Customise the query used to load nodes, + * e.g. to drop an interfering global scope. * @return int The number of changed nodes */ - public function fixTree($root = null): int + public function fixTree($root = null, ?Closure $callback = null): int { $columns = [ $this->model->getKeyName(), @@ -697,6 +725,7 @@ public function fixTree($root = null): int $dictionary = $this->model ->newNestedSetQuery() + ->when($callback, $callback) ->when($root, function (self $query) use ($root) { return $query->whereDescendantOf($root); }) @@ -711,9 +740,9 @@ public function fixTree($root = null): int /** * @param NodeTrait|Model $root */ - public function fixSubtree($root): int + public function fixSubtree($root, ?Closure $callback = null): int { - return $this->fixTree($root); + return $this->fixTree($root, $callback); } /** diff --git a/tests/GlobalScopeTest.php b/tests/GlobalScopeTest.php new file mode 100644 index 0000000..40c99ae --- /dev/null +++ b/tests/GlobalScopeTest.php @@ -0,0 +1,200 @@ +increments('id'); + $table->string('name'); + $table->softDeletes(); + NestedSet::columns($table); + }); + + $data = include __DIR__.'/data/categories.php'; + + DB::table('categories')->insert($data); + + $this->resetAutoIncrement('categories'); + + Category::resetActionsPerformed(); + SelectScopedCategory::resetActionsPerformed(); + FilteredCategory::resetActionsPerformed(); + } + + protected function assertTreeNotBroken(string $table = 'categories'): void + { + $this->assertFalse(Category::isBroken(), "The tree structure of $table is broken!"); + } + + // --------------------------------------------------------------------- + // #595 — columnPatch must render a zero offset as "+0", not concatenate it. + // --------------------------------------------------------------------- + + public function test_column_patch_renders_zero_height_as_addition(): void + { + $sql = $this->invokeColumnPatch('"_lft"', ['height' => 0, 'cut' => 5]); + + $this->assertStringContainsString('+0', $sql); + $this->assertStringNotContainsString('"_lft"0', $sql); + } + + public function test_column_patch_renders_zero_distance_as_addition(): void + { + $sql = $this->invokeColumnPatch('"_lft"', [ + 'distance' => 0, + 'height' => 0, + 'lft' => 1, + 'rgt' => 2, + 'from' => 1, + 'to' => 2, + ]); + + $this->assertStringContainsString('+0', $sql); + $this->assertStringNotContainsString('"_lft"0', $sql); + } + + private function invokeColumnPatch(string $col, array $params): string + { + $builder = Category::query(); + + $method = new ReflectionMethod(QueryBuilder::class, 'columnPatch'); + $method->setAccessible(true); + + $expression = $method->invoke($builder, $col, $params); + + return (string) $expression->getValue($builder->getQuery()->getGrammar()); + } + + // --------------------------------------------------------------------- + // #513 — moving a node must not break when a global scope adds columns. + // --------------------------------------------------------------------- + + public function test_node_data_is_robust_against_select_adding_global_scope(): void + { + // getNodeData must return only lft/rgt, in order, regardless of the + // extra column the global scope selects. + $data = (new SelectScopedCategory) + ->newNestedSetQuery() + ->getNodeData(3); + + $this->assertSame(['_lft', '_rgt'], array_keys($data)); + $this->assertEquals([3, 4], array_values($data)); + } + + public function test_moving_node_with_select_adding_global_scope(): void + { + // "apple" (3) has a next sibling "lenovo" (4); moving it down used to + // throw a TypeError from "$rgt - $lft" because getNodeData returned the + // scope's extra column in the lft/rgt positions. + $node = SelectScopedCategory::query()->findOrFail(3); + + $this->assertTrue($node->down()); + + $this->assertTreeNotBroken(); + $this->assertEquals(5, $node->fresh()->getLft()); + } + + // --------------------------------------------------------------------- + // #536 — tree checks/fixes accept a callback to customise the query. + // --------------------------------------------------------------------- + + public function test_is_broken_is_disrupted_by_an_interfering_global_scope(): void + { + // The integrity checks self-join the table, so an unqualified column in + // a global scope becomes ambiguous and the check cannot run at all. + $this->expectException(QueryException::class); + + FilteredCategory::isBroken(); + } + + public function test_is_broken_callback_can_drop_global_scope(): void + { + $this->assertFalse(FilteredCategory::isBroken(function ($query) { + $query->withoutGlobalScopes(); + })); + } + + public function test_count_errors_callback_can_drop_global_scope(): void + { + $errors = FilteredCategory::countErrors(function ($query) { + $query->withoutGlobalScopes(); + }); + + $this->assertSame(0, array_sum($errors)); + } + + public function test_get_total_errors_callback_can_drop_global_scope(): void + { + $total = FilteredCategory::getTotalErrors(function ($query) { + $query->withoutGlobalScopes(); + }); + + $this->assertSame(0, $total); + } + + public function test_fix_subtree_callback_can_drop_global_scope(): void + { + $root = FilteredCategory::withoutGlobalScopes()->findOrFail(1); + + $changed = FilteredCategory::fixSubtree($root, function ($query) { + $query->withoutGlobalScopes(); + }); + + $this->assertSame(0, $changed); + $this->assertTreeNotBroken(); + } + + public function test_fix_tree_callback_can_drop_global_scope(): void + { + // With the scope active, fixTree only sees a partial tree and would + // "repair" perfectly valid nodes. Dropping the scope leaves it intact. + $changed = FilteredCategory::fixTree(null, function ($query) { + $query->withoutGlobalScopes(); + }); + + $this->assertSame(0, $changed); + $this->assertTreeNotBroken(); + } + + // --------------------------------------------------------------------- + // #300 — withDepth honours global scopes removed from the outer query. + // --------------------------------------------------------------------- + + public function test_with_depth_uses_global_scope_by_default(): void + { + // "galaxy" (8) sits under store > mobile > samsung. With "mobile" + // hidden, the depth subquery counts one fewer ancestor. + $node = FilteredCategory::withDepth()->findOrFail(8); + + $this->assertEquals(2, $node->depth); + } + + public function test_with_depth_respects_removed_global_scopes(): void + { + // Removing the scope on the outer query must also remove it from the + // depth subquery, so the hidden ancestor is counted again. + $node = FilteredCategory::withoutGlobalScopes() + ->withDepth() + ->findOrFail(8); + + $this->assertEquals(3, $node->depth); + } +} diff --git a/tests/Models/FilteredCategory.php b/tests/Models/FilteredCategory.php new file mode 100644 index 0000000..a5da0f1 --- /dev/null +++ b/tests/Models/FilteredCategory.php @@ -0,0 +1,35 @@ +where('name', '!=', 'mobile'); + }); + } + + public static function resetActionsPerformed(): void + { + static::$actionsPerformed = 0; + } +} diff --git a/tests/Models/SelectScopedCategory.php b/tests/Models/SelectScopedCategory.php new file mode 100644 index 0000000..29b8032 --- /dev/null +++ b/tests/Models/SelectScopedCategory.php @@ -0,0 +1,38 @@ +addSelect('*')->selectRaw('1 as extra_attribute'); + }); + } + + public static function resetActionsPerformed(): void + { + static::$actionsPerformed = 0; + } +} diff --git a/tests/QueryScopesTest.php b/tests/QueryScopesTest.php new file mode 100644 index 0000000..1ed98f3 --- /dev/null +++ b/tests/QueryScopesTest.php @@ -0,0 +1,187 @@ +increments('id'); + $table->string('name'); + $table->softDeletes(); + NestedSet::columns($table); + }); + + DB::table('categories')->insert(include __DIR__.'/data/categories.php'); + + $this->resetAutoIncrement('categories'); + + Category::resetActionsPerformed(); + } + + protected function findCategory(string $name): Category + { + return Category::whereName($name)->first(); + } + + protected function assertTreeNotBroken(): void + { + $this->assertFalse(Category::isBroken()); + } + + // ---- Leaf scopes ---------------------------------------------------- + + public function test_where_is_leaf_returns_only_leaves(): void + { + $names = Category::whereIsLeaf()->pluck('name')->all(); + + sort($names); + + $this->assertSame( + ['apple', 'galaxy', 'lenovo', 'lenovo', 'nokia', 'sony', 'store_2'], + $names + ); + } + + public function test_leaves_returns_leaf_collection(): void + { + $this->assertCount(7, Category::leaves()); + } + + // ---- Parent / children scopes -------------------------------------- + + public function test_has_parent_excludes_root_nodes(): void + { + // Every node except the two roots (store, store_2) has a parent. + $this->assertEquals(9, Category::hasParent()->count()); + } + + public function test_has_children_returns_only_branch_nodes(): void + { + // store, notebooks, mobile and samsung have children. + $this->assertEquals(4, Category::hasChildren()->count()); + } + + // ---- Next / prev across subtree boundaries ------------------------- + + public function test_get_next_node_crosses_subtree_boundary(): void + { + // galaxy has no next sibling, so the next node is its uncle "sony". + $next = $this->findCategory('galaxy')->getNextNode(); + + $this->assertEquals('sony', $next->name); + } + + public function test_get_prev_node_crosses_subtree_boundary(): void + { + // The node immediately before galaxy is its parent "samsung". + $prev = $this->findCategory('galaxy')->getPrevNode(); + + $this->assertEquals('samsung', $prev->name); + } + + // ---- Inclusive descendant / ancestor queries ----------------------- + + public function test_descendants_of_excludes_self(): void + { + $this->assertCount(5, Category::descendantsOf($this->findCategory('mobile')->id)); + } + + public function test_descendants_and_self_includes_node(): void + { + $this->assertCount(6, Category::descendantsAndSelf($this->findCategory('mobile')->id)); + } + + public function test_ancestors_and_self_includes_node(): void + { + // galaxy's ancestors are store > mobile > samsung, plus galaxy itself. + $this->assertCount(4, Category::ancestorsAndSelf($this->findCategory('galaxy')->id)); + } + + public function test_where_not_descendant_of_excludes_subtree(): void + { + // 11 nodes minus mobile's 5 descendants (mobile itself is not a + // descendant, so it stays). + $this->assertEquals(6, Category::whereNotDescendantOf($this->findCategory('mobile')->id)->count()); + } + + public function test_or_where_descendant_of_unions_subtrees(): void + { + $count = Category::whereDescendantOf($this->findCategory('notebooks')->id) + ->orWhereDescendantOf($this->findCategory('mobile')->id) + ->count(); + + // notebooks has 2 descendants, mobile has 5. + $this->assertEquals(7, $count); + } + + // ---- Inclusive predicates ------------------------------------------ + + public function test_is_self_or_descendant_of(): void + { + $mobile = $this->findCategory('mobile'); + $galaxy = $this->findCategory('galaxy'); + + $this->assertTrue($galaxy->isSelfOrDescendantOf($mobile)); + $this->assertTrue($galaxy->isSelfOrDescendantOf($galaxy)); + $this->assertFalse($mobile->isSelfOrDescendantOf($galaxy)); + } + + public function test_is_self_or_ancestor_of(): void + { + $mobile = $this->findCategory('mobile'); + $galaxy = $this->findCategory('galaxy'); + + $this->assertTrue($mobile->isSelfOrAncestorOf($galaxy)); + $this->assertTrue($mobile->isSelfOrAncestorOf($mobile)); + $this->assertFalse($galaxy->isSelfOrAncestorOf($mobile)); + } + + // ---- Misc helpers --------------------------------------------------- + + public function test_get_bounds_returns_lft_and_rgt(): void + { + $this->assertEquals([3, 4], $this->findCategory('apple')->getBounds()); + } + + public function test_prepend_to_node_inserts_as_first_child(): void + { + $mobile = $this->findCategory('mobile'); + + $node = new Category(['name' => 'huawei']); + $node->prependToNode($mobile)->save(); + + $this->assertTrue($node->isChildOf($mobile)); + $this->assertEquals($mobile->getLft() + 1, $node->getLft()); + $this->assertTreeNotBroken(); + } +}