Conversation
|
Copyright Validation Results ⏭️ Skipped (Excluded) Files
✅ Valid Files
✅ All files have valid copyright headers! |
rjrudin
force-pushed
the
feature/27078-fix
branch
from
71bd35a to
26d9081
Compare
There was a problem hiding this comment.
Pull request overview
This pull request fixes a bug (MLE-27078) in DocumentManagerImpl where a class field was being used to track whether metadata categories were modified, leading to incorrect behavior. The fix replaces the stateful tracking with a simple method that determines if the metadata categories differ from the default (ALL).
Changes:
- Removed the stateful
isProcessedMetadataModifiedboolean field and the overriddenadd/addAllmethods in theprocessedMetadataHashSet - Introduced a new private method
isMetadataCategoriesNotJustAll()to check if metadata categories have been modified from the default - Added comprehensive test coverage demonstrating the fix allows bulk reads after setting metadata to ALL
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| ReadJsonAsBinaryTest.java | Adds new test methods to verify bulk read behavior with different metadata category configurations, including the case where setting to ALL should work correctly |
| DocumentManagerImpl.java | Removes problematic stateful tracking and replaces it with a cleaner method-based approach; updates all references from processedMetadata to metadataCategories |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| private boolean isMetadataCategoriesNotJustAll() { | ||
| return metadataCategories == null || | ||
| metadataCategories.size() != 1 || | ||
| !Metadata.ALL.equals(metadataCategories.iterator().next()); | ||
| } |
There was a problem hiding this comment.
The null check for metadataCategories is redundant. The field is instantiated as a new HashSet on line 58 and can never be null. The check should be removed or replaced with a check for an empty set if that's the intended behavior.
| private boolean isMetadataCategoriesNotJustAll() { | ||
| return metadataCategories == null || | ||
| metadataCategories.size() != 1 || | ||
| !Metadata.ALL.equals(metadataCategories.iterator().next()); | ||
| } | ||
|
|
||
| @Override | ||
| public boolean addAll(Collection<? extends Metadata> c) { | ||
| isProcessedMetadataModified = true; | ||
| return super.addAll(c); | ||
| } | ||
| }; | ||
| { | ||
| processedMetadata.add(Metadata.ALL); | ||
| // we need to know if the user modifies after us | ||
| isProcessedMetadataModified = false; | ||
| } | ||
| final private Set<Metadata> metadataCategories = new HashSet<>(); | ||
|
|
||
| { | ||
| // Initialize the default metadata to retrieve on a read. | ||
| metadataCategories.add(Metadata.ALL); | ||
| } |
There was a problem hiding this comment.
Inconsistent indentation: this file uses 2-space indentation throughout (see lines 44-45, 49-50, 74-77, 617-623), but tabs are used here. Replace tabs with 2 spaces to maintain consistency.
| for (Metadata category : categories) { | ||
| metadataCategories.add(category); | ||
| } |
There was a problem hiding this comment.
Inconsistent indentation: this file uses 2-space indentation throughout (see lines 44-45, 49-50, 74-77, 617-623), but tabs are used here. Replace tabs with 2 spaces to maintain consistency.
rjrudin
force-pushed
the
feature/27078-fix
branch
3 times, most recently
from
b82f981 to
06eb09b
Compare
rjrudin
changed the title
rjrudin
force-pushed
the
feature/27078-fix
branch
from
06eb09b to
889252b
Compare
Made a very confusing private field easier to understand in DocumentManagerImpl.
rjrudin
force-pushed
the
feature/27078-fix
branch
from
889252b to
31e1744
Compare
2 participants
Made a very confusing private field easier to understand in DocumentManagerImpl.