Skip to content

Commit 55ebcf9

Browse files
committed
fix: implement Copilot PR review suggestions
- Fix test assertion for remove_namespaces feature - Use StandardCharsets.UTF_8 instead of string literal - Replace string reference comparison with isEmpty() - Move regex pattern to static final field for performance
1 parent 0ec3e67 commit 55ebcf9

File tree

2 files changed

+14
-10
lines changed

2 files changed

+14
-10
lines changed

modules/ingest-common/src/main/java/org/elasticsearch/ingest/common/XmlProcessor.java

Lines changed: 12 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -15,12 +15,14 @@
1515
import org.elasticsearch.ingest.IngestDocument;
1616
import org.elasticsearch.ingest.Processor;
1717

18+
import java.nio.charset.StandardCharsets;
1819
import java.util.ArrayList;
1920
import java.util.HashMap;
2021
import java.util.Iterator;
2122
import java.util.List;
2223
import java.util.Locale;
2324
import java.util.Map;
25+
import java.util.regex.Pattern;
2426

2527
import javax.xml.namespace.NamespaceContext;
2628
import javax.xml.parsers.DocumentBuilder;
@@ -52,6 +54,9 @@ public final class XmlProcessor extends AbstractProcessor {
5254

5355
private static final XPathFactory XPATH_FACTORY = XPathFactory.newInstance();
5456

57+
// Pre-compiled pattern to detect namespace prefixes
58+
private static final Pattern NAMESPACE_PATTERN = Pattern.compile(".*\\b[a-zA-Z][a-zA-Z0-9_-]*:[a-zA-Z][a-zA-Z0-9_-]*.*");
59+
5560
// Pre-configured SAX parser factories for secure XML parsing
5661
private static final javax.xml.parsers.SAXParserFactory SAX_PARSER_FACTORY = createSecureSaxParserFactory();
5762
private static final javax.xml.parsers.SAXParserFactory SAX_PARSER_FACTORY_NS = createSecureSaxParserFactoryNamespaceAware();
@@ -383,16 +388,14 @@ public Iterator<String> getPrefixes(String namespaceURI) {
383388
});
384389
}
385390

386-
// Pre-compiled pattern to detect namespace prefixes
387-
java.util.regex.Pattern namespacePattern =
388-
java.util.regex.Pattern.compile(".*\\b[a-zA-Z][a-zA-Z0-9_-]*:[a-zA-Z][a-zA-Z0-9_-]*.*");
391+
// Use pre-compiled pattern to detect namespace prefixes
389392

390393
for (Map.Entry<String, String> entry : xpathExpressions.entrySet()) {
391394
String xpathExpression = entry.getKey();
392395
String targetFieldName = entry.getValue();
393396

394397
// Validate namespace prefixes if no namespaces are configured
395-
if (!hasNamespaces && namespacePattern.matcher(xpathExpression).matches()) {
398+
if (!hasNamespaces && NAMESPACE_PATTERN.matcher(xpathExpression).matches()) {
396399
throw new IllegalArgumentException(
397400
"Invalid XPath expression [" + xpathExpression + "]: contains namespace prefixes but no namespace configuration provided"
398401
);
@@ -476,7 +479,7 @@ public XmlProcessor create(
476479

477480
// Parse parse_options parameter
478481
String parseOptions = ConfigurationUtils.readStringProperty(TYPE, processorTag, config, "parse_options", "");
479-
if (parseOptions != null && parseOptions != "" && !"strict".equals(parseOptions)) {
482+
if (parseOptions != null && !parseOptions.isEmpty() && !"strict".equals(parseOptions)) {
480483
throw new IllegalArgumentException("Invalid parse_options [" + parseOptions + "]. Only 'strict' is supported.");
481484
}
482485

@@ -528,7 +531,7 @@ public void fatalError(org.xml.sax.SAXParseException exception) throws org.xml.s
528531
// Use enhanced handler that can build DOM during streaming when needed
529532
XmlStreamingWithDomHandler handler = new XmlStreamingWithDomHandler(needsDom);
530533

531-
parser.parse(new java.io.ByteArrayInputStream(xmlString.getBytes("UTF-8")), handler);
534+
parser.parse(new java.io.ByteArrayInputStream(xmlString.getBytes(StandardCharsets.UTF_8)), handler);
532535

533536
// Store structured result if needed
534537
if (storeXml) {
@@ -926,10 +929,11 @@ private static DocumentBuilderFactory createSecureDocumentBuilderFactory() {
926929
* @return the appropriate SAX parser factory for the current configuration
927930
*/
928931
private javax.xml.parsers.SAXParserFactory selectSaxParserFactory() {
932+
boolean needsNamespaceAware = hasNamespaces() || removeNamespaces;
929933
if (isStrict()) {
930-
return hasNamespaces() ? SAX_PARSER_FACTORY_NS_STRICT : SAX_PARSER_FACTORY_STRICT;
934+
return needsNamespaceAware ? SAX_PARSER_FACTORY_NS_STRICT : SAX_PARSER_FACTORY_STRICT;
931935
} else {
932-
return hasNamespaces() ? SAX_PARSER_FACTORY_NS : SAX_PARSER_FACTORY;
936+
return needsNamespaceAware ? SAX_PARSER_FACTORY_NS : SAX_PARSER_FACTORY;
933937
}
934938
}
935939
}

modules/ingest-common/src/test/java/org/elasticsearch/ingest/common/XmlProcessorTests.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -583,8 +583,8 @@ public void testRemoveNamespaces() {
583583
Map<String, Object> data = ingestDocument.getFieldValue(TARGET_FIELD, Map.class);
584584
Map<String, Object> foo = (Map<String, Object>) data.get("foo");
585585

586-
assertTrue("Element with namespace should be present", foo.containsKey("ns:bar"));
587-
assertThat(foo.get("ns:bar"), equalTo("value"));
586+
assertTrue("Element without namespace should be present", foo.containsKey("bar"));
587+
assertThat(foo.get("bar"), equalTo("value"));
588588

589589
// Now test with removeNamespaces=false
590590
IngestDocument ingestDocument2 = createTestIngestDocument(xml);

0 commit comments

Comments
 (0)