Skip to content

SimpleXsdSchema 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.

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

Open
wants to merge 1 commit into
base: 4.0.x
Choose a base branch
from
Open

Conversation

nosan
Copy link

@nosan nosan commented May 2, 2025

See #1043

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label May 2, 2025
@nosan nosan force-pushed the 1043 branch 7 times, most recently from e51b606 to d7b42c8 Compare May 2, 2025 15:17
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 <dimanosan@gmail.com>
@nosan nosan changed the base branch from main to 4.0.x May 2, 2025 16:35
@@ -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
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);
		}
	}

@snicoll
Copy link
Member

snicoll commented May 6, 2025

Thanks @nosan. I want to acknowledge that I've seen this but will need a bit of time to review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: waiting-for-triage An issue we've not yet triaged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants