Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

chore(api v1): Deprecate datasource/save and datasource/get endpoints #23678

Merged
merged 21 commits into from
Apr 19, 2023
Merged

chore(api v1): Deprecate datasource/save and datasource/get endpoints #23678

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
29d5be4
First draft
jfrag1 Apr 13, 2023
a67be31
Formatting
jfrag1 Apr 13, 2023
e0c0105
Merge branch 'master' into jack/migrate-datasource-apis-to-v1
jfrag1 Apr 13, 2023
99a0873
Lint
jfrag1 Apr 13, 2023
81e52de
rm debugging code
jfrag1 Apr 13, 2023
5ab4fc1
Fix mypy
jfrag1 Apr 13, 2023
6da1bb9
Fixes
jfrag1 Apr 13, 2023
37f53b2
Fix metric adding
jfrag1 Apr 13, 2023
1ffdc96
Add new fields to dataset api test
jfrag1 Apr 13, 2023
1002ce6
Formatting
jfrag1 Apr 13, 2023
83cf76c
rm debugging code
jfrag1 Apr 13, 2023
8c37492
Fix test
jfrag1 Apr 13, 2023
a372dba
Fix lint
jfrag1 Apr 13, 2023
1dbc15d
Fix test for mysql
jfrag1 Apr 13, 2023
bbe1ab8
Fix test for mysql again
jfrag1 Apr 13, 2023
8470eac
Fix frontend unit tests
jfrag1 Apr 13, 2023
d37b126
Attempt to fix failing cypress test
jfrag1 Apr 13, 2023
d504856
Fetch from get endpoint after update to get all needed fields
jfrag1 Apr 13, 2023
ce23c54
Update frontend tests to account for put then get
jfrag1 Apr 13, 2023
6140fb1
Oops
jfrag1 Apr 13, 2023
389ce01
Break up the mock interference
jfrag1 Apr 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion superset-frontend/packages/superset-ui-chart-controls/src/fixtures.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import { DatasourceType } from '@superset-ui/core';
import { Dataset } from './types';

