Skip to content

SimpleXsdSchema#loadSchema is not thread-safe #1556

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

SimpleXsdSchema#loadSchema is not thread-safe #1556

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 23 additions & 16 deletions spring-xml/src/main/java/org/springframework/xml/xsd/SimpleXsdSchema.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;

Expand All @@ -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);
Expand Down Expand Up @@ -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);
}
Expand All @@ -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));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am unsure if DOM is the best way to parse XML to find the targetNamespace. I believe StAX might be a better choice here to avoid loading the entire XML document into memory.

@Override
	public void afterPropertiesSet() throws Exception {
		Assert.notNull(this.xsdResource, "'xsd' is required");
		Assert.isTrue(this.xsdResource.exists(), "xsd '" + this.xsdResource + "' does not exist");
		XmlRootElement element = XmlRootElement.of(this.xsdResource);
		Assert.isTrue(SCHEMA_NAME.getLocalPart().equals(element.localName()),
				this.xsdResource + " has invalid root element : [" + element.localName() + "] instead of [schema]");
		Assert.isTrue(SCHEMA_NAME.getNamespaceURI().equals(element.namespaceURI()),
				this.xsdResource + " has invalid root element: [" + element.namespaceURI() + "] instead of ["
						+ SCHEMA_NAME.getNamespaceURI() + "]");
		Assert.hasText(element.targetNamespace(), this.xsdResource + " has no targetNamespace");
		this.targetNamespace = element.targetNamespace();
	}


	private record XmlRootElement(String localName, String namespaceURI,
								  String targetNamespace) {
		static XmlRootElement of(Resource resource) throws Exception {
			XMLInputFactory factory = StaxUtils.createDefensiveInputFactory();
			factory.setProperty(XMLInputFactory.IS_NAMESPACE_AWARE, true);
			try (InputStream is = resource.getInputStream()) {
				XMLStreamReader reader = factory.createXMLStreamReader(SaxUtils.getSystemId(resource), is);
				try {
					while (reader.hasNext()) {
						if (reader.next() == XMLStreamConstants.START_ELEMENT) {
							return new XmlRootElement(reader.getLocalName(), reader.getNamespaceURI(),
									reader.getAttributeValue(null, "targetNamespace"));
						}
					}
				}
				finally {
					reader.close();
				}
			}
			return new XmlRootElement(null, null, null);
		}
	}

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() {
Expand Down
150 changes: 76 additions & 74 deletions spring-xml/src/test/java/org/springframework/xml/xsd/AbstractXsdSchemaTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -38,99 +42,97 @@

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 {

Resource resource = new ClassPathResource("single.xsd", AbstractXsdSchemaTests.class);
XsdSchema single = createSchema(resource);

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();

XmlAssert.assertThat(result).and(expected).ignoreWhitespace().areIdentical();
Resource xsdResource = new ClassPathResource("single.xsd", AbstractXsdSchemaTests.class);
XsdSchema xsdSchema = createSchema(xsdResource);
assertThat(xsdSchema.getTargetNamespace()).isEqualTo("http://www.springframework.org/spring-ws/single/schema");
Document actual = createDocument(xsdSchema);
Document expected = createDocument(xsdResource);
XmlAssert.assertThat(actual).and(expected).ignoreWhitespace().areIdentical();
}

@Test
void testIncludes() throws Exception {

Resource resource = new ClassPathResource("including.xsd", AbstractXsdSchemaTests.class);
XsdSchema including = createSchema(resource);

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();

XmlAssert.assertThat(result).and(expected).ignoreWhitespace().areIdentical();
Resource xsdResource = new ClassPathResource("including.xsd", AbstractXsdSchemaTests.class);
XsdSchema xsdSchema = createSchema(xsdResource);
assertThat(xsdSchema.getTargetNamespace()).isEqualTo("http://www.springframework.org/spring-ws/include/schema");
Document expected = createDocument(xsdResource);
Document actual = createDocument(xsdSchema);
XmlAssert.assertThat(actual).and(expected).ignoreWhitespace().areIdentical();
}

@Test
void testImports() throws Exception {

Resource resource = new ClassPathResource("importing.xsd", AbstractXsdSchemaTests.class);
XsdSchema importing = createSchema(resource);

assertThat(importing.getTargetNamespace())
Resource xsdResource = new ClassPathResource("importing.xsd", AbstractXsdSchemaTests.class);
XsdSchema xsdSchema = createSchema(xsdResource);
assertThat(xsdSchema.getTargetNamespace())
.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();

XmlAssert.assertThat(result).and(expected).ignoreWhitespace().areIdentical();
Document expected = createDocument(xsdResource);
Document actual = createDocument(xsdSchema);
XmlAssert.assertThat(actual).and(expected).ignoreWhitespace().areIdentical();
}

@Test
void testXmlNamespace() throws Exception {

Resource resource = new ClassPathResource("xmlNamespace.xsd", AbstractXsdSchemaTests.class);
XsdSchema importing = createSchema(resource);

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();

XmlAssert.assertThat(result).and(expected).ignoreWhitespace().areIdentical();
Resource xsdResource = new ClassPathResource("xmlNamespace.xsd", AbstractXsdSchemaTests.class);
XsdSchema xsdSchema = createSchema(xsdResource);
assertThat(xsdSchema.getTargetNamespace()).isEqualTo("http://www.springframework.org/spring-ws/xmlNamespace");
Document expected = createDocument(xsdResource);
Document actual = createDocument(xsdSchema);
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();

Resource xsdResource = new ClassPathResource("single.xsd", AbstractXsdSchemaTests.class);
XsdSchema xsdSchema = createSchema(xsdResource);
XmlValidator validator = xsdSchema.createValidator();
assertThat(validator).isNotNull();
}

@Test
void testLoadXsdSchemaConcurrently() throws Exception {
ClassPathResource xsdResource = new ClassPathResource("single.xsd", AbstractXsdSchemaTests.class);
XsdSchema xsdSchema = createSchema(xsdResource);
int numberOfThreads = 4;
CountDownLatch startSignal = new CountDownLatch(1);
CountDownLatch readyToStart = new CountDownLatch(numberOfThreads);
ExecutorService executorService = Executors.newFixedThreadPool(numberOfThreads);
List<Future<Document>> documents = new ArrayList<>();
try {
for (int i = 0; i < numberOfThreads; i++) {
documents.add(executorService.submit(() -> {
readyToStart.countDown();
startSignal.await();
return createDocument(xsdSchema);
}));
}
readyToStart.await();
startSignal.countDown();
Document expected = createDocument(xsdResource);
assertThat(documents).hasSize(numberOfThreads)
.extracting(Future::get)
.allSatisfy((actual) -> XmlAssert.assertThat(actual).and(expected).ignoreWhitespace());
}
finally {
executorService.shutdownNow();
}
}

protected abstract XsdSchema createSchema(Resource resource) throws Exception;

private 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();
}

}
4 changes: 2 additions & 2 deletions spring-xml/src/test/java/org/springframework/xml/xsd/SimpleXsdSchemaTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {

Expand All @@ -35,7 +35,7 @@ protected XsdSchema createSchema(Resource resource) throws Exception {

@Test
void testBareXsdSchema() {
assertThatIllegalArgumentException().isThrownBy(() -> new SimpleXsdSchema().toString());
assertThatIllegalStateException().isThrownBy(() -> new SimpleXsdSchema().toString());
}

}