chore(api v1): Deprecate datasource/save and datasource/get endpoints #23678
Conversation
Codecov Report
@@ Coverage Diff @@
## master #23678 +/- ##
==========================================
+ Coverage 68.08% 68.11% +0.02%
==========================================
Files 1920 1922 +2
Lines 73991 74068 +77
Branches 8092 8101 +9
==========================================
+ Hits 50374 50448 +74
Misses 21548 21548
- Partials 2069 2072 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
jfrag1
requested review from
jinghua-qa,
geido,
eschutho,
rusackas and
betodealmeida
as code owners
| and not is_safe_url(default_endpoint) | ||
| and current_app.config["PREVENT_UNSAFE_DEFAULT_URLS_ON_DATASET"] | ||
| ): | ||
| exceptions.append(DatasetEndpointUnsafeValidationError()) |
There was a problem hiding this comment.
Porting this check over from the datasource/save endpoint
| @@ -67,7 +67,7 @@ export interface Dataset { | |||
| type: DatasourceType; | |||
| columns: ColumnMeta[]; | |||
| metrics: Metric[]; | |||
| column_format: Record<string, string>; | |||
| column_formats: Record<string, string>; | |||
There was a problem hiding this comment.
I believe this was a typo, the backend returns column_formats
| SupersetClient.put({ | ||
| endpoint: `/api/v1/dataset/${currentDatasource.id}`, | ||
| jsonPayload: { | ||
| table_name: currentDatasource.table_name, |
There was a problem hiding this comment.
Why are we not spreading as we were before? same with metric and column below.
There was a problem hiding this 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.
| metrics: currentDatasource?.metrics?.map( | ||
| (metric: Record<string, unknown>) => ({ | ||
| ...metric, | ||
| SupersetClient.put({ |
There was a problem hiding this comment.
Is it possible to save a new dataset in this modal or only update?
There was a problem hiding this comment.
only update, the datasource/save endpoint only supported updating an existing dataset, not creating a new one
…ndpoints (apache#23678)" This reverts commit 44557f5.
…ndpoints (apache#23678)" This reverts commit 44557f5.
mistercrunch
added
🏷️ bot
SUMMARY
Deprecates the
datasource/saveanddatasource/getendpoints. They can be replaced with the existingPUT /api/v1/dataset/{pk}andGET /api/v1/dataset/{pk}endpoints.Several fields that the frontend expects needed to be added to the payload on the
GETendpoint so that explore doesn't break after swapping datasetsBEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION