Closed mycarrysun closed 4 years ago
mikebronner
commented
5 years ago @mycarrysun Thanks for reporting this. This has critical performance implications, so I'll try to look into it as soon as possible, but might not be before the weekend. I'm pretty swamped this week.
mycarrysun
commented
5 years ago No worries - for me specifically I have rolled back to a forked version so am fine for now. Can't speak for others though
mikebronner
commented
5 years ago @mycarrysun Does the forked version include a fix?
mycarrysun
commented
5 years ago No it is just a previous version before the change that introduced the bug. Not sure where the bug is happening.
mikebronner
commented
5 years ago Can you link the fork? That might help me identify when the bug was introduced.
mycarrysun
commented
5 years ago https://github.com/mycarrysun/laravel-model-caching
Here you go :)
mikebronner
commented
4 years ago @mycarrysun Thanks for the link to the fork. That helps. Would you be able to also provide me with your global scope declarations and your complete Order model? Thanks!
mycarrysun
commented
4 years ago @mikebronner sorry man - can't provide the complete Order model but I can provide some relevant code including the global scopes on Order
mycarrysun
commented
4 years ago // These scopes are global scopes applied to the Order model
namespace Scopes;
use Illuminate\Database\Eloquent\Builder;
use Illuminate\Database\Eloquent\Model;
use Illuminate\Database\Eloquent\Scope;
class RecentScope implements Scope
{
public function apply(Builder $builder, Model $model)
{
$builder->orderBy($model->getTable().'.id', 'DESC');
}
}
//
namespace Scopes;
use Illuminate\Database\Eloquent\Builder;
use Illuminate\Database\Eloquent\Model;
use Illuminate\Database\Eloquent\Scope;
use Illuminate\Support\Facades\Auth;
class RestrictByCompany implements Scope
{
public function apply(Builder $builder, Model $model)
{
if ($user = Auth::user()) {
if ($user->is_customer) {
return $builder->where($model->getTable().'.company_id', $user->company_id);
}
}
return $builder;
}
}Here is the call where I create the query:
Order::with(Order::$RELATIONS[$relations])
->selectAll()
->withCompany()
->withGlobalScope('with_created_user', new ScopeWithCreatedUser)
->withGlobalScope('with_contact', new ScopeWithContact)
->when($user->is_customer, function ($query) {
return $query->withGlobalScope('restrict_drafts_and_cart', new RestrictDraftsAndCart);
})
->withCarrierCount()
->withCommentCount();Here are the relevant scopes for that call:
class ScopeWithCreatedUser implements Scope
{
public function apply(Builder $query, Model $model)
{
return $query->join(\DB::raw('users cu'), 'cu.id', '=', $model->getTable().'.created_by')
->addSelect(\DB::raw('concat(coalesce(cu.first_name, ""), " ", coalesce(cu.last_name, "")) as created_by_name'));
}
}
class ScopeWithContact implements Scope
{
public function apply(Builder $query, Model $model)
{
return $query->selectSub(function ($query) use ($model) {
return $query->select([DB::raw('CONCAT(COALESCE(users.first_name, ""), " ", COALESCE(users.last_name, "")) as contact_name')])
->from('users')
->where('users.id', DB::raw($model->getTable().'.contact_id'));
}, 'contact_name');
}
}
class RestrictDraftsAndCart implements Scope
{
public function apply(Builder $builder, Model $model)
{
if ($user = Auth::user()) {
$account_types = [User::CUSTOMER, User::CUSTOMER_ADMIN];
if ($user->is_staff) {
return $builder;
}
return $builder->where(function ($query) use ($model, $account_types) {
return $query->whereIn($model->getTable().'.status_id', ['draft', 'in_cart', 'staff_cart'])
->whereIn(DB::raw('(select users.account_type_id from users where users.id = '
.$model->getTable()
.'.created_by)'), $account_types);
})->orWhereNotIn($model->getTable().'.status_id', ['draft', 'in_cart', 'staff_cart']);
}
return $builder;
}
}Now scopes on the Order model:
public function scopeWithCarrierCount($query)
{
return $query->selectSub(function ($query) {
return $query->select(DB::raw('count(distinct permits.carrier_id)'))
->from('permits')
->where('permits.order_id', DB::raw('orders.id'))
->whereNull('permits.deleted_at');
}, 'carrier_count');
}More relevant scopes:
class ScopeWithCommentCount implements Scope
{
/**
* Get the `comment_count` and `important` properties
*
* @param Builder $query
* @param Model $model
*
* @return Builder|\Illuminate\Database\Query\Builder
*/
public function apply(Builder $query, Model $model)
{
$user = Auth::user();
return $query->leftJoin('comments', DB::raw('(comments.commentable_type'), '=', DB::raw('"'
.Helpers::getModelRelationName($model)
.'" and comments.commentable_id = '
.$model->getTable()
.'.id and comments.deleted_at is null'.($user && $user->is_customer ? ' and comments.private = 0' : '')
.')'))
->addSelect(DB::raw('count(distinct comments.id) as comment_count'))
->addSelect(DB::raw('(select count(distinct comments.id) as important where comments.important = 1 limit 1) as important'))
->groupBy($model->getTable().'.id');
}
}The only scope not posted here is the withCompany() and it is a simple inner join on the company_id column.
Hope this helps
saernz
commented
4 years ago I've also just ran into the same bug. It looks like it's just because Eloquent reapply the scopes after the closure in retrieveCachedValue calls the parent's get() method on the eloquent builder. There does not appear to be any checks in place by eloquent to avoid reapplying the scopes as I guess they don't expect them to be already applied. So the scopes first get applied after calling makeCacheKey() then get reapplied later by eloquent in their get() method of their builder. This could possibly be fixed if Buildable overrides the eloquent builders applyScopes() method and just wrapped the parent function in an if which checks the scopesAreApplied variable, if they're applied just return the builder if they're not applied pass through to parent.
mikebronner
commented
4 years ago @saernz @mycarrysun Would you guys try this again with version 0.8.7 or greater? Thanks @saernz for posting the solution!
mycarrysun
commented
4 years ago @saernz @mycarrysun Would you guys try this again with version 0.8.7 or greater? Thanks @saernz for posting the solution!
Unfortunately I don't work at the company where we used this package but hopefully @saernz can test. Looks like his unit tests covered it pretty well though.
mikebronner
commented
4 years ago Sorry to hear that @mycarrysun. All the best to you in your future positions!
saernz
commented
4 years ago @mikebronner Sorry only just seen this. I tested it out straight away after your merge, working well as far as I can tell. I'm on 0.8.8. I will be merging into staging then prod very soon. I actually needed this fix so I could reenable the package in prod again.
mikebronner
commented
4 years ago @saernz Awesome, keep me posted. :)
Describe the bug When applying scopes, if there is a join it is applied twice.
Eloquent Query Please provide the complete eloquent query that caused the bug, for example:
Stack Trace The stacktrace below was generated with a
dd()insideIlluminate\Database\Query\Builder::join()only on the second time the function is fired for the same table. I confirmed that the scope is not being applied twice in the scope's handler using the same method. It is also in reverse order so the last line is the most recent function call.Environment