Skip to content

Qualify async cache reads with the acting tenant - #42

Open
Hariomlokhande-coder wants to merge 1 commit into
tenantlayer-io:mainfrom
Hariomlokhande-coder:fix/async-cache-retrieve
Open

Hariomlokhande-coder wants to merge 1 commit into
tenantlayer-io:mainfrom
Hariomlokhande-coder:fix/async-cache-retrieve

Conversation

@Hariomlokhande-coder

Copy link
Copy Markdown

What this changes

Fixes #41. TenantAwareCache now implements both Cache.retrieve(...) overloads, so async @Cacheable keeps working with TenantLayer on the classpath and stays per tenant.

  • retrieve(key) qualifies the key. No tenant is a miss, same as get(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 as get(key, Callable).

Two things came up beyond the issue:

  • ConcurrentMapCache.retrieve(key, loader) runs the loader through supplyAsync on 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 with TenantContext.callWithTenant.
  • On the no-tenant path, a loader that throws goes into the returned future instead of escaping. If it escapes, Spring's AbstractCacheInvoker.doRetrieve takes it for a cache error, and with a logging-only CacheErrorHandler it calls the loader again, so the method would run twice.

One thing I documented rather than fixed: without sync, Spring does the put when the future completes, which can be on another thread, and the key gets qualified with whatever tenant is bound there. TenantAwareCache can't see the original caller at that point, and sync = true avoids it. Happy to open a separate issue if you'd like that handled at the aspect level.

Also, retrieve is @since 6.1 per the Javadoc. I said 6.2 in the issue.

Mutation test

  • Broke: removed both overrides (current main). Result: all five new tests fail with UnsupportedOperationException.

  • Broke: retrieve(key, loader) passes the raw key. Result: asyncTenantsDoNotShareCachedValues and syncAsyncCacheableIsPerTenantAndLoadsAsTheCaller fail; globex is served acme invoice 1, load 1.

  • Broke: retrieve(key) passes the raw key. Result: asyncTenantsDoNotShareCachedValues and asyncCacheableIsPerTenant fail; acme no longer gets its own value.

  • Broke: loader not bound to the caller's tenant. Result: syncAsyncCacheableIsPerTenantAndLoadsAsTheCaller fails with no tenant invoice 1, load 1.

  • Broke: no-tenant loader delegated with the raw key. Result: asyncWithNoTenantBypassesTheCache fails; the loader ran once and the value was stored.

  • Broke: no-tenant loader exception thrown instead of returned as a failed future. Result: asyncLoaderFailureWithNoTenantIsReportedThroughTheFuture fails.

  • Broke: tenantlayer.cache.shared=invoices,receipts in the context tests. Result: both @Cacheable tests 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

  • This does not change TenantResolver, TenantContextStorage, TenantScope,
    TenantMembershipVerifier, TenantRegistry, or anything in io.tenantlayer.test
  • …or it does, and I have described the break and the migration below

Checks

  • All commits are signed off (git commit -s) — see DCO.md
  • mvn test passes. Locally on JDK 21: 158 pass, including CacheIsolationTest 13/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 -Phibernate7 passes, if this touches persistence (n/a)
  • No new non-optional dependency
  • Docs updated, if this changes behaviour or adds a property (docs/caching.md and CHANGELOG.md)

Signed-off-by: Hariomlokhande-coder <158686388+Hariomlokhande-coder@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

TenantAwareCache breaks async @Cacheable — Cache.retrieve() is not implemented

1 participant