Make brokerName immutable in RegionBroker (apache#1917) (apache#1924)

Cherry-picked from 5.19.x: 9e69872

Author: jgallimore

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

@@ -1053,7 +1053,8 @@ public String getBrokerName() {
     }
 
 
-    private static final String brokerNameReplacedCharsRegExp = "[^a-zA-Z0-9\\.\\_\\-\\:]";
+    // Matches a single character that is invalid in a broker name
+    private static final String INVALID_BROKER_NAME_CHAR_REG_EXP = "[^a-zA-Z0-9._\\-:]";
 
     /**
      * Sets the name of this broker; which must be unique in the network.
@@ -1062,9 +1063,9 @@ public void setBrokerName(String brokerName) {
         if (brokerName == null) {
             throw new NullPointerException("The broker name cannot be null");
         }
-        String str = brokerName.replaceAll(brokerNameReplacedCharsRegExp, "_");
+        String str = brokerName.replaceAll(INVALID_BROKER_NAME_CHAR_REG_EXP, "_");
         if (!str.equals(brokerName)) {
-            LOG.error("Broker Name: {} contained illegal characters matching regExp: {} - replaced with {}", brokerName, brokerNameReplacedCharsRegExp, str);
+            LOG.error("Broker Name: {} contained illegal characters matching regExp: {} - replaced with {}", brokerName, INVALID_BROKER_NAME_CHAR_REG_EXP, str);
         }
         this.brokerName = str.trim();
     }
@@ -2396,7 +2397,6 @@ protected Broker createRegionBroker(DestinationInterceptor destinationIntercepto
         }
         destinationFactory.setRegionBroker(regionBroker);
         regionBroker.setKeepDurableSubsActive(keepDurableSubsActive);
-        regionBroker.setBrokerName(getBrokerName());
         regionBroker.getDestinationStatistics().setEnabled(enableStatistics);
         regionBroker.setAllowTempAutoCreationOnSend(isAllowTempAutoCreationOnSend());
         if (brokerId != null) {

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

@@ -24,6 +24,7 @@
 import java.util.List;
 import java.util.Locale;
 import java.util.Map;
+import java.util.Objects;
 import java.util.Set;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.CopyOnWriteArrayList;
@@ -104,7 +105,7 @@ public class RegionBroker extends EmptyBroker {
 
     private final LongSequenceGenerator sequenceGenerator = new LongSequenceGenerator();
     private BrokerId brokerId;
-    private String brokerName;
+    private final String brokerName;
     private final Map<String, ConnectionContext> clientIdSet = new HashMap<String, ConnectionContext>();
     private final DestinationInterceptor destinationInterceptor;
     private ConnectionContext adminConnectionContext;
@@ -138,7 +139,8 @@ public void run() {
 
     public RegionBroker(BrokerService brokerService, TaskRunnerFactory taskRunnerFactory, SystemUsage memoryManager, DestinationFactory destinationFactory,
         DestinationInterceptor destinationInterceptor, Scheduler scheduler, ThreadPoolExecutor executor) throws IOException {
-        this.brokerService = brokerService;
+        this.brokerService = Objects.requireNonNull(brokerService);
+        this.brokerName = Objects.requireNonNull(brokerService.getBrokerName(), "The broker name cannot be null");
         this.executor = executor;
         this.scheduler = scheduler;
         if (destinationFactory == null) {
@@ -564,20 +566,9 @@ public void setBrokerId(BrokerId brokerId) {
 
     @Override
     public String getBrokerName() {
-        if (brokerName == null) {
-            try {
-                brokerName = InetAddressUtil.getLocalHostName().toLowerCase(Locale.ENGLISH);
-            } catch (Exception e) {
-                brokerName = "localhost";
-            }
-        }
         return brokerName;
     }
 
-    public void setBrokerName(String brokerName) {
-        this.brokerName = brokerName;
-    }
-
     public DestinationStatistics getDestinationStatistics() {
         return destinationStatistics;
     }

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

@@ -91,6 +91,44 @@ public void testSystemUsage() {
         assertEquals( 1024L * 1024 * 1024 * 100, service.getSystemUsage().getStoreUsage().getLimit() );
     }
 
+    public void testSetBrokerNameInvalidChars() {
+        final BrokerService brokerService = new BrokerService();
+
+        // All valid
+        brokerService.setBrokerName("valid");
+        assertEquals("valid", brokerService.getBrokerName());
+        brokerService.setBrokerName("valid123");
+        assertEquals("valid123", brokerService.getBrokerName());
+        brokerService.setBrokerName("this_is_valid");
+        assertEquals("this_is_valid", brokerService.getBrokerName());
+        brokerService.setBrokerName("this_123_valid");
+        assertEquals("this_123_valid", brokerService.getBrokerName());
+        brokerService.setBrokerName("valid-name123");
+        assertEquals("valid-name123", brokerService.getBrokerName());
+        brokerService.setBrokerName("1235.6789");
+        assertEquals("1235.6789", brokerService.getBrokerName());
+        brokerService.setBrokerName("valid:123");
+        assertEquals("valid:123", brokerService.getBrokerName());
+
+        // Test invalid names
+        brokerService.setBrokerName("abc?bad");
+        assertEquals("abc_bad", brokerService.getBrokerName());
+        brokerService.setBrokerName("#");
+        assertEquals("_", brokerService.getBrokerName());
+        brokerService.setBrokerName("?");
+        assertEquals("_", brokerService.getBrokerName());
+        brokerService.setBrokerName("invalid%");
+        assertEquals("invalid_", brokerService.getBrokerName());
+        brokerService.setBrokerName("\\");
+        assertEquals("_", brokerService.getBrokerName());
+        brokerService.setBrokerName("<>");
+        assertEquals("__", brokerService.getBrokerName());
+        brokerService.setBrokerName("abc=");
+        assertEquals("abc_", brokerService.getBrokerName());
+        brokerService.setBrokerName("name:abc=?bad");
+        assertEquals("name:abc__bad", brokerService.getBrokerName());
+    }
+
     @Test
     public void testLargeFileSystem() throws Exception {
         BrokerService service = new BrokerService();