Final review#60
Conversation
SummaryThe following content is AI-generated and provides a summary of the pull request: Make Prediction Cache Tenant-Specific and Document Manual SAP_Recommendations SetupBug Fix / Documentation🐛 Fixed the per-entity prediction cache to be tenant-specific, and 📝 documented how to manually add the 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 generally well-structured: the cache is correctly made tenant-aware, the RecommendationModelChangedHandler cleanly wires in cache invalidation on model changes, and the new documentation section for manual SAP_Recommendations addition is a useful addition. A few issues were flagged: a typo in the README, a clarity/formatting problem with the misplaced warning note, and a potential composite cache-key collision if tenant IDs happen to contain the colon separator character.
PR Bot Information
Version: 1.22.4
- LLM:
anthropic--claude-4.6-sonnet - Event Trigger:
pull_request.opened - File Content Strategy: Full file content
- Correlation ID:
af542b3f-56b7-45fb-b507-c6e9482682f7
| void invalidateTenant(String tenantId) { | ||
| String prefix = tenantKey(tenantId) + ":"; |
There was a problem hiding this comment.
Bug: invalidateTenant prefix collision — a tenant whose ID is a prefix of another tenant's ID will incorrectly evict the other tenant's cache entries.
For example, if two tenants are named "t" and "t2", calling invalidateTenant("t") builds the prefix "t:". The key for tenant "t2" is "t2:<entity>" which does not start with "t:", so this particular pair is actually safe. However, consider tenants "tenant" and "tenant-extended": after invalidateTenant("tenant"), the prefix is "tenant:" and any key beginning with "tenant:" will be removed — that is correct. But consider two tenants "a" and "ab": prefix "a:" will only match "a:…", not "ab:…", because of the colon separator, so the colon already prevents simple substring collisions.
The real remaining risk is that a malicious or misconfigured tenant ID that itself contains ":" (e.g. "t:en") would produce tenantKey("t:en") + ":" = "t:en:", which could match cache keys for tenant "t" whose entity name starts with "en:…". While that is an edge case, the separator character used for the composite key should be one that cannot appear in either component, or the components should be length-prefixed / otherwise escaped.
Consider using a separator that cannot appear in tenant IDs and entity names, or use a Map<String, Map<String, Boolean>> (tenant → entity-name → flag) to avoid any composite-key ambiguity entirely.
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 `@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_Recommendationspropertyshould beSAP_Recommendations property (space before "property").
| - 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
| - 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: The warning about missing SAP_Recommendations navigation property is misplaced. It is indented at the same level as the sub-bullets, but it applies to the overall prerequisite and should be visually separated from the list items so readers don't mistake it for a third option.
Consider placing it as a standalone note below the two sub-bullets, or dedenting it to the parent bullet level, e.g.:
- 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.
> **Note:** Without the `SAP_Recommendations` navigation property, the predictions will be computed but not serialized in OData responses.
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
1 participant
Start to work on comments from #49: