[GitHub] [incubator-superset] willbarrett commented on a change in pull request #9144: [database] Fix, tables endpoint

[GitBox <gi...@apache.org>]

willbarrett commented on a change in pull request #9144: [database] Fix, tables endpoint
URL: https://github.com/apache/incubator-superset/pull/9144#discussion_r380825490
 
 

 ##########
 File path: superset/views/database/api.py
 ##########
 @@ -274,6 +351,139 @@ def table_metadata(self, database: Database, table_name: str, schema_name: str):
         self.incr_stats("success", self.table_metadata.__name__)
         return self.response(200, **table_info)
 
+    @expose("/<int:pk>/tables/<string:schema_name>/<string:substr>/", methods=["GET"])
+    @expose(
+        "/<int:pk>/tables/<string:schema_name>/<string:substr>/<force_refresh>/",
+        methods=["GET"],
+    )
+    @protect()
+    @safe
+    @event_logger.log_this
+    def tables(  # pylint: disable=invalid-name,too-many-locals
 
 Review comment:
   I'm starting to wonder if we want to do this migration now as part of the security fix PR? This looks like we've pulled the method definition over almost as-is, but as `pylint` is showing us, this really could use a refactor. Would it be better to have one PR that adds the security fix, then a second that refactors the method into the new API?
   
   Another approach here could be to "strangle" the current implementation by proxying to the current implementation, then refactoring it over time.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org