Repository navigation
Improve Nested Set correctness and scale eager loading and repair - #58
Merged
Merged
Conversation
The node tests ran only on SQLite with integer keys and unprefixed tables. The shared cases now live in an abstract NodeTestBase with two concrete classes: NodeTest for integer keys and NodeUuidTest for UUID primary and parent keys, matching Aimeos' test layout. Both use a prefixed connection, so the raw nested set SQL is checked against prefixed tables. Cases that only make sense for integer keys, such as a zero parent key, stay in NodeTest. Thin subclasses in tests/Integration/NestedSet/Database run both classes on MySQL, MariaDB and Postgres. Each class migrates its fixture schema once. ResetRefreshDatabaseState clears the migration state at both class boundaries, so the integer and UUID schemas never reuse each other's tables and a later class still migrates its own. The table one test used to create in the middle of a test is now a fixture migration. Rebuilding the schema before every test cost about 0.6 seconds per test on MySQL and MariaDB. Seeding moved from setUp() into afterRefreshingDatabase(), which runs inside the test coroutine and its transaction; MySQL failed when it ran outside the coroutine. The Postgres sequence reset now uses the prefixed table name. Several assertions depended on row order that the query does not set, which held on SQLite but not reliably on other databases. Tests that check which rows come back now compare sets. Tests that pick particular rows order the query with defaultOrder(). The root used to check that a root is not a leaf could previously be store_2, which is a leaf. The next-node test now calls getNextNode(), matching the previous-node test. Expected SQL now uses the active grammar's quoting, and expected error messages use the active connection name. Helpers and data providers have docblocks and typed parameters, and callbacks declare their types. Ports the test changes from Aimeos #2 (UUID keys), 28e6a4e066 (test layout), 1e225e5b14 (removal of the all() helper) and #21 (all supported databases), and the multi-database test setup from Lunar #2, at Aimeos 90ea384feb and Lunar 12419691f0. Validation: NodeTest and NodeUuidTest pass on SQLite, MySQL 8.4, MariaDB 11 and Postgres 17. Each database directory passes serially and under ParaTest, as does the full tests/NestedSet suite, and php-cs-fixer reports no changes.
The scoped node tests ran only on SQLite with integer keys and unprefixed tables, and rebuilt the schema before every test. They now follow the node test layout: an abstract ScopedNodeTestBase with ScopedNodeTest for integer keys and ScopedNodeUuidTest for UUID primary and parent keys, matching Aimeos' test layout. Both use a prefixed connection, and thin subclasses in tests/Integration/NestedSet/Database run both classes on MySQL, MariaDB and Postgres. Each class migrates its fixture schema once, with ResetRefreshDatabaseState clearing the migration state at both class boundaries. Two tests changed the schema inside the test: one created a table with a nullable scope column and one added a deleted_at column. On MySQL and MariaDB that schema change commits the test transaction, so both are fixture migrations now. The two tests stay in ScopedNodeTest, since their fixtures use integer keys. Seeding runs in afterRefreshingDatabase(), inside the test coroutine and its transaction. The Postgres sequence reset moved to ScopedNodeTest and now uses the prefixed table name. Two tests took the first row of a query with no order, which held on SQLite but not reliably on other databases. They now order the query with defaultOrder(). Expected error messages use the fixture class, and callbacks and data providers have types and docblocks. Ports the scoped test changes from Aimeos #2 (UUID keys), 28e6a4e066 (test layout) and #21 (all supported databases), and the multi-database test setup from Lunar #2, at Aimeos 90ea384feb and Lunar 12419691f0. Validation: ScopedNodeTest and ScopedNodeUuidTest pass on SQLite, MySQL 8.4, MariaDB 11 and Postgres 17. Each database directory passes serially and under ParaTest, as does the full tests/NestedSet suite, and php-cs-fixer reports no changes.
Hypervel already caches soft-delete detection per model class through Model::isSoftDeletable(), which HasNode::usesSoftDelete() uses, and caches node trait detection per class in NestedSet::isNode(). Aimeos added a test for each cache. NestedSetCacheTest ports both. It checks that each model or plain class is recorded once with its result, and that NestedSet::flushState() clears the node class cache. Ports Aimeos fb1545e9d9 and 622a2bbf99 (cache tests) and 1fd8b7ccc0 (no deprecated reflection calls) at Aimeos 90ea384feb. Validation: NestedSetCacheTest and the tests/NestedSet suite pass serially and under ParaTest, and php-cs-fixer reports no changes.
HasNode::newEloquentBuilder() used the #[UseEloquentBuilder] attribute but ignored a model's static $builder property, so a model declaring a custom nested set builder that way still received the base QueryBuilder. Naming the nested set QueryBuilder itself in the attribute also threw, because the check required a subclass. Builder resolution now follows the attribute, then a non-default static $builder, then the nested set QueryBuilder. Any resolved class must be the nested set QueryBuilder or extend it; otherwise a LogicException names the model and the required builder. The builder tests cover the attribute, the attribute naming the nested set builder, the property, attribute precedence over the property, and rejection of incompatible builders from either source. The node class cache test moves to NestedSetCacheTest, which covers the same cache. Ports Aimeos ddbd084faf (respect #[UseEloquentBuilder]) at Aimeos 90ea384feb, extended to the builder property. Validation: NestedSetTest and the tests/NestedSet suite pass serially and under ParaTest, php-cs-fixer reports no changes and composer analyse reports no errors.
The nested set schema indexed the left bound alone, so ancestor queries had to read each candidate row to check its right bound. The left-bound index now also holds the right bound, matching Aimeos' layout: the schema helpers create (scope..., _rgt), (scope..., _lft, _rgt) and (scope..., parent_id, _lft), and dropNestedSet() drops the same set. Measured on 50,000-node random trees, scoped and unscoped, on MySQL 8.4, MariaDB 11, Postgres 17 and SQLite: ancestor reads are about 1.5x faster on MySQL and MariaDB and about 3x faster on Postgres and SQLite, and SQLite move ranges are about 4x faster. Large gap writes are about 5 to 15 percent slower on MySQL and SQLite, with no measurable change on MariaDB and Postgres. On MySQL, the depth lookup for a parent with no loaded depth rises from about 0.6 to 5 ms. Descendant, child, sibling and max reads are unchanged. The schema test ran only on SQLite, and NestedSetDatabaseTestCase repeated part of it on each database. NestedSetSchemaTest now runs on all four databases with a prefixed connection and DatabaseMigrations. It checks every macro's key, bound and depth column types and the exact non-primary indexes, with and without scope columns, and checks that dropNestedSet() leaves only the original columns and no extra indexes. The duplicated schema tests and tables leave NestedSetDatabaseTestCase. The test bases read the default connection name with the typed config getter. Hypervel keeps its typed schema macros, which add the depth column and indexes, instead of Aimeos' key column and type parameters and its separate depth and index macros. The README records the difference, the service provider notes where the omitted macros would sit, and the guide describes the indexes and their write cost. Ports Aimeos 52aee35ad3, 05f77e28e5, f504d66247 and a5c0d0c92c (indexes), 90ea384feb (schema test) and #2 (UUID schema), Lunar #2 (ServiceProviderTest, native types and the getDefaultColumns() return type), at Aimeos 90ea384feb and Lunar 12419691f0. Validation: NestedSetSchemaTest passes on SQLite, MySQL 8.4, MariaDB 11 and Postgres 17, as do the node, scoped and database test classes. Each database directory passes serially and under ParaTest, as does the full tests/NestedSet suite. php-cs-fixer reports no changes and composer analyse reports no errors.
The package is ported from aimeos/laravel-nestedset, whose MIT license notice in its README reads "(c) 2026 Aimeos and contributors" alongside Alexander Kalnoy. The MIT license requires that notice in copies, so the Nested Set license now includes the Aimeos line between the original author's and Hypervel's. Source: Aimeos README license section at 90ea384feb.
A save cleared its pending structural action before the action could fail, so a retry no longer did what was asked. A new scoped node saved without its scope lost the action name and failed again after the scope was set. A node rejected for a parent in another tree became a root on the next save. After an observer veto and the documented rollback, saving a moved node again wrote only its new parent_id, leaving the tree with a wrong parent. callPendingAction() now keeps the whole pending action until it runs. save() and saveOrIgnore() restore that action when their own save returns false or throws, unless an observer queued a new one. A successful save never touches the pending action, so a save repeated from a created or updated observer does not replay it, and an action an observer queues waits for the next save. The guide's transaction section now covers saves that return false through saveOrIgnore() and the retry. Other fixes: - Internal structural reloads read the base query on the write connection, so they no longer fire retrieved listeners, and still throw ModelNotFoundException for a missing row. - Assigning parent_id through setAttribute() returns the model. - rawNode() and setDepth() store a missing depth as 0 instead of writing null into the non-null depth column. - The protected hooks use upstream's names again: callPendingAction() and assertNodeExists(), which keeps Hypervel's positive-bounds check. Only null marks a root. Upstream also treats 0 and '' as root parent IDs; here they stay real parent keys, so the README records the difference and setParentId() notes it. Upstream's root tests are ported. The scoped parent-before-scope tests use upstream's names and order. Ports Aimeos 9da1a8fb86 (direct dispatch), f3a0a5aa51 (setDepth() default), e8a74e20fd (root tests only) and b5faf0167e (test names), plus the creation and raw-node depth parts of #10, at Aimeos 90ea384feb. Validation: NodeTest, NodeUuidTest, ScopedNodeTest and ScopedNodeUuidTest pass on SQLite, MySQL 8.4, MariaDB 11 and PostgreSQL 17. The saveOrIgnore() test runs only on SQLite and PostgreSQL, whose grammars support it. Each database's Nested Set directory and tests/NestedSet pass serially and under ParaTest. php-cs-fixer reports no changes and composer analyse reports no errors.
The package now includes tests ported from lunarphp/nestedset, whose MIT license adds "Copyright (c) 2026 Neon Digital (maintenance for Lunar)" to Alexander Kalnoy's notice. The MIT license requires that notice in copies, so the Nested Set license now includes the Lunar line between the Aimeos and Hypervel lines. Source: Lunar LICENSE.md at 12419691f0.
…extra reload getNodeData() passed its columns to first(), which ignores them once a global scope has selected columns. A model whose global scope added columns got that projection back instead of _lft, _rgt and depth, so moving one of its nodes failed. Its column and key references were also unqualified, so a self-join or a global scope joining another table with an id column made the lookup ambiguous. getNodeData() now calls select() with the model's qualified structural columns and filters on the qualified key, keeping the query's visibility constraints. Other changes: - Model::create() re-read every new node after inserting it. It now re-reads only when it created children, whose descendants widen the node after the last append refreshed it. Upstream dropped the reload entirely, which leaves stale bounds in that case; a test covers it. - columnPatch() rendered a zero height or distance as "_lft"0, which is invalid SQL. It now renders "+ 0", as both forks do. The package's own callers skip zero offsets before reaching it. - moveNode() gets back upstream's two comments. - The README records that the query builder's getDepth($position) is depthForPosition($position), which returns the depth of a node inserted at that position rather than the enclosing node's depth. Tests: Aimeos's six move tests replace the original package's testCategoryMovesDown and testCategoryMovesUp, and Lunar's global-scope lookup, move and zero-offset tests are ported. The subtree-depth test now checks persisted depth. The database-only subtree-depth and savepoint-rollback tests and the SQLite-only transaction-retry test move into the driver matrix, which covers each on every database. Ports Aimeos 3d866e0 (#5), 14374a7 (#6), the create() part of a5c2d4f866 and the move tests from 28e6a4e066 at Aimeos 90ea384feb, and the getNodeData() and columnPatch() parts of Lunar 7a81875 at Lunar 12419691f0. Validation: NodeTest, NodeUuidTest, ScopedNodeTest, ScopedNodeUuidTest and NestedSetDatabaseTest pass on SQLite, MySQL 8.4, MariaDB 11 and PostgreSQL 17 where applicable. Each database's Nested Set directory and tests/NestedSet pass serially and under ParaTest. php-cs-fixer reports no changes and composer analyse reports no errors.
…concile query scope tests whereDescendantOf() with a key looked up the node's bounds through a fresh model query. On a read/write split connection that lookup always went to the replica, even when the outer query used useWritePdo(). It now uses the builder's own lookup query, as depthForPosition() and moveNode() already do, so the lookup follows the query's connection choice with no extra query. The README now states that the depth column is required and that withDepth() reads the stored depth, so global scopes that hide ancestors do not change a node's depth. Upstream reconciliation (Aimeos master 90ea384feb, Lunar main 12419691f0): - aimeos/laravel-nestedset#25 (c861196): ancestor and descendant constraints already accept a node or a key. - 7fc60fcb00: the parent ID was already qualified. Its upstream test, testWhereIsRootQualifiesParentId, now holds the whereIsRoot(), withoutRoot() and hasParent() self-join assertions. - 2dbf21a582: withDepth() always reads the stored column, so there is no schema check or computed fallback. The upstream test is ported. - 3cbd0fa6e0: Hypervel's scalar position subqueries already select one row by primary key without global scopes; its test was already present. - lunarphp/nestedset#3 (7a81875): QueryScopesTest's missing cases move into the shared node tests, so they run on all four databases. The previous-node boundary case now checks sony to galaxy; upstream's galaxy to samsung is the parent case testRetrievesPrevNode covers. Its computed-depth withDepth() cases do not apply to stored depth. A test covers the remaining ancestor and descendant shortcut methods. The integration database test drops its union ordering half, which the driver matrix covers through testDefaultOrderClearsPreviousUnionOrderBindings. Validation: NodeTest and NodeUuidTest on SQLite, MySQL 8.4, MariaDB 11 and PostgreSQL 17; scoped node tests, NestedSetDatabaseTest and NestedSetReadWriteTest; tests/NestedSet serially and with ParaTest; php-cs-fixer and composer analyse.
linkNodes() and toTree() cleared every node's parent and children
relations before linking. A node whose parent was not in the collection
lost an eager-loaded parent, so accessing it ran another query. Linking
now replaces children and in-collection parents only; roots still get a
null parent. A regression test covers a parent loaded with with('parent')
outside a descendant collection.
Reconcile the collection tests with Aimeos: port the leaf children,
collection-order and attribute-equality assertions, and rename the
serialization, deep flat tree and multiple-root tests to the upstream
names. The flat tree test now checks the complete key order.
Port the relation count hash refactor and the parent stub comment. Keep
serializing a linked node's parent stub like any loaded relation, as
Aimeos does since #23; the original package and Lunar hide parent from
serialization entirely.
Document that null, 0 and an empty string passed to toTree() or
toFlatTree() select nodes with that parent ID; Aimeos infers the root
for 0 and an empty string and rejects null.
Upstream: aimeos/laravel-nestedset master 90ea384feb (#8 6045762,
a67c93f, #23 f5c2c8b, 97b0a6e, 12d81c2, 3b906ef, ea791eb, cf37f7b,
dfa0681, 0b4388b).
Validation: NodeTest, NodeUuidTest, ScopedNodeTest and ScopedNodeUuidTest
on SQLite, MySQL 8.4, MariaDB 11 and PostgreSQL 17; tests/NestedSet
serially and with ParaTest; php-cs-fixer; composer analyse.
Eager loading ancestors, descendants or siblings for many parents was slow or failed. Matching compared results with every parent: ancestors for all nodes of an 11,111-node tree took 50 s, and descendants ordered by name took over 58 s. Each parent added an "or" group, so the all-node ancestor query took 28 s on MySQL, and SQLite rejected loads of about 1,000 or more parents because it limits expressions to 1,000 levels. Constraints: - Ancestors use one group per scope: the reduced parents' bounds and a balanced CASE that finds the nearest parent right bound at or after a row's left bound. - Descendant and sibling groups are nested in balanced "or" groups of at most 64. Parents without room for descendants are skipped, so a load of only leaves runs no query. - Integer bounds are inlined, as whereIntegerInRaw() does. Matching: matchMany() receives the persisted parents together, replacing Aimeos's per-parent hooks. Ancestors use one stack sweep per scope, descendants a binary search over sorted buckets, and siblings their scope and parent buckets, shared by siblingsAndSelf parents. A single parent uses a linear filter. Custom result order is restored only when results were re-sorted. Constraint hooks receive the base query builder. On the same tree, all-node ancestor matching takes 77 ms, name-ordered descendant matching 134 ms, and the MySQL ancestor query 176 ms. Loads of more than 1,000 parents or scopes pass on SQLite. getAncestors(), getDescendants() and getSiblings() keep running fresh queries instead of returning or replacing loaded relations (Aimeos 30c56b4); a test covers loaded relations staying unchanged. The README records this and the hook changes. Port the upstream relation tests under their names, folding in Hypervel's duplicates, and add tests for leaf-only descendant loads, more than 1,000 parent intervals and scopes, and fresh get*() queries. Upstream: aimeos/laravel-nestedset master 90ea384feb (#3 3b14873, #4 1d34707, #15 f7e4a72, #22 c2ed517, 0121850, 30c56b4, 3588a2a, 7b19f85, a06877c, 32f5f32, 0d3603a, 1685638, 92e64e7, 5087f54, and the eager order test from 3e4f690); lazychaser/laravel-nestedset v7 4e9ad66a3c (#606 10ac72d). Validation: NodeTest, NodeUuidTest, ScopedNodeTest and ScopedNodeUuidTest on SQLite, MySQL 8.4, MariaDB 11 and PostgreSQL 17; tests/NestedSet serially and with ParaTest; php-cs-fixer; composer analyse.
ApplicationTest's default-migrations cases left TESTBENCH_WITHOUT_DEFAULT_MIGRATIONS in $_SERVER and $_ENV, so later tests in the same worker dropped the default migration path. In default order, LoadMigrationsFromArrayTest::itCanRegisterMigrations failed; in reverse order, the default-migrations case itself failed. A full parallel run failed the same way once. Testbench writes the configured environment through the Env repository's immutable writer, which records the keys it loaded. While restoring the suite's masked APP_ENV, createApplication() then calls Env::flushRepository(). The test's Env::forget() cleared through a new writer, which treats the value as externally defined and keeps it. Wrap the case in withEnvironmentValue(), which restores getenv(), $_SERVER and $_ENV even when application creation throws. Validation: tests/Testbench/Foundation in default and reverse order; composer test:testbench; ParaTest over tests/Testbench; php-cs-fixer.
…llow it Hard deletes removed the node's row before its descendants, so a restricting foreign key on parent_id rejected every subtree delete. Tree upkeep ran in five boot listeners, and a custom $dispatchesEvents listener that returned a value skipped them: a save could leave null bounds, and a delete could leave descendants with crossing intervals. Soft deletes and restores skipped descendants hidden by ordinary global scopes, and forceDestroy() of a node together with its descendant threw ModelNotFoundException. fireModelEvent() now owns the upkeep for saving, deleting, deleted, restoring and restored, keeping the parent's handling of quiet operations, $halt and listener results: - deleting reloads the node's bounds, then notifies observers. Once they allow a hard delete, descendants are removed in descending _lft order before the node's own row, because MySQL and MariaDB check a restricting parent key per row. A veto leaves the subtree unchanged. A row already removed by an ancestor's delete returns false, so destroy() counts only the rows it deleted. - deleted closes the gap after hard deletes and trashes descendants after soft deletes. - Cascades and restores use the scope-free nested set query. With shouldFireDescendantEvents(), restoring also runs through descendant model events, parents first, in the same bounded chunks as deletion (children first). getDescendantDeleteChunkSize() becomes getDescendantChunkSize(), a short chunk ends the loop, and a vetoed descendant throws. Transactions stay with the caller: deleteOrFail() or DB::transaction() roll back a partial failure. SoftDeletes::forceDelete() now resets $forceDeleting when delete() throws, so a later delete() on the same model stays a soft delete. Restore keeps the stored deletion time as its threshold, so descendants deleted in the same second need no startOfSecond() adjustment. Aimeos's skipped reload for soft deletes is not ported: the soft cascade selects descendants by the node's bounds. Tests cover a restricting parent key on every database (NodeTest now enables SQLite foreign keys), a deleting veto registered after boot, rollback after partial failures, hidden descendants, custom event results, destroy() across a node and its descendant, evented restore and its veto, and the timestamp boundary. Upstream's chunked delete test is ported with an exact query count. The README records the opt-in events, the renamed hook and the delete timing; the guide covers parent keys, evented restore and transactions. Upstream: aimeos/laravel-nestedset master 90ea384feb (#13 a681d9c, #21 82044df, #24 fecfae3, 0a48675, bfcaaa8, a68b411, 4a25401, a0f3ea1, 0d0502d). Validation: NodeTest, NodeUuidTest, ScopedNodeTest, ScopedNodeUuidTest and NestedSetDatabaseTest on SQLite, MySQL 8.4, MariaDB 11 and PostgreSQL 17; tests/NestedSet serially and with ParaTest; DatabaseEloquentSoftDeletesIntegrationTest; php-cs-fixer; composer analyse.
fixTree() and fixSubtree() hydrated every node they selected. Repairing a healthy 111,111-node tree took 2.7 s and 367.5 MB on MySQL and fired a retrieved event per node. countErrors(), getTotalErrors() and isBroken() built fresh queries, so they ignored the caller's useWritePdo(). rebuildTree(delete: true) deleted removed nodes in no set order, and MySQL and MariaDB rejected deleting a parent before its children under a restricting foreign key on parent_id. Repair now reads plain rows with the same columns, on the write connection and in default order, and records each node's placement in one traversal. Rows whose bounds, parent and depth already match are left alone. The others are hydrated through the model, assigned and saved, so saving and saved observers and extraColumns still apply. The healthy tree now takes 358 ms and 73.2 MB with no retrieved events. A damaged 11,111-node tree with 11,110 saves drops from 9.5 s and 60.8 MB to 7.0 s and 7.4 MB on MySQL, and from 6.4 s to 5.0 s on PostgreSQL. When a subtree grows, rows shifted by the gap update are compared with their shifted values, so rebuild no longer saves unchanged children a second time. The returned count still adds the gap update's rows to the repaired nodes. fixSubtree() accepts null, as Aimeos's does. Diagnostics share the caller's useWritePdo() and keep Hypervel's SQL checks. Aimeos now counts errors with a PHP row scan; on the 111,111-node tree it takes 1.25 s on MySQL, MariaDB and PostgreSQL and 4.1 s on SQLite, using 234 MB. The SQL checks take 1.2 s, 0.86 s, 0.45 s and 0.38 s respectively, using 0.5 MB. Rebuild deletes removed nodes in descending _lft order. The README records the error-count differences from Aimeos and that repair hydrates only the nodes it saves. Upstream's diagnostics, repair and rebuild tests are ported under their names with Hypervel's error keys, folding in overlapping Hypervel tests. Lunar's global-scope cases are folded into the countErrors() and fixTree() global-scope tests; its scope-removal callbacks are not ported, because diagnostics and repair already use the scope-free nested set query. New tests cover diagnostics on the writer, rebuild deletion under a restricting parent key, branching parent buckets, nested rebuild payloads, soft-deleted rebuild removals, the events repair fires, saves and counts when a subtree grows, and scoped orphan repair. The integration case's composite diagnostics and UUID repair tests move into the scoped matrix. Upstream: aimeos/laravel-nestedset master 90ea384feb (#7 0ce31f3, e81300e, 449da3c, 8e6198c, 9c6fb87, 2b591d4, c9a58b2, b09ef00, 0ff8d44, 4e03763, bbf2c6c, a6c59fd, 3e4f690); lunarphp/nestedset main 12419691f0 (#3 3393853). Validation: NodeTest, NodeUuidTest, ScopedNodeTest, ScopedNodeUuidTest and NestedSetDatabaseTest on SQLite, MySQL 8.4, MariaDB 11 and PostgreSQL 17; tests/NestedSet with ParaTest; php-cs-fixer; composer analyse.
Lunar's root scoped node tests match the original package's ScopedNodeTest, and each of those tests already exists in the scoped test base under its upstream name. Upstream keeps testRebuildsTree commented out with no assertions, and Aimeos dropped it. - testRebuildsTree rebuilds one menu with delete enabled. It checks that only that menu's other nodes are deleted, and that a menu_id in the payload does not move the new child into another menu. - A root created in an empty scope starts its own tree at [1, 2]. - The soft-delete restore test now checks, right after the delete, that the other menu's node inside the deleted bounds stays active. The final restore assertions alone would not catch an unscoped cascade. - The separate integration test case and its MySQL, MariaDB, Postgres and SQLite wrappers are removed. The node, UUID and scoped test matrix covers its integer and UUID trees, depth, scope isolation and soft-delete restore on every driver. - ensureConcreteNestedSetScope() drops its model parameter, which no caller passed. - The README records the getScopeAttributes() return type and ensureSameTree() differences from Aimeos. Upstream: lunarphp/nestedset main 12419691f0 (root 60101d3d), lazychaser/laravel-nestedset v7 4e9ad66a3c. Validation: NodeTest, NodeUuidTest, ScopedNodeTest, ScopedNodeUuidTest and NestedSetSchemaTest on SQLite, MySQL 8.4, MariaDB 11 and PostgreSQL 17; ParaTest tests/NestedSet; php-cs-fixer; composer analyse.
…lain rows rebuildTree() created each new node with newInstance($scopeAttributes), which mass assigns. A scope attribute missing from $fillable was dropped or rejected, so the new node could not join the rebuilt tree. The scope is now set as raw attributes over the fresh model's attributes, so model defaults remain and the rebuilt tree's scope takes precedence. Aimeos, the original package and Lunar mass assign new rebuild nodes the same way. depthForPosition() read the depth through the Eloquent value(), which hydrates a model and fires retrieved listeners. It now reads through the base query. Aimeos's getDepth() hydrates the same way. Tests cover rebuilding a scope whose scope attribute is guarded and has a model default, and a low-level move that derives its depth without firing retrieved. Upstream: aimeos/laravel-nestedset master 90ea384feb, lazychaser/laravel-nestedset v7 4e9ad66a3c, lunarphp/nestedset main 12419691f0. Validation: NodeTest, NodeUuidTest, ScopedNodeTest, ScopedNodeUuidTest and NestedSetSchemaTest on SQLite, MySQL 8.4, MariaDB 11 and PostgreSQL 17; ParaTest tests/NestedSet; php-cs-fixer; composer analyse.
Applications analyzed with PHPStan could not see Nested Set builder methods: HasNode did not bind its builder through HasBuilder, so calls such as Category::whereIsRoot() and Category::query()->root() were undefined, and relations and collections lost their model type. - QueryBuilder is generic over its model (aimeos/laravel-nestedset #9). Its collection queries return Collection<int, TModel>, and root() returns the model or null. - HasNode binds HasBuilder<QueryBuilder<static>>, so query(), static calls and custom builders keep the model. Its relation, builder and getter methods return typed relations, builders and collections. - BaseRelation and the ancestor, descendant and sibling relations relate a node to its own class and return the nested set collection. - The collection is generic. toTree() and toFlatTree() return integer-keyed collections. flattenTree() builds a node list that toFlatTree() wraps once, as toTree() does, instead of pushing each node onto the result. - types/NestedSet covers builders, static forwarding, a subclass, relations, collections, nullable getters, a custom builder and flattening a string-keyed collection. DescendantsRelation now notes why eager constraints keep per-parent ranges, and that SQLite still prepares thousands of disjoint ranges in quadratic time. The PHPStan config comment names HasNode. Upstream: aimeos/laravel-nestedset #9, reconciled at 90ea384feb. Validation: composer analyse (both configurations), php-cs-fixer, and ParaTest tests/NestedSet (811 tests).
Aimeos master is the only tracked Nested Set upstream. The original lazychaser/laravel-nestedset and lunarphp/nestedset are not tracked, because Aimeos carries their applicable fixes. The notes map upstream source, tests and README onto Hypervel's layout, keep the grouped method order (find upstream changes by method name), and direct ported cases into the shared test bases that also run on MySQL, MariaDB and PostgreSQL. Checkpoint fields stay unset until the reconciliation is complete.
PHPStan reported the builder macros registered by SoftDeletingScope and Scout's SearchableScope as undefined on builders and relations: withTrashed(), withoutTrashed(), onlyTrashed(), restore(), restoreOrCreate(), createOrRestore(), searchable() and unsearchable(). Static model calls fell back to the SoftDeletes @method tags, which return Builder<static> and drop a custom builder. - BuilderMacroResolver maps a model trait to an interface declaring its macros through the eloquentBuilderMacros parameter. The database extension maps SoftDeletes to SoftDeletingMacros. Scout's new extension.neon maps Searchable to SearchableMacros; it is listed in Scout's extra.phpstan.includes and in both components configurations. - Traits are found through parent classes and nested traits. Names match exactly, as Builder::hasMacro() does, so the extensions' method caches now keep the exact spelling. - Fluent macros return the builder or relation they run on, including custom builders. The others keep their declared returns: int, the model, or void. - Macros take precedence over same-named scopes, as in Builder::__call(), for builders, relations, static model calls and the named scope extension. Native builder methods still win. - SoftDeletes keeps Laravel's @method tags: registered extensions run before PHPDoc annotations, so the tags no longer decide the type. - The database and installation docs mention the soft deleting methods and the Scout extension. - types cover plain and custom builders, inherited and nested traits, relations including HasManyThrough, a legacy scope sharing a macro name, case sensitivity, a model without the trait and a soft-deleting nested set node. Validation: composer analyse (both configurations) on PHPStan 2.3.0, and the types configuration on 2.2.16; php-cs-fixer; tests/Database/PHPStan, Scout PackageMetadataTest and ComposerFileTest; composer validate for src/scout. Case-insensitive cache keys, scopes before macros and the previous model forwarding condition each fail the type fixtures.
The components analysis allowed PHPStan ^2.2.15, but 2.2.16 reports "Variable $actualMessage might not be defined" in InteractsWithExceptionHandling::assertThrows(). The variable is set in the catch block, and Assert::assertTrue($thrown) stops the method unless that block ran. PHPStan 2.3 follows this, so the code stays as it is and nothing is suppressed. The root development requirement (through Composer) and the database and foundation split packages' require-dev now use ^2.3. Foundation contains the reported code; database ships the PHPStan extension. This is a development tool minimum only: the types configuration, which loads the extensions, also passes on PHPStan 2.2.16. Validation: composer analyse (both configurations) on PHPStan 2.3.0; PackageManifestConsistencyTest, Database and Foundation PackageMetadataTest; composer validate for src/database and src/foundation.
The guide now covers what the Aimeos, original and Lunar READMEs document and Hypervel supports, checked against the current source. New sections cover adding nested set columns to an existing table, custom column names, deleting nodes, and transactions and concurrency. Other sections add relationship-based creation, neighbor moves, ancestor order, related-model subqueries, stored depth, flat trees, scoped key lookups and the soft-deleted leaf rule. A warning covers SQLite's slow query preparation when eager loading descendants for thousands of parents. Upstream advice that does not hold for Hypervel was not carried over: whereDescendantAndSelf() exists in no upstream source, eager loading needs no scoped query because constraints are grouped by each parent's scope, and the lock example waits with block() instead of skipping the write. The transaction examples throw when a structural save returns false, because the save has already updated other rows' bounds. The README names the HasNode trait in place of upstream's NodeTrait and drops an entry describing a correctness fix and a sentence describing a performance change, neither of which is a public difference. prevNodes()'s docblock no longer claims a reversed order. The custom-parent fixture becomes a custom-column table (lft, rgt, level, ancestor_id), with a test that creates, moves, deletes, diagnoses and repairs a tree through those columns. Validation: php-cs-fixer, composer analyse, NodeTest on SQLite, MySQL, MariaDB and PostgreSQL, and ParaTest tests/NestedSet.
Aimeos master is reconciled through 90ea384feb, including the source, tests and README, with pull request 25 as the last one examined.
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PR Summary by QodoNested Set: fix tree-consistency bugs, scale eager loading/repair, add generics
AI Description
Diagram
High-Level Assessment
Files changed (71)
|
Code Review by Qodo
1. Older tree migrations cannot roll back
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| { | ||
| $table->dropIndex([...$scopes, self::RGT]); | ||
| $table->dropIndex([...$scopes, self::LFT]); | ||
| $table->dropIndex([...$scopes, self::LFT, self::RGT]); |
There was a problem hiding this comment.
dropNestedSet() now drops an index on [_lft, _rgt], but tables created before this change have an index on _lft alone. After upgrading, rolling back an existing migration tries to drop a nonexistent index and fails before removing the columns.
Support both index layouts, or provide an upgrade step that runs before existing migrations can be rolled back.
Member
Author
There was a problem hiding this comment.
Not changing this. Hypervel 0.4 is unreleased, so no released version created the (scope..., _lft) index, and dropNestedSet() only needs to drop the indexes nestedSet() creates. Supporting both layouts would mean checking which index exists on every rollback, for tables no released version can have. Aimeos's dropColumnsIndex() likewise drops only its current layout.
There was a problem hiding this comment.
That context makes this intentional and valid: since Hypervel 0.4 is unreleased, no released migration can have produced the old _lft-only index, and dropColumns() only needs to reverse the layout created by the current nestedSet() helper. I withdraw this finding.
Comment on lines
+1292
to
+1294
| if ($relation->isNotEmpty()) { | ||
| $instance->refreshNode(); | ||
| } |
There was a problem hiding this comment.
When a created observer adds a default child through $model->children()->create(...), that child widens the stored parent through a separately loaded model. With no explicit children payload, this condition skips refreshing the model returned by create().
A new root still reports bounds [1, 2] instead of [1, 4]. isLeaf(), getDescendantCount(), and fresh descendant queries then give wrong results. Keep the refresh for observer-created children and add a test for this case.
Suggested change
| if ($relation->isNotEmpty()) { | |
| $instance->refreshNode(); | |
| } | |
| $instance->refreshNode(); |
Member
Author
There was a problem hiding this comment.
Not changing this. Any observer that changes the tree through another model instance leaves the saved model's bounds stale, and a plain save() with the same created observer has the same result. The package refreshes bounds where its own operations change them, including create() with a children payload, but it does not re-read every new node to pick up changes made elsewhere, which would add a query to every create. An observer that adds children can call refreshNode() on the parent. Aimeos's create() does not re-read the node either.
There was a problem hiding this comment.
That’s fair. The stale instance is caused by an observer mutating the tree through a different model instance, rather than by create()’s own child handling. Re-reading every newly created node would add a query to every create() for a case the package does not otherwise synchronize, and matching Aimeos’s behavior supports keeping this optimization.
I’m withdrawing this finding; an observer that performs such an external tree mutation should refresh the parent instance if it needs to use its bounds immediately.
…by alias
Assigning a new parent_id only queues the append; the new parent is
looked up when the save runs. A parent relation loaded before the
assignment stayed cached, so $node->parent returned the old parent
until the node was saved and refreshed. The parent_id mutator now
unsets the loaded relation, so the next access loads the new parent.
Assigning null already went through makeRoot(), which clears it, and
the raw setParentId() setter is unchanged.
getNodeData() qualified its columns with the model's table name, so a
query that selects from an alias (from('categories as c')) referenced
a table that is not in the statement. It now qualifies through the
builder, which uses the alias when one is set and still avoids
ambiguous columns in joined queries.
Validation: php-cs-fixer, composer analyse, NodeTest and NodeUuidTest
on SQLite, MySQL, MariaDB and PostgreSQL.
HasNode and HasBuilder both define newEloquentBuilder(), so a model that adds HasBuilder to type a custom builder fails when PHP loads the class with a trait method collision. The type fixture and the custom builder test model now keep HasNode's version with insteadof, so the model builds its Nested Set builder at runtime while PHPStan reads the builder type from HasBuilder. The test model loads that pattern for real. The guide's custom builder section shows the same pattern with a generic builder, and a collections example uses a literal key in place of a variable it never defined. Validation: php-cs-fixer, composer analyse, the types configuration and NestedSetTest.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This improves Nested Set correctness, performance and static analysis. It keeps tree changes consistent through failed saves and vetoed deletes, makes eager loading and repair scale to larger trees, and preserves model types across builders, relations and collections.
It also includes two framework changes.
SoftDeletes::forceDelete()now resets its state when the delete throws, and PHPStan now understands the builder macros added by soft deletes and Scout.The node and scoped node tests now run on MySQL, MariaDB and PostgreSQL as well as SQLite, with integer and UUID keys, against a prefixed connection.
Saving and moving nodes
parent_id. The pending action now stays until it runs, andsave()andsaveOrIgnore()put it back when the save returns false or throws, unless an observer queued a new one.retrievedlisteners and still throwModelNotFoundExceptionfor a missing row. The builder'sdepthForPosition()reads the same way.getNodeData()passed its columns tofirst(), which ignores them once a global scope has selected columns. Called directly on such a model, it returned the scope's columns instead of the bounds and depth. Its column and key references were also unqualified, so a join with anotheridcolumn made the lookup ambiguous. It now selects the structural columns and filters on the key, qualified with the query's table or alias, keeping the query's other constraints. The package's own moves were not affected: they pass the node's data in or use the scope-free lookup query.create()re-read every new node after inserting it. It now re-reads only when it created children, whose inserts widen the node after the last append refreshed it.columnPatch()rendered a zero offset as invalid SQL ("_lft"0). It now renders+ 0.rawNode()andsetDepth()store a missing depth as 0 instead of writing null into the non-null depth column.parent_idthroughsetAttribute()returns the model.parent_idforgets aparentrelation loaded before it. The new parent is only looked up when the node is saved, so$node->parentused to return the old parent until then.whereDescendantOf()with a key looked up the node through a fresh model query, which always read from the replica even when the outer query useduseWritePdo(). It now uses the builder's own lookup query.rebuildTree()mass assigned each new node's scope attributes, so a scope column missing from$fillablewas dropped and the node could not join the rebuilt tree. The scope is now set directly, over the model's defaults.$builderproperty is now honored after#[UseEloquentBuilder], and naming the Nested SetQueryBuilderitself in the attribute no longer throws. Any other builder must extend it.Deleting and restoring nodes
parent_idrejected every subtree delete. Descendants are now removed children first, in descending_lftorder because MySQL and MariaDB check the key per row, and only afterdeletingobservers allow the delete. A veto leaves the subtree untouched.rebuildTree(delete: true)removes nodes in the same order.$dispatchesEventslistener that returned a value skipped them. A save could then leave null bounds, and a delete could leave descendants with crossing intervals.fireModelEvent()now owns the upkeep forsaving,deleting,deleted,restoringandrestored, keeping the framework's handling of quiet operations,$haltand listener results.destroy()orforceDestroy()call threwModelNotFoundException, because the ancestor's delete had already removed the descendant's row. That row'sdeletingnow returns false, so the call counts only the rows it deleted.shouldFireDescendantEvents()enabled, restoring now runs descendant model events too, parents first, in the same bounded chunks deletion uses (children first).getDescendantDeleteChunkSize()is renamedgetDescendantChunkSize(), a short chunk ends the loop, and a vetoed descendant throws.SoftDeletes::forceDelete()now resets$forceDeletingin afinallyblock. Before, if the delete threw, a laterdelete()on the same model force deleted instead of soft deleting.Transactions stay with the caller. A structural save updates other rows' bounds before
savingobservers run, so a veto or asaveOrIgnore()conflict can return false after the tree changed, andsaveOrFail()commits in that case. The guide now shows how to wrap these writes in a transaction and lock the tree.Eager loading
Eager loading ancestors, descendants or siblings for many parents was slow or failed outright. Each parent added an
orgroup to the query, and every result was compared with every parent in PHP. On an 11,111-node tree, matching ancestors for every node took 50 seconds and matching descendants ordered by name over 58 seconds. The ancestor query itself took 28.5 seconds on MySQL, and SQLite rejected loads of about 1,000 or more parents because it limits expressions to 1,000 levels.CASEthat finds the nearest parent right bound at or after each row's left bound.orgroups of at most 64. Parents with no room for descendants are skipped, so loading descendants for leaves runs no query.whereIntegerInRaw()does.matchMany(), which replaces the per-parent hooks: one stack sweep per scope for ancestors, a binary search over sorted buckets for descendants, and scope and parent buckets for siblings. A custom result order is restored only when the results were re-sorted.On the same tree, ancestor matching takes 77 ms and descendant matching 134 ms. The ancestor query takes 176 ms on MySQL, and on PostgreSQL it drops from 4.7 seconds to 80 ms.
siblingsAndSelfdrops from 527 ms and 205 MB to 24 ms and 2.3 MB.One cost remains: SQLite prepares a query with thousands of separate descendant ranges in quadratic time. The guide warns about it.
getAncestors(),getDescendants()andgetSiblings()still run fresh queries rather than returning a loaded relation.linkNodes()andtoTree()no longer clear a parent that was eager loaded from outside the collection, so reading it doesn't run another query.Indexes
The left-bound index now includes the right bound, so ancestor queries check both bounds within the index. The schema helpers create
(scope..., _rgt),(scope..., _lft, _rgt)and(scope..., parent_id, _lft), anddropNestedSet()drops the same set.On 50,000-node trees, ancestor reads are about 1.5x faster on MySQL and MariaDB and about 3x faster on PostgreSQL and SQLite, and SQLite move ranges about 4x faster. The tradeoff: large gap writes are 5 to 15 percent slower on MySQL and SQLite, the PostgreSQL index is about 5 percent larger, and on MySQL a low-level
moveNode()without a target depth spends about 5 ms instead of 0.6 ms looking it up. Descendant, child, sibling and max reads are unchanged.Diagnostics and repair
fixTree()andfixSubtree()hydrated every node they selected. They now read plain rows on the write connection, compare each row's bounds, parent and depth, and hydrate and save only rows that change, sosavingandsavedobservers andextraColumnsstill apply to those. Repairing a healthy 111,111-node tree went from 2.7 seconds and 367.5 MB to 358 ms and 73.2 MB on MySQL, and from 3.3 seconds to 389 ms on PostgreSQL. A damaged 11,111-node tree with 11,110 changed rows went from 9.5 seconds and 60.8 MB to 7.0 seconds and 7.4 MB on MySQL.countErrors(),getTotalErrors()andisBroken()now follow the caller'suseWritePdo().fixSubtree()accepts null, as upstream's does.Static analysis
QueryBuilder, the relations and the collection are generic over the model.HasNodebindsHasBuilder<QueryBuilder<static>>, so calls such asCategory::whereIsRoot()andCategory::query()->root()resolve, custom builders keep their type, and collections keep their model.SoftDeletingScopeand Scout'sSearchableScope(withTrashed(),onlyTrashed(),restore(),searchable()and the rest) as undefined on builders and relations, and static model calls dropped custom builders. A small resolver maps a model trait to an interface declaring its macros through theeloquentBuilderMacrosparameter. The database extension mapsSoftDeletes, and Scout ships a newextension.neonmappingSearchable. Fluent macros return the builder or relation they run on, macros take precedence over same-named scopes as inBuilder::__call(), and native builder methods still win.^2.3in the root, database and foundation packages. PHPStan 2.2.16 can't followassertThrows()'s control flow and reports a false error.Documentation
The Nested Set guide now covers adding nested set columns to an existing table, custom column names, deleting nodes, and transactions and concurrency. It also adds relationship-based creation, neighbor moves, ancestor order, related-model subqueries, stored depth, flat trees, scoped key lookups, the soft-deleted leaf rule and typing a custom builder for static analysis.
The README records the deliberate differences from upstream, including null-only roots, the typed schema macros, stored depth, the diagnostics error keys and the changed relation hooks. The license credits Aimeos and Lunar's maintainers, and
docs/upstream-sync/sync.yamltracks Aimeosmasteras the package's upstream.Tests
The node and scoped node tests are split into shared bases with integer and UUID key classes, plus thin subclasses for MySQL, MariaDB and PostgreSQL. Each class migrates its schema once instead of before every test. Assertions that relied on unspecified row order now compare sets or use
defaultOrder(). Upstream tests from Aimeos, the original package and Lunar are ported under their upstream names, the schema test checks the exact columns and indexes on every database, and a custom-column fixture exercises creating, moving, deleting and repairing a tree through renamed columns.Note
Improve nested-set correctness, eager loading, repair, and descendant events
HasNode::fireModelEventin HasNode.php so tree maintenance runs from model lifecycle events, pending actions survive failed saves, and hard/soft descendant deletion and restoration are chunked and evented viachangeDescendantsWithEventsBaseRelationand the ancestors/descendants/siblings relations to batch all parents into one scope-aware query with balanced OR groups, and match results in bulk while restoring original query orderfixSubtreein QueryBuilder.php to work on plain rows, save only changed placements, treat numeric strings as integers, and reconcile gap shifts; depth/distance constraints now accept zeroBuilderMacroResolverthat maps traits likeSoftDeletesto macro interfaces (see SoftDeletingMacros.php and SearchableMacros.php) so soft-delete and Scout builder macros type-check correctly; PHPStan dev constraint raised to ^2.3ensureConcreteNestedSetScopeno longer accepts a Model argument,fixSubtreerequires a nullable root, and the schema macro now drops/creates a different composite index — out-of-tree callers of these symbols will breakMacroscope summarized ac916a3.