Optimize mimetype validation to only run when annotation exists#761
Optimize mimetype validation to only run when annotation exists#761Schmarvinius wants to merge 3 commits intomainfrom
Conversation
![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 a correctness issue: the new early-return optimisation relies on allowedTypesByElementName.isEmpty(), but MediaTypeResolver always populates the map (falling back to WILDCARD_MEDIA_TYPE for unannotated entities), so the guard can never trigger. The companion test masks this by fully mocking MediaTypeResolver to return Map.of(), giving false confidence that the optimisation works end-to-end. Both issues need to be addressed together — either by changing MediaTypeResolver to exclude unannotated entities from the result, or by using a different signal to detect the "no annotation present" case.
PR Bot Information
Version: 1.19.1 | 📖 Documentation | 🚨 Create Incident | 💬 Feedback
- Event Trigger:
pull_request.opened - Correlation ID:
d7e27720-25fe-11f1-89a4-6b40a77c1aec - LLM:
anthropic--claude-4.6-sonnet
...nts/handler/applicationservice/helper/mimeTypeValidation/AttachmentValidationHelperTest.java
Show resolved
Hide resolved
...chments/handler/applicationservice/helper/mimeTypeValidation/AttachmentValidationHelper.java
Show resolved
Hide resolved
MediaTypeResolver.fetchAcceptableMediaTypes() now returns Optional and only populates the map for entities that have the @Core.AcceptableMediaTypes annotation. This makes the isEmpty() early-return guard in AttachmentValidationHelper effective. Also adjusts findInvalidFilesByElementName to skip unconstrained elements instead of defaulting to wildcard.
There was a problem hiding this comment.
The overall quality of this PR is good — the optimization is logically sound and the test coverage is adequate. The one comment I posted about verifyNoInteractions() scope was incorrect (the call is already inside the try block); no real defects were identified. The only minor concern is that WILDCARD_MEDIA_TYPE becomes effectively dead code in the error-message fallback path after this refactor, but this is a minor maintainability note rather than a correctness issue.
PR Bot Information
Version: 1.19.1 | 📖 Documentation | 🚨 Create Incident | 💬 Feedback
- Correlation ID:
d967c220-25ff-11f1-93dd-2d62b4339b3e - LLM:
anthropic--claude-4.6-sonnet - Event Trigger:
issue_comment.created
...nts/handler/applicationservice/helper/mimeTypeValidation/AttachmentValidationHelperTest.java
Show resolved
Hide resolved
After the refactor, invalidFilesByElement can only contain keys that exist in acceptableMediaTypesByElementName, making the getOrDefault fallback to WILDCARD_MEDIA_TYPE unreachable. Replace with direct get() and remove the unused constant.
| (elementName, files) -> { | ||
| // Resolve the allowed media types for this field / element. | ||
| List<String> acceptableTypes = | ||
| acceptableMediaTypesByElementName.getOrDefault(elementName, WILDCARD_MEDIA_TYPE); |
There was a problem hiding this comment.
Is this functional behaviour check adjustment intended? From my PoV / is no longer possible to send is it?
There was a problem hiding this comment.
This is intentional. For the user there is no difference. Its just that elements without @Core.AcceptableMediaTypes are skipped for validation since there is no restriction, having the same effect as restricting them with a wildcard, just without the computation.
This is pure performance improvements (which we should do more often ^^)
2 participants
Optimize MIME Type Validation to Short-Circuit When No Annotation Exists
Refactor
♻️ Refactored the MIME type validation logic in
AttachmentValidationHelperto short-circuit early when no media entity has the@Core.AcceptableMediaTypesannotation, avoiding unnecessary data extraction and MIME resolution overhead.Changes
AttachmentValidationHelper.java: Moved theallowedTypesByElementNameresolution (viaMediaTypeResolver) to occur before the data extraction step. If the resulting map is empty (i.e., no entity has the annotation), the method returns immediately — skipping theAttachmentDataExtractorcall entirely. Also updatedfindInvalidFilesByElementNameto skip unconstrained elements by usingmap.get()instead ofgetOrDefault()with a wildcard fallback.MediaTypeResolver.java: ChangedfetchAcceptableMediaTypesto returnOptional<List<String>>instead ofList<String>. Entities without the@Core.AcceptableMediaTypesannotation are now excluded from the result map (viaifPresent), so the map accurately reflects only annotated entities.AttachmentValidationHelperTest.java: Added a new test casedoesNothing_whenNoEntityHasAcceptableMediaTypesAnnotationthat verifies the early-return behavior — whenMediaTypeResolverreturns an empty map,AttachmentDataExtractoris never invoked.MediaTypeResolverTest.java: Renamed and corrected the testshouldExcludeEntityWithoutAnnotation(previouslyshouldResolveMediaTypesUsingCascader) to assert that entities without the annotation are excluded from the result map rather than included with a wildcard fallback.📬 Subscribe to the Hyperspace PR Bot DL to get the latest announcements and pilot features!
PR Bot Information
Version:
1.19.1| 📖 Documentation | 🚨 Create Incident | 💬 Feedbackdbc62480-25ff-11f1-8e0c-572de37a2b5dpull_request.editedanthropic--claude-4.6-sonnet