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/06/17 04:50:20 UTC

[GitHub] [incubator-superset] john-bodley opened a new pull request #10079: fix: Ensuring /queries/ route accepts float or int

john-bodley opened a new pull request #10079:
URL: https://github.com/apache/incubator-superset/pull/10079


   ### SUMMARY
   
   Apologies for the regression which was fixed in https://github.com/apache/incubator-superset/pull/10070. This PR updates the logic to ensure that Unix time provided is either a float or integer. 
   
   ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
   <!--- Skip this if not applicable -->
   
   ### TEST PLAN
   
   CI. 
   
   ### 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
   


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



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

[GitHub] [incubator-superset] john-bodley commented on a change in pull request #10079: fix: Ensuring /queries/ route accepts float or int

Posted by GitBox <gi...@apache.org>.
john-bodley commented on a change in pull request #10079:
URL: https://github.com/apache/incubator-superset/pull/10079#discussion_r441279107



##########
File path: superset/views/core.py
##########
@@ -2658,17 +2658,18 @@ def fetch_datasource_metadata(self) -> FlaskResponse:
         return json_success(json.dumps(datasource.data))
 
     @has_access_api
-    @expose("/queries/<last_updated_ms>")
-    def queries(self, last_updated_ms: str) -> FlaskResponse:
+    @expose("/queries/<float:last_updated_ms>")

