Refactor XML handling #667

jukzi · 2023-07-12T10:17:54Z

class PDEXmlProcessorFactory combines classes:
XmlDocumentBuilderFactory,
XmlParserFactory,
XmlTransformerFactory

[performance] reuse single DocumentBuilderFactory instance see PDEXmlProcessorFactoryTest.main(String[]) - 100x faster

github-actions · 2023-07-12T10:47:19Z

Test Results

    246 files -   6     246 suites - 6 58m 44s ⏱️ - 2m 43s
  3 298 tests +  5   3 274 ✔️ +11 24 💤 ±0 0 ❌ - 3
10 191 runs +15 10 119 ✔️ +21 72 💤 ±0 0 ❌ - 3

Results for commit c0c32f4. ± Comparison against base commit 13c0d89.

This pull request removes 8 and adds 13 tests. Note that renamed tests count towards both.

AllPDEMinimalTests org.eclipse.pde.core.tests.internal.AllPDECoreTests org.eclipse.pde.core.tests.internal.XmlDocumentBuilderFactoryTest ‑ testTransformXmlWithExternalEntity
AllPDEMinimalTests org.eclipse.pde.core.tests.internal.AllPDECoreTests org.eclipse.pde.core.tests.internal.XmlDocumentBuilderFactoryTest ‑ testTransformXmlWithoutExternalEntity
AllPDEMinimalTests org.eclipse.pde.core.tests.internal.AllPDECoreTests org.eclipse.pde.core.tests.internal.XmlParserFactoryTest testParseXmlWithExternalEntity ‑ Unknown test
AllPDEMinimalTests org.eclipse.pde.core.tests.internal.AllPDECoreTests org.eclipse.pde.core.tests.internal.XmlParserFactoryTest ‑ testParseXmlWithIgnoredExternalEntity
AllPDEMinimalTests org.eclipse.pde.core.tests.internal.AllPDECoreTests org.eclipse.pde.core.tests.internal.XmlParserFactoryTest ‑ testParseXmlWithoutExternalEntity
AllPDEMinimalTests org.eclipse.pde.core.tests.internal.AllPDECoreTests org.eclipse.pde.core.tests.internal.XmlParserFactoryTest ‑ testParseXmlWithoutIgnoredExternalEntity
AllPDEMinimalTests org.eclipse.pde.core.tests.internal.AllPDECoreTests org.eclipse.pde.core.tests.internal.XmlTransformerTest ‑ testTransformXmlWithExternalEntity
AllPDEMinimalTests org.eclipse.pde.core.tests.internal.AllPDECoreTests org.eclipse.pde.core.tests.internal.XmlTransformerTest ‑ testTransformXmlWithoutExternalEntity

AllPDEMinimalTests org.eclipse.pde.core.tests.internal.AllPDECoreTests org.eclipse.pde.core.tests.internal.PDEXmlProcessorFactoryTest testParseXmlWithExternalEntity ‑ Unknown test
AllPDEMinimalTests org.eclipse.pde.core.tests.internal.AllPDECoreTests org.eclipse.pde.core.tests.internal.PDEXmlProcessorFactoryTest ‑ testDocumentBuilderFactoryIgnoringDoctypeMalcious
AllPDEMinimalTests org.eclipse.pde.core.tests.internal.AllPDECoreTests org.eclipse.pde.core.tests.internal.PDEXmlProcessorFactoryTest ‑ testDocumentBuilderFactoryIgnoringDoctypeNormal
AllPDEMinimalTests org.eclipse.pde.core.tests.internal.AllPDECoreTests org.eclipse.pde.core.tests.internal.PDEXmlProcessorFactoryTest ‑ testDocumentBuilderFactoryWithoutExternalEntity
AllPDEMinimalTests org.eclipse.pde.core.tests.internal.AllPDECoreTests org.eclipse.pde.core.tests.internal.PDEXmlProcessorFactoryTest ‑ testDocumentBuilderIgnoringDoctypeMalcious
AllPDEMinimalTests org.eclipse.pde.core.tests.internal.AllPDECoreTests org.eclipse.pde.core.tests.internal.PDEXmlProcessorFactoryTest ‑ testDocumentBuilderIgnoringDoctypeNormal
AllPDEMinimalTests org.eclipse.pde.core.tests.internal.AllPDECoreTests org.eclipse.pde.core.tests.internal.PDEXmlProcessorFactoryTest ‑ testDocumentBuilderWithoutExternalEntity
AllPDEMinimalTests org.eclipse.pde.core.tests.internal.AllPDECoreTests org.eclipse.pde.core.tests.internal.PDEXmlProcessorFactoryTest ‑ testDocumentBuilderXmlWithExternalEntity
AllPDEMinimalTests org.eclipse.pde.core.tests.internal.AllPDECoreTests org.eclipse.pde.core.tests.internal.PDEXmlProcessorFactoryTest ‑ testParseXmlWithIgnoredExternalEntity
AllPDEMinimalTests org.eclipse.pde.core.tests.internal.AllPDECoreTests org.eclipse.pde.core.tests.internal.PDEXmlProcessorFactoryTest ‑ testParseXmlWithoutExternalEntity
…

♻️ This comment has been updated with latest results.

HannesWell

Looks much better 👍🏽
I have a few remarks below how the code could be further improved.

ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/util/PDEXmlProcessorFactory.java

HannesWell · 2023-07-12T18:46:43Z

ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/util/PDEXmlProcessorFactory.java

+	/**
+	 * Creates DocumentBuilderFactory which throws SAXParseException when
+	 * detecting external entities.
+	 *
+	 * @return javax.xml.parsers.DocumentBuilderFactory
+	 */
+	public static SAXParserFactory createSAXFactoryWithErrorOnDOCTYPE() {
+		SAXParserFactory f = SAXParserFactory.newInstance();
+		try {
+			// force org.xml.sax.SAXParseException for any DOCTYPE:
+			f.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true); //$NON-NLS-1$
+		} catch (Exception e) {
+			throw new RuntimeException(e);
+		}
+		return f;
+	}
+


Suggested change

/**

* Creates DocumentBuilderFactory which throws SAXParseException when

* detecting external entities.

*

* @return javax.xml.parsers.DocumentBuilderFactory

*/

public static SAXParserFactory createSAXFactoryWithErrorOnDOCTYPE() {

SAXParserFactory f = SAXParserFactory.newInstance();

try {

// force org.xml.sax.SAXParseException for any DOCTYPE:

f.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true); //$NON-NLS-1$

} catch (Exception e) {

throw new RuntimeException(e);

}

return f;

}

The Call-Hierarchy tells me that this method is never called. Can we then remove it?

I would like to keep for other projects.

For which one?
We should not keep internal methods for other projects, because if it is not used it can happen that one day in the future somebody comes along and cleans up this class and then that other project suddenly breaks. Everything that is not API can be removed at any time. Furthermore even a method that is not called has maintanance cost, because somebody reading this class has to understand what it is doing and then wonders who's calling it and why no code in PDE does that. In that regard no code is the best code.

And at least for the Eclipse SDK (Equinox, Platform, JDT and PDE), PDE is at the end of the dependency hierarchy, so other projects are outside the SDK? Or, since you mentioned copy-pasting, do you want to add this class to other Eclipse SDK projects as it is?
Then I suggest to add it to one at the top of the hierarchy, probably equinox or eclipse(-runtime), as an internal class, add a comment that states that this is intended to be used in the entire Eclipse-SDK (Equinox, Platform, JDT and PDE, but not more) so that one knows where to search callers.
This way the code can be re-used, but we don't have to make it an API.

I struggled to find a place that both needs this methods itself (which would make sure it is not deleted because unused and not API) and is available in all other modules as well. I would highly appreciate help to find a good place, Otherwise i would copy & paste the (small) solution to other projects and keep only the test at one module.
Part of it would be needed for example at
equinox ExtensionRegistry or org.eclipse.core.internal.resources but in at least 8 distinct git repositories. I think it's very bad to share no-API across repositories.
@jonahgraham any idea?

I don't know where to put that stuff. Some of that kind of stuff lived in org.eclipse.core.runtime IIRC. But to have it at the root of the dependency tree means either A- putting it in equinox somewhere or B- making a new bundle (perhaps published to maven central)

