Encourage optional feature flag names to work as startup props

Author: labkey-adam

api/src/org/labkey/api/settings/OptionalFeatureFlag.java

@@ -12,24 +12,47 @@ public class OptionalFeatureFlag implements Comparable<OptionalFeatureFlag>, Sta
     private static final Logger LOG = LogHelper.getLogger(OptionalFeatureFlag.class, "Warnings about optional feature flag names");
 
     private final String _flag;
+    private final String _propertyName;
     private final String _title;
     private final String _description;
     private final boolean _requiresRestart;
     private final boolean _hidden;
     private final FeatureType _type;
 
     /**
-     * @param flag must be unique. Can be used as a startup property to enable/disable the task, but only if it follows
-     *             the Java identifier rules (e.g., alphanumeric plus _, start with a letter, no spaces).
+     * @param flag must be unique and conform to the Java identifier rules (e.g., alphanumeric plus _, start with a
+     *             letter, no spaces). That way it can be used as a startup property to enable/disable the task.
      */
     public OptionalFeatureFlag(String flag, String title, String description, boolean requiresRestart, boolean hidden, FeatureType type)
+    {
+        this(flag, title, description, requiresRestart, hidden, type, false);
+    }
+
+    OptionalFeatureFlag(String flag, String title, String description, boolean requiresRestart, boolean hidden, FeatureType type, boolean useDumbName /* true means allow a name that can't be used as a startup property name */)
     {
         _flag = flag;
         _title = title;
         _description = description;
         _requiresRestart = requiresRestart;
         _hidden = hidden;
         _type = type;
+        if (!StringUtilsLabKey.isValidJavaIdentifier(_flag))
+        {
+            if (useDumbName)
+            {
+                _propertyName = null;
+            }
+            else
+            {
+                // TODO: Switch to throw IllegalStateException
+                LOG.error(_flag + " is not a valid Java identifier. Correct it so it can be used as a startup property. Or set useDumbName to true.", new IllegalStateException());
+                _propertyName = null;
+            }
+        }
+        else
+        {
+            _propertyName = _flag;
+        }
     }
 
     public String getFlag()
@@ -83,14 +106,6 @@ public FeatureType getType()
     @Override
     public String getPropertyName()
     {
-        String name = getFlag();
-        if (!StringUtilsLabKey.isValidJavaIdentifier(name))
-        {
-            // This is a developer thing... no need to log a warning on production deployments
-            if (AppProps.getInstance().isDevMode())
-                LOG.warn("Feature flag name doesn't conform to the property name rules so it won't be available as a startup property: {}", name);
-            name = null;
-        }
-        return name;
+        return _propertyName;
     }
 }

api/src/org/labkey/api/settings/OptionalFeatureService.java

@@ -43,12 +43,21 @@ static void setInstance(OptionalFeatureService impl)
     }
 
     /**
-     * @param flag must be unique. Can be used as a startup property to enable/disable the task, but only if it follows
-     *             the Java identifier rules (e.g., alphanumeric plus _, start with a letter, no spaces).
+     * @param flag must be unique and conform to the Java identifier rules (e.g., alphanumeric plus _, start with a
+     *             letter, no spaces). That way it can be used as a startup property to enable/disable the task. If
+     *             you must use a flag that doesn't conform to these rules (why?) the call the other variant.
      */
     default void addExperimentalFeatureFlag(String flag, String title, String description, boolean requiresRestart)
     {
-        addFeatureFlag(new OptionalFeatureFlag(flag, title, description, requiresRestart, false, FeatureType.Experimental));
+        addExperimentalFeatureFlag(flag, title, description, requiresRestart, false);
+    }
+
+    /**
+     * This is left for backward compatibility. Use the variant above and provide flag that follows Java identifier rules.
+     */
+    default void addExperimentalFeatureFlag(String flag, String title, String description, boolean requiresRestart, boolean useDumbName)
+    {
+        addFeatureFlag(new OptionalFeatureFlag(flag, title, description, requiresRestart, false, FeatureType.Experimental, useDumbName));
     }
 
     void addFeatureFlag(OptionalFeatureFlag optionalFeatureFlag);

core/src/org/labkey/core/CoreModule.java

@@ -529,19 +529,20 @@ public QuerySchema createSchema(DefaultSchema schema, Module module)
         }
 
         OptionalFeatureService.get().addExperimentalFeatureFlag(NotificationMenuView.EXPERIMENTAL_NOTIFICATION_MENU, "Notifications Menu",
-            "Notifications 'inbox' count display in the header bar with click to show the notifications panel of unread notifications.", false);
+            "Notifications 'inbox' count display in the header bar with click to show the notifications panel of unread notifications.", false, true);
         OptionalFeatureService.get().addExperimentalFeatureFlag(DataColumn.EXPERIMENTAL_USE_QUERYSELECT_COMPONENT, "Use QuerySelect for row insert/update form",
             "This feature will switch the query based select inputs on the row insert/update form to use the React QuerySelect" +
             "component. This will allow for a user to view the first 100 options in the select but then use type ahead" +
-            "search to find the other select values.", false);
+            "search to find the other select values.", false, true);
         OptionalFeatureService.get().addExperimentalFeatureFlag(SQLFragment.FEATUREFLAG_DISABLE_STRICT_CHECKS, "Disable SQLFragment strict checks",
-            "SQLFragment now has very strict usage validation, these checks may cause errors in code that has not been updated. Turn on this feature to disable checks.", false);
+            "SQLFragment now has very strict usage validation, these checks may cause errors in code that has not been updated. Turn on this feature to disable checks.", false, true);
         OptionalFeatureService.get().addExperimentalFeatureFlag(LoginController.FEATUREFLAG_DISABLE_LOGIN_XFRAME, "Disable Login X-FRAME-OPTIONS=DENY",
-            "By default LabKey disables all framing of login related actions. Disabling this feature will revert to using the standard site settings.", false);
+            "By default LabKey disables all framing of login related actions. Disabling this feature will revert to using the standard site settings.", false, true);
         OptionalFeatureService.get().addExperimentalFeatureFlag(PageTemplate.EXPERIMENTAL_SHORT_CIRCUIT_ROBOTS,
             "Short-circuit robots",
             "Save resources by not rendering pages marked as 'noindex' for robots. This is experimental as not all robots are search engines.",
-            false);
+            false,
+            true);
         OptionalFeatureService.get().addExperimentalFeatureFlag(AppProps.REJECT_CONTROLLER_FIRST_URLS,
             "Reject controller-first URLs",
             "Require standard path-first URLs.",

devtools/src/org/labkey/devtools/DevtoolsModule.java

@@ -21,7 +21,6 @@
 import org.labkey.api.module.CodeOnlyModule;
 import org.labkey.api.module.ModuleContext;
 import org.labkey.api.security.AuthenticationManager;
-import org.labkey.api.settings.OptionalFeatureFlag;
 import org.labkey.api.settings.OptionalFeatureService;
 import org.labkey.api.util.JspTestCase;
 import org.labkey.api.view.WebPartFactory;
@@ -62,10 +61,10 @@ protected void init()
         addController("testsso", TestSsoController.class);
         AuthenticationManager.registerProvider(new TestSsoProvider());
 
-        OptionalFeatureService.get().addFeatureFlag(new OptionalFeatureFlag(Domain.EXPERIMENTAL_FUZZ_STORAGE_NAME,
+        OptionalFeatureService.get().addExperimentalFeatureFlag(Domain.EXPERIMENTAL_FUZZ_STORAGE_NAME,
             "'fuzz' name of database columns used to back domain properties",
             "This is dev/test feature and not intended for any production usage.",
-            false, false, OptionalFeatureService.FeatureType.Experimental));
+            false, true);
     }
 
     @Override

experiment/src/org/labkey/experiment/ExperimentModule.java

@@ -262,7 +262,7 @@ protected void init()
         ExperimentService.get().registerNameExpressionType("dataclass", "exp", "DataClass", "nameexpression");
 
         OptionalFeatureService.get().addExperimentalFeatureFlag(AppProps.EXPERIMENTAL_RESOLVE_PROPERTY_URI_COLUMNS, "Resolve property URIs as columns on experiment tables",
-            "If a column is not found on an experiment table, attempt to resolve the column name as a Property URI and add it as a property column", false);
+            "If a column is not found on an experiment table, attempt to resolve the column name as a Property URI and add it as a property column", false, true);
         if (CoreSchema.getInstance().getSqlDialect().isSqlServer())
         {
             OptionalFeatureService.get().addExperimentalFeatureFlag(NameGenerator.EXPERIMENTAL_WITH_COUNTER, "Use strict incremental withCounter and rootSampleCount expression",
@@ -282,9 +282,9 @@ protected void init()
         OptionalFeatureService.get().addExperimentalFeatureFlag(AppProps.QUANTITY_COLUMN_SUFFIX_TESTING, "Quantity column suffix testing",
                 "If a column name contains a \"__<unit>\" suffix, this feature allows for testing it as a Quantity display column", false);
         OptionalFeatureService.get().addExperimentalFeatureFlag(ExperimentService.EXPERIMENTAL_FEATURE_FROM_EXPANCESTORS, "SQL syntax: 'FROM EXPANCESTORS()'",
-                "Support for querying lineage of experiment objects", false);
+                "Support for querying lineage of experiment objects", false, true);
         OptionalFeatureService.get().addExperimentalFeatureFlag(SampleTypeUpdateServiceDI.EXPERIMENTAL_FEATURE_ALLOW_ROW_ID_SAMPLE_MERGE, "Allow RowId to be accepted when merging samples",
-                "If the incoming data includes a RowId column we will allow the column but ignore it's values.", false);
+                "If the incoming data includes a RowId column we will allow the column but ignore it's values.", false, true);
 
         RoleManager.registerPermission(new DesignVocabularyPermission(), true);
         RoleManager.registerRole(new SampleTypeDesignerRole());

query/src/org/labkey/query/QueryModule.java

@@ -237,7 +237,7 @@ public QuerySchema createSchema(DefaultSchema schema, Module module)
         DataViewService.get().registerProvider(InheritedQueryDataViewProvider.TYPE, new InheritedQueryDataViewProvider());
 
         OptionalFeatureService.get().addExperimentalFeatureFlag(QueryView.EXPERIMENTAL_GENERIC_DETAILS_URL, "Generic [details] link in grids/queries",
-            "This feature will turn on generating a generic [details] URL link in most grids.", false);
+            "This feature will turn on generating a generic [details] URL link in most grids.", false, true);
         OptionalFeatureService.get().addExperimentalFeatureFlag(QueryServiceImpl.EXPERIMENTAL_LAST_MODIFIED, "Include Last-Modified header on query metadata requests",
             "For schema, query, and view metadata requests include a Last-Modified header such that the browser can cache the response. " +
             "The metadata is invalidated when performing actions such as creating a new List or modifying the columns on a custom view", false);