export const TestDataset: Dataset = {
column_format: {},
column_formats: {},
columns: [
{
advanced_data_type: undefined,
Expand Down
2 changes: 1 addition & 1 deletion superset-frontend/packages/superset-ui-chart-controls/src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ export interface Dataset {
type: DatasourceType;
columns: ColumnMeta[];
metrics: Metric[];
column_format: Record<string, string>;
column_formats: Record<string, string>;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this was a typo, the backend returns column_formats

verbose_map: Record<string, string>;
main_dttm_col: string;
// eg. ['["ds", true]', 'ds [asc]']
Expand Down
2 changes: 1 addition & 1 deletion superset-frontend/packages/superset-ui-chart-controls/test/utils/columnChoices.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ describe('columnChoices()', () => {
},
],
verbose_map: {},
column_format: { fiz: 'NUMERIC', about: 'STRING', foo: 'DATE' },
column_formats: { fiz: 'NUMERIC', about: 'STRING', foo: 'DATE' },
datasource_name: 'my_datasource',
description: 'this is my datasource',
}),
Expand Down
2 changes: 1 addition & 1 deletion superset-frontend/packages/superset-ui-chart-controls/test/utils/defineSavedMetrics.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ describe('defineSavedMetrics', () => {
time_grain_sqla: 'P1D',
columns: [],
verbose_map: {},
column_format: {},
column_formats: {},
datasource_name: 'my_datasource',
description: 'this is my datasource',
};
Expand Down
4 changes: 2 additions & 2 deletions superset-frontend/src/components/Datasource/ChangeDatasourceModal.test.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ const datasourceData = {

const DATASOURCES_ENDPOINT =
'glob:*/api/v1/dataset/?q=(order_column:changed_on_delta_humanized,order_direction:desc,page:0,page_size:25)';
const DATASOURCE_ENDPOINT = `glob:*/datasource/get/${datasourceData.type}/${datasourceData.id}`;
const DATASOURCE_ENDPOINT = `glob:*/api/v1/dataset/${datasourceData.id}`;
const DATASOURCE_PAYLOAD = { new: 'data' };

const INFO_ENDPOINT = 'glob:*/api/v1/dataset/_info?*';
Expand Down Expand Up @@ -112,6 +112,6 @@ describe('ChangeDatasourceModal', () => {
});
await waitForComponentToPaint(wrapper);

expect(fetchMock.calls(/datasource\/get\/table\/7/)).toHaveLength(1);
expect(fetchMock.calls(/api\/v1\/dataset\/7/)).toHaveLength(1);
});
});
6 changes: 4 additions & 2 deletions superset-frontend/src/components/Datasource/ChangeDatasourceModal.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,10 +173,12 @@ const ChangeDatasourceModal: FunctionComponent<ChangeDatasourceModalProps> = ({

const handleChangeConfirm = () => {
SupersetClient.get({
endpoint: `/datasource/get/${confirmedDataset?.type}/${confirmedDataset?.id}/`,
endpoint: `/api/v1/dataset/${confirmedDataset?.id}`,
})
.then(({ json }) => {
onDatasourceSave(json);
// eslint-disable-next-line no-param-reassign
json.result.type = 'table';
onDatasourceSave(json.result);
onChange(`${confirmedDataset?.id}__table`);
})
.catch(response => {
Expand Down
12 changes: 9 additions & 3 deletions superset-frontend/src/components/Datasource/DatasourceModal.test.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,8 @@ const datasource = mockDatasource['7__table'];

const SAVE_ENDPOINT = 'glob:*/api/v1/dataset/7';
const SAVE_PAYLOAD = { new: 'data' };
const SAVE_DATASOURCE_ENDPOINT = 'glob:*/datasource/save/';
const SAVE_DATASOURCE_ENDPOINT = 'glob:*/api/v1/dataset/7';
const GET_DATASOURCE_ENDPOINT = SAVE_DATASOURCE_ENDPOINT;

const mockedProps = {
datasource,
Expand Down Expand Up @@ -96,7 +97,8 @@ describe('DatasourceModal', () => {

it('saves on confirm', async () => {
const callsP = fetchMock.post(SAVE_ENDPOINT, SAVE_PAYLOAD);
fetchMock.post(SAVE_DATASOURCE_ENDPOINT, {});
fetchMock.put(SAVE_DATASOURCE_ENDPOINT, {});
fetchMock.get(GET_DATASOURCE_ENDPOINT, {});
act(() => {
wrapper
.find('button[data-test="datasource-modal-save"]')
Expand All @@ -111,7 +113,11 @@ describe('DatasourceModal', () => {
okButton.simulate('click');
});
await waitForComponentToPaint(wrapper);
const expected = ['http://localhost/datasource/save/'];
// one call to PUT, then one to GET
const expected = [
'http://localhost/api/v1/dataset/7',
'http://localhost/api/v1/dataset/7',
];
expect(callsP._calls.map(call => call[0])).toEqual(
expected,
); /* eslint no-underscore-dangle: 0 */
Expand Down
98 changes: 70 additions & 28 deletions superset-frontend/src/components/Datasource/DatasourceModal.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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({
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, perfect.

endpoint: `/api/v1/dataset/${currentDatasource.id}`,
jsonPayload: {
table_name: currentDatasource.table_name,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

database_id: currentDatasource.database?.id,
sql: currentDatasource.sql,
filter_select_enabled: currentDatasource.filter_select_enabled,
fetch_values_predicate: currentDatasource.fetch_values_predicate,
schema,
description: currentDatasource.description,
main_dttm_col: currentDatasource.main_dttm_col,
offset: currentDatasource.offset,
default_endpoint: currentDatasource.default_endpoint,
cache_timeout:
currentDatasource.cache_timeout === ''
? null
: currentDatasource.cache_timeout,
is_sqllab_view: currentDatasource.is_sqllab_view,
template_params: currentDatasource.template_params,
extra: currentDatasource.extra,
is_managed_externally: currentDatasource.is_managed_externally,
external_url: currentDatasource.external_url,
metrics: currentDatasource?.metrics?.map(
(metric: Record<string, unknown>) => {
const metricBody: any = {
expression: metric.expression,
description: metric.description,
metric_name: metric.metric_name,
metric_type: metric.metric_type,
d3format: metric.d3format,
verbose_name: metric.verbose_name,
warning_text: metric.warning_text,
uuid: metric.uuid,
extra: buildExtraJsonObject(metric),
}),
),
columns: currentDatasource?.columns?.map(
(column: Record<string, unknown>) => ({
...column,
extra: buildExtraJsonObject(column),
}),
),
type: currentDatasource.type || currentDatasource.datasource_type,
owners: currentDatasource.owners.map(
(o: Record<string, number>) => o.value || o.id,
),
},
};
if (!Number.isNaN(Number(metric.id))) {
metricBody.id = metric.id;
}
return metricBody;
},
),
columns: currentDatasource?.columns?.map(
(column: Record<string, unknown>) => ({
id: column.id,
column_name: column.column_name,
type: column.type,
advanced_data_type: column.advanced_data_type,
verbose_name: column.verbose_name,
description: column.description,
expression: column.expression,
filterable: column.filterable,
groupby: column.groupby,
is_active: column.is_active,
is_dttm: column.is_dttm,
python_date_format: column.python_date_format,
uuid: column.uuid,
extra: buildExtraJsonObject(column),
}),
),
owners: currentDatasource.owners.map(
(o: Record<string, number>) => o.value || o.id,
),
},
})
.then(({ json }) => {
.then(() => {
addSuccessToast(t('The dataset has been saved'));
return SupersetClient.get({
endpoint: `/api/v1/dataset/${currentDatasource?.id}`,
});
})
.then(({ json }) => {
// eslint-disable-next-line no-param-reassign
json.result.type = 'table';
onDatasourceSave({
...json,
...json.result,
owners: currentDatasource.owners,
});
onHide();
Expand Down
2 changes: 1 addition & 1 deletion superset-frontend/src/dashboard/constants.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ export const PLACEHOLDER_DATASOURCE: Datasource = {
columns: [],
column_types: [],
metrics: [],
column_format: {},
column_formats: {},
verbose_map: {},
main_dttm_col: '',
description: '',
Expand Down
4 changes: 2 additions & 2 deletions superset-frontend/src/explore/actions/datasourcesActions.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ const CURRENT_DATASOURCE = {
type: DatasourceType.Table,
columns: [],
metrics: [],
column_format: {},
column_formats: {},
verbose_map: {},
main_dttm_col: '__timestamp',
// eg. ['["ds", true]', 'ds [asc]']
Expand All @@ -47,7 +47,7 @@ const NEW_DATASOURCE = {
type: DatasourceType.Table,
columns: [],
metrics: [],
column_format: {},
column_formats: {},
verbose_map: {},
main_dttm_col: '__timestamp',
// eg. ['["ds", true]', 'ds [asc]']
Expand Down
23 changes: 17 additions & 6 deletions ...set-frontend/src/explore/components/controls/DatasourceControl/DatasourceControl.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,9 +69,20 @@ const createProps = (overrides: JsonObject = {}) => ({
});

async function openAndSaveChanges(datasource: any) {
fetchMock.post('glob:*/datasource/save/', datasource, {
overwriteRoutes: true,
});
fetchMock.put(
'glob:*/api/v1/dataset/*',
{},
{
overwriteRoutes: true,
},
);
fetchMock.get(
'glob:*/api/v1/dataset/*',
{ result: datasource },
{
overwriteRoutes: true,
},
);
userEvent.click(screen.getByTestId('datasource-menu-trigger'));
userEvent.click(await screen.findByTestId('edit-dataset'));
userEvent.click(await screen.findByTestId('datasource-modal-save'));
Expand Down Expand Up @@ -154,7 +165,7 @@ test('Should show SQL Lab for sql_lab role', async () => {

test('Click on Swap dataset option', async () => {
const props = createProps();
SupersetClientGet.mockImplementation(
SupersetClientGet.mockImplementationOnce(
async ({ endpoint }: { endpoint: string }) => {
if (endpoint.includes('_info')) {
return {
Expand Down Expand Up @@ -182,7 +193,7 @@ test('Click on Swap dataset option', async () => {

test('Click on Edit dataset', async () => {
const props = createProps();
SupersetClientGet.mockImplementation(
SupersetClientGet.mockImplementationOnce(
async () => ({ json: { result: [] } } as any),
);
render(<DatasourceControl {...props} />, {
Expand All @@ -206,7 +217,7 @@ test('Edit dataset should be disabled when user is not admin', async () => {
// @ts-expect-error
props.user.roles = {};
props.datasource.owners = [];
SupersetClientGet.mockImplementation(
SupersetClientGet.mockImplementationOnce(
async () => ({ json: { result: [] } } as any),
);

Expand Down
2 changes: 1 addition & 1 deletion superset-frontend/src/explore/controlUtils/controlUtils.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ describe('controlUtils', () => {
type: DatasourceType.Table,
columns: [{ column_name: 'a' }],
metrics: [{ metric_name: 'first' }, { metric_name: 'second' }],
column_format: {},
column_formats: {},
verbose_map: {},
main_dttm_col: '',
datasource_name: '1__table',
Expand Down
2 changes: 1 addition & 1 deletion superset-frontend/src/explore/controlUtils/getControlValuesCompatibleWithDatasource.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ const sampleDatasource: Dataset = {
{ column_name: 'sample_column_4' },
],
metrics: [{ metric_name: 'saved_metric_2' }],
column_format: {},
column_formats: {},
verbose_map: {},
main_dttm_col: '',
datasource_name: 'Sample Dataset',
Expand Down
4 changes: 2 additions & 2 deletions superset-frontend/src/explore/fixtures.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@ export const exploreInitialData: ExplorePageInitialData = {
type: DatasourceType.Table,
columns: [{ column_name: 'a' }],
metrics: [{ metric_name: 'first' }, { metric_name: 'second' }],
column_format: {},
column_formats: {},
verbose_map: {},
main_dttm_col: '',
datasource_name: '8__table',
Expand All @@ -153,7 +153,7 @@ export const fallbackExploreInitialData: ExplorePageInitialData = {
type: DatasourceType.Table,
columns: [],
metrics: [],
column_format: {},
column_formats: {},
verbose_map: {},
main_dttm_col: '',
owners: [],
Expand Down
2 changes: 1 addition & 1 deletion superset-frontend/src/utils/getDatasourceUid.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ const TEST_DATASOURCE = {
type: DatasourceType.Table,
columns: [],
metrics: [],
column_format: {},
column_formats: {},
verbose_map: {},
main_dttm_col: '__timestamp',
// eg. ['["ds", true]', 'ds [asc]']
Expand Down
Loading
Loading