refactor(ai-core): migrate AICoreService to RemoteService#73
Merged
Schmarvinius merged 26 commits intoJun 15, 2026
Conversation
- 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.
![hyperspace-insights[bot]](https://avatars.githubusercontent.com/u/2531208?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary
This PR migrates AICoreService from CqnService to RemoteService, removes the AICoreApplicationServiceHandler (which previously delegated application-service projections to the AICore remote service), adds a model-presence guard in services(), and renames DEFAULT_NAME from "AICore" to "AICore$Default".
Two issues were flagged:
-
getDefinition()returningnull— if the"AICore"CDS service definition is absent at runtime, the CAP framework will receivenullfromgetDefinition()and likely throw aNullPointerExceptiondownstream. A descriptive exception should be thrown instead. -
eventHandlers()missing the model-presence guard —services()now returns early when the AICore CDS model is absent, buteventHandlers()has no matching guard, leading to an inconsistent and fragile contract between the two lifecycle methods.
Additionally (not posted as a comment due to diff line constraints): the rename of DEFAULT_NAME from "AICore" to "AICore$Default" is a breaking public API change — any downstream code that looks up the service by the old name string will silently fail to find it. This warrants explicit migration guidance and a major version bump if the library follows semantic versioning.
PR Bot Information
Version: 1.22.5
- Event Trigger:
pull_request.opened - File Content Strategy: Full file content
- LLM:
anthropic--claude-4.6-sonnet - Correlation ID:
439f9b88-37e7-4cfc-9b67-aa7206c3b107
…ce-migration-v2 # Conflicts: # .github/actions/integration-tests/action.yml # .github/actions/scan-with-sonar/action.yml # cds-feature-ai-core/src/main/java/com/sap/cds/feature/aicore/core/AbstractAICoreService.java # cds-feature-ai-core/src/test/java/com/sap/cds/feature/aicore/core/AICoreServiceConfigurationTest.java # cds-feature-ai-core/src/test/java/com/sap/cds/feature/aicore/core/AICoreServiceImplDeploymentIdTest.java # cds-feature-ai-core/src/test/java/com/sap/cds/feature/aicore/core/MockAICoreServiceImplTest.java
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
2 participants
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.
Refactor: Migrate
AICoreServicetoRemoteServiceRefactor
♻️ Migrated the
AICoreServicecontract from extendingCqnServicetoRemoteService, aligning it with the correct CAP Java service type for remote integrations. This includes removing theAICoreApplicationServiceHandler(and related delegation logic), adding a CDS model guard in the configuration, and updating the default service name convention.Changes
AICoreService.java: Changed the parent interface fromCqnServicetoRemoteService. UpdatedDEFAULT_NAMEfrom"AICore"to"AICore$Default"to follow CAP naming conventions.AbstractAICoreService.java: AddedgetDefinition()override to resolve theAICoreCDS service definition from the runtime model, required by theRemoteServicecontract.AICoreServiceConfiguration.java: Added ahasAICoreModel()guard that skips service registration when theAICoreCDS model is not present in the runtime. Removed all registrations of the now-deletedAICoreApplicationServiceHandler.AICoreApplicationServiceHandler.java: Removed — the application service delegation handler is no longer needed.AICoreServiceConfigurationTest.java: Removed assertions that verifiedAICoreApplicationServiceHandlerregistration, reflecting its removal.ApplicationServiceDelegationTest.java(integration test): Removed — integration tests for the deleted delegation handler.test-service.cds: Removed theAICoreentity projections (configurations,deployments,resourceGroups) fromTestServiceand dropped theusing { AICore }import, cleaning up the test CDS model.PR Bot Information
Version:
1.22.5pull_request.openedanthropic--claude-4.6-sonnet439f9b88-37e7-4cfc-9b67-aa7206c3b107