feat: add @Validation.MaxItems and @Validation.MinItems support for compositions - Single opencode agent#754
feat: add @Validation.MaxItems and @Validation.MinItems support for compositions - Single opencode agent#754samyuktaprabhu wants to merge 2 commits intomainfrom
Conversation
…ompositions Implements item-count validation for composition properties annotated with @Validation.MaxItems and/or @Validation.MinItems. This allows applications to constrain the number of items in a composition (e.g., attachments). Key design decisions: - Handler fires on @before CREATE/UPDATE on ApplicationService - Draft detection via ThreadDataStorageReader (reuses existing pattern) - Draft activation (DRAFT_SAVE -> CREATE/UPDATE): warnings only - Direct active CREATE/UPDATE: errors (throwIfError) - Annotation values support integers and property references - Messages target the composition table for proper Fiori elements display - i18n keys: Validation_MaxItems, Validation_MinItems in messages.properties - Per-entity/property message override via ResourceBundle fallback pattern: Validation_MaxItems/<EntityName>/<PropertyName> Files changed: - New: ItemsCountValidationHandler.java (handler + annotation resolution) - New: ItemsCountValidationHandlerTest.java (comprehensive unit tests) - Modified: Registration.java (wire new handler) - Modified: messages.properties (add validation messages) - Modified: db-model.cds (test model with annotations) - Modified: data-model.cds (integration test model) - Modified: attachments.cds (sample bookshop demo) - Modified: RegistrationTest.java (updated handler count to 9) - Modified: AssociationCascaderTest.java (updated for new composition)
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 PR introduces a well-structured ItemsCountValidationHandler with solid test coverage. A few issues were flagged: (1) resolveMessageKey creates a new ResourceBundle on every call without caching, which causes unnecessary I/O per event; (2) messages.throwIfError() is called unconditionally on every active operation regardless of whether any annotated composition was present; and (3) unresolvable annotation values silently skip validation with no log output, making misconfiguration hard to diagnose. Please address these before merging.
PR Bot Information
Version: 1.18.5 | 📖 Documentation | 🚨 Create Incident | 💬 Feedback
- LLM:
anthropic--claude-4.6-sonnet - Correlation ID:
f5cc5ca0-1ede-11f1-8bbc-1d7039e73691 - Event Trigger:
pull_request.opened
| static String resolveMessageKey(String baseKey, String entityName, String propertyName) { | ||
| String specificKey = baseKey + "/" + entityName + "/" + propertyName; | ||
| try { | ||
| ResourceBundle bundle = ResourceBundle.getBundle("messages"); |
There was a problem hiding this comment.
Bug: resolveMessageKey loads a new ResourceBundle on every call, with no caching
ResourceBundle.getBundle("messages") is called for every composition element that has a constraint, on every CREATE/UPDATE event. ResourceBundle.getBundle involves class-loading, I/O and synchronisation overhead. In practice, the static bundle is re-loaded thousands of times per request in a busy application.
Consider caching the bundle (or the lookup result) in a static final field, or wrapping getBundle in a try-catch at class initialisation time and storing the result:
| ResourceBundle bundle = ResourceBundle.getBundle("messages"); | |
| static String resolveMessageKey(String baseKey, String entityName, String propertyName) { | |
| String specificKey = baseKey + "/" + entityName + "/" + propertyName; | |
| try { | |
| ResourceBundle bundle = ResourceBundle.getBundle("messages"); | |
| bundle.getString(specificKey); | |
| return specificKey; | |
| } catch (MissingResourceException e) { | |
| 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
| }); | ||
|
|
||
| if (!isDraftActivation) { | ||
| messages.throwIfError(); |
There was a problem hiding this comment.
Logic Error: throwIfError() is called unconditionally after all elements are processed, even when no compositions with annotations were present
When isDraftActivation is false, messages.throwIfError() is always called at line 157, regardless of whether any annotated compositions were found or whether any error was added. While harmless if throwIfError is a no-op with no errors, the intent per the Javadoc is that this is only meaningful after violations were detected. More importantly, if no compositions have constraints, the loop short-circuits at line 103, but throwIfError is still invoked unconditionally outside the loop. This is wasteful and may cause unintended side-effects depending on the Messages implementation.
Consider only calling throwIfError() when at least one error was actually emitted, or move the call inside the loop after the violation check block.
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 -1; |
There was a problem hiding this comment.
Logic Error: Silent fallback to -1 masks unresolvable annotation values
When resolveAnnotationValue cannot resolve the annotation value (unsupported type, non-existent property reference, non-numeric property value), it returns -1. The caller then skips validation with the >= 0 guard (if (maxItems >= 0 && ...) / if (minItems >= 0 && ...)). This means a misconfigured annotation like @Validation.MaxItems: 'nonExistentProp' silently produces no validation, offering no feedback to the developer. At minimum a logger.warn should be emitted when resolution fails to make misconfiguration diagnosable.
| return -1; | |
| logger.warn( | |
| "Could not resolve annotation value '{}' to an integer. Skipping validation.", | |
| annotationValue); | |
| return -1; | |
| } |
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
| Messages messages = context.getMessages(); | ||
|
|
||
| entity | ||
| .elements() |
There was a problem hiding this comment.
Bug: entity.elements() is consumed by the Stream.forEach inside a loop over data, but a Stream can only be iterated once
entity.elements() returns a Stream. At line 83 it is chained with .filter(...).forEach(element -> { for (CdsData d : data) { ... } }). The outer loop is over elements() (via forEach) and the inner loop is over data — this is correct and not a double-consumption issue here. However, if entity.elements() is ever called more than once (e.g. for re-validation), callers must be aware the stream is fresh each invocation. This is fine as long as CdsEntity.elements() returns a new stream each call, which is the typical CAP SDK contract but is worth confirming. No action required if confirmed; flag for awareness.
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
samyuktaprabhu
changed the title
1 participant
// single opencode agent w/ mcp server
Summary
@Validation.MaxItemsand@Validation.MinItemsannotation support for composition properties, allowing apps to constrain the number of items in a composition (e.g., attachments)ItemsCountValidationHandleron@Before CREATE/UPDATEforApplicationServicewith draft-aware behavior: warnings during draft activation, errors on direct active operationsDesign
Annotations
Draft Behavior
messages.throwIfError()— blocks the saveError Message Targeting
Messages target the composition property (
messages.error(...).target("in", b -> b.to(compositionName))) so Fiori elements highlights the table.i18n Override Pattern
Validation_MaxItems,Validation_MinItemsinmessages.propertiesValidation_MaxItems/<EntityName>/<PropertyName>in the app'smessages.properties— the handler checks viaResourceBundlefallbackAnnotation Value Resolution
@Validation.MaxItems: 20→ used directly@Validation.MaxItems: '20'→ parsed to int@Validation.MaxItems: 'stock'→ resolved from entity data at runtime-1(skips validation)Files Changed
ItemsCountValidationHandler.javaItemsCountValidationHandlerTest.javaRegistration.javamessages.propertiesValidation_MaxItemsandValidation_MinItemskeysdb-model.cds(unit test)limitedAttachmentswith annotationsdata-model.cds(integration test)itemsLimitedAttachmentswith annotationsattachments.cds(bookshop sample)RegistrationTest.javaAssociationCascaderTest.javaTesting
All 356 unit tests pass (0 failures, 0 errors).