feat: add @Validation.MaxItems and @Validation.MinItems for compositions#755
feat: add @Validation.MaxItems and @Validation.MinItems for compositions#755samyuktaprabhu wants to merge 1 commit intomainfrom
Conversation
Add validation support for limiting the number of items in composition
associations via @Validation.MaxItems and @Validation.MinItems CDS
annotations.
Key behaviors:
- Active instances (CREATE/UPDATE): produces ERRORs that reject the request
- Draft mode (DRAFT_PATCH/DRAFT_NEW): produces WARNINGs (drafts allow invalid state)
- Draft activation (DRAFT_SAVE): produces ERRORs (must be valid to save)
- Error messages correctly target the Fiori Elements table
- i18n override pattern: {baseKey}_{EntityName}_{compositionName}
New files:
- ItemCountValidationHandler (ApplicationService CREATE/UPDATE)
- DraftItemCountValidationHandler (DraftService DRAFT_SAVE/PATCH/NEW)
- Unit tests (55 test cases) and integration tests for both handlers
Currently supports integer literal values. Property references and
dynamic expressions are designed as extension points (TODOs).
SummaryThe following content is AI-generated and provides a summary of the pull request: Add
|
![hyperspace-insights[bot]](https://avatars.githubusercontent.com/u/2531208?s=60&v=4){kind=small}
There was a problem hiding this comment.
The pull request introduces @Validation.MaxItems / @Validation.MinItems annotation support with generally solid structure and good test coverage, but has several correctness issues that need to be addressed before merging: the existingData parameter in validateCompositionItemCounts is accepted and a DB read is performed in processBeforeUpdate to populate it, yet the parameter is completely ignored throughout the method body (wasted DB query, misleading API); the DraftItemCountValidationHandler.processBeforeDraftSave early-return on empty data silently bypasses MinItems checks for drafts with no items in their composition; and a test-specific message key (CompositionMaxItemsExceeded_RootTable_attachments) is bundled into the production messages.properties, which will corrupt messages for any consumer with an entity named RootTable.
PR Bot Information
Version: 1.18.5 | 📖 Documentation | 🚨 Create Incident | 💬 Feedback
- LLM:
anthropic--claude-4.6-sonnet - Event Trigger:
pull_request.opened - Correlation ID:
667864b0-1eeb-11f1-910f-7138f9663558
| public static void validateCompositionItemCounts( | ||
| CdsEntity entity, | ||
| List<? extends CdsData> requestData, | ||
| List<? extends CdsData> existingData, |
There was a problem hiding this comment.
Bug: existingData parameter is accepted but never used in validateCompositionItemCounts
The method signature accepts existingData (line 96) and the UPDATE handler passes it (line 80–81), but the entire method body ignores existingData completely — computeEffectiveItemCount only examines the request payload. This means the UPDATE path reads existing data from DB (incurring an unnecessary DB query) and then silently discards it. The Javadoc on processBeforeUpdate also says "Read existing data from DB to compute the effective count", but no merging ever happens.
Should either remove the existingData parameter (and the DB read in processBeforeUpdate) until the incremental-update TODO is implemented, or actually use it to compute the merged count.
| public static void validateCompositionItemCounts( | |
| CdsEntity entity, | |
| List<? extends CdsData> requestData, | |
| List<? extends CdsData> existingData, | |
| public static void validateCompositionItemCounts( | |
| CdsEntity entity, | |
| List<? extends CdsData> requestData, | |
| Messages messages, | |
| Severity severity) { |
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
| // Read existing data from DB to compute the effective count | ||
| CqnSelect select = CqnUtils.toSelect(context.getCqn(), context.getTarget()); | ||
| List<? extends CdsData> existingData = | ||
| attachmentsReader.readAttachments(context.getModel(), target, select); | ||
|
|
||
| validateCompositionItemCounts( | ||
| target, data, existingData, context.getMessages(), Severity.ERROR); | ||
| } |
There was a problem hiding this comment.
Bug: Unnecessary DB read in processBeforeUpdate — the result is never used
attachmentsReader.readAttachments(...) is called and its result is passed as existingData, but validateCompositionItemCounts ignores that parameter entirely. This triggers a real database query on every annotated UPDATE for no benefit.
Consider removing the DB read until incremental-update merging is actually implemented.
| // Read existing data from DB to compute the effective count | |
| CqnSelect select = CqnUtils.toSelect(context.getCqn(), context.getTarget()); | |
| List<? extends CdsData> existingData = | |
| attachmentsReader.readAttachments(context.getModel(), target, select); | |
| validateCompositionItemCounts( | |
| target, data, existingData, context.getMessages(), Severity.ERROR); | |
| } | |
| validateCompositionItemCounts( | |
| target, data, context.getMessages(), Severity.ERROR); |
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
| public static String resolveMessageKey( | ||
| String baseKey, String entitySimpleName, String compositionName) { | ||
| String specificKey = baseKey + "_" + entitySimpleName + "_" + compositionName; | ||
| try { | ||
| ResourceBundle bundle = ResourceBundle.getBundle("messages"); |
There was a problem hiding this comment.
Performance: ResourceBundle.getBundle("messages") is called on every single composition element validation
resolveMessageKey loads the ResourceBundle from the classpath on each invocation (once per composition per request row). ResourceBundle.getBundle has internal JVM caching, but loading and cache-lookup overhead inside a tight loop is avoidable. More importantly, this ties the library's validation logic to a hard-coded "messages" bundle name, which may conflict with the application's own bundle.
Consider caching the bundle (or its absence) in a static final field, and guarding with a null-check to avoid repeated MissingResourceException catching.
| public static String resolveMessageKey( | |
| String baseKey, String entitySimpleName, String compositionName) { | |
| String specificKey = baseKey + "_" + entitySimpleName + "_" + compositionName; | |
| try { | |
| ResourceBundle bundle = ResourceBundle.getBundle("messages"); | |
| private static final ResourceBundle MESSAGE_BUNDLE = loadBundle(); | |
| private static ResourceBundle loadBundle() { | |
| try { | |
| return ResourceBundle.getBundle("messages"); | |
| } catch (java.util.MissingResourceException e) { | |
| return null; | |
| } | |
| } | |
| public static String resolveMessageKey( | |
| String baseKey, String entitySimpleName, String compositionName) { | |
| String specificKey = baseKey + "_" + entitySimpleName + "_" + compositionName; | |
| if (MESSAGE_BUNDLE != null && MESSAGE_BUNDLE.containsKey(specificKey)) { | |
| return specificKey; | |
| } | |
| return baseKey; | |
| } |
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
| List<CdsData> draftData = readDraftDataWithCompositions(context, target); | ||
| if (draftData.isEmpty()) { | ||
| logger.debug("No draft data found for entity {}", target.getQualifiedName()); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Logic Error: processBeforeDraftSave skips validation when no draft data is found, but this is a silent no-op on a missing draft
If readDraftDataWithCompositions returns an empty list because no draft record matches the CQN (e.g., the entity was already deleted, or the CQN targets a non-existent ID), the handler silently returns without validating. A more robust approach would be to distinguish between "entity not found" (which is already an error state in the surrounding CAP framework) and "entity found but composition list is empty". The current guard swallows both cases — an empty composition list that should be validated against a MinItems constraint is never checked.
Consider removing the early return and letting validateCompositionItemCounts handle the empty-list case (it already skips validation when requestData is empty), or at least logging a warning before returning.
| List<CdsData> draftData = readDraftDataWithCompositions(context, target); | |
| if (draftData.isEmpty()) { | |
| logger.debug("No draft data found for entity {}", target.getQualifiedName()); | |
| return; | |
| } | |
| ItemCountValidationHandler.validateCompositionItemCounts( | |
| target, draftData, context.getMessages(), Severity.ERROR); |
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
1 participant
// multi agents + MCP server
Summary
@Validation.MaxItemsand@Validation.MinItemsannotation support on composition elements to validate item count constraintsDetails
Annotations
Error Messages & i18n
messages.properties:CompositionMaxItemsExceeded,CompositionMinItemsNotMet{baseKey}_{EntityName}_{compositionName}with fallback to{baseKey}.target(compositionName)Handlers
ItemCountValidationHandlerDraftItemCountValidationHandlerDraftItemCountValidationHandlerNew Files
ItemCountValidationHandler.java— validates on active entity CREATE/UPDATEDraftItemCountValidationHandler.java— validates on draft lifecycle events (reads draft data from persistence for DRAFT_SAVE)ItemCountValidationHandlerTest.java— 37 unit testsDraftItemCountValidationHandlerTest.java— 18 unit testsItemCountValidationNonDraftTest.java— integration tests for non-draft serviceItemCountValidationDraftTest.java— integration tests for draft serviceModified Files
Registration.java— registers both new handlersmessages.properties— adds error message keys with i18n override documentationdata-model.cds(integration tests) — addsMaxLimitedItementity and@Validation.MaxItems: 3annotationdb-model.cds(unit tests) — adds annotated compositions for test coverageAssociationCascaderTest.java,RegistrationTest.java— updated for new compositions/handlersFuture Work
@Validation.MinItems: stock)@Validation.MinItems: (stock > 20 ? 2 : 0))Test Results
mvn spotless:applyapplied