Make brokerName immutable in RegionBroker (#1917)

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.

Author: cshannon

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

@@ -1055,7 +1055,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.
@@ -1064,9 +1065,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();
     }
@@ -2408,7 +2409,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

@@ -95,6 +95,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());
+    }
+
     /** // AMQ-9239 FIXME: byte-buddy module opens
     @Test
     public void testLargeFileSystem() throws Exception {