Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[mypy] Enforcing typing for superset.dashboards #9418

Merged
merged 9 commits into from
Apr 7, 2020
2 changes: 1 addition & 1 deletion setup.cfg
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ order_by_type = false
ignore_missing_imports = true
no_implicit_optional = true

[mypy-superset.charts.*,superset.db_engine_specs.*]
[mypy-superset.charts.*,superset.dashboards.*,superset.db_engine_specs.*]
check_untyped_defs = true
disallow_untyped_calls = true
disallow_untyped_defs = true
4 changes: 2 additions & 2 deletions superset/charts/commands/delete.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,9 @@
)
from superset.charts.dao import ChartDAO
from superset.commands.base import BaseCommand
from superset.connectors.sqla.models import SqlaTable
from superset.dao.exceptions import DAODeleteFailedError
from superset.exceptions import SupersetSecurityException
from superset.models.slice import Slice
from superset.views.base import check_ownership

logger = logging.getLogger(__name__)
Expand All @@ -39,7 +39,7 @@ class DeleteChartCommand(BaseCommand):
def __init__(self, user: User, model_id: int):
self._actor = user
self._model_id = model_id
self._model: Optional[SqlaTable] = None
self._model: Optional[Slice] = None
dpgaspar marked this conversation as resolved.
Show resolved Hide resolved

def run(self) -> Model:
self.validate()
Expand Down
4 changes: 2 additions & 2 deletions superset/charts/commands/update.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,10 +32,10 @@
from superset.charts.dao import ChartDAO
from superset.commands.base import BaseCommand
from superset.commands.utils import get_datasource_by_id, populate_owners
from superset.connectors.sqla.models import SqlaTable
from superset.dao.exceptions import DAOUpdateFailedError
from superset.dashboards.dao import DashboardDAO
from superset.exceptions import SupersetSecurityException
from superset.models.slice import Slice
from superset.views.base import check_ownership

logger = logging.getLogger(__name__)
Expand All @@ -46,7 +46,7 @@ def __init__(self, user: User, model_id: int, data: Dict):
self._actor = user
self._model_id = model_id
self._properties = data.copy()
self._model: Optional[SqlaTable] = None
self._model: Optional[Slice] = None
villebro marked this conversation as resolved.
Show resolved Hide resolved

def run(self) -> Model:
self.validate()
Expand Down
7 changes: 5 additions & 2 deletions superset/dashboards/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
# specific language governing permissions and limitations
# under the License.
import logging
from typing import Any

from flask import g, make_response, request, Response
from flask_appbuilder.api import expose, protect, rison, safe
Expand Down Expand Up @@ -285,7 +286,9 @@ def delete(self, pk: int) -> Response: # pylint: disable=arguments-differ
@protect()
@safe
@rison(get_delete_ids_schema)
def bulk_delete(self, **kwargs) -> Response: # pylint: disable=arguments-differ
def bulk_delete(
self, **kwargs: Any
) -> Response: # pylint: disable=arguments-differ
"""Delete bulk Dashboards
---
delete:
Expand Down Expand Up @@ -343,7 +346,7 @@ def bulk_delete(self, **kwargs) -> Response: # pylint: disable=arguments-differ
@protect()
@safe
@rison(get_export_ids_schema)
def export(self, **kwargs):
def export(self, **kwargs: Any) -> Response:
"""Export dashboards
---
get:
Expand Down
2 changes: 1 addition & 1 deletion superset/dashboards/commands/bulk_delete.py
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ def __init__(self, user: User, model_ids: List[int]):
self._model_ids = model_ids
self._models: Optional[List[Dashboard]] = None

def run(self):
def run(self) -> None:
Copy link
Member

@john-bodley john-bodley Mar 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be Optional[Model] per #9416? Note I presume that the run methods should have consistent return type.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My take is that the abstract is defined has Optional[Model] and the implementations can return None or Model. If we set (for example) create.run -> Optional[Model]` then I should check for the model's existence on the return from: https://github.com/apache/incubator-superset/blob/master/superset/dashboards/api.py#L164.

And I'm assuming that the commands succeed and return or raise. What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the other hand we can make them more coherent and always expect Optional[Model] this can make future implementation more safely "blind" to the command ifself, just run run

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer the later, i.e., all the run signatures having the same return type. It helps to ensures consistency both now and for future classes.

Copy link
Member Author

@dpgaspar dpgaspar Apr 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ended up reverting typing on command to a more explicit type, following @villebro comments and reasoning around:

  • We want to migrate logic implemented on models to commands and the return types for Command.run() will vary a lot
  • Optional[Model] will not cover all future use cases and will force us to implement logic that is unnecessary such has testing if the run method returned something, when it will always return of raise
  • We will not be able to implement a single return type for all Command.run() unless creating a very big type definition

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dpgaspar would you mind updating this line as well to ensure consistency?

self.validate()
try:
DashboardDAO.bulk_delete(self._models)
Expand Down
3 changes: 2 additions & 1 deletion superset/dashboards/commands/create.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import logging
from typing import Dict, List, Optional

from flask_appbuilder.models.sqla import Model
from flask_appbuilder.security.sqla.models import User
from marshmallow import ValidationError

Expand All @@ -38,7 +39,7 @@ def __init__(self, user: User, data: Dict):
self._actor = user
self._properties = data.copy()

def run(self):
def run(self) -> Model:
self.validate()
try:
dashboard = DashboardDAO.create(self._properties)
Expand Down
3 changes: 2 additions & 1 deletion superset/dashboards/commands/delete.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import logging
from typing import Optional

from flask_appbuilder.models.sqla import Model
from flask_appbuilder.security.sqla.models import User

from superset.commands.base import BaseCommand
Expand All @@ -40,7 +41,7 @@ def __init__(self, user: User, model_id: int):
self._model_id = model_id
self._model: Optional[Dashboard] = None

def run(self):
def run(self) -> Model:
self.validate()
try:
dashboard = DashboardDAO.delete(self._model)
Expand Down
2 changes: 1 addition & 1 deletion superset/dashboards/commands/exceptions.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ class DashboardSlugExistsValidationError(ValidationError):
Marshmallow validation error for dashboard slug already exists
"""

def __init__(self):
def __init__(self) -> None:
super().__init__(_("Must be unique"), field_names=["slug"])


Expand Down
7 changes: 4 additions & 3 deletions superset/dashboards/commands/update.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,12 @@
import logging
from typing import Dict, List, Optional

from flask_appbuilder.models.sqla import Model
from flask_appbuilder.security.sqla.models import User
from marshmallow import ValidationError

from superset.commands.base import BaseCommand
from superset.commands.utils import populate_owners
from superset.connectors.sqla.models import SqlaTable
from superset.dao.exceptions import DAOUpdateFailedError
from superset.dashboards.commands.exceptions import (
DashboardForbiddenError,
Expand All @@ -33,6 +33,7 @@
)
from superset.dashboards.dao import DashboardDAO
from superset.exceptions import SupersetSecurityException
from superset.models.dashboard import Dashboard
from superset.views.base import check_ownership

logger = logging.getLogger(__name__)
Expand All @@ -43,9 +44,9 @@ def __init__(self, user: User, model_id: int, data: Dict):
self._actor = user
self._model_id = model_id
self._properties = data.copy()
self._model: Optional[SqlaTable] = None
self._model: Optional[Dashboard] = None
dpgaspar marked this conversation as resolved.
Show resolved Hide resolved

def run(self):
def run(self) -> Model:
self.validate()
try:
dashboard = DashboardDAO.update(self._model, self._properties)
Expand Down
15 changes: 8 additions & 7 deletions superset/dashboards/dao.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
# specific language governing permissions and limitations
# under the License.
import logging
from typing import List
from typing import List, Optional

from sqlalchemy.exc import SQLAlchemyError

Expand Down Expand Up @@ -46,13 +46,14 @@ def validate_update_slug_uniqueness(dashboard_id: int, slug: str) -> bool:
return not db.session.query(dashboard_query.exists()).scalar()

@staticmethod
def bulk_delete(models: List[Dashboard], commit=True):
item_ids = [model.id for model in models]
def bulk_delete(models: Optional[List[Dashboard]], commit: bool = True) -> None:
item_ids = [model.id for model in models] if models else []
# bulk delete, first delete related data
for model in models:
model.slices = []
model.owners = []
db.session.merge(model)
if models:
for model in models:
model.slices = []
model.owners = []
db.session.merge(model)
# bulk delete itself
try:
db.session.query(Dashboard).filter(Dashboard.id.in_(item_ids)).delete(
Expand Down
5 changes: 4 additions & 1 deletion superset/dashboards/filters.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,10 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
from typing import Any

from sqlalchemy import and_, or_
from sqlalchemy.orm.query import Query

from superset import db, security_manager
from superset.models.core import FavStar
Expand All @@ -35,7 +38,7 @@ class DashboardFilter(BaseFilter): # pylint: disable=too-few-public-methods
if they wish to see those dashboards which are published first
"""

def apply(self, query, value):
def apply(self, query: Query, value: Any) -> Query:
user_roles = [role.name.lower() for role in list(get_user_roles())]
if "admin" in user_roles:
return query
Expand Down
7 changes: 4 additions & 3 deletions superset/dashboards/schemas.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
# under the License.
import json
import re
from typing import Any, Dict, Union

from marshmallow import fields, pre_load, Schema
from marshmallow.validate import Length, ValidationError
Expand All @@ -27,14 +28,14 @@
get_export_ids_schema = {"type": "array", "items": {"type": "integer"}}


def validate_json(value):
def validate_json(value: Union[bytes, bytearray, str]) -> None:
try:
utils.validate_json(value)
except SupersetException:
raise ValidationError("JSON not valid")


def validate_json_metadata(value):
def validate_json_metadata(value: Union[bytes, bytearray, str]) -> None:
if not value:
return
try:
Expand All @@ -60,7 +61,7 @@ class DashboardJSONMetadataSchema(Schema):

class BaseDashboardSchema(Schema):
@pre_load
def pre_load(self, data): # pylint: disable=no-self-use
def pre_load(self, data: Dict[str, Any]) -> None: # pylint: disable=no-self-use
if data.get("slug"):
data["slug"] = data["slug"].strip()
data["slug"] = data["slug"].replace(" ", "-")
Expand Down
8 changes: 5 additions & 3 deletions superset/views/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@
from flask_appbuilder.actions import action
from flask_appbuilder.forms import DynamicForm
from flask_appbuilder.models.sqla.filters import BaseFilter
from flask_appbuilder.security.sqla.models import User
from flask_appbuilder.security.sqla.models import Role, User
from flask_appbuilder.widgets import ListWidget
from flask_babel import get_locale, gettext as __, lazy_gettext as _
from flask_wtf.form import FlaskForm
Expand Down Expand Up @@ -89,7 +89,9 @@ def data_payload_response(payload_json, has_error=False):
return json_success(payload_json, status=status)


def generate_download_headers(extension, filename=None):
def generate_download_headers(
extension: str, filename: Optional[str] = None
) -> Dict[str, Any]:
filename = filename if filename else datetime.now().strftime("%Y%m%d_%H%M%S")
content_disp = f"attachment; filename={filename}.{extension}"
headers = {"Content-Disposition": content_disp}
Expand Down Expand Up @@ -146,7 +148,7 @@ def get_datasource_exist_error_msg(full_name):
return __("Datasource %(name)s already exists", name=full_name)


def get_user_roles():
def get_user_roles() -> List[Role]:
if g.user.is_anonymous:
public_role = conf.get("AUTH_ROLE_PUBLIC")
return [security_manager.find_role(public_role)] if public_role else []
Expand Down