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 2020/02/14 11:00:24 UTC
[GitHub] [incubator-superset] dpgaspar opened a new pull request #9144:
[database] Fix, tables endpoint
dpgaspar opened a new pull request #9144: [database] Fix, tables endpoint
URL: https://github.com/apache/incubator-superset/pull/9144
### CATEGORY
Choose one
- [ ] Bug Fix
- [ ] Enhancement (new features, refinement)
- [ ] Refactor
- [ ] Add tests
- [ ] Build / Development Environment
- [ ] Documentation
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TEST PLAN
<!--- What steps should be taken to verify the changes -->
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Changes UI
- [ ] Requires DB Migration.
- [ ] Confirm DB Migration upgrade and downgrade tested.
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
### REVIEWERS
----------------------------------------------------------------
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
[GitHub] [incubator-superset] villebro commented on a change in pull request
#9144: [database] Fix, tables endpoint
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #9144: [database] Fix, tables endpoint
URL: https://github.com/apache/incubator-superset/pull/9144#discussion_r381354705
##########
File path: superset/views/core.py
##########
@@ -1068,30 +1054,39 @@ def schemas(self, db_id, force_refresh="false"):
@api
@has_access_api
- @expose("/tables/<db_id>/<schema>/<substr>/")
- @expose("/tables/<db_id>/<schema>/<substr>/<force_refresh>/")
- def tables(self, db_id, schema, substr, force_refresh="false"):
+ @expose("/tables/<int:db_id>/<schema>/<substr>/")
+ @expose("/tables/<int:db_id>/<schema>/<substr>/<force_refresh>/")
+ def tables(
+ self, db_id: int, schema: str, substr: str, force_refresh: str = "false"
+ ):
"""Endpoint to fetch the list of tables for given database"""
- db_id = int(db_id)
- force_refresh = force_refresh.lower() == "true"
- schema = utils.parse_js_uri_path_item(schema, eval_undefined=True)
- substr = utils.parse_js_uri_path_item(substr, eval_undefined=True)
- database = db.session.query(models.Database).filter_by(id=db_id).one()
+ # Guarantees database filtering by security access
+ query = db.session.query(models.Database)
+ query = DatabaseFilter("id", SQLAInterface(models.Database, db.session)).apply(
+ query, None
+ )
+ database = query.filter_by(id=db_id).one_or_none()
+ if not database:
+ return json_error_response("Not found", 404)
Review comment:
Did we settle for try-except with `.one()` or `.one_or_none()` with `if not`? I'm fine either way, just prefer to have it as uniform as possible in the codebase.
----------------------------------------------------------------
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
[GitHub] [incubator-superset] codecov-io edited a comment on issue #9144:
[database] Fix, tables endpoint
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #9144: [database] Fix, tables endpoint
URL: https://github.com/apache/incubator-superset/pull/9144#issuecomment-587079073
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9144?src=pr&el=h1) Report
> Merging [#9144](https://codecov.io/gh/apache/incubator-superset/pull/9144?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/63f4f00cd1e7669b439c1e72a07303fff5164803?src=pr&el=desc) will **not change** coverage.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/incubator-superset/pull/9144?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #9144 +/- ##
=======================================
Coverage 59.06% 59.06%
=======================================
Files 372 372
Lines 11922 11922
Branches 2919 2919
=======================================
Hits 7042 7042
Misses 4698 4698
Partials 182 182
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/9144?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...frontend/src/dashboard/components/MissingChart.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9144/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL01pc3NpbmdDaGFydC5qc3g=) | `100% <ø> (ø)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9144?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9144?src=pr&el=footer). Last update [63f4f00...f1993ea](https://codecov.io/gh/apache/incubator-superset/pull/9144?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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
[GitHub] [incubator-superset] villebro commented on a change in pull request
#9144: [database] Fix, tables endpoint
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #9144: [database] Fix, tables endpoint
URL: https://github.com/apache/incubator-superset/pull/9144#discussion_r381354577
##########
File path: superset/views/core.py
##########
@@ -1068,30 +1054,39 @@ def schemas(self, db_id, force_refresh="false"):
@api
@has_access_api
- @expose("/tables/<db_id>/<schema>/<substr>/")
- @expose("/tables/<db_id>/<schema>/<substr>/<force_refresh>/")
- def tables(self, db_id, schema, substr, force_refresh="false"):
+ @expose("/tables/<int:db_id>/<schema>/<substr>/")
+ @expose("/tables/<int:db_id>/<schema>/<substr>/<force_refresh>/")
+ def tables(
+ self, db_id: int, schema: str, substr: str, force_refresh: str = "false"
+ ):
"""Endpoint to fetch the list of tables for given database"""
- db_id = int(db_id)
- force_refresh = force_refresh.lower() == "true"
- schema = utils.parse_js_uri_path_item(schema, eval_undefined=True)
- substr = utils.parse_js_uri_path_item(substr, eval_undefined=True)
- database = db.session.query(models.Database).filter_by(id=db_id).one()
+ # Guarantees database filtering by security access
+ query = db.session.query(models.Database)
+ query = DatabaseFilter("id", SQLAInterface(models.Database, db.session)).apply(
+ query, None
+ )
+ database = query.filter_by(id=db_id).one_or_none()
+ if not database:
+ return json_error_response("Not found", 404)
Review comment:
Did we settle for try-except with `.one()` or `.one_or_none()` with `if not`? I'm fine either way, just prefer to have it as uniform as possible in the codebase.
----------------------------------------------------------------
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
[GitHub] [incubator-superset] willbarrett commented on a change in pull
request #9144: [database] Fix, tables endpoint
Posted by 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_r380817620
##########
File path: superset/views/core.py
##########
@@ -1068,30 +1054,47 @@ def schemas(self, db_id, force_refresh="false"):
@api
@has_access_api
- @expose("/tables/<db_id>/<schema>/<substr>/")
- @expose("/tables/<db_id>/<schema>/<substr>/<force_refresh>/")
- def tables(self, db_id, schema, substr, force_refresh="false"):
+ @expose("/tables/<int:db_id>/<schema>/<substr>/")
+ @expose("/tables/<int:db_id>/<schema>/<substr>/<force_refresh>/")
+ def tables(
+ self, db_id: int, schema: str, substr: str, force_refresh: str = "false"
+ ):
"""Endpoint to fetch the list of tables for given database"""
- db_id = int(db_id)
- force_refresh = force_refresh.lower() == "true"
- schema = utils.parse_js_uri_path_item(schema, eval_undefined=True)
- substr = utils.parse_js_uri_path_item(substr, eval_undefined=True)
- database = db.session.query(models.Database).filter_by(id=db_id).one()
+ logger.warning(
+ "/superset/tables/ API endpoint "
+ "is deprecated and will be removed in version 1.0.0"
+ )
+ stats_logger.incr(f"{self.__class__.__name__}.tables.init")
Review comment:
`.init` seems odd - this isn't an initialization function. Is that copy/pasta or was it intentional? Should this include the `deprecated` prefix?
----------------------------------------------------------------
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
[GitHub] [incubator-superset] dpgaspar commented on a change in pull request
#9144: [database] Fix, tables endpoint
Posted by GitBox <gi...@apache.org>.
dpgaspar commented on a change in pull request #9144: [database] Fix, tables endpoint
URL: https://github.com/apache/incubator-superset/pull/9144#discussion_r380840672
##########
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:
Agreed I'll break this into 2 PR's
----------------------------------------------------------------
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
[GitHub] [incubator-superset] dpgaspar commented on a change in pull request
#9144: [database] Fix, tables endpoint
Posted by GitBox <gi...@apache.org>.
dpgaspar commented on a change in pull request #9144: [database] Fix, tables endpoint
URL: https://github.com/apache/incubator-superset/pull/9144#discussion_r381406178
##########
File path: superset/views/core.py
##########
@@ -1068,30 +1054,39 @@ def schemas(self, db_id, force_refresh="false"):
@api
@has_access_api
- @expose("/tables/<db_id>/<schema>/<substr>/")
- @expose("/tables/<db_id>/<schema>/<substr>/<force_refresh>/")
- def tables(self, db_id, schema, substr, force_refresh="false"):
+ @expose("/tables/<int:db_id>/<schema>/<substr>/")
+ @expose("/tables/<int:db_id>/<schema>/<substr>/<force_refresh>/")
+ def tables(
+ self, db_id: int, schema: str, substr: str, force_refresh: str = "false"
+ ):
"""Endpoint to fetch the list of tables for given database"""
- db_id = int(db_id)
- force_refresh = force_refresh.lower() == "true"
- schema = utils.parse_js_uri_path_item(schema, eval_undefined=True)
- substr = utils.parse_js_uri_path_item(substr, eval_undefined=True)
- database = db.session.query(models.Database).filter_by(id=db_id).one()
+ # Guarantees database filtering by security access
+ query = db.session.query(models.Database)
+ query = DatabaseFilter("id", SQLAInterface(models.Database, db.session)).apply(
+ query, None
+ )
+ database = query.filter_by(id=db_id).one_or_none()
+ if not database:
+ return json_error_response("Not found", 404)
Review comment:
There is a majority of `one_or_none()` on `core.py`
----------------------------------------------------------------
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
[GitHub] [incubator-superset] dpgaspar commented on a change in pull request
#9144: [database] Fix, tables endpoint
Posted by GitBox <gi...@apache.org>.
dpgaspar commented on a change in pull request #9144: [database] Fix, tables endpoint
URL: https://github.com/apache/incubator-superset/pull/9144#discussion_r380839720
##########
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
+ self, pk: int, schema_name: str, substr: str, force_refresh: str = "false"
+ ) -> Response:
+ """ Table schema info
+ ---
+ get:
+ description: >-
+ Get a list of tables from a database/schema
+ parameters:
+ - in: path
+ name: pk
+ description: The database id
+ schema:
+ type: integer
+ - in: path
+ description: Table schema
+ name: schema
+ schema:
+ type: string
+ - in: path
+ description: Sub string
+ name: substr
+ schema:
+ type: string
+ - in: path
+ description: force cache refresh
+ name: force_refresh
+ schema:
+ type: bool
+ responses:
+ 200:
+ description: Table schema info
+ content:
+ text/plain:
+ schema:
+ type: object
+ properties:
+ options:
+ type: array
+ items:
+ type: object
+ properties:
+ label:
+ type: string
+ schema:
+ type: string
+ title:
+ type: string
+ type:
+ type: string
+ value:
+ type: string
+ tableLength:
+ type: number
+ 400:
+ $ref: '#/components/responses/400'
+ 401:
+ $ref: '#/components/responses/401'
+ 404:
+ $ref: '#/components/responses/404'
+ 422:
+ $ref: '#/components/responses/422'
+ 500:
+ $ref: '#/components/responses/500'
+ """
+ self.incr_stats("init", self.tables.__name__)
+ force_refresh_parsed = force_refresh.lower() == "true"
+ schema_parsed = parse_js_uri_path_item(schema_name, eval_undefined=True)
+ substr_parsed = parse_js_uri_path_item(substr, eval_undefined=True)
+ # Guarantees filtering on databases
+ database: Database = self.datamodel.get(pk, self._base_filters)
+ if not database:
+ self.incr_stats("error", self.tables.__name__)
+ return self.response_404()
+
+ tables = get_all_table_names_in_database(
+ database, schema_parsed, substr_parsed, force_refresh_parsed
+ )
+ views = get_all_view_names_in_database(
+ database, schema_parsed, substr_parsed, force_refresh_parsed
+ )
+
+ if not schema_parsed and database.default_schemas:
+ user_schema = g.user.email.split("@")[0]
+ valid_schemas = set(database.default_schemas + [user_schema])
+
+ tables = [tn for tn in tables if tn.schema in valid_schemas]
+ views = [vn for vn in views if vn.schema in valid_schemas]
+
+ max_items = app.config["MAX_TABLE_NAMES"] or len(tables)
Review comment:
Changed it to `current_app`
----------------------------------------------------------------
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
[GitHub] [incubator-superset] dpgaspar merged pull request #9144: [database]
Fix, tables endpoint
Posted by GitBox <gi...@apache.org>.
dpgaspar merged pull request #9144: [database] Fix, tables endpoint
URL: https://github.com/apache/incubator-superset/pull/9144
----------------------------------------------------------------
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
[GitHub] [incubator-superset] willbarrett commented on a change in pull
request #9144: [database] Fix, tables endpoint
Posted by 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_r380823242
##########
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
+ self, pk: int, schema_name: str, substr: str, force_refresh: str = "false"
+ ) -> Response:
+ """ Table schema info
+ ---
+ get:
+ description: >-
+ Get a list of tables from a database/schema
+ parameters:
+ - in: path
+ name: pk
+ description: The database id
+ schema:
+ type: integer
+ - in: path
+ description: Table schema
+ name: schema
+ schema:
+ type: string
+ - in: path
+ description: Sub string
+ name: substr
+ schema:
+ type: string
+ - in: path
+ description: force cache refresh
+ name: force_refresh
+ schema:
+ type: bool
+ responses:
+ 200:
+ description: Table schema info
+ content:
+ text/plain:
+ schema:
+ type: object
+ properties:
+ options:
+ type: array
+ items:
+ type: object
+ properties:
+ label:
+ type: string
+ schema:
+ type: string
+ title:
+ type: string
+ type:
+ type: string
+ value:
+ type: string
+ tableLength:
+ type: number
+ 400:
+ $ref: '#/components/responses/400'
+ 401:
+ $ref: '#/components/responses/401'
+ 404:
+ $ref: '#/components/responses/404'
+ 422:
+ $ref: '#/components/responses/422'
+ 500:
+ $ref: '#/components/responses/500'
+ """
+ self.incr_stats("init", self.tables.__name__)
+ force_refresh_parsed = force_refresh.lower() == "true"
+ schema_parsed = parse_js_uri_path_item(schema_name, eval_undefined=True)
+ substr_parsed = parse_js_uri_path_item(substr, eval_undefined=True)
+ # Guarantees filtering on databases
+ database: Database = self.datamodel.get(pk, self._base_filters)
+ if not database:
+ self.incr_stats("error", self.tables.__name__)
+ return self.response_404()
+
+ tables = get_all_table_names_in_database(
+ database, schema_parsed, substr_parsed, force_refresh_parsed
+ )
+ views = get_all_view_names_in_database(
+ database, schema_parsed, substr_parsed, force_refresh_parsed
+ )
+
+ if not schema_parsed and database.default_schemas:
+ user_schema = g.user.email.split("@")[0]
+ valid_schemas = set(database.default_schemas + [user_schema])
+
+ tables = [tn for tn in tables if tn.schema in valid_schemas]
+ views = [vn for vn in views if vn.schema in valid_schemas]
+
+ max_items = app.config["MAX_TABLE_NAMES"] or len(tables)
Review comment:
Is there a way we can get at this configuration value without pulling in the entire `app` object? Is it possible to use the `current_app` proxy or load the config value directly?
----------------------------------------------------------------
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
[GitHub] [incubator-superset] willbarrett commented on a change in pull
request #9144: [database] Fix, tables endpoint
Posted by 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
[GitHub] [incubator-superset] willbarrett commented on a change in pull
request #9144: [database] Fix, tables endpoint
Posted by 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_r380822161
##########
File path: superset/views/core.py
##########
@@ -1105,20 +1108,22 @@ def tables(self, db_id, schema, substr, force_refresh="false"):
cache=True, force=False, cache_timeout=24 * 60 * 60
)
tables = security_manager.get_datasources_accessible_by_user(
- database, tables, schema
+ database, tables, schema_parsed
)
views = security_manager.get_datasources_accessible_by_user(
- database, views, schema
+ database, views, schema_parsed
)
def get_datasource_label(ds_name: utils.DatasourceName) -> str:
- return ds_name.table if schema else f"{ds_name.schema}.{ds_name.table}"
+ return (
+ ds_name.table if schema_parsed else f"{ds_name.schema}.{ds_name.table}"
+ )
if substr:
Review comment:
Should this now be `if substr_parsed`?
----------------------------------------------------------------
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
[GitHub] [incubator-superset] villebro commented on a change in pull request
#9144: [database] Fix, tables endpoint
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #9144: [database] Fix, tables endpoint
URL: https://github.com/apache/incubator-superset/pull/9144#discussion_r381354705
##########
File path: superset/views/core.py
##########
@@ -1068,30 +1054,39 @@ def schemas(self, db_id, force_refresh="false"):
@api
@has_access_api
- @expose("/tables/<db_id>/<schema>/<substr>/")
- @expose("/tables/<db_id>/<schema>/<substr>/<force_refresh>/")
- def tables(self, db_id, schema, substr, force_refresh="false"):
+ @expose("/tables/<int:db_id>/<schema>/<substr>/")
+ @expose("/tables/<int:db_id>/<schema>/<substr>/<force_refresh>/")
+ def tables(
+ self, db_id: int, schema: str, substr: str, force_refresh: str = "false"
+ ):
"""Endpoint to fetch the list of tables for given database"""
- db_id = int(db_id)
- force_refresh = force_refresh.lower() == "true"
- schema = utils.parse_js_uri_path_item(schema, eval_undefined=True)
- substr = utils.parse_js_uri_path_item(substr, eval_undefined=True)
- database = db.session.query(models.Database).filter_by(id=db_id).one()
+ # Guarantees database filtering by security access
+ query = db.session.query(models.Database)
+ query = DatabaseFilter("id", SQLAInterface(models.Database, db.session)).apply(
+ query, None
+ )
+ database = query.filter_by(id=db_id).one_or_none()
+ if not database:
+ return json_error_response("Not found", 404)
Review comment:
Did we settle for try-except with `.one()` or `.one_or_none()` with `if not`? I'm fine either way, just prefer to have it as uniform as possible in the codebase.
----------------------------------------------------------------
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
[GitHub] [incubator-superset] dpgaspar commented on a change in pull request
#9144: [database] Fix, tables endpoint
Posted by GitBox <gi...@apache.org>.
dpgaspar commented on a change in pull request #9144: [database] Fix, tables endpoint
URL: https://github.com/apache/incubator-superset/pull/9144#discussion_r380839259
##########
File path: superset/views/core.py
##########
@@ -1068,30 +1054,47 @@ def schemas(self, db_id, force_refresh="false"):
@api
@has_access_api
- @expose("/tables/<db_id>/<schema>/<substr>/")
- @expose("/tables/<db_id>/<schema>/<substr>/<force_refresh>/")
- def tables(self, db_id, schema, substr, force_refresh="false"):
+ @expose("/tables/<int:db_id>/<schema>/<substr>/")
+ @expose("/tables/<int:db_id>/<schema>/<substr>/<force_refresh>/")
+ def tables(
+ self, db_id: int, schema: str, substr: str, force_refresh: str = "false"
+ ):
"""Endpoint to fetch the list of tables for given database"""
- db_id = int(db_id)
- force_refresh = force_refresh.lower() == "true"
- schema = utils.parse_js_uri_path_item(schema, eval_undefined=True)
- substr = utils.parse_js_uri_path_item(substr, eval_undefined=True)
- database = db.session.query(models.Database).filter_by(id=db_id).one()
+ logger.warning(
+ "/superset/tables/ API endpoint "
+ "is deprecated and will be removed in version 1.0.0"
+ )
+ stats_logger.incr(f"{self.__class__.__name__}.tables.init")
Review comment:
Fixed, yes `init` may be debatable but was the metric name agreed on #9054
----------------------------------------------------------------
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
[GitHub] [incubator-superset] codecov-io commented on issue #9144:
[database] Fix, tables endpoint
Posted by GitBox <gi...@apache.org>.
codecov-io commented on issue #9144: [database] Fix, tables endpoint
URL: https://github.com/apache/incubator-superset/pull/9144#issuecomment-587079073
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9144?src=pr&el=h1) Report
> Merging [#9144](https://codecov.io/gh/apache/incubator-superset/pull/9144?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/931ea552707eb320655af0ff339fc74d10cb60c7?src=pr&el=desc) will **decrease** coverage by `0.03%`.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/incubator-superset/pull/9144?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #9144 +/- ##
==========================================
- Coverage 59.1% 59.06% -0.04%
==========================================
Files 372 372
Lines 11922 11922
Branches 2919 2919
==========================================
- Hits 7046 7042 -4
- Misses 4694 4698 +4
Partials 182 182
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/9144?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [superset-frontend/src/components/TableSelector.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9144/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvVGFibGVTZWxlY3Rvci5qc3g=) | `78.35% <ø> (ø)` | :arrow_up: |
| [superset-frontend/src/components/Loading.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9144/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvTG9hZGluZy5qc3g=) | `55.55% <0%> (-44.45%)` | :arrow_down: |
| [...frontend/src/dashboard/components/MissingChart.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9144/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL01pc3NpbmdDaGFydC5qc3g=) | `100% <0%> (ø)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9144?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9144?src=pr&el=footer). Last update [931ea55...eaa6e70](https://codecov.io/gh/apache/incubator-superset/pull/9144?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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
[GitHub] [incubator-superset] dpgaspar commented on a change in pull request
#9144: [database] Fix, tables endpoint
Posted by GitBox <gi...@apache.org>.
dpgaspar commented on a change in pull request #9144: [database] Fix, tables endpoint
URL: https://github.com/apache/incubator-superset/pull/9144#discussion_r380839515
##########
File path: superset/views/core.py
##########
@@ -1105,20 +1108,22 @@ def tables(self, db_id, schema, substr, force_refresh="false"):
cache=True, force=False, cache_timeout=24 * 60 * 60
)
tables = security_manager.get_datasources_accessible_by_user(
- database, tables, schema
+ database, tables, schema_parsed
)
views = security_manager.get_datasources_accessible_by_user(
- database, views, schema
+ database, views, schema_parsed
)
def get_datasource_label(ds_name: utils.DatasourceName) -> str:
- return ds_name.table if schema else f"{ds_name.schema}.{ds_name.table}"
+ return (
+ ds_name.table if schema_parsed else f"{ds_name.schema}.{ds_name.table}"
+ )
if substr:
Review comment:
Yes, great catch!
----------------------------------------------------------------
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
[GitHub] [incubator-superset] codecov-io edited a comment on issue #9144:
[database] Fix, tables endpoint
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #9144: [database] Fix, tables endpoint
URL: https://github.com/apache/incubator-superset/pull/9144#issuecomment-587079073
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9144?src=pr&el=h1) Report
> Merging [#9144](https://codecov.io/gh/apache/incubator-superset/pull/9144?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/931ea552707eb320655af0ff339fc74d10cb60c7?src=pr&el=desc) will **decrease** coverage by `0.03%`.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/incubator-superset/pull/9144?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #9144 +/- ##
==========================================
- Coverage 59.1% 59.06% -0.04%
==========================================
Files 372 372
Lines 11922 11922
Branches 2919 2919
==========================================
- Hits 7046 7042 -4
- Misses 4694 4698 +4
Partials 182 182
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/9144?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [superset-frontend/src/components/TableSelector.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9144/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvVGFibGVTZWxlY3Rvci5qc3g=) | `78.35% <ø> (ø)` | :arrow_up: |
| [superset-frontend/src/components/Loading.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9144/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvTG9hZGluZy5qc3g=) | `55.55% <0%> (-44.45%)` | :arrow_down: |
| [...frontend/src/dashboard/components/MissingChart.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9144/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL01pc3NpbmdDaGFydC5qc3g=) | `100% <0%> (ø)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9144?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9144?src=pr&el=footer). Last update [931ea55...e5fa701](https://codecov.io/gh/apache/incubator-superset/pull/9144?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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