RFR: 8303530: Redefine JAXP Configuration File [v15]

[Stuart Marks]

On Wed, 31 May 2023 21:58:44 GMT, Joe Wang <joehw at openjdk.org> wrote:

>> Add a system property, jdk.xml.config.file, to return the path to a custom JAXP configuration file. The current configuration file, jaxp.properties, that the JDK supports will become the default configuration file.
>> 
>> CSR: https://bugs.openjdk.org/browse/JDK-8303531
>> 
>> Tests: XML SQE and JCK tests passed.
>
> Joe Wang has updated the pull request incrementally with one additional commit since the last revision:
> 
>   adjust javadoc

Marked as reviewed by smarks (Reviewer).

Overall I think this is OK to go in as it stands. The specs have been thoroughly reviewed and the loading of the new config file needs to match the specs and it's been well tested. I especially like how the duplicated logic in the factory finders has been converted to centralized infrastructure.

Now that things are centralized, it's easier to find and highlight additional items for cleanup. This probably can be considered as technical debt that can be worked on over time. I've mentioned a few things near individual places in the code; I'll repeat them here for completeness. In addition there are a few other things I noticed that we should also probably look at at some point in the future.

JdkConstants.CONFIG_FILE could probably be renamed.

NameMap is mapping from new to old property names. This could probably be changed to a Map<String,String> instead of a search through the enum's values. There are only three enums (I think) so this isn't terrible, but it does add some extra code that could be simplified if it were changed to an actual Map. Another alternative is to merge the old name into the Limits enum itself, as another field; that would save a lookup and avoid having to keep a separate data structure in sync.

There are a few places where NumberFormatException is caught and then a new NFE is thrown, discarding the old one. The new one provides more information but it's a bit odd not to chain the exceptions. Unfortunately NFE doesn't have a chaining constructor; maybe one should be added? Alternatively, initCause() could be called.

A larger question is whether we want exceptions to be thrown from initialization code. This will have the effect of causing an exception to be thrown to arbitrary user code. (Or will it?) For property-reading code I think a better alternative is to ignore errant values, possibly issuing a warning message. However this will require further discussion.

There are two XMLSecurityManager files:
* com/sun/org/apache/xerces/internal/utils/XMLSecurityManager.java
* jdk/xml/internal/XMLSecurityManager.java

It seems like there's a lot of duplication between them. This should be revisited.

Meanwhile, in both XMLSecurityManagers the state loaded from config files, properties, etc. is stored in parallel arrays indexed by the enum's ordinals. This suggests an alternative which is to use an EnumMap. The value of the EnumMap could be a record consisting of the property value and its state (origin) and possibly whether it was set explicitly.

At some point we also might want to take another look at the tests. Testing configuration files indeed requires multiple invocations of the JVM. There are ways a single jtreg test can invoke multiple JVMs using multiple `@run` lines. Each can pass different arguments in order to run different test cases, even different test classes if necessary. But this will require additional thinking and discussion.

-------------

PR Review: https://git.openjdk.org/jdk/pull/12985#pullrequestreview-1458364801
PR Comment: https://git.openjdk.org/jdk/pull/12985#issuecomment-1574210212