8351969: Add Public Identifiers to the JDK built-in Catalog #24039

JoeWang-Java · 2025-03-13T19:01:03Z

Add public identifiers to the JDK built-in Catalog; Replace the incorrect Schema 1.1 DTD files (note the Public Identifier at line 2) with the correct Schema 1.0 DTDs. The JDK built-in Catalog contains Schema 1.0 files only.

DTD files: the Schema 1.1 files, XMLSchema.dtd and datatypes.dtd, were removed and replaced with those of Schema 1.0. These files were downloaded from w3.org without modification. The file names were unfortunately the same, which (with the Diffs) made it look like they were modified though they were not.

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

Issue

JDK-8351969: Add Public Identifiers to the JDK built-in Catalog (Bug - P4)

Reviewers

Lance Andersen (@LanceAndersen - Reviewer) Review applies to 4482d590
Iris Clark (@irisclark - Reviewer) Review applies to 4482d590
Roger Riggs (@RogerRiggs - Reviewer)
Naoto Sato (@naotoj - Reviewer) Review applies to 4482d590

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 24039

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-03-13T19:02:00Z

👋 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 · 2025-03-13T19:02:56Z

@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:

8351969: Add Public Identifiers to the JDK built-in Catalog

Reviewed-by: rriggs, lancea, iris, naoto

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 59 new commits pushed to the master branch:

fed34e4: 8351933: Inaccurate masking of TC subfield decrement in ForkJoinPool
8f64ccc: 8350485: C2: factor out common code in Node::grow() and Node::out_grow()
c2be19c: 8351902: RISC-V: Several tests fail after JDK-8351145
e57b272: 8350578: Refactor useless Parse and Template Assertion Predicate elimination code by using a PredicateVisitor
577ede7: 8352302: Test sun/security/tools/jarsigner/TimestampCheck.java is failing
20d4fe3: 8351464: Shenandoah: Hang on ShenandoahController::handle_alloc_failure when run test TestAllocHumongousFragment#generational
8e53063: 8352275: Clean up dead code in jpackage revealed with improved negative test coverage
c8a11f2: 8352293: jpackage tests build rpm packages on Ubuntu test machines after JDK-8351372
4a02de8: 8352098: -Xrunjdwp fails on static JDK
355b2f3: 8351374: Improve comment about queue.remove timeout in CleanerImpl.run
... and 49 more: https://git.openjdk.org/jdk/compare/771e160da4daa98bfe37bf1acba65454c088910c...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 · 2025-03-13T19:03:43Z

@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.

mlbridge · 2025-03-13T19:06:02Z

Webrevs

myankelev

Looks good to me, just a few very minor questions. Thank you

myankelev · 2025-03-14T15:30:27Z

test/jaxp/javax/xml/jaxp/unittest/common/jdkcatalog/JDKCatalogTest.java

