Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Inavalidation Problems since last version #91

Closed
jetwes opened this issue Feb 28, 2018 · 32 comments
Closed

Inavalidation Problems since last version #91

jetwes opened this issue Feb 28, 2018 · 32 comments
Assignees

Comments

@jetwes
Copy link

jetwes commented Feb 28, 2018

I just updated to the latest version and now i have strange errors - maybe something todo with invalidation. (or not invalidating)

Example:
In my SalesController i prepare to add another product to the basket.
$saleItem = SaleItem::where('id','=',$request->get('id'))->first(); $product = Product::where('id',$request->get('product_id'))->first(); $saleItem->product_id = $product->id;
Now sometimes - mostly when more than one product is in the basket i get this error:
Property [id] does not exist on this collection instance. (Regarding the line $product->id)
If i disable the cache everything is ok. Before i updated to the current version everything was fine, too.

What could be the problem over here. I have the same error on different Controllers - not only regarding products.. I disabled the whole cache now - because i'm getting many errors like these...

PHP 7.1
Laravel 5.5
latest package version

@mikebronner
Copy link
Owner

Thanks, @jetwes , I will look into this. Have you been able to see any kind of pattern that might be causing this? Are you using the cache cool down option?

@jetwes
Copy link
Author

jetwes commented Feb 28, 2018

No, i'm not using the cool down option. I cant'see a pattern at the moment.
Another example:
if ($user->last_shop_id != null) { $shop = Shop::where('id', '=', $user->last_shop_id)->first(); } else { $shop = Shop::where('id', '=', $user->main_shop_id)->first(); } $user->setShop(['id' => $shop->id, 'name' => $shop->shortname]);

I get an Trying to get property of non-object error on the last line - it seems that $shop isn't fetched correctly. If i disable the cache everything is fine. It's on my login controller - within the setShop method i set a default shop for my session. I don't cache the user model. If i disable the cache on the shop model - everything is ok. I was on version 0.2.23 before updating this morning. The issues started right after the update to the latest version.

additional info: I'm using redis 2.0.8 (latest) and a seperate db for model caching

@jetwes
Copy link
Author

jetwes commented Feb 28, 2018

It seems a bit like if the queries are hit in short time the error occurs (just a guess) - but i'm not using cool down. I have about 170 users that are working within this app (all at the same time, it's a business portal for managing mobile phone shops) averaging about 90.000 Request per 24h..

@jetwes
Copy link
Author

jetwes commented Feb 28, 2018

Another example:
$shops = Shop::where('active',1)->where('shortname','!=','Zentrale')->where('micro_region',Shop::where('id',Auth::user()->main_shop_id)->first()->micro_region)->orderBy('shortname','ASC')

Error: Exception · Property [micro_region] does not exist on this collection instance.

The errors don't accur on every requests - but a couple of times in a minute...

@mikebronner
Copy link
Owner

@jetwes thanks again for the details. It almost sounds like different users are overriding the query cache in different circumstances, which if true, means that some keys are not granular enough, and even though you specified `first()1st it's actually returning the collection.

@mikebronner
Copy link
Owner

@jetwes I had another thought. If you could see if you can get this error to happen again in your dev environment, would you be able to look at the collection that it errors on, and see what query created that collection? If so, that would be the smoking gun we need to identify the source of the problem. In the mean while I am writing a few more tests to cover some scenarios I can think of.

@jetwes
Copy link
Author

jetwes commented Feb 28, 2018

ok, i'll have a look at it, later

@jetwes
Copy link
Author

jetwes commented Feb 28, 2018

Ok - seems that the mess is on my side. I have a couple of old queries ending ->get()[0]; instead of using ->first();
All this queries fail. When i change them, flush the cache - than they are working.
Sorry for confusing. I will monitor this further...

But there's a strange thing. php artisan modelCache:flush --model=Portal\Models\Sale
I tried php artisan modelCache:flush --model=Portal\\Models\\Shop to flush the cache model and i get the error 'Portal\Models\Sale' is not an instance of CachedModel.
The Model uses the trait Cachable...

namespace Portal\Models;

/**

  • Class Shop.
    */
    class Shop extends PortalBaseModel

namespace Portal\Models;

use GeneaLabs\LaravelModelCaching\Traits\Cachable;
use Illuminate\Database\Eloquent\Model;

abstract class PortalBaseModel extends Model
{
use Cachable;
}

@mikebronner
Copy link
Owner

Hi @jetwes, awesome, thanks for researching this further.

Try the following on the commandline: php artisan modelCache:flush --model=\\Portal\\Models\\Shop (not the leading slash). Let me know if that doesn't work.

@jetwes
Copy link
Author

jetwes commented Feb 28, 2018

nope - same error - on all my models. I can't flush the model from the command line..

did you change the way you return collections in the last couple of versions? Just asking because my get()[0] queries were ok in the old version...

@mikebronner
Copy link
Owner

Thanks for checking. I will look into why models can't be flushed from the command-line and see if there is an edge-case.

The way data is stored in the cache changed, but it shouldn't affect being able to access an index on a collection (since get should always return a collection, which is stored in cache). Could you provide me with a full query that you are trying to run, ending in [0]. I will use that to write a test to see what happens.

In the mean time, I would highly recommend using a dedicated cache store for model-caching, if you haven't already done so. You can then flush the cache for models without affecting your normal cache, by simply omitting the model options on the command-line: php artisan modelCache:flush. I do still need to document that better on the readme -- both how to flush the complete cache, as well as set up a dedicated cache.

@jetwes
Copy link
Author

jetwes commented Feb 28, 2018

i'm using redis in a seperate database.

An example Query:
$saleItem = SaleItem::where('id','=',Request::input('id'))->get()[0];

I already refactored most of the old queries to first() - i will monitor this in the next days..

@mikebronner
Copy link
Owner

Thanks! I will try to make a test case this evening and report back

@mikebronner
Copy link
Owner

Hi @jetwes, I tried to replicate the problem, but was unable to get a failure out of it. See my unit test here (44fccfc). If you can think of anything or provide steps to reproduce, or even a unit test, that would be awesome!

@mikebronner
Copy link
Owner

Also, you could simplify your query, by using find(), perhaps like so:

$saleItem = SaleItem::find(Request::input('id'));

@mikebronner
Copy link
Owner

Also, I covered the process I went through here, let me know if I missed something: https://t.co/CHKiHkfYGz

I would probably suggest opening a new issue if you find a problem, as the original issue on this one has been resolved, and we're working on secondary things now. :)

@mikebronner
Copy link
Owner

I am able to replicate the flushing issue ... working on that separate from this issue

@mikebronner
Copy link
Owner

Fix for model flush command will be in the next release in a few minutes. :)

@jetwes
Copy link
Author

jetwes commented Mar 1, 2018

Thank you!

@jetwes
Copy link
Author

jetwes commented Mar 1, 2018

The problem still persists. Changing to first() fixed the error message itself - but the value is still not fetched from the cache - sometimes.
image
In this example i display a list of users. The query is
$users = User::with('role')->with('shop')->with('user_salaries')->get();
As you can see in the pitcture the field under "Bonn-Duisdorf" is empty . Thats wrong - when i disable the cache i see the correct value "Münster-Hiltrup".

The view for this looks like {{ $user->main_shop($user->main_shop_id)->shortname }}
Method main_shop in my user model:
public function main_shop($id) { return Shop::where('id', '=', $id)->first(); }

@mikebronner
Copy link
Owner

mikebronner commented Mar 1, 2018

Is Shop also using Cachable? In your view, can dump out the user I'd in the loop, before you display the main_shop? Could you post a demo repo with just the view, route, and controller needed to get this specific page to work? That way I could try to replicate the problem. I suspect the issue lies somewhere other than in the query itself.

@jetwes
Copy link
Author

jetwes commented Mar 1, 2018

i dumped out the user id - it's the correct user_id (the other list entries are correct - so no surprise)
image
Yes, Shop is cachable.
I try to post a demo repo .

@jetwes
Copy link
Author

jetwes commented Mar 1, 2018

by the way - the user is not cachable.

@jetwes
Copy link
Author

jetwes commented Mar 2, 2018

i posted it to a repo - you got an invite link. Its straighforwarded - i uploaded vendor, too. so just clone it and you are ready. a sql is inside the migrations folder for quick generetion of the db tables.
I deleted all other stuff, like auth etc.

@mikebronner
Copy link
Owner

Got the invite, thanks! Will take a look at it tomorrow :)

@mikebronner
Copy link
Owner

@jetwes I started installing the repo, but it appears the migration folder is missing. Could you add that for me? :) Thanks!

@mikebronner mikebronner reopened this Mar 3, 2018
@jetwes
Copy link
Author

jetwes commented Mar 5, 2018

oh sorry - i'm looking for it this evening - i was sick the last couple of days - sorry for the delay

@mikebronner
Copy link
Owner

Hope you feel better - gute Besserung!

@jetwes
Copy link
Author

jetwes commented Mar 7, 2018

thank you mike - danke schön ;)
Just uploaded the folder

@mikebronner
Copy link
Owner

@jetwes I fixed an issue with first() not invalidating, could you try again with the latest version?

@jetwes
Copy link
Author

jetwes commented Mar 12, 2018

@mikebronner it seems to work as expected! I will have a look on the logs today - but it's looking good at the moment!

@mikebronner
Copy link
Owner

Closing for now. If it crops up again, please open a new issue. :)

Repository owner locked as resolved and limited conversation to collaborators Mar 12, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants