Caching of LazyLoaded Relationships
jradtilbrook opened this issue · 29 comments
Issue
From the README and video I am trying to cache a belongsToMany relationship but it doesn't seem to be working. In my set up caching other relationships does work such as hasOne, hasMany and belongsTo but the many-to-many case does not.
I have had a brief look through the code but I am not following where either it or I could be going wrong. The documentation states that adding the Cachable trait just works including relationships.
I do note however, that the test for relationships only seems to test a single type (many-to-one).
Also note that eagerly loading a relationship does appear to work (ie. using with()) whereas lazy loading does not.
I would appreciate any help. Let me know if you need more info.
PS. This is an awesome library!
Environment
Laravel Version: 5.6.*
Laravel Model Caching Package Version: 0.2.57
PHP Version: 7.2
Homestead Version: N/A
Operating System & Version: ArchLinux
Stack Trace
N/A
Hi @jradtilbrook, thanks for reporting these issues. I think perhaps you may have found two holes that haven't been plugged yet. I'll look into it and see if its a bug, or missing functionality. :)
Thanks!
Awesome, no worries. I'm keen to contribute if you need it so let me know. I'm in the process of tracing through to help find anything. I'll let you know if I do 👍
That would be awesome :)
Looks like it might be this line https://github.com/GeneaLabs/laravel-model-caching/blob/master/src/Traits/ModelCaching.php#L57. The internals of laravel calls that passing in the base query builder which is just a standard eloquent builder. It then uses that to perform the actual query (without caching).
To get around this and not have infinite recursion, the CachedBuilder class may have to override the getModels() method of the parent Builder to not proxy to the default non-cached query, all without a call back to its own get(). See also https://github.com/illuminate/database/blob/master/Eloquent/Builder.php#L492-L517
@jradtilbrook looking into this now, to see if I can handle lazy-loading of relationships. You're assessment is spot on. This will take a while to sort out. In the mean time I will clarify in the documentation that lazy-loading is NOT cached.
No worries. I started looking and thought it might be quite complex so thought I'd leave it for now. Thanks for checking it out and good luck!
Any help in form of a PR (by anyone) would be greatly appreciated and help speed things up. :)
I'm still very interested in contributing I've just been really busy. I'm hoping I'll have the time in the next 1-2 weeks. Last I looked though, it seemed like it might need some fairly large refactoring to support, but it's been so long I'll have to basically start again. I'll let you know my thoughts when I get too it! 😄
Hey @mikebronner, so just as a catch-up, it's been 2 weeks now and I haven't had enough time to sit down and go through this yet. I also have some other projects pulling me away so I think it's safe to say I'm not going to be able to help out with this any time soon. Hopefully you can sort it or someone else will be able to help and if it's still available when I have more time I'll take another look
I believe I am having this same issue - although as @jradtilbrook mentioned the with() method is working for him...it is not working for me. Does lazy loading and eager loading work the same with this library?
@mycarrysun I think it works in belongs-to relationships, but in none of the others. Lazy loading works differently from eager loading, which is why things break down. As mentioned above, I believe the problem is that lazy loading expects a Builder instance, and not a CacheBuilder instance. :(
Not sure how to get around that one, it starts messing with your mind when things get conditionally nested like this, doesn't it? It's almost as if we would have to hijack the relationship methods and pass on the CachedBuilder, or something.
@mikebronner please, check the PR for this issue.
We use this library for woub.chat project and this was an important for us, so it's a small contribution from Woub :)
@tmishutin That's awesome! Thanks for submitting that PR. I will review it and try to implement it over the next few days.
WOOHOOOO!!! Thanks @tmishutin for leading the way on this. Release 0.4.20 includes caching lazy-loaded belongs-to relationships. Hopefully this can be used as a template for implementing lazy-loading of other relationships.
I added additional tests to confirm lazy-loading of the following relationships:
- BelongsTo
- HasOne
- HasMany
The following tests are still required:
- HasOneThrough
- HasManyThrough
- Polymorphic One to One
- Polymorphic One to Many
- Polymorphic Many to Many
Once those two are confirmed working, lazy loading will be supported -- I believe across the board?
@mikebronner It seems like I still have some issues with belongsToMany... Cache is not being invalidate.
attach (even with save on belongsTo)
detach
etc.
It has to do with taggging of the cache. Tags are being set based on the model name, not on the relation. So when you add book to store, book tag will be used to invalidate cache. Which is not right. book-store should be used.
Please, correct me if I'm wrong.
@tmishutin Good point, I forgot to write tests for invalidating. I will look into this.
@tmishutin do you see anything off the top of your head that I missed in CachedBelongsToMany.php that introduced this error?
@mikebronner which is most important, I think. In coding there are only two problems:
- How to name a variable
- When to invalidate cache :)
Seems like FiresPivotEventsTrait is not pushing enough params. Instance is not enough in this case.
@mikebronner Third param from FiresPivotEventsTrait should be passed as well / received. So will be something like $this->model."-".$relation
ModelCaching:
static::pivotAttached(function ($instance, $ignoreMe, $relation) { $instance->checkCooldownAndFlushAfterPersisting($instance, $relation); });
@tmishutin Thanks for the ideas! Please test release 0.4.23 and let me know if that fixes it. :) Thanks!
@mikebronner Cool! Seems to work. But only if both models implement cachable traits. If book is cachable, but store is not, it will fail.
Baby-steps, right? Hehe ....
@tmishutin I'm wondering if it even makes sense to invalidate the cache of a relationship that isn't cached? Can you test clearing the cache (I wonder if the cache entries aren't a remnant from before?), then run a BelongsToMany relationship query that has the source side cached and the target not? I would expect the relationship to not cache.
@mikebronner it seems like it should be invalidated as well... Don't know why, but it's cached both ways.
@tmishutin Try release 0.4.24. Now uncached BelongsToMany relationships don't get cached in the first place. :) Let me know how it works for you.
@mikebronner yup, not caching any more.
I just want to share my experinece with this issue. I just faced the BelongsToMany relationship issue using laravel and Nova. it doesn't update redis on save or update or create using Nova. I have to flush redis to see changes made by laravel nova. But when i use disableCache() on my models, it's working fine. But almost all my models using BelongsToMany relationships. So no point to use this package if it's not going to cache my models :) It might be related with the events between nova and models? Maybe this package doesn't regionise Nova events to trigger updates on redis.
Details about the issue:
I got Work and Service models. It's many to many relationships. Work has many Services and Services has Many Works.
Models in project looks like:
Work Model
class Work extends Model implements HasMedia
{
use HasImage, HasTranslate, Cachable;
public $translatable = ['client', 'project'];
protected $collection = 'slider';
protected $casts = [
'date' => 'datetime',
];
public function services()
{
return $this->belongsToMany(Service::class);
}
}
Service Model
class Service extends Model
{
use HasTranslate, HasSlug, Cachable;
public $translatable = ['name', 'description'];
public function getSlugOptions(): SlugOptions
{
return SlugOptions::create()
->generateSlugsFrom('name')
->saveSlugsTo('slug');
}
public function works()
{
return $this->belongsToMany(Work::class);
}
}
Works table
public function up()
{
Schema::create('works', function (Blueprint $table) {
$table->bigIncrements('id');
$table->json('client');
$table->json('project');
$table->date('date');
$table->string('country');
$table->string('url')->nullable();
$table->timestamps();
});
}
Services table
public function up()
{
Schema::create('services', function (Blueprint $table) {
$table->bigIncrements('id');
$table->json('name');
$table->json('description');
$table->string('slug')->unique();
$table->string('icon');
$table->timestamps();
});
}
Service Work pivot table
public function up()
{
Schema::create('service_work', function (Blueprint $table) {
$table->bigIncrements('id');
$table->integer('service_id')->unsigned();
$table->integer('work_id')->unsigned();
});
}
Laravel : 5.8.30
Redis server v=5.0.5
Laravel Nova 2.0.9
I hope this help you to figure out the issue.
Update:
I just notice the key names in redis is a bit weird.
xxx:tag:genealabs:laravel-model-caching:mysql:nemrut:appservice:key,
i believe there should be : between app and service so it should looks like
xxx:tag:genealabs:laravel-model-caching:mysql:nemrut:app:service:key,
is key naming working correctly? maybe shoud check that out.
@nemrutco Please open new issues for items that aren't the exact same thing. let's continue the discussion there.
The issue is that the relations are fetched in another why in Laravel. According to this answer I tried to do this in __get method and it worked perfectly:
public function __get($key)
{
// If the propriety is a relationship, get it now
if ($this->isRelation($key)) {
// If the relation is already loaded and set to the current instance of model, return it
if ($this->relationLoaded($key)) {
return $this->getRelationValue($key);
}
// Get the relation from the cache, or load it from the datasource and set to the cache
$data = Cache::rememberForever($this->getCacheKey($key), function () use ($key) {
return $this->getRelationValue($key);
}, 'using', $key);
// Set the relation to the current instance of model
$this->setRelation($key, $data);
return $data;
}
return parent::__get($key);
}
The only issue that i faced was using lazy-eager loading that is using the load or loadMissing methods to get the data ($user->loadMissing('roles')), as I found that one of the packages I use is using it. To solve the issue I just overrides the load and loadMissing to let them return nothing later Laravel calls the __get method where we just cached the relation and it works fine for me.
public function loadMissing($relationship)
{
return;
}
public function load($relationship)
{
return;
}