+    public void setUpClass() throws Exception {
+        // initialize JDKCatalog
+        JdkCatalog.init("continue");
+        jdkCatalog = JdkCatalog.catalog;


This seems to be a set up just for 1 test. Do you think it might be better to have this logic in the beginning of the testDTDsInJDKCatalog?

myankelev · 2025-03-14T15:32:48Z

test/jaxp/javax/xml/jaxp/unittest/common/jdkcatalog/JDKCatalogTest.java

    static final String TARGET_NAMESPACE = "{{targetNamespace}}";
    static final String ROOT_ELEMENT = "{{rootElement}}";

+    Catalog jdkCatalog;


Nitpick: if this will stay as a global var I'd personally make it private and move to the top of the file. What do you think?

myankelev · 2025-03-14T15:45:21Z

test/jaxp/javax/xml/jaxp/unittest/common/jdkcatalog/JDKCatalogTest.java

 * @test
- * @bug 8344800 8345353
+ * @bug 8344800 8345353 8351969
+ * @modules java.xml/jdk.xml.internal


I know that this is not a part of your change, but this (line 56) doesn't seem to be used at all. Could you please remove it if there will be another revision? If not it's ok as is 😄
static String CLS_DIR = System.getProperty("test.classes");

myankelev · 2025-03-14T15:47:03Z

test/jaxp/javax/xml/jaxp/unittest/common/jdkcatalog/JDKCatalogTest.java

+     */
+    @Test(dataProvider = "DTDsInJDKCatalog")
+    public void testDTDsInJDKCatalog(String publicId, String systemId)
+            throws Exception {


Nit: Don't think throw exception is doing anything here.

myankelev · 2025-03-14T15:47:05Z

test/jaxp/javax/xml/jaxp/unittest/common/jdkcatalog/JDKCatalogTest.java

+     * Data provided: public and system Ids, see test testDTDsInJDKCatalog
+     */
+    @DataProvider(name = "DTDsInJDKCatalog")
+    public Object[][] getDTDsInJDKCatalog() throws Exception {


Nit: Don't think throw exception is doing anything here.

JoeWang-Java · 2025-03-14T18:35:12Z

Thanks for review. Updated the test.

myankelev · 2025-03-17T12:25:21Z

Thanks for review. Updated the test.

Thank you!

LanceAndersen

changes look OK Joe

RogerRiggs

The XMLSchema.dtd changes the referenced version from the 2009 version to the 2001 version. Is that intentional.

RogerRiggs · 2025-03-18T18:26:09Z

src/java.xml/share/classes/jdk/xml/internal/jdkcatalog/w3c/dtd/schema10/XMLSchema.dtd

+<!-- $Id: XMLSchema.dtd,v 1.31 2001/10/24 15:50:16 ht Exp $ -->
+<!-- Note this DTD is NOT normative, or even definitive. -->           <!--d-->
+<!-- prose copy in the structures REC is the definitive version -->    <!--d-->
+<!-- (which shouldn't differ from this one except for this -->         <!--d-->
+<!-- comment and entity expansions, but just in case) -->              <!--d-->


The other XML comments use the multi-line format with a single begin
What is the meaning of ""?
Is "REC" a typo?
The comment seems unnecessarily apologetic. It should be sufficient to say:
Note: this DTD is NOT normative, any differences from RFC XXX are insidential". (or similar).

Makes sense. The question would be, would we want to modify the standard file?

Likely, a number of my comments were due to comparing with the wrong XMLSchema.data version.

RogerRiggs · 2025-03-18T18:27:56Z

src/java.xml/share/classes/jdk/xml/internal/jdkcatalog/w3c/dtd/schema10/XMLSchema.dtd

+<!ENTITY % xs-datatypes PUBLIC 'datatypes' 'datatypes.dtd' >

-<!ENTITY % p 'xs:'> <!-- can be overridden in the internal subset of a
+<!ENTITY % p 'xs:'> <!-- can be overriden in the internal subset of a


The original version is correct "overridden".

Typos in the original should be retained to avoid gratuitous difference.

RogerRiggs · 2025-03-18T18:55:32Z

test/jaxp/javax/xml/jaxp/unittest/common/jdkcatalog/JDKCatalogTest.java

 */
 package common.jdkcatalog;

+import static jaxp.library.JAXPTestUtilities.SRC_DIR;


Static imports across classes (even with a package) make it harder to read the test and know where it is imported from. The reference in the source here should include the class its imported from.

RogerRiggs · 2025-03-18T19:05:32Z

test/jaxp/javax/xml/jaxp/unittest/common/jdkcatalog/JDKCatalogTest.java

+        String matchingPubId = JDKCATALOG.matchPublic(publicId);
+        String matchingSysId = JDKCATALOG.matchSystem(systemId);


Isn't there a public API to get these values. It seems a bit of a hack to have to break module encapsulation.

Updated. Thanks!

JoeWang-Java · 2025-03-18T19:40:01Z

The XMLSchema.dtd changes the referenced version from the 2009 version to the 2001 version. Is that intentional.

Yes. Note the Public Identifier: "-//W3C//DTD XSD 1.1//EN" vs "-//W3C//DTD XMLSCHEMA 200102//EN" at line 2. The 2009 version was for Schema 1.1 while the 2001 version for Schema 1.0. As shown in the JDKCatalog.xml, only Schema 1.0 is included in the catalog. It's a bit unfortunate the files were named the same.

The 1.1 version file was indeed better formatted/worded. But since this is standard file, it was a simple copy without modification.

RogerRiggs

The .dtds look fine.
Thanks for the update to the test.

RogerRiggs · 2025-03-19T13:50:04Z

src/java.xml/share/classes/jdk/xml/internal/jdkcatalog/w3c/dtd/schema10/XMLSchema.dtd

+<!ENTITY % xs-datatypes PUBLIC 'datatypes' 'datatypes.dtd' >

-<!ENTITY % p 'xs:'> <!-- can be overridden in the internal subset of a
+<!ENTITY % p 'xs:'> <!-- can be overriden in the internal subset of a


Typos in the original should be retained to avoid gratuitous difference.

JoeWang-Java · 2025-03-19T16:35:52Z

Thanks all for the reviews!

JoeWang-Java · 2025-03-19T16:36:00Z

/integrate

openjdk · 2025-03-19T16:37:05Z

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

c7f3338: 8352109: java/awt/Desktop/MailTest.java fails in platforms where Action.MAIL is not supported
fed34e4: 8351933: Inaccurate masking of TC subfield decrement in ForkJoinPool
8f64ccc: 8350485: C2: factor out common code in Node::grow() and Node::out_grow()
c2be19c: 8351902: RISC-V: Several tests fail after JDK-8351145
e57b272: 8350578: Refactor useless Parse and Template Assertion Predicate elimination code by using a PredicateVisitor
577ede7: 8352302: Test sun/security/tools/jarsigner/TimestampCheck.java is failing
20d4fe3: 8351464: Shenandoah: Hang on ShenandoahController::handle_alloc_failure when run test TestAllocHumongousFragment#generational
8e53063: 8352275: Clean up dead code in jpackage revealed with improved negative test coverage
c8a11f2: 8352293: jpackage tests build rpm packages on Ubuntu test machines after JDK-8351372
4a02de8: 8352098: -Xrunjdwp fails on static JDK
... and 50 more: https://git.openjdk.org/jdk/compare/771e160da4daa98bfe37bf1acba65454c088910c...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-03-19T16:37:12Z

@JoeWang-Java Pushed as commit 8e999b8.

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

8351969: Add Public Identifiers to the JDK built-in Catalog

c423ef9

openjdk bot added the rfr Pull request is ready for review label Mar 13, 2025

openjdk bot added the core-libs core-libs-dev@openjdk.org label Mar 13, 2025

myankelev reviewed Mar 14, 2025

View reviewed changes

update the test: change variables and etc.

4482d59

LanceAndersen approved these changes Mar 18, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Mar 18, 2025

irisclark approved these changes Mar 18, 2025

View reviewed changes

RogerRiggs suggested changes Mar 18, 2025

View reviewed changes

RogerRiggs reviewed Mar 18, 2025

View reviewed changes

naotoj approved these changes Mar 18, 2025

View reviewed changes

replace JDK Catalog in the java.xml module with public API

0b5a1cc

openjdk bot removed the ready Pull request is ready to be integrated label Mar 19, 2025

RogerRiggs approved these changes Mar 19, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Mar 19, 2025

openjdk bot added the integrated Pull request has been integrated label Mar 19, 2025

openjdk bot closed this Mar 19, 2025

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Mar 19, 2025

JoeWang-Java deleted the JDK-8351969 branch March 31, 2025 17:30

		String matchingPubId = JDKCATALOG.matchPublic(publicId);
		String matchingSysId = JDKCATALOG.matchSystem(systemId);

8351969: Add Public Identifiers to the JDK built-in Catalog #24039

8351969: Add Public Identifiers to the JDK built-in Catalog #24039

Uh oh!

Conversation

JoeWang-Java commented Mar 13, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Mar 13, 2025

Uh oh!

openjdk bot commented Mar 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Mar 13, 2025

Uh oh!

mlbridge bot commented Mar 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

myankelev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JoeWang-Java commented Mar 14, 2025

Uh oh!

myankelev commented Mar 17, 2025

Uh oh!

LanceAndersen left a comment

Choose a reason for hiding this comment

Uh oh!

RogerRiggs left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JoeWang-Java commented Mar 18, 2025

Uh oh!

RogerRiggs left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JoeWang-Java commented Mar 19, 2025

Uh oh!

JoeWang-Java commented Mar 19, 2025

Uh oh!

openjdk bot commented Mar 19, 2025

Uh oh!

openjdk bot commented Mar 19, 2025

Uh oh!

Reviewers

Assignees

Labels

Milestone

JoeWang-Java commented Mar 13, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Mar 13, 2025 •

edited

Loading

mlbridge bot commented Mar 13, 2025 •

edited

Loading