[GitHub] [superset] john-bodley commented on a diff in pull request #24301: fix: Ability to share saved queries

["john-bodley (via GitHub)" <gi...@apache.org>]

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: 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