This is an automated email from the ASF dual-hosted git repository. ppkarwasz pushed a commit to branch feature/reduce-shade-footprint in repository https://gitbox.apache.org/repos/asf/commons-xml.git
commit 67cf7ad3de527ad083cffaed4d8ea628bbd1b3c5 Author: Piotr P. Karwasz <[email protected]> AuthorDate: Wed Jul 8 10:22:21 2026 +0200 Centralize hardening messages on HardeningException Add two static helpers to HardeningException so every hardener and every resolver floor shares one message format: - settingFailed(kind, name, target, cause) replaces JaxpSetters' inline "Failed to set ..." construction. - forbidden(...) replaces the private Resolvers.forbiddenMessage, so the five floors no longer route their message through the outer Resolvers class. Behavior-preserving: the messages are byte-identical. This is the shared core for the per-hardener shade-footprint reduction that follows. Add ShadingFootprintTest, which uses jdependency (the library maven-shade minimizeJar uses) to pin each hardener entry point's transitive class closure and print its size as a share of the full library, so later phases can show the footprint shrinking. Assisted-By: Claude Opus 4.8 <[email protected]> --- pom.xml | 9 ++ .../org/apache/commons/xml/HardeningException.java | 32 +++- .../java/org/apache/commons/xml/JaxpSetters.java | 2 +- .../java/org/apache/commons/xml/Resolvers.java | 51 +++---- .../apache/commons/xml/ShadingFootprintTest.java | 163 +++++++++++++++++++++ 5 files changed, 224 insertions(+), 33 deletions(-) diff --git a/pom.xml b/pom.xml index 5adc12f..1f7ad21 100644 --- a/pom.xml +++ b/pom.xml @@ -71,6 +71,8 @@ limitations under the License. <commons.woodstox.version>7.1.1</commons.woodstox.version> <commons.xalan.version>2.7.3</commons.xalan.version> <commons.xerces.version>2.12.2</commons.xerces.version> + <!-- Test-only: computes each hardener's shade closure, mirroring maven-shade minimizeJar, for ShadingFootprintTest. --> + <commons.jdependency.version>2.16</commons.jdependency.version> </properties> <dependencies> <!-- @@ -89,6 +91,13 @@ limitations under the License. <artifactId>junit-jupiter</artifactId> <scope>test</scope> </dependency> + <!-- Reads the compiled classes to compute each hardener's transitive class closure (the same computation maven-shade minimizeJar performs). --> + <dependency> + <groupId>org.vafer</groupId> + <artifactId>jdependency</artifactId> + <version>${commons.jdependency.version}</version> + <scope>test</scope> + </dependency> </dependencies> <build> <defaultGoal>clean checkstyle:check spotbugs:check pmd:check javadoc:javadoc verify</defaultGoal> diff --git a/src/main/java/org/apache/commons/xml/HardeningException.java b/src/main/java/org/apache/commons/xml/HardeningException.java index c549428..b980162 100644 --- a/src/main/java/org/apache/commons/xml/HardeningException.java +++ b/src/main/java/org/apache/commons/xml/HardeningException.java @@ -35,11 +35,35 @@ class HardeningException extends IllegalStateException { private static final long serialVersionUID = 1L; - HardeningException(final String message) { - super(message); - } - HardeningException(final String message, final Throwable cause) { super(message, cause); } + + /** + * Builds the standard exception for a rejected hardening setting. + * + * @param kind the kind of setting: {@code "feature"}, {@code "attribute"} or {@code "property"}. + * @param name the name of the feature, attribute or property that could not be set. + * @param target the factory, parser, validator or reader that rejected the setting; its concrete class names the offending implementation. + * @param cause the original checked or unchecked exception from the JAXP implementation. + * @return the exception to throw. + */ + static HardeningException settingFailed(final String kind, final String name, final Object target, final Throwable cause) { + return new HardeningException("Failed to set " + kind + " '" + name + "' on " + target.getClass().getName(), cause); + } + + /** + * Builds the standard "forbidden by hardening" message shared by every resolver floor. + * + * @param type the resource kind, or {@code null} if not applicable. + * @param namespace the namespace (or, for Woodstox, the entity name), or {@code null}. + * @param publicId the public identifier, or {@code null} if none. + * @param systemId the system identifier of the denied resource. + * @param baseURI the base URI for relative resolution, or {@code null}. + * @return the message describing the denied lookup. + */ + static String forbidden(final String type, final String namespace, final String publicId, final String systemId, final String baseURI) { + return String.format("External resource fetch forbidden by hardening: type=%s, namespace=%s, publicId=%s, systemId=%s, baseURI=%s", type, namespace, + publicId, systemId, baseURI); + } } diff --git a/src/main/java/org/apache/commons/xml/JaxpSetters.java b/src/main/java/org/apache/commons/xml/JaxpSetters.java index 3aa3365..471a3a2 100644 --- a/src/main/java/org/apache/commons/xml/JaxpSetters.java +++ b/src/main/java/org/apache/commons/xml/JaxpSetters.java @@ -48,7 +48,7 @@ private static void apply(final Object factory, final String kind, final String try { action.run(); } catch (final Exception e) { - throw new HardeningException("Failed to set " + kind + " '" + name + "' on " + factory.getClass().getName(), e); + throw HardeningException.settingFailed(kind, name, factory, e); } } diff --git a/src/main/java/org/apache/commons/xml/Resolvers.java b/src/main/java/org/apache/commons/xml/Resolvers.java index fc38aae..3a7d92e 100644 --- a/src/main/java/org/apache/commons/xml/Resolvers.java +++ b/src/main/java/org/apache/commons/xml/Resolvers.java @@ -74,6 +74,25 @@ static class FallbackDenyResolver extends DefaultHandler2 { */ private EntityResolver delegate; + /** + * Resolves {@code systemId} against {@code baseURI}. + * + * @param baseURI The absolute base URI to resolve against, or {@code null} if none is available. + * @param systemId The system identifier, possibly relative to {@code baseURI}. + * @return The absolutized system identifier, or {@code systemId} unchanged when it cannot or need not be resolved. + */ + private static String absolutize(final String baseURI, final String systemId) { + if (systemId == null || baseURI == null) { + return systemId; + } + try { + final URI system = new URI(systemId); + return system.isAbsolute() ? systemId : new URI(baseURI).resolve(system).toString(); + } catch (final URISyntaxException e) { + return systemId; + } + } + FallbackDenyResolver(final EntityResolver delegate) { this.delegate = delegate; } @@ -117,7 +136,7 @@ public final InputSource resolveEntity(final String name, final String publicId, */ protected InputSource onUnresolved(final String name, final String publicId, final String baseURI, final String systemId) throws SAXException, IOException { - throw new SAXException(forbiddenMessage(name, null, publicId, systemId, baseURI)); + throw new SAXException(HardeningException.forbidden(name, null, publicId, systemId, baseURI)); } private InputSource resolveWithDelegate(final String name, final String publicId, final String baseURI, @@ -161,7 +180,7 @@ public LSInput resolveResource(final String type, final String namespaceURI, fin if (resolved != null) { return resolved; } - throw new SecurityException(forbiddenMessage(type, namespaceURI, publicId, systemId, baseURI)); + throw new SecurityException(HardeningException.forbidden(type, namespaceURI, publicId, systemId, baseURI)); } } @@ -195,7 +214,7 @@ public Source resolve(final String href, final String base) throws TransformerEx if (resolved != null) { return resolved; } - throw new TransformerException(forbiddenMessage("uri", null, null, href, base)); + throw new TransformerException(HardeningException.forbidden("uri", null, null, href, base)); } } @@ -257,7 +276,7 @@ protected Object onUnresolved(final String publicID, final String systemID, fina * @return The exception to throw. */ protected final XMLStreamException denied(final String publicID, final String systemID, final String baseURI, final String namespace) { - return new XMLStreamException(forbiddenMessage(null, namespace, publicID, systemID, baseURI)); + return new XMLStreamException(HardeningException.forbidden(null, namespace, publicID, systemID, baseURI)); } } @@ -284,30 +303,6 @@ protected Object onUnresolved(final String publicID, final String systemID, fina } } - /** - * Resolves {@code systemId} against {@code baseURI}. - * - * @param baseURI The absolute base URI to resolve against, or {@code null} if none is available. - * @param systemId The system identifier, possibly relative to {@code baseURI}. - * @return The absolutized system identifier, or {@code systemId} unchanged when it cannot or need not be resolved. - */ - private static String absolutize(final String baseURI, final String systemId) { - if (systemId == null || baseURI == null) { - return systemId; - } - try { - final URI system = new URI(systemId); - return system.isAbsolute() ? systemId : new URI(baseURI).resolve(system).toString(); - } catch (final URISyntaxException e) { - return systemId; - } - } - - private static String forbiddenMessage(final String type, final String namespace, final String publicId, final String systemId, final String baseURI) { - return String.format("External resource fetch forbidden by hardening: type=%s, namespace=%s, publicId=%s, systemId=%s, baseURI=%s", type, namespace, - publicId, systemId, baseURI); - } - private Resolvers() { } } diff --git a/src/test/java/org/apache/commons/xml/ShadingFootprintTest.java b/src/test/java/org/apache/commons/xml/ShadingFootprintTest.java new file mode 100644 index 0000000..08348ec --- /dev/null +++ b/src/test/java/org/apache/commons/xml/ShadingFootprintTest.java @@ -0,0 +1,163 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.commons.xml; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +import java.io.IOException; +import java.io.UncheckedIOException; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.util.Arrays; +import java.util.Locale; +import java.util.Set; +import java.util.TreeSet; + +import org.junit.jupiter.api.AfterAll; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.Test; +import org.vafer.jdependency.Clazz; +import org.vafer.jdependency.Clazzpath; + +/** + * Guards the shade footprint: the set of classes a consumer pulls in when they shade a single hardener entry point. + * + * <p>Using {@code jdependency}, the same library {@code maven-shade-plugin}'s {@code minimizeJar} uses, this test computes each entry point's transitive class + * closure over the compiled {@code target/classes} and pins it to an expected set. It keeps the DOM, SAX and StAX hardeners from silently regaining a dependency + * on classes they should not need (for example the shared {@code JaxpSetters} or the sibling resolver floors), and records that the TrAX, XPath and schema entry + * points still pull the whole library through the {@link XmlFactories} re-hardening cycle. Update the expected sets deliberately: a change here is a change to what + * a downstream shade includes.</p> + */ +class ShadingFootprintTest { + + private static final String PKG = "org.apache.commons.xml."; + + /** Every hardener needs this shared exception (its {@code settingFailed}/{@code forbidden} message helpers). */ + private static final String CORE = "HardeningException"; + + private static final Set<String> DOCUMENT_BUILDER_HARDENER = set( + "DocumentBuilderHardener", "HardeningDocumentBuilder", "HardeningDocumentBuilderFactory", CORE, + "JaxpSetters", "JaxpSetters$ThrowingAction", + "Resolvers", "Resolvers$FallbackDenyResolver", "Resolvers$FallbackDenyLSResourceResolver", "Resolvers$FallbackDenyURIResolver", + "Resolvers$FallbackDenyXMLResolver", "Resolvers$FallbackIgnoreXMLResolver"); + + private static final Set<String> SAX_PARSER_HARDENER = set( + "SAXParserHardener", "SAXParserHardener$DtdAwareDenyResolver", "SAXParserHardener$HardeningExpatXMLReader", + "HardeningSAXParser", "HardeningSAXParserFactory", "HardeningXMLReader", CORE, + "JaxpSetters", "JaxpSetters$ThrowingAction", + "Resolvers", "Resolvers$FallbackDenyResolver", "Resolvers$FallbackDenyLSResourceResolver", "Resolvers$FallbackDenyURIResolver", + "Resolvers$FallbackDenyXMLResolver", "Resolvers$FallbackIgnoreXMLResolver"); + + private static final Set<String> STAX_HARDENER = set( + "StaxHardener", "StaxHardener$DtdSubsetFloor", "HardeningXMLInputFactory", CORE, + "JaxpSetters", "JaxpSetters$ThrowingAction", + "Resolvers", "Resolvers$FallbackDenyResolver", "Resolvers$FallbackDenyLSResourceResolver", "Resolvers$FallbackDenyURIResolver", + "Resolvers$FallbackDenyXMLResolver", "Resolvers$FallbackIgnoreXMLResolver"); + + /** The TrAX/XPath/schema entry points all pull the whole library through {@link XmlFactories}; this is its class count (Phase 4 territory to reduce). */ + private static final int WHOLE_LIBRARY_SIZE = 35; + + /** Entry points reported by the {@link #reportFootprint()} diagnostic, most-focused first, ending with the whole library. */ + private static final String[] REPORTED = { + "DocumentBuilderHardener", "SAXParserHardener", "StaxHardener", "TransformerHardener", "XPathHardener", "HardeningSchemaFactory", "XmlFactories"}; + + private static Clazzpath clazzpath; + private static Path classesDir; + + @BeforeAll + static void indexCompiledClasses() throws Exception { + classesDir = Paths.get(HardeningException.class.getProtectionDomain().getCodeSource().getLocation().toURI()); + clazzpath = new Clazzpath(); + clazzpath.addClazzpathUnit(classesDir); + } + + /** Prints each entry point's shade closure size (uncompressed {@code .class} bytes) and its share of the full library, to track the footprint over the refactor. */ + @AfterAll + static void reportFootprint() { + final long library = bytesOf(closureOf("XmlFactories")); + final StringBuilder report = new StringBuilder("\nShade footprint (uncompressed .class bytes, % of full library):\n"); + for (final String entry : REPORTED) { + final Set<String> closure = closureOf(entry); + final long bytes = bytesOf(closure); + report.append(String.format(Locale.ROOT, " %-24s %2d classes %7d bytes %5.1f%%%n", entry, closure.size(), bytes, 100.0 * bytes / library)); + } + System.out.print(report); + } + + @Test + void documentBuilderHardenerFootprint() { + assertEquals(DOCUMENT_BUILDER_HARDENER, closureOf("DocumentBuilderHardener")); + } + + @Test + void saxParserHardenerFootprint() { + assertEquals(SAX_PARSER_HARDENER, closureOf("SAXParserHardener")); + } + + @Test + void staxHardenerFootprint() { + assertEquals(STAX_HARDENER, closureOf("StaxHardener")); + } + + @Test + void traxXPathAndSchemaPullTheWholeLibrary() { + final Set<String> whole = closureOf("XmlFactories"); + assertEquals(WHOLE_LIBRARY_SIZE, whole.size(), "XmlFactories closure size drifted: " + whole); + assertEquals(whole, closureOf("TransformerHardener"), "TransformerHardener no longer pulls exactly the whole library"); + assertEquals(whole, closureOf("XPathHardener"), "XPathHardener no longer pulls exactly the whole library"); + assertEquals(whole, closureOf("HardeningSchemaFactory"), "HardeningSchemaFactory no longer pulls exactly the whole library"); + } + + /** Transitive class closure of {@code PKG + simpleName}, restricted to this library's own package and reported by simple name. */ + private static Set<String> closureOf(final String simpleName) { + final Clazz entry = clazzpath.getClazz(PKG + simpleName); + if (entry == null) { + throw new IllegalStateException("Not on the compiled classpath: " + PKG + simpleName); + } + final Set<String> names = new TreeSet<>(); + names.add(strip(entry.getName())); + for (final Clazz dependency : entry.getTransitiveDependencies()) { + if (dependency.getName().startsWith(PKG)) { + names.add(strip(dependency.getName())); + } + } + return names; + } + + /** Sums the uncompressed {@code .class} file sizes of a closure's classes, as they would land in a shaded jar. */ + private static long bytesOf(final Set<String> simpleNames) { + long total = 0; + for (final String name : simpleNames) { + try { + total += Files.size(classesDir.resolve("org/apache/commons/xml/" + name + ".class")); + } catch (final IOException e) { + throw new UncheckedIOException(e); + } + } + return total; + } + + private static String strip(final String qualifiedName) { + return qualifiedName.substring(PKG.length()); + } + + private static Set<String> set(final String... names) { + return new TreeSet<>(Arrays.asList(names)); + } +}
