Idiomatic improvements: interface cleanup, handler decomposition, element constants#39
Idiomatic improvements: interface cleanup, handler decomposition, element constants#39Schmarvinius wants to merge 6 commits into
Conversation
- Replace string concatenation in ServiceException messages with SLF4J-style
{} placeholders in AICoreSetupHandler (3 locations)
- Replace Collectors.toList() with .toList() in AbstractCrudHandler
- Replace new ArrayList<>() with List.of() for null case
- Remove unused imports (ArrayList, Collectors)
Closes #32
…ler with record - Create AICoreElements class with typed element name constants for Deployment, ResourceGroup, Configuration, and Label entities - Replace 17-parameter buildDeploymentData method with DeploymentFields record - Replace two toMap() overloads with extractFields() + toCdsData() pattern - Update all handlers to use constants instead of hardcoded strings: DeploymentHandler, ConfigurationHandler, ResourceGroupHandler, ActionHandler Closes #33
Remove implementation-detail methods from AICoreService interface: - getDefaultResourceGroup() - getResourceGroupPrefix() - getTenantResourceGroupCache() - getResourceGroupDeploymentCache() - clearTenantCache() - resolveResourceGroupFromKeys() These methods remain public on AICoreServiceImpl and MockAICoreServiceImpl but are no longer part of the service contract. Only domain-level API (resourceGroupForTenant, deploymentId, inferenceClient, isMultiTenancyEnabled) and getRetry() (cross-module consumer) remain on the interface. Closes #34
Extract two helper classes from the 400-line handler: - RecommendationContextBuilder: entity analysis, prediction element discovery, context query building, synthetic key computation, and row assembly - RecommendationResultParser: prediction value type coercion, text path resolution from annotations, description DB lookups, and final recommendations map assembly The handler is now a slim orchestrator (~110 lines) that delegates complex logic to the helpers. Also replace unbounded ConcurrentHashMap.newKeySet() with a bounded Caffeine cache (max 10,000 entries) for the entity negative-result cache. Closes #35
Add overloaded constructor to AICoreServiceImpl that accepts DeploymentApi, ConfigurationApi, ResourceGroupApi, and AiCoreService as parameters. The existing convenience constructor delegates to it with default instances. This enables direct injection of mocked clients in unit tests without needing to mock getter methods on the service itself. Closes #36
Add Javadoc to RptInferenceClient clarifying that it is part of the public API for applications that need direct RPT-1 inference outside the automatic Fiori recommendation flow. Includes usage example. Closes #37
SummaryThe following content is AI-generated and provides a summary of the pull request: Idiomatic Improvements: Interface Cleanup, Handler Decomposition, and Element Constants♻️ Refactor: A series of idiomatic improvements aligned with Changes
GitHub Issues
PR Bot InformationVersion:
💌 Have ideas or want to contribute? Create an issue and share your thoughts with us! Made with ❤️ by Hyperspace. |
![hyperspace-pr-bot[bot]](https://avatars.githubusercontent.com/u/2531208?s=60&v=4){kind=small}
There was a problem hiding this comment.
The PR is a well-structured refactoring that successfully slims the interface, introduces typed constants, decomposes a large handler, and improves testability. The main concerns raised are: buildContextQuery() lacks a precondition guard making it unsafe to call in isolation; computeSyntheticKey has wider visibility than needed; the ServiceException vararg+cause overload resolution should be verified to ensure the stack trace is not silently swallowed; and the per-request RecommendationContextBuilder construction performs repeated CDS model traversals that could be expensive on hot read paths.
PR Bot Information
Version: 1.21.0 | 📖 Documentation | 🚨 Create Incident | 💬 Feedback
- Correlation ID:
42572f90-550a-4a5a-9f0c-03da16648fec - File Content Strategy: Full file content
- LLM:
anthropic--claude-4.6-sonnet - Event Trigger:
pull_request.opened
| return CdsData.create(predictRow); | ||
| } | ||
|
|
||
| String computeSyntheticKey(Map<String, Object> row) { |
There was a problem hiding this comment.
Bug: buildContextQuery() is called before checking predictionElementNames.isEmpty(), but if predictionElementNames is empty the CQL.withAnd() collector will produce an empty conjunction. More critically, this method is only safe to call when predictionElementNames is non-empty; however the guard that checks predictionElementNames().isEmpty() in the handler fires after the builder is already constructed, so the issue is in ordering, not in construction. The real problem is that computeSyntheticKey has package-private visibility but is only an internal implementation detail of assembleRows. Exposing it as package-visible (no access modifier) means test code or sibling classes can call it independently, creating unnecessary coupling.
Consider making computeSyntheticKey private since it is only called from assembleRows.
| String computeSyntheticKey(Map<String, Object> row) { | |
| private String computeSyntheticKey(Map<String, Object> row) { |
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
| return syntheticKeyNeeded; | ||
| } | ||
|
|
||
| CqnSelect buildContextQuery() { |
There was a problem hiding this comment.
Bug: buildContextQuery() calls predictionElementNames.stream().map(...).collect(CQL.withAnd()) without guarding against an empty list. If predictionElementNames is empty (which is checked by the caller after construction), this will either throw or produce a degenerate query. The check in FioriRecommendationHandler (builder.predictionElementNames().isEmpty()) correctly bails out before calling buildContextQuery(), but the method itself offers no internal protection and its contract is undocumented. At minimum, an assertion or an IllegalStateException guard should be added to make the precondition explicit and avoid silent misuse.
| CqnSelect buildContextQuery() { | |
| CqnSelect buildContextQuery() { | |
| if (predictionElementNames.isEmpty()) { | |
| throw new IllegalStateException("buildContextQuery() called with no prediction elements"); | |
| } | |
| List<String> selectColumns = new ArrayList<>(contextColumns); |
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
| throw new ServiceException( | ||
| ErrorStatuses.SERVER_ERROR, | ||
| "Failed to create AI Core resources for tenant: " + tenantId, | ||
| "Failed to create AI Core resources for tenant: {}", | ||
| tenantId, | ||
| e); |
There was a problem hiding this comment.
Bug: The ServiceException constructor is being called with the signature (ErrorStatus, String messageTemplate, Object... args) where the last argument is both the tenantId vararg and the Throwable cause. Depending on how the ServiceException constructor is overloaded, the exception e may be treated as a plain message argument (i.e., e.toString() interpolated into {}), not as the cause — meaning the original stack trace would be lost in the logged/serialized error.
Verify that ServiceException has an overload that unambiguously accepts a trailing Throwable cause separately from the vararg message parameters. If not, pass the cause explicitly using the dedicated two-argument form or cast to Throwable to resolve the overload correctly.
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
| .or(() -> target.keyElements().map(CdsElement::getName).findFirst()) | ||
| .ifPresent(col -> select.orderBy(CQL.get(col).desc())); | ||
|
|
||
| var builder = new RecommendationContextBuilder(target, rowType, limit); |
There was a problem hiding this comment.
Performance: A new RecommendationContextBuilder is instantiated on every afterRead invocation for every entity, even for entities that have already been determined to have no prediction elements (cached in entitiesWithoutPredictions). However, the cache check happens before builder construction, so that fast path is fine. The issue is that for entities that do have prediction elements, computePredictionElements() and computeContextColumns() stream over the CDS model on every single read event for every row. Since CdsStructuredType is immutable, these results could be cached per entity name alongside (or instead of) the negative entitiesWithoutPredictions cache, avoiding repeated model traversals on hot paths.
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
|
Restructuring into individual stacked PRs per issue. |
1 participant
Summary
A series of idiomatic improvements identified by comparing this codebase against
cds-servicesandcds4jconventions.Changes
1. Fix non-idiomatic patterns (#32)
ServiceExceptionmessages with{}placeholdersCollectors.toList()with.toList(),new ArrayList<>()withList.of()2. Introduce element name constants; refactor DeploymentHandler (#33)
AICoreElementsclass with typed constants for all entity elementsbuildDeploymentDatamethod with typedDeploymentFieldsrecord3. Slim AICoreService interface to public API surface (#34)
getRetry()(cross-module consumer)4. Decompose FioriRecommendationHandler (#35)
RecommendationContextBuilder(entity analysis, query building, row assembly)RecommendationResultParser(type coercion, text paths, description lookup)ConcurrentHashMap.newKeySet()with bounded Caffeine cache5. Accept SDK API clients as constructor parameters (#36)
AICoreServiceImplfor testability6. Document RptInferenceClient as public API (#37)
Test Results
All 41 unit tests pass across both modules (26 in ai-core, 15 in recommendations).
Closes
Closes #32, closes #33, closes #34, closes #35, closes #36, closes #37