search

mikebronner / laravel-model-caching

Eloquent model-caching made easy.
MIT License
2.24k stars 214 forks source link

User (no cache) Notifications (with cache) pivot paginate not refreshing #326

Closed dhcmega closed 4 years ago

dhcmega commented 4 years ago

Describe the bug User has multiple notifications (belongsToMany). The pivot table has a "read" column. When a user reads a notification I run updateExistingPivot. But the next read of user->notifications() is cached and not updated. User has no Cachable. Notifications has Cachable.

Eloquent Query

        $user = \App\Models\User::with(['notifications' => function ($query) {
            $query->where('notifications.id', 1352);
        }])->find(105);

        $notification = $user->notifications->first();

        dump($notification->pivot->read); // read is 0, unread.

        $user->notifications()->updateExistingPivot($notification->id, ['read' => 1]);

        $user = \App\Models\User::with(['notifications' => function ($query) {
            $query->where('notifications.id', 1352);
        }])->find(105);

        $notification = $user->notifications->first();

        dump($notification->pivot->read); // read is 1, read

        $received = $user->notifications()
            ->orderBy('notifications.id', 'desc')
            ->paginate(1, ['notifications.id', 'notifications.account_id', 'notifications.title', 'notifications.message', 'notifications.created_at', 'notifications.menu_id', 'notifications.element_id'], 'page', 0)
            ->items();

        dump($received[0]->pivot->read); // read is 0, unread (cached?)

Stack Trace No exceptions thrown

Environment

Additional context I tried with ['*'] for the columns, same problem. I tried to use Cachable on User but I get trait collision. Paginate on belongsToMany uses ->items() which is a ->all() method. In case all() is not using Cachable or something.

EDIT1: Manually flushing fixes it: $notification->flushCache(); before running the pagination query

mikebronner commented 4 years ago

Hi @dhcmega! You mention you get a trait collision when adding caching to the user? What trait and package is it colliding with? If you have another package that overrides EloquentQueryBuilder, this is likely the problem preventing the caching from working correctly, even if you are not caching the user.

Please post your User model class. Also, please post your composer.json.

dhcmega commented 4 years ago

Hi @mikebronner, thanks for getting back to me so fast. I have removed the Metable trait and the problem was solved. It was also overriding __get. The package is "kodeine/laravel-meta": "master",. I wasn't caching the user, Cachable is there to confirm no collision after removing Metable.

User.php

<?php

namespace App\Models;

use Eloquent as Model;
use App\Http\Scopes\AccountScope;
use GeneaLabs\LaravelModelCaching\Traits\Cachable;
use Illuminate\Database\Eloquent\SoftDeletes;
use Kodeine\Metable\Metable;
use Silber\Bouncer\Database\HasRolesAndAbilities;

/**
 * Class User
 * @package App\Models
 * @version July 11, 2017, 5:06 pm UTC
 */
class User extends Model
{
    use SoftDeletes, HasRolesAndAbilities, Cachable;//, Metable;

    const CREATED_AT = 'created_at';
    const UPDATED_AT = 'updated_at';

    public $auditEvents = [
        0 => "created",
        1 => "updated",
        2 => "deleted",
        3 => "restored",
    ];

    public $metaClass = \App\Models\MetaData::class;

    public $table = 'users';
    public $fillable = [
        'account_id',
        'first_name',
        'last_name',
        'email',
        'password',
//        'remember_token'
    ];
    protected $dates = ['dob', 'deleted_at'];
    /**
     * The attributes that should be casted to native types.
     *
     * @var array
     */
    protected $casts = [
        'id' => 'integer',
        'account_id' => 'integer',
        'record' => 'string',
        'first_name' => 'string',
        'last_name' => 'string',
        'email' => 'string',
        'password' => 'string',
        'remember_token' => 'string',
        'verified' => 'integer',
        'active' => 'integer',
    ];

    /**
     * The attributes that should be hidden for arrays.
     *
     * @var array
     */
    protected $hidden = [
        'password', 'remember_token',
    ];

    /**
     * The attributes that should added to the model.
     *
     * @var array
     */
    protected $appends = [
        'full_name',
    ];

    /**
     * Prepare a date for array / JSON serialization.
     *
     * @param  \DateTimeInterface  $date
     * @return string
     */
    protected function serializeDate(\DateTimeInterface $date)
    {
        return $date->format('Y-m-d H:i:s');
    }

