Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8303530: Redefine JAXP Configuration File #12985

Closed
wants to merge 15 commits into from

Conversation

JoeWang-Java
Copy link
Member

@JoeWang-Java JoeWang-Java commented Mar 11, 2023

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.


Progress

  • Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change requires CSR request JDK-8303531 to be approved

Issues

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/12985/head:pull/12985
$ git checkout pull/12985

Update a local copy of the PR:
$ git checkout pull/12985
$ git pull https://git.openjdk.org/jdk.git pull/12985/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 12985

View PR using the GUI difftool:
$ git pr show -t 12985

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/12985.diff

Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Mar 11, 2023

👋 Welcome back joehw! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr Pull request is ready for review label Mar 11, 2023
@openjdk
Copy link

openjdk bot commented Mar 11, 2023

@JoeWang-Java The following label will be automatically applied to this pull request:

  • core-libs

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added core-libs core-libs-dev@openjdk.org csr Pull request needs approved CSR before integration labels Mar 11, 2023
@mlbridge
Copy link

mlbridge bot commented Mar 11, 2023

* <h2 id="ConfigurationFile">JAXP Configuration File</h2>
* JAXP supports the use of a configuration file for the
* <a href="#LookupMechanism">Factory Lookup Mechanism</a> and
* setting properties that have defined corresponding system properties.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The first sentence of the module description lists the 4 XML APIs and their acronyms. It is immediately followed by a section with title "JAXP Configuration File" which suggests that this is a configuration file for the first API that is listed (JAXP). As I understand it, properties for the streaming API can also be defined in this file. So it might be that a bit more setup is needed in the opening text to make it clearer that this is the configuration file for all of the APIs.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One other thing to say on this is that jaxp.properties has "XML Library (java.xml) Configuration File" at the top of the file, maybe that could be used in the module description instead of "JAXP Configuration File".

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed the opening statement and also the naming of config file as above.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've looked through the updated proposal but I think the intro is going to take a few iterations to get to something that is easy to read. I had hoped the intro would provide an overview of the APIs in the module and establish the terminology that is needed to read further. The existing text isn't great but it does name the APIs in the module so the reader knows there are several APIs for processing XML. With the update, the names of the APIs in the module is lost, and in its place, the text switches to "XML library" and talks about factories and processors. I understand the focus of this PR is to introduce a system property to allow for configuration file but it's changing important API docs that introduce the APIs.

My view is that we have to create a new introduction that provides a summary of the APIs in the module and a short summary of some concepts. We can't have API docs talk about "factories" and "processors" without defining them. This intro/setup is needed before diving into the configuration, which we might title "Configuring factories and processors" with a summary of the various ways that they can be configured. I think the text from the "Property Precedence" section will need to move into the section on Configuration as it is hard to list the various ways to configure without also talking about precedence.

Once you have a good introduction and seciton on Configuration then I think the other sections will follow without too much work.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New introduction added, along with sections leading up to the Configuration File.

src/java.xml/share/classes/module-info.java Outdated Show resolved Hide resolved
src/java.xml/share/classes/module-info.java Outdated Show resolved Hide resolved
src/java.xml/share/classes/module-info.java Outdated Show resolved Hide resolved
src/java.xml/share/classes/module-info.java Outdated Show resolved Hide resolved
src/java.xml/share/classes/module-info.java Outdated Show resolved Hide resolved
Copy link
Member

@naotoj naotoj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding the new unit tests, it looks like there are a lot of boilerplate codes. Can they be shared?

@JoeWang-Java
Copy link
Member Author

Regarding the new unit tests, it looks like there are a lot of boilerplate codes. Can they be shared?

Multiple tests per a processor, and separate test per each processor, if that's what you meant, they unfortunately can not be run within one test since the config file is defined to be loaded once only.

@JoeWang-Java JoeWang-Java changed the title 8303530: Add system property for custom JAXP configuration file 8303530: Redefine JAXP Configuration File May 12, 2023
…ctions leading to the Configuration File section with most of the text edited; removed duplicate sections in implNote; Other adjustments reflecting these changes.
@@ -179,7 +179,7 @@
*
* <ul>
* <li><p>
* Through the APIs for factories or processors
* With the APIs for factories or processors
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not think "With" makes sense here. perhaps just remove it completely

@@ -221,7 +221,7 @@
* }
* </li>
* <li><p>
* If the property is not set on the factory, or through its system property,
* If the property is not set on the factory, or with its system property,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perhaps. "or using a system property"

* If the {@code java.xml.config.file} property is included within a configuration
* file, it will be ignored.
* In addition to the {@code jaxp.properties} file, the system property
* {@systemProperty java.xml.config.file} can be set on the command line or at run-time
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can simplify to "...the system property {@systemProperty java.xml.config.file} can be set to specify..."

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Lance. Adjusted the javadoc accordingly.

