lazychaser / laravel-nestedset

Effective tree structures in Laravel 4-8
3.61k stars 471 forks source link

`Kalnoy\Nestedset\QueryBuilder::moveNode` throws exception `A non-numeric value encountered` if the model uses a global scope to add additional attributes #513

Open nagmat84 opened 3 years ago

nagmat84 commented 3 years ago

Maybe this issue is not an issue of this library but an error in my code, but I am unable to track down the root cause. It simply manifests as the exception "A non-numeric value encountered" inside Kalnoy\Nestedset\QueryBuilder::moveNode at line 556 $height = $rgt - $lft + 1.

I explain what I try to achieve and maybe someone either spots the bug in my code (I guess so) or it is really an actual bug in the library.

A have two Eloquent models Album and Photo. An album can contain further sub-albums and uses the trait Kalnoy\Nestedset\NodeTrait. Photos belong to albums. A photo has an attribute taken_at which stores the timestamp when the photo has been taken.

For each album, I want to have two "transient" or "virtual" attributes max_taken_at and min_taken_at which keeps the smallest and largest value of taken_at for all photos within the album and its sub-albums recursively. The aggregate functions for relations Album:query()->withMin('photos', 'taken_at')->withMax('photos','taken_at') which are provided by Eloquent don't do what I want, because the only consider photos which are direct children of the album, but not aggregate recursively over photos of sub-albums down the tree.

I exploited Eloquent's scopes to modify the default query of an album and include the desired attributes into the query like this:

class Album extends Model {
  protected static function booted() {
    parent::booted();
    // Normally "scopes" are used to restrict the result of the query
    // to a particular subset through adding additional WHERE-clauses
    // to the default query.
    // However, "scopes" can be used to manipulate the query in any way.
    // Here we add to additional "virtual" columns to the query.
    static::addGlobalScope('calc_minmax_taken_at', function (Builder $builder) {
      $builder->addSelect([
        'max_taken_at' => Photo::query()
          ->leftJoin(...)
          ->select('taken_at')
          ->where('a._lft', '>=', DB::raw('_lft'))
          ->where('a._rgt', '<=', DB::raw('_rgt'))
          ->whereNotNull('taken_at')
          ->orderBy('taken_at', 'desc')
          ->limit(1),
        'min_taken_at' => Photo::query()
          ->leftJoin('albums as a', 'a.id', '=', 'album_id')
          ->select('taken_at')
          ->where('a._lft', '>=', DB::raw('_lft'))
          ->where('a._rgt', '<=', DB::raw('_rgt'))
          ->whereNotNull('taken_at')
          ->orderBy('taken_at', 'asc')
          ->limit(1),
      ]);
    });
  }
}

This way I get two more attributes in the array attributes of an Album object when an album is hydrated from the database. The result is correct and as expected.

However, I get an exception when I try to move an album to a different parent album and save it.

$albumA = Album::query()->find(42);
$albumB = Album::query()->find(4711);
// up to here everything is fine, the "virtual" attributes `min_taken_at` and `max_taken_at` are properly hydrated from DB and part of the album model
$albumA->parent_id = $albumB->id;
$albumA->save(); // this throws an exception

throws the exception "A non-numeric value encountered" inside Kalnoy\Nestedset\QueryBuilder::moveNode at line 556 $height = $rgt - $lft + 1.

nagmat84 commented 3 years ago

Addendum: After adding some var_dump I can know confirm that the variable $rgt happens to equal the title of album A (each album has an attribute title) and $lft equals the ID of album A.

nagmat84 commented 3 years ago

I believe I found an solution and it is indeed a bug. QueryBuilder::moveNode($key, $position), line 544 assumes that

list($lft, $rgt) = $this->model->newNestedSetQuery()->getPlainNodeData($key, true);

only returns an array with exactly two values in that order. However, this is incorrect, if the base model has defined scopes. The query in QueryBuilder::getNodeData($id, $required), line 34 and 38,

$query = $this->toBase();
// ...
$data = $query->first([
  $this->model->getLftName(),
  $this->model->getRgtName()
]);

returns more than the queried to columns, if the base model uses a global scope which in turn uses addSelect.

The fix is easy and only requires patching NodeTrait. Instead of calling $this->newQuery (lines 252, 341, 678 and 690) $this->newQueryWithoutScopes() should be invoked. See PR #514.