From 16411224ea96c1933899dc29a23873f606a9f32c Mon Sep 17 00:00:00 2001 From: Paul Ferraro Date: Mon, 11 Mar 2024 18:16:22 -0400 Subject: [PATCH 1/3] WFCORE-6652 Improve user experience when using extension or namespace of unsupported stability. --- .../extension/ExtensionAddHandler.java | 27 +-- .../extension/ExtensionRegistry.java | 77 +++++-- .../UnstableSubsystemNamespaceParser.java | 31 +++ .../controller/logging/ControllerLogger.java | 3 + .../parsing/ExtensionParsingContext.java | 6 +- .../extension/ExtensionRegistryTestCase.java | 213 ++++++++++++++++++ 6 files changed, 324 insertions(+), 33 deletions(-) create mode 100644 controller/src/main/java/org/jboss/as/controller/extension/UnstableSubsystemNamespaceParser.java create mode 100644 controller/src/test/java/org/jboss/as/controller/extension/ExtensionRegistryTestCase.java diff --git a/controller/src/main/java/org/jboss/as/controller/extension/ExtensionAddHandler.java b/controller/src/main/java/org/jboss/as/controller/extension/ExtensionAddHandler.java index c30bae4d6e5..025a87cbd17 100644 --- a/controller/src/main/java/org/jboss/as/controller/extension/ExtensionAddHandler.java +++ b/controller/src/main/java/org/jboss/as/controller/extension/ExtensionAddHandler.java @@ -110,21 +110,22 @@ static void initializeExtension(ExtensionRegistry extensionRegistry, String modu } while (extensions.hasNext()) { Extension extension = extensions.next(); - if (extensionRegistry.enables(extension)) { - ClassLoader oldTccl = WildFlySecurityManager.setCurrentContextClassLoaderPrivileged(extension.getClass()); - try { - if (unknownModule || !extensionRegistry.getExtensionModuleNames().contains(module)) { - // This extension wasn't handled by the standalone.xml or domain.xml parsing logic, so we - // need to initialize its parsers so we can display what XML namespaces it supports - extensionRegistry.initializeParsers(extension, module, null); - // AS7-6190 - ensure we initialize parsers for other extensions from this module - // now that we know the registry was unaware of the module - unknownModule = true; - } + ClassLoader oldTccl = WildFlySecurityManager.setCurrentContextClassLoaderPrivileged(extension.getClass()); + try { + if (unknownModule || !extensionRegistry.getExtensionModuleNames().contains(module)) { + // This extension wasn't handled by the standalone.xml or domain.xml parsing logic, so we + // need to initialize its parsers so we can display what XML namespaces it supports + extensionRegistry.initializeParsers(extension, module, null); + // AS7-6190 - ensure we initialize parsers for other extensions from this module + // now that we know the registry was unaware of the module + unknownModule = true; + } + // If extension is not enabled by stability level of process, skip model registration + if (extensionRegistry.enables(extension)) { extension.initialize(extensionRegistry.getExtensionContext(module, extension.getStability(), rootRegistration, extensionRegistryType)); - } finally { - WildFlySecurityManager.setCurrentContextClassLoaderPrivileged(oldTccl); } + } finally { + WildFlySecurityManager.setCurrentContextClassLoaderPrivileged(oldTccl); } } } catch (ModuleNotFoundException e) { diff --git a/controller/src/main/java/org/jboss/as/controller/extension/ExtensionRegistry.java b/controller/src/main/java/org/jboss/as/controller/extension/ExtensionRegistry.java index 25dc77a1c18..322681a1b46 100644 --- a/controller/src/main/java/org/jboss/as/controller/extension/ExtensionRegistry.java +++ b/controller/src/main/java/org/jboss/as/controller/extension/ExtensionRegistry.java @@ -348,9 +348,9 @@ public ExtensionParsingContext getExtensionParsingContext(final String moduleNam * be any actual parsing (e.g. in a slave Host Controller or in a server in a managed domain) */ public void initializeParsers(final Extension extension, final String moduleName, final XMLMapper xmlMapper) { - ExtensionParsingContextImpl parsingContext = new ExtensionParsingContextImpl(moduleName, xmlMapper); - extension.initializeParsers(parsingContext); - parsingContext.attemptCurrentParserInitialization(); + try (AbstractExtensionParsingContext context = this.enables(extension) ? new ExtensionParsingContextImpl(moduleName, xmlMapper) : new UnstableExtensionParsingContext(moduleName, xmlMapper)) { + extension.initializeParsers(context); + } } /** @@ -542,30 +542,25 @@ public Stability getStability() { return this.stability; } - private class ExtensionParsingContextImpl implements ExtensionParsingContext { - - private final ExtensionInfo extension; - private boolean hasNonSupplierParser; - private String latestNamespace; - private Supplier>> latestSupplier; + private abstract class AbstractExtensionParsingContext implements ExtensionParsingContext, AutoCloseable { + final ExtensionInfo extension; - private ExtensionParsingContextImpl(String extensionName, XMLMapper xmlMapper) { - extension = getExtensionInfo(extensionName); + AbstractExtensionParsingContext(String extensionName, XMLMapper xmlMapper) { + this.extension = ExtensionRegistry.this.getExtensionInfo(extensionName); if (xmlMapper != null) { - synchronized (extension) { - extension.xmlMapper = xmlMapper; + synchronized (this.extension) { + this.extension.xmlMapper = xmlMapper; } } } - @Override public ProcessType getProcessType() { - return processType; + return ExtensionRegistry.this.processType; } @Override public RunningMode getRunningMode() { - return runningModeControl.getRunningMode(); + return ExtensionRegistry.this.runningModeControl.getRunningMode(); } @Override @@ -578,6 +573,22 @@ public boolean enables(F feature) { return ExtensionRegistry.this.enables(feature); } + @Override + public void close() { + // Do nothing + } + } + + private class ExtensionParsingContextImpl extends AbstractExtensionParsingContext { + + private boolean hasNonSupplierParser; + private String latestNamespace; + private Supplier>> latestSupplier; + + ExtensionParsingContextImpl(String extensionName, XMLMapper xmlMapper) { + super(extensionName, xmlMapper); + } + @Override public void setSubsystemXmlMapping(String subsystemName, String namespaceUri, XMLElementReader> reader) { assert subsystemName != null : "subsystemName is null"; @@ -618,7 +629,8 @@ public void setProfileParsingCompletionHandler(ProfileParsingCompletionHandler h } } - private void attemptCurrentParserInitialization() { + @Override + public void close() { if (ExtensionRegistry.this.processType != ProcessType.DOMAIN_SERVER && !hasNonSupplierParser && latestSupplier != null) { // We've only been passed suppliers for parsers, which means the model @@ -645,6 +657,37 @@ private void preCacheParserDescription(XMLElementReader> reader) } } + private class UnstableExtensionParsingContext extends AbstractExtensionParsingContext { + + UnstableExtensionParsingContext(String extensionName, XMLMapper xmlMapper) { + super(extensionName, xmlMapper); + } + + @Override + public void setSubsystemXmlMapping(String subsystemName, String namespaceUri, XMLElementReader> reader) { + this.setSubsystemXMLMapping(subsystemName, namespaceUri); + } + + @Override + public void setSubsystemXmlMapping(String subsystemName, String namespaceUri, Supplier>> supplier) { + this.setSubsystemXMLMapping(subsystemName, namespaceUri); + } + + private void setSubsystemXMLMapping(String subsystemName, String namespaceUri) { + synchronized (this.extension) { + this.extension.getSubsystemInfo(subsystemName).addParsingNamespace(namespaceUri); + if (this.extension.xmlMapper != null) { + this.extension.xmlMapper.registerRootElement(new QName(namespaceUri, SUBSYSTEM), new UnstableSubsystemNamespaceParser(subsystemName)); + } + } + } + + @Override + public void setProfileParsingCompletionHandler(ProfileParsingCompletionHandler handler) { + // Ignore + } + } + @SuppressWarnings("deprecation") private class ExtensionContextImpl implements ExtensionContext, ExtensionContextSupplement { diff --git a/controller/src/main/java/org/jboss/as/controller/extension/UnstableSubsystemNamespaceParser.java b/controller/src/main/java/org/jboss/as/controller/extension/UnstableSubsystemNamespaceParser.java new file mode 100644 index 00000000000..0fbd917d5d5 --- /dev/null +++ b/controller/src/main/java/org/jboss/as/controller/extension/UnstableSubsystemNamespaceParser.java @@ -0,0 +1,31 @@ +/* + * Copyright The WildFly Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package org.jboss.as.controller.extension; + +import java.util.List; + +import javax.xml.stream.XMLStreamException; + +import org.jboss.as.controller.logging.ControllerLogger; +import org.jboss.dmr.ModelNode; +import org.jboss.staxmapper.XMLElementReader; +import org.jboss.staxmapper.XMLExtendedStreamReader; + +/** + * A placeholder parser for a subsystem namespace not enabled by the stability level of the current process. + */ +public class UnstableSubsystemNamespaceParser implements XMLElementReader> { + private final String subsystemName; + + public UnstableSubsystemNamespaceParser(String subsystemName) { + this.subsystemName = subsystemName; + } + + @Override + public void readElement(XMLExtendedStreamReader reader, List value) throws XMLStreamException { + throw ControllerLogger.ROOT_LOGGER.unstableSubsystemNamespace(this.subsystemName, reader.getNamespaceURI()); + } +} diff --git a/controller/src/main/java/org/jboss/as/controller/logging/ControllerLogger.java b/controller/src/main/java/org/jboss/as/controller/logging/ControllerLogger.java index 33d7cd4ecd9..facbd6daefa 100644 --- a/controller/src/main/java/org/jboss/as/controller/logging/ControllerLogger.java +++ b/controller/src/main/java/org/jboss/as/controller/logging/ControllerLogger.java @@ -3752,4 +3752,7 @@ OperationFailedRuntimeException capabilityAlreadyRegisteredInContext(String capa @Message(id = 504, value = "The operation %s is not defined for resource %s.") UnsupportedOperationException missingOperationForResource(String op, String address); + + @Message(id = 505, value = "%s subsystem namespace %s is not enabled by the current stability level") + XMLStreamException unstableSubsystemNamespace(String subsystemName, String namespaceURI); } diff --git a/controller/src/main/java/org/jboss/as/controller/parsing/ExtensionParsingContext.java b/controller/src/main/java/org/jboss/as/controller/parsing/ExtensionParsingContext.java index c67d7211f1c..0eb63146247 100644 --- a/controller/src/main/java/org/jboss/as/controller/parsing/ExtensionParsingContext.java +++ b/controller/src/main/java/org/jboss/as/controller/parsing/ExtensionParsingContext.java @@ -13,6 +13,7 @@ import org.jboss.as.controller.ProcessType; import org.jboss.as.controller.RunningMode; import org.jboss.as.controller.SubsystemSchema; +import org.jboss.as.controller.extension.UnstableSubsystemNamespaceParser; import org.jboss.dmr.ModelNode; import org.jboss.staxmapper.XMLElementReader; @@ -81,9 +82,8 @@ public interface ExtensionParsingContext extends FeatureRegistry { */ default > void setSubsystemXmlMappings(String subsystemName, Set schemas) { for (S schema : schemas) { - if (this.enables(schema)) { - this.setSubsystemXmlMapping(subsystemName, schema.getNamespace().getUri(), schema); - } + XMLElementReader> reader = this.enables(schema) ? schema : new UnstableSubsystemNamespaceParser(subsystemName); + this.setSubsystemXmlMapping(subsystemName, schema.getNamespace().getUri(), reader); } } diff --git a/controller/src/test/java/org/jboss/as/controller/extension/ExtensionRegistryTestCase.java b/controller/src/test/java/org/jboss/as/controller/extension/ExtensionRegistryTestCase.java new file mode 100644 index 00000000000..a7fba45a2ea --- /dev/null +++ b/controller/src/test/java/org/jboss/as/controller/extension/ExtensionRegistryTestCase.java @@ -0,0 +1,213 @@ +/* + * Copyright The WildFly Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package org.jboss.as.controller.extension; + +import static org.junit.Assert.*; +import static org.mockito.Mockito.*; + +import java.util.EnumSet; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.Set; + +import javax.xml.namespace.QName; +import javax.xml.stream.XMLStreamException; + +import org.jboss.as.controller.Extension; +import org.jboss.as.controller.ExtensionContext; +import org.jboss.as.controller.ProcessType; +import org.jboss.as.controller.SubsystemSchema; +import org.jboss.as.controller.parsing.ExtensionParsingContext; +import org.jboss.as.controller.xml.VersionedNamespace; +import org.jboss.as.version.Stability; +import org.jboss.dmr.ModelNode; +import org.jboss.staxmapper.IntVersion; +import org.jboss.staxmapper.XMLElementReader; +import org.jboss.staxmapper.XMLExtendedStreamReader; +import org.jboss.staxmapper.XMLMapper; +import org.junit.Test; +import org.mockito.ArgumentCaptor; + +/** + * Unit test for {@link ExtensionRegistry}. + */ +public class ExtensionRegistryTestCase { + + /** + * Verify {@link ExtensionRegistryTestCase#initializeParsers()} for stable vs unstable extensions. + */ + @Test + public void initializeParsers() { + // Validate combinations of registry vs extension stability + for (Stability stability : EnumSet.allOf(Stability.class)) { + ExtensionRegistry registry = ExtensionRegistry.builder(ProcessType.SELF_CONTAINED).withStability(stability).build(); + + for (Stability extensionStability : EnumSet.allOf(Stability.class)) { + TestExtension extension = new TestExtension(extensionStability.toString(), extensionStability); + + if (registry.enables(extension)) { + this.initializeStableExtensionParsers(registry, extension); + } else { + this.initializeUnstableExtensionParsers(registry, extension); + } + } + } + } + + private void initializeStableExtensionParsers(ExtensionRegistry registry, TestExtension extension) { + String subsystemName = extension.getSubsystemName(); + + // Record stability per namespace for use during verification + Map namespaces = new HashMap<>(); + // Create subsystem schema per stability + for (Stability stability : EnumSet.allOf(Stability.class)) { + VersionedNamespace namespace = SubsystemSchema.createSubsystemURN(subsystemName, stability, new IntVersion(1)); + extension.addSchema(new TestSubsystemSchema(namespace)); + namespaces.put(namespace.getUri(), stability); + } + + // Capture registered parser per namespace + XMLMapper mapper = mock(XMLMapper.class); + ArgumentCaptor capturedNames = ArgumentCaptor.forClass(QName.class); + ArgumentCaptor> capturedReaders = ArgumentCaptor.forClass(XMLElementReader.class); + + registry.initializeParsers(extension, subsystemName, mapper); + + // Extension should have registered a parser per stability + verify(mapper, times(namespaces.size())).registerRootElement(capturedNames.capture(), capturedReaders.capture()); + + List names = capturedNames.getAllValues(); + List> readers = capturedReaders.getAllValues(); + assertEquals(namespaces.size(), names.size()); + assertEquals(namespaces.size(), readers.size()); + + for (int i = 0; i < namespaces.size(); ++i) { + QName name = names.get(i); + XMLElementReader reader = readers.get(i); + + Stability stability = namespaces.get(name.getNamespaceURI()); + assertNotNull(stability); + + XMLExtendedStreamReader mockReader = mock(XMLExtendedStreamReader.class); + doReturn(name.getNamespaceURI()).when(mockReader).getNamespaceURI(); + + if (registry.getStability().enables(stability)) { + // Verify expected parsing if namespace is enabled by registry + try { + reader.readElement(mockReader, null); + verify(mockReader).discardRemainder(); + } catch (XMLStreamException e) { + fail(name.getNamespaceURI()); + } + } else { + // Verify parsing of disabled namespace throws exception + assertThrows(name.getNamespaceURI(), XMLStreamException.class, () -> reader.readElement(mockReader, null)); + } + } + } + + private void initializeUnstableExtensionParsers(ExtensionRegistry registry, TestExtension extension) { + // Create 2 schema versions per stability: + // * V1 uses traditional parser registration + // * V2 uses versioned schema registration + String subsystemName = extension.getSubsystemName(); + extension.addParser(SubsystemSchema.createSubsystemURN(subsystemName, new IntVersion(1)).getUri(), (reader, value) -> reader.discardRemainder()); + for (Stability stability : EnumSet.allOf(Stability.class)) { + // Versioned namespace parsers + extension.addSchema(new TestSubsystemSchema(SubsystemSchema.createSubsystemURN(subsystemName, stability, new IntVersion(2)))); + } + // Expected number of registered parsers + int namespaces = EnumSet.allOf(Stability.class).size() + 1; + + // Capture registered parser per namespace + XMLMapper mapper = mock(XMLMapper.class); + ArgumentCaptor capturedNames = ArgumentCaptor.forClass(QName.class); + ArgumentCaptor> capturedReaders = ArgumentCaptor.forClass(XMLElementReader.class); + + registry.initializeParsers(extension, subsystemName, mapper); + + // Extension should have registered 1 parser per stability + 1 traditional parser + verify(mapper, times(namespaces)).registerRootElement(capturedNames.capture(), capturedReaders.capture()); + + List names = capturedNames.getAllValues(); + List> readers = capturedReaders.getAllValues(); + assertEquals(namespaces, names.size()); + assertEquals(namespaces, readers.size()); + + // Verify that all parsers of an unstable extension throw an exception + for (int i = 0; i < namespaces; ++i) { + QName name = names.get(i); + XMLElementReader reader = readers.get(i); + + XMLExtendedStreamReader mockReader = mock(XMLExtendedStreamReader.class); + doReturn(name.getNamespaceURI()).when(mockReader).getNamespaceURI(); + + assertThrows(name.getNamespaceURI(), XMLStreamException.class, () -> reader.readElement(mockReader, null)); + } + } + + class TestSubsystemSchema implements SubsystemSchema { + private final VersionedNamespace namespace; + + TestSubsystemSchema(VersionedNamespace namespace) { + this.namespace = namespace; + } + + @Override + public VersionedNamespace getNamespace() { + return this.namespace; + } + + @Override + public void readElement(XMLExtendedStreamReader reader, List value) throws XMLStreamException { + reader.discardRemainder(); + } + } + + class TestExtension implements Extension { + private final String subsystemName; + private final Stability stability; + private final Map>> parsers = new HashMap<>(); + private final Map schemas = new HashMap<>(); + + TestExtension(String subsystemName, Stability stability) { + this.subsystemName = subsystemName; + this.stability = stability; + } + + String getSubsystemName() { + return this.subsystemName; + } + + void addParser(String namespaceURI, XMLElementReader> parser) { + this.parsers.put(namespaceURI, parser); + } + + void addSchema(TestSubsystemSchema schema) { + this.schemas.put(schema.getNamespace().getUri(), schema); + } + + @Override + public void initialize(ExtensionContext context) { + } + + @Override + public void initializeParsers(ExtensionParsingContext context) { + for (Map.Entry>> entry : this.parsers.entrySet()) { + context.setSubsystemXmlMapping(this.subsystemName, entry.getKey(), entry.getValue()); + } + for (Map.Entry entry : this.schemas.entrySet()) { + context.setSubsystemXmlMappings(this.subsystemName, Set.of(entry.getValue())); + } + } + + @Override + public Stability getStability() { + return this.stability; + } + } +} From 791455debdeb72c5e7b0ef2cbe87a2e6f6bd6a87 Mon Sep 17 00:00:00 2001 From: Paul Ferraro Date: Mon, 11 Mar 2024 18:37:13 -0400 Subject: [PATCH 2/3] Log warning when encountering extension not enabled by the stabilty level of the process. --- .../jboss/as/controller/extension/ExtensionAddHandler.java | 2 ++ .../org/jboss/as/controller/logging/ControllerLogger.java | 4 ++++ .../domain/controller/operations/ApplyExtensionsHandler.java | 3 +++ 3 files changed, 9 insertions(+) diff --git a/controller/src/main/java/org/jboss/as/controller/extension/ExtensionAddHandler.java b/controller/src/main/java/org/jboss/as/controller/extension/ExtensionAddHandler.java index 025a87cbd17..6f5c6dde2e9 100644 --- a/controller/src/main/java/org/jboss/as/controller/extension/ExtensionAddHandler.java +++ b/controller/src/main/java/org/jboss/as/controller/extension/ExtensionAddHandler.java @@ -123,6 +123,8 @@ static void initializeExtension(ExtensionRegistry extensionRegistry, String modu // If extension is not enabled by stability level of process, skip model registration if (extensionRegistry.enables(extension)) { extension.initialize(extensionRegistry.getExtensionContext(module, extension.getStability(), rootRegistration, extensionRegistryType)); + } else { + ControllerLogger.ROOT_LOGGER.unstableExtension(extension.getClass().getName(), module); } } finally { WildFlySecurityManager.setCurrentContextClassLoaderPrivileged(oldTccl); diff --git a/controller/src/main/java/org/jboss/as/controller/logging/ControllerLogger.java b/controller/src/main/java/org/jboss/as/controller/logging/ControllerLogger.java index facbd6daefa..7a48a72942e 100644 --- a/controller/src/main/java/org/jboss/as/controller/logging/ControllerLogger.java +++ b/controller/src/main/java/org/jboss/as/controller/logging/ControllerLogger.java @@ -3755,4 +3755,8 @@ OperationFailedRuntimeException capabilityAlreadyRegisteredInContext(String capa @Message(id = 505, value = "%s subsystem namespace %s is not enabled by the current stability level") XMLStreamException unstableSubsystemNamespace(String subsystemName, String namespaceURI); + + @LogMessage(level = WARN) + @Message(id = 506, value = "Extension %s from module %s is not enabled by the current stability level") + void unstableExtension(String extensionName, String moduleName); } diff --git a/host-controller/src/main/java/org/jboss/as/domain/controller/operations/ApplyExtensionsHandler.java b/host-controller/src/main/java/org/jboss/as/domain/controller/operations/ApplyExtensionsHandler.java index b8d80026ad2..238baea92ad 100644 --- a/host-controller/src/main/java/org/jboss/as/domain/controller/operations/ApplyExtensionsHandler.java +++ b/host-controller/src/main/java/org/jboss/as/domain/controller/operations/ApplyExtensionsHandler.java @@ -35,6 +35,7 @@ import org.jboss.as.controller.extension.ExtensionRegistry; import org.jboss.as.controller.extension.ExtensionRegistryType; import org.jboss.as.controller.extension.ExtensionResource; +import org.jboss.as.controller.logging.ControllerLogger; import org.jboss.as.controller.registry.ManagementResourceRegistration; import org.jboss.as.controller.registry.Resource; import org.jboss.as.domain.controller.LocalHostControllerInfo; @@ -203,6 +204,8 @@ protected void initializeExtension(String module, ManagementResourceRegistration } finally { SecurityActions.setThreadContextClassLoader(oldTccl); } + } else { + ControllerLogger.ROOT_LOGGER.unstableExtension(extension.getClass().getName(), module); } } } catch (ModuleLoadException e) { From 9e50fa69607ab2ae259673eeea2e6bb0dad9d656 Mon Sep 17 00:00:00 2001 From: Paul Ferraro Date: Fri, 15 Mar 2024 09:37:11 -0400 Subject: [PATCH 3/3] WFCORE-6741 ExtensionContext.getStability() always returns Stability.DEFAULT --- .../org/jboss/as/controller/extension/ExtensionRegistry.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/controller/src/main/java/org/jboss/as/controller/extension/ExtensionRegistry.java b/controller/src/main/java/org/jboss/as/controller/extension/ExtensionRegistry.java index 322681a1b46..f2853619461 100644 --- a/controller/src/main/java/org/jboss/as/controller/extension/ExtensionRegistry.java +++ b/controller/src/main/java/org/jboss/as/controller/extension/ExtensionRegistry.java @@ -776,7 +776,7 @@ public ProcessType getProcessType() { @Override public Stability getStability() { - return this.profileRegistration.getStability(); + return ExtensionRegistry.this.getStability(); } @Override