@openjdk
Copy link

openjdk bot commented Jun 1, 2023

@JoeWang-Java This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8303530: Redefine JAXP Configuration File

Reviewed-by: naoto, lancea, alanb, smarks

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 112 new commits pushed to the master branch:

  • 1bb037b: 8309329: com/sun/jdi/DeferredStepTest.java fails with virtual threads due to not waiting for threads to exit
  • a23bbea: 8309241: ClassForNameLeak fails intermittently as the class loader hasn't been unloaded
  • 931913f: 8309200: java/net/httpclient/ExecutorShutdown fails intermittently, if connection closed during upgrade
  • dc21e8a: 8296411: AArch64: Accelerated Poly1305 intrinsics
  • 59d9d9f: 8303215: Make thread stacks not use huge pages
  • cb1e5e3: 8309286: G1: Remove unused G1HeapRegionAttr::is_valid_gen
  • e8268d9: 8309210: Extend VM Operations hs_err logging
  • 7dbdad5: 8308892: Bad graph detected in build_loop_late after JDK-8305635
  • dc8bc6c: 8308090: Add container tests for on-the-fly resource quota updates
  • 73e7af9: 8309287: Add fontconfig requirement to building.md for Debian
  • ... and 102 more: https://git.openjdk.org/jdk/compare/4870234552d2c63c786641493794a87654b98b7b...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added ready Pull request is ready to be integrated and removed csr Pull request needs approved CSR before integration labels Jun 1, 2023
Copy link
Contributor

@AlanBateman AlanBateman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm very happy where the spec changes have got to in this PR, esp. the module description and allowing a config file be specified with all configuration.

* @since 21
*/
public static final String CONFIG_FILE = "java.xml.config.file";

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't the name of the config file but instead is the name of the property that points to the config file. At some point maybe this could be renamed, maybe to CONFIG_FILE_PROPNAME or something.

} catch (NumberFormatException e) {
//invalid setting
throw new NumberFormatException("Invalid setting for system property: " + limit.systemProperty());
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kind of a questionable idiom here to catch NFE and then throw a new NFE, discarding the previous one. Maybe wrap instead? But the larger question is, I think, what should happen in this case. If the user sets a malformed property, will this cause exceptions to be thrown at arbitrary points in the program? Both items are things to consider for later.

break;
}
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a little hard to see what's going on here but this essentially boils down to a Map<String,String> mapping new names to old names. This or further refactoring should be considered.

@stuart-marks
Copy link
Member

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.

@JoeWang-Java
Copy link
Member Author

Thanks! Will discuss these issues in separate threads for each of them. The question on the duplication of XMLSecurityManager has an easier answer: the later was added to replace the former that was specific to Xerces. Note that the one for Xalan was already deleted. There were too many references. Since this changeset is already pretty large, I decided to leave it to a simpler changeset (e.g. DTD) to do it.

@JoeWang-Java
Copy link
Member Author

/integrate

@openjdk
Copy link

openjdk bot commented Jun 2, 2023

Going to push as commit aff9cea.
Since your change was applied there have been 112 commits pushed to the master branch:

  • 1bb037b: 8309329: com/sun/jdi/DeferredStepTest.java fails with virtual threads due to not waiting for threads to exit
  • a23bbea: 8309241: ClassForNameLeak fails intermittently as the class loader hasn't been unloaded
  • 931913f: 8309200: java/net/httpclient/ExecutorShutdown fails intermittently, if connection closed during upgrade
  • dc21e8a: 8296411: AArch64: Accelerated Poly1305 intrinsics
  • 59d9d9f: 8303215: Make thread stacks not use huge pages
  • cb1e5e3: 8309286: G1: Remove unused G1HeapRegionAttr::is_valid_gen
  • e8268d9: 8309210: Extend VM Operations hs_err logging
  • 7dbdad5: 8308892: Bad graph detected in build_loop_late after JDK-8305635
  • dc8bc6c: 8308090: Add container tests for on-the-fly resource quota updates
  • 73e7af9: 8309287: Add fontconfig requirement to building.md for Debian
  • ... and 102 more: https://git.openjdk.org/jdk/compare/4870234552d2c63c786641493794a87654b98b7b...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Jun 2, 2023
@openjdk openjdk bot closed this Jun 2, 2023
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jun 2, 2023
@openjdk
Copy link

openjdk bot commented Jun 2, 2023

@JoeWang-Java Pushed as commit aff9cea.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@JoeWang-Java JoeWang-Java deleted the JDK-8303530 branch June 6, 2023 21:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core-libs core-libs-dev@openjdk.org integrated Pull request has been integrated
5 participants