john-bodley commented on code in PR #24301:
URL: https://github.com/apache/superset/pull/24301#discussion_r1221911649
##########
superset-frontend/src/SqlLab/components/ShareSqlLabQuery/index.tsx:
##########
@@ -76,10 +77,10 @@ const ShareSqlLabQuery = ({
const getCopyUrlForSavedQuery = (callback: Function) => {
let savedQueryToastContent;
- if (remoteId) {
+ if (uuid) {
savedQueryToastContent = `${
window.location.origin + window.location.pathname
- }?savedQueryId=${remoteId}`;
+ }?savedQueryId=${uuid}`;
Review Comment:
Is there a concern that by switching to UUID existing URLs will break?
##########
superset-frontend/src/pages/SavedQueryList/index.tsx:
##########
@@ -216,10 +216,10 @@ function SavedQueryList({
};
const copyQueryLink = useCallback(
- (id: number) => {
+ (uuid: string) => {
Review Comment:
Does TypeScript support a UUID type?
##########
superset/queries/saved_queries/api.py:
##########
@@ -174,6 +182,52 @@ def pre_add(self, item: SavedQuery) -> None:
def pre_update(self, item: SavedQuery) -> None:
self.pre_add(item)
+ @expose("/<_id>", methods=("GET",))
+ @protect()
+ @safe
+ @permission_name("get")
+ @statsd_metrics
+ @event_logger.log_this_with_context(
+ action=lambda self, *args, **kwargs: f"{self.__class__.__name__}.get",
+ log_to_statsd=False,
+ )
+ def get(self, _id: str, **kwargs: Any) -> Response:
Review Comment:
```suggestion
def get(self, id_: str, **kwargs: Any) -> Response:
```
The convention is to add (rather than prepend) an underscore to variable
names which conflict with the global namespace.
Additionally can't the ID be an integer (rather than a string) or UUID.
##########
superset/queries/saved_queries/dao.py:
##########
@@ -44,3 +47,20 @@ def bulk_delete(models: Optional[list[SavedQuery]], commit:
bool = True) -> None
except SQLAlchemyError as ex:
db.session.rollback()
raise DAODeleteFailedError() from ex
+
+ @classmethod
+ def get_by_id(cls, _id: str) -> Optional[SavedQuery]:
Review Comment:
I don't believe this is the right approach, i.e., per this code block the
UUID option is bypass the security check (by way of the filter on line #63).
The correct approach would be to generate a permalink similar to charts and
dashboards.
cc @michael-s-molina and @villebro as we've discussed this previously.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
