[9.x] Adds cache refresh() (now its final)
#42342
Conversation
|
@DarkGhostHunter i really like this idea. However, one use case I would use for this is to make a http call to retrieve and store a oAuth 2 client credential token. My only concern is that the ttl value to use for the very first call when the cache is empty cannot be made out of the actual value that got returned by the callback sense this should be the value that should be cached. example here: cache()->upsert(“token”, fn($message) => Http::post(“some-url”)->json(), $ttlComputedFromCallback)Background: Can this maybe be addressed in this pr? |
Great idea. I can add an anonymous object that stores the item TTL, so you can change it, as second parameter. IMHO it's better than returning a an object with the item and a new TTL. use Illuminate\Support\Facades\Http;
use Illuminate\Support\Facades\Cache;
Cache::upsert('token', function ($token, $expire) {
$response = Http::post("https://oauth.service.com");
$expire->at = $response->json('expires_at');
return $response;
});Using that object you could also add or remove the expiration time at will inside the function. |
|
@DarkGhostHunter that would be a great addition! 👍 |
|
I was trying to add some similar functionality called I think the 'updating existing items' is the missing feature in cache system https://stackoverflow.com/questions/48110089/pushing-value-to-existing-cache-value-in-laravel/48110145 |
I think mine has more chances of success because it covers more use cases than just pushing an array, and using |
|
This does generally have a race condition since you are calling get and then put as two separate operations. |
There is a way to use get & put in one operation? My other solution brings locking. I mean, I can go the extra mile for sake of having one callback to bring upserts without race conditions. |
|
What about this approach? Seems a little overkill but it works with locking only if the store supports it, otherwise it uses a simple get->put. $new = Message::latest()->first();
Cache::of('messages')->upsert(function ($messages, $expire) use ($new) {
return Collection::wrap($messages)->add($new);
});It supports managing the lock fluently. If the store doesn't support locking, these methods do nothing . Cache::of('messages')->lockBy(20)->owner('my-app-1')->wait(10)->upsert( /* ... */ );I updated this PR with this approach anyway. I suggest somebody checks the locking mechanism if it's okay. |
|
Eh, on second thought I think it's mainly the method name that I think is a bit confusing. ALL Cache::put calls are effectively already "upserts" in the database sense. They update a key's value if it exists. If it doesn't, the key is created with the associated value. So, Cache::put is already "upsert". But, you are wanting a new method that conveniently lets you modify the retrieved value then put it back in the cache with one method call. |
Exactly. I hereby resign mine own liberty to nameth the method, unless it's I double down on the lock mechanism, if someone can check it's okay. |
|
Ready for review guys. |
|
|
Noone else? Last call! |
getSet()refresh() (now its final)
|
It's Since the developer expects an atomic operation (the value doesn't change while it's being manipulated), this will be done only if the store supports locking, otherwise an exception will be thrown. ✅ Doesn't calls the cache store if nothing to put. Updated initial post to reflect changes. I 100% confident this is final. |
|
@DarkGhostHunter Taylor will review this once he has time. |
|
Thanks for your pull request to Laravel! Unfortunately, I'm going to delay merging this code for now. To preserve our ability to adequately maintain the framework, we need to be very careful regarding the amount of code we include. If possible, please consider releasing your code as a package so that the community can still take advantage of your contributions! If you feel absolutely certain that this code corrects a bug in the framework, please "@" mention me in a follow-up comment with further explanation so that GitHub will send me a notification of your response. |
|
May be “getset” is better. Well, naming things it’s not my forte.
I double down on the lock code review.
Italo Baeza C.
… El 12-05-2022, a la(s) 11:17, Taylor Otwell ***@***.***> escribió:
Eh, on second thought I think it's mainly the method name that I think is a bit confusing. ALL Cache::put calls are effectively already "upserts" in the database sense. They update a key's value if it exists. If it doesn't, the key is created with the associated value. So, Cache::put is already "upsert". But, you are wanting a new method that conveniently lets you modify the retrieved value then put it back in the cache with one method call.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you were mentioned.
|
What?
Retrieves an item from the Cache and stores back the new value, refreshing its lifetime.
The expiration time can be computed within the callback using the second parameter.
Behind the scenes it uses the lock to avoid data races, with a waiting default of 10 seconds (same as the HTTP Client). Not issuing a callback returns a
RefreshOperationinstance to further change the lock configuration.Caveats?
Well, if the item doesn't exist, the callback argument is
null. Since it's a callback, the developer has total control on what to do for a default value.Works exclusively with cache locks.
BC?
None, just adding a method... and a class.