SimpleXsdSchema is not thread-safe

Before this commit, SimpleXsdSchema had a reference to
an instance of `org.w3c.dom.Element`, which is not thread safe.
This causes issues when multiple clients are requesting
the schema file simultaneously.

This commit updates SimpleXsdSchema to always create
a new `ResourceSource` from an XSD Resource whenever getSource is requested.

Signed-off-by: Dmytro Nosan <[email protected]>

Author: nosan

spring-xml/src/main/java/org/springframework/xml/xsd/SimpleXsdSchema.java

@@ -23,7 +23,6 @@
 import javax.xml.parsers.DocumentBuilderFactory;
 import javax.xml.parsers.ParserConfigurationException;
 import javax.xml.transform.Source;
-import javax.xml.transform.dom.DOMSource;
 
 import org.w3c.dom.Document;
 import org.w3c.dom.Element;
@@ -34,6 +33,7 @@
 import org.springframework.util.Assert;
 import org.springframework.xml.DocumentBuilderFactoryUtils;
 import org.springframework.xml.sax.SaxUtils;
+import org.springframework.xml.transform.ResourceSource;
 import org.springframework.xml.validation.XmlValidator;
 import org.springframework.xml.validation.XmlValidatorFactory;
 
@@ -50,15 +50,15 @@
  */
 public class SimpleXsdSchema implements XsdSchema, InitializingBean {
 
-	private static DocumentBuilderFactory documentBuilderFactory = DocumentBuilderFactoryUtils.newInstance();
+	private static final DocumentBuilderFactory documentBuilderFactory = DocumentBuilderFactoryUtils.newInstance();
 
 	private static final String SCHEMA_NAMESPACE = "http://www.w3.org/2001/XMLSchema";
 
 	private static final QName SCHEMA_NAME = new QName(SCHEMA_NAMESPACE, "schema", "xsd");
 
 	private Resource xsdResource;
 
-	private Element schemaElement;
+	private String targetNamespace;
 
 	static {
 		documentBuilderFactory.setNamespaceAware(true);
@@ -95,20 +95,25 @@ public void setXsd(Resource xsdResource) {
 
 	@Override
 	public String getTargetNamespace() {
-
-		Assert.notNull(this.schemaElement,
-				"schemaElement must not be null! Did you run afterPropertiesSet() or register this as a Spring bean?");
-
-		return this.schemaElement.getAttribute("targetNamespace");
+		Assert.state(this.targetNamespace != null,
+				() -> "'targetNamespace' cannot be accessed before afterPropertiesSet() has been called on this instance");
+		return this.targetNamespace;
 	}
 
 	@Override
 	public Source getSource() {
-		return new DOMSource(this.schemaElement);
+		Assert.state(this.xsdResource != null, () -> "Source cannot be created from a null XSD resource");
+		try {
+			return new ResourceSource(this.xsdResource);
+		}
+		catch (IOException ex) {
+			throw new XsdSchemaException(ex.getMessage(), ex);
+		}
 	}
 
 	@Override
 	public XmlValidator createValidator() {
+		Assert.state(this.xsdResource != null, () -> "XmlValidator cannot be created from a null XSD resource");
 		try {
 			return XmlValidatorFactory.createValidator(this.xsdResource, XmlValidatorFactory.SCHEMA_W3C_XML);
 		}
@@ -127,13 +132,15 @@ public void afterPropertiesSet() throws ParserConfigurationException, IOExceptio
 
 	private void loadSchema(DocumentBuilder documentBuilder) throws SAXException, IOException {
 		Document schemaDocument = documentBuilder.parse(SaxUtils.createInputSource(this.xsdResource));
-		this.schemaElement = schemaDocument.getDocumentElement();
-		Assert.isTrue(SCHEMA_NAME.getLocalPart().equals(this.schemaElement.getLocalName()), this.xsdResource
-				+ " has invalid root element : [" + this.schemaElement.getLocalName() + "] instead of [schema]");
-		Assert.isTrue(SCHEMA_NAME.getNamespaceURI().equals(this.schemaElement.getNamespaceURI()),
-				this.xsdResource + " has invalid root element: [" + this.schemaElement.getNamespaceURI()
-						+ "] instead of [" + SCHEMA_NAME.getNamespaceURI() + "]");
-		Assert.hasText(getTargetNamespace(), this.xsdResource + " has no targetNamespace");
+		Element schemaElement = schemaDocument.getDocumentElement();
+		Assert.isTrue(SCHEMA_NAME.getLocalPart().equals(schemaElement.getLocalName()), this.xsdResource
+				+ " has invalid root element : [" + schemaElement.getLocalName() + "] instead of [schema]");
+		Assert.isTrue(SCHEMA_NAME.getNamespaceURI().equals(schemaElement.getNamespaceURI()),
+				this.xsdResource + " has invalid root element: [" + schemaElement.getNamespaceURI() + "] instead of ["
+						+ SCHEMA_NAME.getNamespaceURI() + "]");
+		String targetNamespace = schemaElement.getAttribute("targetNamespace");
+		Assert.hasText(targetNamespace, this.xsdResource + " has no targetNamespace");
+		this.targetNamespace = targetNamespace;
 	}
 
 	public String toString() {

spring-xml/src/test/java/org/springframework/xml/xsd/AbstractXsdSchemaTests.java

@@ -16,13 +16,17 @@
 
 package org.springframework.xml.xsd;
 
-import javax.xml.parsers.DocumentBuilder;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+
 import javax.xml.parsers.DocumentBuilderFactory;
-import javax.xml.transform.Transformer;
 import javax.xml.transform.TransformerFactory;
 import javax.xml.transform.dom.DOMResult;
 
-import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 import org.w3c.dom.Document;
 import org.xmlunit.assertj.XmlAssert;
@@ -38,20 +42,6 @@
 
 public abstract class AbstractXsdSchemaTests {
 
-	private DocumentBuilder documentBuilder;
-
-	protected Transformer transformer;
-
-	@BeforeEach
-	public final void setUp() throws Exception {
-
-		DocumentBuilderFactory documentBuilderFactory = DocumentBuilderFactoryUtils.newInstance();
-		documentBuilderFactory.setNamespaceAware(true);
-		this.documentBuilder = documentBuilderFactory.newDocumentBuilder();
-		TransformerFactory transformerFactory = TransformerFactoryUtils.newInstance();
-		this.transformer = transformerFactory.newTransformer();
-	}
-
 	@Test
 	void testSingle() throws Exception {
 
@@ -61,12 +51,10 @@ void testSingle() throws Exception {
 		assertThat(single.getTargetNamespace()).isEqualTo("http://www.springframework.org/spring-ws/single/schema");
 
 		resource = new ClassPathResource("single.xsd", AbstractXsdSchemaTests.class);
-		Document expected = this.documentBuilder.parse(SaxUtils.createInputSource(resource));
-		DOMResult domResult = new DOMResult();
-		this.transformer.transform(single.getSource(), domResult);
-		Document result = (Document) domResult.getNode();
+		Document expected = createDocument(resource);
+		Document actual = createDocument(single);
 
-		XmlAssert.assertThat(result).and(expected).ignoreWhitespace().areIdentical();
+		XmlAssert.assertThat(actual).and(expected).ignoreWhitespace().areIdentical();
 	}
 
 	@Test
@@ -78,12 +66,10 @@ void testIncludes() throws Exception {
 		assertThat(including.getTargetNamespace()).isEqualTo("http://www.springframework.org/spring-ws/include/schema");
 
 		resource = new ClassPathResource("including.xsd", AbstractXsdSchemaTests.class);
-		Document expected = this.documentBuilder.parse(SaxUtils.createInputSource(resource));
-		DOMResult domResult = new DOMResult();
-		this.transformer.transform(including.getSource(), domResult);
-		Document result = (Document) domResult.getNode();
+		Document expected = createDocument(resource);
+		Document actual = createDocument(including);
 
-		XmlAssert.assertThat(result).and(expected).ignoreWhitespace().areIdentical();
+		XmlAssert.assertThat(actual).and(expected).ignoreWhitespace().areIdentical();
 	}
 
 	@Test
@@ -96,12 +82,10 @@ void testImports() throws Exception {
 			.isEqualTo("http://www.springframework.org/spring-ws/importing/schema");
 
 		resource = new ClassPathResource("importing.xsd", AbstractXsdSchemaTests.class);
-		Document expected = this.documentBuilder.parse(SaxUtils.createInputSource(resource));
-		DOMResult domResult = new DOMResult();
-		this.transformer.transform(importing.getSource(), domResult);
-		Document result = (Document) domResult.getNode();
+		Document expected = createDocument(resource);
+		Document actual = createDocument(importing);
 
-		XmlAssert.assertThat(result).and(expected).ignoreWhitespace().areIdentical();
+		XmlAssert.assertThat(actual).and(expected).ignoreWhitespace().areIdentical();
 	}
 
 	@Test
@@ -113,24 +97,63 @@ void testXmlNamespace() throws Exception {
 		assertThat(importing.getTargetNamespace()).isEqualTo("http://www.springframework.org/spring-ws/xmlNamespace");
 
 		resource = new ClassPathResource("xmlNamespace.xsd", AbstractXsdSchemaTests.class);
-		Document expected = this.documentBuilder.parse(SaxUtils.createInputSource(resource));
-		DOMResult domResult = new DOMResult();
-		this.transformer.transform(importing.getSource(), domResult);
-		Document result = (Document) domResult.getNode();
+		Document expected = createDocument(resource);
+		Document actual = createDocument(importing);
 
-		XmlAssert.assertThat(result).and(expected).ignoreWhitespace().areIdentical();
+		XmlAssert.assertThat(actual).and(expected).ignoreWhitespace().areIdentical();
 	}
 
 	@Test
 	void testCreateValidator() throws Exception {
-
 		Resource resource = new ClassPathResource("single.xsd", AbstractXsdSchemaTests.class);
 		XsdSchema single = createSchema(resource);
 		XmlValidator validator = single.createValidator();
 
 		assertThat(validator).isNotNull();
 	}
 
+	@Test
+	void testLoadXsdSchemaConcurrently() throws Exception {
+		ClassPathResource xsdResource = new ClassPathResource("single.xsd", AbstractXsdSchemaTests.class);
+		XsdSchema xsdSchema = createSchema(xsdResource);
+		CountDownLatch signal = new CountDownLatch(1);
+		CountDownLatch threadSignal = new CountDownLatch(4);
+		ExecutorService executorService = Executors.newFixedThreadPool(4);
+		try {
+			List<Future<Document>> futures = new ArrayList<>();
+			for (int i = 0; i < 4; i++) {
+				futures.add(executorService.submit(() -> {
+					threadSignal.countDown();
+					signal.await();
+					return createDocument(xsdSchema);
+				}));
+			}
+			threadSignal.await();
+			signal.countDown();
+			for (Future<Document> future : futures) {
+				Document actual = future.get();
+				Document expected = createDocument(xsdResource);
+				XmlAssert.assertThat(actual).and(expected).ignoreWhitespace().areIdentical();
+			}
+		}
+		finally {
+			executorService.shutdownNow();
+		}
+	}
+
 	protected abstract XsdSchema createSchema(Resource resource) throws Exception;
 
+	protected Document createDocument(Resource resource) throws Exception {
+		DocumentBuilderFactory documentBuilderFactory = DocumentBuilderFactoryUtils.newInstance();
+		documentBuilderFactory.setNamespaceAware(true);
+		return documentBuilderFactory.newDocumentBuilder().parse(SaxUtils.createInputSource(resource));
+	}
+
+	private Document createDocument(XsdSchema schema) throws Exception {
+		DOMResult domResult = new DOMResult();
+		TransformerFactory transformerFactory = TransformerFactoryUtils.newInstance();
+		transformerFactory.newTransformer().transform(schema.getSource(), domResult);
+		return (Document) domResult.getNode();
+	}
+
 }

spring-xml/src/test/java/org/springframework/xml/xsd/SimpleXsdSchemaTests.java

@@ -20,7 +20,7 @@
 
 import org.springframework.core.io.Resource;
 
-import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException;
+import static org.assertj.core.api.Assertions.assertThatIllegalStateException;
 
 class SimpleXsdSchemaTests extends AbstractXsdSchemaTests {
 
@@ -35,7 +35,7 @@ protected XsdSchema createSchema(Resource resource) throws Exception {
 
 	@Test
 	void testBareXsdSchema() {
-		assertThatIllegalArgumentException().isThrownBy(() -> new SimpleXsdSchema().toString());
+		assertThatIllegalStateException().isThrownBy(() -> new SimpleXsdSchema().toString());
 	}
 
 }