[GitHub] [superset] villebro commented on a change in pull request #14684: chore: Perform feature/config condition checks at request time

[GitBox <gi...@apache.org>]

villebro commented on a change in pull request #14684:
URL: https://github.com/apache/superset/pull/14684#discussion_r636243376



##########
File path: superset/connectors/druid/views.py
##########
@@ -49,7 +51,18 @@
 logger = logging.getLogger(__name__)
 
 
-class DruidColumnInlineView(CompactCRUDMixin, SupersetModelView):
+class EnsureEnabled:

Review comment:
       nit: could this be called `EnsureEnabledMixin`?

##########
File path: superset/connectors/druid/views.py
##########
@@ -49,7 +51,18 @@
 logger = logging.getLogger(__name__)
 
 
-class DruidColumnInlineView(CompactCRUDMixin, SupersetModelView):
+class EnsureEnabled:
+    @staticmethod
+    def is_enabled() -> bool:
+        return bool(app.config["DRUID_IS_ACTIVE"])
+
+    @before_request
+    def ensure_enabled(self) -> None:
+        if not self.is_enabled():
+            raise NotFound()
+
+
+class DruidColumnInlineView(CompactCRUDMixin, SupersetModelView, EnsureEnabled):

Review comment:
       nit: while it really doesn't matter here, `EnsureEnabled` should probably come to the left of `SupersetModelView` as per convention (same for the others).

##########
File path: superset/connectors/sqla/views.py
##########
@@ -369,6 +371,15 @@ class RowLevelSecurityFiltersModelView(  # pylint: disable=too-many-ancestors
         add_form_query_rel_fields = app.config["RLS_FORM_QUERY_REL_FIELDS"]
         edit_form_query_rel_fields = add_form_query_rel_fields
 
+    @staticmethod
+    def is_enabled() -> bool:
+        return is_feature_enabled("ROW_LEVEL_SECURITY")
+
+    @before_request
+    def ensure_enabled(self) -> None:
+        if not self.is_enabled():
+            raise NotFound()

Review comment:
       We could potentially generalize this by breaking this out into a Mixin that checks `is_feature_enabled` on a reserved property. Something like this:
   
   ```python
   class FeatureFlagMixin:
       feature_flag: str
   
       def is_enabled(self) -> bool:
           return is_feature_enabled(self.feature_flag)
   
       @before_request
       def ensure_enabled(self) -> None:
           if not self.is_enabled():
               raise NotFound()
   ```
   
   Then later this could be used as follows:
   
   ```python
   class RowLevelSecurityFiltersModelView(  # pylint: disable=too-many-ancestors
       FeatureFlagMixin, SupersetModelView, DeleteMixin
   ):
       feature_flag = "ROW_LEVEL_SECURITY"
       ....
   ```
   we could potentially also add support for callbacks that make it possible to add custom validators that don't rely on `is_feature_enabled`:
   
   ```python
   class FeatureFlagMixin:
       feature_flag: Union[str, Callable[[], str]]
   
       def is_enabled(self) -> bool:
           if callable(self.feature_flag):
               return self.feature_flag()
           return is_feature_enabled(self.feature_flag)
   
       @before_request
       def ensure_enabled(self) -> None:
           if not self.is_enabled():
               raise NotFound()
   ```
   
   and then use it like this:
   
   ```python
       feature_flag = lambda _: app.config["FAB_ADD_SECURITY_VIEWS"] and app.config["SUPERSET_LOG_VIEW"]
   ```




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org