Qualify async cache reads with the acting tenant - #42
Open
Hariomlokhande-coder wants to merge 1 commit into
Open
Hariomlokhande-coder wants to merge 1 commit into
Hariomlokhande-coder wants to merge 1 commit into
Conversation
Signed-off-by: Hariomlokhande-coder <158686388+Hariomlokhande-coder@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this changes
Fixes #41.
TenantAwareCachenow implements bothCache.retrieve(...)overloads, so async@Cacheablekeeps working with TenantLayer on the classpath and stays per tenant.retrieve(key)qualifies the key. No tenant is a miss, same asget(key).retrieve(key, loader)qualifies the key and runs the loader with the caller's tenant bound. With no tenant the loader runs and nothing is stored, same asget(key, Callable).Two things came up beyond the issue:
ConcurrentMapCache.retrieve(key, loader)runs the loader throughsupplyAsyncon the common pool. Without binding the tenant, the cached method ran as no tenant and its empty result got cached under the caller's key, so the loader is wrapped withTenantContext.callWithTenant.AbstractCacheInvoker.doRetrievetakes it for a cache error, and with a logging-onlyCacheErrorHandlerit calls the loader again, so the method would run twice.One thing I documented rather than fixed: without
sync, Spring does theputwhen the future completes, which can be on another thread, and the key gets qualified with whatever tenant is bound there.TenantAwareCachecan't see the original caller at that point, andsync = trueavoids it. Happy to open a separate issue if you'd like that handled at the aspect level.Also,
retrieveis@since 6.1per the Javadoc. I said 6.2 in the issue.Mutation test
Broke: removed both overrides (current
main). Result: all five new tests fail withUnsupportedOperationException.Broke:
retrieve(key, loader)passes the raw key. Result:asyncTenantsDoNotShareCachedValuesandsyncAsyncCacheableIsPerTenantAndLoadsAsTheCallerfail; globex is servedacme invoice 1, load 1.Broke:
retrieve(key)passes the raw key. Result:asyncTenantsDoNotShareCachedValuesandasyncCacheableIsPerTenantfail; acme no longer gets its own value.Broke: loader not bound to the caller's tenant. Result:
syncAsyncCacheableIsPerTenantAndLoadsAsTheCallerfails withno tenant invoice 1, load 1.Broke: no-tenant loader delegated with the raw key. Result:
asyncWithNoTenantBypassesTheCachefails; the loader ran once and the value was stored.Broke: no-tenant loader exception thrown instead of returned as a failed future. Result:
asyncLoaderFailureWithNoTenantIsReportedThroughTheFuturefails.Broke:
tenantlayer.cache.shared=invoices,receiptsin the context tests. Result: both@Cacheabletests fail, so they really go through the wrapper.I broke the implementation and confirmed the new test fails
Every "cannot see the other tenant" assertion is paired with "can see its own rows" (here: acme still gets its own cached value)
Tests connect as a least-privileged role, not a superuser (n/a, these tests don't touch a database)
Public API
TenantResolver,TenantContextStorage,TenantScope,TenantMembershipVerifier,TenantRegistry, or anything inio.tenantlayer.testChecks
git commit -s) — see DCO.mdmvn testpasses. Locally on JDK 21: 158 pass, includingCacheIsolationTest13/13. The 16 Testcontainers classes need Docker, which I don't have on this machine, so I'm relying on CI for those.mvn test -Phibernate7passes, if this touches persistence (n/a)docs/caching.mdandCHANGELOG.md)