mikebronner / laravel-model-caching

Eloquent model-caching made easy.
MIT License
2.23k stars 212 forks source link

Uncacheable eager loaded relationships are incorrectly cached #416

Closed joshuadwire closed 2 years ago

joshuadwire commented 2 years ago

I'm having an issue when I have a cacheable model that is related to other uncacheable models. When I request the cacheable model and eager load the uncacheable model, everything is cached, even the uncacheable models.

The issue seems to be on line 294 here: https://github.com/GeneaLabs/laravel-model-caching/blob/efaeb5fb881fc753fb9ec9d12334280d817d4422/src/Traits/Caching.php#L290-L311

$this in this context is the query builder, which doesn't seem like it would ever have the method referenced by $related. Seems like it should be $this->model? If that's correct, I can submit a PR, but I wanted to be sure.

mikebronner commented 2 years ago

@joshuadwire Thanks for writing in. I would be happy to receive a PR for this -- please add any tests to cover the changes. If the tests pass, we should be good. You can try making the PR without adding any new tests first, then see if the tests pass when they run.

m7vm7v commented 1 year ago

Hi, could we possibly add a config key for reverting this?

Possible case scenario - "I have a comment model with a user relationship that has been eager loaded. The user, as recommended is not cachable, but the comment is. So now, most likely, all the places I need the comment I eager load the user with it. With this change, the user will never be cached so then the comment won't either. I would like to be able to cache a model only when is part of a relationship, meaning - if the user is not cachable, then do not cache it, but if the comment is and the user is eager loaded then cache it as part of the comment cache."

Thanks for the package! Can you please advise if that is currently doable or is not a desired behaviour?

mikebronner commented 1 year ago

@m7vm7v Good question! This is actually desired behavior (to cache eager loaded relationships, even if the relationships are not cachable). The premise here is that the query being run for that model in question is what is being cached, in totality. So the user that the comment is related to will always be contained within that query, and not need to be queried again, unless the comment changes.

m7vm7v commented 1 year ago

Thanks for coming back to me @mikebronner!

I can suggest a solution about going forward could be a trait specified for the none-cachable-model-but-part-of-relationship (the user from the above example) that could have an overwritable property with a list of what models are related to (the comment from the example), so then in the same file this issue relates to could check the list if the user model has listed the comment as relatable if so then cache it. In that new trait, we could also have the check when the user is manipulated/updated then check the new property list and flush the models that relate to (the comment from the example).

Can you please advise if you are looking into implementing this feature(ignoring my suggestion if you have something else in mind of course) and if so when roughly shall we expect it? Thanks again.

mikebronner commented 1 year ago

@m7vm7v Thanks for the follow-up! Unfortunately this is probably not a feature I would consider adding to the package, as it negates its functionality. The end-effect would be that queries for models that have non-cachable relationships just wouldn't be cached as well.