Skip to content

Conversation

nosan
Copy link
Contributor

@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
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);
		}
	}

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

@snicoll snicoll changed the title SimpleXsdSchema is not thread-safe SimpleXsdSchema#loadSchema is not thread-safe May 19, 2025
@snicoll snicoll added type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged labels May 19, 2025
@snicoll snicoll added this to the 4.0.15 milestone May 19, 2025
@snicoll snicoll self-assigned this May 19, 2025
snicoll pushed a commit that referenced this pull request May 19, 2025
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.

See gh-1556

Signed-off-by: Dmytro Nosan <dimanosan@gmail.com>
@snicoll snicoll closed this in 7a731eb May 19, 2025
@nosan
Copy link
Contributor Author

nosan commented May 19, 2025

Thanks @snicoll

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug A general bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants