Skip to content

Improve FactoryFinder Validation #1793

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

Merged
cshannon merged 3 commits into from
Mar 18, 2026
Merged

Improve FactoryFinder Validation #1793

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2fe1f53
Improve FactoryFinder Validation
cshannon Mar 10, 2026
f0901eb
Remove unused imports
cshannon Mar 18, 2026
f7ce848
Address comments
cshannon Mar 18, 2026
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
6 changes: 4 additions & 2 deletions activemq-broker/src/main/java/org/apache/activemq/broker/BrokerFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,14 +31,16 @@
*/
public final class BrokerFactory {

private static final FactoryFinder BROKER_FACTORY_HANDLER_FINDER = new FactoryFinder("META-INF/services/org/apache/activemq/broker/");
private static final FactoryFinder<BrokerFactoryHandler> BROKER_FACTORY_HANDLER_FINDER
= new FactoryFinder<>("META-INF/services/org/apache/activemq/broker/", BrokerFactoryHandler.class,
null);

private BrokerFactory() {
}

public static BrokerFactoryHandler createBrokerFactoryHandler(String type) throws IOException {
try {
return (BrokerFactoryHandler)BROKER_FACTORY_HANDLER_FINDER.newInstance(type);
return BROKER_FACTORY_HANDLER_FINDER.newInstance(type);
} catch (Throwable e) {
throw IOExceptionSupport.create("Could not load " + type + " factory:" + e, e);
}
Expand Down
6 changes: 4 additions & 2 deletions ...vemq-broker/src/main/java/org/apache/activemq/broker/region/group/GroupFactoryFinder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,9 @@
import org.apache.activemq.util.URISupport;

public class GroupFactoryFinder {
private static final FactoryFinder GROUP_FACTORY_FINDER = new FactoryFinder("META-INF/services/org/apache/activemq/groups/");
private static final FactoryFinder<MessageGroupMapFactory> GROUP_FACTORY_FINDER =
new FactoryFinder<>("META-INF/services/org/apache/activemq/groups/", MessageGroupMapFactory.class,
null);

private GroupFactoryFinder() {
}
Expand All @@ -40,7 +42,7 @@ public static MessageGroupMapFactory createMessageGroupMapFactory(String type) t
factoryType = factoryType.substring(0,p);
properties = URISupport.parseQuery(propertiesString);
}
MessageGroupMapFactory result = (MessageGroupMapFactory)GROUP_FACTORY_FINDER.newInstance(factoryType);
MessageGroupMapFactory result = GROUP_FACTORY_FINDER.newInstance(factoryType);
if (properties != null && result != null){
IntrospectionSupport.setProperties(result,properties);
}
Expand Down
12 changes: 8 additions & 4 deletions activemq-broker/src/main/java/org/apache/activemq/transport/auto/AutoTcpTransportServer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,15 +80,19 @@ public class AutoTcpTransportServer extends TcpTransportServer {
protected int maxConnectionThreadPoolSize = Integer.MAX_VALUE;
protected int protocolDetectionTimeOut = 30000;

private static final FactoryFinder TRANSPORT_FACTORY_FINDER = new FactoryFinder("META-INF/services/org/apache/activemq/transport/");
private static final FactoryFinder<TransportFactory> TRANSPORT_FACTORY_FINDER =
new FactoryFinder<>("META-INF/services/org/apache/activemq/transport/", TransportFactory.class,
null);
private final ConcurrentMap<String, TransportFactory> transportFactories = new ConcurrentHashMap<String, TransportFactory>();

private static final FactoryFinder WIREFORMAT_FACTORY_FINDER = new FactoryFinder("META-INF/services/org/apache/activemq/wireformat/");
private static final FactoryFinder<WireFormatFactory> WIREFORMAT_FACTORY_FINDER =
new FactoryFinder<>("META-INF/services/org/apache/activemq/wireformat/", WireFormatFactory.class,
null);

public WireFormatFactory findWireFormatFactory(String scheme, Map<String, Map<String, Object>> options) throws IOException {
WireFormatFactory wff = null;
try {
wff = (WireFormatFactory)WIREFORMAT_FACTORY_FINDER.newInstance(scheme);
wff = WIREFORMAT_FACTORY_FINDER.newInstance(scheme);
if (options != null) {
final Map<String, Object> wfOptions = new HashMap<>();
if (options.get(AutoTransportUtils.ALL) != null) {
Expand Down Expand Up @@ -117,7 +121,7 @@ public TransportFactory findTransportFactory(String scheme, Map<String, ?> optio
if (tf == null) {
// Try to load if from a META-INF property.
try {
tf = (TransportFactory)TRANSPORT_FACTORY_FINDER.newInstance(scheme);
tf = TRANSPORT_FACTORY_FINDER.newInstance(scheme);
if (options != null) {
IntrospectionSupport.setProperties(tf, options);
}
Expand Down
23 changes: 21 additions & 2 deletions activemq-broker/src/main/java/org/apache/activemq/util/osgi/Activator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,15 @@ protected void unregister(long bundleId) {
// ================================================================

@Override
public Object create(String path) throws IllegalAccessException, InstantiationException, IOException, ClassNotFoundException {
public Object create(String path)
throws IllegalAccessException, InstantiationException, IOException, ClassNotFoundException {
throw new UnsupportedOperationException("Create is not supported without requiredType and allowed impls");
Copy link
Member

@jbonofre jbonofre Mar 18, 2026

Choose a reason for hiding this comment

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

nit: as it's a "breaking change", we should remember to note in the release note.

Copy link
Contributor Author

@cshannon cshannon Mar 18, 2026

Choose a reason for hiding this comment

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

I would not call this a breaking change because ActiveMQ never calls this method. FactoryFinder now calls the new method in the ObjectFactory interface so this new method is never invoked unless someone extended or implemented the interface themselves or wrote something custom. If someone has their own version of ObjectFactory everything works fine because the new method just delegates to the legacy. This is only breaking if it's expected users would invoke Activator themselves and not just the broker but I don't see why we would expect that.

}

@SuppressWarnings("unchecked")
@Override
public <T> T create(String path, Class<T> requiredType, Set<Class<? extends T>> allowedImpls)
throws IllegalAccessException, InstantiationException, IOException, ClassNotFoundException {
Class<?> clazz = serviceCache.get(path);
if (clazz == null) {
StringBuilder warnings = new StringBuilder();
Expand All @@ -199,6 +207,10 @@ public Object create(String path) throws IllegalAccessException, InstantiationEx
continue;
}

// no reason to cache if invalid so validate before caching
// the class inside of serviceCache
FactoryFinder.validateClass(clazz, requiredType, allowedImpls);

// Yay.. the class was found. Now cache it.
serviceCache.put(path, clazz);
wrapper.cachedServices.add(path);
Expand All @@ -214,10 +226,17 @@ public Object create(String path) throws IllegalAccessException, InstantiationEx
}
throw new IOException(msg);
}
} else {
// Validate again (even for previously cached classes) in case
// a path is re-used with a different requiredType.
// This object factory is shared by all factory finder instances, so it would be
// possible (although probably a mistake) to use the same
// path again with a different requiredType in a different FactoryFinder
FactoryFinder.validateClass(clazz, requiredType, allowedImpls);
}

try {
return clazz.getConstructor().newInstance();
return (T) clazz.getConstructor().newInstance();
} catch (InvocationTargetException | NoSuchMethodException e) {
throw new InstantiationException(e.getMessage());
}
Expand Down
12 changes: 8 additions & 4 deletions activemq-client/src/main/java/org/apache/activemq/transport/TransportFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,12 @@

public abstract class TransportFactory {

private static final FactoryFinder TRANSPORT_FACTORY_FINDER = new FactoryFinder("META-INF/services/org/apache/activemq/transport/");
private static final FactoryFinder WIREFORMAT_FACTORY_FINDER = new FactoryFinder("META-INF/services/org/apache/activemq/wireformat/");
private static final FactoryFinder<TransportFactory> TRANSPORT_FACTORY_FINDER
= new FactoryFinder<>("META-INF/services/org/apache/activemq/transport/", TransportFactory.class,
null);
private static final FactoryFinder<WireFormatFactory> WIREFORMAT_FACTORY_FINDER
= new FactoryFinder<>("META-INF/services/org/apache/activemq/wireformat/", WireFormatFactory.class,
null);
private static final ConcurrentMap<String, TransportFactory> TRANSPORT_FACTORYS = new ConcurrentHashMap<String, TransportFactory>();

private static final String WRITE_TIMEOUT_FILTER = "soWriteTimeout";
Expand Down Expand Up @@ -179,7 +183,7 @@ public static TransportFactory findTransportFactory(URI location) throws IOExcep
if (tf == null) {
// Try to load if from a META-INF property.
try {
tf = (TransportFactory)TRANSPORT_FACTORY_FINDER.newInstance(scheme);
tf = TRANSPORT_FACTORY_FINDER.newInstance(scheme);
TRANSPORT_FACTORYS.put(scheme, tf);
} catch (Throwable e) {
throw IOExceptionSupport.create("Transport scheme NOT recognized: [" + scheme + "]", e);
Expand All @@ -201,7 +205,7 @@ protected WireFormatFactory createWireFormatFactory(Map<String, String> options)
}

try {
WireFormatFactory wff = (WireFormatFactory)WIREFORMAT_FACTORY_FINDER.newInstance(wireFormat);
WireFormatFactory wff = WIREFORMAT_FACTORY_FINDER.newInstance(wireFormat);
IntrospectionSupport.setProperties(wff, options, "wireFormat.");
return wff;
} catch (Throwable e) {
Expand Down
6 changes: 4 additions & 2 deletions ...q-client/src/main/java/org/apache/activemq/transport/discovery/DiscoveryAgentFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,9 @@

public abstract class DiscoveryAgentFactory {

private static final FactoryFinder DISCOVERY_AGENT_FINDER = new FactoryFinder("META-INF/services/org/apache/activemq/transport/discoveryagent/");
private static final FactoryFinder<DiscoveryAgentFactory> DISCOVERY_AGENT_FINDER =
new FactoryFinder<>("META-INF/services/org/apache/activemq/transport/discoveryagent/",
DiscoveryAgentFactory.class, null);
private static final ConcurrentMap<String, DiscoveryAgentFactory> DISCOVERY_AGENT_FACTORYS = new ConcurrentHashMap<String, DiscoveryAgentFactory>();

/**
Expand All @@ -43,7 +45,7 @@ private static DiscoveryAgentFactory findDiscoveryAgentFactory(URI uri) throws I
if (daf == null) {
// Try to load if from a META-INF property.
try {
daf = (DiscoveryAgentFactory)DISCOVERY_AGENT_FINDER.newInstance(scheme);
daf = DISCOVERY_AGENT_FINDER.newInstance(scheme);
DISCOVERY_AGENT_FACTORYS.put(scheme, daf);
} catch (Throwable e) {
throw IOExceptionSupport.create("DiscoveryAgent scheme NOT recognized: [" + scheme + "]", e);
Expand Down
Loading
Loading