Skip to content

Conversation

@Schmarvinius
Copy link
Collaborator

@Schmarvinius Schmarvinius commented Jan 12, 2026

Validation Maximum Annotation for File Attachments

New Features

✨ Introduced file size validation using the @Validation.Maximum annotation 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 of CdsDataProcessor. 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 the getValMaxValue() 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 defining CONTENT_TOO_LARGE error status with HTTP 413 code for size validation failures.

  • ApplicationHandlerHelper.java: Added extractKeys() 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 mock ParameterInfo and 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: Made persistenceService field 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: Added attachments2 composition to Roots entity for integration testing purposes.

  • test-service.cds: Applied @Validation.Maximum: '5MB' annotation to Roots.attachments2 for validation testing.

  • pom.xml: Updated project version from 1.2.4 to 1.2.5-SNAPSHOT and upgraded Spring Boot to 3.5.9.

  • .gitignore: Added .cdsrc-private.json to ignore CDS-specific private configuration files.

  • 🔄 Regenerate and Update Summary

@Schmarvinius Schmarvinius self-assigned this Jan 12, 2026
Copy link
Contributor

@hyperspace-insights hyperspace-insights bot left a 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:

  1. 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.

  2. Type mismatches: The method convertValMaxToInt returns long but is named as if it returns int, creating confusion throughout the codebase.

  3. Poor error handling: IOExceptions are wrapped in generic RuntimeExceptions, losing important context for debugging and error recovery.

  4. Performance inefficiency: The condenseAttachments method is called twice with the same data, wasting processing time.

  5. 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

@Schmarvinius Schmarvinius marked this pull request as ready for review January 15, 2026 09:22
Copy link
Contributor

@hyperspace-insights hyperspace-insights bot left a 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:

  1. Critical Logic Error in Exception Handling: The ModifyApplicationHandlerHelper catches its own thrown RuntimeException, which causes incorrect error handling. Other RuntimeException subtypes like IllegalArgumentException from parsing will be misclassified as size validation errors instead of bad request errors.

  2. Critical Logic Error in ExtendedErrorStatuses: The getByCode() method iterates over the wrong enum (ErrorStatuses instead of ExtendedErrorStatuses) and compares the wrong field (HTTP status vs. code), making the method non-functional.

  3. Performance/Maintainability Issue: The UpdateAttachmentsHandler contains duplicate code for key extraction that could use the newly added ApplicationHandlerHelper.extractKeys() method instead.

  4. 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

@hyperspace-insights hyperspace-insights bot deleted a comment from Schmarvinius Jan 15, 2026
Schmarvinius and others added 4 commits January 15, 2026 12:10
Co-authored-by: hyperspace-insights[bot] <209611008+hyperspace-insights[bot]@users.noreply.github.com>
@Schmarvinius
Copy link
Collaborator Author

Added back the InputStream wrapper because Content-Length won't be available for HTTP2 or chunked uploads.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants