search

mikebronner / laravel-model-caching

Eloquent model-caching made easy.
MIT License
2.26k stars 217 forks source link

Global Scopes Possibly Not Working Again? #256

Closed kolirt closed 5 years ago

kolirt commented 5 years ago

Describe the bug SQL queries cached, but always return first cached query. The SQL queries make a package, namely a generateQuery method

Help me, please. It's very important problem. I don't know what I need to do to worked it

SQL Query First SQL query

select articles.*, (SELECT `string` FROM `translations` WHERE `translation_id`=`articles`.`id` AND `translation_type`='articles' AND `lang`='uk' AND `key`='name') as `name`, (SELECT `text` FROM `translations` WHERE `translation_id`=`articles`.`id` AND `translation_type`='articles' AND `lang`='uk' AND `key`='short_description') as `short_description`, (SELECT `text` FROM `translations` WHERE `translation_id`=`articles`.`id` AND `translation_type`='articles' AND `lang`='uk' AND `key`='text') as `text`, (SELECT `string` FROM `translations` WHERE `translation_id`=`articles`.`id` AND `translation_type`='articles' AND `lang`='uk' AND `key`='meta_title') as `meta_title`, (SELECT `text` FROM `translations` WHERE `translation_id`=`articles`.`id` AND `translation_type`='articles' AND `lang`='uk' AND `key`='meta_description') as `meta_description` from `articles` where `slug` = 'ivanchenko-bogdan-yosipovich' and `active` = 1 limit 1

Second SQL query

select articles.*, (SELECT `string` FROM `translations` WHERE `translation_id`=`articles`.`id` AND `translation_type`='articles' AND `lang`='en' AND `key`='name') as `name`, (SELECT `text` FROM `translations` WHERE `translation_id`=`articles`.`id` AND `translation_type`='articles' AND `lang`='en' AND `key`='short_description') as `short_description`, (SELECT `text` FROM `translations` WHERE `translation_id`=`articles`.`id` AND `translation_type`='articles' AND `lang`='en' AND `key`='text') as `text`, (SELECT `string` FROM `translations` WHERE `translation_id`=`articles`.`id` AND `translation_type`='articles' AND `lang`='en' AND `key`='meta_title') as `meta_title`, (SELECT `text` FROM `translations` WHERE `translation_id`=`articles`.`id` AND `translation_type`='articles' AND `lang`='en' AND `key`='meta_description') as `meta_description` from `articles` where `slug` = 'ivanchenko-bogdan-yosipovich' and `active` = 1 limit 1

This queries are different by lang key in subquery

Environment

mikebronner commented 5 years ago

@kolirt Please submit the Eloquent queries used in your code. That will help me narrow down the problem. Thanks :)

kolirt commented 5 years ago

@kolirt Please submit the Eloquent queries used in your code. That will help me narrow down the problem. Thanks :)

Problem in this sql subqueries

(SELECT `string` FROM `translations` WHERE `translation_id`=`articles`.`id` AND `translation_type`='articles' AND `lang`='uk' AND `key`='name') as `name`, 
(SELECT `text` FROM `translations` WHERE `translation_id`=`articles`.`id` AND `translation_type`='articles' AND `lang`='uk' AND `key`='short_description') as `short_description`, 
(SELECT `text` FROM `translations` WHERE `translation_id`=`articles`.`id` AND `translation_type`='articles' AND `lang`='uk' AND `key`='text') as `text`, 
(SELECT `string` FROM `translations` WHERE `translation_id`=`articles`.`id` AND `translation_type`='articles' AND `lang`='uk' AND `key`='meta_title') as `meta_title`, 
(SELECT `text` FROM `translations` WHERE `translation_id`=`articles`.`id` AND `translation_type`='articles' AND `lang`='uk' AND `key`='meta_description') as `meta_description`

BlogController

public function article(Request $request, Article $article)
{
    return view('pages.article', compact('article'));
}

Article (model)

class Article extends Model
{

    use Translatable, Cachable, Activable;

    protected $fillable = [
        'name',
        'slug',
        'image',
        'short_description',
        'text',
        'meta_title',
        'meta_description',
        'active'
    ];

    protected $translatable = [
        'name',
        'short_description' => 'text',
        'text' => 'text',
        'meta_title',
        'meta_description' => 'text'
    ];

    public function getRouteKeyName()
    {
        return 'slug';
    }
}

Activable (trait)

trait Activable
{

    public static function bootActivable()
    {
        static::addGlobalScope('active', function($builder){
            $builder->where('active', true);
        });
    }

}

Translatable (trait) (you can see full code this trait)

trait Translatable
{
    public static function bootTranslatable()
    {
        $class = new self;

        if (!empty($class->translatable) && config('translations.active', true)) {
            static::addGlobalScope('translatable', function(Builder $builder) use ($class){
                $builder->addSelect(\DB::raw($class->getTable() . ".*"));

                foreach ($class->translatable as $column => $type) {
                    $builder->addSelect($class->generateQuery($class, $column, $type));
                }
            });
        }
    }

    private function generateQuery($class, $column, $type)
    {
        if (!in_array($type, Translation::COLUMN_TYPE)) {
            $column = $type;
            $type = Translation::COLUMN_TYPE[0];
        }

        return \DB::raw("(SELECT `" . $type . "` FROM `translations` WHERE `translation_id`=`" . $class->getTable() . "`.`" . $class->getKeyName() . "` AND `translation_type`='" . $class->getTable() . "' AND `lang`='" . app()->getLocale() . "' AND `key`='" . $column . "') as `" . $column . "`");
    }
}
mikebronner commented 5 years ago

@kolirt Only eloquent queries will be cached. You are running a Query Builder query, which will not be cached, and could cause problems.

My recommendation would be to rewrite this functionality using eloquent. Since you are using addSelect() to add the Query Builder query to the Eloquent query, I believe the problem lies in that we currently cannot cache select clauses.

I will see if I can recreate this in a unit test, but for now I would recommend rewriting this to be completely eloquent.

kolirt commented 5 years ago

@mikebronner

I can't understand how to rewrite. Please, can you prompt me?

mikebronner commented 5 years ago

@kolirt Actually, I take that back. I just added a test, and it caches the select query correctly. I am unable to replicate your problem.

Sorry, I can't provide support for rewriting app functionality, as I would need complete access to the app, and it could take hours or even days to rewrite something like this.

I can't say for sure why things aren't working as expected. If you could create an integration test that reproduces the issue, I can problem help more, and likely find a solution. For examples, see the last one here: https://github.com/GeneaLabs/laravel-model-caching/blob/master/tests/Integration/CachedBuilder/SelectTest.php

kolirt commented 5 years ago

@mikebronner

I created example project on github and invited you to project

I wrote code that causes an error in TestController. Don't forget to start the migration

I will be grateful for help. Thanks

mikebronner commented 5 years ago

Sounds good, I will take a look later on.

mikebronner commented 5 years ago

@kolirt I tried taking a look at your test project. I was unable to fix the problem, but I know it lies in the bootTranslatable() method. It is not applying the addSelect to the correct object. I'm unable to recreate the issue in my own tests in this package. Why doesn't your taggable package utilize Eloquent for its tag lookups? I'm afraid I don't have more time to dig into this, sorry. If you can create a unit test within this package that reproduces the issue in a PR, I'll be more than happy to help.

kolirt commented 5 years ago

@mikebronner

Seems to be a problem with addGlobalScope. You wrote about this problem here https://github.com/GeneaLabs/laravel-model-caching/issues/199#issuecomment-445429917. When will this problem be solved?

mikebronner commented 5 years ago

Hi @kolirt, this should already be resolved -- global scopes are working in the unit tests: https://github.com/GeneaLabs/laravel-model-caching/blob/master/tests/Integration/CachedBuilder/ScopeTest.php

kolirt commented 5 years ago

@mikebronner

Is it already in release? Local scope works, but global scope doesn't work

mikebronner commented 5 years ago

@kolirt yes, it has been for a while. Global scopes work when applied to the models. I believe that in your case the problem is with how the addSelect is applied within the scope, not the scope itself.

kolirt commented 5 years ago

@mikebronner

I checked. addSelect has nothing to do with the problem. The problem is in the GlobalScope

Model

<?php

namespace App;

use App\Scopes\Activable;
use GeneaLabs\LaravelModelCaching\Traits\Cachable;
use Illuminate\Database\Eloquent\Model;

class Tag extends Model
{

    use Cachable, Activable;

    protected $fillable = ['name', 'description', 'active'];

}

Activable scope

<?php

namespace App\Scopes;

trait Activable
{

    public static function bootActivable()
    {
        static::addGlobalScope('active', function($builder){
            $builder->where('active', 1);
        });
    }

}

Controller

<?php

namespace App\Http\Controllers;

use App\Tag;
use Illuminate\Http\Request;

class TestController extends Controller
{

    public function index(Request $request)
    {
        dump('with globalScope');
        $tags = Tag::query()->get();
        foreach ($tags as $tag) {
            dump($tag->toArray());
        }

        dump('without globalScope');
        $tags = Tag::query()->withoutGlobalScope('active')->get();
        foreach ($tags as $tag) {
            dump($tag->toArray());
        }

        dump('without globalScope and cacheable');
        $tags = Tag::query()->disableCache()->withoutGlobalScope('active')->get();
        foreach ($tags as $tag) {
            dump($tag->toArray());
        }
    }

}

Result

image

mikebronner commented 5 years ago

@kolirt I'm going through the global scope tests and re-evaluating the process, as well as adding a test for inline global scopes. I will report back in a bit.

mikebronner commented 5 years ago

@kolirt My apologies, and thank you for being persistent. I have confirmed that the GlobalScope functionality is not working as intended. I am working on a fix, hopefully I will have something ready over the next day or so.

mikebronner commented 5 years ago

@kolirt Can you give release 0.5.4 a try and let me know how it goes?

kolirt commented 5 years ago

@mikebronner

It works. Thanks

mikebronner commented 5 years ago

Awesome, that's great to hear. Be sure to update again ... I released version 0.5.5 which adds support for the soft-delete-related macros.

kolirt commented 4 years ago

Hi there! Unfortunately, this problem is back again :(

mikebronner commented 4 years ago

@kolirt please prove all the details necessary to debug this: pacjage version, Laravel version, full stack trace of any error, etc.

mikebronner commented 4 years ago

@kolirt Would you mind testing this against release 0.8.4 or newer? Additional functionality has been added to deal with global scopes. Please let me know how it works for you.