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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