    public static function calculateUnread($guard, $user, $unread = 'all')
    {
        $unread_count = 0;
        $time_start = microtime(true);
            if ($unread == 'notification' || $unread == 'all') {
                $unread_count += $user->notifications()
                    ->wherePivot('read', 0)
                    ->count();

            }

        $exec_time = number_format(microtime(true) - $time_start, 3, ',', '.');
        \Log::debug($guard . ' Unread Execution time for user_id(' . $user->id . '): ' . $exec_time . 's: ' . $unread . ' ' . var_export($unread_count, 1));

        return $unread_count;
    }

    /**
     * The "booting" method of the model.
     *
     * @return void
     */
    protected static function boot()
    {
        parent::boot();

        static::addGlobalScope(new AccountScope);
    }

    /**
     * Get the user's full name.
     *
     * @param string $value
     * @return string
     */
    public function getFullNameAttribute()
    {
        return mb_convert_case($this->first_name . ' ' . $this->last_name, MB_CASE_TITLE, "UTF-8");
    }

    /**
     * @return \Illuminate\Database\Eloquent\Relations\BelongsTo
     **/
    public function account()
    {
        return $this->belongsTo(\App\Models\Account::class);
    }

    /**
     * @return \Illuminate\Database\Eloquent\Relations\BelongsToMany
     **/
    public function notifications()
    {
        return $this->belongsToMany(\App\Models\Notification::class)->withPivot('read')->withTimestamps();
    }
}

composer.json

{
    "name": "laravel/laravel",
    "type": "project",
    "description": "The Laravel Framework.",
    "keywords": [
        "framework",
        "laravel",
    ],
    "license": "MIT",
    "require": {
        "php": "^7.2",
        "barryvdh/laravel-snappy": "^0.4.6",
        "doctrine/dbal": "~2.3",
        "fideloper/proxy": "^4.0",
        "genealabs/laravel-model-caching": "*",
        "guzzlehttp/guzzle": "^6.4",
        "infyomlabs/adminlte-templates": "7.0.x-dev",
        "infyomlabs/laravel-generator": "7.0.x-dev",
        "intervention/image": "^2.5",
        "kodeine/laravel-meta": "master",
        "laravel/framework": "^7.0",
        "laravel/helpers": "^1.2",
        "laravel/tinker": "^2.0",
        "laravel/ui": "^2.0",
        "laravelcollective/html": "^6.0",
        "league/flysystem-aws-s3-v3": "^1.0",
        "milon/barcode": "^7.0",
        "owen-it/laravel-auditing": "^10.0",
        "silber/bouncer": "v1.0.0-rc.8",
        "spatie/laravel-image-optimizer": "^1.5",
        "tymon/jwt-auth": "^1.0",
        "yajra/laravel-datatables-buttons": "^4.0",
        "yajra/laravel-datatables-html": "^4.0",
        "yajra/laravel-datatables-oracle": "~9.0"
    },
    "require-dev": {
        "barryvdh/laravel-debugbar": "^3.2",
        "facade/ignition": "^2.0",
        "fzaninotto/faker": "^1.4",
        "mockery/mockery": "^1.0",
        "nunomaduro/collision": "^4.1",
        "nunomaduro/larastan": "^0.5.5",
        "phpunit/phpunit": "^8.5"
    },
    "config": {
        "optimize-autoloader": true,
        "preferred-install": "dist",
        "sort-packages": true
    },
    "extra": {
        "laravel": {
            "dont-discover": []
        }
    },
    "autoload": {
        "psr-4": {
            "App\\": "app/"
        },
        "classmap": [
            "database/seeds",
            "database/factories"
        ],
        "files": [
            "app/Helpers/Helper.php"
        ]
    },
    "autoload-dev": {
        "psr-4": {
            "Tests\\": "tests/"
        }
    },
    "minimum-stability": "dev",
    "prefer-stable": true,
    "scripts": {
        "post-autoload-dump": [
            "Illuminate\\Foundation\\ComposerScripts::postAutoloadDump",
            "@php artisan package:discover --ansi"
        ],
        "post-root-package-install": [
            "@php -r \"file_exists('.env') || copy('.env.example', '.env');\""
        ],
        "post-create-project-cmd": [
            "@php artisan key:generate --ansi"
        ]
    }
}
dhcmega commented 4 years ago

Is it possible to create my own Trait that includes both traits and I override __get with the code from each one?

