fix: feature flags typing (#15254)

* fix: feature flags typing

* fix tests

* add note in UPDATING.md

* fix frontend

* also move SCHEDULED_QUERIES to top level

* fix test

Co-authored-by: Ville Brofeldt <ville.v.brofeldt@gmail.com>

UPDATING.md

@@ -26,6 +26,7 @@ assists people when migrating to a new version.
 
 ### Breaking Changes
 
+- [15254](https://github.com/apache/superset/pull/15254): Previously `QUERY_COST_FORMATTERS_BY_ENGINE`, `SQL_VALIDATORS_BY_ENGINE` and `SCHEDULED_QUERIES` were expected to be defined in the feature flag dictionary in the `config.py` file. These should now be defined as a top-level config, with the feature flag dictionary being reserved for boolean only values.
 - [17290](https://github.com/apache/superset/pull/17290): Bumps pandas to `1.3.4` and pyarrow to `5.0.0`
 - [16660](https://github.com/apache/incubator-superset/pull/16660): The `columns` Jinja parameter has been renamed `table_columns` to make the `columns` query object parameter available in the Jinja context.
 - [16711](https://github.com/apache/incubator-superset/pull/16711): The `url_param` Jinja function will now by default escape the result. For instance, the value `O'Brien` will now be changed to `O''Brien`. To disable this behavior, call `url_param` with `escape_result` set to `False`: `url_param("my_key", "my default", escape_result=False)`.

superset-frontend/src/SqlLab/components/ScheduleQueryButton/index.tsx

@@ -26,6 +26,12 @@ import { Form, FormItem } from 'src/components/Form';
 import './ScheduleQueryButton.less';
 import Button from 'src/components/Button';
 
+const appContainer = document.getElementById('app');
+const bootstrapData = JSON.parse(
+  appContainer?.getAttribute('data-bootstrap') || '{}',
+);
+const scheduledQueriesConf = bootstrapData?.common?.conf?.SCHEDULED_QUERIES;
+
 const validators = {
   greater: (a: number, b: number) => a > b,
   greater_equal: (a: number, b: number) => a >= b,
@@ -34,7 +40,7 @@ const validators = {
 };
 
 const getJSONSchema = () => {
-  const jsonSchema = window.featureFlags.SCHEDULED_QUERIES?.JSONSCHEMA;
+  const jsonSchema = scheduledQueriesConf?.JSONSCHEMA;
   // parse date-time into usable value (eg, 'today' => `new Date()`)
   if (jsonSchema) {
     Object.entries(jsonSchema.properties).forEach(
@@ -52,10 +58,9 @@ const getJSONSchema = () => {
   return {};
 };
 
-const getUISchema = () => window.featureFlags.SCHEDULED_QUERIES?.UISCHEMA;
+const getUISchema = () => scheduledQueriesConf?.UISCHEMA;
 
-const getValidationRules = () =>
-  window.featureFlags.SCHEDULED_QUERIES?.VALIDATION || [];
+const getValidationRules = () => scheduledQueriesConf?.VALIDATION || [];
 
 const getValidator = () => {
   const rules: any = getValidationRules();

superset-frontend/src/SqlLab/components/SqlEditor/index.jsx

@@ -82,6 +82,14 @@ const SET_QUERY_EDITOR_SQL_DEBOUNCE_MS = 2000;
 const VALIDATION_DEBOUNCE_MS = 600;
 const WINDOW_RESIZE_THROTTLE_MS = 100;
 
+const appContainer = document.getElementById('app');
+const bootstrapData = JSON.parse(
+  appContainer.getAttribute('data-bootstrap') || '{}',
+);
+const validatorMap =
+  bootstrapData?.common?.conf?.SQL_VALIDATORS_BY_ENGINE || {};
+const scheduledQueriesConf = bootstrapData?.common?.conf?.SCHEDULED_QUERIES;
+
 const LimitSelectStyled = styled.span`
   .ant-dropdown-trigger {
     align-items: center;
@@ -391,8 +399,7 @@ class SqlEditor extends React.PureComponent {
   canValidateQuery() {
     // Check whether or not we can validate the current query based on whether
     // or not the backend has a validator configured for it.
-    const validatorMap = window.featureFlags.SQL_VALIDATORS_BY_ENGINE;
-    if (this.props.database && validatorMap != null) {
+    if (this.props.database) {
       return validatorMap.hasOwnProperty(this.props.database.backend);
     }
     return false;
@@ -531,7 +538,7 @@ class SqlEditor extends React.PureComponent {
             />
           </Menu.Item>
         )}
-        {isFeatureEnabled(FeatureFlag.SCHEDULED_QUERIES) && (
+        {scheduledQueriesConf && (
           <Menu.Item>
             <ScheduleQueryButton
               defaultLabel={qe.title}

superset-frontend/src/dashboard/components/Dashboard.jsx

@@ -99,7 +99,7 @@ class Dashboard extends React.PureComponent {
 
   componentDidMount() {
     const appContainer = document.getElementById('app');
-    const bootstrapData = appContainer?.getAttribute('data-bootstrap') || '';
+    const bootstrapData = appContainer?.getAttribute('data-bootstrap') || '{}';
     const { dashboardState, layout } = this.props;
     const eventData = {
       is_soft_navigation: Logger.timeOriginOffset > 0,

superset-frontend/src/showSavedQuery/index.jsx

@@ -26,7 +26,7 @@ const scheduleInfoContainer = document.getElementById('schedule-info');
 const bootstrapData = JSON.parse(
   scheduleInfoContainer.getAttribute('data-bootstrap'),
 );
-const config = bootstrapData.common.feature_flags.SCHEDULED_QUERIES;
+const config = bootstrapData.common.conf.SCHEDULED_QUERIES;
 const { query } = bootstrapData.common;
 const scheduleInfo = query.extra_json.schedule_info;
 const linkback = config.linkback ? interpolate(config.linkback, query) : null;

superset/config.py

@@ -798,10 +798,12 @@ class CeleryConfig:  # pylint: disable=too-few-public-methods
 #
 #     return out
 #
-# FEATURE_FLAGS = {
-#     "ESTIMATE_QUERY_COST": True,
-#     "QUERY_COST_FORMATTERS_BY_ENGINE": {"postgresql": postgres_query_cost_formatter},
-# }
+#  Then on define the formatter on the config:
+#
+# "QUERY_COST_FORMATTERS_BY_ENGINE": {"postgresql": postgres_query_cost_formatter},
+QUERY_COST_FORMATTERS_BY_ENGINE: Dict[
+    str, Callable[[List[Dict[str, Any]]], List[Dict[str, Any]]]
+] = {}
 
 # Flag that controls if limit should be enforced on the CTA (create table as queries).
 SQLLAB_CTAS_NO_LIMIT = False

superset/utils/feature_flag_manager.py

@@ -15,7 +15,7 @@
 # specific language governing permissions and limitations
 # under the License.
 from copy import deepcopy
-from typing import Any, Dict
+from typing import Dict
 
 from flask import Flask
 
@@ -25,15 +25,15 @@ def __init__(self) -> None:
         super().__init__()
         self._get_feature_flags_func = None
         self._is_feature_enabled_func = None
-        self._feature_flags: Dict[str, Any] = {}
+        self._feature_flags: Dict[str, bool] = {}
 
     def init_app(self, app: Flask) -> None:
         self._get_feature_flags_func = app.config["GET_FEATURE_FLAGS_FUNC"]
         self._is_feature_enabled_func = app.config["IS_FEATURE_ENABLED_FUNC"]
         self._feature_flags = app.config["DEFAULT_FEATURE_FLAGS"]
         self._feature_flags.update(app.config["FEATURE_FLAGS"])
 
-    def get_feature_flags(self) -> Dict[str, Any]:
+    def get_feature_flags(self) -> Dict[str, bool]:
         if self._get_feature_flags_func:
             return self._get_feature_flags_func(deepcopy(self._feature_flags))
         if callable(self._is_feature_enabled_func):

superset/views/base.py

@@ -96,6 +96,7 @@
     "DISPLAY_MAX_ROW",
     "GLOBAL_ASYNC_QUERIES_TRANSPORT",
     "GLOBAL_ASYNC_QUERIES_POLLING_DELAY",
+    "SQL_VALIDATORS_BY_ENGINE",
     "SQLALCHEMY_DOCS_URL",
     "SQLALCHEMY_DISPLAY_TEXT",
     "GLOBAL_ASYNC_QUERIES_WEBSOCKET_URL",

superset/views/core.py

@@ -51,7 +51,6 @@
     conf,
     db,
     event_logger,
-    get_feature_flags,
     is_feature_enabled,
     results_backend,
     results_backend_use_msgpack,
@@ -2221,9 +2220,9 @@ def estimate_query_cost(  # pylint: disable=no-self-use
             return json_error_response(utils.error_msg_from_exception(ex))
 
         spec = mydb.db_engine_spec
-        query_cost_formatters: Dict[str, Any] = get_feature_flags().get(
-            "QUERY_COST_FORMATTERS_BY_ENGINE", {}
-        )
+        query_cost_formatters: Dict[str, Any] = app.config[
+            "QUERY_COST_FORMATTERS_BY_ENGINE"
+        ]
         query_cost_formatter = query_cost_formatters.get(
             spec.engine, spec.query_cost_formatter
         )
@@ -2414,7 +2413,7 @@ def validate_sql_json(
             )
 
         spec = mydb.db_engine_spec
-        validators_by_engine = get_feature_flags().get("SQL_VALIDATORS_BY_ENGINE")
+        validators_by_engine = app.config["SQL_VALIDATORS_BY_ENGINE"]
         if not validators_by_engine or spec.engine not in validators_by_engine:
             return json_error_response(
                 "no SQL validator is configured for {}".format(spec.engine), status=400

tests/integration_tests/sql_validator_tests.py

@@ -34,13 +34,11 @@
 
 from .base_tests import SupersetTestCase
 
-PRESTO_TEST_FEATURE_FLAGS = {
-    "SQL_VALIDATORS_BY_ENGINE": {
-        "presto": "PrestoDBSQLValidator",
-        "sqlite": "PrestoDBSQLValidator",
-        "postgresql": "PrestoDBSQLValidator",
-        "mysql": "PrestoDBSQLValidator",
-    }
+PRESTO_SQL_VALIDATORS_BY_ENGINE = {
+    "presto": "PrestoDBSQLValidator",
+    "sqlite": "PrestoDBSQLValidator",
+    "postgresql": "PrestoDBSQLValidator",
+    "mysql": "PrestoDBSQLValidator",
 }
 
 
@@ -65,8 +63,8 @@ def test_validate_sql_endpoint_noconfig(self):
 
     @patch("superset.views.core.get_validator_by_name")
     @patch.dict(
-        "superset.extensions.feature_flag_manager._feature_flags",
-        PRESTO_TEST_FEATURE_FLAGS,
+        "superset.config.SQL_VALIDATORS_BY_ENGINE",
+        PRESTO_SQL_VALIDATORS_BY_ENGINE,
         clear=True,
     )
     def test_validate_sql_endpoint_mocked(self, get_validator_by_name):
@@ -98,8 +96,8 @@ def test_validate_sql_endpoint_mocked(self, get_validator_by_name):
 
     @patch("superset.views.core.get_validator_by_name")
     @patch.dict(
-        "superset.extensions.feature_flag_manager._feature_flags",
-        PRESTO_TEST_FEATURE_FLAGS,
+        "superset.config.SQL_VALIDATORS_BY_ENGINE",
+        PRESTO_SQL_VALIDATORS_BY_ENGINE,
         clear=True,
     )
     def test_validate_sql_endpoint_failure(self, get_validator_by_name):