Test Workshop: Add validation for attachment item counts and update messages#751
Test Workshop: Add validation for attachment item counts and update messages#751samyuktaprabhu wants to merge 2 commits intomainfrom
Conversation
SummaryThe following content is AI-generated and provides a summary of the pull request: Add Validation for Attachment Item Counts (
|
![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 new ItemsCountValidationHandler with solid test coverage, but there are two substantive correctness issues: a NullPointerException risk when annotation values are null, and an early-exit bug in reportViolations that silently drops all violations after the first one in non-draft mode. Additionally, the UPDATE path has a design flaw where existing data (flat attachment records) is index-matched against parent entity records, which will silently corrupt the merge used for property-reference annotation resolution.
PR Bot Information
Version: 1.18.5 | 📖 Documentation | 🚨 Create Incident | 💬 Feedback
- LLM:
anthropic--claude-4.6-sonnet - Event Trigger:
pull_request.opened - Correlation ID:
92ced100-1ed9-11f1-816d-eb32b61e4271
|
|
||
| private static Optional<Integer> resolveValue( | ||
| Object value, Map<String, Object> parentData, String annotationName) { | ||
| if ("true".equals(value.toString())) { |
There was a problem hiding this comment.
Bug: resolveValue throws NullPointerException when annotation value is null
CdsAnnotation::getValue can return null, and the chain .flatMap(value -> resolveValue(value, parentData, annotationName)) will pass null into resolveValue. The very first line value.toString() will throw an NPE in that case.
The test resolveAnnotationValueReturnsEmptyForNullAnnotationValue explicitly covers this scenario and would fail at runtime.
Consider guarding against null before calling resolveValue:
| if ("true".equals(value.toString())) { | |
| if (value == null || "true".equals(value.toString())) { | |
| // null or bare annotation without value, ignore | |
| return Optional.empty(); | |
| } |
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
| for (ItemsCountViolation violation : violations) { | ||
| String messageKey = violation.getBaseMessageKey(); | ||
| String target = violation.compositionName(); | ||
|
|
||
| if (isDraft) { | ||
| messages.warn(messageKey, violation.limit(), violation.actualCount()).target(target); | ||
| } else { | ||
| throw new ServiceException( | ||
| ErrorStatuses.CONFLICT, messageKey, violation.limit(), violation.actualCount()); | ||
| } | ||
| } |
There was a problem hiding this comment.
Logic Error: reportViolations stops reporting after the first non-draft violation; remaining violations in the list are silently dropped
When isDraft is false, the method throws a ServiceException on the first violation and exits the loop. Any subsequent violations in the list are never reported, so the user only ever sees one error at a time even when multiple constraints are breached.
Consider collecting all violations into the exception (e.g. as multiple messages or a combined message) rather than throwing inside the loop, or at a minimum report the first violation clearly in the exception details. For the draft path this is already handled correctly via the loop.
| for (ItemsCountViolation violation : violations) { | |
| String messageKey = violation.getBaseMessageKey(); | |
| String target = violation.compositionName(); | |
| if (isDraft) { | |
| messages.warn(messageKey, violation.limit(), violation.actualCount()).target(target); | |
| } else { | |
| throw new ServiceException( | |
| ErrorStatuses.CONFLICT, messageKey, violation.limit(), violation.actualCount()); | |
| } | |
| } | |
| static void reportViolations( | |
| List<ItemsCountViolation> violations, Messages messages, boolean isDraft) { | |
| if (isDraft) { | |
| for (ItemsCountViolation violation : violations) { | |
| String messageKey = violation.getBaseMessageKey(); | |
| String target = violation.compositionName(); | |
| messages.warn(messageKey, violation.limit(), violation.actualCount()).target(target); | |
| } | |
| } else { | |
| ItemsCountViolation first = violations.get(0); | |
| throw new ServiceException( | |
| ErrorStatuses.CONFLICT, | |
| first.getBaseMessageKey(), | |
| first.limit(), | |
| first.actualCount()); | |
| } | |
| } |
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
| CqnSelect select = CqnUtils.toSelect(context.getCqn(), target); | ||
| List<Attachments> existingData = | ||
| attachmentsReader.readAttachments(context.getModel(), target, select); | ||
|
|
||
| List<ItemsCountViolation> violations = ItemsCountValidator.validate(target, data, existingData); |
There was a problem hiding this comment.
Bug: validateOnUpdate reads existing data using attachmentsReader, which traverses the entity path to the Attachments media entity. However, the retrieved List<Attachments> is a flat list of attachment records, not a list of parent-entity records. When this is passed to ItemsCountValidator.validate, the validator iterates over data (parent records) and tries to match them with existingData at the same index — but existingData contains attachment rows, not parent rows. The size comparison and field lookup are therefore mismatched, meaning the merge of existing parent data with request data does not work as intended.
Consider reading existing parent-entity records (not attachment records) for the merge, or rethinking how existing data is supplied to the validator for the UPDATE path.
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
| for (int i = 0; i < dataList.size(); i++) { | ||
| CdsData data = dataList.get(i); | ||
| CdsData existingData = | ||
| (existingDataList != null && i < existingDataList.size()) | ||
| ? existingDataList.get(i) | ||
| : null; |
There was a problem hiding this comment.
Logic Error: Existing data is matched to request data by positional index, which is incorrect for UPDATE operations
The loop assumes existingDataList.get(i) corresponds to dataList.get(i), but an UPDATE payload can contain records in any order and the existing data from the database may be in a completely different order. A mis-match silently causes mergeData to mix values from unrelated records (e.g. the wrong parent's maxCount property reference is used) or fall back to null.
Records should be matched by their key rather than by position.
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
/review