-
Notifications
You must be signed in to change notification settings - Fork 6
Validation Maximum Annotation #711
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Summary
This pull request implements validation for the @Validation.Maximum annotation on attachment file sizes. While the feature addition is valuable, there are critical bugs that must be addressed before merging:
Critical Issues Found:
-
Incorrect size measurement: Using
InputStream.available()to check file size is fundamentally flawed—it returns an estimate, not the actual size, which will cause false positives/negatives in validation. -
Type mismatches: The method
convertValMaxToIntreturnslongbut is named as if it returnsint, creating confusion throughout the codebase. -
Poor error handling: IOExceptions are wrapped in generic RuntimeExceptions, losing important context for debugging and error recovery.
-
Performance inefficiency: The
condenseAttachmentsmethod is called twice with the same data, wasting processing time. -
Logic errors: Debug logging is placed after validation, making it useless; potential null pointer if annotation lookup fails.
Minor Issues:
- Typo in error message ("Unkown" → "Unknown")
- Public visibility on internal filter constant
- User-unfriendly error messages (raw byte counts instead of MB/GB)
- Missing exception context for ArithmeticException
Recommendation: The core file size validation logic needs to be redesigned to properly read and measure stream content before this can be safely merged.
PR Bot Information
Version: 1.17.5
- LLM:
anthropic--claude-4.5-sonnet - Event Trigger:
pull_request.opened - Correlation ID:
66718bf0-ef8d-11f0-8d85-cac426081643
...ava/com/sap/cds/feature/attachments/handler/applicationservice/UpdateAttachmentsHandler.java
Outdated
Show resolved
Hide resolved
...ava/com/sap/cds/feature/attachments/handler/applicationservice/UpdateAttachmentsHandler.java
Outdated
Show resolved
Hide resolved
...ava/com/sap/cds/feature/attachments/handler/applicationservice/UpdateAttachmentsHandler.java
Outdated
Show resolved
Hide resolved
...ava/com/sap/cds/feature/attachments/handler/applicationservice/UpdateAttachmentsHandler.java
Outdated
Show resolved
Hide resolved
...ava/com/sap/cds/feature/attachments/handler/applicationservice/UpdateAttachmentsHandler.java
Outdated
Show resolved
Hide resolved
...ava/com/sap/cds/feature/attachments/handler/applicationservice/UpdateAttachmentsHandler.java
Outdated
Show resolved
Hide resolved
...ava/com/sap/cds/feature/attachments/handler/applicationservice/UpdateAttachmentsHandler.java
Outdated
Show resolved
Hide resolved
...in/java/com/sap/cds/feature/attachments/handler/applicationservice/helper/FileSizeUtils.java
Outdated
Show resolved
Hide resolved
...in/java/com/sap/cds/feature/attachments/handler/applicationservice/helper/FileSizeUtils.java
Outdated
Show resolved
Hide resolved
...in/java/com/sap/cds/feature/attachments/handler/applicationservice/helper/FileSizeUtils.java
Outdated
Show resolved
Hide resolved
Co-authored-by: hyperspace-insights[bot] <209611008+hyperspace-insights[bot]@users.noreply.github.com>
…ies for size limits
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Summary
I've identified several substantive issues in this pull request:
-
Critical Logic Error in Exception Handling: The
ModifyApplicationHandlerHelpercatches its own thrownRuntimeException, which causes incorrect error handling. OtherRuntimeExceptionsubtypes likeIllegalArgumentExceptionfrom parsing will be misclassified as size validation errors instead of bad request errors. -
Critical Logic Error in ExtendedErrorStatuses: The
getByCode()method iterates over the wrong enum (ErrorStatusesinstead ofExtendedErrorStatuses) and compares the wrong field (HTTP status vs. code), making the method non-functional. -
Performance/Maintainability Issue: The
UpdateAttachmentsHandlercontains duplicate code for key extraction that could use the newly addedApplicationHandlerHelper.extractKeys()method instead. -
Minor Style Issues: Missing trailing newlines in two files (data-model.cds and messages.properties), though these don't affect functionality.
The most critical issues are the exception handling logic and the broken getByCode() method, which should be addressed before merging.
PR Bot Information
Version: 1.17.15 | 📖 Documentation | 🚨 Create Incident | 💬 Feedback
- LLM:
anthropic--claude-4.5-sonnet - Event Trigger:
issue_comment.created - Correlation ID:
b257e840-f201-11f0-910d-927a814d12e7
...ds/feature/attachments/handler/applicationservice/helper/ModifyApplicationHandlerHelper.java
Outdated
Show resolved
Hide resolved
...com/sap/cds/feature/attachments/handler/applicationservice/helper/ExtendedErrorStatuses.java
Show resolved
Hide resolved
...ava/com/sap/cds/feature/attachments/handler/applicationservice/UpdateAttachmentsHandler.java
Show resolved
Hide resolved
Co-authored-by: hyperspace-insights[bot] <209611008+hyperspace-insights[bot]@users.noreply.github.com>
|
Added back the InputStream wrapper because Content-Length won't be available for HTTP2 or chunked uploads. |
Validation Maximum Annotation for File Attachments
New Features
✨ Introduced file size validation using the
@Validation.Maximumannotation to enforce maximum attachment sizes during upload operations. The feature supports human-readable size formats (e.g., "20MB", "5MB") and provides proper HTTP 413 error responses when size limits are exceeded.Changes
Core Implementation
UpdateAttachmentsHandler.java: Enhanced update handler to validate attachment sizes before processing. Refactored attachment deletion logic to use direct iteration instead ofCdsDataProcessor. Improved code formatting and field ordering for better maintainability.FileSizeUtils.java: Created utility class to parse human-readable file size strings into byte values. Supports both decimal (KB, MB, GB, TB) and binary (KiB, MiB, GiB, TiB) units with comprehensive validation and error handling.ModifyApplicationHandlerHelper.java: Integrated size validation logic using thegetValMaxValue()method to extract annotation values. Added Content-Length header validation and proper error handling with HTTP 413 responses when limits are exceeded.ExtendedErrorStatuses.java: Added new enum definingCONTENT_TOO_LARGEerror status with HTTP 413 code for size validation failures.ApplicationHandlerHelper.java: AddedextractKeys()helper method to extract key fields from CdsData based on entity definitions.messages.properties: Added localized error message for attachment size validation failures.Testing
CreateAttachmentsHandlerTest.java,UpdateAttachmentsHandlerTest.java,DraftPatchAttachmentsHandlerTest.java: Updated tests to mockParameterInfoand handle validation scenarios. Improved test assertions and data setup for better coverage.FileSizeUtilsTest.java: Comprehensive test suite for file size parsing covering valid formats, case-insensitive units, error handling, and edge cases.ExtendedErrorStatusesTest.java: Test coverage for the new error status enum.Attachments2SizeValidationDraftTest.java,Attachments2SizeValidationNonDraftTest.java: Integration tests validating the size limit enforcement for both draft and non-draft scenarios, including upload and update operations.OdataRequestValidationBase.java,DraftOdataRequestValidationBase.java: MadepersistenceServicefield protected and added helper methods for attachments2 navigation and content upload.Sample Application
attachments.cds: Demonstrated usage by adding@Validation.Maximum: '20MB'annotation to Books attachments content field in the bookshop sample.data-model.cds: Addedattachments2composition to Roots entity for integration testing purposes.test-service.cds: Applied@Validation.Maximum: '5MB'annotation toRoots.attachments2for validation testing.pom.xml: Updated project version from1.2.4to1.2.5-SNAPSHOTand upgraded Spring Boot to3.5.9..gitignore: Added.cdsrc-private.jsonto ignore CDS-specific private configuration files.