You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "jfrag1 (via GitHub)" <gi...@apache.org> on 2023/04/13 12:39:41 UTC

[GitHub] [superset] jfrag1 opened a new pull request, #23678: chore(api v1): Deprecate datasource/save and datasource/get endpoints

jfrag1 opened a new pull request, #23678:
URL: https://github.com/apache/superset/pull/23678

   <!---
   Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
   Example:
   fix(dashboard): load charts correctly
   -->
   
   ### SUMMARY
   <!--- Describe the change below, including rationale and design decisions -->
   Deprecates the `datasource/save` and `datasource/get` endpoints.  They can be replaced with the existing `PUT /api/v1/dataset/{pk}` and `GET /api/v1/dataset/{pk}` endpoints.
   
   Several fields that the frontend expects needed to be added to the payload on the `GET` endpoint so that explore doesn't break after swapping datasets
   
   ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
   <!--- Skip this if not applicable -->
   
   ### TESTING INSTRUCTIONS
   <!--- Required! What steps can be taken to manually 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:
   - [ ] Required feature flags:
   - [ ] Changes UI
   - [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
     - [ ] Migration is atomic, supports rollback & is backwards-compatible
     - [ ] Confirm DB migration upgrade and downgrade tested
     - [ ] Runtime estimates and downtime expectations provided
   - [ ] 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.

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

[GitHub] [superset] jfrag1 commented on a diff in pull request #23678: chore(api v1): Deprecate datasource/save and datasource/get endpoints

Posted by "jfrag1 (via GitHub)" <gi...@apache.org>.
jfrag1 commented on code in PR #23678:
URL: https://github.com/apache/superset/pull/23678#discussion_r1166038067


##########
superset/datasets/commands/update.py:
##########
@@ -101,6 +104,14 @@ def validate(self) -> None:
             self._properties["owners"] = owners
         except ValidationError as ex:
             exceptions.append(ex)
+        # Validate default URL safety
+        default_endpoint = self._properties.get("default_endpoint")
+        if (
+            default_endpoint
+            and not is_safe_url(default_endpoint)
+            and current_app.config["PREVENT_UNSAFE_DEFAULT_URLS_ON_DATASET"]
+        ):
+            exceptions.append(DatasetEndpointUnsafeValidationError())

Review Comment:
   Porting this check over from the datasource/save endpoint



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

[GitHub] [superset] jfrag1 commented on a diff in pull request #23678: chore(api v1): Deprecate datasource/save and datasource/get endpoints

Posted by "jfrag1 (via GitHub)" <gi...@apache.org>.
jfrag1 commented on code in PR #23678:
URL: https://github.com/apache/superset/pull/23678#discussion_r1170464527


##########
superset-frontend/src/components/Datasource/DatasourceModal.tsx:
##########
@@ -96,39 +96,81 @@ const DatasourceModal: FunctionComponent<DatasourceModalProps> = ({
       currentDatasource.schema;
 
     setIsSaving(true);
-    SupersetClient.post({
-      endpoint: '/datasource/save/',
-      postPayload: {
-        data: {
-          ...currentDatasource,
-          cache_timeout:
-            currentDatasource.cache_timeout === ''
-              ? null
-              : currentDatasource.cache_timeout,
-          schema,
-          metrics: currentDatasource?.metrics?.map(
-            (metric: Record<string, unknown>) => ({
-              ...metric,
+    SupersetClient.put({

Review Comment:
   only update, the `datasource/save` endpoint only supported updating an existing dataset, not creating a new one



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

[GitHub] [superset] jfrag1 commented on a diff in pull request #23678: chore(api v1): Deprecate datasource/save and datasource/get endpoints

Posted by "jfrag1 (via GitHub)" <gi...@apache.org>.
jfrag1 commented on code in PR #23678:
URL: https://github.com/apache/superset/pull/23678#discussion_r1167156725


##########
superset-frontend/src/components/Datasource/DatasourceModal.tsx:
##########
@@ -96,39 +96,81 @@ const DatasourceModal: FunctionComponent<DatasourceModalProps> = ({
       currentDatasource.schema;
 
     setIsSaving(true);
-    SupersetClient.post({
-      endpoint: '/datasource/save/',
-      postPayload: {
-        data: {
-          ...currentDatasource,
-          cache_timeout:
-            currentDatasource.cache_timeout === ''
-              ? null
-              : currentDatasource.cache_timeout,
-          schema,
-          metrics: currentDatasource?.metrics?.map(
-            (metric: Record<string, unknown>) => ({
-              ...metric,
+    SupersetClient.put({
+      endpoint: `/api/v1/dataset/${currentDatasource.id}`,
+      jsonPayload: {
+        table_name: currentDatasource.table_name,

Review Comment:
   The old endpoint didn't have a rigid structure it accepted as a payload; it would just take a JSON string, parse it, and then set all keys that it was able to set on the target dataset, ignoring irrelevant keys.
   
   The new endpoint is different; it accepts the payload defined by the `DatasetPutSchema`, which only includes the relevant keys that can be updated. If the payload contains any other keys, the endpoint 400's.  The datasource object here contains several of these other irrelevant keys, so now we have to extract only the ones that are needed.



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

[GitHub] [superset] eschutho commented on a diff in pull request #23678: chore(api v1): Deprecate datasource/save and datasource/get endpoints

Posted by "eschutho (via GitHub)" <gi...@apache.org>.
eschutho commented on code in PR #23678:
URL: https://github.com/apache/superset/pull/23678#discussion_r1170509795


##########
superset-frontend/src/components/Datasource/DatasourceModal.tsx:
##########
@@ -96,39 +96,81 @@ const DatasourceModal: FunctionComponent<DatasourceModalProps> = ({
       currentDatasource.schema;
 
     setIsSaving(true);
-    SupersetClient.post({
-      endpoint: '/datasource/save/',
-      postPayload: {
-        data: {
-          ...currentDatasource,
-          cache_timeout:
-            currentDatasource.cache_timeout === ''
-              ? null
-              : currentDatasource.cache_timeout,
-          schema,
-          metrics: currentDatasource?.metrics?.map(
-            (metric: Record<string, unknown>) => ({
-              ...metric,
+    SupersetClient.put({

Review Comment:
   Ok, perfect. 



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

[GitHub] [superset] jfrag1 commented on a diff in pull request #23678: chore(api v1): Deprecate datasource/save and datasource/get endpoints

Posted by "jfrag1 (via GitHub)" <gi...@apache.org>.
jfrag1 commented on code in PR #23678:
URL: https://github.com/apache/superset/pull/23678#discussion_r1166042128


##########
superset-frontend/packages/superset-ui-chart-controls/src/types.ts:
##########
@@ -67,7 +67,7 @@ export interface Dataset {
   type: DatasourceType;
   columns: ColumnMeta[];
   metrics: Metric[];
-  column_format: Record<string, string>;
+  column_formats: Record<string, string>;

Review Comment:
   I believe this was a typo, the backend returns `column_formats`



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

[GitHub] [superset] codecov[bot] commented on pull request #23678: chore(api v1): Deprecate datasource/save and datasource/get endpoints

Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #23678:
URL: https://github.com/apache/superset/pull/23678#issuecomment-1506907667

   ## [Codecov](https://codecov.io/gh/apache/superset/pull/23678?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#23678](https://codecov.io/gh/apache/superset/pull/23678?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (40d856a) into [master](https://codecov.io/gh/apache/superset/commit/0974fa1172564126c646c1e3d65e640a1b9bcbcd?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (0974fa1) will **decrease** coverage by `11.42%`.
   > The diff coverage is `87.50%`.
   
   > :exclamation: Current head 40d856a differs from pull request most recent head 99a0873. Consider uploading reports for the commit 99a0873 to get more accurate results
   
   ```diff
   @@             Coverage Diff             @@
   ##           master   #23678       +/-   ##
   ===========================================
   - Coverage   68.08%   56.67%   -11.42%     
   ===========================================
     Files        1920     1920               
     Lines       73990    74005       +15     
     Branches     8092     8090        -2     
   ===========================================
   - Hits        50379    41940     -8439     
   - Misses      21540    29996     +8456     
   + Partials     2071     2069        -2     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | hive | `?` | |
   | mysql | `?` | |
   | postgres | `?` | |
   | presto | `53.11% <87.50%> (+0.01%)` | :arrow_up: |
   | python | `59.43% <87.50%> (-23.72%)` | :arrow_down: |
   | sqlite | `?` | |
   | unit | `53.04% <81.25%> (+0.01%)` | :arrow_up: |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/superset/pull/23678?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [...ackages/superset-ui-chart-controls/src/fixtures.ts](https://codecov.io/gh/apache/superset/pull/23678?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY2hhcnQtY29udHJvbHMvc3JjL2ZpeHR1cmVzLnRz) | `100.00% <ø> (ø)` | |
   | [...d/packages/superset-ui-chart-controls/src/types.ts](https://codecov.io/gh/apache/superset/pull/23678?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY2hhcnQtY29udHJvbHMvc3JjL3R5cGVzLnRz) | `100.00% <ø> (ø)` | |
   | [...rc/components/Datasource/ChangeDatasourceModal.tsx](https://codecov.io/gh/apache/superset/pull/23678?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvRGF0YXNvdXJjZS9DaGFuZ2VEYXRhc291cmNlTW9kYWwudHN4) | `80.00% <ø> (+1.31%)` | :arrow_up: |
   | [...tend/src/components/Datasource/DatasourceModal.tsx](https://codecov.io/gh/apache/superset/pull/23678?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvRGF0YXNvdXJjZS9EYXRhc291cmNlTW9kYWwudHN4) | `68.42% <ø> (+0.23%)` | :arrow_up: |
   | [superset-frontend/src/dashboard/constants.ts](https://codecov.io/gh/apache/superset/pull/23678?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb25zdGFudHMudHM=) | `100.00% <ø> (ø)` | |
   | [...re/components/controls/DatasourceControl/index.jsx](https://codecov.io/gh/apache/superset/pull/23678?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9jb250cm9scy9EYXRhc291cmNlQ29udHJvbC9pbmRleC5qc3g=) | `91.50% <ø> (ø)` | |
   | [superset-frontend/src/explore/fixtures.tsx](https://codecov.io/gh/apache/superset/pull/23678?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvZml4dHVyZXMudHN4) | `75.00% <ø> (ø)` | |
   | [superset/datasets/api.py](https://codecov.io/gh/apache/superset/pull/23678?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGF0YXNldHMvYXBpLnB5) | `46.90% <ø> (-41.10%)` | :arrow_down: |
   | [superset/datasets/commands/update.py](https://codecov.io/gh/apache/superset/pull/23678?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGF0YXNldHMvY29tbWFuZHMvdXBkYXRlLnB5) | `25.84% <40.00%> (-68.21%)` | :arrow_down: |
   | [superset/datasets/commands/exceptions.py](https://codecov.io/gh/apache/superset/pull/23678?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGF0YXNldHMvY29tbWFuZHMvZXhjZXB0aW9ucy5weQ==) | `80.88% <66.66%> (-12.97%)` | :arrow_down: |
   | ... and [3 more](https://codecov.io/gh/apache/superset/pull/23678?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   ... and [296 files with indirect coverage changes](https://codecov.io/gh/apache/superset/pull/23678/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   :mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   


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

[GitHub] [superset] eschutho commented on a diff in pull request #23678: chore(api v1): Deprecate datasource/save and datasource/get endpoints

Posted by "eschutho (via GitHub)" <gi...@apache.org>.
eschutho commented on code in PR #23678:
URL: https://github.com/apache/superset/pull/23678#discussion_r1170457774


##########
superset-frontend/src/components/Datasource/DatasourceModal.tsx:
##########
@@ -96,39 +96,81 @@ const DatasourceModal: FunctionComponent<DatasourceModalProps> = ({
       currentDatasource.schema;
 
     setIsSaving(true);
-    SupersetClient.post({
-      endpoint: '/datasource/save/',
-      postPayload: {
-        data: {
-          ...currentDatasource,
-          cache_timeout:
-            currentDatasource.cache_timeout === ''
-              ? null
-              : currentDatasource.cache_timeout,
-          schema,
-          metrics: currentDatasource?.metrics?.map(
-            (metric: Record<string, unknown>) => ({
-              ...metric,
+    SupersetClient.put({

Review Comment:
   Is it possible to save a new dataset in this modal or only update?



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

[GitHub] [superset] jfrag1 closed pull request #23678: chore(api v1): Deprecate datasource/save and datasource/get endpoints

Posted by "jfrag1 (via GitHub)" <gi...@apache.org>.
jfrag1 closed pull request #23678: chore(api v1): Deprecate datasource/save and datasource/get endpoints
URL: https://github.com/apache/superset/pull/23678


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

[GitHub] [superset] jfrag1 closed pull request #23678: chore(api v1): Deprecate datasource/save and datasource/get endpoints

Posted by "jfrag1 (via GitHub)" <gi...@apache.org>.
jfrag1 closed pull request #23678: chore(api v1): Deprecate datasource/save and datasource/get endpoints
URL: https://github.com/apache/superset/pull/23678


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

[GitHub] [superset] Antonio-RiveroMartnez commented on a diff in pull request #23678: chore(api v1): Deprecate datasource/save and datasource/get endpoints

Posted by "Antonio-RiveroMartnez (via GitHub)" <gi...@apache.org>.
Antonio-RiveroMartnez commented on code in PR #23678:
URL: https://github.com/apache/superset/pull/23678#discussion_r1167145713


##########
superset-frontend/src/components/Datasource/DatasourceModal.tsx:
##########
@@ -96,39 +96,81 @@ const DatasourceModal: FunctionComponent<DatasourceModalProps> = ({
       currentDatasource.schema;
 
     setIsSaving(true);
-    SupersetClient.post({
-      endpoint: '/datasource/save/',
-      postPayload: {
-        data: {
-          ...currentDatasource,
-          cache_timeout:
-            currentDatasource.cache_timeout === ''
-              ? null
-              : currentDatasource.cache_timeout,
-          schema,
-          metrics: currentDatasource?.metrics?.map(
-            (metric: Record<string, unknown>) => ({
-              ...metric,
+    SupersetClient.put({
+      endpoint: `/api/v1/dataset/${currentDatasource.id}`,
+      jsonPayload: {
+        table_name: currentDatasource.table_name,

Review Comment:
   Why are we not spreading as we were before? same with metric and column below.



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

[GitHub] [superset] eschutho merged pull request #23678: chore(api v1): Deprecate datasource/save and datasource/get endpoints

Posted by "eschutho (via GitHub)" <gi...@apache.org>.
eschutho merged PR #23678:
URL: https://github.com/apache/superset/pull/23678


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