Comments from #49: Tenant Scoping#68
Conversation
SummaryThe following content is AI-generated and provides a summary of the pull request: Tenant-Scoped Cache for Recommendation HandlerRefactor / New Feature♻️ Refactors the entity prediction cache in Previously, the cache used plain entity names as keys, meaning entries were shared across tenants. Since entity prediction requirements can differ per tenant (e.g., due to tenant-specific model extensions), the cache is now keyed by Changes
PR Bot InformationVersion:
|
![hyperspace-insights[bot]](https://avatars.githubusercontent.com/u/2531208?s=60&v=4){kind=small}
There was a problem hiding this comment.
The PR is a clean, focused change that properly scopes the entity-prediction cache per tenant and wires in an invalidation handler on model-change events. The implementation is correct for the common case, but there is one substantive correctness concern: the : separator used in the cache key is also a valid character in tenant IDs, which can cause two distinct (tenantId, entityName) pairs to produce the same key and, more dangerously, cause invalidateTenant to evict entries belonging to a different tenant.
PR Bot Information
Version: 1.22.5
- Correlation ID:
ced6f49b-e5a9-4a93-be26-5be15de2108d - Event Trigger:
pull_request.opened - File Content Strategy: Full file content
- LLM:
anthropic--claude-4.6-sonnet
- Replace resourceGroupForTenant(String) with resourceGroup() on the public AICoreService interface. The implementation reads the tenant from the current RequestContext internally. - Remove isMultiTenancyEnabled() and getRetry() from the public interface; they remain accessible on AbstractAICoreService for internal callers. - Remove the CDS function 'resourceGroupForTenant' from index.cds and its action handler. - Detect multi-tenancy via standard CAP Java cds.multiTenancy.sidecar.url property and DeploymentService presence instead of custom flag. - Update RecommendationClientResolver to drop tenantId parameter. - Update samples, tests, and javadoc accordingly. Addresses review comments from PR #49 (Issue 2).
- Add tenant ownership verification on ResourceGroupHandler for READ (by-key), UPDATE, and DELETE operations. Returns 404 if the resource group belongs to a different tenant. - Scope list queries (READ without key) to the current tenant's resource groups via the tenant label filter in multi-tenancy mode. - Add ensureResourceGroupAccessible() guard to DeploymentHandler and ConfigurationHandler, validating the addressed resource group belongs to the current tenant before forwarding to AI Core. - Provider/system users are exempt from tenant restrictions and can access all resource groups (useful for ops/debug scenarios). - Add isProviderUser() and currentTenantId() as public helpers on AbstractAICoreService for use by handler classes. Addresses review comments from PR #49 (Issue 3a).
- Rename all configuration properties from cds.requires.AICore.* to cds.ai.core.* to align with CAP Java property naming conventions. - Rename cds.requires.recommendations.contextRowLimit to cds.ai.recommendations.contextRowLimit. - Drop the cds.requires.AICore.multiTenancy flag entirely; multi- tenancy is now auto-detected from standard CAP Java properties. - Update README with new configuration namespace and examples. Addresses review comments from PR #49 (Issue 3b).
When resourceGroupForTenant is called with a null tenantId (which happens when currentTenantId() returns null in single-tenant or non-tenant-scoped RequestContexts), fall back to the default resource group instead of passing null to the Caffeine cache (which throws NPE). This fixes integration test failures in the CI pipeline where the ApplicationServiceDelegation and Recommendation tests run without an explicit tenant in the RequestContext.
The cleanup step previously only deleted resource groups matching the
exact current run_id AND run_attempt. When a run failed and was re-run,
the previous attempt's resource groups were never cleaned up, eventually
hitting the AI Core resource group limit (50).
Changes:
- Match prefix 'itest-{run_id}-' (all attempts) instead of the exact
'itest-{run_id}-{run_attempt}' string.
- Same for 'sonar-{run_id}-' prefix.
- Also delete 'cds-itest-' prefixed resource groups which are created
by the multi-tenancy integration tests via resourceGroupForTenant()
and were never cleaned up by the pipeline.
The source code (commit c30080b) renamed properties from cds.requires.AICore.* to cds.ai.core.*, but the integration test application.yaml files were not updated. This meant the CDS_AICORE_TEST_RESOURCE_GROUP env var set by CI was silently ignored and tests always ran against the literal default resource group. - spring/application.yaml: cds.requires.AICore -> cds.ai.core - mtx-local/application.yaml: remove obsolete cds.requires.AICore.multiTenancy (now auto-detected from cds.multi-tenancy.sidecar.url)
Cover new code paths introduced by the tenant-scoping branch: - TenantScopingTest (7 tests): exercises every branch of AbstractCrudHandler.ensureResourceGroupAccessible() — provider bypass, single-tenancy bypass, null tenant, matching/non-matching labels, 404. - MockAICoreServiceImplTest (9 tests): both constructors, MT enabled/disabled, resourceGroupForTenant, cache isolation, clearTenantCache, getRetry, config property reads. - AICoreServiceImplDeploymentIdTest (+2 tests): resourceGroupForTenant(null) returns default even with MT enabled; single-tenancy always returns default.
Document the missing E2E coverage for RecommendationModelChangedHandler. The proper test requires an extensibility-enabled sidecar with extension JSON that adds prediction columns — not yet set up in mtx-local. The cache-invalidation logic itself is covered by the existing unit test FioriRecommendationHandlerTest.invalidateTenant_removesOnlyThatTenantsEntries.
Each parallel CI job (Java 17, Java 21, SonarQube) was using broad
prefixes in its cleanup step, deleting resource groups belonging to
sibling jobs still in progress. This caused intermittent 403 Forbidden
errors when the affected jobs tried to use their now-deleted resource
groups.
Narrow the cleanup prefixes so each job only deletes its own:
- integration-tests: itest-{run_id}-{attempt}-j{version}*
- scan-with-sonar: sonar-{run_id}-{attempt}*
Both still clean up itest-rg-* (ResourceGroupTest leftovers).
- DeploymentHandler: test onCreate (with/without TTL) and onUpdate happy path (targetStatus and configurationId branches) - ResourceGroupHandler: test onUpdate with/without labels, buildTenantLabelSelector branches (tenantId filter, MT non-provider, MT null tenant, single tenancy), ensureOwnedByCurrentTenant branches (provider, single tenant, wrong tenant, matching tenant) - AICoreServiceConfiguration: test eventHandlers() MockAICoreServiceImpl branch (with and without multi-tenancy), test detectMultiTenancy via services() for sidecarUrl branch and no-MT fallback
The MultiTenancyTest creates per-tenant resource groups with names like
cds-itest-mt-a-{timestamp} (from resourceGroupPrefix 'cds-' + tenant
name 'itest-*'). These are unique per test run (timestamped) and safe
to clean up from any job without cross-job interference.
Schmarvinius
force-pushed
the
final-review-tenant-scoping
branch
from
eb520f0 to
211d808
Compare
3 participants
Start to work on comments from #49, will collect changes wrt tenant scoping in this PR: