On Fri, 28 Jul 2023 18:41:48 GMT, Joe Wang <jo...@openjdk.org> wrote:
> Add a JDK Impl specific property 'jdk.xml.dtd.support' for applications to > specify how DTDs are handled. This property is uniformly supported across the > JDK XML libraries. It complements, rather than replaces, the existing > properties that are supportDTD for StAX and disallow-doctype-decl for DOM and > SAX processors, which means the later will continue to work as they were and > that if they are set, the new property will have no effect. Applications that > use the existing properties continue to work as expected. > > This patch continues the path made previously with Xalan and XPath in > which functions were moved into the jdk/xml classes. Similar changes are now > made with the Xerces and XML classes, consolidating functions into the > jdk/xml classes, reducing duplicate code for easier future maintenance. > > Tests: new tests are added to cover the various processors. > Existing tests pass. Only one was updated due to the change to the > property impl. OK I looked through a bunch of the code and I didn't see anything that needs to be changed right now. However I did spot a number of issues for future work. :-) Good to see the duplicate XMLSecurityManager has been removed. However (not part of this diff) I note that there are two XMLSecurityPropertyManager classes in xerces and xalan. The code between them seems quite similar, so this might be an opportunity for future deduplication. Most of the logic around handling of properties and config settings looks like it's in jdk/xml/internal. There are bunches of enums here and String arrays indexed by enum ordinals, plus things that search the enum values() arrays. It seems like some things could be done to improve the internal representation of this information, both to reduce the amount of code, and to make things easier for clients of this information. It looks like ValueMapper1 is unused. In XMLDTDScannerImpl.java line 391 this line is inserted: fEntityScanner = fEntityManager.getEntityScanner(); The addition of this line kind of sticks out. The initial value of this field is null. This assignment is done repeatedly in a bunch of places; but other places just assume the field has been initialized. Was this an NPE, and are there possibilities of NPE in other code paths? I think the thing to do is to integrate this code as is, since it's already been tested, and then identify a few candidates for cleanup work. Then chip away at some cleanups in parallel with pursuing other design activities (e.g., Catalog). ------------- Marked as reviewed by smarks (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/15075#pullrequestreview-1616760862
