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

No Eager Loading ? #110

Closed
Iseldore opened this issue Sep 2, 2021 · 10 comments
Closed

No Eager Loading ? #110

Iseldore opened this issue Sep 2, 2021 · 10 comments
Assignees

Comments

@Iseldore
Copy link

Iseldore commented Sep 2, 2021

Hi,

When I use $expand, I have over 200 queries because eager loading is not used.

Could you please update package or tell me how to enable eager loading ?

Thanks !

@27pchrisl
Copy link
Contributor

Hiya @Iseldore thanks for the report! To reproduce what's causing these extra queries could you share a bit more about your setup like the EntitySet types in use (Eloquent vs SQL), how they're related and the OData URL query parameters you're using?

@27pchrisl 27pchrisl self-assigned this Sep 2, 2021
@Iseldore
Copy link
Author

Iseldore commented Sep 2, 2021

Sure, it looks like that :

In AppServiceProvider :
\Lodata::discoverEloquentModel(\App\Models\Address::class);
\Lodata::discoverEloquentModel(\App\Models\User::class);
\Lodata::getEntitySet('Users')->discoverRelationship('addresses');

The request is /odata/Users?$expand=addresses

@27pchrisl
Copy link
Contributor

Got it, could you try editing

foreach ($builder->getModels() as $model) {
locally to change $builder->getModels() to $builder->lazy() ?

I've done that here, and the tests all still pass so the change looks good - just wanted to check you were seeing better behaviour your side before committing.

@Iseldore
Copy link
Author

Iseldore commented Sep 2, 2021

No changes here :
image

still having a select on each users + select on addresses table for each user

@27pchrisl
Copy link
Contributor

This is tricky to completely fix, because Lodata doesn't perform a real database join for expanded entities.

However, as you point out Lodata is doing three queries for each entity, where two of them are unnecessary. I have released v2.1.3 that should eliminate at least 2/3rds of the queries.

Converting the way expansion works to eager load using a proper join is something I will look into as well.

@Iseldore
Copy link
Author

Iseldore commented Sep 2, 2021

Thank you for your reactivity, I confirm that I have less requests now ! 😄

However, it would be great if you could perform that with no n+1 queries.

Thanks you again and for your great package !

@masterix21
Copy link

I can confirm that the package does a lot of queries because don't use the eager loading feature.

@RuslanMelnychenko
Copy link

RuslanMelnychenko commented Mar 11, 2023

I fixed it
Replace the function EloquentEntitySet::query() and add new function EloquentEntitySet::getRelationships()

/**
 * Query eloquent models
 */
public function query(): Generator
{
    $builder = $this->getBuilder();
    $builder->select('*');

    if ($this->navigationSource) {
        /** @var Entity $sourceEntity */
        $sourceEntity = $this->navigationSource->getParent();
        $expansionPropertyName = $sourceEntity->getEntitySet()->getPropertySourceName($this->navigationSource->getProperty());
        /** @var Model $source */
        $source = $sourceEntity->getSource();
        $expansionBuilder = $source->$expansionPropertyName();

        if ($source->relationLoaded($expansionPropertyName)) {
            $relation = \Illuminate\Support\Collection::wrap($source->getRelation($expansionPropertyName));
            foreach ($relation->lazy() as $model) {
                yield $this->modelToEntity($model);
            }
            return;
        }
        $builder = $expansionBuilder;
        if ($builder instanceof HasManyThrough) {
            $builder->select($builder->getRelated()->getTable().'.*');
        }
    }

    $this->implementParametersInBuilder($builder);

    $builder->with($this->getRelationships());

    foreach ($builder->lazy() as $model) {
        yield $this->modelToEntity($model);
    }
}

/**
 * Implement $compute, $top, $skip, $orderby, $filter, $search to Builder
 * @param $builder
 * @return void
 */
protected function implementParametersInBuilder($builder) {
    $this->selectComputedProperties($builder);

    $where = $this->generateWhere();

    if ($where->hasStatement()) {
        $builder->whereRaw($where->getStatement(), $where->getParameters());
    }

    $orderby = $this->generateOrderBy();

    if ($orderby->hasStatement()) {
        $builder->orderByRaw($orderby->getStatement(), $orderby->getParameters());
    }

    if ($this->getTop()->hasValue()) {
        $builder->limit($this->getTop()->getValue());
    }

    if ($this->getSkip()->hasValue()) {
        if (!$this->getTop()->hasValue()) {
            $builder->limit(PHP_INT_MAX);
        }

        $builder->skip($this->getSkip()->getValue());
    }
}

/**
 * Get relations by navigation requests
 * @return array
 */
protected function getRelationships(): array
{
    $relations = [];

    foreach ($this->getType()->getNavigationProperties() as $navigationProperty) {
        $navigationRequest = $this->getTransaction()->getNavigationRequests()->get($navigationProperty->getName());
        if (!$navigationRequest) {
            continue;
        }
        $expansionPropertyName = $this->getPropertySourceName($navigationProperty);
        $expansionTransaction = clone $this->getTransaction();
        $expansionTransaction->setRequest($navigationRequest);
        $expansionSet = clone $this->getBindingByNavigationProperty($navigationProperty)->getTarget();
        $expansionSet->setTransaction($expansionTransaction);
        $relations[$expansionPropertyName] = function ($builder) use ($expansionSet) {
            $expansionSet->implementParametersInBuilder($builder);
        };
        $relations = array_merge(
            $relations,
            \Illuminate\Support\Arr::prependKeysWith(
                $expansionSet->getRelationships(),
                "{$expansionPropertyName}."
            )
        );
    }

    return $relations;
}
Example (Laravel Telescope)

Request

image

Before:

image

After:

image

PS: -1 query for authentication

I'm not sure if this is the right approach, but it works 😎

@27pchrisl
Copy link
Contributor

27pchrisl commented Mar 11, 2023

Thanks for this great work @RuslanMelnychenko ! I have created a PR to get it included... #523

@27pchrisl
Copy link
Contributor

This contribution now merged.

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

No branches or pull requests

4 participants