refactor(ai-core): typesafe handlers decoupled from service impl#74
Merged
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.
…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
Follow standard CAP Java naming convention for service instances (ServiceInterface$Default). The CDS definition name stays 'AICore' (matching the CDS model); only the registered instance name changes.
… API Add typed EventContext interfaces in the api package for the three programmatic API methods: - DeploymentIdContext: for deploymentId(resourceGroupId, spec) - InferenceClientContext: for inferenceClient(resourceGroupId, deploymentId) - ResourceGroupContext: for resourceGroup() These enable the idiomatic CAP pattern where service methods emit events and ON handlers provide the implementation, allowing extensibility via @Before/@after hooks.
…o handler Apply idiomatic CAP Java pattern: service methods create typed EventContext, emit(), and return result. A separately-registered AICoreApiHandler provides the ON implementation with the actual business logic. - AICoreServiceImpl.deploymentId/inferenceClient/resourceGroup/ resourceGroupForTenant now emit typed contexts instead of doing work directly - New AICoreApiHandler handles DeploymentIdContext, InferenceClientContext, ResourceGroupContext with all caching, retry, and SDK logic - ResourceGroupContext extended with optional tenantId for explicit-tenant path - AICoreServiceConfiguration registers AICoreApiHandler - AICoreServiceImpl retains shared state (caches, config, APIs) accessed by handlers via EventContext.getService() This enables extensibility: apps can register @Before/@after handlers on deploymentId, inferenceClient, and resourceGroup events.
…d contexts - Remove AICoreServiceImpl field from AbstractCrudHandler; handlers now obtain the service from EventContext.getService() at invocation time. - Pass SDK API clients (DeploymentApi, ResourceGroupApi, ConfigurationApi) directly via constructor injection — stateless clients don't need the service reference. - ActionHandler uses generated DeploymentsStopContext instead of raw EventContext with string-based key extraction. - All handlers use generated entity name constants (Deployments_.CDS_NAME, ResourceGroups_.CDS_NAME, Configurations_.CDS_NAME) instead of hand-written strings. - Update AICoreServiceConfiguration to pass API clients to handler constructors. - Update all handler unit tests for the new constructor signatures and EventContext-based service access pattern. Addresses issue #70: typesafe handlers decoupled from service impl.
SummaryThe following content is AI-generated and provides a summary of the pull request: Refactor AICore Service to Idiomatic CAP Java Event-Driven Architecture♻️ Refactor: Applies idiomatic CAP Java patterns to the AICore service architecture by introducing typed ChangesThe core change separates business logic from the service implementation:
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.
Summary
The refactor successfully applies CAP Java's event-driven patterns — thin emitters, typed EventContext classes, and decoupled handlers receiving API clients via constructor injection. The structural improvement is clear and the generated-constant migration (Deployments_.CDS_NAME etc.) is a solid correctness improvement.
Three substantive issues were identified and commented:
- Null results after
emit()—deploymentId(),inferenceClient(), andresourceGroup()returnctx.getResult()without guarding againstnull, which silently propagates to callers if no handler is registered. - Unchecked
AICoreServiceImplcast inAICoreApiHandler—(AICoreServiceImpl) context.getService()will throwClassCastExceptionfor any non-AICoreServiceImplservice instance with no descriptive error. Retryinstance recreated perpollUntilRunningcall — a newRetry.of("pollDeployment", …)is constructed on every invocation, which conflicts with anyRetryRegistryusage and loses observability.
Please address these before merging.
PR Bot Information
Version: 1.22.8
- Correlation ID:
c0bca913-e7cb-4743-acc4-e8cf0b65b902 - File Content Strategy: Full file content
- LLM:
anthropic--claude-4.6-sonnet - Event Trigger:
pull_request.opened
Replace heavily-mocked unit tests with integration-style tests that boot a real CdsRuntime, register real handlers, and dispatch CQN through the full handler pipeline. Only SDK API clients remain mocked. - DeploymentHandlerTest: tests CREATE, UPDATE via service.run() - ConfigurationHandlerTest: tests READ, CREATE via service.run() - ResourceGroupHandlerTest: tests CRUD + MT label filtering - TenantScopingTest: tests tenant isolation through actual CQN operations with different RequestContext tenants
Schmarvinius
changed the title
Immutable record for config values and holder for SDK API clients.
Encapsulates caches, locks, retry, and validation behind resolveResourceGroup, resolveDeployment, invalidateTenant.
Handlers receive dependencies via constructor. Use context.getUserInfo() for tenant/provider checks directly.
Zero fields, zero accessors. Delete AbstractAICoreService and MockAICoreServiceImpl. Add resourceGroupForTenant to interface.
Configuration creates AICoreConfig, AICoreClients, DeploymentResolver and injects them into handlers at registration time.
Single-arg constructor, no dependency on service internals. Remove AbstractAICoreService casts from all consumers.
Adapt all unit and integration tests to use AICoreConfig, AICoreClients, DeploymentResolver instead of service accessors.
Only retry the deployment creation call. Polling is handled separately so a poll timeout does not re-create deployments.
Handlers use DeploymentResolver.resolveResourceGroup() directly instead of casting context.getService(). Zero service references.
Add @HandlerOrder to setup handlers for DeploymentService events. Wire MockAICoreSetupHandler to actually call clearTenantCache(). Use ServiceException in mock inference handler.
Fail fast on invalid cds.ai.core.* property values. Document AbstractCdsDefinedService dependency rationale.
Pass ResourceGroupApi to DeploymentResolver constructor. Pass DeploymentResolver to CRUD handler constructors.
Base automatically changed from
refactor/remote-service-migration-v2
to
review-fixes
June 15, 2026 13:09
…safe-handlers # Conflicts: # .github/workflows/pr.yml # cds-feature-ai-core/src/main/java/com/sap/cds/feature/aicore/api/AICoreService.java # cds-feature-ai-core/src/main/java/com/sap/cds/feature/aicore/core/AICoreServiceConfiguration.java # 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.
Applies idiomatic CAP Java patterns to the AICore service architecture:
EventContextevents; handlers provide the implementationAICoreServiceImpl(no stored reference)Changes
Service emits, handler implements:
AICoreServiceImpl.deploymentId(),.inferenceClient(),.resourceGroup()now create a typedEventContext, callemit(), and return the resultAICoreApiHandler(@on handler) contains all business logic: caching, retry, SDK calls, deployment polling@Before/@AfterondeploymentId,inferenceClient,resourceGroupeventsHandler decoupling:
AICoreServiceImplfield fromAbstractCrudHandlerEventContext.getService()Typed contexts & compile-safe constants:
DeploymentIdContext,InferenceClientContext,ResourceGroupContextinapipackageActionHandleruses generatedDeploymentsStopContextinstead of rawEventContextDeployments_.CDS_NAME,ResourceGroups_.CDS_NAME,Configurations_.CDS_NAMENaming:
DEFAULT_NAMErenamed from"AICore$Default"to"AICoreService$Default"(standard CAP convention)