Skip to content

Commit 9e69872

Browse files
cshannoncshannon
authored
Make brokerName immutable in RegionBroker (apache#1917) (apache#1924)
The brokerName should come from BrokerService and should only be configured on first creation. This update changes RegionBroker so that it gets the name from the broker service during construction and verifies that it is not null. The other benefit of this is that BrokerService always validates the name has valid characters. This change also cleans up the name regex to get rid of unnecessary escapes and also adds some regex tests. (cherry picked from commit 084502a)
1 parent e1802c8 commit 9e69872

3 files changed

Lines changed: 46 additions & 17 deletions

File tree

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

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1053,7 +1053,8 @@ public String getBrokerName() {
10531053
}
10541054

10551055

1056-
private static final String brokerNameReplacedCharsRegExp = "[^a-zA-Z0-9\\.\\_\\-\\:]";
1056+
// Matches a single character that is invalid in a broker name
1057+
private static final String INVALID_BROKER_NAME_CHAR_REG_EXP = "[^a-zA-Z0-9._\\-:]";
10571058

10581059
/**
10591060
* Sets the name of this broker; which must be unique in the network.
@@ -1062,9 +1063,9 @@ public void setBrokerName(String brokerName) {
10621063
if (brokerName == null) {
10631064
throw new NullPointerException("The broker name cannot be null");
10641065
}
1065-
String str = brokerName.replaceAll(brokerNameReplacedCharsRegExp, "_");
1066+
String str = brokerName.replaceAll(INVALID_BROKER_NAME_CHAR_REG_EXP, "_");
10661067
if (!str.equals(brokerName)) {
1067-
LOG.error("Broker Name: {} contained illegal characters matching regExp: {} - replaced with {}", brokerName, brokerNameReplacedCharsRegExp, str);
1068+
LOG.error("Broker Name: {} contained illegal characters matching regExp: {} - replaced with {}", brokerName, INVALID_BROKER_NAME_CHAR_REG_EXP, str);
10681069
}
10691070
this.brokerName = str.trim();
10701071
}
@@ -2396,7 +2397,6 @@ protected Broker createRegionBroker(DestinationInterceptor destinationIntercepto
23962397
}
23972398
destinationFactory.setRegionBroker(regionBroker);
23982399
regionBroker.setKeepDurableSubsActive(keepDurableSubsActive);
2399-
regionBroker.setBrokerName(getBrokerName());
24002400
regionBroker.getDestinationStatistics().setEnabled(enableStatistics);
24012401
regionBroker.setAllowTempAutoCreationOnSend(isAllowTempAutoCreationOnSend());
24022402
if (brokerId != null) {

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

Lines changed: 4 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@
2424
import java.util.List;
2525
import java.util.Locale;
2626
import java.util.Map;
27+
import java.util.Objects;
2728
import java.util.Set;
2829
import java.util.concurrent.ConcurrentHashMap;
2930
import java.util.concurrent.CopyOnWriteArrayList;
@@ -104,7 +105,7 @@ public class RegionBroker extends EmptyBroker {
104105

105106
private final LongSequenceGenerator sequenceGenerator = new LongSequenceGenerator();
106107
private BrokerId brokerId;
107-
private String brokerName;
108+
private final String brokerName;
108109
private final Map<String, ConnectionContext> clientIdSet = new HashMap<String, ConnectionContext>();
109110
private final DestinationInterceptor destinationInterceptor;
110111
private ConnectionContext adminConnectionContext;
@@ -138,7 +139,8 @@ public void run() {
138139

139140
public RegionBroker(BrokerService brokerService, TaskRunnerFactory taskRunnerFactory, SystemUsage memoryManager, DestinationFactory destinationFactory,
140141
DestinationInterceptor destinationInterceptor, Scheduler scheduler, ThreadPoolExecutor executor) throws IOException {
141-
this.brokerService = brokerService;
142+
this.brokerService = Objects.requireNonNull(brokerService);
143+
this.brokerName = Objects.requireNonNull(brokerService.getBrokerName(), "The broker name cannot be null");
142144
this.executor = executor;
143145
this.scheduler = scheduler;
144146
if (destinationFactory == null) {
@@ -564,20 +566,9 @@ public void setBrokerId(BrokerId brokerId) {
564566

565567
@Override
566568
public String getBrokerName() {
567-
if (brokerName == null) {
568-
try {
569-
brokerName = InetAddressUtil.getLocalHostName().toLowerCase(Locale.ENGLISH);
570-
} catch (Exception e) {
571-
brokerName = "localhost";
572-
}
573-
}
574569
return brokerName;
575570
}
576571

577-
public void setBrokerName(String brokerName) {
578-
this.brokerName = brokerName;
579-
}
580-
581572
public DestinationStatistics getDestinationStatistics() {
582573
return destinationStatistics;
583574
}

‎activemq-unit-tests/src/test/java/org/apache/activemq/broker/BrokerServiceTest.java‎

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -91,6 +91,44 @@ public void testSystemUsage() {
9191
assertEquals( 1024L * 1024 * 1024 * 100, service.getSystemUsage().getStoreUsage().getLimit() );
9292
}
9393

94+
public void testSetBrokerNameInvalidChars() {
95+
final BrokerService brokerService = new BrokerService();
96+
97+
// All valid
98+
brokerService.setBrokerName("valid");
99+
assertEquals("valid", brokerService.getBrokerName());
100+
brokerService.setBrokerName("valid123");
101+
assertEquals("valid123", brokerService.getBrokerName());
102+
brokerService.setBrokerName("this_is_valid");
103+
assertEquals("this_is_valid", brokerService.getBrokerName());
104+
brokerService.setBrokerName("this_123_valid");
105+
assertEquals("this_123_valid", brokerService.getBrokerName());
106+
brokerService.setBrokerName("valid-name123");
107+
assertEquals("valid-name123", brokerService.getBrokerName());
108+
brokerService.setBrokerName("1235.6789");
109+
assertEquals("1235.6789", brokerService.getBrokerName());
110+
brokerService.setBrokerName("valid:123");
111+
assertEquals("valid:123", brokerService.getBrokerName());
112+
113+
// Test invalid names
114+
brokerService.setBrokerName("abc?bad");
115+
assertEquals("abc_bad", brokerService.getBrokerName());
116+
brokerService.setBrokerName("#");
117+
assertEquals("_", brokerService.getBrokerName());
118+
brokerService.setBrokerName("?");
119+
assertEquals("_", brokerService.getBrokerName());
120+
brokerService.setBrokerName("invalid%");
121+
assertEquals("invalid_", brokerService.getBrokerName());
122+
brokerService.setBrokerName("\\");
123+
assertEquals("_", brokerService.getBrokerName());
124+
brokerService.setBrokerName("<>");
125+
assertEquals("__", brokerService.getBrokerName());
126+
brokerService.setBrokerName("abc=");
127+
assertEquals("abc_", brokerService.getBrokerName());
128+
brokerService.setBrokerName("name:abc=?bad");
129+
assertEquals("name:abc__bad", brokerService.getBrokerName());
130+
}
131+
94132
@Test
95133
public void testLargeFileSystem() throws Exception {
96134
BrokerService service = new BrokerService();

0 commit comments

Comments
 (0)