Skip to content

with not being called in single record request #62

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

Closed
tylergets opened this issue May 29, 2017 · 5 comments
Closed

with not being called in single record request #62

tylergets opened this issue May 29, 2017 · 5 comments
Milestone

Comments

@tylergets
Copy link

Hey guys, I have been using the package for a while and love all the updates. One thing I am having problems with is the with function in the Adapter class. I believe this is supposed to be called on index and read, however I am only able to load the include relationships on an index call. This is causing a lot of N+1 queries in my application.

Here is my Adapter

<?php namespace App\JsonApi\Auctions;

use App\Models\Auction;
use Carbon\Carbon;
use CloudCreativity\LaravelJsonApi\Pagination\StandardStrategy;
use CloudCreativity\LaravelJsonApi\Store\EloquentAdapter;
use Illuminate\Database\Eloquent\Builder;
use Illuminate\Support\Collection;

class Adapter extends EloquentAdapter
{

    /**
     * Adapter constructor.
     * @param StandardStrategy $paging
     */
    public function __construct(StandardStrategy $paging)
    {
        parent::__construct(new Auction(), $paging);
    }

    /**
     * Apply the supplied filters to the builder instance.
     *
     * @param Builder $query
     * @param Collection $filters
     * @return void
     */
    protected function filter(Builder $query, Collection $filters)
    {
        if (!$filters->has('ended')) {
            $query->where('ends_at', '>', Carbon::now('UTC'));
        }
        $query->orderBy('ends_at', 'asc');

    }

    protected function with(Builder $query, Collection $includePaths)
    {
        if($includePaths->contains('location')) {
            $query->with('location');
        }
    }


    /**
     * Is this a search for a singleton resource?
     *
     * @param Collection $filters
     * @return bool
     */
    protected function isSearchOne(Collection $filters)
    {
        return false;
    }

}

And my schema

<?php namespace App\JsonApi\Auctions;

use Carbon\Carbon;
use CloudCreativity\LaravelJsonApi\Schema\EloquentSchema;

class Schema extends EloquentSchema
{
    protected $resourceType = "auctions";

    protected $attributes = [
        'name',
        'removal',
        'highlights',
        'notes',
        'terms',
        'contact',
        'ends_at',
    ];

    protected $dateFormat = Carbon::ISO8601;

    /**
     * @param object $resource
     * @param bool $isPrimary
     * @param array $includeRelationships
     * @return array
     */
    public function getRelationships($resource, $isPrimary, array $includeRelationships)
    {
        $return = [];
        if (isset($includeRelationships['items'])) {
            $return['items'] = [
                self::SHOW_SELF => true,
                self::SHOW_RELATED => true,
                self::DATA => $resource->items,
            ];
        }

        $return['location'] = [
            self::SHOW_SELF => true,
            self::SHOW_RELATED => true,
            self::DATA => isset($includeRelationships['location']) ? $resource->location : $this->createBelongsToIdentity($resource, 'location'),
        ];

        return $return;
    }
}

@lindyhopchris
Copy link
Member

Hi. The with is only called on index calls at the moment. I can change this but wasn't sure what the point of calling it on a read call is? Because N+1 would only occur if you are loading multiple Auction objects, which you wouldn't on a read request (because it is for a single record).

Unless I'm missing something?! Happy to discuss as if there's a good reason it's needed for the with request this can be added.

@tylergets
Copy link
Author

@lindyhopchris In my case, I am including the nested relationships "items,items.bids,items.bids.bidder"

I need to eager load the Bidder object which is N+1ed on a read. Unless I am looking at this wrong?

@lindyhopchris
Copy link
Member

ah, ok that makes sense - couldn't see that in your adapter's with method.

it makes sense for the change to be implemented. I'll take a look at what it involves - I suspect I'd need to include it in the next release because it'll be breaking (I'll have to modify the AdapterInterface so that the find method receives the encoding parameters from the client).

@lindyhopchris
Copy link
Member

Hi @tylergets

I've looked into this and it's going to involve some interface changes. As I'm probably going to have to make interface changes when I sort out all the relationships stuff (see #60) I'll include it in that change.

I'm going to do a release of the 1.0 changes I've already made on #60, then will probably move on to the relationship stuff next.

@lindyhopchris lindyhopchris mentioned this issue Jun 6, 2017
15 tasks
@lindyhopchris lindyhopchris added this to the 1.0.0 milestone Jun 16, 2017
@lindyhopchris
Copy link
Member

This change is now included on the develop (1.0) branch.

The Eloquent adapter now converts JSON API include paths into Eloquent relationship paths for eager loading e.g. comments.created-by is converted to comments.createdBy for eager loading. If you need to map these paths differently, you can specify this using the $includePaths property of your adapter. See https://github.com/cloudcreativity/laravel-json-api/blob/develop/src/Eloquent/Concerns/IncludesModels.php#L18-L47

It also continues to support default paths for eager loading on the $defaultWith property.

In addition, eager loading is now also supported when reading and updating specific resources.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants