Comments from #49: Document changes#67
Open
lisajulia wants to merge 14 commits into
Open
Conversation
SummaryThe following content is AI-generated and provides a summary of the pull request: Document How to Add
|
![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 focused documentation update that adds a useful manual-setup path for the SAP_Recommendations navigation property. The content itself is accurate and helpful, but there are a few formatting and structural issues — a missing space typo, a missing blank line before a heading, an improperly indented note that will render incorrectly in most Markdown engines, and a heading-level inconsistency that breaks the document hierarchy (### downgraded to #### without a parent ### to belong to).
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:
709cce1d-eb8b-4eb0-9d7c-251e27c1dbad
| - The `@cap-js/ai` CDS plugin must be installed (provides the model enhancement that adds `SAP_Recommendations` as a navigation property) | ||
| - The `SAP_Recommendations` navigation property must be added to the entities that should receive recommendations by | ||
| - either installing the `@cap-js/ai` CDS plugin (automatically provides the model enhancement that adds `SAP_Recommendations` as a navigation property) | ||
| - or adding the `SAP_Recommendations`property manually. |
There was a problem hiding this comment.
Typo: Missing space before property in the bullet point
SAP_Recommendationspropertyis missing a space between the backtick and the wordproperty`. This makes the sentence read awkwardly and the missing space may confuse readers skimming the text.
Consider adding the missing space.
Suggested change
| - or adding the `SAP_Recommendations`property manually. | |
| - or adding the `SAP_Recommendations` property manually. |
Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
| } | ||
| } | ||
| ``` | ||
| #### Adding the SAP_Recommendations navigation property manually |
There was a problem hiding this comment.
Formatting: Missing blank line before the new #### heading
The new #### Adding the SAP_Recommendations navigation property manually heading (line 74) immediately follows the closing fence of the JSON code block (line 73) with no blank line in between. In standard Markdown, headings should be preceded by a blank line to be reliably parsed as headings by all renderers and to improve readability.
Suggested change
| #### Adding the SAP_Recommendations navigation property manually | |
| #### Adding the SAP_Recommendations navigation property manually |
Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
| - The `SAP_Recommendations` navigation property must be added to the entities that should receive recommendations by | ||
| - either installing the `@cap-js/ai` CDS plugin (automatically provides the model enhancement that adds `SAP_Recommendations` as a navigation property) | ||
| - or adding the `SAP_Recommendations`property manually. | ||
| Without the `SAP_Recommendations` navigation property, the predictions will be computed but not serialized in OData responses. |
There was a problem hiding this comment.
Clarity / Logic: The "Without the SAP_Recommendations navigation property" sentence is separated from the bullet list it belongs to
The indentation on line 46 ( Without the…) is not aligned with the - or adding… sub-bullet (which is indented with two spaces), so it reads as a new paragraph in most renderers rather than a continuation of the list. Additionally, the sentence is important context that should be clearly visible — consider either aligning it as a proper continuation line or moving it outside the bullet list entirely as a standalone note.
Suggested change
| Without the `SAP_Recommendations` navigation property, the predictions will be computed but not serialized in OData responses. | |
| - or adding the `SAP_Recommendations` property manually. | |
| > **Note**: Without the `SAP_Recommendations` navigation property, the predictions will be computed but not serialized in OData responses. |
Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
| Without the `SAP_Recommendations` navigation property, the predictions will be computed but not serialized in OData responses. | ||
|
|
||
| ### CDS Plugin | ||
| #### CDS Plugin |
There was a problem hiding this comment.
Inconsistency: The "CDS Plugin" and "Adding the SAP_Recommendations…" sections are headed with #### (h4), but they appear directly under a ### Prerequisites section — they should be #### subsections of ### Prerequisites, not peers of it. However, looking at the broader document structure, the "CDS Plugin" section was previously a ### (h3) heading and was downgraded to #### in this PR. The new manual-property section also uses ####. This is correct only if both sub-sections belong under ### Prerequisites; however the content (installation instructions) goes well beyond listing prerequisites and really belongs under ## Setup as ### siblings. This structural mismatch may confuse readers navigating the README.
Consider restoring "CDS Plugin" and "Adding the SAP_Recommendations navigation property manually" as ###-level headings directly under ## Setup, alongside ### Maven and ### Prerequisites.
Suggested change
| #### CDS Plugin | |
| ### CDS Plugin |
Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
| } | ||
| } | ||
| ``` | ||
| #### Adding the SAP_Recommendations navigation property manually |
There was a problem hiding this comment.
Inconsistency: The heading level for the new manual section should match the restored ### level of "CDS Plugin" (see comment on line 48). If both are kept as ####, they are structural siblings of each other, which is fine, but the heading change at line 48 from ### to #### without a matching parent ### heading makes the document hierarchy incorrect.
Suggested change
| #### Adding the SAP_Recommendations navigation property manually | |
| ### Adding the SAP_Recommendations navigation property manually |
Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
Closed
14 tasks
lisajulia
force-pushed
the
final-review-doc-changes
branch
from
6c6660b to
b7ca166
Compare
* Make cache for entitiesWithoutPredictionsPerTenant tenant specific * refactor(ai-core): make AICoreService tenant-agnostic and DI-friendly - 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). * feat(ai-core): restrict AICore entity APIs to current tenant - 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). * chore(ai-core): rename config namespace to cds.ai.core - 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). * fix(ai-core): handle null tenant in resourceGroupForTenant 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. * fix(ci): cleanup all run attempts and cds-itest resource groups 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. * fix(itest): align config namespace with cds.ai.core rename 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) * test(ai-core): add unit tests for tenant scoping and mock service 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. * chore(recommendations): add TODO for model-changed integration test 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. * update cleanup * fix(ci): scope resource group cleanup to own job only 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). * test(ai-core): add unit tests for uncovered code paths - 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 * fix(ci): include cds-itest- prefix in resource group cleanup 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. * refactor(ai-core): migrate AICoreService from CqnService to RemoteService * refactor(ai-core): delete AICoreApplicationServiceHandler * fix(ai-core): guard service registration on AICore model presence * fix(test): mock CdsModel in unit tests and restore AICore model import * fix(ai-core): extend AbstractCdsDefinedService for proper RemoteService support * fix(recommendations): promote cds-services-impl to compile scope * refactor(test): use real CdsRuntime in AICoreServiceConfigurationTest * refactor(ai-core): remove AICORE_SERVICE_KEY env var check from binding detection * chore: exclude Mock* classes from SonarQube coverage * refactor(test): use real CdsRuntime in AICoreServiceImplDeploymentIdTest * fix: remove duplicate detectMultiTenancy method from merge --------- Co-authored-by: Lisa Julia Nebel <lisa.nebel@sap.com>
* Make cache for entitiesWithoutPredictionsPerTenant tenant specific * refactor(ai-core): make AICoreService tenant-agnostic and DI-friendly - 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). * feat(ai-core): restrict AICore entity APIs to current tenant - 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). * chore(ai-core): rename config namespace to cds.ai.core - 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). * fix(ai-core): handle null tenant in resourceGroupForTenant 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. * fix(ci): cleanup all run attempts and cds-itest resource groups 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. * fix(itest): align config namespace with cds.ai.core rename 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) * test(ai-core): add unit tests for tenant scoping and mock service 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. * chore(recommendations): add TODO for model-changed integration test 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. * update cleanup * fix(ci): scope resource group cleanup to own job only 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). * test(ai-core): add unit tests for uncovered code paths - 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 * fix(ci): include cds-itest- prefix in resource group cleanup 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. * refactor(ai-core): migrate AICoreService from CqnService to RemoteService * refactor(ai-core): delete AICoreApplicationServiceHandler * fix(ai-core): guard service registration on AICore model presence * fix(test): mock CdsModel in unit tests and restore AICore model import * fix(ai-core): extend AbstractCdsDefinedService for proper RemoteService support * fix(recommendations): promote cds-services-impl to compile scope * refactor(test): use real CdsRuntime in AICoreServiceConfigurationTest * refactor(ai-core): remove AICORE_SERVICE_KEY env var check from binding detection * chore: exclude Mock* classes from SonarQube coverage * refactor(test): use real CdsRuntime in AICoreServiceImplDeploymentIdTest * fix: remove duplicate detectMultiTenancy method from merge * refactor(ai-core): rename DEFAULT_NAME to AICoreService$Default 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. * refactor(ai-core): define EventContext subinterfaces for programmatic 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. * refactor(ai-core): make service API methods emit events; move logic to 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. * refactor(ai-core): decouple CRUD handlers from service impl; use typed 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. * refactor(ai-core): remove redundant event params from handler annotations * test(ai-core): rewrite handler tests to use real CdsRuntime 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 * refactor(ai-core): extract AICoreConfig and AICoreClients Immutable record for config values and holder for SDK API clients. * refactor(ai-core): extract DeploymentResolver Encapsulates caches, locks, retry, and validation behind resolveResourceGroup, resolveDeployment, invalidateTenant. * refactor(ai-core): inject components into handlers Handlers receive dependencies via constructor. Use context.getUserInfo() for tenant/provider checks directly. * refactor(ai-core): slim AICoreServiceImpl to pure delegation Zero fields, zero accessors. Delete AbstractAICoreService and MockAICoreServiceImpl. Add resourceGroupForTenant to interface. * refactor(ai-core): rewire configuration and setup handlers Configuration creates AICoreConfig, AICoreClients, DeploymentResolver and injects them into handlers at registration time. * refactor(recommendations): RptInferenceClient owns its retry Single-arg constructor, no dependency on service internals. Remove AbstractAICoreService casts from all consumers. * test(ai-core): update tests for new component architecture Adapt all unit and integration tests to use AICoreConfig, AICoreClients, DeploymentResolver instead of service accessors. * for pipeline * fix(ai-core): separate retry boundary to prevent orphaned deployments Only retry the deployment creation call. Polling is handled separately so a poll timeout does not re-create deployments. * refactor(ai-core): remove all service references from handlers Handlers use DeploymentResolver.resolveResourceGroup() directly instead of casting context.getService(). Zero service references. * fix(ai-core): add handler ordering and wire mock cleanup Add @HandlerOrder to setup handlers for DeploymentService events. Wire MockAICoreSetupHandler to actually call clearTenantCache(). Use ServiceException in mock inference handler. * fix(ai-core): validate config at startup, document impl coupling Fail fast on invalid cds.ai.core.* property values. Document AbstractCdsDefinedService dependency rationale. * test(ai-core): update tests for DeploymentResolver expansion Pass ResourceGroupApi to DeploymentResolver constructor. Pass DeploymentResolver to CRUD handler constructors. --------- Co-authored-by: Lisa Julia Nebel <lisa.nebel@sap.com>
* move handler to proper package * cleanup not relevant params in Before/After annotation * fix local test
* Separate the predictionRow from the contextRows and add a constant for [PREDICT] * Add comment on why we the @FunctionalInterface annotation is useful * Add comment to FioriRecommendationHandler * Moved everthing RPT-1 specific into RptInferenceClient * Rename RptInferenceClient.api -> RptInferenceClient.rpt and extract code that creates sdkRow to separate method * Moved everthing RPT-1 specific into RptInferenceClient * Replace MANAGED_FIELDS set with annotation-driven exclusion: @Core.Computed, @readonly and remove logic from 'toSdkRow' method that was already executed elsewhere * Change level of log statement when no suitable context columns are found and recommendations are therefore skipped * Move 'return early if predictRow == null' to earlier in afterRead of FioriRecommendationHandler * Extract 'SAP_Recommendations' into a constant * Get Persistence Service db via Dependency Injection * Move missingPredictionElementNames to earlier in the afterRead of the FioriRecommendationHandler * Add comment about conversion from List<Row> to List<CdsData> * Change check for active Entity: only return early if isActiveEntity is selected and false * Add comments to MockRecommendationClient * Change type of slectcolumns to Set * Add test: row for which we want to do predictions is automatically excluded in the select query returned by buildContextQuery * Add comment about ordering in the select query returned by buildContextQuery * Add comment computeSyntheticKey method and add test * Make sure we dont react on @cds.odata.valuelist : false and add a test for that * Simplify filters in computeContextColumn * Use any single key as RPT-1 index column, not just fields called ID and add test * Adjust sample to changes in cds-feature-recomendations * Add comment about books entity in test model * Update javadoc * Do not register FioriRecommendationHandler if no PersistenceService is found * In RptInferenceClient:resolveIndexColumn: fall back to synthetic index column for non-string single keys * Add checks for the presence of keyNames * Minor changes * Remove reflection tests for resolveIndexColumn The index column resolution is also tested by nonIdKey_usesSyntheticKeyColumn and composedKeys_usesSyntheticKeyColumn in FioriRecommendationHandlerTest. * Extract RptIndexColumns utility to share index column logic between RptInferenceClient and MockRecommendationClient * Add synthetic Key (if needed) in the sdkRow creation * Refactor: remove keyNames from RecommendationClient interface — it is now an argument of the Resolver via RecommendationClientResolver<T>
lisajulia
force-pushed
the
final-review-doc-changes
branch
from
70b0c9f to
bd359e7
Compare
* refactor(ai-core): migrate AICoreService to RemoteService - Remove AICoreService interface and AICoreServiceImpl - Introduce AICore constants class with SERVICE_NAME - Convert all handlers to use RemoteService with event contexts - Update DeploymentIdContext, InferenceClientContext, ResourceGroupContext - Update AICoreServiceConfiguration to register handlers on RemoteService - Adapt all unit tests to new RemoteService-based API - Update sample .cdsrc.json and ai-core-service.cds * chore(itests): adapt integration tests to RemoteService - Update all integration tests to use RemoteService + event contexts - Replace AICoreService.deploymentId/resourceGroup calls with context pattern - Update BaseIntegrationTest with new service resolution approach * refactor(recommendations): adapt to RemoteService API - Remove RptIndexColumns utility; inline resolveIndexColumn and addSyntheticKeyIfNeeded into RptInferenceClient - Change RecommendationClient.predict to accept keyNames as argument - Drop generic type parameter from RecommendationClientResolver; resolve now takes RemoteService directly - Remove keyNames from RptInferenceClient constructor (passed at predict time) - FioriRecommendationHandler now holds RemoteService reference and passes keyNames at prediction time - RecommendationConfiguration uses RemoteService + event context pattern (ResourceGroupContext, DeploymentIdContext, InferenceClientContext) - MockRecommendationClient simplified (no keyNames in constructor) - Update all tests to match new signatures * chore(samples): adapt bookshop sample to RemoteService - Replace AICoreService usage with RemoteService + event context pattern - Use AICore.SERVICE_NAME constant for service lookup - Demonstrate ResourceGroupContext, DeploymentIdContext, InferenceClientContext * update last Cqn references * update unit-tests * restore functionality * adapt tests * simplify itests * simplification * spotless * big blunder now fixed whoops * last occurences * spotless
lisajulia
force-pushed
the
final-review-doc-changes
branch
2 times, most recently
from
56ecb2e to
6bde138
Compare
* refactor(ai-core): migrate AICoreService to RemoteService - Remove AICoreService interface and AICoreServiceImpl - Introduce AICore constants class with SERVICE_NAME - Convert all handlers to use RemoteService with event contexts - Update DeploymentIdContext, InferenceClientContext, ResourceGroupContext - Update AICoreServiceConfiguration to register handlers on RemoteService - Adapt all unit tests to new RemoteService-based API - Update sample .cdsrc.json and ai-core-service.cds * chore(itests): adapt integration tests to RemoteService - Update all integration tests to use RemoteService + event contexts - Replace AICoreService.deploymentId/resourceGroup calls with context pattern - Update BaseIntegrationTest with new service resolution approach * refactor(recommendations): adapt to RemoteService API - Remove RptIndexColumns utility; inline resolveIndexColumn and addSyntheticKeyIfNeeded into RptInferenceClient - Change RecommendationClient.predict to accept keyNames as argument - Drop generic type parameter from RecommendationClientResolver; resolve now takes RemoteService directly - Remove keyNames from RptInferenceClient constructor (passed at predict time) - FioriRecommendationHandler now holds RemoteService reference and passes keyNames at prediction time - RecommendationConfiguration uses RemoteService + event context pattern (ResourceGroupContext, DeploymentIdContext, InferenceClientContext) - MockRecommendationClient simplified (no keyNames in constructor) - Update all tests to match new signatures * chore(samples): adapt bookshop sample to RemoteService - Replace AICoreService usage with RemoteService + event context pattern - Use AICore.SERVICE_NAME constant for service lookup - Demonstrate ResourceGroupContext, DeploymentIdContext, InferenceClientContext * update last Cqn references * update unit-tests * restore functionality * adapt tests * simplify itests * simplification * spotless * big blunder now fixed whoops * last occurences * add filter * readme update * test adaption * cleanup * update readme --------- Signed-off-by: Marvin L <marvin.lindner@sap.com>
lisajulia
force-pushed
the
final-review-doc-changes
branch
from
6bde138 to
11a9e2b
Compare
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.
Start to work on comments from #49, will collect all document changes in this PR: