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!: pass datasource_type and datasource_id to form_data #19981

Merged
merged 5 commits into from
Jun 2, 2022
Merged
Changes from 1 commit
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
Prev Previous commit
Next Next commit
add datasource_type to delete command
  • Loading branch information
eschutho committed Jun 1, 2022

Verified

This commit was signed with the committer’s verified signature.
snyk-bot Snyk bot
commit 021b79704f2333c3f6e95829eaca9488b892a2d7
2 changes: 1 addition & 1 deletion scripts/tests/run.sh
Original file line number Diff line number Diff line change
@@ -138,5 +138,5 @@ fi

if [ $RUN_TESTS -eq 1 ]
then
pytest --durations=0 --maxfail=1 "${TEST_MODULE}"
pytest --durations=0 --maxfail=10 "${TEST_MODULE}"
fi
5 changes: 5 additions & 0 deletions superset-frontend/src/constants.ts
Original file line number Diff line number Diff line change
@@ -71,6 +71,10 @@ export const URL_PARAMS = {
name: 'datasource_id',
type: 'string',
},
datasetId: {
name: 'dataset_id',
type: 'string',
},
datasourceType: {
name: 'datasource_type',
type: 'string',
@@ -94,6 +98,7 @@ export const RESERVED_CHART_URL_PARAMS: string[] = [
URL_PARAMS.sliceId.name,
URL_PARAMS.datasourceId.name,
URL_PARAMS.datasourceType.name,
Copy link
Member Author

Choose a reason for hiding this comment

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

@michael-s-molina this could be an old URL, too? I'll datasetId back in here..

URL_PARAMS.datasetId.name,
];
export const RESERVED_DASHBOARD_URL_PARAMS: string[] = [
URL_PARAMS.nativeFilters.name,
Original file line number Diff line number Diff line change
@@ -190,9 +190,8 @@ class DatasourceControl extends React.PureComponent {
let isMissingParams = false;
if (isMissingDatasource) {
const datasourceId = getUrlParam(URL_PARAMS.datasourceId);
const datasourceType = getUrlParam(URL_PARAMS.datasourceType);
const sliceId = getUrlParam(URL_PARAMS.sliceId);
if (!datasourceId && !sliceId && !datasourceType) {
if (!datasourceId && !sliceId) {
isMissingParams = true;
}
}
3 changes: 2 additions & 1 deletion superset/cachekeys/schemas.py
Original file line number Diff line number Diff line change
@@ -22,6 +22,7 @@
datasource_type_description,
datasource_uid_description,
)
from superset.utils.core import DatasourceType


class Datasource(Schema):
@@ -36,7 +37,7 @@ class Datasource(Schema):
)
datasource_type = fields.String(
description=datasource_type_description,
validate=validate.OneOf(choices=("druid", "table", "view")),
validate=validate.OneOf(choices=[ds.value for ds in DatasourceType]),
required=True,
)

7 changes: 4 additions & 3 deletions superset/charts/schemas.py
Original file line number Diff line number Diff line change
@@ -31,6 +31,7 @@
from superset.utils import pandas_postprocessing, schema as utils
from superset.utils.core import (
AnnotationType,
DatasourceType,
FilterOperator,
PostProcessingBoxplotWhiskerType,
PostProcessingContributionOrientation,
@@ -198,7 +199,7 @@ class ChartPostSchema(Schema):
datasource_id = fields.Integer(description=datasource_id_description, required=True)
datasource_type = fields.String(
description=datasource_type_description,
validate=validate.OneOf(choices=("druid", "table", "view")),
validate=validate.OneOf(choices=[ds.value for ds in DatasourceType]),
required=True,
)
datasource_name = fields.String(
@@ -244,7 +245,7 @@ class ChartPutSchema(Schema):
)
datasource_type = fields.String(
description=datasource_type_description,
validate=validate.OneOf(choices=("druid", "table", "view")),
validate=validate.OneOf(choices=[ds.value for ds in DatasourceType]),
allow_none=True,
)
dashboards = fields.List(fields.Integer(description=dashboards_description))
@@ -983,7 +984,7 @@ class ChartDataDatasourceSchema(Schema):
)
type = fields.String(
description="Datasource type",
validate=validate.OneOf(choices=("druid", "table")),
validate=validate.OneOf(choices=[ds.value for ds in DatasourceType]),
)


16 changes: 16 additions & 0 deletions superset/commands/exceptions.py
Original file line number Diff line number Diff line change
@@ -115,8 +115,24 @@ def __init__(self) -> None:
super().__init__([_("Some roles do not exist")], field_name="roles")


class DatasourceTypeInvalidError(ValidationError):
status = 422

def __init__(self) -> None:
super().__init__(
[_("Datasource type is invalid")], field_name="datasource_type"
)


class DatasourceNotFoundValidationError(ValidationError):
status = 404

def __init__(self) -> None:
super().__init__([_("Datasource does not exist")], field_name="datasource_id")


class QueryNotFoundValidationError(ValidationError):
status = 404

def __init__(self) -> None:
super().__init__([_("Query does not exist")], field_name="datasource_id")
6 changes: 3 additions & 3 deletions superset/dao/datasource/dao.py
Original file line number Diff line number Diff line change
@@ -39,11 +39,11 @@
class DatasourceDAO(BaseDAO):

sources: Dict[DatasourceType, Type[Datasource]] = {
DatasourceType.SQLATABLE: SqlaTable,
DatasourceType.TABLE: SqlaTable,
DatasourceType.QUERY: Query,
DatasourceType.SAVEDQUERY: SavedQuery,
DatasourceType.DATASET: Dataset,
DatasourceType.TABLE: Table,
DatasourceType.SLTABLE: Table,
}

@classmethod
@@ -66,7 +66,7 @@ def get_datasource(

@classmethod
def get_all_sqlatables_datasources(cls, session: Session) -> List[Datasource]:
source_class = DatasourceDAO.sources[DatasourceType.SQLATABLE]
source_class = DatasourceDAO.sources[DatasourceType.TABLE]
qry = session.query(source_class)
qry = source_class.default_query(qry)
return qry.all()
3 changes: 1 addition & 2 deletions superset/explore/form_data/commands/create.py
Original file line number Diff line number Diff line change
@@ -47,8 +47,7 @@ def run(self) -> str:
form_data = self._cmd_params.form_data
check_access(datasource_id, chart_id, actor, datasource_type)
contextual_key = cache_key(
session.get(
"_id"), tab_id, datasource_id, chart_id, datasource_type
session.get("_id"), tab_id, datasource_id, chart_id, datasource_type
)
key = cache_manager.explore_form_data_cache.get(contextual_key)
if not key or not tab_id:
8 changes: 7 additions & 1 deletion superset/explore/form_data/commands/delete.py
Original file line number Diff line number Diff line change
@@ -32,6 +32,7 @@
TemporaryCacheDeleteFailedError,
)
from superset.temporary_cache.utils import cache_key
from superset.utils.core import DatasourceType

logger = logging.getLogger(__name__)

@@ -50,14 +51,19 @@ def run(self) -> bool:
if state:
datasource_id: int = state["datasource_id"]
chart_id: Optional[int] = state["chart_id"]
datasource_type: str = state["datasource_type"]
datasource_type = DatasourceType(state["datasource_type"])
check_access(datasource_id, chart_id, actor, datasource_type)
if state["owner"] != get_owner(actor):
raise TemporaryCacheAccessDeniedError()
tab_id = self._cmd_params.tab_id
contextual_key = cache_key(
session.get("_id"), tab_id, datasource_id, chart_id, datasource_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.

todo: I still need to write up something here for deleting old keys. Unless it's ok that it just expires on it's own. The user will get a 404 though.

Copy link
Member

@hughhhh hughhhh May 31, 2022

Choose a reason for hiding this comment

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

Since we are generating new keys for misses, i think it's fine to let the keys expire

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, let's do that, it's only for delete and update. Unless anyone else has thoughts.

)
if contextual_key is None:
# check again with old keys
contextual_key = cache_key(
session.get("_id"), tab_id, datasource_id, chart_id
)
cache_manager.explore_form_data_cache.delete(contextual_key)
return cache_manager.explore_form_data_cache.delete(key)
return False
3 changes: 2 additions & 1 deletion superset/explore/form_data/commands/get.py
Original file line number Diff line number Diff line change
@@ -27,6 +27,7 @@
from superset.explore.form_data.commands.utils import check_access
from superset.extensions import cache_manager
from superset.temporary_cache.commands.exceptions import TemporaryCacheGetFailedError
from superset.utils.core import DatasourceType

logger = logging.getLogger(__name__)

@@ -49,7 +50,7 @@ def run(self) -> Optional[str]:
state["datasource_id"],
state["chart_id"],
actor,
state["datasource_type"],
DatasourceType(state["datasource_type"]),
)
if self._refresh_timeout:
cache_manager.explore_form_data_cache.set(key, state)
4 changes: 3 additions & 1 deletion superset/explore/form_data/commands/parameters.py
Original file line number Diff line number Diff line change
@@ -19,11 +19,13 @@

from flask_appbuilder.security.sqla.models import User

from superset.utils.core import DatasourceType


@dataclass
class CommandParameters:
actor: User
datasource_type: str = ""
datasource_type: DatasourceType = DatasourceType.TABLE
datasource_id: int = 0
Copy link
Member Author

@eschutho eschutho May 7, 2022

Choose a reason for hiding this comment

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

I'm not sure why we have datasource_id as required with a default to 0, so I kept with the same pattern and set datasource_type to an empty string. It almost seems like we should have two different classes here when a key exists and when it doesn't so that we can be more explicit about what is required in each case.

Copy link
Member

Choose a reason for hiding this comment

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

I think it's a good idea to break it into multiple classes 👍🏼

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 agree, but if it's ok, I'll put this into a different PR since it's currently one class, and outside this scope.

chart_id: int = 0
tab_id: Optional[int] = None
11 changes: 7 additions & 4 deletions superset/explore/form_data/commands/update.py
Original file line number Diff line number Diff line change
@@ -65,14 +65,17 @@ def run(self) -> Optional[str]:
# Generate a new key if tab_id changes or equals 0
tab_id = self._cmd_params.tab_id
contextual_key = cache_key(
session.get(
"_id"), tab_id, datasource_id, chart_id, datasource_type
session.get("_id"), tab_id, datasource_id, chart_id, datasource_type
Copy link
Member

Choose a reason for hiding this comment

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

It will miss the cache here for old keys because they were generated without the datasource_type. We can add logic to also query for the old format or assume that new keys will be created and think about how to clean the old entries.

Copy link
Member Author

@eschutho eschutho May 13, 2022

Choose a reason for hiding this comment

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

So currently on update, if the first key can't be found, it will create a new key and return it. That seems ok to me.. do you think that could work for this temporary cache?

)
if contextual_key is None:
# check again with old keys
contextual_key = cache_key(
session.get("_id"), tab_id, datasource_id, chart_id
)
key = cache_manager.explore_form_data_cache.get(contextual_key)
if not key or not tab_id:
key = random_key()
cache_manager.explore_form_data_cache.set(
contextual_key, key)
cache_manager.explore_form_data_cache.set(contextual_key, key)

new_state: TemporaryExploreState = {
"owner": owner,
8 changes: 7 additions & 1 deletion superset/explore/form_data/commands/utils.py
Original file line number Diff line number Diff line change
@@ -31,9 +31,15 @@
TemporaryCacheAccessDeniedError,
TemporaryCacheResourceNotFoundError,
)
from superset.utils.core import DatasourceType


def check_access(datasource_id: int, chart_id: Optional[int], actor: User, datasource_type: str) -> None:
def check_access(
datasource_id: int,
chart_id: Optional[int],
actor: User,
datasource_type: DatasourceType,
) -> None:
try:
explore_check_access(datasource_id, chart_id, actor, datasource_type)
except (ChartNotFoundError, DatasetNotFoundError) as ex:
14 changes: 11 additions & 3 deletions superset/explore/form_data/schemas.py
Original file line number Diff line number Diff line change
@@ -14,15 +14,20 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
from marshmallow import fields, Schema
from marshmallow import fields, Schema, validate

from superset.utils.core import DatasourceType


class FormDataPostSchema(Schema):
datasource_id = fields.Integer(
required=True, allow_none=False, description="The datasource ID"
)
datasource_type = fields.String(
required=True, allow_none=False, description="The datasource type"
required=True,
allow_none=False,
description="The datasource type",
validate=validate.OneOf(choices=[ds.value for ds in DatasourceType]),
)
chart_id = fields.Integer(required=False, description="The chart ID")
form_data = fields.String(
@@ -35,7 +40,10 @@ class FormDataPutSchema(Schema):
required=True, allow_none=False, description="The datasource ID"
)
datasource_type = fields.String(
required=True, allow_none=False, description="The datasource type"
required=True,
allow_none=False,
description="The datasource type",
validate=validate.OneOf(choices=[ds.value for ds in DatasourceType]),
)
chart_id = fields.Integer(required=False, description="The chart ID")
form_data = fields.String(
10 changes: 5 additions & 5 deletions superset/explore/permalink/commands/create.py
Original file line number Diff line number Diff line change
@@ -25,6 +25,7 @@
from superset.explore.utils import check_access as check_chart_access
from superset.key_value.commands.create import CreateKeyValueCommand
from superset.key_value.utils import encode_permalink_key
from superset.utils.core import DatasourceType

logger = logging.getLogger(__name__)

@@ -39,9 +40,9 @@ def __init__(self, actor: User, state: Dict[str, Any]):
def run(self) -> str:
self.validate()
try:
datasource = self.datasource.split("__")
datasource_id: int = int(datasource[0])
datasource_type: str = datasource[1]
d_id, d_type = self.datasource.split("__")
datasource_id = int(d_id)
datasource_type = DatasourceType(d_type)
check_chart_access(
datasource_id, self.chart_id, self.actor, datasource_type
)
@@ -59,8 +60,7 @@ def run(self) -> str:
)
key = command.run()
if key.id is None:
raise ExplorePermalinkCreateFailedError(
"Unexpected missing key id")
raise ExplorePermalinkCreateFailedError("Unexpected missing key id")
return encode_permalink_key(key=key.id, salt=self.salt)
except SQLAlchemyError as ex:
logger.exception("Error running create command")
6 changes: 3 additions & 3 deletions superset/explore/permalink/commands/get.py
Original file line number Diff line number Diff line change
@@ -28,6 +28,7 @@
from superset.key_value.commands.get import GetKeyValueCommand
from superset.key_value.exceptions import KeyValueGetFailedError, KeyValueParseKeyError
from superset.key_value.utils import decode_permalink_id
from superset.utils.core import DatasourceType

logger = logging.getLogger(__name__)

@@ -48,9 +49,8 @@ def run(self) -> Optional[ExplorePermalinkValue]:
if value:
chart_id: Optional[int] = value.get("chartId")
datasource_id: int = value["datasourceId"]
datasource_type: str = value["datasourceType"]
check_chart_access(datasource_id, chart_id,
self.actor, datasource_type)
datasource_type = DatasourceType(value["datasourceType"])
check_chart_access(datasource_id, chart_id, self.actor, datasource_type)
return value
return None
except (
Loading