You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2022/05/06 19:45:30 UTC
[GitHub] [superset] eschutho opened a new pull request, #19981: feat!: pass datasource_type and datasource_id to form_data
eschutho opened a new pull request, #19981:
URL: https://github.com/apache/superset/pull/19981
### SUMMARY
With SIP68 we will be deprecating the ConnectorRegistry and instead having a fixed set of datasources. We currently pass both a dataset_id and datasource_type in the form_data, but we are only passing in the dataset_id to the api itself. In cases where there is no form data, we usually default to a "table" datasource, but this PR allows us to be more flexible about having different types of datasources in the future and changes the api to pass in both the datasource_type and id.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
No visual changes
### TESTING INSTRUCTIONS
Existing tests have been updated and a few new tests added to account for existing form_data cache structure.
To test, you should be able to go through the entire explore flow without any issues.
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] hughhhh commented on pull request #19981: feat!: pass datasource_type and datasource_id to form_data
Posted by GitBox <gi...@apache.org>.
hughhhh commented on PR #19981:
URL: https://github.com/apache/superset/pull/19981#issuecomment-1145162322
🚢 🚢 🚢
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on a diff in pull request #19981: feat!: pass datasource_type and datasource_id to form_data
Posted by GitBox <gi...@apache.org>.
eschutho commented on code in PR #19981:
URL: https://github.com/apache/superset/pull/19981#discussion_r872895626
##########
tests/integration_tests/explore/form_data/api_tests.py:
##########
@@ -67,65 +67,91 @@ def dataset_id() -> int:
return dataset.id
+@pytest.fixture
Review Comment:
yeah, good call
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on a diff in pull request #19981: feat!: pass datasource_type and datasource_id to form_data
Posted by GitBox <gi...@apache.org>.
eschutho commented on code in PR #19981:
URL: https://github.com/apache/superset/pull/19981#discussion_r867275408
##########
superset/explore/form_data/commands/parameters.py:
##########
@@ -23,7 +23,8 @@
@dataclass
class CommandParameters:
actor: User
- dataset_id: int = 0
+ datasource_type: str = ""
+ datasource_id: int = 0
Review Comment:
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.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on a diff in pull request #19981: feat!: pass datasource_type and datasource_id to form_data
Posted by GitBox <gi...@apache.org>.
eschutho commented on code in PR #19981:
URL: https://github.com/apache/superset/pull/19981#discussion_r872890675
##########
superset/explore/form_data/commands/update.py:
##########
@@ -64,7 +65,7 @@ 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, dataset_id, chart_id
+ session.get("_id"), tab_id, datasource_id, chart_id, datasource_type
Review Comment:
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?
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] hughhhh commented on a diff in pull request #19981: feat!: pass datasource_type and datasource_id to form_data
Posted by GitBox <gi...@apache.org>.
hughhhh commented on code in PR #19981:
URL: https://github.com/apache/superset/pull/19981#discussion_r885763269
##########
superset/explore/form_data/commands/delete.py:
##########
@@ -47,14 +49,15 @@ def run(self) -> bool:
key
)
if state:
- dataset_id = state["dataset_id"]
- chart_id = state["chart_id"]
- check_access(dataset_id, chart_id, actor)
+ datasource_id: int = state["datasource_id"]
+ chart_id: Optional[int] = state["chart_id"]
+ 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, dataset_id, chart_id
+ session.get("_id"), tab_id, datasource_id, chart_id, datasource_type
Review Comment:
Since we are generating new keys for misses, i think it's fine to let the keys expire
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho closed pull request #19981: feat!: pass datasource_type and datasource_id to form_data
Posted by GitBox <gi...@apache.org>.
eschutho closed pull request #19981: feat!: pass datasource_type and datasource_id to form_data
URL: https://github.com/apache/superset/pull/19981
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on a diff in pull request #19981: feat!: pass datasource_type and datasource_id to form_data
Posted by GitBox <gi...@apache.org>.
eschutho commented on code in PR #19981:
URL: https://github.com/apache/superset/pull/19981#discussion_r872891133
##########
superset/explore/form_data/commands/update.py:
##########
@@ -64,7 +65,7 @@ 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, dataset_id, chart_id
+ session.get("_id"), tab_id, datasource_id, chart_id, datasource_type
Review Comment:
a cache key is just a uuid, right? It's not encrypted data?
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] hughhhh commented on a diff in pull request #19981: feat!: pass datasource_type and datasource_id to form_data
Posted by GitBox <gi...@apache.org>.
hughhhh commented on code in PR #19981:
URL: https://github.com/apache/superset/pull/19981#discussion_r885763269
##########
superset/explore/form_data/commands/delete.py:
##########
@@ -47,14 +49,15 @@ def run(self) -> bool:
key
)
if state:
- dataset_id = state["dataset_id"]
- chart_id = state["chart_id"]
- check_access(dataset_id, chart_id, actor)
+ datasource_id: int = state["datasource_id"]
+ chart_id: Optional[int] = state["chart_id"]
+ 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, dataset_id, chart_id
+ session.get("_id"), tab_id, datasource_id, chart_id, datasource_type
Review Comment:
Since we are generating new keys for misses, i think it's fine to let the keys expire.
I think it would be a lot of work to have another layer of remapping old keys to new keys.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] github-actions[bot] commented on pull request #19981: feat!: pass datasource_type and datasource_id to form_data
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #19981:
URL: https://github.com/apache/superset/pull/19981#issuecomment-1142321922
@jinghua-qa Ephemeral environment spinning up at http://54.213.76.156:8080. Credentials are `admin`/`admin`. Please allow several minutes for bootstrapping and startup.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on a diff in pull request #19981: feat!: pass datasource_type and datasource_id to form_data
Posted by GitBox <gi...@apache.org>.
eschutho commented on code in PR #19981:
URL: https://github.com/apache/superset/pull/19981#discussion_r867275408
##########
superset/explore/form_data/commands/parameters.py:
##########
@@ -23,7 +23,8 @@
@dataclass
class CommandParameters:
actor: User
- dataset_id: int = 0
+ datasource_type: str = ""
+ datasource_id: int = 0
Review Comment:
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.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on a diff in pull request #19981: feat!: pass datasource_type and datasource_id to form_data
Posted by GitBox <gi...@apache.org>.
eschutho commented on code in PR #19981:
URL: https://github.com/apache/superset/pull/19981#discussion_r868193785
##########
tests/integration_tests/explore/form_data/api_tests.py:
##########
@@ -67,65 +67,92 @@ def dataset_id() -> int:
return dataset.id
+@pytest.fixture
+def datasource_type() -> int:
+ with app.app_context() as ctx:
+ session: Session = ctx.app.appbuilder.get_session
+ dataset = (
+ session.query(SqlaTable)
+ .filter_by(table_name="wb_health_population")
+ .first()
+ )
+ return dataset.type
+
+
@pytest.fixture(autouse=True)
-def cache(chart_id, admin_id, dataset_id):
+def cache(chart_id, admin_id, datasource_id, datasource_type):
entry: TemporaryExploreState = {
"owner": admin_id,
- "dataset_id": dataset_id,
+ "datasource_id": datasource_id,
+ "datasource_type": datasource_type,
"chart_id": chart_id,
"form_data": INITIAL_FORM_DATA,
}
cache_manager.explore_form_data_cache.set(KEY, entry)
-def test_post(client, chart_id: int, dataset_id: int):
+def test_post(client, chart_id: int, datasource_id: int, datasource_type: str):
login(client, "admin")
payload = {
- "dataset_id": dataset_id,
+ "datasource_id": datasource_id,
+ "datasource_type": datasource_type,
"chart_id": chart_id,
"form_data": INITIAL_FORM_DATA,
}
resp = client.post("api/v1/explore/form_data", json=payload)
+ print(resp.data)
Review Comment:
ty
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on a diff in pull request #19981: feat!: pass datasource_type and datasource_id to form_data
Posted by GitBox <gi...@apache.org>.
eschutho commented on code in PR #19981:
URL: https://github.com/apache/superset/pull/19981#discussion_r872891817
##########
superset/explore/utils.py:
##########
@@ -24,15 +24,24 @@
ChartNotFoundError,
)
from superset.charts.dao import ChartDAO
+from superset.commands.exceptions import DatasourceNotFoundValidationError
from superset.datasets.commands.exceptions import (
DatasetAccessDeniedError,
DatasetNotFoundError,
)
from superset.datasets.dao import DatasetDAO
+from superset.utils.core import DatasourceType
from superset.views.base import is_user_admin
from superset.views.utils import is_owner
+def check_datasource_access(datasource_id: int, datasource_type: str) -> Optional[bool]:
+ if datasource_id:
+ if datasource_type == DatasourceType.TABLE:
Review Comment:
I removed this code because the query functionality isn't ready yet, but I think it may make sense to have it. I'll add it back in.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on pull request #19981: feat!: pass datasource_type and datasource_id to form_data
Posted by GitBox <gi...@apache.org>.
eschutho commented on PR #19981:
URL: https://github.com/apache/superset/pull/19981#issuecomment-1129419083
> @eschutho I'm a little lost, why do we need the `datasource_type` post the Druid NoSQL deprecation? i.e., this served merely to ensure the (`datasource_type`, `datasouce_id`) tuple was unique but given we only now have `datasource_type = 'table` this is irrelevant isn't it—unless this is somehow being used @betodealmeida's table/query schema refactor.
yeah, this is a precursor for https://github.com/apache/superset/issues/19953 where we'll be introducing other datasource types: tables, queries, savedqueries.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] jinghua-qa commented on pull request #19981: feat!: pass datasource_type and datasource_id to form_data
Posted by GitBox <gi...@apache.org>.
jinghua-qa commented on PR #19981:
URL: https://github.com/apache/superset/pull/19981#issuecomment-1144091046
Testing are mostly done, tested regression for the following behavior:
1, go from list view to explore and run a chart,
2, change the visualization and filters and run (update chart) for different chart type and then refresh the page.
3, check that the form fields for chart updates
4, Save chart, and add it to a dashboard
did not find major issues in explore, found 1 issue with chart share in dashboard, when i use the permalink of chart share to enter dashboard, the chart is not highlighted, i think this is the PR issue since i checked master, master have the chart highlighted effect
https://user-images.githubusercontent.com/81597121/171490407-5f1c00ae-bcdf-49de-80f7-3b4422698d03.mov
.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on a diff in pull request #19981: feat!: pass datasource_type and datasource_id to form_data
Posted by GitBox <gi...@apache.org>.
eschutho commented on code in PR #19981:
URL: https://github.com/apache/superset/pull/19981#discussion_r872893759
##########
superset/utils/cache_manager.py:
##########
@@ -15,15 +15,40 @@
# specific language governing permissions and limitations
# under the License.
import logging
+from typing import Any, Optional, Union
from flask import Flask
from flask_caching import Cache
+from markupsafe import Markup
+
+from superset.utils.core import DatasourceType
logger = logging.getLogger(__name__)
CACHE_IMPORT_PATH = "superset.extensions.metastore_cache.SupersetMetastoreCache"
+class ExploreFormDataCache(Cache):
+ def get(self, *args: Any, **kwargs: Any) -> Optional[Union[str, Markup]]:
+ cache = self.cache.get(*args, **kwargs)
Review Comment:
Ok, I added a check using the old keys for update and delete as well. I don't really think there's a good way to remap these, unfortunately. The cache persistence is configurable but the default is 7 days.. maybe we can delete this fallback code after three months or so.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on a diff in pull request #19981: feat!: pass datasource_type and datasource_id to form_data
Posted by GitBox <gi...@apache.org>.
eschutho commented on code in PR #19981:
URL: https://github.com/apache/superset/pull/19981#discussion_r872891817
##########
superset/explore/utils.py:
##########
@@ -24,15 +24,24 @@
ChartNotFoundError,
)
from superset.charts.dao import ChartDAO
+from superset.commands.exceptions import DatasourceNotFoundValidationError
from superset.datasets.commands.exceptions import (
DatasetAccessDeniedError,
DatasetNotFoundError,
)
from superset.datasets.dao import DatasetDAO
+from superset.utils.core import DatasourceType
from superset.views.base import is_user_admin
from superset.views.utils import is_owner
+def check_datasource_access(datasource_id: int, datasource_type: str) -> Optional[bool]:
+ if datasource_id:
+ if datasource_type == DatasourceType.TABLE:
Review Comment:
I'll leave a comment to make that more clear.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on a diff in pull request #19981: feat!: pass datasource_type and datasource_id to form_data
Posted by GitBox <gi...@apache.org>.
eschutho commented on code in PR #19981:
URL: https://github.com/apache/superset/pull/19981#discussion_r872891133
##########
superset/explore/form_data/commands/update.py:
##########
@@ -64,7 +65,7 @@ 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, dataset_id, chart_id
+ session.get("_id"), tab_id, datasource_id, chart_id, datasource_type
Review Comment:
a cache key is just a uuid, right? It's not encrypted data?
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michael-s-molina commented on a diff in pull request #19981: feat!: pass datasource_type and datasource_id to form_data
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on code in PR #19981:
URL: https://github.com/apache/superset/pull/19981#discussion_r870541833
##########
tests/integration_tests/utils/cache_manager_tests.py:
##########
@@ -0,0 +1,50 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+import pytest
+
+from superset.extensions import cache_manager
+from superset.utils.core import DatasourceType
+
Review Comment:
Can we add a test that checks for retrieval using keys that were generated without a datasource type?
##########
superset-frontend/src/explore/components/controls/DatasourceControl/index.jsx:
##########
@@ -189,9 +189,10 @@ class DatasourceControl extends React.PureComponent {
const isMissingDatasource = datasource.id == null;
let isMissingParams = false;
if (isMissingDatasource) {
- const datasetId = getUrlParam(URL_PARAMS.datasetId);
+ const datasourceId = getUrlParam(URL_PARAMS.datasourceId);
+ const datasourceType = getUrlParam(URL_PARAMS.datasourceType);
const sliceId = getUrlParam(URL_PARAMS.sliceId);
- if (!datasetId && !sliceId) {
+ if (!datasourceId && !sliceId && !datasourceType) {
Review Comment:
We need to consider old URLs that use the `datasetId`. Maybe read the old parameter and assign it to `datasourceId` and set `datasourceType` to `table`.
We also need to change the alert message `'The URL is missing the dataset_id or slice_id parameters.'` to also include the `dataset_type`.
##########
superset/utils/cache_manager.py:
##########
@@ -15,15 +15,40 @@
# specific language governing permissions and limitations
# under the License.
import logging
+from typing import Any, Optional, Union
from flask import Flask
from flask_caching import Cache
+from markupsafe import Markup
+
+from superset.utils.core import DatasourceType
logger = logging.getLogger(__name__)
CACHE_IMPORT_PATH = "superset.extensions.metastore_cache.SupersetMetastoreCache"
+class ExploreFormDataCache(Cache):
+ def get(self, *args: Any, **kwargs: Any) -> Optional[Union[str, Markup]]:
+ cache = self.cache.get(*args, **kwargs)
Review Comment:
There's a problem here with old keys because the `contextual_key` is now being generated with the `datasource_type` but the old keys didn't have it so it will miss the cache and return `None` for old keys.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michael-s-molina commented on a diff in pull request #19981: feat!: pass datasource_type and datasource_id to form_data
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on code in PR #19981:
URL: https://github.com/apache/superset/pull/19981#discussion_r870535403
##########
superset/explore/form_data/commands/update.py:
##########
@@ -64,7 +65,7 @@ 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, dataset_id, chart_id
+ session.get("_id"), tab_id, datasource_id, chart_id, datasource_type
Review Comment:
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.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on a diff in pull request #19981: feat!: pass datasource_type and datasource_id to form_data
Posted by GitBox <gi...@apache.org>.
eschutho commented on code in PR #19981:
URL: https://github.com/apache/superset/pull/19981#discussion_r868193505
##########
superset/explore/form_data/commands/delete.py:
##########
@@ -46,14 +48,15 @@ def run(self) -> bool:
key
)
if state:
- dataset_id = state["dataset_id"]
- chart_id = state["chart_id"]
- check_access(dataset_id, chart_id, actor)
+ datasource_id: int = state["datasource_id"]
+ chart_id: Optional[int] = state["chart_id"]
+ datasource_type: str = state["datasource_type"]
+ check_chart_access(datasource_id, chart_id, actor, datasource_type)
if state["owner"] != actor.get_user_id():
raise TemporaryCacheAccessDeniedError()
tab_id = self._cmd_params.tab_id
contextual_key = cache_key(
- session.get("_id"), tab_id, dataset_id, chart_id
+ session.get("_id"), tab_id, datasource_id, chart_id
Review Comment:
yes, thanks for catching!
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] github-actions[bot] commented on pull request #19981: feat!: pass datasource_type and datasource_id to form_data
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #19981:
URL: https://github.com/apache/superset/pull/19981#issuecomment-1138851527
@jinghua-qa Ephemeral environment spinning up at http://35.89.192.2:8080. Credentials are `admin`/`admin`. Please allow several minutes for bootstrapping and startup.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on a diff in pull request #19981: feat!: pass datasource_type and datasource_id to form_data
Posted by GitBox <gi...@apache.org>.
eschutho commented on code in PR #19981:
URL: https://github.com/apache/superset/pull/19981#discussion_r883195489
##########
tests/integration_tests/explore/form_data/commands_tests.py:
##########
@@ -0,0 +1,130 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from unittest.mock import patch
+
+import pytest
+
+from superset import app, db, security_manager
+from superset.connectors.sqla.models import SqlaTable
+from superset.explore.form_data.commands.create import CreateFormDataCommand
+from superset.explore.form_data.commands.parameters import CommandParameters
+from superset.explore.form_data.commands.update import UpdateFormDataCommand
+from superset.models.slice import Slice
+from superset.utils.core import DatasourceType, get_example_default_schema
+from superset.utils.database import get_example_database
+from tests.integration_tests.base_tests import SupersetTestCase
+
+
+class TestCreateFormDataCommand(SupersetTestCase):
+ @pytest.fixture()
+ def create_dataset(self):
+ with self.create_app().app_context():
+ dataset = SqlaTable(
+ table_name="dummy_sql_table",
+ database=get_example_database(),
+ schema=get_example_default_schema(),
+ sql="select 123 as intcol, 'abc' as strcol",
+ )
+ session = db.session
+ session.add(dataset)
+ session.commit()
+
+ yield dataset
+
+ # rollback
+ session.delete(dataset)
+ session.commit()
+
+ @pytest.fixture()
+ def create_slice(self):
+ with self.create_app().app_context():
+ session = db.session
+ dataset = (
+ session.query(SqlaTable).filter_by(table_name="dummy_sql_table").first()
+ )
+ slice = Slice(
+ datasource_id=dataset.id,
+ datasource_type=DatasourceType.TABLE,
+ datasource_name="tmp_perm_table",
+ slice_name="slice_name",
+ )
+
+ session.add(slice)
+ session.commit()
+
+ yield slice
+
+ # rollback
+ session.delete(slice)
+ session.commit()
+
+ @patch("superset.security.manager.g")
+ @pytest.mark.usefixtures("create_dataset", "create_slice")
+ def test_create_form_data_command(self, mock_g):
+ mock_g.user = security_manager.find_user("admin")
+
+ dataset = (
+ db.session.query(SqlaTable).filter_by(table_name="dummy_sql_table").first()
+ )
+ slice = db.session.query(Slice).filter_by(slice_name="slice_name").first()
+ args = CommandParameters(
+ actor=mock_g.user,
+ datasource_id=dataset.id,
+ datasource_type=DatasourceType.TABLE,
+ chart_id=slice.id,
+ tab_id=1,
+ form_data="",
+ )
+ command = CreateFormDataCommand(args)
+
+ assert isinstance(command.run(), str)
+
+
+# TODO
Review Comment:
I'd like to finish writing up these tests, too.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on a diff in pull request #19981: feat!: pass datasource_type and datasource_id to form_data
Posted by GitBox <gi...@apache.org>.
eschutho commented on code in PR #19981:
URL: https://github.com/apache/superset/pull/19981#discussion_r883195235
##########
superset/explore/form_data/commands/delete.py:
##########
@@ -47,14 +49,15 @@ def run(self) -> bool:
key
)
if state:
- dataset_id = state["dataset_id"]
- chart_id = state["chart_id"]
- check_access(dataset_id, chart_id, actor)
+ datasource_id: int = state["datasource_id"]
+ chart_id: Optional[int] = state["chart_id"]
+ 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, dataset_id, chart_id
+ session.get("_id"), tab_id, datasource_id, chart_id, datasource_type
Review Comment:
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.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] jinghua-qa commented on pull request #19981: feat!: pass datasource_type and datasource_id to form_data
Posted by GitBox <gi...@apache.org>.
jinghua-qa commented on PR #19981:
URL: https://github.com/apache/superset/pull/19981#issuecomment-1142306119
/testenv up
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov[bot] commented on pull request #19981: feat!: pass datasource_type and datasource_id to form_data
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on PR #19981:
URL: https://github.com/apache/superset/pull/19981#issuecomment-1120033074
# [Codecov](https://codecov.io/gh/apache/superset/pull/19981?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#19981](https://codecov.io/gh/apache/superset/pull/19981?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (026c064) into [master](https://codecov.io/gh/apache/superset/commit/fdf48c63f1220607b9390104b04ba1d9fae7bfcf?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (fdf48c6) will **decrease** coverage by `12.36%`.
> The diff coverage is `57.69%`.
> :exclamation: Current head 026c064 differs from pull request most recent head 70339a3. Consider uploading reports for the commit 70339a3 to get more accurate results
```diff
@@ Coverage Diff @@
## master #19981 +/- ##
===========================================
- Coverage 66.54% 54.18% -12.37%
===========================================
Files 1714 1712 -2
Lines 65102 64001 -1101
Branches 6725 6725
===========================================
- Hits 43321 34677 -8644
- Misses 20069 27612 +7543
Partials 1712 1712
```
| Flag | Coverage Δ | |
|---|---|---|
| hive | `?` | |
| mysql | `?` | |
| postgres | `?` | |
| presto | `53.50% <48.33%> (+0.74%)` | :arrow_up: |
| python | `57.32% <58.33%> (-25.11%)` | :arrow_down: |
| sqlite | `?` | |
| unit | `48.64% <56.66%> (+0.58%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/19981?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [.../src/dashboard/components/gridComponents/Chart.jsx](https://codecov.io/gh/apache/superset/pull/19981/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL2dyaWRDb21wb25lbnRzL0NoYXJ0LmpzeA==) | `60.60% <ø> (ø)` | |
| [...re/components/controls/DatasourceControl/index.jsx](https://codecov.io/gh/apache/superset/pull/19981/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9jb250cm9scy9EYXRhc291cmNlQ29udHJvbC9pbmRleC5qc3g=) | `72.83% <0.00%> (ø)` | |
| [superset/examples/country\_map.py](https://codecov.io/gh/apache/superset/pull/19981/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZXhhbXBsZXMvY291bnRyeV9tYXAucHk=) | `0.00% <0.00%> (ø)` | |
| [superset/examples/deck.py](https://codecov.io/gh/apache/superset/pull/19981/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZXhhbXBsZXMvZGVjay5weQ==) | `0.00% <0.00%> (ø)` | |
| [superset/examples/energy.py](https://codecov.io/gh/apache/superset/pull/19981/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZXhhbXBsZXMvZW5lcmd5LnB5) | `0.00% <0.00%> (ø)` | |
| [superset/examples/long\_lat.py](https://codecov.io/gh/apache/superset/pull/19981/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZXhhbXBsZXMvbG9uZ19sYXQucHk=) | `0.00% <0.00%> (ø)` | |
| [superset/examples/multi\_line.py](https://codecov.io/gh/apache/superset/pull/19981/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZXhhbXBsZXMvbXVsdGlfbGluZS5weQ==) | `0.00% <0.00%> (ø)` | |
| [superset/examples/multiformat\_time\_series.py](https://codecov.io/gh/apache/superset/pull/19981/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZXhhbXBsZXMvbXVsdGlmb3JtYXRfdGltZV9zZXJpZXMucHk=) | `0.00% <0.00%> (ø)` | |
| [superset/examples/random\_time\_series.py](https://codecov.io/gh/apache/superset/pull/19981/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZXhhbXBsZXMvcmFuZG9tX3RpbWVfc2VyaWVzLnB5) | `0.00% <0.00%> (ø)` | |
| [superset/examples/world\_bank.py](https://codecov.io/gh/apache/superset/pull/19981/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZXhhbXBsZXMvd29ybGRfYmFuay5weQ==) | `0.00% <0.00%> (-30.67%)` | :arrow_down: |
| ... and [304 more](https://codecov.io/gh/apache/superset/pull/19981/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/19981?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/19981?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [fdf48c6...70339a3](https://codecov.io/gh/apache/superset/pull/19981?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] hughhhh commented on a diff in pull request #19981: feat!: pass datasource_type and datasource_id to form_data
Posted by GitBox <gi...@apache.org>.
hughhhh commented on code in PR #19981:
URL: https://github.com/apache/superset/pull/19981#discussion_r867278462
##########
superset/explore/form_data/commands/delete.py:
##########
@@ -46,14 +48,15 @@ def run(self) -> bool:
key
)
if state:
- dataset_id = state["dataset_id"]
- chart_id = state["chart_id"]
- check_access(dataset_id, chart_id, actor)
+ datasource_id: int = state["datasource_id"]
+ chart_id: Optional[int] = state["chart_id"]
+ datasource_type: str = state["datasource_type"]
+ check_chart_access(datasource_id, chart_id, actor, datasource_type)
if state["owner"] != actor.get_user_id():
raise TemporaryCacheAccessDeniedError()
tab_id = self._cmd_params.tab_id
contextual_key = cache_key(
- session.get("_id"), tab_id, dataset_id, chart_id
+ session.get("_id"), tab_id, datasource_id, chart_id
Review Comment:
wouldn't we need to add `datasource_type` or we would get the same cache hit for datasource_id but different datasource_type?
```suggestion
session.get("_id"), tab_id, datasource_id, datasource_id, chart_id
```
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on a diff in pull request #19981: feat!: pass datasource_type and datasource_id to form_data
Posted by GitBox <gi...@apache.org>.
eschutho commented on code in PR #19981:
URL: https://github.com/apache/superset/pull/19981#discussion_r872891741
##########
superset/explore/utils.py:
##########
@@ -24,15 +24,24 @@
ChartNotFoundError,
)
from superset.charts.dao import ChartDAO
+from superset.commands.exceptions import DatasourceNotFoundValidationError
from superset.datasets.commands.exceptions import (
DatasetAccessDeniedError,
DatasetNotFoundError,
)
from superset.datasets.dao import DatasetDAO
+from superset.utils.core import DatasourceType
from superset.views.base import is_user_admin
from superset.views.utils import is_owner
+def check_datasource_access(datasource_id: int, datasource_type: str) -> Optional[bool]:
+ if datasource_id:
+ if datasource_type == DatasourceType.TABLE:
Review Comment:
Right now, it's not dependent because there's only one type, but when we start adding other types, we'll do something like this:
```
def check_query_access(query_id: int) -> Optional[bool]:
if query_id:
query = QueryDAO.find_by_id(query_id)
if query:
can_access_datasource = security_manager.raise_for_access(
query=query)
if can_access_datasource:
return True
raise QueryNotFoundError
```
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on a diff in pull request #19981: feat!: pass datasource_type and datasource_id to form_data
Posted by GitBox <gi...@apache.org>.
eschutho commented on code in PR #19981:
URL: https://github.com/apache/superset/pull/19981#discussion_r872888975
##########
superset-frontend/src/explore/components/controls/DatasourceControl/index.jsx:
##########
@@ -189,9 +189,10 @@ class DatasourceControl extends React.PureComponent {
const isMissingDatasource = datasource.id == null;
let isMissingParams = false;
if (isMissingDatasource) {
- const datasetId = getUrlParam(URL_PARAMS.datasetId);
+ const datasourceId = getUrlParam(URL_PARAMS.datasourceId);
+ const datasourceType = getUrlParam(URL_PARAMS.datasourceType);
const sliceId = getUrlParam(URL_PARAMS.sliceId);
- if (!datasetId && !sliceId) {
+ if (!datasourceId && !sliceId && !datasourceType) {
Review Comment:
Thanks, sounds good. I think we set a default for type then we won't be able to validate if it's missing.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on pull request #19981: feat!: pass datasource_type and datasource_id to form_data
Posted by GitBox <gi...@apache.org>.
eschutho commented on PR #19981:
URL: https://github.com/apache/superset/pull/19981#issuecomment-1126591378
Thanks for the review and added context @michael-s-molina. I made some small changes based on the feedback, but still have to come back to this and add in the check access query logic that I removed.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] john-bodley commented on pull request #19981: feat!: pass datasource_type and datasource_id to form_data
Posted by GitBox <gi...@apache.org>.
john-bodley commented on PR #19981:
URL: https://github.com/apache/superset/pull/19981#issuecomment-1126833670
@eschutho I'm a little lost, why do we need the `datasource_type` post the Druid NoSQL deprecation? i.e., this served merely to ensure the (`datasource_type`, `datasouce_id`) tuple was unique but given we only now have `datasource_type = 'table` this is irrelevant isn't it—unless this is somehow being used @betodealmeida's table/query schema refactor.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho merged pull request #19981: feat!: pass datasource_type and datasource_id to form_data
Posted by GitBox <gi...@apache.org>.
eschutho merged PR #19981:
URL: https://github.com/apache/superset/pull/19981
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on a diff in pull request #19981: feat!: pass datasource_type and datasource_id to form_data
Posted by GitBox <gi...@apache.org>.
eschutho commented on code in PR #19981:
URL: https://github.com/apache/superset/pull/19981#discussion_r882946296
##########
superset/utils/cache_manager.py:
##########
@@ -15,15 +15,40 @@
# specific language governing permissions and limitations
# under the License.
import logging
+from typing import Any, Optional, Union
from flask import Flask
from flask_caching import Cache
+from markupsafe import Markup
+
+from superset.utils.core import DatasourceType
logger = logging.getLogger(__name__)
CACHE_IMPORT_PATH = "superset.extensions.metastore_cache.SupersetMetastoreCache"
+class ExploreFormDataCache(Cache):
+ def get(self, *args: Any, **kwargs: Any) -> Optional[Union[str, Markup]]:
+ cache = self.cache.get(*args, **kwargs)
+
+ if not cache:
+ return None
+
+ # rename data keys for existing cache based on new TemporaryExploreState model
+ if isinstance(cache, dict):
Review Comment:
superset/explore/form_data/commands/create.py line 52 for example, it returns the key, not the data. I'm honestly not sure why.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] jinghua-qa commented on pull request #19981: feat!: pass datasource_type and datasource_id to form_data
Posted by GitBox <gi...@apache.org>.
jinghua-qa commented on PR #19981:
URL: https://github.com/apache/superset/pull/19981#issuecomment-1138850053
/testenv up
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] hughhhh commented on a diff in pull request #19981: feat!: pass datasource_type and datasource_id to form_data
Posted by GitBox <gi...@apache.org>.
hughhhh commented on code in PR #19981:
URL: https://github.com/apache/superset/pull/19981#discussion_r885763269
##########
superset/explore/form_data/commands/delete.py:
##########
@@ -47,14 +49,15 @@ def run(self) -> bool:
key
)
if state:
- dataset_id = state["dataset_id"]
- chart_id = state["chart_id"]
- check_access(dataset_id, chart_id, actor)
+ datasource_id: int = state["datasource_id"]
+ chart_id: Optional[int] = state["chart_id"]
+ 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, dataset_id, chart_id
+ session.get("_id"), tab_id, datasource_id, chart_id, datasource_type
Review Comment:
Since we are generating new keys for misses, i think it's fine to let the keys expire. The fix would just to add the old `contextual_key` doesn't include the `datasource_type`
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on pull request #19981: feat!: pass datasource_type and datasource_id to form_data
Posted by GitBox <gi...@apache.org>.
eschutho commented on PR #19981:
URL: https://github.com/apache/superset/pull/19981#issuecomment-1145055696
Thanks @jinghua-qa, I didn't know about that feature! I checked it on my local and then on the ephemeral and I see the highlighting.. is this the expected behavior?
https://user-images.githubusercontent.com/5186919/171676575-a5d18f6e-cdc3-4a01-be32-4b5889184ca9.mov
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] hughhhh commented on a diff in pull request #19981: feat!: pass datasource_type and datasource_id to form_data
Posted by GitBox <gi...@apache.org>.
hughhhh commented on code in PR #19981:
URL: https://github.com/apache/superset/pull/19981#discussion_r882780001
##########
superset/utils/cache_manager.py:
##########
@@ -15,15 +15,40 @@
# specific language governing permissions and limitations
# under the License.
import logging
+from typing import Any, Optional, Union
from flask import Flask
from flask_caching import Cache
+from markupsafe import Markup
+
+from superset.utils.core import DatasourceType
logger = logging.getLogger(__name__)
CACHE_IMPORT_PATH = "superset.extensions.metastore_cache.SupersetMetastoreCache"
+class ExploreFormDataCache(Cache):
+ def get(self, *args: Any, **kwargs: Any) -> Optional[Union[str, Markup]]:
+ cache = self.cache.get(*args, **kwargs)
+
+ if not cache:
+ return None
+
+ # rename data keys for existing cache based on new TemporaryExploreState model
+ if isinstance(cache, dict):
Review Comment:
Why do we need to check if the cache is a dict? Won't it always be a dict?
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on a diff in pull request #19981: feat!: pass datasource_type and datasource_id to form_data
Posted by GitBox <gi...@apache.org>.
eschutho commented on code in PR #19981:
URL: https://github.com/apache/superset/pull/19981#discussion_r886224922
##########
superset/explore/form_data/commands/delete.py:
##########
@@ -47,14 +49,15 @@ def run(self) -> bool:
key
)
if state:
- dataset_id = state["dataset_id"]
- chart_id = state["chart_id"]
- check_access(dataset_id, chart_id, actor)
+ datasource_id: int = state["datasource_id"]
+ chart_id: Optional[int] = state["chart_id"]
+ 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, dataset_id, chart_id
+ session.get("_id"), tab_id, datasource_id, chart_id, datasource_type
Review Comment:
Yeah, let's do that, it's only for delete and update. Unless anyone else has thoughts.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on a diff in pull request #19981: feat!: pass datasource_type and datasource_id to form_data
Posted by GitBox <gi...@apache.org>.
eschutho commented on code in PR #19981:
URL: https://github.com/apache/superset/pull/19981#discussion_r883185082
##########
scripts/tests/run.sh:
##########
@@ -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}"
Review Comment:
```suggestion
pytest --durations=0 --maxfail=1 "${TEST_MODULE}"
```
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on a diff in pull request #19981: feat!: pass datasource_type and datasource_id to form_data
Posted by GitBox <gi...@apache.org>.
eschutho commented on code in PR #19981:
URL: https://github.com/apache/superset/pull/19981#discussion_r872894498
##########
tests/integration_tests/utils/cache_manager_tests.py:
##########
@@ -0,0 +1,50 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+import pytest
+
+from superset.extensions import cache_manager
+from superset.utils.core import DatasourceType
+
Review Comment:
yeah, got it on line 38. 👍
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on a diff in pull request #19981: feat!: pass datasource_type and datasource_id to form_data
Posted by GitBox <gi...@apache.org>.
eschutho commented on code in PR #19981:
URL: https://github.com/apache/superset/pull/19981#discussion_r872890014
##########
superset-frontend/src/constants.ts:
##########
@@ -84,7 +88,8 @@ export const URL_PARAMS = {
export const RESERVED_CHART_URL_PARAMS: string[] = [
URL_PARAMS.formDataKey.name,
URL_PARAMS.sliceId.name,
- URL_PARAMS.datasetId.name,
+ URL_PARAMS.datasourceId.name,
+ URL_PARAMS.datasourceType.name,
Review Comment:
@michael-s-molina this could be an old URL, too? I'll datasetId back in here..
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on a diff in pull request #19981: feat!: pass datasource_type and datasource_id to form_data
Posted by GitBox <gi...@apache.org>.
eschutho commented on code in PR #19981:
URL: https://github.com/apache/superset/pull/19981#discussion_r867275569
##########
superset/explore/permalink/commands/create.py:
##########
@@ -39,11 +39,16 @@ def __init__(self, actor: User, state: Dict[str, Any]):
def run(self) -> str:
self.validate()
try:
- dataset_id = int(self.datasource.split("__")[0])
- check_access(dataset_id, self.chart_id, self.actor)
+ datasource = self.datasource.split("__")
+ datasource_id: int = int(datasource[0])
+ datasource_type: str = datasource[1]
Review Comment:
I really wanted to do a nice tuple unpacking here, but unfortunately mypy wouldn't have it.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on a diff in pull request #19981: feat!: pass datasource_type and datasource_id to form_data
Posted by GitBox <gi...@apache.org>.
eschutho commented on code in PR #19981:
URL: https://github.com/apache/superset/pull/19981#discussion_r872890675
##########
superset/explore/form_data/commands/update.py:
##########
@@ -64,7 +65,7 @@ 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, dataset_id, chart_id
+ session.get("_id"), tab_id, datasource_id, chart_id, datasource_type
Review Comment:
This is creating a new key, though, right?
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] hughhhh commented on a diff in pull request #19981: feat!: pass datasource_type and datasource_id to form_data
Posted by GitBox <gi...@apache.org>.
hughhhh commented on code in PR #19981:
URL: https://github.com/apache/superset/pull/19981#discussion_r867278696
##########
tests/integration_tests/explore/form_data/api_tests.py:
##########
@@ -67,65 +67,92 @@ def dataset_id() -> int:
return dataset.id
+@pytest.fixture
+def datasource_type() -> int:
+ with app.app_context() as ctx:
+ session: Session = ctx.app.appbuilder.get_session
+ dataset = (
+ session.query(SqlaTable)
+ .filter_by(table_name="wb_health_population")
+ .first()
+ )
+ return dataset.type
+
+
@pytest.fixture(autouse=True)
-def cache(chart_id, admin_id, dataset_id):
+def cache(chart_id, admin_id, datasource_id, datasource_type):
entry: TemporaryExploreState = {
"owner": admin_id,
- "dataset_id": dataset_id,
+ "datasource_id": datasource_id,
+ "datasource_type": datasource_type,
"chart_id": chart_id,
"form_data": INITIAL_FORM_DATA,
}
cache_manager.explore_form_data_cache.set(KEY, entry)
-def test_post(client, chart_id: int, dataset_id: int):
+def test_post(client, chart_id: int, datasource_id: int, datasource_type: str):
login(client, "admin")
payload = {
- "dataset_id": dataset_id,
+ "datasource_id": datasource_id,
+ "datasource_type": datasource_type,
"chart_id": chart_id,
"form_data": INITIAL_FORM_DATA,
}
resp = client.post("api/v1/explore/form_data", json=payload)
+ print(resp.data)
Review Comment:
remove this
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on a diff in pull request #19981: feat!: pass datasource_type and datasource_id to form_data
Posted by GitBox <gi...@apache.org>.
eschutho commented on code in PR #19981:
URL: https://github.com/apache/superset/pull/19981#discussion_r877631791
##########
superset/explore/form_data/commands/parameters.py:
##########
@@ -23,7 +23,8 @@
@dataclass
class CommandParameters:
actor: User
- dataset_id: int = 0
+ datasource_type: str = ""
+ datasource_id: int = 0
Review Comment:
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.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michael-s-molina commented on a diff in pull request #19981: feat!: pass datasource_type and datasource_id to form_data
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on code in PR #19981:
URL: https://github.com/apache/superset/pull/19981#discussion_r870272843
##########
superset/explore/utils.py:
##########
@@ -24,15 +24,24 @@
ChartNotFoundError,
)
from superset.charts.dao import ChartDAO
+from superset.commands.exceptions import DatasourceNotFoundValidationError
from superset.datasets.commands.exceptions import (
DatasetAccessDeniedError,
DatasetNotFoundError,
)
from superset.datasets.dao import DatasetDAO
+from superset.utils.core import DatasourceType
from superset.views.base import is_user_admin
from superset.views.utils import is_owner
+def check_datasource_access(datasource_id: int, datasource_type: str) -> Optional[bool]:
+ if datasource_id:
+ if datasource_type == DatasourceType.TABLE:
+ return check_dataset_access(datasource_id)
+ raise DatasourceNotFoundValidationError
+
+
def check_dataset_access(dataset_id: int) -> Optional[bool]:
Review Comment:
The `check_dataset_access` is not being used anymore, right? If so, can we remove it?
##########
superset/explore/form_data/commands/parameters.py:
##########
@@ -23,7 +23,8 @@
@dataclass
class CommandParameters:
actor: User
- dataset_id: int = 0
+ datasource_type: str = ""
+ datasource_id: int = 0
Review Comment:
I think it's a good idea to break it into multiple classes 👍🏼
##########
tests/integration_tests/explore/form_data/api_tests.py:
##########
@@ -67,65 +67,91 @@ def dataset_id() -> int:
return dataset.id
+@pytest.fixture
Review Comment:
Maybe have one fixture that returns the `datasource` with both `id` and `type`?
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michael-s-molina commented on a diff in pull request #19981: feat!: pass datasource_type and datasource_id to form_data
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on code in PR #19981:
URL: https://github.com/apache/superset/pull/19981#discussion_r870516884
##########
superset/explore/utils.py:
##########
@@ -24,15 +24,24 @@
ChartNotFoundError,
)
from superset.charts.dao import ChartDAO
+from superset.commands.exceptions import DatasourceNotFoundValidationError
from superset.datasets.commands.exceptions import (
DatasetAccessDeniedError,
DatasetNotFoundError,
)
from superset.datasets.dao import DatasetDAO
+from superset.utils.core import DatasourceType
from superset.views.base import is_user_admin
from superset.views.utils import is_owner
+def check_datasource_access(datasource_id: int, datasource_type: str) -> Optional[bool]:
+ if datasource_id:
+ if datasource_type == DatasourceType.TABLE:
Review Comment:
I'm not sure if `check_dataset_access` depends on the `datasource_type`. Shouldn’t the check occur independently of the data source type?
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] github-actions[bot] commented on pull request #19981: feat!: pass datasource_type and datasource_id to form_data
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #19981:
URL: https://github.com/apache/superset/pull/19981#issuecomment-1145450797
Ephemeral environment shutdown and build artifacts deleted.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on a diff in pull request #19981: feat!: pass datasource_type and datasource_id to form_data
Posted by GitBox <gi...@apache.org>.
eschutho commented on code in PR #19981:
URL: https://github.com/apache/superset/pull/19981#discussion_r882924239
##########
superset/utils/cache_manager.py:
##########
@@ -15,15 +15,40 @@
# specific language governing permissions and limitations
# under the License.
import logging
+from typing import Any, Optional, Union
from flask import Flask
from flask_caching import Cache
+from markupsafe import Markup
+
+from superset.utils.core import DatasourceType
logger = logging.getLogger(__name__)
CACHE_IMPORT_PATH = "superset.extensions.metastore_cache.SupersetMetastoreCache"
+class ExploreFormDataCache(Cache):
+ def get(self, *args: Any, **kwargs: Any) -> Optional[Union[str, Markup]]:
+ cache = self.cache.get(*args, **kwargs)
+
+ if not cache:
+ return None
+
+ # rename data keys for existing cache based on new TemporaryExploreState model
+ if isinstance(cache, dict):
Review Comment:
actually, some times it returns a string.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on pull request #19981: feat!: pass datasource_type and datasource_id to form_data
Posted by GitBox <gi...@apache.org>.
eschutho commented on PR #19981:
URL: https://github.com/apache/superset/pull/19981#issuecomment-1139187201
This is mostly done.. I just found one other thing last minute that I'd like to fix. I'll still welcome any feedback.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org