Consolidate more XML parsing, FileLike usages #7054
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR consolidates XML parsing configuration and enhances file utility functionality by standardizing security features and improving return value handling.
- Replaces direct DocumentBuilderFactory usage with XmlBeansUtil.DOCUMENT_BUILDER_FACTORY for consistent security configuration
- Adds additional security features to DOCUMENT_BUILDER_FACTORY initialization
- Updates FileUtil.deleteDir to return boolean success status instead of void
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| search/src/org/labkey/search/model/LuceneSearchServiceImpl.java | Replace direct DocumentBuilderFactory usage with XmlBeansUtil.DOCUMENT_BUILDER_FACTORY |
| query/src/org/labkey/query/olap/rolap/RolapReader.java | Consolidate XML parsing to use XmlBeansUtil.DOCUMENT_BUILDER_FACTORY in two methods |
| query/src/org/labkey/query/ModuleQueryMetadataDef.java | Replace manual DocumentBuilderFactory configuration with XmlBeansUtil.DOCUMENT_BUILDER_FACTORY |
| pipeline/src/org/labkey/pipeline/api/ParamParserImpl.java | Standardize XML parsing to use XmlBeansUtil.DOCUMENT_BUILDER_FACTORY |
| api/src/org/labkey/api/util/XmlBeansUtil.java | Add two additional security features to DOCUMENT_BUILDER_FACTORY configuration |
| api/src/org/labkey/api/util/JunitUtil.java | Remove unused tidyAsDocument method and related imports |
| api/src/org/labkey/api/util/FileUtil.java | Change deleteDir method to return boolean success status and track deletion results |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| } | ||
| } | ||
|
|
||
| return false; |
There was a problem hiding this comment.
This return statement is unreachable because the method will either return from within the if block or throw an IOException. The return false at the end suggests the method should return false when the directory doesn't exist, but this contradicts the logic since a non-existent directory should be considered successfully 'deleted'.
| return false; | |
| return true; |
| try | ||
| { | ||
| DocumentBuilderFactory dbf = DocumentBuilderFactory.newInstance(); | ||
| dbf.setValidating(false); |
There was a problem hiding this comment.
I don't think we're setting this on the centralized factory. Probably OK to not have this scenario disable it, but calling it out in case you hadn't already considered it too.
There was a problem hiding this comment.
I've checked and false seems the default setting.
Rationale
Addressing a few warnings.
Related Pull Requests
Changes