dsmiley commented on code in PR #962:
URL: https://github.com/apache/solr/pull/962#discussion_r958450880
##########
solr/core/src/java/org/apache/solr/core/XmlConfigFile.java:
##########
@@ -59,60 +51,18 @@
*/
public class XmlConfigFile { // formerly simply "Config"
private static final Logger log =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
- private static final XMLErrorLogger xmllog = new XMLErrorLogger(log);
static final XPathFactory xpathFactory = XPathFactory.newInstance();
private final Document doc;
- private final Document origDoc; // with unsubstituted properties
private final String prefix;
private final String name;
private final SolrResourceLoader loader;
private final Properties substituteProperties;
private int zkVersion = -1;
- /** Builds a config from a resource name with no xpath prefix. Does no
property substitution. */
- public XmlConfigFile(SolrResourceLoader loader, String name)
- throws ParserConfigurationException, IOException, SAXException {
- this(loader, name, null, null);
- }
-
- /** Builds a config. Does no property substitution. */
- public XmlConfigFile(SolrResourceLoader loader, String name, InputSource is,
String prefix)
- throws ParserConfigurationException, IOException, SAXException {
- this(loader, name, is, prefix, null);
- }
-
- public XmlConfigFile(
- SolrResourceLoader loader,
- String name,
- InputSource is,
- String prefix,
- Properties substituteProps)
- throws ParserConfigurationException, IOException, SAXException {
- this(
- loader,
- s -> {
- try {
- return loader.openResource(s);
- } catch (IOException e) {
- throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, e);
- }
- },
- name,
- is,
- prefix,
- substituteProps);
- }
-
/**
- * Builds a config:
- *
- * <p>Note that the 'name' parameter is used to obtain a valid input stream
if no valid one is
- * provided through 'is'. If no valid stream is provided, a valid
SolrResourceLoader instance
- * should be provided through 'loader' so the resource can be opened (@see
- * SolrResourceLoader#openResource); if no SolrResourceLoader instance is
provided, a default one
- * will be created.
Review Comment:
These docs should be brought back.
##########
solr/core/src/java/org/apache/solr/util/SafeXMLParsing.java:
##########
@@ -51,36 +51,51 @@ public final class SafeXMLParsing {
private SafeXMLParsing() {}
/**
- * Parses a config file from ResourceLoader. Xinclude and external entities
are enabled, but
- * cannot escape the resource loader.
+ * Parses a config file from a Solr config based on ResourceLoader. Xinclude
and external entities
+ * are enabled, but cannot escape the resource loader.
*/
public static Document parseConfigXML(Logger log, ResourceLoader loader,
String file)
throws SAXException, IOException {
try (InputStream in = loader.openResource(file)) {
+ DocumentBuilder db = configDocumentBuilder(loader, log);
+ return db.parse(in,
SystemIdResolver.createSystemIdFromResourceName(file));
+ }
+ }
+
+ /**
+ * Parses a config file from a Solr config based on InputSource. Xinclude
and external entities
+ * are enabled, but cannot escape the resource loader.
+ */
+ public static Document parseConfigXML(Logger log, InputSource is,
ResourceLoader loader)
+ throws SAXException, IOException {
+ return configDocumentBuilder(loader, log).parse(is);
+ }
+
+ private static DocumentBuilder configDocumentBuilder(ResourceLoader loader,
Logger log) {
+ try {
+ if (null == loader) throw new NullPointerException("loader");
Review Comment:
Instead use `Objects.requireNonNull`
##########
solr/core/src/java/org/apache/solr/core/XmlConfigFile.java:
##########
@@ -125,59 +75,30 @@ public XmlConfigFile(
*/
public XmlConfigFile(
SolrResourceLoader loader,
- Function<String, InputStream> fileSupplier,
String name,
InputSource is,
String prefix,
Properties substituteProps)
throws IOException {
if (null == loader) throw new NullPointerException("loader");
this.loader = loader;
-
this.substituteProperties = substituteProps;
this.name = name;
this.prefix = (prefix != null && !prefix.endsWith("/")) ? prefix + '/' :
prefix;
- try {
- javax.xml.parsers.DocumentBuilderFactory dbf =
DocumentBuilderFactory.newInstance();
-
- if (is == null) {
- InputStream in = fileSupplier.apply(name);
- if (in instanceof ZkSolrResourceLoader.ZkByteArrayInputStream) {
- zkVersion = ((ZkSolrResourceLoader.ZkByteArrayInputStream)
in).getStat().getVersion();
- log.debug("loaded config {} with version {} ", name, zkVersion);
- }
- is = new InputSource(in);
- is.setSystemId(SystemIdResolver.createSystemIdFromResourceName(name));
- }
- // only enable xinclude, if a SystemId is available
- if (is.getSystemId() != null) {
- try {
- dbf.setXIncludeAware(true);
- dbf.setNamespaceAware(true);
- } catch (UnsupportedOperationException e) {
- log.warn("{} XML parser doesn't support XInclude option", name);
- }
- }
-
- final DocumentBuilder db = dbf.newDocumentBuilder();
- db.setEntityResolver(new SystemIdResolver(loader));
- db.setErrorHandler(xmllog);
- try {
- doc = db.parse(is);
- origDoc = doc;
- } finally {
- // some XML parsers are broken and don't close the byte stream (but
they should according to
- // spec)
- IOUtils.closeQuietly(is.getByteStream());
- }
- if (substituteProps != null) {
- DOMUtil.substituteProperties(doc, getSubstituteProperties());
+ try {
+ if (is != null) {
+ doc = SafeXMLParsing.parseConfigXML(log, is, loader);
+ } else {
+ doc = SafeXMLParsing.parseConfigXML(log, loader, name);
Review Comment:
Notice how the parameter order between the method you added and the existing
one is kind of jumbled. Please align; loader before what it is to be loaded..
##########
solr/core/src/java/org/apache/solr/util/SafeXMLParsing.java:
##########
@@ -51,36 +51,51 @@ public final class SafeXMLParsing {
private SafeXMLParsing() {}
/**
- * Parses a config file from ResourceLoader. Xinclude and external entities
are enabled, but
- * cannot escape the resource loader.
+ * Parses a config file from a Solr config based on ResourceLoader. Xinclude
and external entities
+ * are enabled, but cannot escape the resource loader.
*/
public static Document parseConfigXML(Logger log, ResourceLoader loader,
String file)
throws SAXException, IOException {
try (InputStream in = loader.openResource(file)) {
+ DocumentBuilder db = configDocumentBuilder(loader, log);
+ return db.parse(in,
SystemIdResolver.createSystemIdFromResourceName(file));
+ }
+ }
+
+ /**
+ * Parses a config file from a Solr config based on InputSource. Xinclude
and external entities
+ * are enabled, but cannot escape the resource loader.
+ */
+ public static Document parseConfigXML(Logger log, InputSource is,
ResourceLoader loader)
+ throws SAXException, IOException {
+ return configDocumentBuilder(loader, log).parse(is);
+ }
+
+ private static DocumentBuilder configDocumentBuilder(ResourceLoader loader,
Logger log) {
+ try {
+ if (null == loader) throw new NullPointerException("loader");
final DocumentBuilderFactory dbf = DocumentBuilderFactory.newInstance();
- dbf.setValidating(false);
dbf.setNamespaceAware(true);
trySetDOMFeature(dbf, XMLConstants.FEATURE_SECURE_PROCESSING, true);
- try {
- dbf.setXIncludeAware(true);
- } catch (UnsupportedOperationException e) {
- throw new SolrException(
- SolrException.ErrorCode.BAD_REQUEST, "XML parser doesn't support
XInclude option", e);
- }
-
+ // only enable xinclude, if systemId is available. this assumes it is
the case as loader
+ // provides one.
+ dbf.setXIncludeAware(true);
Review Comment:
I like how before it was clear that setXIncludeAware might throw an
UnsupportedOperationException. Now it isn't. You could ameliorate that simply
by adding `// may throw UnsupportedOperationException` at the end.
I suppose it's extremely unlikely such an exception would be thrown by other
stuff in the broader try-finally. I've never seen this any way so it's a minor
matter.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
