stormpath-sdk-java icon indicating copy to clipboard operation
stormpath-sdk-java copied to clipboard

Non-caching of collections is costing us large numbers of API requests

Open george-hawkins-work opened this issue 9 years ago • 1 comments

We've breached the quota of API requests for our current subscription.

As the given subscription covers a tenant that we reserve for development only we initially suspected a test gone wild.

But this didn't seem to be it - I then looked at the caching in Stormpath and found that we mainly got cache hits in our scenarios, i.e. we tended to hit on the same resources over and over again so rarely got cache misses.

Except that is for any DefaultGroupList resource! These are continuously re-requested.

It turns out that this seems to be an old and known issue in ReadCacheFilter.isCacheRetrievalEnabled(ResourceDataRequest):

//Collection caching is EXPERIMENTAL so it is off by default
//we do cache ApiKeyList. This is a fix for #216
(!CollectionResource.class.isAssignableFrom(clazz) || ApiKeyList.class.isAssignableFrom(clazz) ||
        (CollectionResource.class.isAssignableFrom(clazz) && isCollectionCachingEnabled()));

#216 was filed back in June 2015.

So while we get nearly everything out of the caches, we always have to hit Stormpath each and every time StormpathAuthenticationProvider.getGrantedAuthorities(Account) is called (in order to get the groups that a user belongs to):

ReadCacheFilter.isCacheRetrievalEnabled(ResourceDataRequest) line: 118	
ReadCacheFilter.filter(ResourceDataRequest, FilterChain) line: 54	
DefaultFilterChain.filter(ResourceDataRequest) line: 52	
DecryptApiKeySecretFilter.filter(ResourceDataRequest, FilterChain) line: 63	
DefaultFilterChain.filter(ResourceDataRequest) line: 52	
EnlistmentFilter.filter(ResourceDataRequest, FilterChain) line: 42	
DefaultFilterChain.filter(ResourceDataRequest) line: 52	
DefaultDataStore.getResourceData(String, Class<Resource>, Map<String,?>) line: 330	
DefaultDataStore.getResource(String, Class<T>, Map<String,Object>) line: 245	
DefaultDataStore.getResource(String, Class<T>) line: 233	
DefaultGroupList(AbstractResource).materialize() line: 177	
DefaultGroupList(AbstractResource).getProperty(String) line: 244	
DefaultGroupList(AbstractCollectionResource<T>).getCurrentPage() line: 114	
AbstractCollectionResource$PaginatedIterator<T>.<init>(AbstractCollectionResource<T>, boolean) line: 187	
AbstractCollectionResource$PaginatedIterator<T>.<init>(AbstractCollectionResource, AbstractCollectionResource, boolean, AbstractCollectionResource$1) line: 171	
DefaultGroupList(AbstractCollectionResource<T>).iterator() line: 151	
StormpathAuthenticationProvider.getGrantedAuthorities(Account) line: 364

So at the moment you're actively omitting a certain class of resource from caching not because of some characteristic that means it shouldn't be cached but because the caching functionality was never completed for this particular class of resource.

I think this is a bit problematic if this then affects what customers are billed - as this does.

george-hawkins-work avatar Nov 24 '16 16:11 george-hawkins-work

Ah... I see we probably wouldn't even be hitting this code except for the unnecessary refresh that's addressed by #1092 (which got released in 1.2 two days ago).

OK - I'm heading home now - but I'll upgrade to 1.2 tomorrow and recheck - unfortunately bumping a version always proves more hassle that one might imagine due to the issue described in #1077 (which hasn't been followed up on 😞 ).

Still the underlying issue stands - that you issue API requests for a class of resources not because they shouldn't be cached but because the cache code for handling collections was never completed.

george-hawkins-work avatar Nov 24 '16 17:11 george-hawkins-work