Skip to content

Commit c9d3963

Browse files
cshannoncshannon
authored
Improve FactoryFinder Validation (#1793)
Also modernizes with generics and adds improved testing
1 parent ed31052 commit c9d3963

20 files changed

Lines changed: 1087 additions & 98 deletions

File tree

‎activemq-broker/src/main/java/org/apache/activemq/broker/BrokerFactory.java‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -31,14 +31,16 @@
3131
*/
3232
public final class BrokerFactory {
3333

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

3638
private BrokerFactory() {
3739
}
3840

3941
public static BrokerFactoryHandler createBrokerFactoryHandler(String type) throws IOException {
4042
try {
41-
return (BrokerFactoryHandler)BROKER_FACTORY_HANDLER_FINDER.newInstance(type);
43+
return BROKER_FACTORY_HANDLER_FINDER.newInstance(type);
4244
} catch (Throwable e) {
4345
throw IOExceptionSupport.create("Could not load " + type + " factory:" + e, e);
4446
}

‎activemq-broker/src/main/java/org/apache/activemq/broker/region/group/GroupFactoryFinder.java‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,9 @@
2525
import org.apache.activemq.util.URISupport;
2626

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

3032
private GroupFactoryFinder() {
3133
}
@@ -40,7 +42,7 @@ public static MessageGroupMapFactory createMessageGroupMapFactory(String type) t
4042
factoryType = factoryType.substring(0,p);
4143
properties = URISupport.parseQuery(propertiesString);
4244
}
43-
MessageGroupMapFactory result = (MessageGroupMapFactory)GROUP_FACTORY_FINDER.newInstance(factoryType);
45+
MessageGroupMapFactory result = GROUP_FACTORY_FINDER.newInstance(factoryType);
4446
if (properties != null && result != null){
4547
IntrospectionSupport.setProperties(result,properties);
4648
}

‎activemq-broker/src/main/java/org/apache/activemq/transport/auto/AutoTcpTransportServer.java‎

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -80,15 +80,19 @@ public class AutoTcpTransportServer extends TcpTransportServer {
8080
protected int maxConnectionThreadPoolSize = Integer.MAX_VALUE;
8181
protected int protocolDetectionTimeOut = 30000;
8282

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

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

8892
public WireFormatFactory findWireFormatFactory(String scheme, Map<String, Map<String, Object>> options) throws IOException {
8993
WireFormatFactory wff = null;
9094
try {
91-
wff = (WireFormatFactory)WIREFORMAT_FACTORY_FINDER.newInstance(scheme);
95+
wff = WIREFORMAT_FACTORY_FINDER.newInstance(scheme);
9296
if (options != null) {
9397
final Map<String, Object> wfOptions = new HashMap<>();
9498
if (options.get(AutoTransportUtils.ALL) != null) {
@@ -117,7 +121,7 @@ public TransportFactory findTransportFactory(String scheme, Map<String, ?> optio
117121
if (tf == null) {
118122
// Try to load if from a META-INF property.
119123
try {
120-
tf = (TransportFactory)TRANSPORT_FACTORY_FINDER.newInstance(scheme);
124+
tf = TRANSPORT_FACTORY_FINDER.newInstance(scheme);
121125
if (options != null) {
122126
IntrospectionSupport.setProperties(tf, options);
123127
}

‎activemq-broker/src/main/java/org/apache/activemq/util/osgi/Activator.java‎

Lines changed: 21 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -172,7 +172,15 @@ protected void unregister(long bundleId) {
172172
// ================================================================
173173

174174
@Override
175-
public Object create(String path) throws IllegalAccessException, InstantiationException, IOException, ClassNotFoundException {
175+
public Object create(String path)
176+
throws IllegalAccessException, InstantiationException, IOException, ClassNotFoundException {
177+
throw new UnsupportedOperationException("Create is not supported without requiredType and allowed impls");
178+
}
179+
180+
@SuppressWarnings("unchecked")
181+
@Override
182+
public <T> T create(String path, Class<T> requiredType, Set<Class<? extends T>> allowedImpls)
183+
throws IllegalAccessException, InstantiationException, IOException, ClassNotFoundException {
176184
Class<?> clazz = serviceCache.get(path);
177185
if (clazz == null) {
178186
StringBuilder warnings = new StringBuilder();
@@ -199,6 +207,10 @@ public Object create(String path) throws IllegalAccessException, InstantiationEx
199207
continue;
200208
}
201209

210+
// no reason to cache if invalid so validate before caching
211+
// the class inside of serviceCache
212+
FactoryFinder.validateClass(clazz, requiredType, allowedImpls);
213+
202214
// Yay.. the class was found. Now cache it.
203215
serviceCache.put(path, clazz);
204216
wrapper.cachedServices.add(path);
@@ -214,10 +226,17 @@ public Object create(String path) throws IllegalAccessException, InstantiationEx
214226
}
215227
throw new IOException(msg);
216228
}
229+
} else {
230+
// Validate again (even for previously cached classes) in case
231+
// a path is re-used with a different requiredType.
232+
// This object factory is shared by all factory finder instances, so it would be
233+
// possible (although probably a mistake) to use the same
234+
// path again with a different requiredType in a different FactoryFinder
235+
FactoryFinder.validateClass(clazz, requiredType, allowedImpls);
217236
}
218237

219238
try {
220-
return clazz.getConstructor().newInstance();
239+
return (T) clazz.getConstructor().newInstance();
221240
} catch (InvocationTargetException | NoSuchMethodException e) {
222241
throw new InstantiationException(e.getMessage());
223242
}

‎activemq-client/src/main/java/org/apache/activemq/transport/TransportFactory.java‎

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -36,8 +36,12 @@
3636

3737
public abstract class TransportFactory {
3838

39-
private static final FactoryFinder TRANSPORT_FACTORY_FINDER = new FactoryFinder("META-INF/services/org/apache/activemq/transport/");
40-
private static final FactoryFinder WIREFORMAT_FACTORY_FINDER = new FactoryFinder("META-INF/services/org/apache/activemq/wireformat/");
39+
private static final FactoryFinder<TransportFactory> TRANSPORT_FACTORY_FINDER
40+
= new FactoryFinder<>("META-INF/services/org/apache/activemq/transport/", TransportFactory.class,
41+
null);
42+
private static final FactoryFinder<WireFormatFactory> WIREFORMAT_FACTORY_FINDER
43+
= new FactoryFinder<>("META-INF/services/org/apache/activemq/wireformat/", WireFormatFactory.class,
44+
null);
4145
private static final ConcurrentMap<String, TransportFactory> TRANSPORT_FACTORYS = new ConcurrentHashMap<String, TransportFactory>();
4246

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

203207
try {
204-
WireFormatFactory wff = (WireFormatFactory)WIREFORMAT_FACTORY_FINDER.newInstance(wireFormat);
208+
WireFormatFactory wff = WIREFORMAT_FACTORY_FINDER.newInstance(wireFormat);
205209
IntrospectionSupport.setProperties(wff, options, "wireFormat.");
206210
return wff;
207211
} catch (Throwable e) {

‎activemq-client/src/main/java/org/apache/activemq/transport/discovery/DiscoveryAgentFactory.java‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,9 @@
2626

2727
public abstract class DiscoveryAgentFactory {
2828

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

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

0 commit comments

Comments
 (0)