Skip to content

[mypy] Enforcing typing for superset.dashboards #9418

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.

Sign up for GitHub

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

Merged
dpgaspar merged 9 commits into from
Apr 7, 2020
Merged

[mypy] Enforcing typing for superset.dashboards #9418

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
67146c4
[mypy] Enforcing typing for superset.dashboards
dpgaspar Mar 30, 2020
96cb335
Merge remote-tracking branch 'upstream/master' into fix/mypy-dashboards
dpgaspar Apr 1, 2020
9585392
Make return types equal on all commands
dpgaspar Apr 1, 2020
9962c2c
Make return types equal on all commands
dpgaspar Apr 1, 2020
ac53820
Merge and conflicts, setup.cfg
dpgaspar Apr 6, 2020
d9e2180
[dashboard] address comments same return type on commands
dpgaspar Apr 6, 2020
c18dd50
lint
dpgaspar Apr 6, 2020
7ab7f96
lint
dpgaspar Apr 6, 2020
ce6b62e
Merge remote-tracking branch 'upstream/master' into fix/mypy-dashboards
dpgaspar Apr 7, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion setup.cfg
View file
Open in desktop
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.bin.*,superset.charts.*,superset.commands.*,superset.common.*,superset.dao.*,superset.db_engine_specs.*,superset.db_engines.*,superset.examples.*]
[mypy-superset.bin.*,superset.charts.*,superset.dashboards.*,superset.commands.*,superset.common.*,superset.dao.*,superset.db_engine_specs.*,superset.db_engines.*,superset.examples.*]
check_untyped_defs = true
disallow_untyped_calls = true
disallow_untyped_defs = true
4 changes: 2 additions & 2 deletions superset/charts/commands/delete.py
View file
Open in desktop
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

def run(self) -> Model:
self.validate()
Expand Down
4 changes: 2 additions & 2 deletions superset/charts/commands/update.py
View file
Open in desktop
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

def run(self) -> Model:
self.validate()
Expand Down
6 changes: 2 additions & 4 deletions superset/commands/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,7 @@
# specific language governing permissions and limitations
# under the License.
from abc import ABC, abstractmethod
from typing import Optional

from flask_appbuilder.models.sqla import Model
from typing import Any


class BaseCommand(ABC):
Expand All @@ -26,7 +24,7 @@ class BaseCommand(ABC):
"""

@abstractmethod
def run(self) -> Optional[Model]:
def run(self) -> Any:
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.

@john-bodley I'm setting base has Any, a bit wide, another option could be maintaining Optional[Model]

"""
Run executes the command. Can raise command exceptions
:raises: CommandException
Expand Down
7 changes: 5 additions & 2 deletions superset/dashboards/api.py
View file
Open in desktop
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
3 changes: 2 additions & 1 deletion superset/dashboards/commands/bulk_delete.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,10 +40,11 @@ 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 •
edited
Loading

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

@dpgaspar dpgaspar Mar 30, 2020

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

@dpgaspar dpgaspar Mar 30, 2020

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

@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.

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 •
edited
Loading

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

@john-bodley john-bodley Apr 6, 2020

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)
return None
except DeleteFailedError as e:
logger.exception(e.exception)
raise DashboardBulkDeleteFailedError()
Expand Down
3 changes: 2 additions & 1 deletion superset/dashboards/commands/create.py
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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

def run(self):
def run(self) -> Model:
self.validate()
try:
dashboard = DashboardDAO.update(self._model, self._properties)
Expand Down
13 changes: 7 additions & 6 deletions superset/dashboards/dao.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,13 +48,14 @@ def validate_update_slug_uniqueness(dashboard_id: int, slug: Optional[str]) -> b
return True

@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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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