We could also register a provider of such factories as (declarative) OSGi Service and can then query the service from any bundle in the runtime. Registering it in something like equinox.common. maybe even cascade it.

But I'm fine doing that in a later step and for now submit this as it is.

...eclipse.pde.ui.tests/src/org/eclipse/pde/core/tests/internal/PDEXmlProcessorFactoryTest.java

ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/util/PDEXmlProcessorFactory.java

...eclipse.pde.ui.tests/src/org/eclipse/pde/core/tests/internal/PDEXmlProcessorFactoryTest.java

HannesWell · 2023-07-13T05:11:54Z

...eclipse.pde.ui.tests/src/org/eclipse/pde/core/tests/internal/PDEXmlProcessorFactoryTest.java

+							}
+							assertThat(firstLine, startsWith("GET"));
+							assertThat(firstLine, not(containsString("secret")));
+							fail("Server was contacted");


Have you checked if AssertionErrors thrown by this method are considered by JUnit for a test result, i.e. can they let the test fail? My first assumption would be that they don't.
So if they are relevant for test results, they probably should be collected and for example thrown again in the Server's close method.
Alternatively, the Server could collect only the first line it has read and provide it to the test method and the assertions could be done there. This way errors would not have to be catched, collected and re-thrown.
If the assertions are actually meaning-less, they should be removed.

thats what "exceptionsInOtherThreads" below does.

class PDEXmlProcessorFactory combines classes: XmlDocumentBuilderFactory, XmlParserFactory, XmlTransformerFactory [performance] reuse single DocumentBuilderFactory instance see PDEXmlProcessorFactoryTest.main(String[]) - 100x faster

jukzi mentioned this pull request Jul 12, 2023

Factory for SAXParser parsing XML without DOCTYPE #632

Merged

jukzi force-pushed the PDEXmlProcessorFactory branch from 6c0ee20 to 685a7c6 Compare July 12, 2023 12:00

jukzi requested a review from HannesWell July 12, 2023 12:48

HannesWell reviewed Jul 12, 2023

View reviewed changes

HannesWell reviewed Jul 13, 2023

View reviewed changes

Refactor XML handling

c0c32f4

class PDEXmlProcessorFactory combines classes: XmlDocumentBuilderFactory, XmlParserFactory, XmlTransformerFactory [performance] reuse single DocumentBuilderFactory instance see PDEXmlProcessorFactoryTest.main(String[]) - 100x faster

jukzi force-pushed the PDEXmlProcessorFactory branch from 8a41eb4 to c0c32f4 Compare July 19, 2023 14:04

HannesWell approved these changes Jul 19, 2023

View reviewed changes

jukzi merged commit 7d66ed1 into eclipse-pde:master Jul 19, 2023
14 checks passed

jukzi deleted the PDEXmlProcessorFactory branch July 19, 2023 17:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor XML handling #667

Refactor XML handling #667

jukzi commented Jul 12, 2023

github-actions bot commented Jul 12, 2023 •

edited

Loading

HannesWell left a comment

HannesWell Jul 12, 2023

jukzi Jul 12, 2023 •

edited

Loading

HannesWell Jul 13, 2023

jukzi Jul 13, 2023

jonahgraham Jul 19, 2023

HannesWell Jul 19, 2023

HannesWell Jul 13, 2023

jukzi Jul 19, 2023

Refactor XML handling #667

Refactor XML handling #667

Conversation

jukzi commented Jul 12, 2023

github-actions bot commented Jul 12, 2023 • edited Loading

Test Results

HannesWell left a comment

Choose a reason for hiding this comment

HannesWell Jul 12, 2023

Choose a reason for hiding this comment

jukzi Jul 12, 2023 • edited Loading

Choose a reason for hiding this comment

HannesWell Jul 13, 2023

Choose a reason for hiding this comment

jukzi Jul 13, 2023

Choose a reason for hiding this comment

jonahgraham Jul 19, 2023

Choose a reason for hiding this comment

HannesWell Jul 19, 2023

Choose a reason for hiding this comment

HannesWell Jul 13, 2023

Choose a reason for hiding this comment

jukzi Jul 19, 2023

Choose a reason for hiding this comment

github-actions bot commented Jul 12, 2023 •

edited

Loading

jukzi Jul 12, 2023 •

edited

Loading