fix: Manager.makeSystemName rejects empty strings

JMRI · May 20, 2019 · 51cb30f · 51cb30f
1 parent e47842f
commit 51cb30f
Show file tree

Hide file tree

Showing 7 changed files with 33 additions and 12 deletions.
diff --git a/java/src/jmri/Manager.java b/java/src/jmri/Manager.java
@@ -63,6 +63,17 @@ public interface Manager<E extends NamedBean> extends PropertyChangeProvider, Ve
     @CheckReturnValue
     public char typeLetter();
 
+    /**
+     * Get the mandatory prefix for the system name of the NamedBeans handled by
+     * this manager.
+     * 
+     * @return the mandatory prefix generated by concatenating the result of
+     * {@link #getSystemPrefix() } and {@link #typeLetter() }
+     */
+    public default String getSystemNamePrefix() {
+        return getSystemPrefix() + typeLetter();
+    }
+
     /**
      * @param s the item to make the system name for
      * @return A system name from a user input, typically a number.

diff --git a/java/src/jmri/managers/AbstractManager.java b/java/src/jmri/managers/AbstractManager.java
@@ -80,7 +80,12 @@ protected void registerSelf() {
     @Override
     @Nonnull
     public String makeSystemName(@Nonnull String s) {
-        return getSystemPrefix() + typeLetter() + s;
+        String prefix = getSystemNamePrefix();
+        if (s.isEmpty()) {
+            log.error("Invalid system name for {}: \"\" needed non-empty name to follow {}", getBeanTypeHandled(), prefix);
+            throw new IllegalArgumentException("Invalid system name for " + getBeanTypeHandled() + ": \"\" needed non-empty name to follow " + prefix);
+        }
+        return prefix + s;
     }
 
     /** {@inheritDoc} */

diff --git a/java/src/jmri/managers/DefaultMemoryManager.java b/java/src/jmri/managers/DefaultMemoryManager.java
@@ -21,14 +21,16 @@ public String getSystemPrefix() {
 
     @Override
     protected Memory createNewMemory(String systemName, String userName) {
-        if(systemName.equals("") || systemName.toUpperCase().equals("IM")){
-           log.error("Invalid system name for memory: \"{}\" but needed IM followed by a suffix",systemName);
-           throw new IllegalArgumentException("Invalid system name for memory: \"" + systemName + "\" but needed IM followed by a suffix");
+        String prefix = getSystemNamePrefix();
+        systemName = normalizeSystemName(systemName); // enforce triming and uppercase requirements
+        if (systemName.isEmpty() || systemName.equals(prefix)) {
+            log.error("Invalid system name for memory: \"{}\" but needed IM followed by a suffix", systemName);
+            throw new IllegalArgumentException("Invalid system name for memory: \"" + systemName + "\" but needed IM followed by a suffix");
         }
         // we've decided to enforce that memory system
         // names start with IM by prepending if not present
-        if (!systemName.toUpperCase().startsWith("IM")) {
-            systemName = "IM" + systemName;
+        if (!systemName.startsWith(prefix)) {
+            systemName = makeSystemName(systemName);
         }
         return new DefaultMemory(systemName, userName);
     }

diff --git a/java/test/jmri/managers/AbstractReporterMgrTestBase.java b/java/test/jmri/managers/AbstractReporterMgrTestBase.java
@@ -80,7 +80,7 @@ public void testProvideFailure() {
         try {
             l.provideReporter("");
         } catch (IllegalArgumentException ex) {
-          jmri.util.JUnitAppender.assertErrorMessage("Invalid system name for reporter: "+l.getSystemPrefix()+l.typeLetter()+" needed "+l.getSystemPrefix()+l.typeLetter());
+          jmri.util.JUnitAppender.assertErrorMessage("Invalid system name for Reporter: \"\" needed non-empty name to follow " + l.getSystemNamePrefix());
           throw ex;
         }
     }

diff --git a/java/test/jmri/managers/AbstractTurnoutMgrTestBase.java b/java/test/jmri/managers/AbstractTurnoutMgrTestBase.java
@@ -55,7 +55,7 @@ public void testProvideFailure() {
         try {
             l.provideTurnout("");
         } catch (IllegalArgumentException ex) {
-          jmri.util.JUnitAppender.assertErrorMessage("Invalid system name for turnout: "+l.getSystemPrefix()+l.typeLetter()+" needed "+l.getSystemPrefix()+l.typeLetter());
+          jmri.util.JUnitAppender.assertErrorMessage("Invalid system name for Turnout: \"\" needed non-empty name");
           throw ex;
         }
     }

diff --git a/java/test/jmri/managers/ProxySensorManagerTest.java b/java/test/jmri/managers/ProxySensorManagerTest.java
@@ -5,6 +5,7 @@
 
 import jmri.*;
 import jmri.jmrix.internal.InternalSensorManager;
+import jmri.util.JUnitAppender;
 import jmri.util.JUnitUtil;
 
 import org.junit.After;
@@ -41,15 +42,15 @@ public void testSensorNameCase() {
         Assert.assertEquals(0, l.getObjectCount());
         // create
         Sensor t = l.provideSensor("IS:XYZ");
-        t = l.provideSensor("IS:xyz");  // upper canse and lower case are the same object
+        Assert.assertEquals(t, l.provideSensor("IS:xyz"));  // upper case and lower case are the same object
         // check
         Assert.assertTrue("real object returned ", t != null);
         Assert.assertEquals("IS:XYZ", t.getSystemName());  // we force upper
         Assert.assertTrue("system name correct ", t == l.getBySystemName("IS:XYZ"));
         Assert.assertEquals(1, l.getObjectCount());
         Assert.assertEquals(1, l.getSystemNameAddedOrderList().size());
-
-        t = l.provideSensor("IS:XYZ");
+        // test providing same name as existing sensor does not create new sensor
+        l.provideSensor("IS:XYZ");
         Assert.assertEquals(1, l.getObjectCount());
         Assert.assertEquals(1, l.getSystemNameAddedOrderList().size());
     }
@@ -101,6 +102,7 @@ public void testProvideFailure() {
         } catch (IllegalArgumentException ex) {
             correct = true;
         }
+        JUnitAppender.assertErrorMessage("Invalid system name for Sensor: \"\" needed non-empty name to follow " + l.getSystemNamePrefix());
         Assert.assertTrue("Exception thrown properly", correct);
     }
 

diff --git a/java/test/jmri/managers/ProxyTurnoutManagerTest.java b/java/test/jmri/managers/ProxyTurnoutManagerTest.java
@@ -74,9 +74,10 @@ public void testProvideFailure() {
             Assert.fail("didn't throw");
         } catch (IllegalArgumentException ex) {
             correct = true;
+            System.out.println(ex.getMessage());
         }
         Assert.assertTrue("Exception thrown properly", correct);
-        jmri.util.JUnitAppender.assertErrorMessage("Invalid system name for turnout: JT needed JT");
+        jmri.util.JUnitAppender.assertErrorMessage("Invalid system name for Turnout: \"\" needed non-empty name to follow " + l.getSystemNamePrefix());
     }
 
     @Test