-
Notifications
You must be signed in to change notification settings - Fork 74
MLE-26420 Improved error handling for incremental writes #1869
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
Conversation
|
Copyright Validation Results ✅ Valid Files
✅ All files have valid copyright headers! |
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.
Pull request overview
This PR improves error handling in the incremental write functionality by ensuring that failures during JSON canonicalization are properly logged while still allowing the write operation to proceed or fail based on the actual format of the content. The key change is the addition of comprehensive test coverage for invalid JSON scenarios and improved documentation around error handling behavior.
Key Changes:
- Added test coverage for invalid JSON handling with and without explicit format specification
- Enhanced error handling documentation in
IncrementalWriteFilter - Refactored test setup to reduce code duplication
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| IncrementalWriteTest.java | Added tests for invalid JSON scenarios, refactored test setup with shared state variables, and removed unused WriteBatcherTemplate utility class |
| IncrementalWriteFilter.java | Updated comment to clarify error handling strategy when JSON canonicalization fails |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| List<DocumentWriteOperation> docs = new ArrayList<>(); | ||
| IncrementalWriteFilter filter; | ||
|
|
||
| @BeforeEach | ||
| void setup() { |
Copilot
AI
Dec 31, 2025
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.
The docs list is now shared across all test methods as instance state. This can lead to test pollution if not properly cleared between tests. Consider initializing it in the @BeforeEach method or making it local to each test method to ensure test isolation.
| List<DocumentWriteOperation> docs = new ArrayList<>(); | |
| IncrementalWriteFilter filter; | |
| @BeforeEach | |
| void setup() { | |
| List<DocumentWriteOperation> docs; | |
| IncrementalWriteFilter filter; | |
| @BeforeEach | |
| void setup() { | |
| // Ensure docs is reinitialized for each test to avoid shared state between tests. | |
| docs = new ArrayList<>(); |
| private void runTest() { | ||
| @Test | ||
| void invalidJsonWithNoFormat() { | ||
| docs.add(new DocumentWriteOperationImpl("/aaa.txt", METADATA, new StringHandle("{\"not actually json"))); |
Copilot
AI
Dec 31, 2025
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.
Corrected spelling of 'aaa' to a more descriptive test file name.
|
|
||
| @Test | ||
| void invalidJsonWithFormat() { | ||
| docs.add(new DocumentWriteOperationImpl("/aaa.json", METADATA, new StringHandle("not actually json").withFormat(Format.JSON))); |
Copilot
AI
Dec 31, 2025
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.
Corrected spelling of 'aaa' to a more descriptive test file name.
a96d910 to
88c9beb
Compare
No description provided.