Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tree only shows upper level #25

Closed
ben-joostens opened this issue Sep 30, 2013 · 26 comments
Closed

Tree only shows upper level #25

ben-joostens opened this issue Sep 30, 2013 · 26 comments

Comments

@ben-joostens
Copy link
Contributor

First of all I want to add that I'm not sure if my data is correct. There's a few things I noticed when creating my items in the database…

Entries in grades_closure were created, but when moving an item to a parent, only the first one succeeded without error. Every other item added under the same parent throws an error because it tried to add a signed value to the position column.

Let's say there was a sibling under a parent with position 0, whenever I moved another sibling under the same parent, it would try to add it with a position -1

It did however end up adding the new item to the database and the position seems to end up ok.

The second thing I've noticed is that whenever an item was moved to the parent (let's say an item with id 10 was moved to a parent with id 20), 2 entries existed in grades_closure:

ancestor 10, descendant 10, depth 0
ancestor 20, descendant 10, depth 1

When requesting the tree with these values in the database, loading was very slow (about a second or two) while there are less than 100 items in the database. On top of that, it showed no tree hierarchy.

When removing these items in grades_closure that referenced themselves, load time when requesting the entire tree was OK again, but now it only showed the parents. From the documentation, I get the impression that it should retrieve the entire tree, parents + siblings.

However, requesting the children of a specific parent works without problems, and I can also request the parent of a given child.

Am I doing something wrong or is this intended behaviour?
Also, the documentation left me guessing a bit as to how I go about creating the actual data, I'm sure it's obvious enough when it works, but when something fails, I was left guessing if I went about it the wrong way.

I'd be happy to add to the documentation if I know the correct approach.

For your convenience, here's an sql dump of the two tables: https://dl.dropboxusercontent.com/u/139621/30-09-13_grades.sql

@vjandrea
Copy link
Contributor

At first sight i can see already something that seems wrong: 39 inserts in grades table, same number of inserts in grades_closure table. Closure table doesn't work in this way, are you manually inserting data? Do you mind pasting the controller code you're using to nest categories?

@ben-joostens
Copy link
Contributor Author

I did not manually insert them, but when I noticed the first issue I described, I removed the self-referencing entries in grades_closure. Are you saying they should be there?

This is how I moved an item to its parent:

$grade = Grade::find(62);
$grade->moveTo(Grade::find(70));

@vjandrea
Copy link
Contributor

Yes, the self referencing entries have to stay there to allow the correct functioning of the methods. Don't touch anything in the closure table, and use only the methods provided, or it won't work at all.
If you want to learn how closure table method works, you can watch this: http://www.percona.tv/percona-webinars/models-for-hierarchical-data-in-sql-and-php, where Bill Karwin, the inventor of this method, explains it comparing it to other techniques.

@ben-joostens
Copy link
Contributor Author

Alright, so I've started from scratch, and now when I try to move them to their parent, this happens (the signed position value I mentioned in my original post):

SQLSTATE[22003]: Numeric value out of range: 1690 BIGINT UNSIGNED value is out of range in '(`database`.`grades`.`position` - 1)' (SQL: update `grades` set `position` = `position` - 1, `updated_at` = ? where `grades`.`deleted_at` is null and `id` in (?) and `position` in (?, ?)) (Bindings: array ( 0 => '2013-10-01 00:38:31', 1 => '73', 2 => 0, 3 => 1, ))

This is how I was moving them:

// Just a range for the id's I wanted to move
$range = range(73, 84);
$grades = Grade::find($range);
$parent = Grade::find(110);

foreach ($grades as $grade):
    $grade->moveTo($parent);
endforeach;

Edit: And thanks for the link, having a look right now.

@franzose
Copy link
Owner

@vjandrea, we should implement ClosureTable\DatabaseSeeder or something, I think. It seems that it could simplify things.

@franzose
Copy link
Owner

@ben-joostens please, post here, as separate comments, your actions that leading to exceptions or erroneous behaviour. Also I'd like to see stack traces for all of that situations, also as separate comments.

Thanks.

@ben-joostens
Copy link
Contributor Author

@franzose Alright, will make work of it tomorrow.

@ben-joostens
Copy link
Contributor Author

Alright, here's the step by step.

First of all, this is the database structure I start out with:
https://dl.dropboxusercontent.com/u/139621/01_grades_initial.sql

@ben-joostens
Copy link
Contributor Author

I then populate the grades table:

$grades = array(
                array(
                    'title' => 'Eerste Kleuterklas A',
                    'title_short' => '1KA',
                ),
                array(
                    'title' => 'Eerste Kleuterklas B',
                    'title_short' => '1KB',
                )
);

foreach ($grades as $grade):
    $ngrade = new Grade($grade);
    $ngrade->save();
endforeach;

Which gives me a database with these contents:
https://dl.dropboxusercontent.com/u/139621/02_grades_populated.sql

@ben-joostens
Copy link
Contributor Author

The next step is where the issue arises. I'll try to move a few items to their parent:

$range = range(1, 12);
$grades = Grade::find($range);
$parent = Grade::find(38);

foreach ($grades as $grade):
    $grade->moveTo($parent);
endforeach;

Which throws this error:

SQLSTATE[22003]: Numeric value out of range: 1690 BIGINT UNSIGNED value is out of range in '(`deputseknipoog_new`.`grades`.`position` - 1)' (SQL: update `grades` set `position` = `position` - 1, `updated_at` = ? where `grades`.`deleted_at` is null and `id` in (?) and `position` in (?, ?)) (Bindings: array ( 0 => '2013-10-01 12:56:11', 1 => '1', 2 => 0, 3 => 1, ))

It ends up moving 2 items to their parent, which you can see in the export, if I rerun the function, it'll behave the same, add some and then throw an error.
https://dl.dropboxusercontent.com/u/139621/03_grades_after-move.sql

23. Exception
…/­vendor/­laravel/­framework/­src/­Illuminate/­Database/­Connection.php555
22. Illuminate\Database\Connection handleQueryException
…/­vendor/­laravel/­framework/­src/­Illuminate/­Database/­Connection.php528
21. Illuminate\Database\Connection run
…/­vendor/­laravel/­framework/­src/­Illuminate/­Database/­Connection.php355
20. Illuminate\Database\Connection affectingStatement
…/­vendor/­laravel/­framework/­src/­Illuminate/­Database/­Connection.php300
19. Illuminate\Database\Connection update
…/­vendor/­laravel/­framework/­src/­Illuminate/­Database/­Query/­Builder.php1438
18. Illuminate\Database\Query\Builder update
…/­vendor/­laravel/­framework/­src/­Illuminate/­Database/­Query/­Builder.php1472
17. Illuminate\Database\Query\Builder decrement
…/­vendor/­laravel/­framework/­src/­Illuminate/­Database/­Eloquent/­Builder.php288
16. Illuminate\Database\Eloquent\Builder decrement
…/­vendor/­franzose/­closure-table/­src/­models/­Entity.php912
15. Franzose\ClosureTable\Entity reorderSiblings
…/­vendor/­franzose/­closure-table/­src/­models/­Entity.php962
14. Franzose\ClosureTable\Entity performUpdate
…/­vendor/­laravel/­framework/­src/­Illuminate/­Database/­Eloquent/­Model.php1066
13. Illuminate\Database\Eloquent\Model save
…/­vendor/­franzose/­closure-table/­src/­models/­Entity.php845
12. Franzose\ClosureTable\Entity moveGivenTo
…/­vendor/­franzose/­closure-table/­src/­models/­Entity.php822
11. Franzose\ClosureTable\Entity moveTo
…/­app/­controllers/­GradeController.php188
10. GradeController index
<#unknown>0
9. call_user_func_array
…/­vendor/­laravel/­framework/­src/­Illuminate/­Routing/­Controllers/­Controller.php138
8. Illuminate\Routing\Controllers\Controller callMethod
…/­vendor/­laravel/­framework/­src/­Illuminate/­Routing/­Controllers/­Controller.php115
7. Illuminate\Routing\Controllers\Controller callAction
…/­vendor/­laravel/­framework/­src/­Illuminate/­Routing/­Router.php985
6. Illuminate\Routing\Router Illuminate\Routing\{closure}
<#unknown>0
5. call_user_func_array
…/­vendor/­laravel/­framework/­src/­Illuminate/­Routing/­Route.php80
4. Illuminate\Routing\Route callCallable
…/­vendor/­laravel/­framework/­src/­Illuminate/­Routing/­Route.php47
3. Illuminate\Routing\Route run
…/­vendor/­laravel/­framework/­src/­Illuminate/­Routing/­Router.php1016
2. Illuminate\Routing\Router dispatch
…/­vendor/­laravel/­framework/­src/­Illuminate/­Foundation/­Application.php530
1. Illuminate\Foundation\Application dispatch
…/­vendor/­laravel/­framework/­src/­Illuminate/­Foundation/­Application.php506
0. Illuminate\Foundation\Application run
…/­public/­index.php49

@ben-joostens
Copy link
Contributor Author

Just for completeness, here is an export of the database after I run the function which moves the items to their parent again. It has moved 2 extra items, but the position for the four items is now 0,2,4,5
https://dl.dropboxusercontent.com/u/139621/04_grades_after-second-move.sql

@ben-joostens
Copy link
Contributor Author

@franzose I hope you have enough information with what I've provided here, if not, let me know.

@ben-joostens
Copy link
Contributor Author

I've been playing around with it for a bit, and managed to get a working tree by using the appendChild method instead of moveTo. However, I have a page load of about 12 seconds (constant querying). My grades table has 89 entries, my grades_closure table has 251.

Not sure if this long load time is normal behaviour?

loadtime

@franzose
Copy link
Owner

franzose commented Oct 2, 2013

@ben-joostens, it's strange but I made tests and they doesn't reproduce the behavior you faced. I mean wrong positioning. Test tables are prepared with the following (you can find this code at tests/ClosureTableTestCase.php):

        // since we use SQLite we have to add foreign keys support manually.
        DB::unprepared('PRAGMA foreign_keys = ON;');
        Schema::create('pages', function($table){
            $table->increments('id')->unsigned();
            $table->string('title', 250);
            $table->string('excerpt', 500);
            $table->text('content');
            $table->string('language', 2);
            $table->integer('position')->unsigned();
            $table->timestamps();
            $table->softDeletes();
        });

        Schema::create('pages_closure', function($table){
            $table->increments('ctid');
            $table->integer('ancestor')->unsigned();
            $table->integer('descendant')->unsigned();
            $table->integer('depth')->unsigned();

            $table->foreign('ancestor')->references('id')->on('pages')->onDelete('cascade');
            $table->foreign('descendant')->references('id')->on('pages')->onDelete('cascade');
            $table->index('depth');
        });

The code of the test is the following:

// ...
        list($page, $child1/*, $child2, $child3, $child4 */) = $this->prepareTestedSiblings();

        $children = Page::find(range(3, 5));

        foreach ($children as $child)
        {
            $child->moveTo($child1);
        }

        $this->assertEquals(3, $child1->countChildren());
        $this->assertEquals($child1->id, $children[1]->parent()->id);
// ...

The test reports Ok.

@ben-joostens
Copy link
Contributor Author

@franzose How do I run that test? I'm not familiar with testing tests from installed packages.

@franzose
Copy link
Owner

franzose commented Oct 2, 2013

@ben-joostens, open terminal, then cd to vendor/franzose/ClosureTable/tests and run phpunit

@vjandrea
Copy link
Contributor

vjandrea commented Oct 2, 2013

@ben-joostens, if you get an autoload.php failure when you run phpunit, perform a composer update in the same folder before you run phpunit

@ben-joostens
Copy link
Contributor Author

Ok, here's the output, forgive me if I'm overlooking something simple, I haven't had the time to check out the tests just yet. But wanted to share it anyway.

http://paste.laravel.com/VXn

@vjandrea
Copy link
Contributor

vjandrea commented Oct 2, 2013

@ben-joostens I'm working to fix the errors, anyway there are no failures in the tests, only sql exceptions that i'll correct

@ben-joostens
Copy link
Contributor Author

@vjandrea Yep that's what I thought. I'll try and replicate the issue I was encountering with a test.

Btw, on those long loading times, can you tell me if that is to be expected?

@vjandrea
Copy link
Contributor

vjandrea commented Oct 2, 2013

@ben-joostens, can you tell me the specs of the machine you're using? That's really not what's expected, normally queries using closure tables are pretty fast.

@ben-joostens
Copy link
Contributor Author

@vjandrea specs won't be an issue, it's running on a dual xeon (Intel(R) Xeon(R) CPU E5620 @ 2.40GHz) with 16 cores total and 12 gigs of ram with an SSD for storage.

Do you mind showing me an export of a working dataset so I can compare? Or would you rather see a new export now that I was able to build the tree without errors?

@ben-joostens
Copy link
Contributor Author

This is what happens when I fetch the 15 descendants of a parent:

http://paste.laravel.com/VZv

@vjandrea
Copy link
Contributor

vjandrea commented Oct 2, 2013

With 33 nested items divided in two root trees i have a loading time of 3.93 seconds for a full rendered page on my local machine (macbook pro), here's the dump http://paste.laravel.com/W3z

@franzose
Copy link
Owner

franzose commented Oct 3, 2013

@vjandrea, slow actually...

@franzose
Copy link
Owner

franzose commented Oct 3, 2013

This is what happens when I fetch the 15 descendants of a parent

@ben-joostens, these weird queries are workaround to set ancestor, descendant and depth directly as the attributes to the Entity model... I'm trying to find a better solution for that.

@franzose franzose closed this as completed Oct 4, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants