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

feat(explore): allow opening charts with missing dataset #12705

Merged
merged 4 commits into from
Jan 25, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ describe('DatasourceControl', () => {
const wrapper = setup();
const alert = wrapper.find(Icon);
expect(alert.at(1).prop('name')).toBe('alert-solid');
const tooltip = wrapper.find(Tooltip).at(1);
const tooltip = wrapper.find(Tooltip).at(0);
expect(tooltip.prop('title')).toBe(
defaultProps.datasource.health_check_message,
);
Expand Down
1 change: 1 addition & 0 deletions superset-frontend/src/common/components/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import { DropDownProps } from 'antd/lib/dropdown';
*/
// eslint-disable-next-line no-restricted-imports
export {
Alert,
Copy link
Member Author

Choose a reason for hiding this comment

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

Wanted to use Antd Alert, but it looks inconsistent with the error box in chart container.

AutoComplete,
Avatar,
Button,
Expand Down
4 changes: 2 additions & 2 deletions superset-frontend/src/explore/components/Control.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,9 @@ export type ControlProps = {
// signature to the original action factory.
actions: Partial<ExploreActions> & Pick<ExploreActions, 'setControlValue'>;
type: ControlType;
label: string;
label?: ReactNode;
name: string;
description?: string;
description?: ReactNode;
tooltipOnClick?: () => ReactNode;
places?: number;
rightNode?: ReactNode;
Expand Down
62 changes: 15 additions & 47 deletions superset-frontend/src/explore/components/DatasourcePanel.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -17,57 +17,25 @@
* under the License.
*/
import React, { useEffect, useState } from 'react';
import { styled, t, QueryFormData } from '@superset-ui/core';
import { styled, t } from '@superset-ui/core';
import { Collapse } from 'src/common/components';
import {
ColumnOption,
MetricOption,
ControlType,
ControlConfig,
DatasourceMeta,
} from '@superset-ui/chart-controls';
import { debounce } from 'lodash';
import { matchSorter, rankings } from 'match-sorter';
import { ExploreActions } from '../actions/exploreActions';
import Control from './Control';

interface DatasourceControl {
validationErrors: Array<any>;
mapStateToProps: QueryFormData;
type: ControlType;
label: string;
datasource?: DatasourceControl;
interface DatasourceControl extends ControlConfig {
datasource?: DatasourceMeta;
Copy link
Member Author

Choose a reason for hiding this comment

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

Bycatch: reuse typing from @superset-ui/chart-controls.

}

type Columns = {
column_name: string;
description: string | undefined;
expression: string | undefined;
filterable: boolean;
groupby: string | undefined;
id: number;
is_dttm: boolean;
python_date_format: string;
type: string;
verbose_name: string;
};

type Metrics = {
certification_details: string | undefined;
certified_by: string | undefined;
d3format: string | undefined;
description: string | undefined;
expression: string;
id: number;
is_certified: boolean;
metric_name: string;
verbose_name: string;
warning_text: string;
};

interface Props {
datasource: {
columns: Array<Columns>;
metrics: Array<Metrics>;
};
datasource: DatasourceMeta;
controls: {
datasource: DatasourceControl;
};
Expand Down Expand Up @@ -228,15 +196,8 @@ export default function DataSourcePanel({
const metricSlice = lists.metrics.slice(0, 50);
const columnSlice = lists.columns.slice(0, 50);

return (
<DatasourceContainer>
<Control
{...datasourceControl}
name="datasource"
validationErrors={datasourceControl.validationErrors}
actions={actions}
formData={datasourceControl.mapStateToProps}
/>
const mainBody = (
<>
<input
type="text"
onChange={evt => {
Expand Down Expand Up @@ -279,6 +240,13 @@ export default function DataSourcePanel({
</Collapse.Panel>
</Collapse>
</div>
</>
);

return (
<DatasourceContainer>
<Control {...datasourceControl} name="datasource" actions={actions} />
{datasource.id != null && mainBody}
Copy link
Member Author

Choose a reason for hiding this comment

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

Datasource ID is null when it is unavailable.

</DatasourceContainer>
);
}
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ import Icon from 'src/components/Icon';
import ChangeDatasourceModal from 'src/datasource/ChangeDatasourceModal';
import DatasourceModal from 'src/datasource/DatasourceModal';
import { postForm } from 'src/explore/exploreUtils';
import Button from 'src/components/Button';
import ErrorAlert from 'src/components/ErrorMessage/ErrorAlert';

const propTypes = {
actions: PropTypes.object.isRequired,
Expand All @@ -51,6 +53,9 @@ const Styles = styled.div`
border-bottom: 1px solid ${({ theme }) => theme.colors.grayscale.light2};
padding: ${({ theme }) => 2 * theme.gridUnit}px;
}
.error-alert {
margin: ${({ theme }) => 2 * theme.gridUnit}px;
}
.ant-dropdown-trigger {
margin-left: ${({ theme }) => 2 * theme.gridUnit}px;
box-shadow: none;
Expand Down Expand Up @@ -152,6 +157,7 @@ class DatasourceControl extends React.PureComponent {
render() {
const { showChangeDatasourceModal, showEditDatasourceModal } = this.state;
const { datasource, onChange } = this.props;
const isMissingDatasource = datasource;
const datasourceMenu = (
<Menu onClick={this.handleMenuItemClick}>
{this.props.isEditable && (
Expand All @@ -164,16 +170,22 @@ class DatasourceControl extends React.PureComponent {
</Menu>
);

// eslint-disable-next-line camelcase
const { health_check_message: healthCheckMessage } = datasource;

return (
<Styles className="DatasourceControl">
<div className="data-container">
<Icon name="dataset-physical" className="dataset-svg" />
<Tooltip title={datasource.name}>
<span className="title-select">{datasource.name}</span>
</Tooltip>
{/* Add a tooltip only for long dataset names */}
{!isMissingDatasource && datasource.name.length > 25 ? (
<Tooltip title={datasource.name}>
<span className="title-select">{datasource.name}</span>
</Tooltip>
) : (
<span title={datasource.name} className="title-select">
{datasource.name}
</span>
)}
{healthCheckMessage && (
<Tooltip title={healthCheckMessage}>
<Icon
Expand All @@ -196,6 +208,35 @@ class DatasourceControl extends React.PureComponent {
</Tooltip>
</Dropdown>
</div>
{/* missing dataset */}
{isMissingDatasource && (
<div className="error-alert">
<ErrorAlert
level="warning"
title={t('Missing dataset')}
source="explore"
subtitle={
<>
<p>
{t(
'The dataset linked to this chart may have been deleted.',
Copy link
Member

Choose a reason for hiding this comment

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

Could we be more explicit here as in the other error - "Dataset does not exist".

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 thinking is this: the dataset does not exist for two reasons, either it never existed or has been deleted. Since the latter is the most likely case, we inform users about this possibility, but also show reservations with "may" to be more accurate.

I think a little bit more color in copy is more helpful comparing to repeating the same general message over and over.

)}
</p>
<p>
<Button
buttonStyle="primary"
onClick={() =>
this.handleMenuItemClick({ key: CHANGE_DATASET })
}
>
{t('Change dataset')}
</Button>
</p>
</>
}
/>
</div>
)}
{showEditDatasourceModal && (
<DatasourceModal
datasource={datasource}
Expand Down
2 changes: 1 addition & 1 deletion superset/commands/exceptions.py
Original file line number Diff line number Diff line change
Expand Up @@ -89,4 +89,4 @@ class DatasourceNotFoundValidationError(ValidationError):
status = 404

def __init__(self) -> None:
super().__init__([_("Datasource does not exist")], field_name="datasource_id")
super().__init__([_("Dataset does not exist")], field_name="datasource_id")
4 changes: 2 additions & 2 deletions superset/commands/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,14 @@
from typing import List, Optional

from flask_appbuilder.security.sqla.models import User
from sqlalchemy.orm.exc import NoResultFound

from superset.commands.exceptions import (
DatasourceNotFoundValidationError,
OwnersNotFoundValidationError,
)
from superset.connectors.base.models import BaseDatasource
from superset.connectors.connector_registry import ConnectorRegistry
from superset.datasets.commands.exceptions import DatasetNotFoundError
from superset.extensions import db, security_manager


Expand Down Expand Up @@ -53,5 +53,5 @@ def get_datasource_by_id(datasource_id: int, datasource_type: str) -> BaseDataso
return ConnectorRegistry.get_datasource(
datasource_type, datasource_id, db.session
)
except (NoResultFound, KeyError):
except DatasetNotFoundError:
raise DatasourceNotFoundValidationError()
17 changes: 15 additions & 2 deletions superset/connectors/connector_registry.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@
from sqlalchemy import or_
from sqlalchemy.orm import Session, subqueryload

from superset.datasets.commands.exceptions import DatasetNotFoundError

if TYPE_CHECKING:
from collections import OrderedDict

Expand All @@ -44,12 +46,23 @@ def register_sources(cls, datasource_config: "OrderedDict[str, List[str]]") -> N
def get_datasource(
cls, datasource_type: str, datasource_id: int, session: Session
) -> "BaseDatasource":
return (
"""Safely get a datasource instance, raises `DatasetNotFoundError` if
`datasource_type` is not registered or `datasource_id` does not
exist."""
if datasource_type not in cls.sources:
raise DatasetNotFoundError()
Copy link
Member Author

Choose a reason for hiding this comment

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

handle_api_exception will be able to render the error properly.


datasource = (
session.query(cls.sources[datasource_type])
.filter_by(id=datasource_id)
.one()
.one_or_none()
)

if not datasource:
raise DatasetNotFoundError()

return datasource

@classmethod
def get_all_datasources(cls, session: Session) -> List["BaseDatasource"]:
datasources: List["BaseDatasource"] = []
Expand Down
4 changes: 2 additions & 2 deletions superset/connectors/druid/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@
BaseSupersetView,
DatasourceFilter,
DeleteMixin,
get_datasource_exist_error_msg,
get_dataset_exist_error_msg,
ListWidgetWithCheckboxes,
SupersetModelView,
validate_json,
Expand Down Expand Up @@ -352,7 +352,7 @@ def pre_add(self, item: "DruidDatasourceModelView") -> None:
models.DruidDatasource.cluster_id == item.cluster_id,
)
if db.session.query(query.exists()).scalar():
raise Exception(get_datasource_exist_error_msg(item.full_name))
raise Exception(get_dataset_exist_error_msg(item.full_name))

def post_add(self, item: "DruidDatasourceModelView") -> None:
item.refresh_metrics()
Expand Down
1 change: 1 addition & 0 deletions superset/datasets/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -231,6 +231,7 @@ def post(self) -> Response:
# This validates custom Schema with custom validations
except ValidationError as error:
return self.response_400(message=error.messages)

try:
new_model = CreateDatasetCommand(g.user, item).run()
return self.response(201, id=new_model.id, result=item)
Expand Down
10 changes: 7 additions & 3 deletions superset/datasets/commands/exceptions.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,10 @@
ImportFailedError,
UpdateFailedError,
)
from superset.views.base import get_datasource_exist_error_msg
Copy link
Member Author

Choose a reason for hiding this comment

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

Move to reduce the change of circular imports.



def get_dataset_exist_error_msg(full_name: str) -> str:
return _("Dataset %(name)s already exists", name=full_name)


class DatabaseNotFoundValidationError(ValidationError):
Expand Down Expand Up @@ -54,7 +57,7 @@ class DatasetExistsValidationError(ValidationError):

def __init__(self, table_name: str) -> None:
super().__init__(
get_datasource_exist_error_msg(table_name), field_name="table_name"
[get_dataset_exist_error_msg(table_name)], field_name="table_name"
Copy link
Member

Choose a reason for hiding this comment

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

Nice catch 👍

)


Expand Down Expand Up @@ -142,7 +145,8 @@ def __init__(self) -> None:


class DatasetNotFoundError(CommandException):
message = "Dataset not found."
status = 404
message = _("Dataset does not exist")


class DatasetInvalidError(CommandInvalidError):
Expand Down
15 changes: 7 additions & 8 deletions superset/datasets/commands/importers/v0.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@
import yaml
from flask_appbuilder import Model
from sqlalchemy.orm import Session
from sqlalchemy.orm.exc import NoResultFound
from sqlalchemy.orm.session import make_transient

from superset import db
Expand Down Expand Up @@ -56,14 +55,14 @@ def lookup_sqla_table(table: SqlaTable) -> Optional[SqlaTable]:


def lookup_sqla_database(table: SqlaTable) -> Optional[Database]:
try:
return (
db.session.query(Database)
.filter_by(database_name=table.params_dict["database_name"])
.one()
)
except NoResultFound:
database = (
db.session.query(Database)
.filter_by(database_name=table.params_dict["database_name"])
.one_or_none()
)
if database is None:
raise DatabaseNotFoundError
return database


def lookup_druid_cluster(datasource: DruidDatasource) -> Optional[DruidCluster]:
Expand Down
2 changes: 1 addition & 1 deletion superset/models/slice.py
Original file line number Diff line number Diff line change
Expand Up @@ -206,7 +206,7 @@ def digest(self) -> str:
"""
Returns a MD5 HEX digest that makes this dashboard unique
"""
return utils.md5_hex(self.params)
return utils.md5_hex(self.params or "")
Copy link
Member

Choose a reason for hiding this comment

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

Not that it matters, but I've always been allergic to md5 on empty string:

>>> import hashlib
>>> hashlib.md5(''.encode()).hexdigest()
'd41d8cd98f00b204e9800998ecf8427e'
Suggested change
return utils.md5_hex(self.params or "")
return utils.md5_hex(self.params) if self.params else ''

Copy link
Member Author

Choose a reason for hiding this comment

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

I think I'll return None here just to be more explicit. The original change is for avoiding an uncaught exception, explicitly returning None forces the downstream handle the case of missing params more properly.

Copy link
Member Author

Choose a reason for hiding this comment

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

Nvm. I'll keep utils.md5_hex(self.params or "") since this key is also used in thumbnail URLs:

        return f"/api/v1/chart/{self.id}/thumbnail/{self.digest}/"

We can make a follow up PR to rename digest to thumbnail_key (for both chart and dashboard) if we think it's necessary.


@property
def thumbnail_url(self) -> str:
Expand Down
7 changes: 2 additions & 5 deletions superset/models/tags.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@
from sqlalchemy import Column, Enum, ForeignKey, Integer, String
from sqlalchemy.engine.base import Connection
from sqlalchemy.orm import relationship, Session, sessionmaker
from sqlalchemy.orm.exc import NoResultFound
from sqlalchemy.orm.mapper import Mapper

from superset.models.helpers import AuditMixinNullable
Expand Down Expand Up @@ -89,13 +88,11 @@ class TaggedObject(Model, AuditMixinNullable):


def get_tag(name: str, session: Session, type_: TagTypes) -> Tag:
try:
tag = session.query(Tag).filter_by(name=name, type=type_).one()
except NoResultFound:
tag = session.query(Tag).filter_by(name=name, type=type_).one_or_none()
if tag is None:
tag = Tag(name=name, type=type_)
session.add(tag)
session.commit()

return tag


Expand Down
Loading