Review comment:
       Note we require both routes as `float`  _only_ supports floating point  per [here](https://github.com/pallets/werkzeug/blob/237b35a63631fda0fe80eb51dccdfe8a2f50d304/src/werkzeug/routing.py#L1277) (and the associated regex) numbers.

##########
File path: setup.cfg
##########
@@ -45,7 +45,7 @@ combine_as_imports = true
 include_trailing_comma = true
 line_length = 88
 known_first_party = superset
-known_third_party =alembic,apispec,backoff,bleach,cachelib,celery,click,colorama,contextlib2,croniter,cryptography,dateutil,flask,flask_appbuilder,flask_babel,flask_caching,flask_compress,flask_login,flask_migrate,flask_sqlalchemy,flask_talisman,flask_testing,flask_wtf,geohash,geopy,humanize,isodate,jinja2,markdown,markupsafe,marshmallow,msgpack,numpy,pandas,parsedatetime,pathlib2,polyline,prison,pyarrow,pyhive,pytz,retry,selenium,setuptools,simplejson,sphinx_rtd_theme,sqlalchemy,sqlalchemy_utils,sqlparse,werkzeug,wtforms,wtforms_json,yaml
+known_third_party =alembic,apispec,backoff,bleach,cachelib,celery,click,colorama,contextlib2,croniter,cryptography,dataclasses,dateutil,flask,flask_appbuilder,flask_babel,flask_caching,flask_compress,flask_login,flask_migrate,flask_sqlalchemy,flask_talisman,flask_testing,flask_wtf,geohash,geopy,humanize,isodate,jinja2,markdown,markupsafe,marshmallow,msgpack,numpy,pandas,parsedatetime,pathlib2,polyline,prison,pyarrow,pyhive,pytz,retry,selenium,setuptools,simplejson,sphinx_rtd_theme,sqlalchemy,sqlalchemy_utils,sqlparse,werkzeug,wtforms,wtforms_json,yaml

Review comment:
       The inclusion/exclusion of `dataclasses` depends on whether one is using Python 3.6 or Python 3.7+.

##########
File path: superset/views/core.py
##########
@@ -2658,17 +2658,18 @@ def fetch_datasource_metadata(self) -> FlaskResponse:
         return json_success(json.dumps(datasource.data))
 
     @has_access_api
-    @expose("/queries/<last_updated_ms>")
-    def queries(self, last_updated_ms: str) -> FlaskResponse:
+    @expose("/queries/<float:last_updated_ms>")
+    @expose("/queries/<int:last_updated_ms>")
+    def queries(self, last_updated_ms: Union[float, int]) -> FlaskResponse:
         """
         Get the updated queries.
 
-        :param last_updated_ms: unix time, milliseconds
+        :param last_updated_ms: Unix time (milliseconds)
         """
-        last_updated_ms_int = int(float(last_updated_ms)) if last_updated_ms else 0
-        return self.queries_exec(last_updated_ms_int)
 
-    def queries_exec(self, last_updated_ms: int) -> FlaskResponse:
+        return self.queries_exec(last_updated_ms)
+
+    def queries_exec(self, last_updated_ms: Union[float, int]) -> FlaskResponse:

Review comment:
       There's no need to round to the nearest millisecond since `datetime` object support microseconds.




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



---------------------------------------------------------------------
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 pull request #10079: fix: Ensuring /queries/ route accepts float or int

Posted by GitBox <gi...@apache.org>.
villebro commented on pull request #10079:
URL: https://github.com/apache/incubator-superset/pull/10079#issuecomment-645173996


   I restarted tests, the flaky cypress test is now green.


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



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

[GitHub] [incubator-superset] john-bodley commented on a change in pull request #10079: fix: Ensuring /queries/ route accepts float or int

Posted by GitBox <gi...@apache.org>.
john-bodley commented on a change in pull request #10079:
URL: https://github.com/apache/incubator-superset/pull/10079#discussion_r441280997



##########
File path: superset/views/core.py
##########
@@ -2658,25 +2658,26 @@ def fetch_datasource_metadata(self) -> FlaskResponse:
         return json_success(json.dumps(datasource.data))
 
     @has_access_api
-    @expose("/queries/<last_updated_ms>")
-    def queries(self, last_updated_ms: str) -> FlaskResponse:
+    @expose("/queries/<float:last_updated_ms>")
+    @expose("/queries/<int:last_updated_ms>")
+    def queries(self, last_updated_ms: Union[float, int]) -> FlaskResponse:
         """
         Get the updated queries.
 
-        :param last_updated_ms: unix time, milliseconds
+        :param last_updated_ms: Unix time (milliseconds)
         """
-        last_updated_ms_int = int(float(last_updated_ms)) if last_updated_ms else 0
-        return self.queries_exec(last_updated_ms_int)
 
-    def queries_exec(self, last_updated_ms: int) -> FlaskResponse:
+        return self.queries_exec(last_updated_ms)
+
+    def queries_exec(self, last_updated_ms: Union[float, int]) -> FlaskResponse:
         stats_logger.incr("queries")
         if not g.user.get_id():
             return json_error_response(
                 "Please login to access the queries.", status=403
             )
 
         # UTC date time, same that is stored in the DB.
-        last_updated_dt = utils.EPOCH + timedelta(seconds=last_updated_ms / 1000)
+        last_updated_dt = datetime.utcfromtimestamp(last_updated_ms / 1000)

Review comment:
       Leveraging the native `datetime` logic for converting unix time to a `datetime` object seems clearer/safer than adding a time delta to 1970-01-01.




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



---------------------------------------------------------------------
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 #10079: fix: Ensuring /queries/ route accepts float or int

Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #10079:
URL: https://github.com/apache/incubator-superset/pull/10079#discussion_r441305070



##########
File path: setup.cfg
##########
@@ -45,7 +45,7 @@ combine_as_imports = true
 include_trailing_comma = true
 line_length = 88
 known_first_party = superset
-known_third_party =alembic,apispec,backoff,bleach,cachelib,celery,click,colorama,contextlib2,croniter,cryptography,dateutil,flask,flask_appbuilder,flask_babel,flask_caching,flask_compress,flask_login,flask_migrate,flask_sqlalchemy,flask_talisman,flask_testing,flask_wtf,geohash,geopy,humanize,isodate,jinja2,markdown,markupsafe,marshmallow,msgpack,numpy,pandas,parsedatetime,pathlib2,polyline,prison,pyarrow,pyhive,pytz,retry,selenium,setuptools,simplejson,sphinx_rtd_theme,sqlalchemy,sqlalchemy_utils,sqlparse,werkzeug,wtforms,wtforms_json,yaml
+known_third_party =alembic,apispec,backoff,bleach,cachelib,celery,click,colorama,contextlib2,croniter,cryptography,dataclasses,dateutil,flask,flask_appbuilder,flask_babel,flask_caching,flask_compress,flask_login,flask_migrate,flask_sqlalchemy,flask_talisman,flask_testing,flask_wtf,geohash,geopy,humanize,isodate,jinja2,markdown,markupsafe,marshmallow,msgpack,numpy,pandas,parsedatetime,pathlib2,polyline,prison,pyarrow,pyhive,pytz,retry,selenium,setuptools,simplejson,sphinx_rtd_theme,sqlalchemy,sqlalchemy_utils,sqlparse,werkzeug,wtforms,wtforms_json,yaml

Review comment:
       I've started ignoring these complaints by doing `--no-verify` on commits that want to flick this switch. We should probably standardise on which versions we use for development. I propose sticking to `3.6` until we make a formal decision of switching to `3.7+`.




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



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

[GitHub] [incubator-superset] codecov-commenter commented on pull request #10079: fix: Ensuring /queries/ route accepts float or int

Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #10079:
URL: https://github.com/apache/incubator-superset/pull/10079#issuecomment-645154517


   # [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10079?src=pr&el=h1) Report
   > Merging [#10079](https://codecov.io/gh/apache/incubator-superset/pull/10079?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/e3013ea1299f13f713af34a338a74c3164be6e31&el=desc) will **decrease** coverage by `0.56%`.
   > The diff coverage is `100.00%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-superset/pull/10079/graphs/tree.svg?width=650&height=150&src=pr&token=KsB0fHcx6l)](https://codecov.io/gh/apache/incubator-superset/pull/10079?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #10079      +/-   ##
   ==========================================
   - Coverage   70.57%   70.00%   -0.57%     
   ==========================================
     Files         586      184     -402     
     Lines       31119    18200   -12919     
     Branches     3193        0    -3193     
   ==========================================
   - Hits        21961    12741    -9220     
   + Misses       9049     5459    -3590     
   + Partials      109        0     -109     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | #cypress | `?` | |
   | #javascript | `?` | |
   | #python | `70.00% <100.00%> (-0.08%)` | :arrow_down: |
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/10079?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [superset/views/core.py](https://codecov.io/gh/apache/incubator-superset/pull/10079/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdmlld3MvY29yZS5weQ==) | `76.13% <100.00%> (-0.22%)` | :arrow_down: |
   | [superset/db\_engine\_specs/mysql.py](https://codecov.io/gh/apache/incubator-superset/pull/10079/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL215c3FsLnB5) | `78.26% <0.00%> (-13.05%)` | :arrow_down: |
   | [superset/models/core.py](https://codecov.io/gh/apache/incubator-superset/pull/10079/diff?src=pr&el=tree#diff-c3VwZXJzZXQvbW9kZWxzL2NvcmUucHk=) | `85.42% <0.00%> (-0.88%)` | :arrow_down: |
   | [superset/connectors/sqla/models.py](https://codecov.io/gh/apache/incubator-superset/pull/10079/diff?src=pr&el=tree#diff-c3VwZXJzZXQvY29ubmVjdG9ycy9zcWxhL21vZGVscy5weQ==) | `88.80% <0.00%> (-0.15%)` | :arrow_down: |
   | [...perset-frontend/src/utils/getControlsForVizType.js](https://codecov.io/gh/apache/incubator-superset/pull/10079/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3V0aWxzL2dldENvbnRyb2xzRm9yVml6VHlwZS5qcw==) | | |
   | [superset-frontend/src/reduxUtils.ts](https://codecov.io/gh/apache/incubator-superset/pull/10079/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3JlZHV4VXRpbHMudHM=) | | |
   | [...d/src/dashboard/util/logging/childChartsDidLoad.js](https://codecov.io/gh/apache/incubator-superset/pull/10079/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC91dGlsL2xvZ2dpbmcvY2hpbGRDaGFydHNEaWRMb2FkLmpz) | | |
   | [...-frontend/src/SqlLab/components/HighlightedSql.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10079/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL0hpZ2hsaWdodGVkU3FsLmpzeA==) | | |
   | [superset-frontend/src/setup/setupFormatters.js](https://codecov.io/gh/apache/incubator-superset/pull/10079/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3NldHVwL3NldHVwRm9ybWF0dGVycy5qcw==) | | |
   | ... and [397 more](https://codecov.io/gh/apache/incubator-superset/pull/10079/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10079?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/10079?src=pr&el=footer). Last update [e3013ea...28dbbee](https://codecov.io/gh/apache/incubator-superset/pull/10079?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



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

[GitHub] [incubator-superset] john-bodley merged pull request #10079: fix: Ensuring /queries/ route accepts float or int

Posted by GitBox <gi...@apache.org>.
john-bodley merged pull request #10079:
URL: https://github.com/apache/incubator-superset/pull/10079


   


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



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

[GitHub] [incubator-superset] codecov-commenter edited a comment on pull request #10079: fix: Ensuring /queries/ route accepts float or int

Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #10079:
URL: https://github.com/apache/incubator-superset/pull/10079#issuecomment-645154517






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



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