mikebronner commented 4 years ago

@dhcmega See the "Conflict Resolution" section here: https://www.php.net/manual/en/language.oop5.traits.php. You will have to write your own __get() method that first calls the aliased __get() method from one trait, and then the aliased __get() method from the other trait.

Let me know how this goes and what the method looks like, I have not had to do this myself yet, but would be good to add to the documentation for others. Good luck! :)

dhcmega commented 4 years ago

@mikebronner I think I have solved it by mixing both methods.

Calling them with alias fails, I'm afraid it's not possible in this case: User has Metable Notifications has Cachable

Maybe I'm getting it wrong. I have created a Trait that aliased the methods, but it fails. Probably becase of the returns in the middle. The Traits it self works ok.

trait CachableMetable
{
    use Cachable, Metable {
        Cachable::__get as __getCachable;
        Cachable::__set as __setCachable;
        Metable::__get as __getMetable;
        Metable::__set as __setMetable;
    }

    public function __get($attr)
    {
        $this->__getCachable($attr);
        $this->__getMetable($attr);

        return parent::__get($attr);
    }

    public function __set($key, $value)
    {
        $this->__setCachable($key, $value);
        $this->__setMetable($key, $value);

        return parent::__set($key, $value);
    }

This is what I think works, I still have to use it more for a few days I guess. It's a combination of both get and set

User -> now uses CachableMetable Notifications ->keeps using Cachable

I fail to understand why it works though

<?php

namespace App\Traits;

use GeneaLabs\LaravelModelCaching\Traits\Cachable;
use Illuminate\Support\Str;
use Kodeine\Metable\Metable;

trait CachableMetable
{
    use Cachable, Metable;

    public function __get($attr)
    {
        // From cachable
        if ($attr === "cachePrefix") {
            return $this->cachePrefix
                ?? "";
        }

        if ($attr === "cacheCooldownSeconds") {
            return $this->cacheCooldownSeconds
                ?? 0;
        }

        // From Metable
        // Check for meta accessor
        $accessor = Str::camel('get_' . $attr . '_meta');

        if (method_exists($this, $accessor)) {
            return $this->{$accessor}();
        }

        // Check for legacy getter
        $getter = 'get' . ucfirst($attr);

        // leave model relation methods for parent::
        $isRelationship = method_exists($this, $attr);

        if (method_exists($this, $getter) && !$isRelationship) {
            return $this->{$getter}();
        }

        return parent::__get($attr);
    }

    public function __set($key, $value)
    {
        // From Cachable
        if ($key === "cachePrefix") {
            $this->cachePrefix = $value;
        }

        if ($key === "cacheCooldownSeconds") {
            $this->cacheCooldownSeconds = $value;
        }

        // From Metable
        // ignore the trait properties being set.
        if (Str::startsWith($key, 'meta') || $key == 'query') {
            $this->$key = $value;

            return;
        }

        // if key is a model attribute, set as is
        if (array_key_exists($key, parent::getAttributes())) {
            parent::setAttribute($key, $value);

            return;
        }

        // If there is a default value, remove the meta row instead - future returns of
        // this value will be handled via the default logic in the accessor
        if (
            isset($this->defaultMetaValues) &&
            array_key_exists($key, $this->defaultMetaValues) &&
            $this->defaultMetaValues[$key] == $value
        ) {
            $this->unsetMeta($key);

            return;
        }

        // if the key has a mutator execute it
        $mutator = Str::camel('set_' . $key . '_meta');

        if (method_exists($this, $mutator)) {
            $this->{$mutator}($value);

            return;
        }

        // if key belongs to meta data, append its value.
        if ($this->metaData->has($key)) {
            /*if ( is_null($value) ) {
                $this->metaData[$key]->markForDeletion();
                return;
            }*/
            $this->metaData[$key]->value = $value;

            return;
        }

        // if model table has the column named to the key
        if (\Schema::connection($this->connection)->hasColumn($this->getTable(), $key)) {
            parent::setAttribute($key, $value);

            return;
        }

        // key doesn't belong to model, lets create a new meta relationship
        //if ( ! is_null($value) ) {
        $this->setMetaString($key, $value);
        //}
    }
}
dhcmega commented 4 years ago

Oh but this way I'm making User Metable AND Cachable. I will keep looking into this.

mikebronner commented 4 years ago

@dhcmega which is OK. It's not wrong to make users cachable.