TEST: feat: add @Validation.MinItems and @Validation.MaxItems for attachment compositions#752
TEST: feat: add @Validation.MinItems and @Validation.MaxItems for attachment compositions#752
Conversation
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 feature for @Validation.MinItems/@Validation.MaxItems support with clear separation between draft and active-entity handling. However, there are several substantive correctness issues: the count aggregation in ItemCountValidationHelper sums items across all records in the batch rather than validating each record independently, the isDraft detection in ItemsCountValidationHandler uses anyMatch which causes active records in a mixed batch to be validated as draft (warnings instead of errors), and the raw (Number) cast can throw ClassCastException for non-numeric annotation values. These should be addressed before merging.
PR Bot Information
Version: 1.18.5 | 📖 Documentation | 🚨 Create Incident | 💬 Feedback
- Correlation ID:
a9eab0b0-1eda-11f1-8aa6-fcb090456ed9 - LLM:
anthropic--claude-4.6-sonnet - Event Trigger:
pull_request.opened
| boolean presentInPayload = data.stream().anyMatch(d -> d.containsKey(compositionName)); | ||
| if (!presentInPayload) { | ||
| return; | ||
| } | ||
|
|
||
| long count = data.stream() | ||
| .filter(d -> d.containsKey(compositionName)) | ||
| .mapToLong(d -> { | ||
| Object val = d.get(compositionName); | ||
| return val instanceof List<?> list ? list.size() : 0L; | ||
| }) | ||
| .sum(); |
There was a problem hiding this comment.
Logic Error: Count aggregation sums items across all records, not per-record, making per-entity validation incorrect
count is computed as the sum of items from all records in the data list. For example, if there are 3 records in the batch each with 4 attachments, count will be 12 even though each individual entity only has 4. The validation should check the constraint per data row, not across the entire payload.
Consider iterating over each item in data individually and validating its composition list count independently:
data.stream()
.filter(d -> d.containsKey(compositionName))
.forEach(d -> {
Object val = d.get(compositionName);
long count = val instanceof List<?> list ? list.size() : 0L;
composition.<Object>findAnnotation("Validation.MinItems").ifPresent(annotation -> {
Object value = annotation.getValue();
if (value instanceof Boolean) return;
int limit = ((Number) value).intValue();
if (count < limit) {
String msgKey = MIN_ITEMS_KEY + "_" + entity.getName() + "_" + compositionName;
issueMessage(isDraft, messages, msgKey, limit, entity.getQualifiedName(), compositionName);
}
});
composition.<Object>findAnnotation("Validation.MaxItems").ifPresent(annotation -> {
Object value = annotation.getValue();
if (value instanceof Boolean) return;
int limit = ((Number) value).intValue();
if (count > limit) {
String msgKey = MAX_ITEMS_KEY + "_" + entity.getName() + "_" + compositionName;
issueMessage(isDraft, messages, msgKey, limit, entity.getQualifiedName(), compositionName);
}
});
});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
| composition.<Object>findAnnotation("Validation.MinItems").ifPresent(annotation -> { | ||
| Object value = annotation.getValue(); | ||
| if (value instanceof Boolean) return; // bare annotation without value → skip | ||
| int limit = ((Number) value).intValue(); |
There was a problem hiding this comment.
Bug: ClassCastException possible when annotation value is not a Number
The cast ((Number) value).intValue() assumes the annotation value is always a Number. However, CDS annotations could carry a String (e.g. a property path for a dynamic reference). This will throw a ClassCastException at runtime for any non-numeric value that isn't Boolean.
Consider adding a type guard before the cast:
if (!(value instanceof Number)) return; // non-numeric value (e.g. path reference) → skip
int limit = ((Number) value).intValue();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
| private boolean isDraftData(List<CdsData> data) { | ||
| return data.stream().anyMatch(d -> Boolean.FALSE.equals(d.get("IsActiveEntity"))); |
There was a problem hiding this comment.
Logic Error: isDraft detection uses anyMatch, so a mixed batch of draft and active records is treated as all-draft
If a data list contains both draft records (IsActiveEntity=false) and active records (IsActiveEntity=true or absent), anyMatch returns true and all records in the batch are validated with isDraft=true (warnings only). Active records in the same batch would silently miss the required error.
The draft/active distinction should be evaluated per record, not once for the whole list. This is closely related to the per-record aggregation fix in ItemCountValidationHelper; passing isDraft per row into the helper (or moving the isDraft check inside the helper loop) would resolve both issues.
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
| int limit = ((Number) value).intValue(); | ||
| if (count < limit) { | ||
| String msgKey = MIN_ITEMS_KEY + "_" + entity.getName() + "_" + compositionName; | ||
| issueMessage(isDraft, messages, msgKey, limit, entity.getQualifiedName(), compositionName); |
There was a problem hiding this comment.
Bug: The same violation message key is issued once per data entry that contains the composition, but the deduplication relies solely on the message key string. If a batch contains multiple root records that each violate the constraint, only one error/warning may be surfaced (depending on framework message de-duplication). The target lambda e -> e.to(compositionName) does not include the record's key, so consumers cannot identify which record triggered the violation.
Consider including a record identifier in the target expression so each message is uniquely targeted to its offending row.
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
…ttachment compositions
Introduces item-count validation for attachment compositions annotated with
@Validation.MinItems and @Validation.MaxItems. In draft mode violations
produce warnings (draft allows an invalid state); on active entities and
during draft activation (DRAFT_SAVE) they produce errors.
- ItemCountValidationHelper: shared utility that reads annotations, counts
items in the payload, and issues warn/error messages targeting the
composition table so Fiori elements can highlight it. Supports integer
values, and the i18n override pattern
(attachment_minItems_{Entity}_{property}) for per-property message
customisation.
- ItemsCountValidationHandler: @before LATE on ApplicationService CREATE
and UPDATE; detects draft state via IsActiveEntity=false in payload.
- DraftSaveItemsCountValidationHandler: @before LATE on DraftService
DRAFT_SAVE; always errors since activation must enforce constraints.
- Registration: registers both new handlers alongside the existing ones.
- i18n: adds attachment_minItems and attachment_maxItems message keys.
lisajulia
force-pushed
the
feat/validation-min-max-items
branch
from
99c05b6 to
9680ab6
Compare
1 participant
Summary
@Validation.MinItems/@Validation.MaxItemsannotation support on attachment compositionsattachment_minItems_{EntityName}_{compositionName}in their own bundle to customise the message per propertyNew files
ItemCountValidationHelperItemsCountValidationHandler@Before LATEonApplicationServiceCREATE + UPDATE; warns on draft (IsActiveEntity=false), errors on activeDraftSaveItemsCountValidationHandler@Before LATEonDraftServiceDRAFT_SAVE; always errorsTest plan
ItemsCountValidationHandlerTest— 13 unit tests (active errors, draft warnings, no-violation, absent composition)DraftSaveItemsCountValidationHandlerTest— 7 unit tests (always errors, never warns)RegistrationTest— updated handler count 8 → 10mvn test -pl cds-feature-attachmentspasses (345 tests, 0 failures)