Skip to content
This repository has been archived by the owner on Dec 12, 2018. It is now read-only.

Cache misses on non-trivial URL structures #476

Open
4storia opened this issue Jun 10, 2016 · 7 comments
Open

Cache misses on non-trivial URL structures #476

4storia opened this issue Jun 10, 2016 · 7 comments

Comments

@4storia
Copy link

4storia commented Jun 10, 2016

https://github.com/stormpath/stormpath-sdk-node/blob/master/lib/cache/CacheHandler.js#L79

^ Cache entry lookup does a split on the resource href to determine which cache region to perform get/put against. Unfortunately, if I make a request for something more complicated than a region, such as an account's associated groups, this splits against https://api.stormpath.com/v1/accounts/<accId>/groups, which results in the cache region being <accId>, rather than accounts. In essence, because this is not a valid cache region, no cache entry is ever created for this request, meaning every request for an account's groups results in an API call.

As a rudimentary tweak, I would change this logic to from href.split('/').slice(-2)[0] to href.split('/').slice(4)[0], but I'm obviously not intimately familiar with this code, so there may be a reason the code wasn't written that way.

In any event, what I'm really after is a way to cache a user account AND that user's corresponding groups. Because requesting accounts via client.getAccount(href, { expand: 'groups' }) results in the cache being skipped entirely, I figured I would just make two separate API calls and then combine the results. Group memberships change very infrequently in my application, so it makes a lot more sense to cache these values than to request them fresh every time I need them.

@robertjd
Copy link
Member

Thanks for looking into this, we're also discussing this in #143. In short: we explicitly don't cache collections, as it can get you into an inconsistent application state. But as you say, it's not always a problem, especially if you group data does not change much. We are considering adding collection caching, but we want to make it easy to opt-out of this for those who would run into problems with that configuration.

Also: consider using the customData resource of the account, for storing permission information, as that linked resource is cacheable at the moment.

@4storia
Copy link
Author

4storia commented Jun 13, 2016

I figured there was probably a reason it wasn't implemented that way :P

So my only problem with using customData is that I'm also using Stormpath's angular ui-router module, which expects the /me route to return a user account + group data (at least if you want to use group authorization in your routes), so teh customData fields don't really seem to fit that model very well.

What I'm doing right now is essentially what I mentioned in my above post - I cache api.stormpath.com/v1.../accounts/<accId>/group in the accounts cache region manually, so future requests pull from that cached object the same as anything else. This seems to work fine for now, although I'm not super fond of having to do client._dataStore.cacheHandler.cacheManager.getCache('accounts') to access the cache. I'd certainly love some kind of option for client/cache instantiation that allows for more aggressive caching.

I see some forks/possible solutions to this problem in #143 - Is there much work left to be done around this particular issue? (#143 seems to have been open since Feb 2015...)

@robertjd
Copy link
Member

Thanks @4storia , your solution is what we're also discussing on my team (to cache the groups by account ID). I'll see if we can spend some time on this in the coming weeks (we've currently prioritized some other work, specifically to make our authenticators easier to use - and documented). Please follow this issue for updates.

@4storia
Copy link
Author

4storia commented Jun 21, 2016

is there a reason all comments on this issue other than my own were deleted??

@robertjd
Copy link
Member

I don't think there were other comments? I just looked at my email thread and it looks like it's just me and you on here :) are you referring to the other conversation on #143 ?

@4storia
Copy link
Author

4storia commented Jun 24, 2016

that was real weird, I lost all of your comments + all of the issue tickets you'd created. Not really sure what happened there....

@chuanhuizhang
Copy link

#584
Here is a PR to fix account groups collection request caching

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants