MLE-26918 Refactor: Changed config class back to a non-record class#1910
MLE-26918 Refactor: Changed config class back to a non-record class#1910
Conversation
Had a discussion with Copilot, agreed that a record isn't a fit here as it implies immutability but that's not currently the case.
|
Copyright Validation Results ✅ Valid Files
✅ All files have valid copyright headers! |
There was a problem hiding this comment.
Pull request overview
This pull request refactors IncrementalWriteConfig from a Java record to a regular class with explicit field declarations, constructor, and getter methods. The decision was made because the class does not maintain true immutability despite the record's immutability semantics.
Changes:
- Converted
IncrementalWriteConfigfrom arecordto a standard class with private final fields and public getter methods - Updated all accessor calls across three filter classes to use standard getter method syntax instead of record accessor syntax
- Maintained the custom
getXmlNamespaces()behavior that returns an empty map instead of null
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| IncrementalWriteConfig.java | Refactored from record to class with explicit fields, constructor, and getter methods |
| IncrementalWriteFilter.java | Updated all config accessor calls to use getter methods (e.g., config.hashKeyName() → config.getHashKeyName()) |
| IncrementalWriteFromViewFilter.java | Updated config accessor calls to use getter methods |
| IncrementalWriteFromLexiconsFilter.java | Updated config accessor call to use getter method |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public String[] getJsonExclusions() { | ||
| return jsonExclusions; | ||
| } |
There was a problem hiding this comment.
The array fields jsonExclusions and xmlExclusions should be defensively copied in their getters to prevent external modification. Unlike Java records which automatically provide defensive copies for arrays, the current implementation allows callers to modify the internal array state. Consider returning Arrays.copyOf(jsonExclusions, jsonExclusions.length) or jsonExclusions != null ? jsonExclusions.clone() : null instead of returning the array directly.
| @Override | ||
| public Map<String, String> xmlNamespaces() { | ||
| public String[] getXmlExclusions() { | ||
| return xmlExclusions; |
There was a problem hiding this comment.
The array field xmlExclusions should be defensively copied in its getter to prevent external modification. Unlike Java records which automatically provide defensive copies for arrays, the current implementation allows callers to modify the internal array state. Consider returning Arrays.copyOf(xmlExclusions, xmlExclusions.length) or xmlExclusions != null ? xmlExclusions.clone() : null instead of returning the array directly.
| return xmlExclusions; | |
| return xmlExclusions != null ? xmlExclusions.clone() : null; |
2 participants
Had a discussion with Copilot, agreed that a record isn't a fit here as it implies immutability but that's not currently the case.