laravel / nova-issues

557 stars 34 forks source link

Eager Loading not working with BelongsTo #112

Closed SeoFood closed 5 years ago

SeoFood commented 6 years ago

Resource:

public static $with = [ 'address'];

public function fields(Request $request)
{
  BelongsTo::make('Address')->searchable(),
}

Result:

image

If I use computed fields manually its working.

Quinndark commented 6 years ago

I had the same problem.

Livijn commented 6 years ago

+1

ragingdave commented 6 years ago

This is also found on morphTo as well

taylorotwell commented 6 years ago

I need you to describe your resource / table structure a bit more. Your code snippet is really out of context so it's hard for me to tell how to recreate things.

BengtHagemeister commented 6 years ago

I have a simulier issue with the following output from debugbar:

Page URL: http://nova.test/resources/coupling-equipments?coupling-equipments_page=2

API URL: http://nova.test/nova-api/coupling-equipments?search=&filters=&orderBy=&orderByDirection=desc&perPage=25&trashed=&page=2&viaResource=&viaResourceId=&viaRelationship=&relationshipType=

bildschirmfoto 2018-09-23 um 17 13 59

Nove Resource CouplingEquipment:


class CouplingEquipment extends Resource {

     ....

     public static $with = ['couplingSize'];

     public function fields(Request $request)
         {
             return [
                 BelongsTo::make('Kupplung', 'couplingSize', CouplingSize::class),
             ];
         }

     ....

}

Nova Resource CouplingSize:


class CouplingSize extends Resource {

     ....

     public function fields(Request $request)
     {

          return [
               HasMany::make('couplingEquipment'),
          ]
     }

     ....

}

Model CouplingEquipment:


class CouplingEquipment extends Model {

     public function couplingSize()
         {
             return $this->belongsTo(CouplingSize::class);
         }
}

Model CouplingSize:


class CouplingSize extends Model {

     public function couplingEquipment()
    {
        return $this->hasMany(CouplingEquipment::class);
    }

}

Table structure:

caseydwyer commented 6 years ago

We're seeing this issue as well. One note, it doesn't appear to be an issue on initial index page-load.

For example, we have a School model & resource, that's a simple BelongsTo relationship with StudentOrganization. There are ~250 Schools and ~250 StudentOrganizations. When visiting the Schools resource page initially (/resources/schools), it loads quickly, without unnecessary queries (analyzing with Laravel Debugbar).

When that URL gains any parameter, though, it starts running several hundred queries, many duplicate. This includes pagination, filter, sorting and search parameters. Hope that helps.

abordage commented 6 years ago

When that URL gains any parameter, though, it starts running several hundred queries, many duplicate. This includes pagination, filter, sorting and search parameters. Hope that helps.

+1, same problem

mikebouwmans commented 6 years ago

I'm actually having the same problem, but for me all pages won't load anymore when eager loading with all relations.

markgp commented 6 years ago

We made the following changes to /nova/src/Fields/BelongsTo.php which solved the issue for us.

You still need to define your relationships using the $with property, obviously.

--- a/BelongsTo.php     2018-10-26 13:54:48.552145700 +1100
+++ b/BelongsTo.php     2018-10-26 13:53:20.778856800 +1100
@@ -144,7 +144,7 @@
      */
     public function resolve($resource, $attribute = null)
     {
-        $value = $resource->{$this->attribute}()->withoutGlobalScopes()->first();
+        $value = $resource->{$this->attribute};

         if ($value) {
             $this->belongsToId = $value->getKey();
abordage commented 6 years ago

@markgp , https://github.com/laravel/nova-issues/issues/246#issuecomment-420939582

jasonlav commented 6 years ago

Also able to verify issue. Belongs to will eager load and proceed to query each relationship.

Etmolf commented 6 years ago

We made the following changes to /nova/src/Fields/BelongsTo.php which solved the issue for us.

You still need to define your relationships using the $with property, obviously.

--- a/BelongsTo.php     2018-10-26 13:54:48.552145700 +1100
+++ b/BelongsTo.php     2018-10-26 13:53:20.778856800 +1100
@@ -144,7 +144,7 @@
      */
     public function resolve($resource, $attribute = null)
     {
-        $value = $resource->{$this->attribute}()->withoutGlobalScopes()->first();
+        $value = $resource->{$this->attribute};

         if ($value) {
             $this->belongsToId = $value->getKey();

For a temporary workaround that does not break updates: make a new action and extend the existing BelongsTo Class in your FieldClass:

php artisan nova:field acme/belongs-to-field

use Laravel\Nova\Fields\BelongsTo;

class BelongsToField extends BelongsTo
{
    /**
     * Resolve the field's value.
     *
     * @param  mixed  $resource
     * @param  string|null  $attribute
     * @return void
     */
    public function resolve($resource, $attribute = null)
    {
        $value = $resource->{$this->attribute};

        if ($value) {
            $this->belongsToId = $value->getKey();

            $this->value = $this->formatDisplayValue($value);
        }
    }
}

You can skip all the node dependencies and resources as the existing nova component will be used.

Change all uses of "BelongsTo" to your own "BelongsToField". If a proper fix is available, just change it back to the nova one.

bomshteyn commented 6 years ago

Thanks @Etmolf ,

Since we would have todo this in few projects, i have created an installable package based on your code. Belongs To field that respects Eager Loading

dillingham commented 5 years ago

Just noticed this also, any updates?

$value = $resource->{$this->attribute}()->withoutGlobalScopes()->first();

being changed to the following does seem to work as stated above

$value = $resource->{$this->attribute};
huoding commented 5 years ago

+1, I have the same question.

dkulyk commented 5 years ago

PR was sent to upstream.

dkulyk commented 5 years ago

This has been fixed and will be included in the next release.

fedeisas commented 5 years ago

@dkulyk ETA for the next release, please. Thanks.

davidhemphill commented 5 years ago

Soon, once we get docs for the new version out. You can use dev-master right now though.

caseydwyer commented 5 years ago

@davidhemphill Thanks for the updates! Really appreciate your responsiveness, and really looking forward to this update. 👍

nouman-ashraf-awan commented 6 days ago

This is my relation in Group Model

/** @return BelongsToMany */
    public function currencies()
    {
        return $this->belongsToMany(Currency::class, CurrencyGroup::class, 'group_id', 'currency_id')
            ->withPivot('id', 'amount', 'remaining', 'type', 'begins_at', 'expires_at', 'contract_id')
            ->withTimestamps('created_at', 'updated_at');
    } 
``` In Group Nova Resource 
BelongsToMany::make('Assigned Currencies', 'currencyContracts', Currency::class)
                ->fields(new CurrencyContractFields)
                ->allowDuplicateRelations()
                ->canSee(fn () => Setting::get('ff_enable_currency_system'))
In CurrencyGroup Model which is pivot of Currency 
I have a relation currencyUsers

public function currencyUsers() { return $this->hasMany(CurrencyUser::class, 'currency_group_id', 'id'); }


Eager loading only working with nova-api/groups/ID 
but not in 
/nova-api/currencies?orderBy=&page=1&perPage=5&relationshipType=belongsToMany&search=&trashed=&viaRelationship=currencies&viaResource=groups&viaResourceId=ID
I want to eager load currencyUsers in Group.php Nova. I have included in $with=['currencies.pivot.currencyUsers']; but this is causing N+1 Problem. How can i achieve eager loading in Group.php Nova Resource.
Quinndark commented 6 days ago

您好,邮件已收到,谢谢。。